Refactor versionbits deployments to avoid potential uninitialized variables #21401
Conversation
There was a problem hiding this comment.
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:
class CChainParams
{
protected:
CChainParams(const Consensus::Params& cons) : consensus{cons} { } // or std::move, whatever
public:
const Consensus::Params consensus;
...
};
class CRegTestParams : public CChainParams
{
public:
CRegTestParams(const ArgsManager& args) : CChainParams{getConsensus(args)}
{ ... }
static Consensus::Params getConsensus(const ArgsManager& args)
{
Consensus::Params consensus{};
consensus.foo = bar; ...
// parse -segwitheight and -vbparams here as well
}
};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?
| consensus.m_deployments.emplace(Consensus::DEPLOYMENT_TESTDUMMY, Consensus::VBitsDeployment(28, false /* active */)); | ||
|
|
||
| // Deployment of Taproot (BIPs 340-342) | ||
| consensus.m_deployments.emplace(Consensus::DEPLOYMENT_TAPROOT, Consensus::VBitsDeployment(2, true /* active */)); |
There was a problem hiding this comment.
We usually do these comments as /* active= */ false I think
That doesn't seem that much different from what we already do now.
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. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK on not using error prone C arrays like maps. |
Co-authored-by: Anthony Towns <aj@erisian.com.au>
Co-authored-by: Anthony Towns <aj@erisian.com.au>
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.
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.
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.
e1cac51 to
585dd6e
Compare
|
This is built on #21392, which is now closed. Should this PR also be closed? |
|
I'd say it needs rebase after #21377 is merged |
|
This seems to be a lot less readable. Is there a good way to get uninitialised warnings without harming readability, perhaps? |
|
Closing for now, may revisit in the future, or someone else can take it over. |
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 whenParamsis 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,
vDeploymentscannot be a simple C array. It is instead changed to be astd::map<DeploymentPos, VBitsDeployment>. This maps the deployment enum to theVBitsDeploymentobject itself; this is essentially how we were usingvDeploymentspreviously. Additionally, by also changingVersionBitsDeploymentInfoto a map as well, we are able to removeMAX_VERSION_BITS_DEPLOYMENTand all of the places where we were iterating the deployments no longer need to cast an int toDeploymentPos.An additional concern is that the members of
VBitsDeploymentare not const and could thus be accidentally changed during runtime. To prevent this, this PR makes all members ofVBitsDeploymentconst.Lastly, to make
VBitsDeploymentactually 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.