Don't warn about activated buried BIP 9 deployments #16704
Conversation
When the height of a block is less than the activation height of a buried bip9 soft fork, compute the version number containing the deployment bits that would have been in use at that time
| @@ -1636,7 +1649,7 @@ class WarningBitsConditionChecker : public AbstractThresholdConditionChecker | |||
| { | |||
| return ((pindex->nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS) && | |||
| ((pindex->nVersion >> bit) & 1) != 0 && | |||
| ((ComputeBlockVersion(pindex->pprev, params) >> bit) & 1) == 0; | |||
| ((ComputeBuriedBlockVersion(pindex->pprev, params) >> bit) & 1) == 0; | |||
There was a problem hiding this comment.
Why would you want to historically check for versionbits warnings, when we assume that signatures in the chain are valid?
| ((ComputeBuriedBlockVersion(pindex->pprev, params) >> bit) & 1) == 0; | |
| ((ComputeBlockVersion(pindex->pprev, params) >> bit) & 1) == 0 && | |
| !IsInAssumedValidChain(pindex, params); |
There was a problem hiding this comment.
That feels less robust to me. assumevalid is something that users can change, so if a user were to change assumevalid to something else (e.g. so they could fully validate signatures too), then the warning would appear for them. Also, I don't think that would work on regtest, but I suppose it's really only a problem there if you are trying to test warnings.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Minimal alternative at #16713 |
| @@ -82,6 +80,16 @@ class CMainParams : public CChainParams { | |||
| consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nStartTime = 1199145601; // January 1, 2008 | |||
| consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = 1230767999; // December 31, 2008 | |||
|
|
|||
| // Add buried depoyments | |||
There was a problem hiding this comment.
Should be "deployments" -- applies throughout this PR :-)
Addresses bitcoin#16697 as a light alternative to bitcoin#16704 This will set all states to DEFINED instead of STARTED and there for skip counting blocks and computing down.
|
Let's get one of these in soon, that |
I agree. I'll make the pitch for #16713: it achieves the same as this with a much smaller code diff that doesn't introduce new code paths. |
|
Agree with @jnewbery |
|
Closing for now. Let me know if I was wrong and this should be reopened. |
The BIP 9 warning checker finds the blocks to warn on by computing the block version number that it would generate based on current active BIP 9 deployments. For buried deployments, their deployment parameters are removed from the active deployments array, so the warning checker triggers a false positive warning when it comes upon the blocks that activated the buried deployment as their version numbers do not match what it computes.
This PR changes the warning checker to take into account these buried deployments. When it scans through history, it includes the activation bits for buried forks as long as the height of the block it is checking is less than the activation height of the deployment. Thus it will find the blocks that activate buried deployments to have the correct version bits set so it does not trigger a false positive warning.
To facilitate this, the BIP 9 buried deployments (CSV and Segwit) are moved into an array of structs that contain their activation height and bit, in a similar way to active deployments.
Closes #16697