From 37b9b67a39554465104c9cf1a74690f40019dbad Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Sat, 9 Dec 2023 10:13:38 +1000
Subject: [PATCH] versionbits: Simplify VersionBitsCache API

Replaces State() (which returned ACTIVE/STARTED/etc) with IsActiveAfter()
which just returns a bool, as this was all State was actually used
for. Drops Mask(), which was only used in tests and can be replaced with
`1<<bit`, and also drops StateSinceHeight() and Statistics(), which are
now only used internally for Info().
---
 src/deploymentstatus.h         |  2 +-
 src/test/versionbits_tests.cpp |  5 +----
 src/versionbits.cpp            | 37 +++++++++++++---------------------
 src/versionbits.h              | 15 ++------------
 4 files changed, 18 insertions(+), 41 deletions(-)

diff --git a/src/deploymentstatus.h b/src/deploymentstatus.h
index 03d3c531cce..97ff8dee312 100644
--- a/src/deploymentstatus.h
+++ b/src/deploymentstatus.h
@@ -20,7 +20,7 @@ inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus
 inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos dep, VersionBitsCache& versionbitscache)
 {
     assert(Consensus::ValidDeployment(dep));
-    return ThresholdState::ACTIVE == versionbitscache.State(pindexPrev, params, dep);
+    return versionbitscache.IsActiveAfter(pindexPrev, params, dep);
 }
 
 /** Determine if a deployment is active for this block */
diff --git a/src/test/versionbits_tests.cpp b/src/test/versionbits_tests.cpp
index 19f75c57bb0..19cb3a27547 100644
--- a/src/test/versionbits_tests.cpp
+++ b/src/test/versionbits_tests.cpp
@@ -297,9 +297,6 @@ void check_computeblockversion(VersionBitsCache& versionbitscache, const Consens
     // Check min_activation_height is on a retarget boundary
     BOOST_REQUIRE_EQUAL(min_activation_height % period, 0U);
 
-    const uint32_t bitmask{versionbitscache.Mask(params, dep)};
-    BOOST_CHECK_EQUAL(bitmask, uint32_t{1} << bit);
-
     // 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.
@@ -438,7 +435,7 @@ BOOST_FIXTURE_TEST_CASE(versionbits_computeblockversion, BlockVersionTest)
             // not take precedence over STARTED/LOCKED_IN. So all softforks on
             // the same bit might overlap, even when non-overlapping start-end
             // times are picked.
-            const uint32_t dep_mask{vbcache.Mask(chainParams->GetConsensus(), dep)};
+            const uint32_t dep_mask{uint32_t{1} << chainParams->GetConsensus().vDeployments[dep].bit};
             BOOST_CHECK(!(chain_all_vbits & dep_mask));
             chain_all_vbits |= dep_mask;
             check_computeblockversion(vbcache, chainParams->GetConsensus(), dep);
diff --git a/src/versionbits.cpp b/src/versionbits.cpp
index 0b416884a67..8cd5815bcec 100644
--- a/src/versionbits.cpp
+++ b/src/versionbits.cpp
@@ -222,16 +222,23 @@ BIP9Info VersionBitsCache::Info(const CBlockIndex& block_index, const Consensus:
 {
     BIP9Info result;
 
-    const auto current_state = State(block_index.pprev, params, id);
-    result.current_state = StateName(current_state);
-    result.since = StateSinceHeight(block_index.pprev, params, id);
+    VersionBitsConditionChecker checker(params, id);
 
-    const auto next_state = State(&block_index, params, id);
+    ThresholdState current_state, next_state;
+
+    {
+        LOCK(m_mutex);
+        current_state = checker.GetStateFor(block_index.pprev, m_caches[id]);
+        next_state = checker.GetStateFor(&block_index, m_caches[id]);
+        result.since = checker.GetStateSinceHeightFor(block_index.pprev, m_caches[id]);
+    }
+
+    result.current_state = StateName(current_state);
     result.next_state = StateName(next_state);
 
     const bool has_signal = (STARTED == current_state || LOCKED_IN == current_state);
     if (has_signal) {
-        result.stats.emplace(Statistics(&block_index, params, id, &result.signalling_blocks));
+        result.stats.emplace(checker.GetStateStatisticsFor(&block_index, &result.signalling_blocks));
         if (LOCKED_IN == current_state) {
             result.stats->threshold = 0;
             result.stats->possible = false;
@@ -278,26 +285,10 @@ BIP9GBTStatus VersionBitsCache::GBTStatus(const CBlockIndex& block_index, const
     return result;
 }
 
-ThresholdState VersionBitsCache::State(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos pos)
+bool VersionBitsCache::IsActiveAfter(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos pos)
 {
     LOCK(m_mutex);
-    return VersionBitsConditionChecker(params, pos).GetStateFor(pindexPrev, m_caches[pos]);
-}
-
-BIP9Stats VersionBitsCache::Statistics(const CBlockIndex* pindex, const Consensus::Params& params, Consensus::DeploymentPos pos, std::vector<bool>* signalling_blocks)
-{
-    return VersionBitsConditionChecker(params, pos).GetStateStatisticsFor(pindex, signalling_blocks);
-}
-
-int VersionBitsCache::StateSinceHeight(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos pos)
-{
-    LOCK(m_mutex);
-    return VersionBitsConditionChecker(params, pos).GetStateSinceHeightFor(pindexPrev, m_caches[pos]);
-}
-
-uint32_t VersionBitsCache::Mask(const Consensus::Params& params, Consensus::DeploymentPos pos)
-{
-    return VersionBitsConditionChecker(params, pos).Mask();
+    return ThresholdState::ACTIVE == VersionBitsConditionChecker(params, pos).GetStateFor(pindexPrev, m_caches[pos]);
 }
 
 static int32_t ComputeBlockVersion(const CBlockIndex* pindexPrev, const Consensus::Params& params, std::array<ThresholdConditionCache, Consensus::MAX_VERSION_BITS_DEPLOYMENTS>& caches)
diff --git a/src/versionbits.h b/src/versionbits.h
index c9fefad9693..732e073228a 100644
--- a/src/versionbits.h
+++ b/src/versionbits.h
@@ -109,25 +109,14 @@ private:
     std::array<ThresholdConditionCache,Consensus::MAX_VERSION_BITS_DEPLOYMENTS> m_caches GUARDED_BY(m_mutex);
 
 public:
-    /** Get the numerical statistics for a given deployment for the signalling period that includes pindex.
-     * If provided, signalling_blocks is set to true/false based on whether each block in the period signalled
-     */
-    static BIP9Stats Statistics(const CBlockIndex* pindex, const Consensus::Params& params, Consensus::DeploymentPos pos, std::vector<bool>* signalling_blocks = nullptr);
-
-    static uint32_t Mask(const Consensus::Params& params, Consensus::DeploymentPos pos);
-
     BIP9Info Info(const CBlockIndex& block_index, const Consensus::Params& params, Consensus::DeploymentPos id) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
 
     BIP9GBTStatus GBTStatus(const CBlockIndex& block_index, const Consensus::Params& params) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
 
     /** Get the BIP9 state for a given deployment for the block after pindexPrev. */
-    ThresholdState State(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos pos) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
+    bool IsActiveAfter(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos pos) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
 
-    /** Get the block height at which the BIP9 deployment switched into the state for the block after pindexPrev. */
-    int StateSinceHeight(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos pos) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
-
-    /** Determine what nVersion a new block should use
-     */
+    /** Determine what nVersion a new block should use */
     int32_t ComputeBlockVersion(const CBlockIndex* pindexPrev, const Consensus::Params& params) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
 
     /** Check for unknown activations