test: clarify timewarp grace period griefing attack

Sjors commented at 3:07 pm on January 23, 2025: member

Adjust the timewarp test to better illustrate the griefing attack discussed here: https://delvingbitcoin.org/t/timewarp-attack-600-second-grace-period/1326/19

Changing MAX_TIMEWARP to something > MAX_FUTURE_BLOCK_TIME in consensus.h and mining_basic.py will cause the updated test to fail. I’m not proposing such a change here of course. The new test should be useful guidance for pool software developers, for why they really should use curtime, or least not ignore mintime.

Additionally, if the proposal is changed to make MAX_TIMEWARP > MAX_FUTURE_BLOCK_TIME then this test will break, which could be used to demonstrate there’s no such griefing attack anymore.

Originally part of #31600.

DrahtBot commented at 3:07 pm on January 23, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31725.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Prabhat1308
Concept NACK	darosior

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

DrahtBot added the label Tests on Jan 23, 2025

DrahtBot added the label CI failed on Jan 26, 2025

DrahtBot removed the label CI failed on Jan 26, 2025

DrahtBot added the label Needs rebase on Jan 31, 2025

Sjors force-pushed on Feb 1, 2025

Sjors commented at 9:07 am on February 1, 2025: member

#31600 landed, ready for review

Sjors marked this as ready for review on Feb 1, 2025

DrahtBot removed the label Needs rebase on Feb 4, 2025

in test/functional/mining_basic.py:147 in 6065b78e22 outdated

147+            node.setmocktime(wall_time)
148             self.generate(self.wallet, 1, sync_fun=self.no_op)
149 
150-        self.log.info("Create block two hours in the future")
151-        self.nodes[0].setmocktime(t + MAX_FUTURE_BLOCK_TIME)
152+        self.log.info("Create block MAX_TIMEWARP < t < MAX_FUTURE_BLOCK_TIME in the future")

Prabhat1308 commented at 7:40 pm on February 11, 2025:

The log mentions t to be strictly less than MAX_FUTURE_BLOCK_TIME. But in line 165 , the assert statement is for future<=MAX_FUTURE_BLOCK_TIME.

Sjors commented at 9:18 am on February 12, 2025:

Changed the log statement to <=

in test/functional/mining_basic.py:264 in 6065b78e22 outdated

198-        bad_block.nTime = t + MAX_FUTURE_BLOCK_TIME - MAX_TIMEWARP - 1
199+        # It can also happen if the pool implements its own logic to adjust its
200+        # timestamp to MTP + 1, but doesn't take the new timewarp rule into
201+        # account (and ignores mintime).
202+        mtp = node.getblock(node.getbestblockhash())["mediantime"] + 1
203+        bad_block.nTime = mtp + 1

Prabhat1308 commented at 7:43 pm on February 11, 2025:

on previous line 199 , mtp is set to node.getblock(node.getbestblockhash())["mediantime"] + 1 . from the rpc commands , shouldnt mtp be just node.getblock(node.getbestblockhash())["mediantime"] ? and then nTime is correct with mtp + 1. Have checked it locally and the test passes.

Sjors commented at 9:18 am on February 12, 2025:

Indeed, fixed.

in test/functional/mining_basic.py:233 in 6065b78e22 outdated

178-        # The template will have an adjusted timestamp, which we then modify
179+        node.setmocktime(wall_time)
180+        # The template will have an adjusted timestamp.
181         tmpl = node.getblocktemplate(NORMAL_GBT_REQUEST_PARAMS)
182-        assert_greater_than_or_equal(tmpl['curtime'], t + MAX_FUTURE_BLOCK_TIME - MAX_TIMEWARP)
183+        assert_equal(tmpl['curtime'], future - MAX_TIMEWARP)

Prabhat1308 commented at 7:46 pm on February 11, 2025:

this is introducing a more stricter equality. Is it because we now have the liberty to choose future as we wish whereas previously it was t + MAX_FUTURE_BLOCK_TIME which not necessarily be equal to template’s curtime ?

Sjors commented at 9:17 am on February 12, 2025:

more stricter equality.

Stricter than what? I don’t fully remember what the original test was trying to do here. But we know the exact value to expect.

curtime is equal to nTime of the template, which is controlled by UpdateTime() in node/miner.cpp. It starts with the actual current time, but then increases if needed to account for the MTP+1 rule and timewarp - and no more than that.

Prabhat1308 commented at 7:48 pm on February 11, 2025: contributor

ACK 6065b78

Sjors force-pushed on Feb 12, 2025

DrahtBot commented at 0:44 am on August 11, 2025: contributor

There hasn’t been much activity lately. What is the status here?

Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

DrahtBot commented at 2:49 am on August 11, 2025: contributor

There hasn’t been much activity lately. What is the status here?

Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

test: clarify timewarp griefing attack

On testnet4 with the timewarp mitigation active, when pool software
ignores the curtime and mintime fields provided by the getblocktemplate
RPC or by createNewBlock() in the Mining interface, they are vulnerable
to a griefing attack.

The test is expanded to illustrate this.

37d22461aa

Sjors force-pushed on Aug 25, 2025

Sjors commented at 12:13 pm on August 25, 2025: member

@Prabhat1308 can you update your review to the latest version?

I’ve rebased it just in case.

Prabhat1308 commented at 12:54 pm on August 25, 2025: contributor

re-ACK 37d2246

fanquake commented at 10:50 am on December 3, 2025: member

@darosior @sedited want to leave a conceptual opinion here, given your comments in #31600?

darosior commented at 2:20 pm on December 3, 2025: member

This is adding a test that demonstrates how Bitcoin Core would reject a block from a broken mining pool software that:

ignores the block chain and set the block timestamp using wall clock time;
ignores the mintime value provided by Bitcoin Core’s block template;
ignores the curtime value provided by Bitcoin Core’s block template.

There is many, many ways a mining pool software that disregards consensus rules and values provided by Bitcoin Core’s block template may create an invalid block. I don’t think it’s useful to exercise one such convoluted scenario in particular in Bitcoin Core’s functional test suite. For this reason i am by default a (weakly held) Concept NACK on this change.

Weakly held because it’s simply a test change and i’m happy to be convinced otherwise.

Sjors commented at 2:54 pm on December 3, 2025: member

BIP54 has been updated to use a 2 hour grace period rather than the MAX_TIMEWARP = 600; which our code still uses.

If you change MAX_TIMEWARP to 7200 there’s no way to make the test in this PR pass. I’ll leave that as an exercise to the reader.

Note that the list of three bullet items boils down to a single mistake: using wall time for nTime. And as documented in the Delving post, that’s not a hypothetical mistake. It’s no longer a problem because BIP54 uses the longer grace period.

Sjors closed this on Dec 3, 2025

test: clarify timewarp grace period griefing attack #31725

Code Coverage & Benchmarks

Reviews

Conflicts