rpc: fix mintime field testnet4, expand timewarp attack test

Sjors commented at 2:41 pm on January 3, 2025: member

#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.

It could be backported to v28. However I don’t expect (testnet4) pools to use mintime in practice, since under normal mainnet circumstances this would produce blocks an hour in the past.

Additionally this PR adjusts 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.

DrahtBot commented at 2:41 pm on January 3, 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/31600.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	tdb3, fjahr
Stale ACK	TheCharlatan

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31376 (Miner: never create a template which exploits the timewarp bug by darosior)
#30941 (test: simplify timewarp test by tdb3)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label RPC/REST/ZMQ on Jan 3, 2025

DrahtBot added the label CI failed on Jan 3, 2025

remcoros commented at 1:15 pm on January 4, 2025: none

ckpool uses curtime: https://bitbucket.org/ckolivas/ckpool/src/bb7b0aebe08ed99d45b8701af2d65bfcdbef0932/src/bitcoin.c#lines-196 public-pool uses mintime: https://github.com/benjamin-wilson/public-pool/blob/master/src/services/stratum-v1-jobs.service.ts#L76

does this mean that currently, public-pool will create an invalid block on testnet4 if in a difficulty-adjustment period and timestamps have moved too far forward?

Sjors commented at 1:50 pm on January 4, 2025: member

@remcoros a testnet4 pool that uses min_time could produce an invalid block each first block of the retarget period, since that’s the only time when mintime can be wrong.

At first glance, it seems that might happen with public-pool. The easiest workaround would be to use curtime for now.

fjahr commented at 2:26 pm on January 4, 2025: contributor

Concept ACK

TheCharlatan commented at 11:41 am on January 5, 2025: contributor

Additionally this PR adjusts the timewarp test to better illustrate the griefing attack discussed here

I think adding the example to the functional test is good, but it is not clear from the description what the griefing attack is that it seeks to prevent.

Sjors force-pushed on Jan 6, 2025

Sjors commented at 9:21 am on January 6, 2025: member

not clear from the description what the griefing attack is that it seeks to prevent.

It doesn’t prevent it, it merely demonstrates. It can only be prevented if the miner actually uses mintime or curtime, or with a consensus change to increase MAX_TIMEWARP to well above MAX_FUTURE_BLOCK_TIME.

I expanded the comment.

DrahtBot removed the label CI failed on Jan 6, 2025

in test/functional/mining_basic.py:149 in f04e472629 outdated

146+        # A timestamp that's more than MAX_TIMEWARP seconds in the future can
147+        # happen by accident, due to a combination of pool software that doesn't
148+        # use "curtime" AND has a faulty clock.
149+        #
150+        # But it could also be intentional, at the end of a retarget period, in
151+        # order to make the next block miner violate the time-timewarp-attack rule.

TheCharlatan commented at 1:38 pm on January 6, 2025:

I think this should also say that the next block miner also has to use their own wall clock time, or something from a hand-rolled calculation and not the one provided by the template?

Sjors commented at 2:00 pm on January 6, 2025:

I expanded the comment.

Sjors force-pushed on Jan 6, 2025

TheCharlatan approved

TheCharlatan commented at 7:55 pm on January 6, 2025: contributor

ACK 5ba770089e9ecf82645ae60aa2bc7f26085a5573

DrahtBot requested review from fjahr on Jan 6, 2025

in test/functional/mining_basic.py:157 in 5ba770089e outdated

155+        #
156+        # The attack is illustrated below.
157+        #
158+        # Force the next block to have a timestamp in the future:
159+        future = t + MAX_TIMEWARP + 1
160+        # Witout violating the 2 hour in the future rule

tdb3 commented at 3:07 am on January 8, 2025:

If retouching: Witout –> Without

in test/functional/mining_basic.py:201 in 5ba770089e outdated

196         bad_block.solve()
197         assert_raises_rpc_error(-25, 'time-timewarp-attack', lambda: node.submitheader(hexdata=CBlockHeader(bad_block).serialize().hex()))
198 
199-        bad_block.nTime = t + MAX_FUTURE_BLOCK_TIME - MAX_TIMEWARP
200+        self.log.info("Test timewarp protection boundary")
201+        bad_block.nTime = future - MAX_TIMEWARP - 1

tdb3 commented at 3:28 am on January 8, 2025:

future - MAX_TIMEWARP - 1 is just t, right? (future = t + MAX_TIMEWARP + 1). Would this be the same logical case as in lines 185-187?

Sjors commented at 10:27 am on January 8, 2025:

This test is supposed to illustrate the highest timestamp at which you trigger the timewarp error, and then the next one shows the first timestamp where you don’t.

Lines 185-187 illustrate what happens if a miner just uses their clock, which doesn’t have to be an edge case.

The difference would probably be more clear if I picked a higher number for future.

Sjors commented at 12:47 pm on January 16, 2025:

Done

in test/functional/mining_basic.py:205 in 5ba770089e outdated

204+        assert_raises_rpc_error(-25, 'time-timewarp-attack', lambda: node.submitheader(hexdata=CBlockHeader(bad_block).serialize().hex()))
205+
206+        good_block = copy.deepcopy(bad_block)
207+        good_block.nTime = future - MAX_TIMEWARP
208+        good_block.solve()
209+        node.submitheader(hexdata=CBlockHeader(good_block).serialize().hex())

tdb3 commented at 3:38 am on January 8, 2025:

Could re-use block instead of doing another copy?

Sjors commented at 12:47 pm on January 16, 2025:

Done

tdb3 commented at 3:39 am on January 8, 2025: contributor

Concept ACK

Planning to circle back and want to make sure #30941 isn’t applicable anymore before closing.

Sjors commented at 10:20 am on January 8, 2025: member

I didn’t know about #30941. Let me know if there’s anything there that you’d like me to add here. After that I’ll address the other feedback, in particular I should probably pick a higher number for future to better distinguish the wall clock scenario from the boundary condition check.

tdb3 commented at 12:02 pm on January 16, 2025: contributor

I didn’t know about #30941. Let me know if there’s anything there that you’d like me to add here. After that I’ll address the other feedback, in particular I should probably pick a higher number for future to better distinguish the wall clock scenario from the boundary condition check.

Since the test approach has changed, I’m not seeing more to add.

refactor: add GetMinimumTime() helper

Before bip94 there was an assumption that the minimum permitted
timestamp is GetMedianTimePast() + 1.

This commit splits a helper function out of UpdateTime() to
obtain the minimum time in a way that (on testnet4) takes the
timewarp attack rule into account.

e7f80ae063

rpc: fix mintime for testnet4

Previously in getblocktemplate only curtime took the timewarp rule into account.

Mining pool software could use either, though in general it should use curtime.

3c205321e1

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.

4d8dea9cb9

Sjors force-pushed on Jan 16, 2025

Sjors commented at 12:47 pm on January 16, 2025: member

Rebased, picked a different future timestamp to better distinguish the griefing attack test case from the boundary value check.

I also renamed t to wall_time for clarity.

tdb3 approved

tdb3 commented at 3:54 am on January 17, 2025: contributor

code review ACK 4d8dea9cb92efa362f72b0c2a5b89d312b452c0a

Comments were addressed. Clarity increased.

DrahtBot requested review from TheCharlatan on Jan 17, 2025

in test/functional/mining_basic.py:205 in 4d8dea9cb9

205+        bad_block.nTime = future - MAX_TIMEWARP - 1
206         bad_block.solve()
207-        node.submitheader(hexdata=CBlockHeader(bad_block).serialize().hex())
208+        assert_raises_rpc_error(-25, 'time-timewarp-attack', lambda: node.submitheader(hexdata=CBlockHeader(bad_block).serialize().hex()))
209+
210+        block.nTime = future - MAX_TIMEWARP

fjahr commented at 3:32 pm on January 20, 2025:

nit: Could have had anther info log here for completeness but completely optional

fjahr commented at 3:36 pm on January 20, 2025: contributor

utACK 4d8dea9cb92efa362f72b0c2a5b89d312b452c0a

The comments in the tests are nice!

rpc: fix mintime field testnet4, expand timewarp attack test #31600

Code Coverage & Benchmarks

Reviews

Conflicts