tests: refactor versionbits fuzz test

Test `VersionBitsConditionChecker` behaviour directly, rather than
reimplementing it, thus slightly improving fuzz test coverage of the
real code.

Author: ajtowns

src/test/fuzz/versionbits.cpp

@@ -21,43 +21,22 @@
 #include <vector>
 
 namespace {
-class TestConditionChecker : public AbstractThresholdConditionChecker
+class TestConditionChecker : public VersionBitsConditionChecker
 {
 private:
     mutable ThresholdConditionCache m_cache;
 
 public:
-    const int64_t m_begin;
-    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 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}
+    TestConditionChecker(const Consensus::BIP9Deployment& dep) : VersionBitsConditionChecker{dep}
     {
-        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);
+        assert(dep.period > 0);
+        assert(dep.threshold <= dep.period);
+        assert(0 <= dep.bit && dep.bit < 32 && dep.bit < VERSIONBITS_NUM_BITS);
+        assert(0 <= dep.min_activation_height);
     }
 
-    bool Condition(const CBlockIndex* pindex) const override { return Condition(pindex->nVersion); }
-    int64_t BeginTime() const override { return m_begin; }
-    int64_t EndTime() const override { return m_end; }
-    int Period() const override { return m_period; }
-    int Threshold() const override { return m_threshold; }
-    int MinActivationHeight() const override { return m_min_activation_height; }
-
     ThresholdState GetStateFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateFor(pindexPrev, m_cache); }
     int GetStateSinceHeightFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateSinceHeightFor(pindexPrev, m_cache); }
-
-    bool Condition(int32_t version) const
-    {
-        uint32_t mask = (uint32_t{1}) << m_bit;
-        return (((version & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS) && (version & mask) != 0);
-    }
 };
 
 /** Track blocks mined for test */
@@ -122,9 +101,6 @@ FUZZ_TARGET(versionbits, .init = initialize)
     const size_t max_periods = 16;
     const size_t max_blocks = 2 * period * max_periods;
 
-    const uint32_t threshold = fuzzed_data_provider.ConsumeIntegralInRange<uint32_t>(1, period);
-    assert(0 < threshold && threshold <= period); // must be able to both pass and fail threshold!
-
     // too many blocks at 10min each might cause uint32_t time to overflow if
     // block_start_time is at the end of the range above
     assert(std::numeric_limits<uint32_t>::max() - MAX_START_TIME > interval * max_blocks);
@@ -134,53 +110,57 @@ FUZZ_TARGET(versionbits, .init = initialize)
     // what values for version will we use to signal / not signal?
     const int32_t ver_signal = fuzzed_data_provider.ConsumeIntegral<int32_t>();
     const int32_t ver_nosignal = fuzzed_data_provider.ConsumeIntegral<int32_t>();
+    if (ver_nosignal < 0) return; // negative values are uninteresting
 
-    // select deployment parameters: bit, start time, timeout
-    const int bit = fuzzed_data_provider.ConsumeIntegralInRange<int>(0, VERSIONBITS_NUM_BITS - 1);
-
-    bool always_active_test = false;
-    bool never_active_test = false;
-    int64_t start_time;
-    int64_t timeout;
-    if (fuzzed_data_provider.ConsumeBool()) {
-        // 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>(0, period * (max_periods - 3));
-
-        start_time = block_start_time + start_block * interval;
-        timeout = block_start_time + end_block * interval;
-
-        // 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;
-    } else {
-        if (fuzzed_data_provider.ConsumeBool()) {
-            start_time = Consensus::BIP9Deployment::ALWAYS_ACTIVE;
-            always_active_test = true;
+    // Now that we have chosen time and versions, setup to mine blocks
+    Blocks blocks(block_start_time, interval, ver_signal, ver_nosignal);
+
+    const bool always_active_test = fuzzed_data_provider.ConsumeBool();
+    const bool never_active_test = !always_active_test && fuzzed_data_provider.ConsumeBool();
+
+    const Consensus::BIP9Deployment dep{[&]() {
+        Consensus::BIP9Deployment dep;
+        dep.period = period;
+
+        dep.threshold = fuzzed_data_provider.ConsumeIntegralInRange<uint32_t>(1, period);
+        assert(0 < dep.threshold && dep.threshold <= dep.period); // must be able to both pass and fail threshold!
+
+        // select deployment parameters: bit, start time, timeout
+        dep.bit = fuzzed_data_provider.ConsumeIntegralInRange<int>(0, VERSIONBITS_NUM_BITS - 1);
+
+        if (always_active_test) {
+            dep.nStartTime = Consensus::BIP9Deployment::ALWAYS_ACTIVE;
+            dep.nTimeout = fuzzed_data_provider.ConsumeBool() ? Consensus::BIP9Deployment::NO_TIMEOUT : fuzzed_data_provider.ConsumeIntegral<int64_t>();
+        } else if (never_active_test) {
+            dep.nStartTime = Consensus::BIP9Deployment::NEVER_ACTIVE;
+            dep.nTimeout = fuzzed_data_provider.ConsumeBool() ? Consensus::BIP9Deployment::NO_TIMEOUT : fuzzed_data_provider.ConsumeIntegral<int64_t>();
         } else {
-            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);
+            // 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>(0, period * (max_periods - 3));
 
-    TestConditionChecker checker(start_time, timeout, period, threshold, min_activation, bit);
+            dep.nStartTime = block_start_time + start_block * interval;
+            dep.nTimeout = block_start_time + end_block * interval;
+
+            // allow for times to not exactly match a block
+            if (fuzzed_data_provider.ConsumeBool()) dep.nStartTime += interval / 2;
+            if (fuzzed_data_provider.ConsumeBool()) dep.nTimeout += interval / 2;
+        }
+        dep.min_activation_height = fuzzed_data_provider.ConsumeIntegralInRange<int>(0, period * max_periods);
+        return dep;
+    }()};
+    TestConditionChecker checker(dep);
 
     // Early exit if the versions don't signal sensibly for the deployment
     if (!checker.Condition(ver_signal)) return;
     if (checker.Condition(ver_nosignal)) return;
-    if (ver_nosignal < 0) return;
 
     // TOP_BITS should ensure version will be positive and meet min
     // version requirement
     assert(ver_signal > 0);
     assert(ver_signal >= VERSIONBITS_LAST_OLD_BLOCK_VERSION);
 
-    // Now that we have chosen time and versions, setup to mine blocks
-    Blocks blocks(block_start_time, interval, ver_signal, ver_nosignal);
-
     /* Strategy:
      *  * we will mine a final period worth of blocks, with
      *    randomised signalling according to a mask
@@ -222,9 +202,9 @@ FUZZ_TARGET(versionbits, .init = initialize)
     // get statistics from end of previous period, then reset
     BIP9Stats last_stats;
     last_stats.period = period;
-    last_stats.threshold = threshold;
+    last_stats.threshold = dep.threshold;
     last_stats.count = last_stats.elapsed = 0;
-    last_stats.possible = (period >= threshold);
+    last_stats.possible = (period >= dep.threshold);
     std::vector<bool> last_signals{};
 
     int prev_next_height = (prev == nullptr ? 0 : prev->nHeight + 1);
@@ -238,7 +218,7 @@ FUZZ_TARGET(versionbits, .init = initialize)
         CBlockIndex* current_block = blocks.mine_block(signal);
 
         // verify that signalling attempt was interpreted correctly
-        assert(checker.Condition(current_block) == signal);
+        assert(checker.Condition(current_block->nVersion) == signal);
 
         // state and since don't change within the period
         const ThresholdState state = checker.GetStateFor(current_block);
@@ -255,10 +235,10 @@ FUZZ_TARGET(versionbits, .init = initialize)
                && stats.possible == stats_no_signals.possible);
 
         assert(stats.period == period);
-        assert(stats.threshold == threshold);
+        assert(stats.threshold == dep.threshold);
         assert(stats.elapsed == b);
         assert(stats.count == last_stats.count + (signal ? 1 : 0));
-        assert(stats.possible == (stats.count + period >= stats.elapsed + threshold));
+        assert(stats.possible == (stats.count + period >= stats.elapsed + dep.threshold));
         last_stats = stats;
 
         assert(signals.size() == last_signals.size() + 1);
@@ -269,21 +249,21 @@ FUZZ_TARGET(versionbits, .init = initialize)
 
     if (exp_state == ThresholdState::STARTED) {
         // double check that stats.possible is sane
-        if (blocks_sig >= threshold - 1) assert(last_stats.possible);
+        if (blocks_sig >= dep.threshold - 1) assert(last_stats.possible);
     }
 
     // mine the final block
     bool signal = (signalling_mask >> (period % 32)) & 1;
     if (signal) ++blocks_sig;
     CBlockIndex* current_block = blocks.mine_block(signal);
-    assert(checker.Condition(current_block) == signal);
+    assert(checker.Condition(current_block->nVersion) == signal);
 
     const BIP9Stats stats = checker.GetStateStatisticsFor(current_block);
     assert(stats.period == period);
-    assert(stats.threshold == threshold);
+    assert(stats.threshold == dep.threshold);
     assert(stats.elapsed == period);
     assert(stats.count == blocks_sig);
-    assert(stats.possible == (stats.count + period >= stats.elapsed + threshold));
+    assert(stats.possible == (stats.count + period >= stats.elapsed + dep.threshold));
 
     // More interesting is whether the state changed.
     const ThresholdState state = checker.GetStateFor(current_block);
@@ -303,33 +283,33 @@ FUZZ_TARGET(versionbits, .init = initialize)
     case ThresholdState::DEFINED:
         assert(since == 0);
         assert(exp_state == ThresholdState::DEFINED);
-        assert(current_block->GetMedianTimePast() < checker.m_begin);
+        assert(current_block->GetMedianTimePast() < dep.nStartTime);
         break;
     case ThresholdState::STARTED:
-        assert(current_block->GetMedianTimePast() >= checker.m_begin);
+        assert(current_block->GetMedianTimePast() >= dep.nStartTime);
         if (exp_state == ThresholdState::STARTED) {
-            assert(blocks_sig < threshold);
-            assert(current_block->GetMedianTimePast() < checker.m_end);
+            assert(blocks_sig < dep.threshold);
+            assert(current_block->GetMedianTimePast() < dep.nTimeout);
         } else {
             assert(exp_state == ThresholdState::DEFINED);
         }
         break;
     case ThresholdState::LOCKED_IN:
         if (exp_state == ThresholdState::LOCKED_IN) {
-            assert(current_block->nHeight + 1 < min_activation);
+            assert(current_block->nHeight + 1 < dep.min_activation_height);
         } else {
             assert(exp_state == ThresholdState::STARTED);
-            assert(blocks_sig >= threshold);
+            assert(blocks_sig >= dep.threshold);
         }
         break;
     case ThresholdState::ACTIVE:
-        assert(always_active_test || min_activation <= current_block->nHeight + 1);
+        assert(always_active_test || dep.min_activation_height <= current_block->nHeight + 1);
         assert(exp_state == ThresholdState::ACTIVE || exp_state == ThresholdState::LOCKED_IN);
         break;
     case ThresholdState::FAILED:
-        assert(never_active_test || current_block->GetMedianTimePast() >= checker.m_end);
+        assert(never_active_test || current_block->GetMedianTimePast() >= dep.nTimeout);
         if (exp_state == ThresholdState::STARTED) {
-            assert(blocks_sig < threshold);
+            assert(blocks_sig < dep.threshold);
         } else {
             assert(exp_state == ThresholdState::FAILED);
         }

src/versionbits_impl.h

@@ -67,14 +67,19 @@ class VersionBitsConditionChecker : public AbstractThresholdConditionChecker {
 
     bool Condition(const CBlockIndex* pindex) const override
     {
-        return (((pindex->nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS) && (pindex->nVersion & Mask()) != 0);
+        return Condition(pindex->nVersion);
     }
 
 public:
     explicit VersionBitsConditionChecker(const Consensus::BIP9Deployment& dep) : dep{dep} {}
     explicit VersionBitsConditionChecker(const Consensus::Params& params, Consensus::DeploymentPos id) : VersionBitsConditionChecker{params.vDeployments[id]} {}
 
     uint32_t Mask() const { return (uint32_t{1}) << dep.bit; }
+
+    bool Condition(int32_t nVersion) const
+    {
+        return (((nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS) && (nVersion & Mask()) != 0);
+    }
 };
 
 #endif // BITCOIN_VERSIONBITS_IMPL_H