Don’t warn about activated buried BIP 9 deployments #16704

1. 
   achow101 commented at 8:39 pm on August 23, 2019: member

   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

2. Introduce BuriedBIP9 and BuriedDeployments for containing info about buried forks 2fc5dee997
3. Compute the block version for a buried fork

   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

   78c289a3b6
4. in src/validation.cpp:1652 in 78c289a3b6

   1648@@ -1636,7 +1649,7 @@ class WarningBitsConditionChecker : public AbstractThresholdConditionChecker
   1649     {
   1650         return ((pindex->nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS) &&
   1651                ((pindex->nVersion >> bit) & 1) != 0 &&
   1652-               ((ComputeBlockVersion(pindex->pprev, params) >> bit) & 1) == 0;
   1653+               ((ComputeBuriedBlockVersion(pindex->pprev, params) >> bit) & 1) == 0;
   

   ---

   ---

   

   MarcoFalke commented at 8:52 pm on August 23, 2019:

   Why would you want to historically check for versionbits warnings, when we assume that signatures in the chain are valid?

   0               ((ComputeBlockVersion(pindex->pprev, params) >> bit) & 1) == 0 &&
   1               !IsInAssumedValidChain(pindex, params);
   

   ---

   

   achow101 commented at 5:06 am on August 24, 2019:

   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.
5. 
   MarcoFalke commented at 8:53 pm on August 23, 2019: member

   Looks like a lot of changes and code for a dumb check that shouldn’t be run on the historic chain anyway.
6. DrahtBot added the label Mining on Aug 23, 2019
7. DrahtBot added the label RPC/REST/ZMQ on Aug 23, 2019
8. DrahtBot added the label Validation on Aug 23, 2019
9. 
   DrahtBot commented at 10:20 pm on August 23, 2019: member

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #16713 (logging: Redefine CSV and segwit deployments to avoid ‘unknown softforks’ warning by jnewbery)
   • #16411 (Signet support by kallewoof)

   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.

10. 
    jnewbery commented at 6:10 pm on August 24, 2019: member

    Minimal alternative at #16713
11. in src/chainparams.cpp:83 in 78c289a3b6

    79@@ -82,6 +80,16 @@ class CMainParams : public CChainParams {
    80         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nStartTime = 1199145601; // January 1, 2008
    81         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = 1230767999; // December 31, 2008
    82 
    83+        // Add buried depoyments
    

    ---

    ---

    

    practicalswift commented at 9:53 pm on August 25, 2019:

    Should be “deployments” – applies throughout this PR :-)
12. HashUnlimited referenced this in commit 78de3574aa on Sep 2, 2019
13. 
    laanwj commented at 1:07 pm on September 5, 2019: member

    Let’s get one of these in soon, that "warnings": "Warning: unknown new rules activated (versionbit 1)" is kind of worrying.
14. laanwj added this to the milestone 0.19.0 on Sep 5, 2019
15. 
    jnewbery commented at 1:54 pm on September 5, 2019: member

    Let’s get one of these in soon

    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.

16. 
    MarcoFalke commented at 5:56 pm on September 5, 2019: member

    Agree with @jnewbery
17. MarcoFalke closed this on Sep 5, 2019

    ---

18. 
    MarcoFalke commented at 6:12 pm on September 5, 2019: member

    Closing for now. Let me know if I was wrong and this should be reopened.
19. DrahtBot locked this on Dec 16, 2021

Contributors
achow101 MarcoFalke DrahtBot jnewbery practicalswift laanwj

Labels
RPC/REST/ZMQ Mining Validation

Milestone
0.19.0