It's sometimes useful to test a deployment on other networks than regtest. This may be e.g. because regtest lacks a property relevant for the test, or simply because the test aims to be portable while regtest is Bitcoin Core specific.
This PR makes it possible to set the -vbparams and -testactivationheight options on any network in unit tests, and on any test network as a startup option.
This is preparatory work for a BIP 54 implementation, but may be useful separately.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35335.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | sedited, instagibbs, edilmedeiros, achow101 |
| Concept ACK | willcl-ark |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, 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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
This encapsulates the soft fork configuration logic as set by the `-testactivationheight` (for
buried deployments) and `-vbparams` (for version bits deployments) options which for the moment
are regtest-only, in order to make them available on other networks as well in the next commit.
Can be reviewed using git's `--color-moved` option with `--color-moved-ws=allow-indentation-change`.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/26175913652/job/77006888711</sub>
<sub>LLM reason (✨ experimental): CI failed due to a C++ build error: chainparams.cpp references missing member dep_opts in CChainParams::SigNetOptions (compile error: “no member named 'dep_opts'”).</sub>
<details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
Concept ACK
Concept ACK.
Seems generally useful and, if I'm honest, I'm kind of surprised we didn't need/have this ability already.
Are you after this for BIP54 because some of the timestamp/retarget-boundary behavior is awkward to model cleanly on regtest? It seems useful to be able to test deployments on signet/testnet without needing custom chainparams patches in any case.
The BIP 54 test vectors use mainnet headers and blocks for the timewarp(s) test cases and nlocktime-in-coinbase test cases. This is because regtest does not have retargets, and is Bitcoin Core specific. Mainnet is more portable to other clients and closer to what we want to test for, so i went the extra mile and generated valid mainnet blocks / block headers for the BIP test vectors (implementation here, documentation to reproduce here).
Re this part of the description:
simply because the test aims to be portable while regtest is Bitcoin Core specific.
I'm not sure what you mean with this, I guess this is about generating test vectors? Maybe link to a branch where you are using them already?
175 | + static std::unique_ptr<const CChainParams> Main(const MainNetOptions& options); 176 | + static std::unique_ptr<const CChainParams> Main() { const MainNetOptions opts{}; return Main(opts); } 177 | + static std::unique_ptr<const CChainParams> TestNet(const TestNetOptions& options); 178 | + static std::unique_ptr<const CChainParams> TestNet() { const TestNetOptions opts{}; return TestNet(opts); } 179 | + static std::unique_ptr<const CChainParams> TestNet4(const TestNetOptions& options); 180 | + static std::unique_ptr<const CChainParams> TestNet4() { const TestNetOptions opts{}; return TestNet4(opts); }
Would be nice to add the same overloads to RegTest and SigNet, we do have a few places that could use them.
Done along with call sites update in a new commit.
1434 | + return InitError(_("The -testactivationheight option may not be used on mainnet.")); 1435 | + } 1436 | + if (args.IsArgSet("-vbparams")) { 1437 | + return InitError(_("The -vbparams option may not be used on mainnet.")); 1438 | + } 1439 | + }
I think this should go in AppInitParameterInteraction. That makes it exit before daemonization and this is basically an interaction between the chain arg and the two args here.
Makes sense. Done.
LGTM
This allows unit tests to set `-testactivationheight` and `-vbparams` on
all networks instead of exclusively on regtest. Those are kept
test-network-only when used as startup parameters.
For consistency with the overloads introduced in the previous commit,
and because there is already a few places where they are useful.
ACK 801e3bfe38e82ca8e99e6628e59dde1f1fe9a060
ACK 801e3bfe38e82ca8e99e6628e59dde1f1fe9a060
lightly tested to ensure bitcoind cannot be started up with these options. bitcoin-tx/utils can be (though couldn't think of a reason why this would be problematic)
85 | @@ -107,6 +86,43 @@ void ReadRegTestArgs(const ArgsManager& args, CChainParams::RegTestOptions& opti 86 | } 87 | } 88 | 89 | +void ReadMainNetArgs(const ArgsManager& args, CChainParams::MainNetOptions& options) 90 | +{ 91 | + HandleDeploymentArgs(args, options.dep_opts);
I wonder if this is needed at all. HandleDeploymentArgs() is handling -testactivationheight and -vbparams, but the test in init.cpp will not allow initialization to proceed in mainnet if these parameters are present. Thus, current behavior will be like no-op, but wouldn't be better to just leave it out?
The point is to make those parameters available from unit tests. This line is necessary so setting those tests in unit tests isn't a noop.
Oh, I see.
Concept ACK
utACK 801e3bfe38e82ca8e99e6628e59dde1f1fe9a060
ACK 801e3bfe38e82ca8e99e6628e59dde1f1fe9a060
