Refactor versionbits deployments to avoid potential uninitialized variables #21401

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.

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?

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.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

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.

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

<!--4a62be1de6b64f3ed646cdc7932c8cf5-->

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

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.