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
Stale ACK	Prabhat1308

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

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:200 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:174 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: none

ACK 6065b78

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.

d016d4f6b6

Sjors force-pushed on Feb 12, 2025

test: clarify timewarp grace period griefing attack #31725

Code Coverage & Benchmarks

Reviews

Conflicts