test: Don’t enforce BIP94 on regtest unless specified by arg #31156

pull mzumsande wants to merge 2 commits into bitcoin:master from mzumsande:202410_bip94_arg changing 6 files +9 −1

mzumsande commented at 6:06 pm on October 25, 2024: contributor

The added arg -test=bip94 is only used in a functional test for BIP94. This is done because the default regtest consensus rules should follow mainnet, not testnet.

Fixes #31137.
DrahtBot commented at 6:06 pm on October 25, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers

ACK tdb3, rkrux, BrandonOdiwuor, laanwj, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
DrahtBot added the label Tests on Oct 25, 2024

Type	Reviewers
ACK	tdb3, rkrux, BrandonOdiwuor, laanwj, achow101

test: Don't enforce BIP94 on regtest unless specified by arg

The added regtest option -test=bip94 is only used in the functional
test for BIP94.
This is done because the default regtest consensus rules
should aim to follow to mainnet, not testnet.

fc7dfb3df5

doc: add release note for 31156 e60cecc811

in src/chainparamsbase.cpp:26 in dc8a9eab2a outdated

22@@ -23,6 +23,7 @@ void SetupChainParamsBaseOptions(ArgsManager& argsman)
23     argsman.AddArg("-signet", "Use the signet chain. Equivalent to -chain=signet. Note that the network is defined by the -signetchallenge parameter", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
24     argsman.AddArg("-signetchallenge", "Blocks must satisfy the given script to be considered valid (only for signet networks; defaults to the global default signet test network challenge)", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::CHAINPARAMS);
25     argsman.AddArg("-signetseednode", "Specify a seed node for the signet network, in the hostname[:port] format, e.g. sig.net:1234 (may be used multiple times to specify multiple seed nodes; defaults to the global default signet test network seed node(s))", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::CHAINPARAMS);
26+    argsman.AddArg("-enforcebip94", "Enforce BIP94 consensus rules (designed for testnet4) on regtest. (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);

laanwj commented at 9:29 am on October 27, 2024:

Might want to prefix this argument with regtest to make it doubly clear that it doesn’t do anything on other networks.

maflcko commented at 7:40 am on October 28, 2024:

nit: Why add a new option at all? I wonder if using the existing -test=<option> hook would work.

mzumsande commented at 3:42 pm on October 28, 2024:

good point, I forgot about -test=. Changed it to that approach, as a a result a regtest prefix is no longer necessary because -test= will fail init on other networks anyway.

mzumsande force-pushed on Oct 28, 2024
tdb3 approved
tdb3 commented at 4:09 pm on October 28, 2024: contributor
cr and light test ACK e60cecc8115d3b28be076792baa5e4ea26d353a6

Sanity checked by removing the argument and seeing that the test fails as expected.
0- self.restart_node(0, extra_args=['-test=bip94']) 1+ self.restart_node(0, extra_args=[])
in doc/release-notes-31156.md:4 in e60cecc811
0@@ -0,0 +1,4 @@ 1+Test 2+------ 3+ 4+The BIP94 timewarp attack mitigation (designed for testnet4) is no longer active on the regtest network. (#31156)
rkrux commented at 10:43 am on October 29, 2024:

Worth creating a directory doc/release-notes-prs and putting it there? I see there’s a release-notes directory that contains notes for the releases.

mzumsande commented at 2:34 pm on October 29, 2024:

Just following the process described in https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#release-notes - a change to that process should be discussed in a separate issue / PR - whatever works best for the maintainers I guess, personally I don’t care at all.

mzumsande commented at 2:40 pm on October 29, 2024:

By the way, the reason I added a release note is that there also was one in the 28.0 release notes about introducing this.

rkrux commented at 2:45 pm on October 29, 2024:

Just following the process described in https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#release-notes - a change to that process should be discussed in a separate issue / PR - whatever works best for the maintainers I guess, personally I don’t care at all.

Thanks for sharing, I didn’t know there is a process already for this.
rkrux approved
rkrux commented at 11:18 am on October 29, 2024: none

tACK e60cecc8115d3b28be076792baa5e4ea26d353a6

Successful make and functional tests. Agree with the approach to enable BIP94 on regtest through the test argument.
maflcko requested review from Sjors on Oct 29, 2024
BrandonOdiwuor commented at 7:40 pm on October 29, 2024: contributor

utACK e60cecc8115d3b28be076792baa5e4ea26d353a6
laanwj approved
laanwj commented at 10:47 am on October 30, 2024: member

Code review ACK e60cecc8115d3b28be076792baa5e4ea26d353a6
achow101 commented at 8:51 pm on October 30, 2024: member

ACK e60cecc8115d3b28be076792baa5e4ea26d353a6
achow101 merged this on Oct 30, 2024
achow101 closed this on Oct 30, 2024
Sjors commented at 10:02 pm on October 30, 2024: member

TIL about the -test=<option> hook, that’s a nice solution. Post-merge concept ACK.
mzumsande deleted the branch on Oct 30, 2024

Contributors
mzumsande DrahtBot laanwj maflcko tdb3 rkrux BrandonOdiwuor achow101 Sjors

Review Requested
Sjors

Labels
Tests

test: Don’t enforce BIP94 on regtest unless specified by arg #31156

Code Coverage

Reviews