rpc: have getblocktemplate mintime account for timewarp #31600

Sjors · 2025-01-03T14:41:39Z

#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 · 2025-01-03T14:41:42Z

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.

remcoros · 2025-01-04T13:15:44Z

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 · 2025-01-04T13:50:42Z

@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 · 2025-01-04T14:26:47Z

Concept ACK

sedited · 2025-01-05T11:41:11Z

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 · 2025-01-06T09:21:10Z

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.

sedited · 2025-01-06T13:38:56Z

test/functional/mining_basic.py

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?

I expanded the comment.

sedited

ACK 5ba770089e9ecf82645ae60aa2bc7f26085a5573

tdb3

Concept ACK

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

test/functional/mining_basic.py

Sjors · 2025-01-08T10:20:48Z

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 · 2025-01-16T12:02:53Z

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 · 2025-01-16T12:47:25Z

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

code review ACK 4d8dea9cb92efa362f72b0c2a5b89d312b452c0a

Comments were addressed. Clarity increased.

fjahr

utACK 4d8dea9cb92efa362f72b0c2a5b89d312b452c0a

The comments in the tests are nice!

fjahr · 2025-01-20T15:32:08Z

test/functional/mining_basic.py

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

darosior · 2025-01-22T17:32:59Z

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

Sjors · 2025-01-22T17:48:25Z

@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 · 2025-01-22T17:51:08Z

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

darosior

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

darosior · 2025-01-22T19:42:45Z

test/functional/mining_basic.py

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

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.

It's in #31725

darosior · 2025-01-22T20:26:09Z

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

luke-jr · 2025-01-28T21:04:12Z

"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

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.

Previously in getblocktemplate only curtime took the timewarp rule into account. Mining pool software could use either, though in general it should use curtime.

Sjors · 2025-01-29T08:48:28Z

I was indeed waiting for more people to chime in.

Pushed the simplification to always apply the rule.

DrahtBot added RPC/REST/ZMQ CI failed labels Jan 3, 2025

This was referenced Jan 4, 2025

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

Merged

test: simplify timewarp test #30941

Closed

Sjors mentioned this pull request Jan 4, 2025

[28.x] 28.1 backports and final changes #31594

Merged

Sjors force-pushed the 2025/01/timewarp-oops branch from 0cf11cf to f04e472 Compare January 6, 2025 09:20

DrahtBot removed the CI failed label Jan 6, 2025

sedited reviewed Jan 6, 2025

View reviewed changes

Sjors force-pushed the 2025/01/timewarp-oops branch from f04e472 to 5ba7700 Compare January 6, 2025 14:00

sedited approved these changes Jan 6, 2025

View reviewed changes

DrahtBot requested a review from fjahr January 6, 2025 19:55

tdb3 reviewed Jan 8, 2025

View reviewed changes

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

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

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

Sjors force-pushed the 2025/01/timewarp-oops branch from 5ba7700 to 4d8dea9 Compare January 16, 2025 12:47

tdb3 approved these changes Jan 17, 2025

View reviewed changes

DrahtBot requested a review from sedited January 17, 2025 03:54

fjahr reviewed Jan 20, 2025

View reviewed changes

test/functional/mining_basic.py Outdated

Copy link

Contributor

fjahr Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

darosior reviewed Jan 22, 2025

View reviewed changes

glozow added this to the 29.0 milestone Jan 28, 2025

Sjors added 4 commits January 29, 2025 09:39

refactor: add GetMinimumTime() helper

0713548

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.

rpc: clarify BIP94 behavior for curtime

79d45b1

rpc: have mintime account for timewarp rule

0082f6a

Previously in getblocktemplate only curtime took the timewarp rule into account. Mining pool software could use either, though in general it should use curtime.

doc: release notes

e1676b0

Sjors force-pushed the 2025/01/timewarp-oops branch from fb2b68d to e1676b0 Compare January 29, 2025 08:42

Sjors changed the title ~~rpc: fix mintime field testnet4~~ rpc: have getblocktemplate mintime account for timewarp Jan 29, 2025

rpc: have getblocktemplate mintime account for timewarp #31600

rpc: have getblocktemplate mintime account for timewarp #31600

Uh oh!

Conversation

Sjors commented Jan 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

remcoros commented Jan 4, 2025

Uh oh!

Sjors commented Jan 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjahr commented Jan 4, 2025

Uh oh!

sedited commented Jan 5, 2025

Uh oh!

Sjors commented Jan 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sedited Jan 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sjors commented Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tdb3 commented Jan 16, 2025

Uh oh!

Sjors commented Jan 16, 2025

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr Jan 20, 2025

Choose a reason for hiding this comment

Uh oh!

darosior commented Jan 22, 2025

Uh oh!

Sjors commented Jan 22, 2025

Uh oh!

Sjors commented Jan 22, 2025

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

darosior Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

sedited Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

Sjors Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

darosior commented Jan 22, 2025

Uh oh!

luke-jr commented Jan 28, 2025

Uh oh!

Sjors commented Jan 29, 2025

Sjors commented Jan 3, 2025 •

edited

Loading

DrahtBot commented Jan 3, 2025 •

edited

Loading

Sjors commented Jan 4, 2025 •

edited

Loading

Sjors commented Jan 6, 2025 •

edited

Loading

sedited Jan 6, 2025 •

edited

Loading

Sjors commented Jan 8, 2025 •

edited

Loading