p2p: prevent IBD stall after empty headers #35839

l0rinc commented at 8:52 PM on July 29, 2026: contributor

Problem: While the best header is at least a day old, a node normally selects one peer for initial headers sync. An inbound or manual peer can answer with valid empty headers and remain selected, so another eligible peer is not asked. Empty headers may mean that the peer is caught up with the node or stuck at the same height, so the response is not misbehavior. When no other preferred download peer is available, the headers timeout will not release the peer, and automatic outbound eviction does not apply.

Fix: Release an inbound or manual peer from initial headers sync after an empty response while the best header is stale and cancel its download deadline, allowing another eligible peer to take over. Restart the normal response window when releasing the slot so the same peer is not immediately selected again. A new block announcement can still trigger the existing one-shot request. Automatic full-relay and block-relay peers keep their existing headers timeout and old-chain eviction behavior.

<details><summary>Manual reproducer</summary>

Both runs use the checked-out build/bin/bitcoind, with the downloaded Python script supplying only two inbound P2P peers.

{ cmake -B build && cmake --build build -j10 --target bitcoind; } >/dev/null 2>&1
build/bin/bitcoind -regtest -daemonwait -datadir="$(mktemp -d)" -connect=0 -listen=1 -port=18455
time python3 <(curl -fsSL https://gist.github.com/l0rinc/39e471dd6835fe332205d3812ebb9e92/raw)
killall bitcoind

The fixed node returns as soon as the replacement peer receives getheaders. On the old node, that wait times out after 10 seconds because the initial peer is still treated as active.

</details>

Fixes #34096

DrahtBot added the label P2P on Jul 29, 2026

DrahtBot commented at 8:52 PM on July 29, 2026: 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/35839.

Reviews

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

Type	Reviewers
Concept ACK	jeanpablojp

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

Conflicts

No conflicts as of last run.

l0rinc marked this as a draft on Jul 29, 2026

l0rinc marked this as ready for review on Jul 29, 2026

l0rinc force-pushed on Jul 29, 2026

DrahtBot added the label CI failed on Jul 29, 2026

l0rinc closed this on Jul 30, 2026

l0rinc reopened this on Jul 30, 2026

maflcko commented at 7:19 AM on July 30, 2026: member

Looks like CI fails on f169af459b5a1a6715fe392f270076cfaa31df0f:

test  2026-07-30T05:28:45.141085Z TestFramework (ERROR): Unexpected exception: 
                                   Traceback (most recent call last):
                                     File "/home/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 145, in main
                                       self.run_test()
                                     File "/home/runner/work/bitcoin/bitcoin/ci_build/test/functional/p2p_segwit.py", line 261, in run_test
                                       self.test_non_witness_transaction()
                                     File "/home/runner/work/bitcoin/bitcoin/ci_build/test/functional/p2p_segwit.py", line 112, in func_wrapper
                                       func(self, *args, **kwargs)
                                     File "/home/runner/work/bitcoin/bitcoin/ci_build/test/functional/p2p_segwit.py", line 308, in test_non_witness_transaction
                                       self.generate(self.wallet, 99)  # let the block mature
                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                     File "/home/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 662, in generate
                                       sync_fun() if sync_fun else self.sync_all()
                                                                   ^^^^^^^^^^^^^^^
                                     File "/home/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 743, in sync_all
                                       self.sync_blocks(nodes)
                                     File "/home/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 714, in sync_blocks
                                       raise AssertionError("Block sync timed out after {}s:{}".format(
                                   AssertionError: Block sync timed out after 60s:
                                     '44c1a1dd2c9cd9b6e27c3236cb82a66ac501e990bc4d84642fa9706b89169839'
                                     '0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206'

mzumsande commented at 5:31 PM on July 30, 2026: contributor

Could you explain the motivation a bit more - is this a countermeasure against deliberate stalling by malicious peers, or something that is sometimes observed with legit peers or maybe broken peers on mainnet? Or just something that could happen in theory?

l0rinc force-pushed on Jul 30, 2026

l0rinc commented at 9:43 PM on July 30, 2026: contributor

Looks like CI fails

It seems I split the change into too many parts, and there apparently isn't a simple enough seam here: releasing the peer while leaving the block-announcement retry for later creates a broken intermediate state. I squashed the two commits.

Could you explain the motivation a bit more

I found this while investigating crashes in code paths I’m more familiar with, rather than from a mainnet observation. My understanding is that an empty headers response is protocol-valid: the peer may be up to date with us or stuck at the same height, so it is not necessarily misbehaving. The second commit adjusted the test explicitly relying on this. I reproduced the stall locally with the reproducer included in this PR. I assume that translates to mainnet behavior. Updated PR description slightly, let me know if it makes sense.

DrahtBot removed the label CI failed on Jul 30, 2026

davidgumberg commented at 10:32 PM on August 4, 2026: contributor

I found this while investigating crashes in code paths I’m more familiar with, rather than from a mainnet observation.

This fixes #34096, isn't that the motivation? (https://github.com/l0rinc/bitcoin/pull/69)

#34096 happens on mainnet and requires the use of addnode, but I don't see why it couldn't work to stall a random new node, but I haven't reproduced this.

l0rinc commented at 10:52 PM on August 4, 2026: contributor

This fixes #34096, isn't that the motivation

Absolutely, it was in the queue for so long that I forgot about it, thanks for noticing, added it to the PR description. Can you check if it fixes the problem for you

in test/functional/p2p_initial_headers_sync.py:76 in fc25b1be02

  80 | +
  81 | +    def test_empty_headers_sync_slot(self):
  82 | +        self.log.info("Test empty headers response during initial sync")
  83 | +        peer1 = self.nodes[0].add_p2p_connection(P2PInterface())
  84 | +        best_block_hash = int(self.nodes[0].getbestblockhash(), 16)
  85 | +        peer1.wait_for_getheaders(block_hash=best_block_hash)

danielabrozzoni commented at 8:32 PM on August 24, 2026:

This wasn’t obvious to me on first read: wait_for_getheaders() removes the matching getheaders entry from last_message. Without knowing this, the assertion below at L96 looked useless, because I assumed it would have been true even if we didn't send any getheaders after receiving the unconnecting header.

Maybe it's worth it to add as a comment:

# wait_for_getheaders() consumes the matching entry from last_message,
# so a later "getheaders" entry represents a newly received request.
peer1.wait_for_getheaders(block_hash=best_block_hash)

l0rinc commented at 10:40 PM on August 29, 2026:

Thanks, that sequence is gone now. The rewritten characterization checks peer4.last_message directly, so it no longer depends on wait_for_getheaders() consuming an earlier message.

in test/functional/p2p_initial_headers_sync.py:100 in fc25b1be02

 104 | +        assert_equal(len(self.getheaders_recipients([peer2, peer3])), 0)  # TODO: Empty headers should release the sync slot.
 105 | +
 106 | +        peer1.send_and_ping(msg_headers())
 107 | +        unconnecting_header.hashPrevBlock = 2
 108 | +        unconnecting_header.solve()
 109 | +        peer1.send_and_ping(msg_headers([unconnecting_header]))

danielabrozzoni commented at 8:35 PM on August 24, 2026:

nit: it can be made slightly clearer with a comment:

Answer the previous getheaders with an empty response,
then send a different unconnecting header to trigger a new
request

l0rinc commented at 10:40 PM on August 29, 2026:

That repeated-empty and unconnecting-header sequence is gone in the rewritten test, so this no longer applies.

in src/net_processing.cpp:3242 in 79aca0b97f

3242 | +            peer.m_initial_headers_sync_released = true;
3243 | +        } else if (!peer.m_initial_headers_sync_released) {
3244 | +            // A headers message with no headers cannot be an announcement, so assume
3245 | +            // it is a response to our last getheaders request, if there is one.
3246 | +            // A peer that released its slot keeps its request timestamp as a backoff
3247 | +            // instead, so the scheduler does not hand the slot straight back to it.

danielabrozzoni commented at 12:50 PM on August 25, 2026:

nit: This should be in the other if branch

l0rinc commented at 10:41 PM on August 29, 2026:

That branch and m_initial_headers_sync_released are gone now. The remaining comment is next to the conditional timestamp clear.

in src/net_processing.cpp:3310 in 79aca0b97f

3303 | @@ -3290,8 +3304,10 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
3304 |  
3305 |      // If headers connect, assume that this is in response to any outstanding getheaders
3306 |      // request we may have sent, and clear out the time of our last request. Non-connecting
3307 | -    // headers cannot be a response to a getheaders request.
3308 | +    // headers cannot be a response to a getheaders request. A connecting response also
3309 | +    // ends any released-slot backoff.
3310 |      peer.m_last_getheaders_timestamp = {};
3311 | +    peer.m_initial_headers_sync_released = false;

danielabrozzoni commented at 1:31 PM on August 25, 2026:

(Found with gpt 5.6-sol): this means that a peer can send an already-known connecting header and bypass the backoff. Consider this:

Peer P receives the initial getheaders
P sends empty headers and is released with backoff
P sends one already-known header (such as our current best header)
The header connects, so we clear the backoff and the released flag
if no repleacement peer has taken the slot yet, SendMessages() can immediately select P again

If P wants to stall us for as long as possible, and there are other eligible peers, it could:

Receive getheaders
Hold the slot without replying until shortly before m_headers_sync_timeout (~15 minutes)
Send an empty headers, releasing the slot
Send an already known connecting header
If no other peer acquired the slot, acquire the slot, refreshing m_headers_sync_timeout
Repeat

(If there are no other eligble peers, P can stall us indefinitely because we won't disconnect, this is true for master too)

Now, I'm not sure if this is worth fixing or not, because I don't know if there are many ways to stall a peer during header sync, but if we decide not to fix, it should be at least pointed out in a comment

l0rinc commented at 10:42 PM on August 29, 2026:

a peer can send an already-known connecting header and bypass the backoff

Good catch, this still applies because connecting headers clear m_last_getheaders_timestamp. I started implementing this first but it got more complicated than I was comfortable with - we should fix it after this is merged. Instead I narrowed this PR to #34096's valid empty-response stall because I do not see a small fix for deliberate reacquisition that would not complicate low-work and continuation handling.

in src/net_processing.cpp:3308 in 79aca0b97f

3303 | @@ -3290,8 +3304,10 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
3304 |  
3305 |      // If headers connect, assume that this is in response to any outstanding getheaders
3306 |      // request we may have sent, and clear out the time of our last request. Non-connecting
3307 | -    // headers cannot be a response to a getheaders request.
3308 | +    // headers cannot be a response to a getheaders request. A connecting response also
3309 | +    // ends any released-slot backoff.

danielabrozzoni commented at 2:47 PM on August 25, 2026:

nit: I would change the order of the sentences, so we explain the case where headers connect first, and then when they don't:

// If headers connect, assume that this is in response to any outstanding getheaders
// request we may have sent, and clear out the time of our last request. This will also
// ends any released-slot backoff. Non-connecting
// headers cannot be a response to a getheaders request.

l0rinc commented at 10:43 PM on August 29, 2026:

That comment and the released-slot flag are gone in the rewrite, so this no longer applies.

danielabrozzoni commented at 2:57 PM on August 25, 2026: member

Partial review, up to 79aca0b97fd61fcf07b5e62f44aefe58946ae37c

I am still wrapping my head around the code, I'm not very familiar with it :)

I left a few nits that, imho, make the comments easier to follow, plus one issue that GPT-5.6-sol found. I reviewed it manually and think it’s correct, but I’m not sure whether it’s worth fixing or whether a comment pointing it out would be enough.

bitcoin deleted a comment on Aug 28, 2026

l0rinc force-pushed on Aug 29, 2026

l0rinc renamed this:
~~p2p: select another peer after empty headers~~
p2p: prevent IBD stall after empty headers
on Aug 29, 2026

l0rinc commented at 10:51 PM on August 29, 2026: contributor

Thanks for the review, rebased and simplified this a lot in the latest push: we don't have to fix every headers sync problem here, it was getting uncomfortably bloated...

The previous version released every initial headers-sync peer after an empty response. That also stopped automatic outbound old-chain eviction, so it needed a new release flag, helper refactors, broader tests, and 2 follow-up commits to keep eviction active. I reverted that machinery and the outbound-specific tests because they expanded the change beyond #34096. Automatic full-relay and block-relay peers now keep their existing bounded timeout and eviction behavior.

The current change focuses on the indefinite inbound/manual stall. During stale-header IBD, an inbound or manual peer can return valid empty headers while retaining the only sync slot. The node releases peer1’s initial-sync slot and cancels the download deadline attached to it, allowing another eligible peer to take over. It retains peer1’s recent getheaders timestamp only to prevent the scheduler from immediately assigning the free slot back to the same peer. Another eligible peer can take over, while a later block announcement can still trigger the existing one-shot request.

The first commit removes the block-announcement test’s dependency on empty-header behavior, the second characterizes the stalled replacement peer, and the third changes production behavior and flips that single expectation. Deliberate reacquisition with an already-known header remains separate follow-up work because I did not find a small fix that stayed independent of low-work headers sync.

l0rinc closed this on Aug 29, 2026

l0rinc reopened this on Aug 29, 2026

DrahtBot added the label CI failed on Aug 29, 2026

DrahtBot removed the label CI failed on Aug 30, 2026

jeanpablojp commented at 8:19 AM on August 31, 2026: contributor

Concept ACK

If the empty response arrives more than two minutes after the request, the peer is released and takes the slot back in the same SendMessages, with a fresh m_headers_sync_timeout. Answering that way every time keeps every other peer out of the sync and the deadline never fires. The retained timestamp is the request's, not the release's, so the backoff has already expired when the response arrives.

With mocktime advanced 121s before each empty response and another peer available, the merge base disconnects the peer at the deadline and this branch keeps it, eighteen initial getheaders in the log and none to the other peer. Same for manual connections. Is this intentional?

in src/net_processing.cpp:3217 in f374ee9995 outdated

3208 | @@ -3209,15 +3209,28 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
3209 |          // If we were in the middle of headers sync, receiving an empty headers
3210 |          // message suggests that the peer suddenly has nothing to give us
3211 |          // (perhaps it reorged to our chain). Clear download state for this peer.
3212 | +        // Automatic outbound peers keep their existing timeout and old-chain eviction state
3213 | +        bool keep_request_timestamp{false};
3214 | +        if (peer.m_last_getheaders_timestamp != NodeClock::time_point{} && (pfrom.IsInboundConn() || pfrom.IsManualConn())) {
3215 | +            LOCK(cs_main);
3216 | +            if (m_chainman.m_best_header->Time() <= NodeClock::now() - 24h) {
3217 | +                keep_request_timestamp = true;

jeanpablojp commented at 8:19 AM on August 31, 2026:

This is outside the fSyncStarted check, so a peer that isn't holding the slot gets the backoff too, and the reason in the commit message doesn't cover that case. Moving it inside keeps the tests green here. Worth it?

l0rinc commented at 3:16 PM on August 31, 2026:

Good catch, even better, we don't need two separate conditions and we can modify m_last_getheaders_timestamp directly (we just have to bump peer.m_last_getheaders_timestamp = {} to the top)

l0rinc commented at 5:28 PM on September 1, 2026:

Looks like I forgot to push yesterday, see the latest push

in test/functional/p2p_initial_headers_sync.py:124 in f374ee9995 outdated

 119 | +        # Disconnect the announcement peers so peer4 is the only eligible replacement
 120 | +        peer2.peer_disconnect()
 121 | +        peer3.peer_disconnect()
 122 | +        self.nodes[0].wait_until(lambda: self.nodes[0].num_test_p2p_connections() == 1)
 123 | +
 124 | +        peer1.send_and_ping(msg_headers())

jeanpablojp commented at 8:19 AM on August 31, 2026:

IsManualConn() is the only arm of the guard with no coverage, and removing it breaks no test. addconnection has no manual type, but you can build one with addnode onetry on top of peer_accept_connection.

l0rinc commented at 5:46 PM on August 31, 2026:

Another good catch, thanks. I added a manual-peer test using addnode onetry on top of peer_accept_connection(), and split the inbound and manual cases into separate characterization/fix pairs. Removing either guard arm now breaks its corresponding test. Added you as coauthor, thanks.

l0rinc commented at 5:49 PM on August 31, 2026: contributor

Rebased after the related lagging-clock headers-sync fix.

Thanks @jeanpablojp, took all your suggestions. This update starts the retry delay when a peer releases the slot, splits inbound and manual handling into separate characterization/fix pairs, flattens the nested release conditions, and adds delayed manual-peer coverage.

in test/functional/p2p_initial_headers_sync.py:94 in f374ee9995 outdated

  90 | @@ -94,17 +91,17 @@ def test_initial_headers_sync(self):
  91 |              assert "getheaders" not in peer2.last_message
  92 |              assert "getheaders" not in peer3.last_message
  93 |  
  94 | +        self.nodes[0].bumpmocktime(HEADERS_RESPONSE_TIME_SEC + 1)

hodlinator commented at 7:50 AM on September 1, 2026:

Could you please add a comment here in the code describing why we bump the mocktime before announcing the block?

Same for the later bump.

l0rinc commented at 5:29 PM on September 1, 2026:

I was travelling yesterday, looks like my force push timed out and you were reviewing the previous version - my mistake, added the code comments and the previous restructure

in src/net_processing.cpp:3232 in f374ee9995 outdated

3230 |              m_headers_presync_stats.erase(pfrom.GetId());
3231 |          }
3232 | -        // A headers message with no headers cannot be an announcement, so assume
3233 | -        // it is a response to our last getheaders request, if there is one.
3234 | -        peer.m_last_getheaders_timestamp = {};
3235 | +        // Empty headers cannot be an announcement, so clear the request timestamp unless it is retained as a retry delay

hodlinator commented at 8:04 AM on September 1, 2026:

nanonit: 80 columns?

        // Empty headers cannot be an announcement, so clear the request
        // timestamp unless it is retained as a retry delay.

l0rinc commented at 4:33 PM on September 1, 2026:

I prefer the code and especially comments to occupy the space proportional to their usefulness and how often we need them, so I usually try to compress all info on a single line for new comments - if they're really needed.

in src/net_processing.cpp:3216 in f374ee9995 outdated

3208 | @@ -3209,15 +3209,28 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
3209 |          // If we were in the middle of headers sync, receiving an empty headers
3210 |          // message suggests that the peer suddenly has nothing to give us
3211 |          // (perhaps it reorged to our chain). Clear download state for this peer.
3212 | +        // Automatic outbound peers keep their existing timeout and old-chain eviction state
3213 | +        bool keep_request_timestamp{false};
3214 | +        if (peer.m_last_getheaders_timestamp != NodeClock::time_point{} && (pfrom.IsInboundConn() || pfrom.IsManualConn())) {
3215 | +            LOCK(cs_main);
3216 | +            if (m_chainman.m_best_header->Time() <= NodeClock::now() - 24h) {

hodlinator commented at 8:12 AM on September 1, 2026:

Could you please add a comment explaining why we check this condition?

"Only keep the header request timestamp for this peer if our local best header is more than 1 day old since [reason not immediately obvious]"

l0rinc commented at 5:29 PM on September 1, 2026:

Added, let me know if it helps

hodlinator commented at 11:56 AM on September 1, 2026: contributor

Will take some time to fully wrap my brain around this, thanks for at least decreasing the scope already.

l0rinc force-pushed on Sep 1, 2026

test: isolate block announcement setup

The block-announcement checks send empty headers only to let the same peer receive another request.
The later fix changes how empty responses affect request timing.

Advance mock time past the normal two-minute response window so the test remains focused on selecting one additional peer per announcement.

ee9662eb28

test: characterize inbound empty headers stall

During IBD, a delayed empty `headers` response leaves an inbound peer selected for initial headers sync, so an eligible replacement receives no `getheaders` request.

Extend the existing test with this stalled replacement before changing inbound peer handling.

3089751763

p2p: release inbound peer after empty headers

During IBD, an inbound peer can answer the initial `getheaders` request with empty `headers` and retain the only sync slot.
With no preferred download peer available, the headers timeout will not release it.

Release the slot after an empty response while the best header is at least a day old.
Restart the normal response window so an eligible replacement is preferred before the same peer can be selected again.
A new block announcement can still trigger the existing one-shot request.

Automatic outbound peers keep their existing headers timeout and old-chain eviction behavior.

f248d495b0

test: characterize manual empty headers stall

A manual peer can also retain the initial headers-sync slot after a delayed empty `headers` response, so an eligible replacement receives no `getheaders` request.

Add the corresponding stalled-replacement case before extending empty-response handling to manual connections.

Co-authored-by: JP <jeanpablo.jp@hotmail.com>

d6e96ec6f1

p2p: release manual peer after empty headers

After an empty `headers` response, a manual peer can also retain the initial headers-sync slot.
With no other preferred download peer, the timeout does not release it, and automatic outbound eviction does not apply.

Apply the same slot release and retry delay used for inbound peers, allowing another eligible peer to take over.

be1f152ffe

l0rinc force-pushed on Sep 1, 2026

DrahtBot added the label CI failed on Sep 1, 2026

DrahtBot removed the label CI failed on Sep 1, 2026