Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

multi: Implement DCP0010 subsidy consensus vote. #2848

Merged
merged 4 commits into from Dec 17, 2021

Conversation

davecgh
Copy link
Member

@davecgh davecgh commented Dec 13, 2021

This requires #2847.

This implements the agenda for voting on the modified subsidy split defined in DCP0010 along with consensus tests.

In particular, once the vote has passed and is active, the PoW subsidy will be 10% of the block reward and the PoS subsidy will be 80%. The Treasury subsidy will remain at 10%.

In terms of the overall effects, this includes updates to:

  • The validation logic for votes, coinbases, and overall block subsidy
  • Enforcement when considering candidate votes for acceptance to the mempool, relaying, and inclusion into block templates
  • Mining template generation
  • The output of the getblocksubsidy RPC

Also note that this does not implement the block version bump that will ultimately be needed by the mining code since there are multiple consensus votes gated behind it and will therefore be done separately.

The following is an overview of the changes:

  • Introduce a convenience function for determining if the vote passed and is now active
  • Add new methods to blockchain/standalone for calculating the work and stake vote subsidies according to either the original or modified values per a provided flag
  • Modify vote validation to enforce the new stake vote subsidy in accordance with the state of the vote
  • Modify coinbase validation to enforce the new work subsidy in accordance with the state of the vote
  • Modify block validation logic to enforce the total overall subsidy in accordance with the state of the vote
  • Add tests for determining if the agenda is active for both mainnet and testnet
  • Add tests for getblocksubsidy RPC
  • Add tests to ensure proper behavior for the modified subsidy splits as follows:
    • Ensure new blockchain validation semantics are enforced once the agenda is active
    • Ensure mempool correctly accepts and rejects votes in accordance with the state of the vote

@davecgh davecgh added this to the 1.7.0 milestone Dec 13, 2021
@davecgh davecgh force-pushed the multi_dcp0010_subsidy_split branch 3 times, most recently from db87955 to b249403 Compare December 13, 2021 21:05
@davecgh davecgh added the consensus changes Changes that involve modifying the consensus rules and thus are required to be gated behind a vote. label Dec 13, 2021
@davecgh davecgh force-pushed the multi_dcp0010_subsidy_split branch 2 times, most recently from 999c545 to 923bc6a Compare December 14, 2021 06:23
@davecgh davecgh changed the title WIP: multi: Implement DCP0010 subsidy consensus vote. multi: Implement DCP0010 subsidy consensus vote. Dec 14, 2021
@davecgh davecgh marked this pull request as ready for review December 14, 2021 06:24
blockchain/standalone/subsidy.go Show resolved Hide resolved
blockchain/standalone/subsidy_test.go Outdated Show resolved Hide resolved
blockchain/standalone/subsidy_test.go Outdated Show resolved Hide resolved
}

// Ensure the total calculated subsidy is the expected value.
const expectedTotalSubsidy = 2100000000015952
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the net result is an increase in total issuance of 215k atoms.

Copy link
Member Author

@davecgh davecgh Dec 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Estimated. Yes. The reason is that fewer atoms are lost due to truncation.

Copy link
Member

@rstaudt2 rstaudt2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to do more testing and another thorough review, but so far, everything is looking good to me.

blockchain/agendas_test.go Outdated Show resolved Hide resolved
blockchain/standalone/subsidy.go Show resolved Hide resolved
// Subsidy aligns with the height being voting on, not with the
// height of the current block.
voteSubsidy := b.subsidyCache.CalcStakeVoteSubsidyV2(node.height-1,
isSubsidySplitEnabled)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I double-checked and confirmed that there are no other locations using the CalcWorkSubsidy or CalcStakeVoteSubsidy versions of these functions (other than tests and internally to the subsidy code).

internal/mining/mining.go Outdated Show resolved Hide resolved
Copy link
Member

@dnldd dnldd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will run some simnet tests later, looking good overall.

@dnldd
Copy link
Member

dnldd commented Dec 15, 2021

Can't mine past SVH on simnet so far. Getting this error from the wallet.

2021-12-15 08:59:00.823 [ERR] WLLT: Failed to send one or more votes: dcrd.PublishTransactions: rejected transaction ef0e9658ef997c1ec41c2781a52281b45124f038a1be325dbdc6795bbb70fb90: vote subsidy input value of 2970297029 is not 7920792079

@davecgh
Copy link
Member Author

davecgh commented Dec 15, 2021

Can't mine past SVH on simnet so far. Getting this error from the wallet.

Yes, that is expected because the agenda is active and your wallet isn't updated to calculate the new required vote subsidy. You'll either have to wait for wallet to be updated or just modify it yourself to call the new methods with the agenda active.

@dnldd
Copy link
Member

dnldd commented Dec 15, 2021

Thought as much, thanks. Will update the wallet.

@davecgh
Copy link
Member Author

davecgh commented Dec 15, 2021

I updated the first commit to include a few more tests so it includes all of the test vectors that are in the DCP.

blockchain/standalone/subsidy_test.go Outdated Show resolved Hide resolved
Comment on lines +893 to +895
deploymentVer, ok := b.deploymentVers[deploymentID]
if !ok {
return true, nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like boilerplate from the other methods, but I'm not clear what this implies. Not found means it was not in chaincfg.Params.Deployments, which means:

  1. "voting is not enabled for this network" as described in isLNFeaturesAgendaActive. Like simnet where it is always active?
  2. This deployment is ancient history, removed from Params.Deployments, and thus passed? Presumably the plan is if an agenda fails then all the related code that would look for it would also be removed if it is also removed from Params.Deployments.

Copy link
Member Author

@davecgh davecgh Dec 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes on number 1. For number 2, in the case of failure, yes, that's correct. In the case of success though, the idea is that the code would be changed so that the historical activation points for mainnet/testnet are added and then these methods would turn into "if that historical activation point is an ancestor of a given block, it's active, otherwise, it's not".

EDIT: I should also note that Matheus and I have discussed previously that it would probably be nicer to add a flag to the deployments for specifying whether or not it should always be considered active instead of the current approach of "active if voting is not enabled for this network due to lack of a definition for the deployment". I am planning to do that at some point.

This adds two new exported functions to the subsidy cache in
blockchain/standalone to calculate calculate the work and stake vote
subsidies according to either the original or modified values defined in
DCP0010 per a provided flag along with tests to ensure expected
behavior.

It should be noted that the values for the modified split are hard coded
as opposed to using the subsidy in order to avoid the need for a major
module bump that would be required if the subsidy params interface were
changed.  The values are the same for all networks, so no additional
logic is necessary on a per-network basis.
This adds a new munger to the test chain generator named
ReplaceVoteSubsidies which modifies a block by replacing the subsidy of
all votes contained in the block to a given value.
@davecgh davecgh force-pushed the multi_dcp0010_subsidy_split branch 2 times, most recently from a858c16 to ec0736c Compare December 16, 2021 01:31
@davecgh
Copy link
Member Author

davecgh commented Dec 16, 2021

Updated to remove the replace for chaincfg/v3 in favor of chaincfg/v3.1.1 now that is has been tagged.

Copy link
Member

@rstaudt2 rstaudt2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to giving this another thorough review, I also tested the following scenarios in simnet (with dcrd updated to activate the agenda at a particular height and dcrwallet updated to use CalcStakeVoteSubsidyV2):

  • When the agenda is NOT active
    • Votes are accepted with the original vote subsidy
    • Votes are NOT accepted with the new vote subsidy (and therefore, blocks can't be mined)
    • Blocks are NOT accepted with the new work subsidy (modified the mining code to test)
    • Blocks are accepted with the original vote and work subsidy
  • When the agenda is active
    • Votes are NOT accepted with the original vote subsidy (and therefore, blocks can't be mined)
    • Votes are accepted with the new vote subsidy
    • Blocks are NOT accepted with the original work subsidy (modified the mining code to test)
    • Blocks are accepted with the new vote and work subsidy
  • Validated that disconnecting and reconnecting blocks across the agenda activation height works as expected

Looks good to me!

internal/mempool/mempool_test.go Outdated Show resolved Hide resolved
This implements the agenda for voting on the modified subsidy split
defined in DCP0010 along with consensus tests.

In particular, once the vote has passed and is active, the PoW subsidy
will be 10% of the block reward and the PoS subsidy will be 80%.  The
Treasury subsidy will remain at 10%.

In terms of the overall effects, this includes updates to:
- The validation logic for votes, coinbases, and overall block subsidy
- Enforcement when considering candidate votes for acceptance to the
  mempool, relaying, and inclusion into block templates
- Mining template generation
- The output of the getblocksubsidy RPC

Also note that this does not implement the block version bump that will
ultimately be needed by the mining code since there are multiple
consensus votes gated behind it and will therefore be done separately.

The following is an overview of the changes:

- Introduce a convenience function for determining if the vote passed
  and is now active
- Add new methods to blockchain/standalone for calculating the work and
  stake vote subsidies according to either the original or modified
  values per a provided flag
- Modify vote validation to enforce the new stake vote subsidy in
  accordance with the state of the vote
- Modify coinbase validation to enforce the new work subsidy in
  accordance with the state of the vote
- Modify block validation logic to enforce the total overall subsidy in
  accordance with the state of the vote
- Add tests for determining if the agenda is active for both mainnet and
  testnet
- Add tests for getblocksubsidy RPC
- Add tests to ensure proper behavior for the modified subsidy splits as
  follows:
  - Ensure new blockchain validation semantics are enforced once the
    agenda is active
  - Ensure mempool correctly accepts and rejects votes in accordance
    with the state of the vote
This updates the simnet environment documentation to account for the
different expected initial balances due to the subsidy split agenda
since it is always active on simnet.
Copy link
Member

@matheusd matheusd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on simnet. Woop!

Copy link
Member

@dnldd dnldd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simnet testing looks good.

@davecgh davecgh merged commit 259b51b into decred:master Dec 17, 2021
@davecgh davecgh deleted the multi_dcp0010_subsidy_split branch December 17, 2021 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus changes Changes that involve modifying the consensus rules and thus are required to be gated behind a vote.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants