net: Disallow invalid HeadersSyncState due to lagging clock by hodlinator · Pull Request #35351 · bitcoin/bitcoin

hodlinator · 2026-05-21T19:56:41Z

Problem

Headers presync computes m_max_commitments from the elapsed time since the chain-start MTP plus MAX_FUTURE_BLOCK_TIME. When the local system clock is more than MAX_FUTURE_BLOCK_TIME behind the chain-start MTP, that elapsed value is negative, but it is used in arithmetic assigned to the unsigned commitment cap. This can turn the intended zero bound into a large cap, letting low-work headers presync continue instead of aborting when a reasonable commitment cap would have been exceeded.

Fix

Instead of allowing an invalid HeadersSyncState object to be created, emit an error and abort the node process.

Typically, the node will detect that the system clock is set too far in the past when comparing it to the chain tip during chain state loading and shut down before we start syncing headers. So in practice this is very unlikely to make a difference (might be possible if the system clock jumps backwards after we loaded the chain state).

Commits

Add functional and unit characterization tests pinning the current behavior.
The fix, along with corresponding test changes.

Replaces #35208 which was clamping m_max_commitments to zero and then letting the HeadersSyncState consume headers until the block height either reached the the next commitment_period point and aborted, or reached the minimum work threshold and succeeded (possible when having been offline for >144 blocks).

DrahtBot · 2026-05-21T19:56:49Z

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/35351.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	l0rinc, sedited
Concept ACK	w0xlt, dergoegge

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36087 (util: Add and use AssertUnreachable by maflcko)
#36074 (scripted-diff: [test] replace assert with Assert by maflcko)
#35820 (refactor: keep duration calculations typed by l0rinc)
#35642 (headersync: do parameter search at runtime by sipa)

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.

LLM Linter (✨ experimental)

Possible places where comparison-specific test macros should replace generic comparisons:

[src/test/headers_sync_chainwork_tests.cpp] BOOST_CHECK_THROW(CreateState(), HeadersSyncState::SystemClockError); -> consider BOOST_CHECK_EXCEPTION with a predicate that verifies the error message if the exact failure text matters, rather than checking only that this exception type was thrown.

^{2026-08-19 11:04:18}

w0xlt · 2026-05-22T14:01:03Z

Concept ACK

hodlinator · 2026-05-27T13:37:11Z

Been thinking more about this during the weekend.

One could posit that Bitcoin nodes should be more shelf stable in a post-apocalyptic future where NTP servers are unreachable. Re-establishing a shared accurate definition of current UTC time just from observing the sun's position could be challenging.

In that kind of scenario, preventing the node from continuing, as this PR does, could be problematic.

So should we do like #35208 and just set m_max_commitments to 0 instead? It would still be pretty unforgiving if start-height is very close to a multiple of the commitment_period - maybe allowing at least 1 commitment could be argued? (commitment_period is currently 641, giving an average of 961 blocks before aborting if we start at a random height).

Although, if clocks in the network disagree too much about UTC time, block propagation would also be suffering due to the rule to not accept blocks from too far into the future (MAX_FUTURE_BLOCK_TIME/2h). The current check failing would also mean that the we somehow ended up with a starting block read from disk that is dated more than 2h in the future, meaning our clock is not just out of sync with UTC but has also jumped backwards since we accepted the starting block. So I think the current approach in the PR is consistent with the rest of the node.

l0rinc

I'm not sure this is going in the right direction, seems weird to discover time drift in the constructor and catch on use site - instead of sanitizing the value before call.
Please see https://github.com/l0rinc/bitcoin/pull/186/commits for how I imagined simplifying this a lot more.

hodlinator

Thanks for your review & suggestions @l0rinc!
Pushed the straightforward ones for now.

I'm not sure this is going in the right direction, seems weird to discover time drift in the constructor and catch on use site - instead of sanitizing the value before call.

Curious what you think about sending unsigned max_commitments into HeadersSyncState() (see inline comment).

hodlinator · 2026-06-16T10:13:02Z

(Rebased against master in latest push to resolve conflicts).

l0rinc

lgtm, please see my remaining suggestion to reduce side-effects and simplify testing

hodlinator

(Latest few pushes after 1c3ad92d0bdcc66f252f647deb58f8becfce2772 attempt to reconcile differing viewpoints which me & @l0rinc partially discussed out-of-band last week).

l0rinc · 2026-07-06T15:42:46Z

ACK 8b680b2

@optout21, @vasild, @dergoegge, this is an alternative to the original acked PR, your re-review here would be welcome.

dergoegge · 2026-07-14T09:06:46Z

Concept ACK

I thought the original PR was also fine. Code here also looks fine, but I won't prioritize giving this a full review.

l0rinc · 2026-08-06T19:17:57Z

@hodlinator, can you please rebase the change?

l0rinc

ACK 70ec860

The approach looks correct: reject the invalid elapsed-time calculation before constructing HeadersSyncState, then trigger fatal shutdown.

I left a few non-blocking suggestions and some prose is also stale:

The PR description still says the constructor throws an exception.
The first commit message says commitment_period=1, although the test uses COMMITMENT_PERIOD (600), and says the fix flips the test although it replaces it with direct boundary checks.
The final commit subject says "local tip", but the comparison uses the chain-start header MTP, which is not necessarily the tip. Its body could also briefly explain why this condition warrants shutdown.

hodlinator

Thanks for your latest review @l0rinc, incorporated all of it in the latest push, with one exception:
You disagree that the chain start header is the same as the local tip - when would that not be the case?

Beyond your feedback I also added/corrected some comments in net_processing.cpp in the last commit.

l0rinc

ACK 0799f3b

This correctly rejects a negative elapsed interval before it can become a large unsigned presync commitment cap, prevents constructing an invalid HeadersSyncState, and routes the unexpected clock condition through fatal shutdown. The unit test covers the exact boundary, while the functional test covers the P2P-to-shutdown path. The focused unit and functional tests pass.

I left some suggestions to make the review easier by separating risky changes from pure refactors (which also help with making the characterization test changes minimal) - I applied these suggestions in https://github.com/l0rinc/bitcoin/pull/276/commits during local review.
I'm also okay with merging this as is and happy to rereview if any of the suggestion are taken.

You disagree that the chain start header is the same as the local tip - when would that not be the case?

My understanding is that the relevant presync path receives a peer-supplied full headers message, then looks up chain_start_header from its first header.
As far as I can tell it may be any known block (we only require valid PoW, continuity, and a connection to our block index), for example an older active-chain block or a stale-fork block. I’d call it the chain-start header rather than the local tip.

hodlinator · 2026-08-11T19:48:59Z

Thanks for the solid feedback again @l0rinc, agree it should make it easier to review. Took the changes from your branch with some variation, hope it's acceptable to you. The largest deviation from your l0rinc#276 end state is that I decided to const m_max_commitments.

l0rinc

ACK 1695d30

The commits are very easy to understand now, thanks for considering my suggestions! The remaining nits aren't important.

l0rinc · 2026-08-11T21:09:33Z

        /*chain_start=*/start_index,
-        /*minimum_required_work=*/min_work);
+        /*minimum_required_work=*/min_work,
+        /*max_commitments=*/*max_commitments);


8593ec4 refactor(p2p): Pass max commitments into HeadersSyncState():

nit, if you need to touch again:

Suggested change

/*max_commitments=*/*max_commitments);

*max_commitments);

sedited · 2026-08-14T12:01:54Z

Looked at the patch set a few days ago and it looks correct and fine to me, but I'm not completely sure yet of the approach. Could it be better to just crash immediately with an abort if the system clock is that far out of sync, akin to maflcko's patch here: https://github.com/bitcoin/bitcoin/pull/35676/changes#diff-c24fd5f39e56e513a81d84b494862608c1620c194e5a7efbded93953b6ef43e2R106 ? Externalizing the max commitments calculation does feel like bit of a code smell to me.

mzumsande

Looked at the patch set a few days ago and it looks correct and fine to me, but I'm not completely sure yet of the approach. Could it be better to just crash immediately with an abort if the system clock is that far out of sync, akin to maflcko's patch here: https://github.com/bitcoin/bitcoin/pull/35676/changes#diff-c24fd5f39e56e513a81d84b494862608c1620c194e5a7efbded93953b6ef43e2R106 ?

with "immediately", do you mean with that to check the system time vs best header in init and crash there without ever starting up network operations?

Externalizing the max commitments calculation does feel like bit of a code smell to me.

Another option could be to only do the time check with chain_start_header.GetMedianTimePast() in TryLowWorkHeadersSync(), and leave the max commitments calculation inside HeadersSync- so that we can change the internal check to an Assume(max_seconds_since_start >= 0);. Having to repeat the time check seems irrelevant performance-wise.

The node currently continues low-work headers presync and requests more headers when its clock is more than `MAX_FUTURE_BLOCK_TIME` behind the chain-start MTP. Record this behavior before the follow-up rejects the invalid elapsed-time calculation. The unit test covers HeadersSyncState() behavior while the functional test covers net_processing.cpp behavior. Co-authored-by: Lőrinc <pap.lorinc@gmail.com>

hodlinator · 2026-08-19T09:55:50Z

Latest push (4cf06b1) switches back to std::abort() and tries to handle the code smell¹ of externalizing the max_commitments calculation in the previous push (1695d30).

It reverts back to throwing an exception from HeaderSyncState(). This avoids duplicating the error condition logic inside TryLowWorkHeadersSync()². Using exceptions was previously pushed back against³, but it is how C++ constructors are supposed to report errors and keeps the unit test clean.

We should not proceed syncing headers from peers when the local system clock is incorrectly set. A node with a system clock set too far back will typically fail early during startup when the chainstate detects the tip to be too far in the future. This means that in practice we don't expect the failure to ever happen in net_processing.cpp. An exception is thrown from HeadersSyncState() in order to only compute the error condition once. An alternative would be to compute it a second time in TryLowWorkHeadersSync() to guard against calling HeadersSyncState(), and have an assert inside HeadersSyncState(). We shut down the process so possible resource leaks due to the exception should not be an issue, although none have been spotted. Throwing an exception also keeps the unit test straightforward. Co-authored-by: Lőrinc <pap.lorinc@gmail.com>

l0rinc · 2026-08-20T00:36:52Z

diff and code review ACK ff3e2e4

nit: the PR description still describes the old graceful-shutdown

sedited

ACK ff3e2e4

DrahtBot added the P2P label May 21, 2026

hodlinator mentioned this pull request May 21, 2026

net: cap future-MTP headers commitments #35208

Closed

DrahtBot added CI failed and removed CI failed labels May 21, 2026

DrahtBot mentioned this pull request May 22, 2026

test: NodeClockContext follow-ups #35114

Merged

l0rinc suggested changes Jun 6, 2026

View reviewed changes

Comment thread src/headerssync.cpp Outdated

Comment thread src/test/fuzz/headerssync.cpp

Comment thread src/headerssync.h Outdated

Comment thread src/test/fuzz/headerssync.cpp Outdated

Comment thread src/test/headers_sync_chainwork_tests.cpp

Comment thread src/net_processing.cpp Outdated

DrahtBot added the Needs rebase label Jun 9, 2026

hodlinator force-pushed the pr/35208_alt branch from 2cb8aa0 to d08db64 Compare June 16, 2026 09:42

hodlinator commented Jun 16, 2026

View reviewed changes

Comment thread src/test/fuzz/headerssync.cpp

Comment thread src/headerssync.cpp Outdated

Comment thread src/net_processing.cpp Outdated

Comment thread src/test/fuzz/headerssync.cpp Outdated

Comment thread src/test/headers_sync_chainwork_tests.cpp

hodlinator force-pushed the pr/35208_alt branch from d08db64 to 1c3ad92 Compare June 16, 2026 10:09

DrahtBot removed the Needs rebase label Jun 16, 2026

l0rinc suggested changes Jun 16, 2026

View reviewed changes

Comment thread src/test/fuzz/headerssync.cpp

Comment thread src/net_processing.cpp Outdated

This was referenced Jun 20, 2026

fuzz: exercise the transaction-handling path in process_message(s) #35482

Merged

Remove boost as a unit test runner #35587

Closed

hodlinator force-pushed the pr/35208_alt branch 2 times, most recently from 11ae9d4 to 6ee2df1 Compare July 3, 2026 20:14

DrahtBot added CI failed and removed CI failed labels Jul 3, 2026

l0rinc approved these changes Jul 4, 2026

View reviewed changes

Comment thread src/headerssync.cpp Outdated

Comment thread src/net_processing.cpp Outdated

Comment thread src/headerssync.h Outdated

DrahtBot mentioned this pull request Jul 4, 2026

headersync: do parameter search at runtime #35642

Open

hodlinator force-pushed the pr/35208_alt branch from 6ee2df1 to 8b680b2 Compare July 6, 2026 09:23

hodlinator commented Jul 6, 2026

View reviewed changes

Comment thread src/headerssync.h Outdated

Comment thread src/net_processing.cpp Outdated

Comment thread src/headerssync.cpp Outdated

This was referenced Jul 29, 2026

refactor: keep duration calculations typed #35820

Open

net_processing: process unique tx INVs only #35878

Merged

sedited requested a review from mzumsande August 6, 2026 13:05

DrahtBot added the Needs rebase label Aug 6, 2026

hodlinator force-pushed the pr/35208_alt branch from 8b680b2 to 70ec860 Compare August 7, 2026 11:25

DrahtBot removed the Needs rebase label Aug 7, 2026

l0rinc approved these changes Aug 8, 2026

View reviewed changes

Comment thread src/test/fuzz/headerssync.cpp Outdated

Comment thread src/test/headers_sync_chainwork_tests.cpp Outdated

Comment thread src/net_processing.cpp

DrahtBot requested a review from dergoegge August 8, 2026 18:58

hodlinator force-pushed the pr/35208_alt branch from 70ec860 to 0799f3b Compare August 10, 2026 08:49

hodlinator commented Aug 10, 2026

View reviewed changes

Comment thread src/test/fuzz/headerssync.cpp Outdated

Comment thread src/test/headers_sync_chainwork_tests.cpp Outdated

Comment thread src/net_processing.cpp

l0rinc approved these changes Aug 10, 2026

View reviewed changes

hodlinator force-pushed the pr/35208_alt branch from 0799f3b to 1695d30 Compare August 11, 2026 19:41

l0rinc approved these changes Aug 11, 2026

View reviewed changes

mzumsande reviewed Aug 17, 2026

View reviewed changes

hodlinator force-pushed the pr/35208_alt branch from 1695d30 to 4cf06b1 Compare August 19, 2026 09:54

hodlinator force-pushed the pr/35208_alt branch from 4cf06b1 to ff3e2e4 Compare August 19, 2026 11:04

DrahtBot added CI failed and removed CI failed labels Aug 19, 2026

This was referenced Aug 26, 2026

util: Add and use AssertUnreachable #36087

Draft

scripted-diff: [test] replace assert with Assert #36074

Draft

sedited approved these changes Aug 29, 2026

View reviewed changes

sedited requested a review from mzumsande August 29, 2026 12:36

Uh oh!

Conversation

hodlinator commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Commits

Uh oh!

DrahtBot commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)

Uh oh!

w0xlt commented May 22, 2026

Uh oh!

hodlinator commented May 27, 2026

Uh oh!

l0rinc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hodlinator commented Jun 16, 2026

Uh oh!

Uh oh!

Uh oh!

l0rinc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

l0rinc commented Jul 6, 2026

Uh oh!

dergoegge commented Jul 14, 2026

Uh oh!

l0rinc commented Aug 6, 2026

Uh oh!

l0rinc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

l0rinc left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hodlinator commented Aug 11, 2026

hodlinator commented May 21, 2026 •

edited

Loading

DrahtBot commented May 21, 2026 •

edited

Loading

l0rinc left a comment •

edited

Loading

mzumsande left a comment •

edited

Loading