Repository: bitcoin/bitcoin (language: cpp)

Issue:
Remove BIP94 from regtest
[BIP94](https://github.com/bitcoin/bips/blob/master/bip-0094.mediawiki) which, amongst others, fixes the timewarp attack on testnet4, has been activated on regtest by #30681 (commit https://github.com/bitcoin/bitcoin/pull/30681/commits/e85f386c4b157b7d1ac16aface9bd2c614e62b46), in order to allow having a functional test for the new testnet4 behavior.

As I argued in https://github.com/bitcoin/bitcoin/pull/30681#pullrequestreview-2275751672 I believe that regtest's main task is to test mainnet behavior, and, as such, it shouldn't diverge from mainnet consensus rules unless there is a very good reason (e.g. for situations where a soft fork has been merged and is a candidate for activation on mainnet). I don't think testnet4 is a good enough reason - for example, writing test coverage for the actual unfixed timewarp issue will no longer be easily possible in a functional test, and if we end up fixing timwarp in a future softwork we might do it in a slightly different way than BIP94, which seems like it could easily get messy.

Possibilities to fix this (if desired) include:
- allowing to pass consensus parameters via command line (Reviving https://github.com/bitcoin/bitcoin/pull/17032 or do something similar), so BIP94 would only be active in the subtest where it's needed.
- rewriting the `mining_basic.py` subtest `test_timewarp` somehow
- removing the test for `testnet4`, with the justification that testing mainnet should have priority over testing testnets.


Discussion / hints:
Concept ACK. i think it's a good point that regtest, at least by default, should be as closest as possible to mainnet, not testnet.
For the first option, the BIP94 timewarp behavior could perhaps be turned into a (versionbits) deployment?

Then mainnet and testnet3 can have it off (by not having a deployment defined), always on on testnet4, and it would be implicitly configurable on regtest using `-vbparams`?
Anything is probably fine here.  A pre-mined testnet4 chain can possibly be used for the test as well, instead.

Fix patch:
diff --git a/doc/release-notes-31156.md b/doc/release-notes-31156.md
new file mode 100644
index 0000000000000..c61cfc08b931a
--- /dev/null
+++ b/doc/release-notes-31156.md
@@ -0,0 +1,4 @@
+Test
+------
+
+The BIP94 timewarp attack mitigation (designed for testnet4) is no longer active on the regtest network. (#31156)
diff --git a/src/chainparams.cpp b/src/chainparams.cpp
index 68319e8e8b258..7966b80a2735f 100644
--- a/src/chainparams.cpp
+++ b/src/chainparams.cpp
@@ -44,6 +44,7 @@ void ReadSigNetArgs(const ArgsManager& args, CChainParams::SigNetOptions& option
 void ReadRegTestArgs(const ArgsManager& args, CChainParams::RegTestOptions& options)
 {
     if (auto value = args.GetBoolArg("-fastprune")) options.fastprune = *value;
+    if (HasTestOption(args, "bip94")) options.enforce_bip94 = true;
 
     for (const std::string& arg : args.GetArgs("-testactivationheight")) {
         const auto found{arg.find('@')};
diff --git a/src/common/args.cpp b/src/common/args.cpp
index 3029625ff72b8..833a0b28bd2fe 100644
--- a/src/common/args.cpp
+++ b/src/common/args.cpp
@@ -709,6 +709,7 @@ std::string HelpMessageOpt(const std::string &option, const std::string &message
 
 const std::vector<std::string> TEST_OPTIONS_DOC{
     "addrman (use deterministic addrman)",
+    "bip94 (enforce BIP94 consensus rules)",
 };
 
 bool HasTestOption(const ArgsManager& args, const std::string& test_option)
diff --git a/src/kernel/chainparams.cpp b/src/kernel/chainparams.cpp
index 0f128d4c56261..349e14a5d6eab 100644
--- a/src/kernel/chainparams.cpp
+++ b/src/kernel/chainparams.cpp
@@ -542,7 +542,7 @@ class CRegTestParams : public CChainParams
         consensus.nPowTargetTimespan = 24 * 60 * 60; // one day
         consensus.nPowTargetSpacing = 10 * 60;
         consensus.fPowAllowMinDifficultyBlocks = true;
-        consensus.enforce_BIP94 = true;
+        consensus.enforce_BIP94 = opts.enforce_bip94;
         consensus.fPowNoRetargeting = true;
         consensus.nRuleChangeActivationThreshold = 108; // 75% for testchains
         consensus.nMinerConfirmationWindow = 144; // Faster than normal for regtest (144 instead of 2016)
diff --git a/src/kernel/chainparams.h b/src/kernel/chainparams.h
index c4584600fdfa7..f7ea141c37c33 100644
--- a/src/kernel/chainparams.h
+++ b/src/kernel/chainparams.h
@@ -155,6 +155,7 @@ class CChainParams
         std::unordered_map<Consensus::DeploymentPos, VersionBitsParameters> version_bits_parameters{};
         std::unordered_map<Consensus::BuriedDeployment, int> activation_heights{};
         bool fastprune{false};
+        bool enforce_bip94{false};
     };
 
     static std::unique_ptr<const CChainParams> RegTest(const RegTestOptions& options);


Test patch:
diff --git a/test/functional/mining_basic.py b/test/functional/mining_basic.py
index aca71933ec503..ee617069ab6d1 100755
--- a/test/functional/mining_basic.py
+++ b/test/functional/mining_basic.py
@@ -122,6 +122,7 @@ def test_blockmintxfee_parameter(self):
     def test_timewarp(self):
         self.log.info("Test timewarp attack mitigation (BIP94)")
         node = self.nodes[0]
+        self.restart_node(0, extra_args=['-test=bip94'])
 
         self.log.info("Mine until the last block of the retarget period")
         blockchain_info = self.nodes[0].getblockchaininfo()