rpc: have getblocktemplate mintime account for timewarp

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.

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

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	darosior, tdb3, fjahr, TheCharlatan, achow101

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:

#31725 (test: clarify timewarp grace period griefing attack by Sjors)

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.

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 outdated

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!

darosior commented at 5:32 pm on January 22, 2025: member

Would have been nice if the unrelated demonstrative functional test was not bundled with the bug fix.

Sjors commented at 5:48 pm on January 22, 2025: member

@darosior the test fails without the code changes (they’re implicitly tested), so it’s annoying to separate. If it gets uncomfortably close for v29 I’ll do that though.

Sjors commented at 5:51 pm on January 22, 2025: member

Actually it’s not entangled, so would be easy to take the last commit out if necessary.

in test/functional/mining_basic.py:152 in 4d8dea9cb9 outdated

152+        #
153+        # But it could also be intentional, at the end of a retarget period, in
154+        # order to make the next block miner violate the time-timewarp-attack rule.
155+        # For this attack to succeed the victim miner needs to ignore both our
156+        # "curtime" and "mintime" values AND use wall clock time. This is true even
157+        # if the victim miner implements the MTP rule.

darosior commented at 7:42 pm on January 22, 2025:

In other words for this “attack” to succeed the miner needs to run broken software that completely ignores values provided by our own software, which this functional test is supposed to check in the first place.

It’s important to test that in the presence of a block as far in the future as possible our getblocktemplate would return a correct mintime, but i don’t think the Bitcoin Core functional tests are an appropriate place to demonstrate however broken software that disregards values provided by Bitcoin Core may or may not behave.

Don’t get me wrong from the purpose of the Delving thread it’s great that you translated the situation you’re concerned about into code. This is helpful for the purpose of this thread. But is it useful for the Bitcoin Core project to merge such demonstration code?..

TheCharlatan commented at 9:54 am on January 29, 2025:

I think there is room for the functional tests to demonstrate some “common misconfigurations” to help avoid regressions, but agree that this case here is probably best documented and described elsewhere.

Sjors commented at 10:02 am on January 29, 2025:

It’s in https://github.com/bitcoin/bitcoin/pull/31725

darosior commented at 8:18 pm on January 22, 2025: member

utACK 4d8dea9cb92efa362f72b0c2a5b89d312b452c0a on the code changes for the mintime bug fix. Not a fan of merging demonstration code for external (broken) software in the Bitcoin Core test suite, but the code in question is concise and related to what Core implements so.. :shrug:

darosior commented at 8:26 pm on January 22, 2025: member

People who reviewed this PR may be interested in giving #31376 a look.

fanquake commented at 11:41 am on January 23, 2025: member

Actually it’s not entangled, so would be easy to take the last commit out if necessary.

Given that this needs to be rebased, and one reviewer has asked for it, seems like a good time to do so?

DrahtBot added the label Needs rebase on Jan 23, 2025

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

Will need to rebase after #31376, and make sure the test still works. So I might as well split that last commit off.

Not a fan of merging demonstration code for external (broken) software in the Bitcoin Core test suite

I think it illustrates a design flaw in the timewarp fix proposal, which can be fixed by increasing the grace period. At that point the new test code would have to be removed, because it would require impossible parameters. But it’s fine to discuss that in a separate PR and not get distracted here.

Sjors force-pushed on Jan 23, 2025

Sjors renamed this:
~~rpc: fix mintime field testnet4, expand timewarp attack test~~
rpc: fix mintime field testnet4
on Jan 23, 2025

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

Rebased and dropped the more elaborate timewarp attack test. Opened #31725 for it.

For now I kept mintime unchanged on mainnet, i.e. it doesn’t enforce the timewarp rule. This choice is debatable though. It seems a bit problematic to have mintime reflect a rule that doesn’t exist yet. At the same time, some mining pool software might use mintime instead of curtime and so this partially defeats the purpose of #31376.

darosior commented at 3:26 pm on January 23, 2025: member

Why would you make mintime use different rules from curtime? This is completely unintuitive to anybody using the interface.

It seems a bit problematic to have mintime reflect a rule that doesn’t exist yet.

This is the whole purpose to have miners not produce invalid blocks! We will never be able to deploy a soft fork to make this rule a reality if miners are not protected beforehand. This was the whole point behind #31376. Restating the motivation there, we should not be providing the functionality to exploit a vulnerability in the protocol even without considering a soft fork to fix it.

Sjors commented at 4:08 pm on January 23, 2025: member

I agree it would be more intuitive. Also, having mintime always honor the timewarp rule simplifies the implementation.

providing the functionality to exploit a vulnerability in the protocol

That’s not really an issue. Anyone actually exploiting the timewarp attack needs to modify their system clock, or - much more likely - modify their pool software to ignore the time in our template.

This change and #31376 merely prevent accidentally violating the timewarp rule.

The meaning of getblocktemplate is defined by BIP22 (curtime) and BIP23 (mintime). So we’re a little less free to just change them like we do for other RPC calls.

curtime: the current time as seen by the server (recommended for block time) - note this is not necessarily the system clock, and must fall within the mintime/maxtime rules
mintime: the minimum time allowed

What your proposing changes mintime from “allowed” to “recommended” on mainnet, so its’ no longer compliant with the BIP - depending on how strict you are about it. The curtime definition was already a bit more flexible.

The question is: does it matter? Pragmatically it seems fine to have mintime account for BIP94, with the main arguments being that:

it avoids potential bugs
there’s no good reason to use a lower time. On mainnet this value is (almost?) always well below wall time.

DrahtBot removed the label Needs rebase on Jan 23, 2025

darosior commented at 8:31 pm on January 28, 2025: member

What’s the state of this? I’d love this to not miss the 29.0 deadline, in order to be as widely available to miners if a timewarp fix ever gets proposed, and given that miners may take years to upgrade. @Sjors’s last message reads like he agrees with me and was going to make the change, so i was waiting on that. Pinged him and turns out he was waiting to see if anybody was going to chime in about his last message. @fjahr @tdb3 since you previously ACKed this do you have an opinion? @luke-jr any opinion? Sjors suggested to ping you, as the author of BIP22.

glozow added this to the milestone 29.0 on Jan 28, 2025

luke-jr commented at 9:04 pm on January 28, 2025: member

“allowed” isn’t necessarily consensus rules. It would be strictly BIP 23 compatible to just reject rpc submissions with an earlier time. I don’t think we need to go that far though - probably fine to tolerate it

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 takes the
timewarp attack rule into account.

0713548137

rpc: clarify BIP94 behavior for curtime 79d45b10f1

rpc: have mintime account for timewarp rule

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

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

0082f6acc1

doc: release notes e1676b08f7

Sjors force-pushed on Jan 29, 2025

Sjors commented at 8:48 am on January 29, 2025: member

I was indeed waiting for more people to chime in.

Pushed the simplification to always apply the rule.

Sjors renamed this:
~~rpc: fix mintime field testnet4~~
rpc: have getblocktemplate mintime account for timewarp
on Jan 29, 2025

fjahr commented at 3:54 pm on January 29, 2025: contributor

I didn’t have time to write yesterday but I’ve been leaning towards having mintime honor the timewarp rule since it’s more intuitive and it’s inconsistent to say we want to prevent potential bugs but also maybe not all the bugs.

Did we have any blocks on mainnet in the last couple of years that would have violated the new mintime? If not, that should be a stronger argument that the mintime change is relatively safe. And if there are some maybe a compromise would be a deprecation cycle for the old behavior? I’m not necessarily in favor of that but maybe that helps get it into v29 if there is disagreement.

darosior approved

darosior commented at 4:35 pm on January 29, 2025: member

utACK e1676b08f7b0b9a6c8ed76e31f24faa03a3facc9 on the code changes

Could bikeshed the doc but i don’t think it’s necessary.

DrahtBot requested review from fjahr on Jan 29, 2025

DrahtBot requested review from tdb3 on Jan 29, 2025

tdb3 approved

tdb3 commented at 5:29 pm on January 29, 2025: contributor

brief code review re ACK e1676b08f7b0b9a6c8ed76e31f24faa03a3facc9

Most significant change observed was to “always apply”

fjahr commented at 0:23 am on January 30, 2025: contributor

tACK e1676b08f7b0b9a6c8ed76e31f24faa03a3facc9

Reviewed the code and verified that the test change covers the new behavior.

Did we have any blocks on mainnet in the last couple of years that would have violated the new mintime?

To answer my own question, I checked this with a small script and there were quite a few blocks before 400k that would have been below the new mintime but since then this has been a rare exception. Here is the full list of such blocks since 400k that I found (pool info from mempool.space):

0542095 (Braiins)
1629197 (Braiins)
2631927 (1THASH)
3655535 (TATMAS)
4724944 (Foundry)
5767826 (AntPool)

I did not limit this retargeting blocks because the goal of this exercise was to see if some specific miner software has a systemic issue in that area or if this is a frequent occurrence in general which might have been worrying. But neither appears to be the case.

Sjors commented at 7:51 am on January 30, 2025: member

@fjahr the new rule only applies to blocks at height % 2016 == 0, so I’m confused about the heights you found.

The change here only affects what miners put in their blocks, it doesn’t change the consensus check. So it shouldn’t be dangerous in any case.

TheCharlatan approved

TheCharlatan commented at 10:49 am on January 30, 2025: contributor

ACK e1676b08f7b0b9a6c8ed76e31f24faa03a3facc9

fjahr commented at 1:13 pm on January 30, 2025: contributor

@fjahr the new rule only applies to blocks at height % 2016 == 0, so I’m confused about the heights you found.

Did you read what I wrote below the heights?

I did not limit this retargeting blocks because the goal of this exercise was to see if some specific miner software has a systemic issue in that area or if this is a frequent occurrence in general which might have been worrying. But neither appears to be the case.

If we would have seen a lot more of these blocks it could have been just be a matter of time until one of them falls on a retargeting block. And had I just looked at retargeting blocks it could have just been pure luck that one of them did not fall on a retargeting block yet.

Sjors commented at 1:28 pm on January 30, 2025: member

I don’t understand what your script is looking for. Are you saying you applied the timewarp rule to every block instead, and then checked which block violated it?

fjahr commented at 1:30 pm on January 30, 2025: contributor

I don’t understand what your script is looking for. Are you saying you applied the timewarp rule to every block instead, and then checked which block violated it?

Yes, I checked for which blocks block_time < (prev_block_time - 600) applies in general.

achow101 commented at 7:33 pm on January 31, 2025: member

ACK e1676b08f7b0b9a6c8ed76e31f24faa03a3facc9

achow101 merged this on Jan 31, 2025

achow101 closed this on Jan 31, 2025

rpc: have getblocktemplate mintime account for timewarp #31600

Code Coverage & Benchmarks

Reviews

Conflicts