Have miner account for timewarp mitigation, activate on regtest, lower nPowTargetTimespan to 144 and add test

Sjors commented at 8:47 am on August 20, 2024: member

Because #30647 reduced the timewarp attack threshold from 7200s to 600s, our miner code will fail to propose a block template (on testnet4) if the last block of the previous period has a timestamp two hours in the future. This PR fixes that and also adds a test.

The non-test changes in the last commit should be in v28, otherwise miners have to patch it themselves. If necessary I can split that out into a separate PR, but I prefer to get the tests in as well.

In order to add the test, we activate BIP94 on regtest.

In order for the test to run faster, we reduce its difficulty retarget period to 144, the same number that’s already used for softfork activation logic. Regtest does not actually adjust its difficulty, so this change has no effect (except for getnetworkhashps, see commit).

An alternative approach would be to run this test on testnet4, by hardcoding its first 2015 in the test suite. But since the timewarp mitigation is a serious candidate for a future mainnet softfork, it seems better to just deploy it on regtest.

The next commits add a test and fix the miner code.

The MAX_TIMEWARP constant is moved to consensus.h so both validation and miner code have access to it.

consensus: lower regtest nPowTargetTimespan to 144

This currently has no effect due to fPowNoRetargeting,
except for the getnetworkhashps when called with -1.

It will when the next commit enforces the timewarp attack mitigation on regtest.

dd154b0568

DrahtBot commented at 8:47 am on August 20, 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	achow101, fjahr, glozow

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Sjors force-pushed on Aug 20, 2024

in src/consensus/params.h:111 in 2781ab513b outdated

107@@ -108,6 +108,7 @@ struct Params {
108     /** Proof of work parameters */
109     uint256 powLimit;
110     bool fPowAllowMinDifficultyBlocks;
111+    /** Enfore BIP94 timewarp attack mitigation */

fjahr commented at 10:57 am on August 20, 2024:

This is also used to enforce the block storm mitigation.

Sjors commented at 11:25 am on August 20, 2024:

Updated the comment to reflect that.

in src/node/miner.cpp:37 in 2781ab513b outdated

32@@ -33,6 +33,13 @@ int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParam
33     int64_t nOldTime = pblock->nTime;
34     int64_t nNewTime{std::max<int64_t>(pindexPrev->GetMedianTimePast() + 1, TicksSinceEpoch<std::chrono::seconds>(NodeClock::now()))};
35 
36+    if (consensusParams.enforce_BIP94) {
37+        const int nHeight = pindexPrev->nHeight + 1;

fjahr commented at 11:12 am on August 20, 2024:

Could you choose a different name for this variable than nHeight? Aside from the outdated style next_height or something like that is describing it better.

Sjors commented at 11:25 am on August 20, 2024:

Renamed to height and added a comment.

fjahr commented at 11:12 am on August 20, 2024: contributor

Concept ACK

consensus: enable BIP94 on regtest e85f386c4b

Sjors force-pushed on Aug 20, 2024

glozow added the label Mining on Aug 20, 2024

glozow added this to the milestone 28.0 on Aug 20, 2024

glozow added the label Tests on Aug 20, 2024

in src/kernel/chainparams.cpp:540 in 775c335b3d outdated

536@@ -537,10 +537,10 @@ class CRegTestParams : public CChainParams
537         consensus.SegwitHeight = 0; // Always active unless overridden
538         consensus.MinBIP9WarningHeight = 0;
539         consensus.powLimit = uint256{"7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"};
540-        consensus.nPowTargetTimespan = 14 * 24 * 60 * 60; // two weeks
541+        consensus.nPowTargetTimespan = 24 * 60 * 60; // one day

glozow commented at 4:21 pm on August 20, 2024:

In order for the test to run faster, we reduce its difficulty retarget period to 144,

The difference between using 144 and 2016 = mining_basic.py takes 30sec longer on my machine, which doesn’t seem like such a big deal that we should change the difficulty adjustment period. I don’t feel strongly, but it could be easier to just count mining_basic.py as one of the 30-60sec functional tests. :shrug:

Sjors commented at 4:42 pm on August 20, 2024:

It felt a lot longer on my 2019 MBP, and I’m a bit worried the valgrind / sanitizer CI would suffer. But haven’t done benchmarking.

I think we should have changed this to 144 back when nMinerConfirmationWindow was introduced in #7575. Not sure why it wasn’t, @sipa?

sipa commented at 4:47 pm on August 20, 2024:

@Sjors I can only guess, but I assume it’s because I didn’t feel like making a consensus rule change to regtest at the same time?

Sjors commented at 5:12 pm on August 20, 2024:

That makes sense.

I don’t think it’s a big deal though. There’s three places where nPowTargetTimespan is accessed outside of test code:

In CalculateNextWorkRequired after the early return in regtest due to fPowNoRetargeting
In PermittedDifficultyTransition used by headerssync.{h,cpp}, after the early return in regtest due to fPowAllowMinDifficultyBlocks
In the value of DifficultyAdjustmentInterval() which is used: i. in ContextualCheckBlockHeader() for the timewarp attack, which we’re testing here ii. in miner.cpp which we’re testing here iii. in the getnetworkhashps RPC call, which we adjust the test for iv. in GetNextWorkRequired which on regtest always returns params.powLimit

achow101 commented at 4:52 pm on August 21, 2024:

With debug enabled, 2016 blocks takes 3.4 minutes for me, vs 20 seconds for 144. Without debug, it’s 54 seconds for 2016 and 15 seconds for 144.

Sjors commented at 8:41 am on August 22, 2024:

If we want to simulate a full timewarp attack, we’d need multiple such periods too.

glozow commented at 10:48 am on August 22, 2024:

I’m convinced, marking resolved.

in test/functional/mining_basic.py:125 in 775c335b3d outdated

120+        self.log.info("Test timewarp attack mitigation (BIP94)")
121+        node = self.nodes[0]
122+
123+        self.log.info("Mine until the last block of the retarget period")
124+        blockchain_info = self.nodes[0].getblockchaininfo()
125+        n = 144 - blockchain_info['blocks'] % 144 - 2

glozow commented at 4:23 pm on August 20, 2024:

It would be nice to reduce the magic numbers in this test a bit

DIFFICULTY_ADJUSTMENT_INTERVAL instead of 144, MAX_FUTURE_BLOCK_TIME instead of 2 * 3600, etc.

Sjors commented at 4:53 pm on August 20, 2024:

I reduced the magic.

glozow commented at 4:24 pm on August 20, 2024: member

concept ACK

Add timewarp attack mitigation test e929054e12

miner: adjust clock to timewarp rule 59ff17e5af

Sjors force-pushed on Aug 20, 2024

in test/functional/mining_basic.py:40 in 59ff17e5af

35 from test_framework.wallet import MiniWallet
36 
37 
38+DIFFICULTY_ADJUSTMENT_INTERVAL = 144
39+MAX_FUTURE_BLOCK_TIME = 2 * 3600
40+MAX_TIMEWARP = 600

achow101 commented at 4:34 pm on August 21, 2024:

In e929054e12210353812f440c685a23329e7040f7 “Add timewarp attack mitigation test”

This constant is unused in this commit.

Sjors commented at 8:40 am on August 22, 2024:

I’ll move it to the other commit, or find a way to use it here, if I need to retouch.

achow101 commented at 4:53 pm on August 21, 2024: member

ACK 59ff17e5af4e382cbe16f183767beef1bdcd9131

DrahtBot requested review from fjahr on Aug 21, 2024

DrahtBot requested review from glozow on Aug 21, 2024

fjahr commented at 7:35 pm on August 21, 2024: contributor

ACK 59ff17e5af4e382cbe16f183767beef1bdcd9131

Reviewed the code and verified that the newly added tests do actually test the change in behavior.

in src/consensus/params.h:112 in e85f386c4b outdated

107@@ -108,6 +108,10 @@ struct Params {
108     /** Proof of work parameters */
109     uint256 powLimit;
110     bool fPowAllowMinDifficultyBlocks;
111+    /**
112+      * Enfore BIP94 timewarp attack mitigation. On testnet4 this also enforces

glozow commented at 10:49 am on August 22, 2024:

typo nit e85f386c4b157b7d1ac16aface9bd2c614e62b46

0      * Enforce BIP94 timewarp attack mitigation. On testnet4 this also enforces

Sjors commented at 11:40 am on August 22, 2024:

Thanks, will fix if I need to retouch.

glozow commented at 11:13 am on August 22, 2024: member

ACK 59ff17e5af4e382cbe16f183767beef1bdcd9131

achow101 merged this on Aug 22, 2024

achow101 closed this on Aug 22, 2024

instagibbs commented at 4:45 pm on August 22, 2024: member

I tested this patchset with 2016 target timespan and tests passed as expected. I also added a boundary test case: https://github.com/bitcoin/bitcoin/pull/30698

Sjors deleted the branch on Aug 23, 2024

maaku commented at 7:59 pm on August 24, 2024: contributor

Hate to be that guy, but this form of the time-warp mitigation closes off the possibility of using something like forward blocks for scaling bitcoin on-chain in the future. That was my voiced objection to the “create consensus clean-up” on the mailing list. Of course it only applies to testnet4, but in practice this ends up throwing up a hurdle to later deployment of something like forward blocks, as it can no longer be tested on the primary testnet. Has there been consideration given to limiting the relative amount of time-warp compared to the last time-warp, instead of an absolute requirement that total timewarp be less than a certain %age?

Sjors commented at 8:03 am on August 26, 2024: member

@maaku this would be better to discuss on the mailinglist and/or Delving.

There’s ongoing discussion what addition / other mitigation is needed to handle another variant of the timewarp that is currently not fixed: https://delvingbitcoin.org/t/zawy-s-alternating-timestamp-attack/1062

Maybe that can be combined with what you have in mind.

We can always launch a testnet5 with completely different rules. I also vaguely remember a pull request that allowed for custom regtest rules.

maaku commented at 10:03 am on August 26, 2024: contributor

@Sjors I have brought this up on the mailing list, 4 months ago: https://groups.google.com/g/bitcoindev/c/CAfm7D5ppjo/m/Jiurx8AiAgAJ

achow101 referenced this in commit 5116dd4b83 on Aug 26, 2024

mzumsande commented at 2:08 pm on September 2, 2024: contributor

I’m late and only saw this PR in the release notes, but FWIW I think that changing regtest consensus rules in order to add test coverage for testnet is setting the wrong priority.

In my opinion, testnet 4 isn’t anywhere near important enough to justify that. Regtest, in particular unit and functional tests, primarily exists to test mainnet and should therefore follow it as close as possible. Rather than this, we should add tests to regtest that demonstrate that timewarp is not mitigated, which is the case for mainnet.

I also don’t think that the timewarp fix being a “serious candidate for a future mainnet softfork” is enough of a reason. At this point, it’s too unclear if, at which point in time and how exactly this will be activated in a future softfork.

maflcko commented at 4:22 pm on September 2, 2024: member

Regtest

Haven’t followed any of this in detail, but on regtest, modifying the consensus rule on the command line should be harmless and is already being done to closer mirror main-net, where the defaults don’t achieve it. It should be easy to apply in this case as well, if there is need to.

Sjors commented at 6:08 pm on September 2, 2024: member

Reviving #17032 seems like a good way to get good test coverage for the timewarp, with and without mitigation. And to avoid arguments on what should and should not be active on regtest.

we should add tests to regtest that demonstrate that timewarp is not mitigated, which is the case for mainnet

This can still be achieved using the previous releases functionality.

mzumsande commented at 8:20 pm on October 22, 2024: contributor

Forgot about this until last week - I opened issue #31137 to continue discussion.

Have miner account for timewarp mitigation, activate on regtest, lower nPowTargetTimespan to 144 and add test #30681

Code Coverage

Reviews