p2p: select another peer after empty headers #35839

pull l0rinc wants to merge 7 commits into bitcoin:master from l0rinc:l0rinc/p2p-empty-headers-sync changing 2 files +150 −37
  1. l0rinc commented at 8:52 PM on July 29, 2026: contributor

    Problem: During IBD, the node asks one peer at a time for the headers needed to catch up. That peer can send valid empty headers, which the node accepts while still treating the peer as active, so no replacement is selected. 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. When no other preferred download peer is available, the headers timeout cannot release that peer either.

    Fix: Stop treating the peer as active after an empty response while the best header is at least a day old, and clear its headers timeout. Keep the time of the recent getheaders request so the same peer is not selected again immediately, while a later block announcement can still retry it. If an outbound peer is already waiting to be disconnected because its chain is old, keep that check running after another peer takes over headers sync.

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

    Both runs use the checked-out build/bin/bitcoind; Python only supplies two inbound P2P peers.

    DATADIR="$(mktemp -d /tmp/bitcoin-empty-headers.XXXXXX)"
    trap 'build/bin/bitcoin-cli -regtest -datadir="$DATADIR" -rpcport=18454 stop >/dev/null 2>&1; rm -rf "$DATADIR"' EXIT
    
    { cmake -B build && cmake --build build -j --target bitcoind bitcoin-cli; } >/dev/null 2>&1
    build/bin/bitcoind -regtest -daemonwait -datadir="$DATADIR" -connect=0 -listen=1 -port=18455 -rpcport=18454
    time python3 '/Users/lorinc/Library/Application Support/JetBrains/CLion2026.1/scratches/scratch_758.py'
    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

  2. DrahtBot added the label P2P on Jul 29, 2026
  3. DrahtBot commented at 8:52 PM on July 29, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35839.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline and AI policy for information on the review process. A summary of reviews will appear here.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #35561 (net: move some CNodeState fields to Peer by Crypt-iQ)
    • #34565 (refactor: extract BlockDownloadManager from PeerManagerImpl 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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  4. l0rinc marked this as a draft on Jul 29, 2026
  5. l0rinc marked this as ready for review on Jul 29, 2026
  6. l0rinc force-pushed on Jul 29, 2026
  7. DrahtBot added the label CI failed on Jul 29, 2026
  8. l0rinc closed this on Jul 30, 2026

  9. l0rinc reopened this on Jul 30, 2026

  10. 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'
    
  11. 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?

  12. test: characterize empty headers sync stall
    During IBD, the node asks one peer at a time for the headers needed to catch up.
    An empty response leaves that peer selected, so no replacement is asked and the same peer can be retried immediately.
    
    Characterize both behaviors before changing them.
    fc25b1be02
  13. test: isolate block announcement setup
    The block-announcement test sends empty headers only to let the node ask that peer for headers again later.
    That setup depends on behavior this series changes.
    
    Advance mock time past the normal two-minute response wait so the test remains focused on selecting one additional peer per block announcement.
    9c76ffe82f
  14. refactor: name stale header check
    The node treats headers sync differently while its best header is at least a day old: it selects one peer at a time and applies a download timeout.
    
    Name that check once so peer selection, timeout handling, and empty-response handling use the same condition.
    297c0f7ca7
  15. refactor: extract initial headers cleanup
    Starting initial headers sync marks one peer as active, increments the number of active sync peers, and sets its download timeout.
    
    Extract the matching cleanup so timeout and empty-response handling clear those three pieces of state together.
    7de4fba5b8
  16. p2p: select another peer after empty headers
    During IBD, the node asks one peer at a time for headers.
    A valid empty response leaves that peer selected, so no replacement is asked.
    When all peers are inbound, the headers timeout cannot stop waiting on that peer either.
    
    After an empty response while the best header is at least a day old, stop treating the peer as active and clear its download timeout.
    Keep the time of the recent `getheaders` request so the same peer is not selected again immediately.
    Record that the peer was released so repeated empty responses preserve the delay, while a later block announcement can retry it.
    Clear the record when connecting headers arrive or a new initial request starts.
    
    This exposes a separate issue for outbound peers: releasing the selected peer also stops a pending disconnect for having an old chain.
    The next two commits characterize and correct that intermediate behavior.
    79aca0b97f
  17. test: characterize outbound old-chain disconnect
    Bitcoin Core gives an outbound peer time to show that its chain has caught up before disconnecting it.
    Releasing that peer from initial headers sync currently stops this check.
    
    Characterize the missing follow-up request and disconnect before keeping the check active.
    92a98ffb30
  18. p2p: continue pending old-chain disconnect
    Bitcoin Core gives an outbound peer time to show that its chain has caught up before disconnecting it.
    Releasing that peer from initial headers sync currently stops both the final `getheaders` request and the disconnect.
    
    Continue the pending check until the peer catches up or is disconnected.
    6d10e8e193
  19. l0rinc force-pushed on Jul 30, 2026
  20. 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.

  21. DrahtBot removed the label CI failed on Jul 30, 2026
  22. 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.

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


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-08-08 03:51 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me