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

Sjors · 2024-08-20T08:47:12Z

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.

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.

DrahtBot · 2024-08-20T08:47:15Z

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.

fjahr

Concept ACK

src/consensus/params.h

src/node/miner.cpp

glozow

concept ACK

src/kernel/chainparams.cpp

test/functional/mining_basic.py

achow101 · 2024-08-21T16:34:05Z

test/functional/mining_basic.py

    assert_raises_rpc_error,
    get_fee,
 )
 from test_framework.wallet import MiniWallet


+DIFFICULTY_ADJUSTMENT_INTERVAL = 144
+MAX_FUTURE_BLOCK_TIME = 2 * 3600
+MAX_TIMEWARP = 600


In e929054 "Add timewarp attack mitigation test"

This constant is unused in this commit.

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

src/kernel/chainparams.cpp

achow101 · 2024-08-21T16:53:14Z

ACK 59ff17e

fjahr · 2024-08-21T19:35:58Z

ACK 59ff17e

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

glozow

ACK 59ff17e

glozow · 2024-08-22T10:49:27Z

src/consensus/params.h

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


typo nit e85f386

Suggested change

* Enfore BIP94 timewarp attack mitigation. On testnet4 this also enforces

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

Thanks, will fix if I need to retouch.

instagibbs · 2024-08-22T16:45:09Z

I tested this patchset with 2016 target timespan and tests passed as expected. I also added a boundary test case: #30698

maaku · 2024-08-24T19:59:37Z

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 · 2024-08-26T08:03:58Z

@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 · 2024-08-26T10:03:37Z

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

31378d4 test: Add time-timewarp-attack boundary cases (Greg Sanders) Pull request description: Basic addition to test case added in #30681 ACKs for top commit: fjahr: Code review ACK 31378d4 achow101: ACK 31378d4 tdb3: ACK 31378d4 Tree-SHA512: 7d18af9e7fac0ecb0fb5e4c009d6ce3f9af849b4abc54ae8cf681dc8b882aaa9b4cffd7f798b7193c76d89f96ec2cb6dbd13e341882d59b0580d3573ec675ffd

mzumsande

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 · 2024-09-02T16:22:16Z

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 · 2024-09-02T18:08:15Z

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 · 2024-10-22T20:20:22Z

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

…arp bug 733fa0b miner: never create a template which exploits the timewarp bug (Antoine Poinsot) Pull request description: This check was introduced in #30681 but only enabled for testnet4. To avoid potentially creating an invalid block template if a soft fork to fix the timewarp attack were to activate in the future, we should have this check on all networks. It also seems wise for our miner to not support it whether or not a soft fork activates to fix it at the consensus level. ACKs for top commit: Sjors: ACK 733fa0b fjahr: utACK 733fa0b TheCharlatan: ACK 733fa0b Tree-SHA512: 9b3bc8b26a57f93425b17dda80bcfac4ecb750a3d26bc3eb8df619135634e369ac15982fac0c9770b1df207bd2e418ffe02a98f37968f024e55262d97715a4f5

e1676b0 doc: release notes (Sjors Provoost) 0082f6a rpc: have mintime account for timewarp rule (Sjors Provoost) 79d45b1 rpc: clarify BIP94 behavior for curtime (Sjors Provoost) 0713548 refactor: add GetMinimumTime() helper (Sjors Provoost) Pull request description: #30681 fixed the `curtime` field of `getblocktemplate` to take the timewarp rule into account. However I forgot to do the same for the `mintime` field, which was hardcoded to use `pindexPrev->GetMedianTimePast()+1`. This PR adds a helper `GetMinimumTime()` and uses it for the `mintime` field. #31376 changed the `curtime` field to always account for the timewarp rule. This PR maintains that behavior. Note that `mintime` now always applies BIP94, including on mainnet. This makes future softfork activation safer. It could be backported to v28. ACKs for top commit: fjahr: tACK e1676b0 achow101: ACK e1676b0 darosior: utACK e1676b0 on the code changes tdb3: brief code review re ACK e1676b0 TheCharlatan: ACK e1676b0 Tree-SHA512: 0e322d8cc3b8ff770849bce211edcb5b6f55d04e5e0dee0657805049663d758f27423b047ee6363bd8f6c6fead13f974760f48b3321ea86f514f446e1b23231c

consensus: lower regtest nPowTargetTimespan to 144

dd154b0

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.

Sjors mentioned this pull request Aug 20, 2024

Move maximum timewarp attack threshold back to 600s from 7200s #30647

Merged

Sjors force-pushed the 2024/08/conervative-timewarp branch from d6c7713 to 2781ab5 Compare August 20, 2024 08:50

fjahr reviewed Aug 20, 2024

View reviewed changes

src/consensus/params.h Outdated Show resolved Hide resolved

src/node/miner.cpp Outdated Show resolved Hide resolved

consensus: enable BIP94 on regtest

e85f386

Sjors force-pushed the 2024/08/conervative-timewarp branch from 2781ab5 to 775c335 Compare August 20, 2024 11:25

glozow added the Mining label Aug 20, 2024

glozow added this to the 28.0 milestone Aug 20, 2024

glozow added the Tests label Aug 20, 2024

glozow reviewed Aug 20, 2024

View reviewed changes

src/kernel/chainparams.cpp Show resolved Hide resolved

test/functional/mining_basic.py Outdated Show resolved Hide resolved

Sjors added 2 commits August 20, 2024 18:49

Add timewarp attack mitigation test

e929054

miner: adjust clock to timewarp rule

59ff17e

Sjors force-pushed the 2024/08/conervative-timewarp branch from 775c335 to 59ff17e Compare August 20, 2024 16:53

achow101 reviewed Aug 21, 2024

View reviewed changes

DrahtBot requested review from fjahr and glozow August 21, 2024 16:53

glozow reviewed Aug 22, 2024

View reviewed changes

achow101 merged commit 338b9d8 into bitcoin:master Aug 22, 2024
16 checks passed

instagibbs mentioned this pull request Aug 22, 2024

test: Add time-timewarp-attack boundary cases #30698

Merged

Sjors deleted the 2024/08/conervative-timewarp branch August 23, 2024 12:40

Sjors mentioned this pull request Aug 26, 2024

2024-08-29 agenda bitdevsamsterdam/bitdevsamsterdam.github.io#22

Closed

Sjors mentioned this pull request Sep 2, 2024

UpdateTime() needs to account for timewarp fix on testnet4 #30614

Closed

Sjors mentioned this pull request Sep 2, 2024

Testnet4 consensus failure due to timewarp related "softfork" #30786

Closed

mzumsande reviewed Sep 2, 2024

View reviewed changes

mzumsande mentioned this pull request Oct 22, 2024

Remove BIP94 from regtest #31137

Closed

darosior mentioned this pull request Nov 26, 2024

Miner: never create a template which exploits the timewarp bug #31376

Merged

Sjors mentioned this pull request Jan 3, 2025

rpc: have getblocktemplate mintime account for timewarp #31600

Merged

Sjors mentioned this pull request Jan 24, 2025

Stratum v2 Template Provider common functionality Sjors/bitcoin#49

Closed

15 tasks

xyephy mentioned this pull request Feb 26, 2025

Upgrade to bitcoin v28 whiveio/whive#12

Open

Sjors mentioned this pull request Mar 28, 2025

miner: timelock the coinbase to the mined block's height #32155

Merged

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

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

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

Uh oh!

Conversation

Sjors commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

achow101 Aug 21, 2024

Choose a reason for hiding this comment

Uh oh!

Sjors Aug 22, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Aug 21, 2024

Uh oh!

fjahr commented Aug 21, 2024

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

glozow Aug 22, 2024

Choose a reason for hiding this comment

Uh oh!

Sjors Aug 22, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

instagibbs commented Aug 22, 2024

Uh oh!

maaku commented Aug 24, 2024

Uh oh!

Sjors commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maaku commented Aug 26, 2024

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Sep 2, 2024

Uh oh!

Sjors commented Sep 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mzumsande commented Oct 22, 2024

Uh oh!

Uh oh!

Sjors commented Aug 20, 2024 •

edited

Loading

DrahtBot commented Aug 20, 2024 •

edited

Loading

Sjors commented Aug 26, 2024 •

edited

Loading

Sjors commented Sep 2, 2024 •

edited

Loading