From 63879f0a4760c0c0f784029849cb5d21ee088abb Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Sat, 27 Mar 2021 20:17:56 +1000
Subject: [PATCH 1/9] tests: pull ComputeBlockVersion test into its own
 function

The intent here is to allow checking ComputeBlockVersion behaviour with
each deployment, rather than only testdummy on mainnet. This commit does
the trivial refactoring component of that change.
---
 src/test/versionbits_tests.cpp | 74 ++++++++++++++++++----------------
 1 file changed, 39 insertions(+), 35 deletions(-)

diff --git a/src/test/versionbits_tests.cpp b/src/test/versionbits_tests.cpp
index 8841a540f2d..b09827ffe5d 100644
--- a/src/test/versionbits_tests.cpp
+++ b/src/test/versionbits_tests.cpp
@@ -242,17 +242,15 @@ BOOST_AUTO_TEST_CASE(versionbits_test)
     }
 }
 
-BOOST_AUTO_TEST_CASE(versionbits_computeblockversion)
+/** Check that ComputeBlockVersion will set the appropriate bit correctly */
+static void check_computeblockversion(const Consensus::Params& params, Consensus::DeploymentPos dep)
 {
-    // Check that ComputeBlockVersion will set the appropriate bit correctly
-    // on mainnet.
-    const auto chainParams = CreateChainParams(*m_node.args, CBaseChainParams::MAIN);
-    const Consensus::Params &mainnetParams = chainParams->GetConsensus();
+    // This implicitly uses versionbitscache, so clear it every time
+    versionbitscache.Clear();
 
-    // Use the TESTDUMMY deployment for testing purposes.
-    int64_t bit = mainnetParams.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].bit;
-    int64_t nStartTime = mainnetParams.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nStartTime;
-    int64_t nTimeout = mainnetParams.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout;
+    int64_t bit = params.vDeployments[dep].bit;
+    int64_t nStartTime = params.vDeployments[dep].nStartTime;
+    int64_t nTimeout = params.vDeployments[dep].nTimeout;
 
     assert(nStartTime < nTimeout);
 
@@ -267,40 +265,40 @@ BOOST_AUTO_TEST_CASE(versionbits_computeblockversion)
     // Before MedianTimePast of the chain has crossed nStartTime, the bit
     // should not be set.
     CBlockIndex *lastBlock = nullptr;
-    lastBlock = firstChain.Mine(mainnetParams.nMinerConfirmationWindow, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
-    BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, mainnetParams) & (1<<bit), 0);
+    lastBlock = firstChain.Mine(params.nMinerConfirmationWindow, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
+    BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, params) & (1<<bit), 0);
 
     // Mine more blocks (4 less than the adjustment period) at the old time, and check that CBV isn't setting the bit yet.
-    for (uint32_t i = 1; i < mainnetParams.nMinerConfirmationWindow - 4; i++) {
-        lastBlock = firstChain.Mine(mainnetParams.nMinerConfirmationWindow + i, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
+    for (uint32_t i = 1; i < params.nMinerConfirmationWindow - 4; i++) {
+        lastBlock = firstChain.Mine(params.nMinerConfirmationWindow + i, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
         // This works because VERSIONBITS_LAST_OLD_BLOCK_VERSION happens
         // to be 4, and the bit we're testing happens to be bit 28.
-        BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, mainnetParams) & (1<<bit), 0);
+        BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, params) & (1<<bit), 0);
     }
     // Now mine 5 more blocks at the start time -- MTP should not have passed yet, so
     // CBV should still not yet set the bit.
     nTime = nStartTime;
-    for (uint32_t i = mainnetParams.nMinerConfirmationWindow - 4; i <= mainnetParams.nMinerConfirmationWindow; i++) {
-        lastBlock = firstChain.Mine(mainnetParams.nMinerConfirmationWindow + i, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
-        BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, mainnetParams) & (1<<bit), 0);
+    for (uint32_t i = params.nMinerConfirmationWindow - 4; i <= params.nMinerConfirmationWindow; i++) {
+        lastBlock = firstChain.Mine(params.nMinerConfirmationWindow + i, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
+        BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, params) & (1<<bit), 0);
     }
 
     // Advance to the next period and transition to STARTED,
-    lastBlock = firstChain.Mine(mainnetParams.nMinerConfirmationWindow * 3, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
+    lastBlock = firstChain.Mine(params.nMinerConfirmationWindow * 3, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
     // so ComputeBlockVersion should now set the bit,
-    BOOST_CHECK((ComputeBlockVersion(lastBlock, mainnetParams) & (1<<bit)) != 0);
+    BOOST_CHECK((ComputeBlockVersion(lastBlock, params) & (1<<bit)) != 0);
     // and should also be using the VERSIONBITS_TOP_BITS.
-    BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, mainnetParams) & VERSIONBITS_TOP_MASK, VERSIONBITS_TOP_BITS);
+    BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, params) & VERSIONBITS_TOP_MASK, VERSIONBITS_TOP_BITS);
 
     // Check that ComputeBlockVersion will set the bit until nTimeout
     nTime += 600;
-    uint32_t blocksToMine = mainnetParams.nMinerConfirmationWindow * 2; // test blocks for up to 2 time periods
-    uint32_t nHeight = mainnetParams.nMinerConfirmationWindow * 3;
+    uint32_t blocksToMine = params.nMinerConfirmationWindow * 2; // test blocks for up to 2 time periods
+    uint32_t nHeight = params.nMinerConfirmationWindow * 3;
     // These blocks are all before nTimeout is reached.
     while (nTime < nTimeout && blocksToMine > 0) {
         lastBlock = firstChain.Mine(nHeight+1, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
-        BOOST_CHECK((ComputeBlockVersion(lastBlock, mainnetParams) & (1<<bit)) != 0);
-        BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, mainnetParams) & VERSIONBITS_TOP_MASK, VERSIONBITS_TOP_BITS);
+        BOOST_CHECK((ComputeBlockVersion(lastBlock, params) & (1<<bit)) != 0);
+        BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, params) & VERSIONBITS_TOP_MASK, VERSIONBITS_TOP_BITS);
         blocksToMine--;
         nTime += 600;
         nHeight += 1;
@@ -309,14 +307,14 @@ BOOST_AUTO_TEST_CASE(versionbits_computeblockversion)
     nTime = nTimeout;
     // FAILED is only triggered at the end of a period, so CBV should be setting
     // the bit until the period transition.
-    for (uint32_t i = 0; i < mainnetParams.nMinerConfirmationWindow - 1; i++) {
+    for (uint32_t i = 0; i < params.nMinerConfirmationWindow - 1; i++) {
         lastBlock = firstChain.Mine(nHeight+1, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
-        BOOST_CHECK((ComputeBlockVersion(lastBlock, mainnetParams) & (1<<bit)) != 0);
+        BOOST_CHECK((ComputeBlockVersion(lastBlock, params) & (1<<bit)) != 0);
         nHeight += 1;
     }
     // The next block should trigger no longer setting the bit.
     lastBlock = firstChain.Mine(nHeight+1, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
-    BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, mainnetParams) & (1<<bit), 0);
+    BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, params) & (1<<bit), 0);
 
     // On a new chain:
     // verify that the bit will be set after lock-in, and then stop being set
@@ -325,26 +323,32 @@ BOOST_AUTO_TEST_CASE(versionbits_computeblockversion)
 
     // Mine one period worth of blocks, and check that the bit will be on for the
     // next period.
-    lastBlock = secondChain.Mine(mainnetParams.nMinerConfirmationWindow, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
-    BOOST_CHECK((ComputeBlockVersion(lastBlock, mainnetParams) & (1<<bit)) != 0);
+    lastBlock = secondChain.Mine(params.nMinerConfirmationWindow, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
+    BOOST_CHECK((ComputeBlockVersion(lastBlock, params) & (1<<bit)) != 0);
 
     // Mine another period worth of blocks, signaling the new bit.
-    lastBlock = secondChain.Mine(mainnetParams.nMinerConfirmationWindow * 2, nTime, VERSIONBITS_TOP_BITS | (1<<bit)).Tip();
+    lastBlock = secondChain.Mine(params.nMinerConfirmationWindow * 2, nTime, VERSIONBITS_TOP_BITS | (1<<bit)).Tip();
     // After one period of setting the bit on each block, it should have locked in.
     // We keep setting the bit for one more period though, until activation.
-    BOOST_CHECK((ComputeBlockVersion(lastBlock, mainnetParams) & (1<<bit)) != 0);
+    BOOST_CHECK((ComputeBlockVersion(lastBlock, params) & (1<<bit)) != 0);
 
     // Now check that we keep mining the block until the end of this period, and
     // then stop at the beginning of the next period.
-    lastBlock = secondChain.Mine((mainnetParams.nMinerConfirmationWindow * 3) - 1, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
-    BOOST_CHECK((ComputeBlockVersion(lastBlock, mainnetParams) & (1 << bit)) != 0);
-    lastBlock = secondChain.Mine(mainnetParams.nMinerConfirmationWindow * 3, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
-    BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, mainnetParams) & (1<<bit), 0);
+    lastBlock = secondChain.Mine((params.nMinerConfirmationWindow * 3) - 1, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
+    BOOST_CHECK((ComputeBlockVersion(lastBlock, params) & (1 << bit)) != 0);
+    lastBlock = secondChain.Mine(params.nMinerConfirmationWindow * 3, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
+    BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, params) & (1<<bit), 0);
 
     // Finally, verify that after a soft fork has activated, CBV no longer uses
     // VERSIONBITS_LAST_OLD_BLOCK_VERSION.
     //BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, mainnetParams) & VERSIONBITS_TOP_MASK, VERSIONBITS_TOP_BITS);
 }
 
+BOOST_AUTO_TEST_CASE(versionbits_computeblockversion)
+{
+    // Use the TESTDUMMY deployment for testing purposes.
+    const auto chainParams = CreateChainParams(*m_node.args, CBaseChainParams::MAIN);
+    check_computeblockversion(chainParams->GetConsensus(), Consensus::DEPLOYMENT_TESTDUMMY);
+}
 
 BOOST_AUTO_TEST_SUITE_END()

From 593274445004506c921d5d851361aefb3434d744 Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Sun, 28 Mar 2021 03:10:48 +1000
Subject: [PATCH 2/9] tests: test ComputeBlockVersion for all deployments

This generalises the ComputeBlockVersion test so that it can apply to
any activation parameters we might set, and checks all the parameters
set for each deployment on each chain, to simultaneously ensure that the
deployments we have configured work sensibly, and that the test code
does not suffer bitrot in the event that all interesting deployments
are buried.
---
 src/test/versionbits_tests.cpp | 106 ++++++++++++++++++++++-----------
 1 file changed, 71 insertions(+), 35 deletions(-)

diff --git a/src/test/versionbits_tests.cpp b/src/test/versionbits_tests.cpp
index b09827ffe5d..7e96ef923ea 100644
--- a/src/test/versionbits_tests.cpp
+++ b/src/test/versionbits_tests.cpp
@@ -252,38 +252,61 @@ static void check_computeblockversion(const Consensus::Params& params, Consensus
     int64_t nStartTime = params.vDeployments[dep].nStartTime;
     int64_t nTimeout = params.vDeployments[dep].nTimeout;
 
-    assert(nStartTime < nTimeout);
+    // should not be any signalling for first block
+    BOOST_CHECK_EQUAL(ComputeBlockVersion(nullptr, params), VERSIONBITS_TOP_BITS);
+
+    // always active deployments shouldn't need to be tested further
+    if (nStartTime == Consensus::BIP9Deployment::ALWAYS_ACTIVE) return;
+
+    BOOST_REQUIRE(nStartTime < nTimeout);
+    BOOST_REQUIRE(nStartTime >= 0);
+    BOOST_REQUIRE(nTimeout <= std::numeric_limits<uint32_t>::max() || nTimeout == Consensus::BIP9Deployment::NO_TIMEOUT);
+    BOOST_REQUIRE(0 <= bit && bit < 32);
+    BOOST_REQUIRE(((1 << bit) & VERSIONBITS_TOP_MASK) == 0);
 
     // In the first chain, test that the bit is set by CBV until it has failed.
     // In the second chain, test the bit is set by CBV while STARTED and
     // LOCKED-IN, and then no longer set while ACTIVE.
     VersionBitsTester firstChain, secondChain;
 
-    // Start generating blocks before nStartTime
-    int64_t nTime = nStartTime - 1;
+    int64_t nTime = nStartTime;
+
+    CBlockIndex *lastBlock = nullptr;
 
     // Before MedianTimePast of the chain has crossed nStartTime, the bit
     // should not be set.
-    CBlockIndex *lastBlock = nullptr;
-    lastBlock = firstChain.Mine(params.nMinerConfirmationWindow, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
-    BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, params) & (1<<bit), 0);
+    if (nTime == 0) {
+        // since CBlockIndex::nTime is uint32_t we can't represent any
+        // earlier time, so will transition from DEFINED to STARTED at the
+        // end of the first period by mining blocks at nTime == 0
+        lastBlock = firstChain.Mine(params.nMinerConfirmationWindow - 1, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
+        BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, params) & (1<<bit), 0);
+        lastBlock = firstChain.Mine(params.nMinerConfirmationWindow, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
+        BOOST_CHECK((ComputeBlockVersion(lastBlock, params) & (1<<bit)) != 0);
+        // then we'll keep mining at nStartTime...
+    } else {
+        // use a time 1s earlier than start time to check we stay DEFINED
+        --nTime;
 
-    // Mine more blocks (4 less than the adjustment period) at the old time, and check that CBV isn't setting the bit yet.
-    for (uint32_t i = 1; i < params.nMinerConfirmationWindow - 4; i++) {
-        lastBlock = firstChain.Mine(params.nMinerConfirmationWindow + i, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
-        // This works because VERSIONBITS_LAST_OLD_BLOCK_VERSION happens
-        // to be 4, and the bit we're testing happens to be bit 28.
-        BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, params) & (1<<bit), 0);
-    }
-    // Now mine 5 more blocks at the start time -- MTP should not have passed yet, so
-    // CBV should still not yet set the bit.
-    nTime = nStartTime;
-    for (uint32_t i = params.nMinerConfirmationWindow - 4; i <= params.nMinerConfirmationWindow; i++) {
-        lastBlock = firstChain.Mine(params.nMinerConfirmationWindow + i, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
+        // Start generating blocks before nStartTime
+        lastBlock = firstChain.Mine(params.nMinerConfirmationWindow, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
         BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, params) & (1<<bit), 0);
+
+        // Mine more blocks (4 less than the adjustment period) at the old time, and check that CBV isn't setting the bit yet.
+        for (uint32_t i = 1; i < params.nMinerConfirmationWindow - 4; i++) {
+            lastBlock = firstChain.Mine(params.nMinerConfirmationWindow + i, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
+            BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, params) & (1<<bit), 0);
+        }
+        // Now mine 5 more blocks at the start time -- MTP should not have passed yet, so
+        // CBV should still not yet set the bit.
+        nTime = nStartTime;
+        for (uint32_t i = params.nMinerConfirmationWindow - 4; i <= params.nMinerConfirmationWindow; i++) {
+            lastBlock = firstChain.Mine(params.nMinerConfirmationWindow + i, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
+            BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, params) & (1<<bit), 0);
+        }
+        // Next we will advance to the next period and transition to STARTED,
     }
 
-    // Advance to the next period and transition to STARTED,
     lastBlock = firstChain.Mine(params.nMinerConfirmationWindow * 3, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
     // so ComputeBlockVersion should now set the bit,
     BOOST_CHECK((ComputeBlockVersion(lastBlock, params) & (1<<bit)) != 0);
@@ -304,17 +327,29 @@ static void check_computeblockversion(const Consensus::Params& params, Consensus
         nHeight += 1;
     }
 
-    nTime = nTimeout;
-    // FAILED is only triggered at the end of a period, so CBV should be setting
-    // the bit until the period transition.
-    for (uint32_t i = 0; i < params.nMinerConfirmationWindow - 1; i++) {
+    if (nTimeout != Consensus::BIP9Deployment::NO_TIMEOUT) {
+        // can reach any nTimeout other than NO_TIMEOUT due to earlier BOOST_REQUIRE
+
+        nTime = nTimeout;
+
+        // finish the last period before we start timing out
+        while (nHeight % params.nMinerConfirmationWindow != 0) {
+            lastBlock = firstChain.Mine(nHeight+1, nTime - 1, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
+            BOOST_CHECK((ComputeBlockVersion(lastBlock, params) & (1<<bit)) != 0);
+            nHeight += 1;
+        }
+
+        // FAILED is only triggered at the end of a period, so CBV should be setting
+        // the bit until the period transition.
+        for (uint32_t i = 0; i < params.nMinerConfirmationWindow - 1; i++) {
+            lastBlock = firstChain.Mine(nHeight+1, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
+            BOOST_CHECK((ComputeBlockVersion(lastBlock, params) & (1<<bit)) != 0);
+            nHeight += 1;
+        }
+        // The next block should trigger no longer setting the bit.
         lastBlock = firstChain.Mine(nHeight+1, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
-        BOOST_CHECK((ComputeBlockVersion(lastBlock, params) & (1<<bit)) != 0);
-        nHeight += 1;
+        BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, params) & (1<<bit), 0);
     }
-    // The next block should trigger no longer setting the bit.
-    lastBlock = firstChain.Mine(nHeight+1, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
-    BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, params) & (1<<bit), 0);
 
     // On a new chain:
     // verify that the bit will be set after lock-in, and then stop being set
@@ -338,17 +373,18 @@ static void check_computeblockversion(const Consensus::Params& params, Consensus
     BOOST_CHECK((ComputeBlockVersion(lastBlock, params) & (1 << bit)) != 0);
     lastBlock = secondChain.Mine(params.nMinerConfirmationWindow * 3, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
     BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, params) & (1<<bit), 0);
-
-    // Finally, verify that after a soft fork has activated, CBV no longer uses
-    // VERSIONBITS_LAST_OLD_BLOCK_VERSION.
-    //BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, mainnetParams) & VERSIONBITS_TOP_MASK, VERSIONBITS_TOP_BITS);
 }
 
 BOOST_AUTO_TEST_CASE(versionbits_computeblockversion)
 {
-    // Use the TESTDUMMY deployment for testing purposes.
-    const auto chainParams = CreateChainParams(*m_node.args, CBaseChainParams::MAIN);
-    check_computeblockversion(chainParams->GetConsensus(), Consensus::DEPLOYMENT_TESTDUMMY);
+    // check that any deployment on any chain can conceivably reach both
+    // ACTIVE and FAILED states in roughly the way we expect
+    for (const auto& chain_name : {CBaseChainParams::MAIN, CBaseChainParams::TESTNET, CBaseChainParams::SIGNET, CBaseChainParams::REGTEST}) {
+        const auto chainParams = CreateChainParams(*m_node.args, chain_name);
+        for (int i = 0; i < (int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++i) {
+            check_computeblockversion(chainParams->GetConsensus(), static_cast<Consensus::DeploymentPos>(i));
+        }
+    }
 }
 
 BOOST_AUTO_TEST_SUITE_END()

From 9e6b65f6fa205eee5c3b99343988adcb8d320460 Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Sun, 28 Mar 2021 00:07:49 +1000
Subject: [PATCH 3/9] tests: clean up versionbits test

Simplify the versionbits unit test slightly to make the next set of
changes a little easier to follow.
---
 src/test/versionbits_tests.cpp | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/src/test/versionbits_tests.cpp b/src/test/versionbits_tests.cpp
index 7e96ef923ea..b42aaff9ad1 100644
--- a/src/test/versionbits_tests.cpp
+++ b/src/test/versionbits_tests.cpp
@@ -100,7 +100,7 @@ public:
         while (vpblock.size() < height) {
             CBlockIndex* pindex = new CBlockIndex();
             pindex->nHeight = vpblock.size();
-            pindex->pprev = vpblock.size() > 0 ? vpblock.back() : nullptr;
+            pindex->pprev = Tip();
             pindex->nTime = nTime;
             pindex->nVersion = nVersion;
             pindex->BuildSkip();
@@ -110,13 +110,14 @@ public:
     }
 
     VersionBitsTester& TestStateSinceHeight(int height) {
+        const CBlockIndex* tip = Tip();
         for (int i = 0; i < CHECKERS; i++) {
             if (InsecureRandBits(i) == 0) {
-                BOOST_CHECK_MESSAGE(checker[i].GetStateSinceHeightFor(vpblock.empty() ? nullptr : vpblock.back()) == height, strprintf("Test %i for StateSinceHeight", num));
-                BOOST_CHECK_MESSAGE(checker_always[i].GetStateSinceHeightFor(vpblock.empty() ? nullptr : vpblock.back()) == 0, strprintf("Test %i for StateSinceHeight (always active)", num));
+                BOOST_CHECK_MESSAGE(checker[i].GetStateSinceHeightFor(tip) == height, strprintf("Test %i for StateSinceHeight", num));
+                BOOST_CHECK_MESSAGE(checker_always[i].GetStateSinceHeightFor(tip) == 0, strprintf("Test %i for StateSinceHeight (always active)", num));
 
                 // never active may go from DEFINED -> FAILED at the first period
-                const auto never_height = checker_never[i].GetStateSinceHeightFor(vpblock.empty() ? nullptr : vpblock.back());
+                const auto never_height = checker_never[i].GetStateSinceHeightFor(tip);
                 BOOST_CHECK_MESSAGE(never_height == 0 || never_height == checker_never[i].Period(paramsDummy), strprintf("Test %i for StateSinceHeight (never active)", num));
             }
         }
@@ -125,9 +126,9 @@ public:
     }
 
     VersionBitsTester& TestState(ThresholdState exp) {
+        const CBlockIndex* pindex = Tip();
         for (int i = 0; i < CHECKERS; i++) {
             if (InsecureRandBits(i) == 0) {
-                const CBlockIndex* pindex = vpblock.empty() ? nullptr : vpblock.back();
                 ThresholdState got = checker[i].GetStateFor(pindex);
                 ThresholdState got_always = checker_always[i].GetStateFor(pindex);
                 ThresholdState got_never = checker_never[i].GetStateFor(pindex);
@@ -149,7 +150,7 @@ public:
     VersionBitsTester& TestActive() { return TestState(ThresholdState::ACTIVE); }
     VersionBitsTester& TestFailed() { return TestState(ThresholdState::FAILED); }
 
-    CBlockIndex * Tip() { return vpblock.size() ? vpblock.back() : nullptr; }
+    CBlockIndex* Tip() { return vpblock.empty() ? nullptr : vpblock.back(); }
 };
 
 BOOST_FIXTURE_TEST_SUITE(versionbits_tests, TestingSetup)
@@ -217,7 +218,10 @@ BOOST_AUTO_TEST_CASE(versionbits_test)
                            .Mine(6000, TestTime(20000), 0).TestFailed().TestStateSinceHeight(6000)
                            .Mine(7000, TestTime(20000), 0x100).TestFailed().TestStateSinceHeight(6000);
     }
+}
 
+BOOST_AUTO_TEST_CASE(versionbits_sanity)
+{
     // Sanity checks of version bit deployments
     const auto chainParams = CreateChainParams(*m_node.args, CBaseChainParams::MAIN);
     const Consensus::Params &mainnetParams = chainParams->GetConsensus();
@@ -271,7 +275,7 @@ static void check_computeblockversion(const Consensus::Params& params, Consensus
 
     int64_t nTime = nStartTime;
 
-    CBlockIndex *lastBlock = nullptr;
+    const CBlockIndex *lastBlock = nullptr;
 
     // Before MedianTimePast of the chain has crossed nStartTime, the bit
     // should not be set.

From 73d4a706393e6dbd6b6d6b6428f8d3233ac0a2d8 Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Sat, 6 Mar 2021 18:18:49 +1000
Subject: [PATCH 4/9] versionbits: Add support for delayed activation

---
 src/chainparams.cpp               | 25 ++++++++++++++++++++-----
 src/chainparamsbase.cpp           |  2 +-
 src/consensus/params.h            |  5 +++++
 src/rpc/blockchain.cpp            |  2 ++
 src/test/versionbits_tests.cpp    |  2 ++
 src/versionbits.cpp               |  8 ++++++--
 src/versionbits.h                 |  3 ++-
 test/functional/rpc_blockchain.py |  4 +++-
 8 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/src/chainparams.cpp b/src/chainparams.cpp
index 16efffa6f0f..7e2c2ac7cf6 100644
--- a/src/chainparams.cpp
+++ b/src/chainparams.cpp
@@ -83,11 +83,13 @@ public:
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].bit = 28;
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nStartTime = 1199145601; // January 1, 2008
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = 1230767999; // December 31, 2008
+        consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].min_activation_height = 0; // No activation delay
 
         // Deployment of Taproot (BIPs 340-342)
         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].bit = 2;
         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nStartTime = 1199145601; // January 1, 2008
         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nTimeout = 1230767999; // December 31, 2008
+        consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].min_activation_height = 0; // No activation delay
 
         consensus.nMinimumChainWork = uint256S("0x00000000000000000000000000000000000000001533efd8d716a517fe2c5008");
         consensus.defaultAssumeValid = uint256S("0x0000000000000000000b9d2ec5a352ecba0592946514a92f14319dc2b367fc72"); // 654683
@@ -200,11 +202,13 @@ public:
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].bit = 28;
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nStartTime = 1199145601; // January 1, 2008
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = 1230767999; // December 31, 2008
+        consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].min_activation_height = 0; // No activation delay
 
         // Deployment of Taproot (BIPs 340-342)
         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].bit = 2;
         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nStartTime = 1199145601; // January 1, 2008
         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nTimeout = 1230767999; // December 31, 2008
+        consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].min_activation_height = 0; // No activation delay
 
         consensus.nMinimumChainWork = uint256S("0x0000000000000000000000000000000000000000000001db6ec4ac88cf2272c6");
         consensus.defaultAssumeValid = uint256S("0x000000000000006433d1efec504c53ca332b64963c425395515b01977bd7b3b0"); // 1864000
@@ -335,11 +339,13 @@ public:
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].bit = 28;
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nStartTime = 1199145601; // January 1, 2008
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = 1230767999; // December 31, 2008
+        consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].min_activation_height = 0; // No activation delay
 
         // Activation of Taproot (BIPs 340-342)
         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].bit = 2;
         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nStartTime = Consensus::BIP9Deployment::ALWAYS_ACTIVE;
         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT;
+        consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].min_activation_height = 0; // No activation delay
 
         // message start is defined as the first 4 bytes of the sha256d of the block script
         CHashWriter h(SER_DISK, 0);
@@ -397,12 +403,16 @@ public:
         consensus.fPowNoRetargeting = true;
         consensus.nRuleChangeActivationThreshold = 108; // 75% for testchains
         consensus.nMinerConfirmationWindow = 144; // Faster than normal for regtest (144 instead of 2016)
+
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].bit = 28;
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nStartTime = 0;
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT;
+        consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].min_activation_height = 0; // No activation delay
+
         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].bit = 2;
         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nStartTime = Consensus::BIP9Deployment::ALWAYS_ACTIVE;
         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT;
+        consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].min_activation_height = 0; // No activation delay
 
         consensus.nMinimumChainWork = uint256{};
         consensus.defaultAssumeValid = uint256{};
@@ -466,10 +476,11 @@ public:
     /**
      * Allows modifying the Version Bits regtest parameters.
      */
-    void UpdateVersionBitsParameters(Consensus::DeploymentPos d, int64_t nStartTime, int64_t nTimeout)
+    void UpdateVersionBitsParameters(Consensus::DeploymentPos d, int64_t nStartTime, int64_t nTimeout, int min_activation_height)
     {
         consensus.vDeployments[d].nStartTime = nStartTime;
         consensus.vDeployments[d].nTimeout = nTimeout;
+        consensus.vDeployments[d].min_activation_height = min_activation_height;
     }
     void UpdateActivationParametersFromArgs(const ArgsManager& args);
 };
@@ -492,22 +503,26 @@ void CRegTestParams::UpdateActivationParametersFromArgs(const ArgsManager& args)
     for (const std::string& strDeployment : args.GetArgs("-vbparams")) {
         std::vector<std::string> vDeploymentParams;
         boost::split(vDeploymentParams, strDeployment, boost::is_any_of(":"));
-        if (vDeploymentParams.size() != 3) {
-            throw std::runtime_error("Version bits parameters malformed, expecting deployment:start:end");
+        if (vDeploymentParams.size() < 3 || 4 < vDeploymentParams.size()) {
+            throw std::runtime_error("Version bits parameters malformed, expecting deployment:start:end[:min_activation_height]");
         }
         int64_t nStartTime, nTimeout;
+        int min_activation_height = 0;
         if (!ParseInt64(vDeploymentParams[1], &nStartTime)) {
             throw std::runtime_error(strprintf("Invalid nStartTime (%s)", vDeploymentParams[1]));
         }
         if (!ParseInt64(vDeploymentParams[2], &nTimeout)) {
             throw std::runtime_error(strprintf("Invalid nTimeout (%s)", vDeploymentParams[2]));
         }
+        if (vDeploymentParams.size() >= 4 && !ParseInt32(vDeploymentParams[3], &min_activation_height)) {
+            throw std::runtime_error(strprintf("Invalid min_activation_height (%s)", vDeploymentParams[3]));
+        }
         bool found = false;
         for (int j=0; j < (int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++j) {
             if (vDeploymentParams[0] == VersionBitsDeploymentInfo[j].name) {
-                UpdateVersionBitsParameters(Consensus::DeploymentPos(j), nStartTime, nTimeout);
+                UpdateVersionBitsParameters(Consensus::DeploymentPos(j), nStartTime, nTimeout, min_activation_height);
                 found = true;
-                LogPrintf("Setting version bits activation parameters for %s to start=%ld, timeout=%ld\n", vDeploymentParams[0], nStartTime, nTimeout);
+                LogPrintf("Setting version bits activation parameters for %s to start=%ld, timeout=%ld, min_activation_height=%d\n", vDeploymentParams[0], nStartTime, nTimeout, min_activation_height);
                 break;
             }
         }
diff --git a/src/chainparamsbase.cpp b/src/chainparamsbase.cpp
index 16311764778..e71b4bc8593 100644
--- a/src/chainparamsbase.cpp
+++ b/src/chainparamsbase.cpp
@@ -22,7 +22,7 @@ void SetupChainParamsBaseOptions(ArgsManager& argsman)
                  "This is intended for regression testing tools and app development. Equivalent to -chain=regtest.", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
     argsman.AddArg("-segwitheight=<n>", "Set the activation height of segwit. -1 to disable. (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-testnet", "Use the test chain. Equivalent to -chain=test.", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
-    argsman.AddArg("-vbparams=deployment:start:end", "Use given start/end times for specified version bits deployment (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
+    argsman.AddArg("-vbparams=deployment:start:end[:min_activation_height]", "Use given start/end times and min_activation_height for specified version bits deployment (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
     argsman.AddArg("-signet", "Use the signet chain. Equivalent to -chain=signet. Note that the network is defined by the -signetchallenge parameter", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
     argsman.AddArg("-signetchallenge", "Blocks must satisfy the given script to be considered valid (only for signet networks; defaults to the global default signet test network challenge)", ArgsManager::ALLOW_STRING, OptionsCategory::CHAINPARAMS);
     argsman.AddArg("-signetseednode", "Specify a seed node for the signet network, in the hostname[:port] format, e.g. sig.net:1234 (may be used multiple times to specify multiple seed nodes; defaults to the global default signet test network seed node(s))", ArgsManager::ALLOW_STRING, OptionsCategory::CHAINPARAMS);
diff --git a/src/consensus/params.h b/src/consensus/params.h
index 217cb019e1a..705205c95b0 100644
--- a/src/consensus/params.h
+++ b/src/consensus/params.h
@@ -29,6 +29,11 @@ struct BIP9Deployment {
     int64_t nStartTime;
     /** Timeout/expiry MedianTime for the deployment attempt. */
     int64_t nTimeout;
+    /** If lock in occurs, delay activation until at least this block
+     *  height.  Note that activation will only occur on a retarget
+     *  boundary.
+     */
+    int min_activation_height{0};
 
     /** Constant for nTimeout very far in the future. */
     static constexpr int64_t NO_TIMEOUT = std::numeric_limits<int64_t>::max();
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 3ba2fa46ae5..acf2f5d6b48 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -1258,6 +1258,7 @@ static void BIP9SoftForkDescPushBack(UniValue& softforks, const std::string &nam
         statsUV.pushKV("possible", statsStruct.possible);
         bip9.pushKV("statistics", statsUV);
     }
+    bip9.pushKV("min_activation_height", consensusParams.vDeployments[id].min_activation_height);
 
     UniValue rv(UniValue::VOBJ);
     rv.pushKV("type", "bip9");
@@ -1304,6 +1305,7 @@ RPCHelpMan getblockchaininfo()
                                     {RPCResult::Type::NUM_TIME, "start_time", "the minimum median time past of a block at which the bit gains its meaning"},
                                     {RPCResult::Type::NUM_TIME, "timeout", "the median time past of a block at which the deployment is considered failed if not yet locked in"},
                                     {RPCResult::Type::NUM, "since", "height of the first block to which the status applies"},
+                                    {RPCResult::Type::NUM, "min_activation_height", "minimum height of blocks for which the rules may be enforced"},
                                     {RPCResult::Type::OBJ, "statistics", "numeric statistics about BIP9 signalling for a softfork (only for \"started\" status)",
                                     {
                                         {RPCResult::Type::NUM, "period", "the length in blocks of the BIP9 signalling period"},
diff --git a/src/test/versionbits_tests.cpp b/src/test/versionbits_tests.cpp
index b42aaff9ad1..ae0d47b2b43 100644
--- a/src/test/versionbits_tests.cpp
+++ b/src/test/versionbits_tests.cpp
@@ -255,6 +255,7 @@ static void check_computeblockversion(const Consensus::Params& params, Consensus
     int64_t bit = params.vDeployments[dep].bit;
     int64_t nStartTime = params.vDeployments[dep].nStartTime;
     int64_t nTimeout = params.vDeployments[dep].nTimeout;
+    int min_activation_height = params.vDeployments[dep].min_activation_height;
 
     // should not be any signalling for first block
     BOOST_CHECK_EQUAL(ComputeBlockVersion(nullptr, params), VERSIONBITS_TOP_BITS);
@@ -267,6 +268,7 @@ static void check_computeblockversion(const Consensus::Params& params, Consensus
     BOOST_REQUIRE(nTimeout <= std::numeric_limits<uint32_t>::max() || nTimeout == Consensus::BIP9Deployment::NO_TIMEOUT);
     BOOST_REQUIRE(0 <= bit && bit < 32);
     BOOST_REQUIRE(((1 << bit) & VERSIONBITS_TOP_MASK) == 0);
+    BOOST_REQUIRE(min_activation_height == 0);
 
     // In the first chain, test that the bit is set by CBV until it has failed.
     // In the second chain, test the bit is set by CBV while STARTED and
diff --git a/src/versionbits.cpp b/src/versionbits.cpp
index af07c67ccf6..11da729596f 100644
--- a/src/versionbits.cpp
+++ b/src/versionbits.cpp
@@ -9,6 +9,7 @@ ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex*
 {
     int nPeriod = Period(params);
     int nThreshold = Threshold(params);
+    int min_activation_height = MinActivationHeight(params);
     int64_t nTimeStart = BeginTime(params);
     int64_t nTimeTimeout = EndTime(params);
 
@@ -78,8 +79,10 @@ ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex*
                 break;
             }
             case ThresholdState::LOCKED_IN: {
-                // Always progresses into ACTIVE.
-                stateNext = ThresholdState::ACTIVE;
+                // Progresses into ACTIVE provided activation height will have been reached.
+                if (pindexPrev->nHeight + 1 >= min_activation_height) {
+                    stateNext = ThresholdState::ACTIVE;
+                }
                 break;
             }
             case ThresholdState::FAILED:
@@ -170,6 +173,7 @@ private:
 protected:
     int64_t BeginTime(const Consensus::Params& params) const override { return params.vDeployments[id].nStartTime; }
     int64_t EndTime(const Consensus::Params& params) const override { return params.vDeployments[id].nTimeout; }
+    int MinActivationHeight(const Consensus::Params& params) const override { return params.vDeployments[id].min_activation_height; }
     int Period(const Consensus::Params& params) const override { return params.nMinerConfirmationWindow; }
     int Threshold(const Consensus::Params& params) const override { return params.nRuleChangeActivationThreshold; }
 
diff --git a/src/versionbits.h b/src/versionbits.h
index 6df1db88147..634a848ef58 100644
--- a/src/versionbits.h
+++ b/src/versionbits.h
@@ -25,7 +25,7 @@ static const int32_t VERSIONBITS_NUM_BITS = 29;
 enum class ThresholdState {
     DEFINED,   // First state that each softfork starts out as. The genesis block is by definition in this state for each deployment.
     STARTED,   // For blocks past the starttime.
-    LOCKED_IN, // For one retarget period after the first retarget period with STARTED blocks of which at least threshold have the associated bit set in nVersion.
+    LOCKED_IN, // For at least one retarget period after the first retarget period with STARTED blocks of which at least threshold have the associated bit set in nVersion, until min_activation_height is reached.
     ACTIVE,    // For all blocks after the LOCKED_IN retarget period (final state)
     FAILED,    // For all blocks once the first retarget period after the timeout time is hit, if LOCKED_IN wasn't already reached (final state)
 };
@@ -57,6 +57,7 @@ protected:
     virtual bool Condition(const CBlockIndex* pindex, const Consensus::Params& params) const =0;
     virtual int64_t BeginTime(const Consensus::Params& params) const =0;
     virtual int64_t EndTime(const Consensus::Params& params) const =0;
+    virtual int MinActivationHeight(const Consensus::Params& params) const { return 0; }
     virtual int Period(const Consensus::Params& params) const =0;
     virtual int Threshold(const Consensus::Params& params) const =0;
 
diff --git a/test/functional/rpc_blockchain.py b/test/functional/rpc_blockchain.py
index e0900302056..821409ccc14 100755
--- a/test/functional/rpc_blockchain.py
+++ b/test/functional/rpc_blockchain.py
@@ -149,6 +149,7 @@ class BlockchainTest(BitcoinTestFramework):
                         'count': 57,
                         'possible': True,
                     },
+                    'min_activation_height': 0,
                 },
                 'active': False
             },
@@ -158,7 +159,8 @@ class BlockchainTest(BitcoinTestFramework):
                     'status': 'active',
                     'start_time': -1,
                     'timeout': 9223372036854775807,
-                    'since': 0
+                    'since': 0,
+                    'min_activation_height': 0,
                 },
                 'height': 0,
                 'active': True

From dd85d5411c1702c8ae259610fe55050ba212e21e Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Wed, 7 Apr 2021 11:37:47 +1000
Subject: [PATCH 5/9] tests: test versionbits delayed activation

---
 src/test/versionbits_tests.cpp | 83 ++++++++++++++++++++++++++++++----
 1 file changed, 74 insertions(+), 9 deletions(-)

diff --git a/src/test/versionbits_tests.cpp b/src/test/versionbits_tests.cpp
index ae0d47b2b43..1ce31c9af90 100644
--- a/src/test/versionbits_tests.cpp
+++ b/src/test/versionbits_tests.cpp
@@ -44,6 +44,12 @@ public:
     int GetStateSinceHeightFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateSinceHeightFor(pindexPrev, paramsDummy, cache); }
 };
 
+class TestDelayedActivationConditionChecker : public TestConditionChecker
+{
+public:
+    int MinActivationHeight(const Consensus::Params& params) const override { return 15000; }
+};
+
 class TestAlwaysActiveConditionChecker : public TestConditionChecker
 {
 public:
@@ -68,6 +74,8 @@ class VersionBitsTester
     // The first one performs all checks, the second only 50%, the third only 25%, etc...
     // This is to test whether lack of cached information leads to the same results.
     TestConditionChecker checker[CHECKERS];
+    // Another 6 that assume delayed activation
+    TestDelayedActivationConditionChecker checker_delayed[CHECKERS];
     // Another 6 that assume always active activation
     TestAlwaysActiveConditionChecker checker_always[CHECKERS];
     // Another 6 that assume never active activation
@@ -77,14 +85,18 @@ class VersionBitsTester
     int num;
 
 public:
-    VersionBitsTester() : num(0) {}
+    VersionBitsTester() : num(1000) {}
 
     VersionBitsTester& Reset() {
+        // Have each group of tests be counted by the 1000s part, starting at 1000
+        num = num - (num % 1000) + 1000;
+
         for (unsigned int i = 0; i < vpblock.size(); i++) {
             delete vpblock[i];
         }
         for (unsigned int  i = 0; i < CHECKERS; i++) {
             checker[i] = TestConditionChecker();
+            checker_delayed[i] = TestDelayedActivationConditionChecker();
             checker_always[i] = TestAlwaysActiveConditionChecker();
             checker_never[i] = TestNeverActiveConditionChecker();
         }
@@ -109,11 +121,18 @@ public:
         return *this;
     }
 
-    VersionBitsTester& TestStateSinceHeight(int height) {
+    VersionBitsTester& TestStateSinceHeight(int height)
+    {
+        return TestStateSinceHeight(height, height);
+    }
+
+    VersionBitsTester& TestStateSinceHeight(int height, int height_delayed)
+    {
         const CBlockIndex* tip = Tip();
         for (int i = 0; i < CHECKERS; i++) {
             if (InsecureRandBits(i) == 0) {
                 BOOST_CHECK_MESSAGE(checker[i].GetStateSinceHeightFor(tip) == height, strprintf("Test %i for StateSinceHeight", num));
+                BOOST_CHECK_MESSAGE(checker_delayed[i].GetStateSinceHeightFor(tip) == height_delayed, strprintf("Test %i for StateSinceHeight (delayed)", num));
                 BOOST_CHECK_MESSAGE(checker_always[i].GetStateSinceHeightFor(tip) == 0, strprintf("Test %i for StateSinceHeight (always active)", num));
 
                 // never active may go from DEFINED -> FAILED at the first period
@@ -125,17 +144,31 @@ public:
         return *this;
     }
 
-    VersionBitsTester& TestState(ThresholdState exp) {
+    VersionBitsTester& TestState(ThresholdState exp)
+    {
+        return TestState(exp, exp);
+    }
+
+    VersionBitsTester& TestState(ThresholdState exp, ThresholdState exp_delayed)
+    {
+        if (exp != exp_delayed) {
+            // only expected differences are that delayed stays in locked_in longer
+            BOOST_CHECK_EQUAL(exp, ThresholdState::ACTIVE);
+            BOOST_CHECK_EQUAL(exp_delayed, ThresholdState::LOCKED_IN);
+        }
+
         const CBlockIndex* pindex = Tip();
         for (int i = 0; i < CHECKERS; i++) {
             if (InsecureRandBits(i) == 0) {
                 ThresholdState got = checker[i].GetStateFor(pindex);
+                ThresholdState got_delayed = checker_delayed[i].GetStateFor(pindex);
                 ThresholdState got_always = checker_always[i].GetStateFor(pindex);
                 ThresholdState got_never = checker_never[i].GetStateFor(pindex);
                 // nHeight of the next block. If vpblock is empty, the next (ie first)
                 // block should be the genesis block with nHeight == 0.
                 int height = pindex == nullptr ? 0 : pindex->nHeight + 1;
                 BOOST_CHECK_MESSAGE(got == exp, strprintf("Test %i for %s height %d (got %s)", num, StateName(exp), height, StateName(got)));
+                BOOST_CHECK_MESSAGE(got_delayed == exp_delayed, strprintf("Test %i for %s height %d (got %s; delayed case)", num, StateName(exp_delayed), height, StateName(got_delayed)));
                 BOOST_CHECK_MESSAGE(got_always == ThresholdState::ACTIVE, strprintf("Test %i for ACTIVE height %d (got %s; always active case)", num, height, StateName(got_always)));
                 BOOST_CHECK_MESSAGE(got_never == ThresholdState::DEFINED|| got_never == ThresholdState::FAILED, strprintf("Test %i for DEFINED/FAILED height %d (got %s; never active case)", num, height, StateName(got_never)));
             }
@@ -150,6 +183,9 @@ public:
     VersionBitsTester& TestActive() { return TestState(ThresholdState::ACTIVE); }
     VersionBitsTester& TestFailed() { return TestState(ThresholdState::FAILED); }
 
+    // non-delayed should be active; delayed should still be locked in
+    VersionBitsTester& TestActiveDelayed() { return TestState(ThresholdState::ACTIVE, ThresholdState::LOCKED_IN); }
+
     CBlockIndex* Tip() { return vpblock.empty() ? nullptr : vpblock.back(); }
 };
 
@@ -170,7 +206,6 @@ BOOST_AUTO_TEST_CASE(versionbits_test)
                            .Mine(2001, TestTime(30003), 0x100).TestFailed().TestStateSinceHeight(1000)
                            .Mine(2999, TestTime(30004), 0x100).TestFailed().TestStateSinceHeight(1000)
                            .Mine(3000, TestTime(30005), 0x100).TestFailed().TestStateSinceHeight(1000)
-
         // DEFINED -> STARTED -> FAILED
                            .Reset().TestDefined().TestStateSinceHeight(0)
                            .Mine(1, TestTime(1), 0).TestDefined().TestStateSinceHeight(0)
@@ -203,9 +238,10 @@ BOOST_AUTO_TEST_CASE(versionbits_test)
                            .Mine(2999, TestTime(19999), 0x200).TestStarted().TestStateSinceHeight(2000) // 49 old blocks
                            .Mine(3000, TestTime(29999), 0x200).TestLockedIn().TestStateSinceHeight(3000) // 1 old block (so 900 out of the past 1000)
                            .Mine(3999, TestTime(30001), 0).TestLockedIn().TestStateSinceHeight(3000)
-                           .Mine(4000, TestTime(30002), 0).TestActive().TestStateSinceHeight(4000)
-                           .Mine(14333, TestTime(30003), 0).TestActive().TestStateSinceHeight(4000)
-                           .Mine(24000, TestTime(40000), 0).TestActive().TestStateSinceHeight(4000)
+                           .Mine(4000, TestTime(30002), 0).TestActiveDelayed().TestStateSinceHeight(4000, 3000) // delayed will not become active until height=15000
+                           .Mine(14333, TestTime(30003), 0).TestActiveDelayed().TestStateSinceHeight(4000, 3000)
+                           .Mine(15000, TestTime(40000), 0).TestActive().TestStateSinceHeight(4000, 15000)
+                           .Mine(24000, TestTime(40000), 0).TestActive().TestStateSinceHeight(4000, 15000)
 
         // DEFINED multiple periods -> STARTED multiple periods -> FAILED
                            .Reset().TestDefined().TestStateSinceHeight(0)
@@ -216,7 +252,8 @@ BOOST_AUTO_TEST_CASE(versionbits_test)
                            .Mine(4000, TestTime(10000), 0).TestStarted().TestStateSinceHeight(3000)
                            .Mine(5000, TestTime(10000), 0).TestStarted().TestStateSinceHeight(3000)
                            .Mine(6000, TestTime(20000), 0).TestFailed().TestStateSinceHeight(6000)
-                           .Mine(7000, TestTime(20000), 0x100).TestFailed().TestStateSinceHeight(6000);
+                           .Mine(7000, TestTime(20000), 0x100).TestFailed().TestStateSinceHeight(6000)
+        ;
     }
 }
 
@@ -230,6 +267,13 @@ BOOST_AUTO_TEST_CASE(versionbits_sanity)
         // Make sure that no deployment tries to set an invalid bit.
         BOOST_CHECK_EQUAL(bitmask & ~(uint32_t)VERSIONBITS_TOP_MASK, bitmask);
 
+        // Check min_activation_height is on a retarget boundary
+        BOOST_CHECK_EQUAL(mainnetParams.vDeployments[i].min_activation_height % mainnetParams.nMinerConfirmationWindow, 0);
+        // Check min_activation_height is 0 for ALWAYS_ACTIVE and never active deployments
+        if (mainnetParams.vDeployments[i].nStartTime == Consensus::BIP9Deployment::ALWAYS_ACTIVE || mainnetParams.vDeployments[i].nTimeout <= 1230768000) {
+            BOOST_CHECK_EQUAL(mainnetParams.vDeployments[i].min_activation_height, 0);
+        }
+
         // Verify that the deployment windows of different deployment using the
         // same bit are disjoint.
         // This test may need modification at such time as a new deployment
@@ -268,7 +312,8 @@ static void check_computeblockversion(const Consensus::Params& params, Consensus
     BOOST_REQUIRE(nTimeout <= std::numeric_limits<uint32_t>::max() || nTimeout == Consensus::BIP9Deployment::NO_TIMEOUT);
     BOOST_REQUIRE(0 <= bit && bit < 32);
     BOOST_REQUIRE(((1 << bit) & VERSIONBITS_TOP_MASK) == 0);
-    BOOST_REQUIRE(min_activation_height == 0);
+    BOOST_REQUIRE(min_activation_height >= 0);
+    BOOST_REQUIRE_EQUAL(min_activation_height % params.nMinerConfirmationWindow, 0);
 
     // In the first chain, test that the bit is set by CBV until it has failed.
     // In the second chain, test the bit is set by CBV while STARTED and
@@ -378,6 +423,16 @@ static void check_computeblockversion(const Consensus::Params& params, Consensus
     lastBlock = secondChain.Mine((params.nMinerConfirmationWindow * 3) - 1, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
     BOOST_CHECK((ComputeBlockVersion(lastBlock, params) & (1 << bit)) != 0);
     lastBlock = secondChain.Mine(params.nMinerConfirmationWindow * 3, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
+
+    if (lastBlock->nHeight + 1 < min_activation_height) {
+        // check signalling continues while min_activation_height is not reached
+        lastBlock = secondChain.Mine(min_activation_height - 1, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
+        BOOST_CHECK((ComputeBlockVersion(lastBlock, params) & (1 << bit)) != 0);
+        // then reach min_activation_height, which was already REQUIRE'd to start a new period
+        lastBlock = secondChain.Mine(min_activation_height, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
+    }
+
+    // Check that we don't signal after activation
     BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, params) & (1<<bit), 0);
 }
 
@@ -391,6 +446,16 @@ BOOST_AUTO_TEST_CASE(versionbits_computeblockversion)
             check_computeblockversion(chainParams->GetConsensus(), static_cast<Consensus::DeploymentPos>(i));
         }
     }
+
+    {
+        // Use regtest/testdummy to ensure we always exercise the
+        // min_activation_height test, even if we're not using that in a
+        // live deployment
+        ArgsManager args;
+        args.ForceSetArg("-vbparams", "testdummy:1199145601:1230767999:403200"); // January 1, 2008 - December 31, 2008, min act height 403200
+        const auto chainParams = CreateChainParams(args, CBaseChainParams::REGTEST);
+        check_computeblockversion(chainParams->GetConsensus(), Consensus::DEPLOYMENT_TESTDUMMY);
+    }
 }
 
 BOOST_AUTO_TEST_SUITE_END()

From dd07e6da48040dc7eae46bc7941db48d98a669fd Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Tue, 16 Mar 2021 22:12:19 +1000
Subject: [PATCH 6/9] fuzz: test versionbits delayed activation

---
 src/test/fuzz/versionbits.cpp | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/src/test/fuzz/versionbits.cpp b/src/test/fuzz/versionbits.cpp
index 88c1a1a9cb8..11640d1ee90 100644
--- a/src/test/fuzz/versionbits.cpp
+++ b/src/test/fuzz/versionbits.cpp
@@ -29,14 +29,16 @@ public:
     const int64_t m_end;
     const int m_period;
     const int m_threshold;
+    const int m_min_activation_height;
     const int m_bit;
 
-    TestConditionChecker(int64_t begin, int64_t end, int period, int threshold, int bit)
-        : m_begin{begin}, m_end{end}, m_period{period}, m_threshold{threshold}, m_bit{bit}
+    TestConditionChecker(int64_t begin, int64_t end, int period, int threshold, int min_activation_height, int bit)
+        : m_begin{begin}, m_end{end}, m_period{period}, m_threshold{threshold}, m_min_activation_height{min_activation_height}, m_bit{bit}
     {
         assert(m_period > 0);
         assert(0 <= m_threshold && m_threshold <= m_period);
         assert(0 <= m_bit && m_bit < 32 && m_bit < VERSIONBITS_NUM_BITS);
+        assert(0 <= m_min_activation_height);
     }
 
     bool Condition(const CBlockIndex* pindex, const Consensus::Params& params) const override { return Condition(pindex->nVersion); }
@@ -44,6 +46,7 @@ public:
     int64_t EndTime(const Consensus::Params& params) const override { return m_end; }
     int Period(const Consensus::Params& params) const override { return m_period; }
     int Threshold(const Consensus::Params& params) const override { return m_threshold; }
+    int MinActivationHeight(const Consensus::Params& params) const override { return m_min_activation_height; }
 
     ThresholdState GetStateFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateFor(pindexPrev, dummy_params, m_cache); }
     int GetStateSinceHeightFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateSinceHeightFor(pindexPrev, dummy_params, m_cache); }
@@ -168,8 +171,9 @@ FUZZ_TARGET_INIT(versionbits, initialize)
             never_active_test = true;
         }
     }
+    int min_activation = fuzzed_data_provider.ConsumeIntegralInRange<int>(0, period * max_periods);
 
-    TestConditionChecker checker(start_time, timeout, period, threshold, bit);
+    TestConditionChecker checker(start_time, timeout, period, threshold, min_activation, bit);
 
     // Early exit if the versions don't signal sensibly for the deployment
     if (!checker.Condition(ver_signal)) return;
@@ -306,11 +310,16 @@ FUZZ_TARGET_INIT(versionbits, initialize)
         }
         break;
     case ThresholdState::LOCKED_IN:
-        assert(exp_state == ThresholdState::STARTED);
-        assert(current_block->GetMedianTimePast() < checker.m_end);
-        assert(blocks_sig >= threshold);
+        if (exp_state == ThresholdState::LOCKED_IN) {
+            assert(current_block->nHeight + 1 < min_activation);
+        } else {
+            assert(exp_state == ThresholdState::STARTED);
+            assert(current_block->GetMedianTimePast() < checker.m_end);
+            assert(blocks_sig >= threshold);
+        }
         break;
     case ThresholdState::ACTIVE:
+        assert(always_active_test || min_activation <= current_block->nHeight + 1);
         assert(exp_state == ThresholdState::ACTIVE || exp_state == ThresholdState::LOCKED_IN);
         break;
     case ThresholdState::FAILED:

From 55ac5f568a3b73d6f1ef4654617fb76e8bcbccdf Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Sat, 27 Mar 2021 23:00:14 +1000
Subject: [PATCH 7/9] versionbits: Add explicit NEVER_ACTIVE deployments

Previously we used deployments that would timeout prior to Bitcoin's
invention, which allowed the deployment to still be activated in unit
tests. This switches those deployments to be truly never active.
---
 src/chainparams.cpp            | 20 ++++++++++----------
 src/consensus/params.h         |  5 +++++
 src/rpc/blockchain.cpp         |  6 ++----
 src/test/fuzz/versionbits.cpp  | 27 ++++++++++-----------------
 src/test/versionbits_tests.cpp | 24 +++++++++++++++---------
 src/versionbits.cpp            |  7 ++++++-
 6 files changed, 48 insertions(+), 41 deletions(-)

diff --git a/src/chainparams.cpp b/src/chainparams.cpp
index 7e2c2ac7cf6..318b5c9a713 100644
--- a/src/chainparams.cpp
+++ b/src/chainparams.cpp
@@ -81,14 +81,14 @@ public:
         consensus.nRuleChangeActivationThreshold = 1916; // 95% of 2016
         consensus.nMinerConfirmationWindow = 2016; // nPowTargetTimespan / nPowTargetSpacing
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].bit = 28;
-        consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nStartTime = 1199145601; // January 1, 2008
-        consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = 1230767999; // December 31, 2008
+        consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nStartTime = Consensus::BIP9Deployment::NEVER_ACTIVE;
+        consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT;
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].min_activation_height = 0; // No activation delay
 
         // Deployment of Taproot (BIPs 340-342)
         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].bit = 2;
-        consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nStartTime = 1199145601; // January 1, 2008
-        consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nTimeout = 1230767999; // December 31, 2008
+        consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nStartTime = Consensus::BIP9Deployment::NEVER_ACTIVE;
+        consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT;
         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].min_activation_height = 0; // No activation delay
 
         consensus.nMinimumChainWork = uint256S("0x00000000000000000000000000000000000000001533efd8d716a517fe2c5008");
@@ -200,14 +200,14 @@ public:
         consensus.nRuleChangeActivationThreshold = 1512; // 75% for testchains
         consensus.nMinerConfirmationWindow = 2016; // nPowTargetTimespan / nPowTargetSpacing
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].bit = 28;
-        consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nStartTime = 1199145601; // January 1, 2008
-        consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = 1230767999; // December 31, 2008
+        consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nStartTime = Consensus::BIP9Deployment::NEVER_ACTIVE;
+        consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT;
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].min_activation_height = 0; // No activation delay
 
         // Deployment of Taproot (BIPs 340-342)
         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].bit = 2;
-        consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nStartTime = 1199145601; // January 1, 2008
-        consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nTimeout = 1230767999; // December 31, 2008
+        consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nStartTime = Consensus::BIP9Deployment::NEVER_ACTIVE;
+        consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT;
         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].min_activation_height = 0; // No activation delay
 
         consensus.nMinimumChainWork = uint256S("0x0000000000000000000000000000000000000000000001db6ec4ac88cf2272c6");
@@ -337,8 +337,8 @@ public:
         consensus.MinBIP9WarningHeight = 0;
         consensus.powLimit = uint256S("00000377ae000000000000000000000000000000000000000000000000000000");
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].bit = 28;
-        consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nStartTime = 1199145601; // January 1, 2008
-        consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = 1230767999; // December 31, 2008
+        consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nStartTime = Consensus::BIP9Deployment::NEVER_ACTIVE;
+        consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT;
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].min_activation_height = 0; // No activation delay
 
         // Activation of Taproot (BIPs 340-342)
diff --git a/src/consensus/params.h b/src/consensus/params.h
index 705205c95b0..28c95e08841 100644
--- a/src/consensus/params.h
+++ b/src/consensus/params.h
@@ -43,6 +43,11 @@ struct BIP9Deployment {
      *  process (which takes at least 3 BIP9 intervals). Only tests that specifically test the
      *  behaviour during activation cannot use this. */
     static constexpr int64_t ALWAYS_ACTIVE = -1;
+
+    /** Special value for nStartTime indicating that the deployment is never active.
+     *  This is useful for integrating the code changes for a new feature
+     *  prior to deploying it on some or all networks. */
+    static constexpr int64_t NEVER_ACTIVE = -2;
 };
 
 /**
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index acf2f5d6b48..1130a384e53 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -1225,10 +1225,8 @@ static void BuriedForkDescPushBack(UniValue& softforks, const std::string &name,
 static void BIP9SoftForkDescPushBack(UniValue& softforks, const std::string &name, const Consensus::Params& consensusParams, Consensus::DeploymentPos id) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 {
     // For BIP9 deployments.
-    // Deployments (e.g. testdummy) with timeout value before Jan 1, 2009 are hidden.
-    // A timeout value of 0 guarantees a softfork will never be activated.
-    // This is used when merging logic to implement a proposed softfork without a specified deployment schedule.
-    if (consensusParams.vDeployments[id].nTimeout <= 1230768000) return;
+    // Deployments that are never active are hidden.
+    if (consensusParams.vDeployments[id].nStartTime == Consensus::BIP9Deployment::NEVER_ACTIVE) return;
 
     UniValue bip9(UniValue::VOBJ);
     const ThresholdState thresholdState = VersionBitsState(::ChainActive().Tip(), consensusParams, id, versionbitscache);
diff --git a/src/test/fuzz/versionbits.cpp b/src/test/fuzz/versionbits.cpp
index 11640d1ee90..41773232336 100644
--- a/src/test/fuzz/versionbits.cpp
+++ b/src/test/fuzz/versionbits.cpp
@@ -163,13 +163,12 @@ FUZZ_TARGET_INIT(versionbits, initialize)
     } else {
         if (fuzzed_data_provider.ConsumeBool()) {
             start_time = Consensus::BIP9Deployment::ALWAYS_ACTIVE;
-            timeout = Consensus::BIP9Deployment::NO_TIMEOUT;
             always_active_test = true;
         } else {
-            start_time = 1199145601; // January 1, 2008
-            timeout = 1230767999;    // December 31, 2008
+            start_time = Consensus::BIP9Deployment::NEVER_ACTIVE;
             never_active_test = true;
         }
+        timeout = fuzzed_data_provider.ConsumeBool() ? Consensus::BIP9Deployment::NO_TIMEOUT : fuzzed_data_provider.ConsumeIntegral<int64_t>();
     }
     int min_activation = fuzzed_data_provider.ConsumeIntegralInRange<int>(0, period * max_periods);
 
@@ -323,7 +322,7 @@ FUZZ_TARGET_INIT(versionbits, initialize)
         assert(exp_state == ThresholdState::ACTIVE || exp_state == ThresholdState::LOCKED_IN);
         break;
     case ThresholdState::FAILED:
-        assert(current_block->GetMedianTimePast() >= checker.m_end);
+        assert(never_active_test || current_block->GetMedianTimePast() >= checker.m_end);
         assert(exp_state != ThresholdState::LOCKED_IN && exp_state != ThresholdState::ACTIVE);
         break;
     default:
@@ -335,26 +334,20 @@ FUZZ_TARGET_INIT(versionbits, initialize)
         assert(state == ThresholdState::ACTIVE || state == ThresholdState::FAILED);
     }
 
-    // "always active" has additional restrictions
     if (always_active_test) {
+        // "always active" has additional restrictions
         assert(state == ThresholdState::ACTIVE);
         assert(exp_state == ThresholdState::ACTIVE);
         assert(since == 0);
+    } else if (never_active_test) {
+        // "never active" does too
+        assert(state == ThresholdState::FAILED);
+        assert(exp_state == ThresholdState::FAILED);
+        assert(since == 0);
     } else {
-        // except for always active, the initial state is always DEFINED
+        // for signalled deployments, the initial state is always DEFINED
         assert(since > 0 || state == ThresholdState::DEFINED);
         assert(exp_since > 0 || exp_state == ThresholdState::DEFINED);
     }
-
-    // "never active" does too
-    if (never_active_test) {
-        assert(state == ThresholdState::FAILED);
-        assert(since == period);
-        if (exp_since == 0) {
-            assert(exp_state == ThresholdState::DEFINED);
-        } else {
-            assert(exp_state == ThresholdState::FAILED);
-        }
-    }
 }
 } // namespace
diff --git a/src/test/versionbits_tests.cpp b/src/test/versionbits_tests.cpp
index 1ce31c9af90..80072eff9c5 100644
--- a/src/test/versionbits_tests.cpp
+++ b/src/test/versionbits_tests.cpp
@@ -59,8 +59,7 @@ public:
 class TestNeverActiveConditionChecker : public TestConditionChecker
 {
 public:
-    int64_t BeginTime(const Consensus::Params& params) const override { return 0; }
-    int64_t EndTime(const Consensus::Params& params) const override { return 1230768000; }
+    int64_t BeginTime(const Consensus::Params& params) const override { return Consensus::BIP9Deployment::NEVER_ACTIVE; }
 };
 
 #define CHECKERS 6
@@ -134,10 +133,7 @@ public:
                 BOOST_CHECK_MESSAGE(checker[i].GetStateSinceHeightFor(tip) == height, strprintf("Test %i for StateSinceHeight", num));
                 BOOST_CHECK_MESSAGE(checker_delayed[i].GetStateSinceHeightFor(tip) == height_delayed, strprintf("Test %i for StateSinceHeight (delayed)", num));
                 BOOST_CHECK_MESSAGE(checker_always[i].GetStateSinceHeightFor(tip) == 0, strprintf("Test %i for StateSinceHeight (always active)", num));
-
-                // never active may go from DEFINED -> FAILED at the first period
-                const auto never_height = checker_never[i].GetStateSinceHeightFor(tip);
-                BOOST_CHECK_MESSAGE(never_height == 0 || never_height == checker_never[i].Period(paramsDummy), strprintf("Test %i for StateSinceHeight (never active)", num));
+                BOOST_CHECK_MESSAGE(checker_never[i].GetStateSinceHeightFor(tip) == 0, strprintf("Test %i for StateSinceHeight (never active)", num));
             }
         }
         num++;
@@ -170,7 +166,7 @@ public:
                 BOOST_CHECK_MESSAGE(got == exp, strprintf("Test %i for %s height %d (got %s)", num, StateName(exp), height, StateName(got)));
                 BOOST_CHECK_MESSAGE(got_delayed == exp_delayed, strprintf("Test %i for %s height %d (got %s; delayed case)", num, StateName(exp_delayed), height, StateName(got_delayed)));
                 BOOST_CHECK_MESSAGE(got_always == ThresholdState::ACTIVE, strprintf("Test %i for ACTIVE height %d (got %s; always active case)", num, height, StateName(got_always)));
-                BOOST_CHECK_MESSAGE(got_never == ThresholdState::DEFINED|| got_never == ThresholdState::FAILED, strprintf("Test %i for DEFINED/FAILED height %d (got %s; never active case)", num, height, StateName(got_never)));
+                BOOST_CHECK_MESSAGE(got_never == ThresholdState::FAILED, strprintf("Test %i for FAILED height %d (got %s; never active case)", num, height, StateName(got_never)));
             }
         }
         num++;
@@ -270,7 +266,7 @@ BOOST_AUTO_TEST_CASE(versionbits_sanity)
         // Check min_activation_height is on a retarget boundary
         BOOST_CHECK_EQUAL(mainnetParams.vDeployments[i].min_activation_height % mainnetParams.nMinerConfirmationWindow, 0);
         // Check min_activation_height is 0 for ALWAYS_ACTIVE and never active deployments
-        if (mainnetParams.vDeployments[i].nStartTime == Consensus::BIP9Deployment::ALWAYS_ACTIVE || mainnetParams.vDeployments[i].nTimeout <= 1230768000) {
+        if (mainnetParams.vDeployments[i].nStartTime == Consensus::BIP9Deployment::ALWAYS_ACTIVE || mainnetParams.vDeployments[i].nStartTime == Consensus::BIP9Deployment::NEVER_ACTIVE) {
             BOOST_CHECK_EQUAL(mainnetParams.vDeployments[i].min_activation_height, 0);
         }
 
@@ -304,8 +300,9 @@ static void check_computeblockversion(const Consensus::Params& params, Consensus
     // should not be any signalling for first block
     BOOST_CHECK_EQUAL(ComputeBlockVersion(nullptr, params), VERSIONBITS_TOP_BITS);
 
-    // always active deployments shouldn't need to be tested further
+    // always/never active deployments shouldn't need to be tested further
     if (nStartTime == Consensus::BIP9Deployment::ALWAYS_ACTIVE) return;
+    if (nStartTime == Consensus::BIP9Deployment::NEVER_ACTIVE) return;
 
     BOOST_REQUIRE(nStartTime < nTimeout);
     BOOST_REQUIRE(nStartTime >= 0);
@@ -447,6 +444,15 @@ BOOST_AUTO_TEST_CASE(versionbits_computeblockversion)
         }
     }
 
+    {
+        // Use regtest/testdummy to ensure we always exercise some
+        // deployment that's not always/never active
+        ArgsManager args;
+        args.ForceSetArg("-vbparams", "testdummy:1199145601:1230767999"); // January 1, 2008 - December 31, 2008
+        const auto chainParams = CreateChainParams(args, CBaseChainParams::REGTEST);
+        check_computeblockversion(chainParams->GetConsensus(), Consensus::DEPLOYMENT_TESTDUMMY);
+    }
+
     {
         // Use regtest/testdummy to ensure we always exercise the
         // min_activation_height test, even if we're not using that in a
diff --git a/src/versionbits.cpp b/src/versionbits.cpp
index 11da729596f..df666c963fa 100644
--- a/src/versionbits.cpp
+++ b/src/versionbits.cpp
@@ -18,6 +18,11 @@ ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex*
         return ThresholdState::ACTIVE;
     }
 
+    // Check if this deployment is never active.
+    if (nTimeStart == Consensus::BIP9Deployment::NEVER_ACTIVE) {
+        return ThresholdState::FAILED;
+    }
+
     // A block's state is always the same as that of the first of its period, so it is computed based on a pindexPrev whose height equals a multiple of nPeriod - 1.
     if (pindexPrev != nullptr) {
         pindexPrev = pindexPrev->GetAncestor(pindexPrev->nHeight - ((pindexPrev->nHeight + 1) % nPeriod));
@@ -129,7 +134,7 @@ BIP9Stats AbstractThresholdConditionChecker::GetStateStatisticsFor(const CBlockI
 int AbstractThresholdConditionChecker::GetStateSinceHeightFor(const CBlockIndex* pindexPrev, const Consensus::Params& params, ThresholdConditionCache& cache) const
 {
     int64_t start_time = BeginTime(params);
-    if (start_time == Consensus::BIP9Deployment::ALWAYS_ACTIVE) {
+    if (start_time == Consensus::BIP9Deployment::ALWAYS_ACTIVE || start_time == Consensus::BIP9Deployment::NEVER_ACTIVE) {
         return 0;
     }
 

From f054f6bcd2c2ce5fea84cf8681013f85a444e7ea Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Wed, 7 Apr 2021 10:20:46 +1000
Subject: [PATCH 8/9] versionbits: simplify state transitions

This removes the DEFINED->FAILED transition and changes the
STARTED->FAILED transition to only occur if signalling didn't pass the
threshold. This ensures that it is always possible for activation to
occur, no matter what settings are chosen, or the speed at which blocks
are found.
---
 src/test/fuzz/versionbits.cpp  | 17 +++++++----------
 src/test/versionbits_tests.cpp | 34 +++++++++++++++++++---------------
 src/versionbits.cpp            | 10 +++-------
 3 files changed, 29 insertions(+), 32 deletions(-)

diff --git a/src/test/fuzz/versionbits.cpp b/src/test/fuzz/versionbits.cpp
index 41773232336..91868218362 100644
--- a/src/test/fuzz/versionbits.cpp
+++ b/src/test/fuzz/versionbits.cpp
@@ -147,19 +147,14 @@ FUZZ_TARGET_INIT(versionbits, initialize)
         // pick the timestamp to switch based on a block
         // note states will change *after* these blocks because mediantime lags
         int start_block = fuzzed_data_provider.ConsumeIntegralInRange<int>(0, period * (max_periods - 3));
-        int end_block = fuzzed_data_provider.ConsumeIntegralInRange<int>(start_block, period * (max_periods - 3));
+        int end_block = fuzzed_data_provider.ConsumeIntegralInRange<int>(0, period * (max_periods - 3));
 
         start_time = block_start_time + start_block * interval;
         timeout = block_start_time + end_block * interval;
 
-        assert(start_time <= timeout);
-
         // allow for times to not exactly match a block
         if (fuzzed_data_provider.ConsumeBool()) start_time += interval / 2;
         if (fuzzed_data_provider.ConsumeBool()) timeout += interval / 2;
-
-        // this may make timeout too early; if so, don't run the test
-        if (start_time > timeout) return;
     } else {
         if (fuzzed_data_provider.ConsumeBool()) {
             start_time = Consensus::BIP9Deployment::ALWAYS_ACTIVE;
@@ -297,13 +292,12 @@ FUZZ_TARGET_INIT(versionbits, initialize)
         assert(since == 0);
         assert(exp_state == ThresholdState::DEFINED);
         assert(current_block->GetMedianTimePast() < checker.m_begin);
-        assert(current_block->GetMedianTimePast() < checker.m_end);
         break;
     case ThresholdState::STARTED:
         assert(current_block->GetMedianTimePast() >= checker.m_begin);
-        assert(current_block->GetMedianTimePast() < checker.m_end);
         if (exp_state == ThresholdState::STARTED) {
             assert(blocks_sig < threshold);
+            assert(current_block->GetMedianTimePast() < checker.m_end);
         } else {
             assert(exp_state == ThresholdState::DEFINED);
         }
@@ -313,7 +307,6 @@ FUZZ_TARGET_INIT(versionbits, initialize)
             assert(current_block->nHeight + 1 < min_activation);
         } else {
             assert(exp_state == ThresholdState::STARTED);
-            assert(current_block->GetMedianTimePast() < checker.m_end);
             assert(blocks_sig >= threshold);
         }
         break;
@@ -323,7 +316,11 @@ FUZZ_TARGET_INIT(versionbits, initialize)
         break;
     case ThresholdState::FAILED:
         assert(never_active_test || current_block->GetMedianTimePast() >= checker.m_end);
-        assert(exp_state != ThresholdState::LOCKED_IN && exp_state != ThresholdState::ACTIVE);
+        if (exp_state == ThresholdState::STARTED) {
+            assert(blocks_sig < threshold);
+        } else {
+            assert(exp_state == ThresholdState::FAILED);
+        }
         break;
     default:
         assert(false);
diff --git a/src/test/versionbits_tests.cpp b/src/test/versionbits_tests.cpp
index 80072eff9c5..05838ec19a2 100644
--- a/src/test/versionbits_tests.cpp
+++ b/src/test/versionbits_tests.cpp
@@ -190,18 +190,20 @@ BOOST_FIXTURE_TEST_SUITE(versionbits_tests, TestingSetup)
 BOOST_AUTO_TEST_CASE(versionbits_test)
 {
     for (int i = 0; i < 64; i++) {
-        // DEFINED -> FAILED
+        // DEFINED -> STARTED after timeout reached -> FAILED
         VersionBitsTester().TestDefined().TestStateSinceHeight(0)
                            .Mine(1, TestTime(1), 0x100).TestDefined().TestStateSinceHeight(0)
                            .Mine(11, TestTime(11), 0x100).TestDefined().TestStateSinceHeight(0)
                            .Mine(989, TestTime(989), 0x100).TestDefined().TestStateSinceHeight(0)
-                           .Mine(999, TestTime(20000), 0x100).TestDefined().TestStateSinceHeight(0)
-                           .Mine(1000, TestTime(20000), 0x100).TestFailed().TestStateSinceHeight(1000)
-                           .Mine(1999, TestTime(30001), 0x100).TestFailed().TestStateSinceHeight(1000)
-                           .Mine(2000, TestTime(30002), 0x100).TestFailed().TestStateSinceHeight(1000)
-                           .Mine(2001, TestTime(30003), 0x100).TestFailed().TestStateSinceHeight(1000)
-                           .Mine(2999, TestTime(30004), 0x100).TestFailed().TestStateSinceHeight(1000)
-                           .Mine(3000, TestTime(30005), 0x100).TestFailed().TestStateSinceHeight(1000)
+                           .Mine(999, TestTime(20000), 0x100).TestDefined().TestStateSinceHeight(0) // Timeout and start time reached simultaneously
+                           .Mine(1000, TestTime(20000), 0).TestStarted().TestStateSinceHeight(1000) // Hit started, stop signalling
+                           .Mine(1999, TestTime(30001), 0).TestStarted().TestStateSinceHeight(1000)
+                           .Mine(2000, TestTime(30002), 0x100).TestFailed().TestStateSinceHeight(2000) // Hit failed, start signalling again
+                           .Mine(2001, TestTime(30003), 0x100).TestFailed().TestStateSinceHeight(2000)
+                           .Mine(2999, TestTime(30004), 0x100).TestFailed().TestStateSinceHeight(2000)
+                           .Mine(3000, TestTime(30005), 0x100).TestFailed().TestStateSinceHeight(2000)
+                           .Mine(4000, TestTime(30006), 0x100).TestFailed().TestStateSinceHeight(2000)
+
         // DEFINED -> STARTED -> FAILED
                            .Reset().TestDefined().TestStateSinceHeight(0)
                            .Mine(1, TestTime(1), 0).TestDefined().TestStateSinceHeight(0)
@@ -212,19 +214,19 @@ BOOST_AUTO_TEST_CASE(versionbits_test)
                            .Mine(3000, TestTime(20000), 0).TestFailed().TestStateSinceHeight(3000) // 50 old blocks (so 899 out of the past 1000)
                            .Mine(4000, TestTime(20010), 0x100).TestFailed().TestStateSinceHeight(3000)
 
-        // DEFINED -> STARTED -> FAILED while threshold reached
+        // DEFINED -> STARTED -> LOCKEDIN after timeout reached -> ACTIVE
                            .Reset().TestDefined().TestStateSinceHeight(0)
                            .Mine(1, TestTime(1), 0).TestDefined().TestStateSinceHeight(0)
                            .Mine(1000, TestTime(10000) - 1, 0x101).TestDefined().TestStateSinceHeight(0) // One second more and it would be defined
                            .Mine(2000, TestTime(10000), 0x101).TestStarted().TestStateSinceHeight(2000) // So that's what happens the next period
                            .Mine(2999, TestTime(30000), 0x100).TestStarted().TestStateSinceHeight(2000) // 999 new blocks
-                           .Mine(3000, TestTime(30000), 0x100).TestFailed().TestStateSinceHeight(3000) // 1 new block (so 1000 out of the past 1000 are new)
-                           .Mine(3999, TestTime(30001), 0).TestFailed().TestStateSinceHeight(3000)
-                           .Mine(4000, TestTime(30002), 0).TestFailed().TestStateSinceHeight(3000)
-                           .Mine(14333, TestTime(30003), 0).TestFailed().TestStateSinceHeight(3000)
-                           .Mine(24000, TestTime(40000), 0).TestFailed().TestStateSinceHeight(3000)
+                           .Mine(3000, TestTime(30000), 0x100).TestLockedIn().TestStateSinceHeight(3000) // 1 new block (so 1000 out of the past 1000 are new)
+                           .Mine(3999, TestTime(30001), 0).TestLockedIn().TestStateSinceHeight(3000)
+                           .Mine(4000, TestTime(30002), 0).TestActiveDelayed().TestStateSinceHeight(4000, 3000)
+                           .Mine(14333, TestTime(30003), 0).TestActiveDelayed().TestStateSinceHeight(4000, 3000)
+                           .Mine(24000, TestTime(40000), 0).TestActive().TestStateSinceHeight(4000, 15000)
 
-        // DEFINED -> STARTED -> LOCKEDIN at the last minute -> ACTIVE
+        // DEFINED -> STARTED -> LOCKEDIN before timeout -> ACTIVE
                            .Reset().TestDefined()
                            .Mine(1, TestTime(1), 0).TestDefined().TestStateSinceHeight(0)
                            .Mine(1000, TestTime(10000) - 1, 0x101).TestDefined().TestStateSinceHeight(0) // One second more and it would be defined
@@ -247,8 +249,10 @@ BOOST_AUTO_TEST_CASE(versionbits_test)
                            .Mine(3000, TestTime(10000), 0).TestStarted().TestStateSinceHeight(3000)
                            .Mine(4000, TestTime(10000), 0).TestStarted().TestStateSinceHeight(3000)
                            .Mine(5000, TestTime(10000), 0).TestStarted().TestStateSinceHeight(3000)
+                           .Mine(5999, TestTime(20000), 0).TestStarted().TestStateSinceHeight(3000)
                            .Mine(6000, TestTime(20000), 0).TestFailed().TestStateSinceHeight(6000)
                            .Mine(7000, TestTime(20000), 0x100).TestFailed().TestStateSinceHeight(6000)
+                           .Mine(24000, TestTime(20000), 0x100).TestFailed().TestStateSinceHeight(6000) // stay in FAILED no matter how much we signal
         ;
     }
 }
diff --git a/src/versionbits.cpp b/src/versionbits.cpp
index df666c963fa..df2ec4e056e 100644
--- a/src/versionbits.cpp
+++ b/src/versionbits.cpp
@@ -57,18 +57,12 @@ ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex*
 
         switch (state) {
             case ThresholdState::DEFINED: {
-                if (pindexPrev->GetMedianTimePast() >= nTimeTimeout) {
-                    stateNext = ThresholdState::FAILED;
-                } else if (pindexPrev->GetMedianTimePast() >= nTimeStart) {
+                if (pindexPrev->GetMedianTimePast() >= nTimeStart) {
                     stateNext = ThresholdState::STARTED;
                 }
                 break;
             }
             case ThresholdState::STARTED: {
-                if (pindexPrev->GetMedianTimePast() >= nTimeTimeout) {
-                    stateNext = ThresholdState::FAILED;
-                    break;
-                }
                 // We need to count
                 const CBlockIndex* pindexCount = pindexPrev;
                 int count = 0;
@@ -80,6 +74,8 @@ ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex*
                 }
                 if (count >= nThreshold) {
                     stateNext = ThresholdState::LOCKED_IN;
+                } else if (pindexPrev->GetMedianTimePast() >= nTimeTimeout) {
+                    stateNext = ThresholdState::FAILED;
                 }
                 break;
             }

From ffe33dfbd4c3b11e3475b022b6c1dd077613de79 Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Sat, 6 Mar 2021 18:42:58 +1000
Subject: [PATCH 9/9] chainparams: drop versionbits threshold to 90% for
 mainnnet and signet

---
 src/chainparams.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/chainparams.cpp b/src/chainparams.cpp
index 318b5c9a713..0656b099a91 100644
--- a/src/chainparams.cpp
+++ b/src/chainparams.cpp
@@ -78,7 +78,7 @@ public:
         consensus.nPowTargetSpacing = 10 * 60;
         consensus.fPowAllowMinDifficultyBlocks = false;
         consensus.fPowNoRetargeting = false;
-        consensus.nRuleChangeActivationThreshold = 1916; // 95% of 2016
+        consensus.nRuleChangeActivationThreshold = 1815; // 90% of 2016
         consensus.nMinerConfirmationWindow = 2016; // nPowTargetTimespan / nPowTargetSpacing
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].bit = 28;
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nStartTime = Consensus::BIP9Deployment::NEVER_ACTIVE;
@@ -332,7 +332,7 @@ public:
         consensus.nPowTargetSpacing = 10 * 60;
         consensus.fPowAllowMinDifficultyBlocks = false;
         consensus.fPowNoRetargeting = false;
-        consensus.nRuleChangeActivationThreshold = 1916; // 95% of 2016
+        consensus.nRuleChangeActivationThreshold = 1815; // 90% of 2016
         consensus.nMinerConfirmationWindow = 2016; // nPowTargetTimespan / nPowTargetSpacing
         consensus.MinBIP9WarningHeight = 0;
         consensus.powLimit = uint256S("00000377ae000000000000000000000000000000000000000000000000000000");