From e9d617095d4ce9525a4337d33624cac9d6b4abe6 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Thu, 7 Dec 2023 10:17:28 +1000 Subject: [PATCH] versionbits: Remove params from AbstractThresholdConditionChecker For an abstract class, specifying parameters in detail serves no point; and for the concrete implementation, changing the consensus parameters between invocations doesn't make sense. So simplify the class by removing the consensus params from the method arguments, and just make it a member variable in the concrete object where needed. This also allows dropping dummy parameters from the unit/fuzz tests. --- src/test/fuzz/versionbits.cpp | 20 +++++------ src/test/versionbits_tests.cpp | 22 ++++++------ src/validation.cpp | 16 ++++----- src/versionbits.cpp | 61 +++++++++++++++++----------------- src/versionbits.h | 18 +++++----- 5 files changed, 66 insertions(+), 71 deletions(-) diff --git a/src/test/fuzz/versionbits.cpp b/src/test/fuzz/versionbits.cpp index c1b0f552ea0..f52c17cffc1 100644 --- a/src/test/fuzz/versionbits.cpp +++ b/src/test/fuzz/versionbits.cpp @@ -24,7 +24,6 @@ class TestConditionChecker : public AbstractThresholdConditionChecker { private: mutable ThresholdConditionCache m_cache; - const Consensus::Params dummy_params{}; public: const int64_t m_begin; @@ -43,24 +42,21 @@ public: assert(0 <= m_min_activation_height); } - bool Condition(const CBlockIndex* pindex, const Consensus::Params& params) const override { return Condition(pindex->nVersion); } - int64_t BeginTime(const Consensus::Params& params) const override { return m_begin; } - 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; } + 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, dummy_params, m_cache); } - int GetStateSinceHeightFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateSinceHeightFor(pindexPrev, dummy_params, m_cache); } - BIP9Stats GetStateStatisticsFor(const CBlockIndex* pindex, std::vector* signals=nullptr) const { return AbstractThresholdConditionChecker::GetStateStatisticsFor(pindex, dummy_params, signals); } + 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); } - - bool Condition(const CBlockIndex* pindex) const { return Condition(pindex->nVersion); } }; /** Track blocks mined for test */ diff --git a/src/test/versionbits_tests.cpp b/src/test/versionbits_tests.cpp index 29240a45f09..a59dfdad40f 100644 --- a/src/test/versionbits_tests.cpp +++ b/src/test/versionbits_tests.cpp @@ -27,40 +27,38 @@ static std::string StateName(ThresholdState state) return ""; } -static const Consensus::Params paramsDummy = Consensus::Params(); - class TestConditionChecker : public AbstractThresholdConditionChecker { private: mutable ThresholdConditionCache cache; public: - int64_t BeginTime(const Consensus::Params& params) const override { return TestTime(10000); } - int64_t EndTime(const Consensus::Params& params) const override { return TestTime(20000); } - int Period(const Consensus::Params& params) const override { return 1000; } - int Threshold(const Consensus::Params& params) const override { return 900; } - bool Condition(const CBlockIndex* pindex, const Consensus::Params& params) const override { return (pindex->nVersion & 0x100); } + int64_t BeginTime() const override { return TestTime(10000); } + int64_t EndTime() const override { return TestTime(20000); } + int Period() const override { return 1000; } + int Threshold() const override { return 900; } + bool Condition(const CBlockIndex* pindex) const override { return (pindex->nVersion & 0x100); } - ThresholdState GetStateFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateFor(pindexPrev, paramsDummy, cache); } - int GetStateSinceHeightFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateSinceHeightFor(pindexPrev, paramsDummy, cache); } + ThresholdState GetStateFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateFor(pindexPrev, cache); } + int GetStateSinceHeightFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateSinceHeightFor(pindexPrev, cache); } }; class TestDelayedActivationConditionChecker : public TestConditionChecker { public: - int MinActivationHeight(const Consensus::Params& params) const override { return 15000; } + int MinActivationHeight() const override { return 15000; } }; class TestAlwaysActiveConditionChecker : public TestConditionChecker { public: - int64_t BeginTime(const Consensus::Params& params) const override { return Consensus::BIP9Deployment::ALWAYS_ACTIVE; } + int64_t BeginTime() const override { return Consensus::BIP9Deployment::ALWAYS_ACTIVE; } }; class TestNeverActiveConditionChecker : public TestConditionChecker { public: - int64_t BeginTime(const Consensus::Params& params) const override { return Consensus::BIP9Deployment::NEVER_ACTIVE; } + int64_t BeginTime() const override { return Consensus::BIP9Deployment::NEVER_ACTIVE; } }; #define CHECKERS 6 diff --git a/src/validation.cpp b/src/validation.cpp index 3f774fd0a1e..c9d5e866866 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2376,17 +2376,17 @@ private: public: explicit WarningBitsConditionChecker(const ChainstateManager& chainman, int bit) : m_chainman{chainman}, m_bit(bit) {} - int64_t BeginTime(const Consensus::Params& params) const override { return 0; } - int64_t EndTime(const Consensus::Params& params) const override { return std::numeric_limits::max(); } - int Period(const Consensus::Params& params) const override { return params.nMinerConfirmationWindow; } - int Threshold(const Consensus::Params& params) const override { return params.nRuleChangeActivationThreshold; } + int64_t BeginTime() const override { return 0; } + int64_t EndTime() const override { return std::numeric_limits::max(); } + int Period() const override { return m_chainman.GetConsensus().nMinerConfirmationWindow; } + int Threshold() const override { return m_chainman.GetConsensus().nRuleChangeActivationThreshold; } - bool Condition(const CBlockIndex* pindex, const Consensus::Params& params) const override + bool Condition(const CBlockIndex* pindex) const override { - return pindex->nHeight >= params.MinBIP9WarningHeight && + return pindex->nHeight >= m_chainman.GetConsensus().MinBIP9WarningHeight && ((pindex->nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS) && ((pindex->nVersion >> m_bit) & 1) != 0 && - ((m_chainman.m_versionbitscache.ComputeBlockVersion(pindex->pprev, params) >> m_bit) & 1) == 0; + ((m_chainman.m_versionbitscache.ComputeBlockVersion(pindex->pprev, m_chainman.GetConsensus()) >> m_bit) & 1) == 0; } }; @@ -3029,7 +3029,7 @@ void Chainstate::UpdateTip(const CBlockIndex* pindexNew) const CBlockIndex* pindex = pindexNew; for (int bit = 0; bit < VERSIONBITS_NUM_BITS; bit++) { WarningBitsConditionChecker checker(m_chainman, bit); - ThresholdState state = checker.GetStateFor(pindex, m_chainman.GetConsensus(), m_chainman.m_warningcache.at(bit)); + ThresholdState state = checker.GetStateFor(pindex, m_chainman.m_warningcache.at(bit)); if (state == ThresholdState::ACTIVE || state == ThresholdState::LOCKED_IN) { const bilingual_str warning = strprintf(_("Unknown new rules activated (versionbit %i)"), bit); if (state == ThresholdState::ACTIVE) { diff --git a/src/versionbits.cpp b/src/versionbits.cpp index fa9d1fe9c99..958bb76abbb 100644 --- a/src/versionbits.cpp +++ b/src/versionbits.cpp @@ -6,13 +6,13 @@ #include #include -ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex* pindexPrev, const Consensus::Params& params, ThresholdConditionCache& cache) const +ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex* pindexPrev, ThresholdConditionCache& cache) const { - int nPeriod = Period(params); - int nThreshold = Threshold(params); - int min_activation_height = MinActivationHeight(params); - int64_t nTimeStart = BeginTime(params); - int64_t nTimeTimeout = EndTime(params); + int nPeriod = Period(); + int nThreshold = Threshold(); + int min_activation_height = MinActivationHeight(); + int64_t nTimeStart = BeginTime(); + int64_t nTimeTimeout = EndTime(); // Check if this deployment is always active. if (nTimeStart == Consensus::BIP9Deployment::ALWAYS_ACTIVE) { @@ -68,7 +68,7 @@ ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex* const CBlockIndex* pindexCount = pindexPrev; int count = 0; for (int i = 0; i < nPeriod; i++) { - if (Condition(pindexCount, params)) { + if (Condition(pindexCount)) { count++; } pindexCount = pindexCount->pprev; @@ -99,12 +99,12 @@ ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex* return state; } -BIP9Stats AbstractThresholdConditionChecker::GetStateStatisticsFor(const CBlockIndex* pindex, const Consensus::Params& params, std::vector* signalling_blocks) const +BIP9Stats AbstractThresholdConditionChecker::GetStateStatisticsFor(const CBlockIndex* pindex, std::vector* signalling_blocks) const { BIP9Stats stats = {}; - stats.period = Period(params); - stats.threshold = Threshold(params); + stats.period = Period(); + stats.threshold = Threshold(); if (pindex == nullptr) return stats; @@ -123,7 +123,7 @@ BIP9Stats AbstractThresholdConditionChecker::GetStateStatisticsFor(const CBlockI do { ++elapsed; --blocks_in_period; - if (Condition(currentIndex, params)) { + if (Condition(currentIndex)) { ++count; if (signalling_blocks) signalling_blocks->at(blocks_in_period) = true; } @@ -137,21 +137,21 @@ BIP9Stats AbstractThresholdConditionChecker::GetStateStatisticsFor(const CBlockI return stats; } -int AbstractThresholdConditionChecker::GetStateSinceHeightFor(const CBlockIndex* pindexPrev, const Consensus::Params& params, ThresholdConditionCache& cache) const +int AbstractThresholdConditionChecker::GetStateSinceHeightFor(const CBlockIndex* pindexPrev, ThresholdConditionCache& cache) const { - int64_t start_time = BeginTime(params); + int64_t start_time = BeginTime(); if (start_time == Consensus::BIP9Deployment::ALWAYS_ACTIVE || start_time == Consensus::BIP9Deployment::NEVER_ACTIVE) { return 0; } - const ThresholdState initialState = GetStateFor(pindexPrev, params, cache); + const ThresholdState initialState = GetStateFor(pindexPrev, cache); // BIP 9 about state DEFINED: "The genesis block is by definition in this state for each deployment." if (initialState == ThresholdState::DEFINED) { return 0; } - const int nPeriod = Period(params); + const int nPeriod = Period(); // 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. // To ease understanding of the following height calculation, it helps to remember that @@ -163,7 +163,7 @@ int AbstractThresholdConditionChecker::GetStateSinceHeightFor(const CBlockIndex* const CBlockIndex* previousPeriodParent = pindexPrev->GetAncestor(pindexPrev->nHeight - nPeriod); - while (previousPeriodParent != nullptr && GetStateFor(previousPeriodParent, params, cache) == initialState) { + while (previousPeriodParent != nullptr && GetStateFor(previousPeriodParent, cache) == initialState) { pindexPrev = previousPeriodParent; previousPeriodParent = pindexPrev->GetAncestor(pindexPrev->nHeight - nPeriod); } @@ -179,23 +179,24 @@ namespace */ class VersionBitsConditionChecker : public AbstractThresholdConditionChecker { private: + const Consensus::Params& params; const Consensus::DeploymentPos id; 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; } + int64_t BeginTime() const override { return params.vDeployments[id].nStartTime; } + int64_t EndTime() const override { return params.vDeployments[id].nTimeout; } + int MinActivationHeight() const override { return params.vDeployments[id].min_activation_height; } + int Period() const override { return params.nMinerConfirmationWindow; } + int Threshold() const override { return params.nRuleChangeActivationThreshold; } - bool Condition(const CBlockIndex* pindex, const Consensus::Params& params) const override + bool Condition(const CBlockIndex* pindex) const override { - return (((pindex->nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS) && (pindex->nVersion & Mask(params)) != 0); + return (((pindex->nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS) && (pindex->nVersion & Mask()) != 0); } public: - explicit VersionBitsConditionChecker(Consensus::DeploymentPos id_) : id(id_) {} - uint32_t Mask(const Consensus::Params& params) const { return (uint32_t{1}) << params.vDeployments[id].bit; } + explicit VersionBitsConditionChecker(const Consensus::Params& params, Consensus::DeploymentPos id) : params{params}, id{id} {} + uint32_t Mask() const { return (uint32_t{1}) << params.vDeployments[id].bit; } }; } // namespace @@ -203,23 +204,23 @@ public: ThresholdState VersionBitsCache::State(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos pos) { LOCK(m_mutex); - return VersionBitsConditionChecker(pos).GetStateFor(pindexPrev, params, m_caches[pos]); + return VersionBitsConditionChecker(params, pos).GetStateFor(pindexPrev, m_caches[pos]); } BIP9Stats VersionBitsCache::Statistics(const CBlockIndex* pindex, const Consensus::Params& params, Consensus::DeploymentPos pos, std::vector* signalling_blocks) { - return VersionBitsConditionChecker(pos).GetStateStatisticsFor(pindex, params, 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(pos).GetStateSinceHeightFor(pindexPrev, params, m_caches[pos]); + return VersionBitsConditionChecker(params, pos).GetStateSinceHeightFor(pindexPrev, m_caches[pos]); } uint32_t VersionBitsCache::Mask(const Consensus::Params& params, Consensus::DeploymentPos pos) { - return VersionBitsConditionChecker(pos).Mask(params); + return VersionBitsConditionChecker(params, pos).Mask(); } int32_t VersionBitsCache::ComputeBlockVersion(const CBlockIndex* pindexPrev, const Consensus::Params& params) @@ -229,7 +230,7 @@ int32_t VersionBitsCache::ComputeBlockVersion(const CBlockIndex* pindexPrev, con for (int i = 0; i < (int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; i++) { Consensus::DeploymentPos pos = static_cast(i); - ThresholdState state = VersionBitsConditionChecker(pos).GetStateFor(pindexPrev, params, m_caches[pos]); + ThresholdState state = VersionBitsConditionChecker(params, pos).GetStateFor(pindexPrev, m_caches[pos]); if (state == ThresholdState::LOCKED_IN || state == ThresholdState::STARTED) { nVersion |= Mask(params, pos); } diff --git a/src/versionbits.h b/src/versionbits.h index 8d2cd0f43d4..2f334081ab4 100644 --- a/src/versionbits.h +++ b/src/versionbits.h @@ -57,23 +57,23 @@ struct BIP9Stats { */ class AbstractThresholdConditionChecker { 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; + virtual bool Condition(const CBlockIndex* pindex) const =0; + virtual int64_t BeginTime() const =0; + virtual int64_t EndTime() const =0; + virtual int MinActivationHeight() const { return 0; } + virtual int Period() const =0; + virtual int Threshold() const =0; public: /** Returns the numerical statistics of an in-progress BIP9 softfork in the period including pindex * If provided, signalling_blocks is set to true/false based on whether each block in the period signalled */ - BIP9Stats GetStateStatisticsFor(const CBlockIndex* pindex, const Consensus::Params& params, std::vector* signalling_blocks = nullptr) const; + BIP9Stats GetStateStatisticsFor(const CBlockIndex* pindex, std::vector* signalling_blocks = nullptr) const; /** Returns the state for pindex A based on parent pindexPrev B. Applies any state transition if conditions are present. * Caches state from first block of period. */ - ThresholdState GetStateFor(const CBlockIndex* pindexPrev, const Consensus::Params& params, ThresholdConditionCache& cache) const; + ThresholdState GetStateFor(const CBlockIndex* pindexPrev, ThresholdConditionCache& cache) const; /** Returns the height since when the ThresholdState has started for pindex A based on parent pindexPrev B, all blocks of a period share the same */ - int GetStateSinceHeightFor(const CBlockIndex* pindexPrev, const Consensus::Params& params, ThresholdConditionCache& cache) const; + int GetStateSinceHeightFor(const CBlockIndex* pindexPrev, ThresholdConditionCache& cache) const; }; /** BIP 9 allows multiple softforks to be deployed in parallel. We cache