Merge #21377: Speedy trial support for versionbits

ffe33dfbd4c3b11e3475b022b6c1dd077613de79 chainparams: drop versionbits threshold to 90% for mainnnet and signet (Anthony Towns)
f054f6bcd2c2ce5fea84cf8681013f85a444e7ea versionbits: simplify state transitions (Anthony Towns)
55ac5f568a3b73d6f1ef4654617fb76e8bcbccdf versionbits: Add explicit NEVER_ACTIVE deployments (Anthony Towns)
dd07e6da48040dc7eae46bc7941db48d98a669fd fuzz: test versionbits delayed activation (Anthony Towns)
dd85d5411c1702c8ae259610fe55050ba212e21e tests: test versionbits delayed activation (Anthony Towns)
73d4a706393e6dbd6b6d6b6428f8d3233ac0a2d8 versionbits: Add support for delayed activation (Anthony Towns)
9e6b65f6fa205eee5c3b99343988adcb8d320460 tests: clean up versionbits test (Anthony Towns)
593274445004506c921d5d851361aefb3434d744 tests: test ComputeBlockVersion for all deployments (Anthony Towns)
63879f0a4760c0c0f784029849cb5d21ee088abb tests: pull ComputeBlockVersion test into its own function (Anthony Towns)

Pull request description:

  BIP9-based implementation of "speedy trial" activation specification, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-March/018583.html

  Edge cases are tested by fuzzing added in #21380.

ACKs for top commit:
  instagibbs:
    tACK https://github.com/bitcoin/bitcoin/pull/21377/commits/ffe33dfbd4c3b11e3475b022b6c1dd077613de79
  jnewbery:
    utACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
  MarcoFalke:
    review ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79 💈
  achow101:
    re-ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
  gmaxwell:
    ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
  benthecarman:
    ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
  Sjors:
    ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
  jonatack:
    Initial approach ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79 after a first pass of review, building and testing each commit, mostly looking at the changes and diffs. Will do a more high-level review iteration. A few minor comments follow to pick/choose/ignore.
  ariard:
    Code Review ACK ffe33df

Tree-SHA512: f79a7146b2450057ee92155cbbbcec12cd64334236d9239c6bd7d31b32eec145a9781c320f178da7b44ababdb8808b84d9d22a40e0851e229ba6d224e3be747c

2 files changed, 244 insertions, 124 deletions

diff --git a/src/test/fuzz/versionbits.cpp b/src/test/fuzz/versionbits.cpp
index 88c1a1a9cb..9186821836 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); }
@@ -144,32 +147,27 @@ 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;
-            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);
 
-    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;
@@ -294,28 +292,35 @@ 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);
         }
         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(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:
-        assert(current_block->GetMedianTimePast() >= checker.m_end);
-        assert(exp_state != ThresholdState::LOCKED_IN && exp_state != ThresholdState::ACTIVE);
+        assert(never_active_test || current_block->GetMedianTimePast() >= checker.m_end);
+        if (exp_state == ThresholdState::STARTED) {
+            assert(blocks_sig < threshold);
+        } else {
+            assert(exp_state == ThresholdState::FAILED);
+        }
         break;
     default:
         assert(false);
@@ -326,26 +331,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 8841a540f2..05838ec19a 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:
@@ -53,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
@@ -68,6 +73,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 +84,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();
         }
@@ -100,7 +111,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();
@@ -109,34 +120,53 @@ 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(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));
-
-                // never active may go from DEFINED -> FAILED at the first period
-                const auto never_height = checker_never[i].GetStateSinceHeightFor(vpblock.empty() ? nullptr : vpblock.back());
-                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[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));
+                BOOST_CHECK_MESSAGE(checker_never[i].GetStateSinceHeightFor(tip) == 0, strprintf("Test %i for StateSinceHeight (never active)", num));
             }
         }
         num++;
         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) {
-                const CBlockIndex* pindex = vpblock.empty() ? nullptr : vpblock.back();
                 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)));
+                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++;
@@ -149,7 +179,10 @@ public:
     VersionBitsTester& TestActive() { return TestState(ThresholdState::ACTIVE); }
     VersionBitsTester& TestFailed() { return TestState(ThresholdState::FAILED); }
 
-    CBlockIndex * Tip() { return vpblock.size() ? vpblock.back() : nullptr; }
+    // 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(); }
 };
 
 BOOST_FIXTURE_TEST_SUITE(versionbits_tests, TestingSetup)
@@ -157,18 +190,19 @@ 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)
@@ -180,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
@@ -202,9 +236,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)
@@ -214,10 +249,16 @@ 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(7000, TestTime(20000), 0x100).TestFailed().TestStateSinceHeight(6000)
+                           .Mine(24000, TestTime(20000), 0x100).TestFailed().TestStateSinceHeight(6000) // stay in FAILED no matter how much we signal
+        ;
     }
+}
 
+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();
@@ -226,6 +267,13 @@ BOOST_AUTO_TEST_CASE(versionbits_test)
         // 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].nStartTime == Consensus::BIP9Deployment::NEVER_ACTIVE) {
+            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
@@ -242,81 +290,118 @@ 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();
+
+    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);
 
-    // 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;
+    // 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;
 
-    assert(nStartTime < nTimeout);
+    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);
+    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
     // 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;
+
+    const CBlockIndex *lastBlock = nullptr;
 
     // 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);
-
-    // 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();
-        // 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);
-    }
-    // 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);
+    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;
+
+        // 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(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;
     }
 
-    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++) {
+    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, mainnetParams) & (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, mainnetParams) & (1<<bit), 0);
 
     // On a new chain:
     // verify that the bit will be set after lock-in, and then stop being set
@@ -325,26 +410,62 @@ 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);
-
-    // 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);
+    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);
 }
 
+BOOST_AUTO_TEST_CASE(versionbits_computeblockversion)
+{
+    // 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));
+        }
+    }
+
+    {
+        // 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
+        // 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()