Skip to content

Commit e9d6170

Browse files
ajtownsajtowns
committed
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.
1 parent 9bc41f1 commit e9d6170

File tree

5 files changed

+67
-72
lines changed

5 files changed

+67
-72
lines changed

‎src/test/fuzz/versionbits.cpp

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ class TestConditionChecker : public AbstractThresholdConditionChecker
2424
{
2525
private:
2626
mutable ThresholdConditionCache m_cache;
27-
const Consensus::Params dummy_params{};
2827

2928
public:
3029
const int64_t m_begin;
@@ -43,24 +42,21 @@ class TestConditionChecker : public AbstractThresholdConditionChecker
4342
assert(0 <= m_min_activation_height);
4443
}
4544

46-
bool Condition(const CBlockIndex* pindex, const Consensus::Params& params) const override { return Condition(pindex->nVersion); }
47-
int64_t BeginTime(const Consensus::Params& params) const override { return m_begin; }
48-
int64_t EndTime(const Consensus::Params& params) const override { return m_end; }
49-
int Period(const Consensus::Params& params) const override { return m_period; }
50-
int Threshold(const Consensus::Params& params) const override { return m_threshold; }
51-
int MinActivationHeight(const Consensus::Params& params) const override { return m_min_activation_height; }
45+
bool Condition(const CBlockIndex* pindex) const override { return Condition(pindex->nVersion); }
46+
int64_t BeginTime() const override { return m_begin; }
47+
int64_t EndTime() const override { return m_end; }
48+
int Period() const override { return m_period; }
49+
int Threshold() const override { return m_threshold; }
50+
int MinActivationHeight() const override { return m_min_activation_height; }
5251

53-
ThresholdState GetStateFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateFor(pindexPrev, dummy_params, m_cache); }
54-
int GetStateSinceHeightFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateSinceHeightFor(pindexPrev, dummy_params, m_cache); }
55-
BIP9Stats GetStateStatisticsFor(const CBlockIndex* pindex, std::vector<bool>* signals=nullptr) const { return AbstractThresholdConditionChecker::GetStateStatisticsFor(pindex, dummy_params, signals); }
52+
ThresholdState GetStateFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateFor(pindexPrev, m_cache); }
53+
int GetStateSinceHeightFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateSinceHeightFor(pindexPrev, m_cache); }
5654

5755
bool Condition(int32_t version) const
5856
{
5957
uint32_t mask = (uint32_t{1}) << m_bit;
6058
return (((version & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS) && (version & mask) != 0);
6159
}
62-
63-
bool Condition(const CBlockIndex* pindex) const { return Condition(pindex->nVersion); }
6460
};
6561

6662
/** Track blocks mined for test */

‎src/test/versionbits_tests.cpp

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -27,40 +27,38 @@ static std::string StateName(ThresholdState state)
2727
return "";
2828
}
2929

30-
static const Consensus::Params paramsDummy = Consensus::Params();
31-
3230
class TestConditionChecker : public AbstractThresholdConditionChecker
3331
{
3432
private:
3533
mutable ThresholdConditionCache cache;
3634

3735
public:
38-
int64_t BeginTime(const Consensus::Params& params) const override { return TestTime(10000); }
39-
int64_t EndTime(const Consensus::Params& params) const override { return TestTime(20000); }
40-
int Period(const Consensus::Params& params) const override { return 1000; }
41-
int Threshold(const Consensus::Params& params) const override { return 900; }
42-
bool Condition(const CBlockIndex* pindex, const Consensus::Params& params) const override { return (pindex->nVersion & 0x100); }
43-
44-
ThresholdState GetStateFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateFor(pindexPrev, paramsDummy, cache); }
45-
int GetStateSinceHeightFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateSinceHeightFor(pindexPrev, paramsDummy, cache); }
36+
int64_t BeginTime() const override { return TestTime(10000); }
37+
int64_t EndTime() const override { return TestTime(20000); }
38+
int Period() const override { return 1000; }
39+
int Threshold() const override { return 900; }
40+
bool Condition(const CBlockIndex* pindex) const override { return (pindex->nVersion & 0x100); }
41+
42+
ThresholdState GetStateFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateFor(pindexPrev, cache); }
43+
int GetStateSinceHeightFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateSinceHeightFor(pindexPrev, cache); }
4644
};
4745

4846
class TestDelayedActivationConditionChecker : public TestConditionChecker
4947
{
5048
public:
51-
int MinActivationHeight(const Consensus::Params& params) const override { return 15000; }
49+
int MinActivationHeight() const override { return 15000; }
5250
};
5351

5452
class TestAlwaysActiveConditionChecker : public TestConditionChecker
5553
{
5654
public:
57-
int64_t BeginTime(const Consensus::Params& params) const override { return Consensus::BIP9Deployment::ALWAYS_ACTIVE; }
55+
int64_t BeginTime() const override { return Consensus::BIP9Deployment::ALWAYS_ACTIVE; }
5856
};
5957

6058
class TestNeverActiveConditionChecker : public TestConditionChecker
6159
{
6260
public:
63-
int64_t BeginTime(const Consensus::Params& params) const override { return Consensus::BIP9Deployment::NEVER_ACTIVE; }
61+
int64_t BeginTime() const override { return Consensus::BIP9Deployment::NEVER_ACTIVE; }
6462
};
6563

6664
#define CHECKERS 6

‎src/validation.cpp

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2376,17 +2376,17 @@ class WarningBitsConditionChecker : public AbstractThresholdConditionChecker
23762376
public:
23772377
explicit WarningBitsConditionChecker(const ChainstateManager& chainman, int bit) : m_chainman{chainman}, m_bit(bit) {}
23782378

2379-
int64_t BeginTime(const Consensus::Params& params) const override { return 0; }
2380-
int64_t EndTime(const Consensus::Params& params) const override { return std::numeric_limits<int64_t>::max(); }
2381-
int Period(const Consensus::Params& params) const override { return params.nMinerConfirmationWindow; }
2382-
int Threshold(const Consensus::Params& params) const override { return params.nRuleChangeActivationThreshold; }
2379+
int64_t BeginTime() const override { return 0; }
2380+
int64_t EndTime() const override { return std::numeric_limits<int64_t>::max(); }
2381+
int Period() const override { return m_chainman.GetConsensus().nMinerConfirmationWindow; }
2382+
int Threshold() const override { return m_chainman.GetConsensus().nRuleChangeActivationThreshold; }
23832383

2384-
bool Condition(const CBlockIndex* pindex, const Consensus::Params& params) const override
2384+
bool Condition(const CBlockIndex* pindex) const override
23852385
{
2386-
return pindex->nHeight >= params.MinBIP9WarningHeight &&
2386+
return pindex->nHeight >= m_chainman.GetConsensus().MinBIP9WarningHeight &&
23872387
((pindex->nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS) &&
23882388
((pindex->nVersion >> m_bit) & 1) != 0 &&
2389-
((m_chainman.m_versionbitscache.ComputeBlockVersion(pindex->pprev, params) >> m_bit) & 1) == 0;
2389+
((m_chainman.m_versionbitscache.ComputeBlockVersion(pindex->pprev, m_chainman.GetConsensus()) >> m_bit) & 1) == 0;
23902390
}
23912391
};
23922392

@@ -3029,7 +3029,7 @@ void Chainstate::UpdateTip(const CBlockIndex* pindexNew)
30293029
const CBlockIndex* pindex = pindexNew;
30303030
for (int bit = 0; bit < VERSIONBITS_NUM_BITS; bit++) {
30313031
WarningBitsConditionChecker checker(m_chainman, bit);
3032-
ThresholdState state = checker.GetStateFor(pindex, m_chainman.GetConsensus(), m_chainman.m_warningcache.at(bit));
3032+
ThresholdState state = checker.GetStateFor(pindex, m_chainman.m_warningcache.at(bit));
30333033
if (state == ThresholdState::ACTIVE || state == ThresholdState::LOCKED_IN) {
30343034
const bilingual_str warning = strprintf(_("Unknown new rules activated (versionbit %i)"), bit);
30353035
if (state == ThresholdState::ACTIVE) {

‎src/versionbits.cpp

Lines changed: 31 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,13 @@
66
#include <util/check.h>
77
#include <versionbits.h>
88

9-
ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex* pindexPrev, const Consensus::Params& params, ThresholdConditionCache& cache) const
9+
ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex* pindexPrev, ThresholdConditionCache& cache) const
1010
{
11-
int nPeriod = Period(params);
12-
int nThreshold = Threshold(params);
13-
int min_activation_height = MinActivationHeight(params);
14-
int64_t nTimeStart = BeginTime(params);
15-
int64_t nTimeTimeout = EndTime(params);
11+
int nPeriod = Period();
12+
int nThreshold = Threshold();
13+
int min_activation_height = MinActivationHeight();
14+
int64_t nTimeStart = BeginTime();
15+
int64_t nTimeTimeout = EndTime();
1616

1717
// Check if this deployment is always active.
1818
if (nTimeStart == Consensus::BIP9Deployment::ALWAYS_ACTIVE) {
@@ -68,7 +68,7 @@ ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex*
6868
const CBlockIndex* pindexCount = pindexPrev;
6969
int count = 0;
7070
for (int i = 0; i < nPeriod; i++) {
71-
if (Condition(pindexCount, params)) {
71+
if (Condition(pindexCount)) {
7272
count++;
7373
}
7474
pindexCount = pindexCount->pprev;
@@ -99,12 +99,12 @@ ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex*
9999
return state;
100100
}
101101

102-
BIP9Stats AbstractThresholdConditionChecker::GetStateStatisticsFor(const CBlockIndex* pindex, const Consensus::Params& params, std::vector<bool>* signalling_blocks) const
102+
BIP9Stats AbstractThresholdConditionChecker::GetStateStatisticsFor(const CBlockIndex* pindex, std::vector<bool>* signalling_blocks) const
103103
{
104104
BIP9Stats stats = {};
105105

106-
stats.period = Period(params);
107-
stats.threshold = Threshold(params);
106+
stats.period = Period();
107+
stats.threshold = Threshold();
108108

109109
if (pindex == nullptr) return stats;
110110

@@ -123,7 +123,7 @@ BIP9Stats AbstractThresholdConditionChecker::GetStateStatisticsFor(const CBlockI
123123
do {
124124
++elapsed;
125125
--blocks_in_period;
126-
if (Condition(currentIndex, params)) {
126+
if (Condition(currentIndex)) {
127127
++count;
128128
if (signalling_blocks) signalling_blocks->at(blocks_in_period) = true;
129129
}
@@ -137,21 +137,21 @@ BIP9Stats AbstractThresholdConditionChecker::GetStateStatisticsFor(const CBlockI
137137
return stats;
138138
}
139139

140-
int AbstractThresholdConditionChecker::GetStateSinceHeightFor(const CBlockIndex* pindexPrev, const Consensus::Params& params, ThresholdConditionCache& cache) const
140+
int AbstractThresholdConditionChecker::GetStateSinceHeightFor(const CBlockIndex* pindexPrev, ThresholdConditionCache& cache) const
141141
{
142-
int64_t start_time = BeginTime(params);
142+
int64_t start_time = BeginTime();
143143
if (start_time == Consensus::BIP9Deployment::ALWAYS_ACTIVE || start_time == Consensus::BIP9Deployment::NEVER_ACTIVE) {
144144
return 0;
145145
}
146146

147-
const ThresholdState initialState = GetStateFor(pindexPrev, params, cache);
147+
const ThresholdState initialState = GetStateFor(pindexPrev, cache);
148148

149149
// BIP 9 about state DEFINED: "The genesis block is by definition in this state for each deployment."
150150
if (initialState == ThresholdState::DEFINED) {
151151
return 0;
152152
}
153153

154-
const int nPeriod = Period(params);
154+
const int nPeriod = Period();
155155

156156
// 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.
157157
// To ease understanding of the following height calculation, it helps to remember that
@@ -163,7 +163,7 @@ int AbstractThresholdConditionChecker::GetStateSinceHeightFor(const CBlockIndex*
163163

164164
const CBlockIndex* previousPeriodParent = pindexPrev->GetAncestor(pindexPrev->nHeight - nPeriod);
165165

166-
while (previousPeriodParent != nullptr && GetStateFor(previousPeriodParent, params, cache) == initialState) {
166+
while (previousPeriodParent != nullptr && GetStateFor(previousPeriodParent, cache) == initialState) {
167167
pindexPrev = previousPeriodParent;
168168
previousPeriodParent = pindexPrev->GetAncestor(pindexPrev->nHeight - nPeriod);
169169
}
@@ -179,47 +179,48 @@ namespace
179179
*/
180180
class VersionBitsConditionChecker : public AbstractThresholdConditionChecker {
181181
private:
182+
const Consensus::Params& params;
182183
const Consensus::DeploymentPos id;
183184

184185
protected:
185-
int64_t BeginTime(const Consensus::Params& params) const override { return params.vDeployments[id].nStartTime; }
186-
int64_t EndTime(const Consensus::Params& params) const override { return params.vDeployments[id].nTimeout; }
187-
int MinActivationHeight(const Consensus::Params& params) const override { return params.vDeployments[id].min_activation_height; }
188-
int Period(const Consensus::Params& params) const override { return params.nMinerConfirmationWindow; }
189-
int Threshold(const Consensus::Params& params) const override { return params.nRuleChangeActivationThreshold; }
186+
int64_t BeginTime() const override { return params.vDeployments[id].nStartTime; }
187+
int64_t EndTime() const override { return params.vDeployments[id].nTimeout; }
188+
int MinActivationHeight() const override { return params.vDeployments[id].min_activation_height; }
189+
int Period() const override { return params.nMinerConfirmationWindow; }
190+
int Threshold() const override { return params.nRuleChangeActivationThreshold; }
190191

191-
bool Condition(const CBlockIndex* pindex, const Consensus::Params& params) const override
192+
bool Condition(const CBlockIndex* pindex) const override
192193
{
193-
return (((pindex->nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS) && (pindex->nVersion & Mask(params)) != 0);
194+
return (((pindex->nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS) && (pindex->nVersion & Mask()) != 0);
194195
}
195196

196197
public:
197-
explicit VersionBitsConditionChecker(Consensus::DeploymentPos id_) : id(id_) {}
198-
uint32_t Mask(const Consensus::Params& params) const { return (uint32_t{1}) << params.vDeployments[id].bit; }
198+
explicit VersionBitsConditionChecker(const Consensus::Params& params, Consensus::DeploymentPos id) : params{params}, id{id} {}
199+
uint32_t Mask() const { return (uint32_t{1}) << params.vDeployments[id].bit; }
199200
};
200201

201202
} // namespace
202203

203204
ThresholdState VersionBitsCache::State(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos pos)
204205
{
205206
LOCK(m_mutex);
206-
return VersionBitsConditionChecker(pos).GetStateFor(pindexPrev, params, m_caches[pos]);
207+
return VersionBitsConditionChecker(params, pos).GetStateFor(pindexPrev, m_caches[pos]);
207208
}
208209

209210
BIP9Stats VersionBitsCache::Statistics(const CBlockIndex* pindex, const Consensus::Params& params, Consensus::DeploymentPos pos, std::vector<bool>* signalling_blocks)
210211
{
211-
return VersionBitsConditionChecker(pos).GetStateStatisticsFor(pindex, params, signalling_blocks);
212+
return VersionBitsConditionChecker(params, pos).GetStateStatisticsFor(pindex, signalling_blocks);
212213
}
213214

214215
int VersionBitsCache::StateSinceHeight(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos pos)
215216
{
216217
LOCK(m_mutex);
217-
return VersionBitsConditionChecker(pos).GetStateSinceHeightFor(pindexPrev, params, m_caches[pos]);
218+
return VersionBitsConditionChecker(params, pos).GetStateSinceHeightFor(pindexPrev, m_caches[pos]);
218219
}
219220

220221
uint32_t VersionBitsCache::Mask(const Consensus::Params& params, Consensus::DeploymentPos pos)
221222
{
222-
return VersionBitsConditionChecker(pos).Mask(params);
223+
return VersionBitsConditionChecker(params, pos).Mask();
223224
}
224225

225226
int32_t VersionBitsCache::ComputeBlockVersion(const CBlockIndex* pindexPrev, const Consensus::Params& params)
@@ -229,7 +230,7 @@ int32_t VersionBitsCache::ComputeBlockVersion(const CBlockIndex* pindexPrev, con
229230

230231
for (int i = 0; i < (int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; i++) {
231232
Consensus::DeploymentPos pos = static_cast<Consensus::DeploymentPos>(i);
232-
ThresholdState state = VersionBitsConditionChecker(pos).GetStateFor(pindexPrev, params, m_caches[pos]);
233+
ThresholdState state = VersionBitsConditionChecker(params, pos).GetStateFor(pindexPrev, m_caches[pos]);
233234
if (state == ThresholdState::LOCKED_IN || state == ThresholdState::STARTED) {
234235
nVersion |= Mask(params, pos);
235236
}

‎src/versionbits.h

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -57,23 +57,23 @@ struct BIP9Stats {
5757
*/
5858
class AbstractThresholdConditionChecker {
5959
protected:
60-
virtual bool Condition(const CBlockIndex* pindex, const Consensus::Params& params) const =0;
61-
virtual int64_t BeginTime(const Consensus::Params& params) const =0;
62-
virtual int64_t EndTime(const Consensus::Params& params) const =0;
63-
virtual int MinActivationHeight(const Consensus::Params& params) const { return 0; }
64-
virtual int Period(const Consensus::Params& params) const =0;
65-
virtual int Threshold(const Consensus::Params& params) const =0;
60+
virtual bool Condition(const CBlockIndex* pindex) const =0;
61+
virtual int64_t BeginTime() const =0;
62+
virtual int64_t EndTime() const =0;
63+
virtual int MinActivationHeight() const { return 0; }
64+
virtual int Period() const =0;
65+
virtual int Threshold() const =0;
6666

6767
public:
6868
/** Returns the numerical statistics of an in-progress BIP9 softfork in the period including pindex
6969
* If provided, signalling_blocks is set to true/false based on whether each block in the period signalled
7070
*/
71-
BIP9Stats GetStateStatisticsFor(const CBlockIndex* pindex, const Consensus::Params& params, std::vector<bool>* signalling_blocks = nullptr) const;
71+
BIP9Stats GetStateStatisticsFor(const CBlockIndex* pindex, std::vector<bool>* signalling_blocks = nullptr) const;
7272
/** Returns the state for pindex A based on parent pindexPrev B. Applies any state transition if conditions are present.
7373
* Caches state from first block of period. */
74-
ThresholdState GetStateFor(const CBlockIndex* pindexPrev, const Consensus::Params& params, ThresholdConditionCache& cache) const;
74+
ThresholdState GetStateFor(const CBlockIndex* pindexPrev, ThresholdConditionCache& cache) const;
7575
/** 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 */
76-
int GetStateSinceHeightFor(const CBlockIndex* pindexPrev, const Consensus::Params& params, ThresholdConditionCache& cache) const;
76+
int GetStateSinceHeightFor(const CBlockIndex* pindexPrev, ThresholdConditionCache& cache) const;
7777
};
7878

7979
/** BIP 9 allows multiple softforks to be deployed in parallel. We cache

0 commit comments

Comments
 (0)