Refactor versionbits deployments to avoid potential uninitialized variables

achow101 commented at 11:44 pm on March 9, 2021: member

Currently versionbits deployments are stored in a simple C array in Params. Setting the actual values needs to be done externally. Since the memory is allocated and initialized when Params is initialized, it is possible to have a deployment with uninitialized (or otherwise garbage and invalid) values set for its parameters if something is forgotten when they are being set.

Instead of this two step approach (create deployment object, then set its members), I think it is better to have a constructor where those parameters are passed in and set when the deployment is initialized. This PR does that. In order for this to work though, vDeployments cannot be a simple C array. It is instead changed to be a std::map<DeploymentPos, VBitsDeployment>. This maps the deployment enum to the VBitsDeployment object itself; this is essentially how we were using vDeployments previously. Additionally, by also changing VersionBitsDeploymentInfo to a map as well, we are able to remove MAX_VERSION_BITS_DEPLOYMENT and all of the places where we were iterating the deployments no longer need to cast an int to DeploymentPos.

An additional concern is that the members of VBitsDeployment are not const and could thus be accidentally changed during runtime. To prevent this, this PR makes all members of VBitsDeployment const.

Lastly, to make VBitsDeployment actually fully describe a deployment, this PR moves the number of blocks in a period into the struct. In order to ensure that this variable matches up with the retarget period, a unit test is added.

Built on top of #21392 and #21380 for the refactors and other changes they make to VBitsDeployment.

in src/chainparams.cpp:330 in c2549d0326 outdated

337-        consensus.m_deployments[Consensus::DEPLOYMENT_TAPROOT].threshold = 1916; // 95% of 2016
338-        consensus.m_deployments[Consensus::DEPLOYMENT_TAPROOT].m_min_activation_height = 0; // No minimum activation height
339+        consensus.m_deployments.emplace(Consensus::DEPLOYMENT_TESTDUMMY, Consensus::VBitsDeployment(28, false /* active */));
340+
341+        // Deployment of Taproot (BIPs 340-342)
342+        consensus.m_deployments.emplace(Consensus::DEPLOYMENT_TAPROOT, Consensus::VBitsDeployment(2, true /* active */));

ajtowns commented at 0:17 am on March 10, 2021:

We usually do these comments as /* active= */ false I think

ajtowns commented at 0:56 am on March 10, 2021: member

globalChainParams / Params() is already const during runtime, so that should already prevent changing any of the fields in vDeployments. Ah, a different approach that might be worth considering:

 0class CChainParams
 1{
 2protected:
 3    CChainParams(const Consensus::Params& cons) : consensus{cons} { } // or std::move, whatever
 4public:
 5    const Consensus::Params consensus;
 6    ...
 7};
 8
 9class CRegTestParams : public CChainParams
10{
11public:
12    CRegTestParams(const ArgsManager& args) : CChainParams{getConsensus(args)}
13    { ... }
14    
15    static Consensus::Params getConsensus(const ArgsManager& args)
16    {
17        Consensus::Params consensus{};
18        consensus.foo = bar; ...
19        // parse -segwitheight and -vbparams here as well
20    }
21};

and have default initializers for everything in Consensus::Params?

Not keen on the map – the possibility that deployments.at(DEPLOYMENT_FOO) might throw doesn’t seem that great to me; and if you’re just using deployments[DEPLOYMENT_FOO] and relying on it getting default initialised to something sane, might as well give VBitsDeployment a default initiaizer and just use the array?

DrahtBot added the label Build system on Mar 10, 2021

DrahtBot added the label Consensus on Mar 10, 2021

DrahtBot added the label Docs on Mar 10, 2021

DrahtBot added the label Mining on Mar 10, 2021

DrahtBot added the label RPC/REST/ZMQ on Mar 10, 2021

DrahtBot added the label Validation on Mar 10, 2021

achow101 commented at 2:33 am on March 10, 2021: member

Ah, a different approach that might be worth considering:

That doesn’t seem that much different from what we already do now.

Not keen on the map – the possibility that deployments.at(DEPLOYMENT_FOO) might throw doesn’t seem that great to me; and if you’re just using deployments[DEPLOYMENT_FOO] and relying on it getting default initialised to something sane, might as well give VBitsDeployment a default initiaizer and just use the array?

It seems better to me to throw (and hard shutdown) than to use some defaults, or worse, uninitialized members. I don’t really like the idea of consensus params being initialized with default values that might not actually be what is wanted.

DrahtBot commented at 4:37 am on March 10, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21391 ([Bundle 5/n] Prune g_chainman usage in RPC modules by dongcarl)
#21377 (Speedy trial support for versionbits by ajtowns)
#20354 (test: Add feature_taproot.py –previous_release by MarcoFalke)
#19573 (Replace unused BIP 9 logic with draft BIP 8 by luke-jr)
#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
#17783 (util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky)
#17581 (refactor: Remove settings merge reverse precedence code by ryanofsky)
#17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
#17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)
#10102 ([experimental] Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

practicalswift commented at 7:12 am on March 10, 2021: contributor

Concept ACK on not using error prone C arrays like maps.

laanwj removed the label Build system on Mar 10, 2021

laanwj removed the label Consensus on Mar 10, 2021

laanwj removed the label Docs on Mar 10, 2021

laanwj removed the label Mining on Mar 10, 2021

laanwj removed the label RPC/REST/ZMQ on Mar 10, 2021

laanwj removed the label Validation on Mar 10, 2021

laanwj added the label Refactoring on Mar 10, 2021

DrahtBot added the label Needs rebase on Mar 15, 2021

achow101 force-pushed on Mar 15, 2021

DrahtBot removed the label Needs rebase on Mar 15, 2021

DrahtBot commented at 4:48 pm on March 15, 2021: member

🕵️ @harding @sipa have been requested to review this pull request as specified in the REVIEWERS file.

DrahtBot added the label Needs rebase on Mar 15, 2021

[refactor] versionbits: make AbstractThresholdConditionChecker less abstract

AbstractThresholdConditionChecker is already tightly tied to
Consensus::BIP9Deployment, so have it reference that directly rather
than abstracting it. It also does not need most of Consensus::Params,
so pull out the two relevant fields directly.

Also pull the Condition() implementation and Mask() function into the
base class, though keeping Condition() virtual to allow it to be replaced.

This simplifies the API substantially, but more importantly makes it
easier to test independently.

524b41ffdb

chainparams: make versionbits threshold per-deployment d86a699edf

scripted-diff: Rename AbstractThresholdConditionChecker and BIP9* classes

-BEGIN VERIFY SCRIPT-
sed -i -e 's/AbstractThresholdConditionChecker/ThresholdConditionChecker/g' $(git grep -l AbstractThresholdConditionChecker src/)
sed -i -e 's/BIP9Deployment/VBitsDeployment/g' $(git grep -l BIP9Deployment src/)
sed -i -e 's/BIP9Stats/VBitsStats/g' $(git grep -l BIP9Stats src/)
-END VERIFY SCRIPT-

ae541ab9a7

versionbits: GetStateStatisticsFor

Document that GetStateStatisticsFor expects a pointer to the previous block;
which means that the "current period" it's reporting statistics for
may not yet have any blocks in it. This is consistent with its usage
from getblockchaininfo.

Also fix a bug where GetStateStatisticsFor would crash if pindexPrev was
a block in the first retarget period, but not the final block of that
retarget period. Note that GetStateStatisticsFor would never be called
in this case, as that would require the first retarget period to be in
STARTED state, but it is always in DEFINED state.

c67ead16f5

versionbits: make Mask signed to match CBlockIndex::nVersion

This avoids undefined behaviour when doing bitwise operations, so long as
the versions are not negative.

414640fbaf

achow101 force-pushed on Mar 15, 2021

DrahtBot removed the label Needs rebase on Mar 16, 2021

Migrate versionbits to use height instead of MTP

Co-authored-by: Anthony Towns <aj@erisian.com.au>

25ab012cfd

Rename user facing mentions of BIP 9 to versionbits and/or BIP 8

Co-authored-by: Anthony Towns <aj@erisian.com.au>

a440de1a77

Add minimum activation height to VBitsDeployments 8269a341f7

tests: test versionbits delayed activation c0efe1896c

Clarify and reduce nRuleChangeActivationThreshold

As thresholds are now parameterized, nRuleChangeActivationThreshold is
no longer the threshold used for activating new rule changes. Instead it
is now only used to warn if there is an unkonwn versionbits deployment.
To make this clear, rename to m_vbits_min_threshold and update the
comment describing it.

Additionally, because this is just a minimum used for a warning, reduce
the threshold to 75% so that future soft forks which may have thresholds
lower than 95% will still have warnings.

409a8cba9b

test: add min_activation_height to -vbparams e3f7654fe1

test: BIP 8 delayed activation functional test 3cc72deda8

Change versionbits deployments to maps

vDeployments is a C array that is used like a map. To make construction
of deployments and later iteration easier, use a std::map for all things
containing versionbits deployment data.

Also removes MAX_VERSION_BITS_DEPLOYMENTS as it is no longer needed.

434b43ca85

Add constructors for VBitsDeployment

Instead of using the default struct constructor and then filling in the
parameters later, add constructors to VBitsDeployment where the
parameters are filled in at initialization.

de47b3189c

Make parameters in VBitsDeployment const 48771d52c7

Move period into VBitsDeployment 585dd6e774

achow101 force-pushed on Mar 16, 2021

jnewbery commented at 2:36 pm on April 11, 2021: member

This is built on #21392, which is now closed. Should this PR also be closed?

fanquake added the label Waiting for author on Apr 12, 2021

fanquake marked this as a draft on Apr 12, 2021

MarcoFalke commented at 7:54 am on April 12, 2021: member

I’d say it needs rebase after #21377 is merged

MarcoFalke removed the label Waiting for author on Apr 15, 2021

MarcoFalke added the label Needs rebase on Apr 15, 2021

luke-jr commented at 8:33 pm on May 13, 2021: member

This seems to be a lot less readable. Is there a good way to get uninitialised warnings without harming readability, perhaps?

achow101 commented at 4:56 am on May 25, 2021: member

Closing for now, may revisit in the future, or someone else can take it over.

achow101 closed this on May 25, 2021

DrahtBot locked this on Aug 16, 2022

Refactor versionbits deployments to avoid potential uninitialized variables #21401

Conflicts