[Staking] Align block value with masternode payment #967
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CaveSpectre11
changed the title
CaveSpectre11
force-pushed
the
height-alignment
branch
from
bf51418
to
cf7409e
Compare
|
rebased with current master |
1 task
random-zebra
added a commit
that referenced
this pull request
Oct 6, 2020
288953d [Refactor] Cleaner GetBlockValue implementation (random-zebra) 0d2fc0a [Bug] Align GetBlockValue calls to CreateCoinStake: current height + 1 (random-zebra) Pull request description: This is a known old bug, which was never truly fixed, due to it being harmless (and discovered after the last block-value reduction). The function `GetBlockValue(int nHeight)` returns the block-reward value for a block at a given height (based on a "halving" schedule hardcoded within the function). The issue is that it was being called passing the height of the *previous* block. At some point, one particular call (in `CreateCoinStake`) was changed to use the (correct) current block height, while leaving the wrong one in all other places (including masternode payments/budgets and validation code). This made no real difference with PIVX, as the change was introduced before the last milestone (which increased the block value from 4.5 to 5, rather than reducing it), and, after that, the block value never changed anymore (so it has always been `GetBlockValue(N) == GetBlockValue(N-1)`). But it was an issue for all PIVX forks that had different schedules (or that introduced drastic changes to the masternode/staker-rewards proportions). In #967 @CaveSpectre11 gives a complete explanation of the problem (already discussed on a wrong fix attempt in #814), and proposes to change only the call in `CreateCoinStake`, aligning it to the (wrong) height (mostly for the sake of consistency and for the benefit of future PIVX forks, as it has no effect on PIVX chain, at his current height). This PR, instead, proposes the opposite solution. Align all other calls, so that the correct height is passed (`pindex->nHeight` during validation, and `pindexPrev->nHeight + 1` when creating a new block). This makes the code consistent with the actual block values on chain. For example, the first-6-masternodes-premine was **not** in the genesis block, but at block 1. Therefore we should have had `60001 * COIN` returned by `GetBlockValue(1)`, not by `GetBlockValue(0)`. - main-net <https://explorer.pivx.link/block/000005504fa4a6766e854b2a2c3f21cd276fd7305b84f416241fd4431acbd12d> - testnet <https://testnet.pivx.link/block/0000081796e8bf7f8047cc84681ceecb64da781e11961da844d0f8b7c13690a5> Thus, to align it with the values on chain (and prevent issues during initial sync) the schedule defined inside `GetBlockValue` is "shifted ahead" by one block. This also refactors `GetBlockValue` to make it more readable. Tested Resyncing from scratch on both testnet and mainnet. Based on top of - [x] #1844 just to avoid merge conflicts with the refactoring of `CBudgetManager::FillBlockPayee`. Only the last two commits are new here. ACKs for top commit: furszy: utACK 288953d after rebase. Fuzzbawls: ACK 288953d Tree-SHA512: 684bb9aee09067e459abd2bbcb338e1a93d499e76732043e85a89b607d70fc22a778253426ae14763a26be6b93e60a65cb165368ea8b7dbf77d87c9021b993b8
|
Superseded by #1848 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[Splitting from #952 ]
Release Notes
Details
There is a documented overmint error (PR #814 ), that requires a fair effort of rolling back through the chain and making sure that all overmints are accounted for when validating the full chain. However, part of PR #582 made one small change in CreateCoinStake(); calling GetBlockValue() with the next height, instead of the current height:
This, of course, is the right block value to get. However everywhere else (consensus and masternode payment) is using the previous block height for it's calculations. So this correction actually threw the whole system out of whack on block 1153160. If you look at that block; you will see where this was a minor issue. The block creator calculated the block reward to be 5; but then when filling the masternode payee; it went through SeeSaw to calculate the masternode payment. It's a historical issue; but should the block values change again, this could pose a much larger problem.... i.e. if it had been in place on block 648000; The miner would have figured the reward was 4.5; but then calculated the masternode's share from a block value of 9; likely paying the masternode more than the total block reward (i.e. the staker ends up "paying" to stake the block).
Of course consensus didn't care, because it just cares that you didn't overmint (at least their perception of overmint), and cares that you didn't short change the masternode. Since the writer of 1153160 actually short changed itself (as far as consensus was concerned); the block was accepted.
So while the issue still exists where the reward changes will happen 1 block after intended; all parties will at least be looking up the same block height (the previous block) when determining the reward split; and if any future subsidy changes are made; the staker doesn't rip themselves off. This area of code can be corrected when the rest of the system is.