p2p: prevent IBD stall after empty headers #35839

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

Draft: seeking design input: This PR changes when a peer gives up the sole initial headers-sync slot. Before merging, I’d like to confirm whether retaining that slot after a valid empty headers response is intentional, or whether another eligible peer should be tried. The existing timeout does not hand off the slot when no other preferred download peer is available; an additional sync attempt also depends on receiving a qualifying new-block announcement.

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. The peer may simply be caught up with the node or stuck at the same height. Bitcoin Core itself can also return empty headers while its active chain is below minimumchainwork, so an empty response is not misbehavior. This can prevent IBD from progressing when no other preferred download peer is available.

Fix: After such a response from an inbound or manual peer, release its initial-sync slot and cancel its download deadline. Start a new response window so another eligible peer can take the slot before the same peer is selected again. Automatic full-relay and block-relay peers retain their existing timeout and old-chain eviction behavior.

Remaining cases: This does not prevent deliberate stalling. A block announcement can bypass the retry delay, and an already-known connecting header can clear it. Short non-empty responses can also end headers download without releasing the slot. Those cases, timeout handoff without a preferred peer, and early release for automatic outbound peers need separate design discussion.

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

Run these commands before and after the fix. The Python script connects 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, hodlinator

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35502 (refactor: extract per-message helpers from ProcessMessage (move-only) by w0xlt)

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.

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 8:51 AM on September 15, 2026:

thread #35839 (review):

            // Match the scheduler's one-day threshold and retain the timestamp so this peer cannot immediately retake the sync slot

Still not sure which scheduler this refers to. My best guess is that this is referring to the two 24h thresholds in PeerManagerImpl::SendMessages()?

Long lines in tests as in #35839 (review) seem more okay. But here we go far beyond the method's established convention of ~80 chars (past 120 too).

l0rinc commented at 10:26 PM on September 22, 2026:

Thanks, I’ve replaced the vague and long comment, 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

DrahtBot added the label CI failed on Sep 1, 2026

DrahtBot removed the label CI failed on Sep 1, 2026

in test/functional/p2p_initial_headers_sync.py:72 in be1f152ffe outdated

  68 | @@ -68,18 +69,14 @@ def assert_single_getheaders_recipient(self, peers):
  69 |  
  70 |      def test_initial_headers_sync(self):
  71 |          self.log.info("Test initial headers sync")
  72 | +        self.nodes[0].setmocktime(int(time.time()))

hodlinator commented at 8:25 AM on September 15, 2026:

nit: Could explain why:

        # Take control over time so we can trigger timeouts precisely.
        self.nodes[0].setmocktime(int(time.time()))

l0rinc commented at 10:19 PM on September 22, 2026:

Added something similar

in test/functional/p2p_initial_headers_sync.py:82 in be1f152ffe outdated

  79 |          peer1.wait_for_getheaders(block_hash=best_block_hash)
  80 | -        # An empty reply will clear the outstanding getheaders request,
  81 | -        # allowing additional getheaders requests to be sent to this peer in
  82 | -        # the future.
  83 | -        peer1.send_without_ping(msg_headers())
  84 | -

hodlinator commented at 8:28 AM on September 15, 2026:

nit: Could replace this with:

-         # An empty reply will clear the outstanding getheaders request,
-         # allowing additional getheaders requests to be sent to this peer in
-         # the future.
-         peer1.send_without_ping(msg_headers())
- 
+        # Don't answer the getheaders request since we want to time out later.
+

l0rinc commented at 10:20 PM on September 22, 2026:

Added something similar, thanks

in src/net_processing.cpp:3229 in be1f152ffe outdated

3224 | +        // it is a response to our last getheaders request, if there is one.
3225 | +        const bool is_response{peer.m_last_getheaders_timestamp != NodeClock::time_point{}};
3226 | +        peer.m_last_getheaders_timestamp = {};
3227 | +
3228 | +        // Automatic outbound peers already time out or get evicted
3229 | +        if (is_response && (pfrom.IsInboundConn() || pfrom.IsManualConn())) {

hodlinator commented at 8:42 AM on September 15, 2026:

remark: In my mind manually added nodes are even more trusted than outbound nodes, which are more trusted than inbound nodes. I guess there could still be some kind of hiccup causing manual nodes to not have all the headers though.

l0rinc commented at 10:29 PM on September 22, 2026:

Agreed, but keeping a manual peer selected after an otherwise perfectly valid empty headers response can prevent IBD from progressing. This releases the sync slot without disconnecting or penalizing the peer.

in src/net_processing.cpp:3228 in be1f152ffe

3219 | @@ -3220,15 +3220,31 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
3220 |          // If we were in the middle of headers sync, receiving an empty headers
3221 |          // message suggests that the peer suddenly has nothing to give us
3222 |          // (perhaps it reorged to our chain). Clear download state for this peer.
3223 | +        // A headers message with no headers cannot be an announcement, so assume
3224 | +        // it is a response to our last getheaders request, if there is one.
3225 | +        const bool is_response{peer.m_last_getheaders_timestamp != NodeClock::time_point{}};
3226 | +        peer.m_last_getheaders_timestamp = {};
3227 | +
3228 | +        // Automatic outbound peers already time out or get evicted

hodlinator commented at 9:01 AM on September 15, 2026:

Could you please include a reference to PeerManagerImpl::ConsiderEviction(), where outbound or block relay peers are evicted, either in the comment or the commit message?

l0rinc commented at 10:29 PM on September 22, 2026:

Added ConsiderEviction() to the code comment next to the connection-type check. Thanks.

in src/net_processing.cpp:1 in be1f152ffe outdated

hodlinator commented at 9:30 AM on September 15, 2026:

PR desc:

A new block announcement can still trigger the existing one-shot request.

This sounds like it re-enables the attack somewhat? Could add a note admitting so and that it should be fixed in a later PR.

Could also add a sentence about deferring fixing #35839 (review) to the PR desc.

Not sure the reproducer script linked in the PR desc adds that much beyond the functional tests. I guess it's nice to have a minimal example. But it feels insecure to encourage download and execution of script source code obscured behind a URL.

l0rinc commented at 10:37 PM on September 22, 2026:

Good points. I’ve documented both bypasses as remaining cases for separate discussion. I kept the manual reproducer because it isolates the stall, but I agree the command should let readers inspect the script before running it.

hodlinator commented at 9:31 AM on September 15, 2026: contributor

Concept ACK be1f152ffe18143651ca7456113d6d684b53da17

in test/functional/p2p_initial_headers_sync.py:146 in d6e96ec6f1 outdated

 141 | +        manual_peer.wait_for_connect()
 142 | +        node.p2ps.append(manual_peer)
 143 | +        manual_peer.wait_for_getheaders(block_hash=best_block_hash)
 144 | +        assert_equal(node.getpeerinfo()[0]["connection_type"], "manual")
 145 | +
 146 | +        node.bumpmocktime(HEADERS_RESPONSE_TIME_SEC + 1)

danielabrozzoni commented at 4:45 PM on September 16, 2026:

I'm confused, is the bumpmocktime really necessary? I think the point is that regardless of timeout, if an inbound/manual sends empty headers, we release the slot. The test seems to pass even if I comment this out

l0rinc commented at 10:39 PM on September 22, 2026:

This is meant to catch a possible race where a delayed empty response frees the slot, but the original retry window has expired and the same peer retakes it before the replacement connects. The bump makes that case reproducible. I’ve added a comment to clarify that.

mzumsande commented at 3:40 PM on September 18, 2026: contributor

Some observations:

this is not an attack. Any peer could just send nothing instead of sending an empty header and stall until the timeout hits. So this is more about interactions between honest nodes.
Empty header messages are sent by Bitcoin Core itself not only when we have no more headers, but also when the node is below minchainwork. I think this is important information that belongs into the OP.
ConsiderEviction is just one of two timeout mechanisms. m_headers_sync_timeout applies to all peers.
#34096 is not a permanent stall to my understanding due to m_headers_sync_timeout, see #34096 (comment)
Even if the stall was permanent, we'd just sync from another peer as soon as a new block is found, see https://github.com/bitcoin/bitcoin/blob/0e9018e8b65611b0769545e177110e4b7fc51244/src/net_processing.cpp#L4413-L4422.

I would say that there are two separate possible problems, none of which are critical: 1.) headerssync can finish early before we get to the today -1 threshold because our peer's chain is not far enough. This could happen due to an empty message or a non-full message, it could happen after or before MinChainWork is reached (so we may have a HeadersSync object or not). In this case, it is strictly unnecessary to wait for the timeout. Instead, we could have a shortcut, in which we release the current sync peer immediately, give it a refractory period, so that we can sync from another peer. But nothing here is specific to inbound/manual peers, plus we might as well not do it and wait ~10 minutes for the next block instead.

2.) When m_headers_sync_timeout hits and we do not have a preferred peer (which means we are in a very bad situation anyway), we do nothing. Instead, we could reset nSyncStarted, and possibly add a refractory period (this is optional in my opinion) so we could try sync from another non-preferred peer (e.g. an inbound).

I'm not sure about the current approach because it

unnecessarily adds a restriction to inbound/manual peers to the empty-headers case, when full outbounds would benefit from it just as well. I don't think resettting ConsiderEviction is so important here, we just need headers from someone to start IBD!
only addresses one of multiple possible paths in which we can return early from headers sync.

l0rinc marked this as a draft on Sep 22, 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.

8c57e7a5c4

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.

2bacbeff65

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.

Already-known connecting headers can still end the retry delay, and preventing deliberate slot reacquisition is deferred.

73e2593a04

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>

a09a3c6024

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.

45d2f0ad9c

l0rinc force-pushed on Sep 22, 2026

l0rinc commented at 11:07 PM on September 22, 2026: contributor

Thanks for the detailed reviews. I rebased the branch and clarified that empty headers can be a normal response, while #34096 may recover through the timeout or a new-block announcement. This PR addressed only one case, and since the broader headers-sync behavior change is more complicated than anticipated, I’m closing the PR so someone more familiar with this area can take it forward. @mzumsande / @hodlinator, would any of you be interested?

l0rinc closed this on Sep 22, 2026