p2p: Suspend ping timeout while downloading blocks from a peer #36080

pull mzumsande wants to merge 4 commits into bitcoin:master from mzumsande:202608_ping_ibd changing 3 files +234 −18

mzumsande commented at 3:59 PM on August 25, 2026: contributor
While serving blocks, there is a system in place that prioritizes a peer's block requests before answering other p2p messages: See https://github.com/bitcoin/bitcoin/blob/11090c8bb359f894ef7d97b65aff52fe8191aec1/src/net_processing.cpp#L5436

As a result it can happen that if we do IBD with a low download bandwidth (that is distributed over 10 peers) a peer will not get around to answering our ping before the timeout of 20 minutes, in which case we would disconnect them, although they have done nothing wrong and are not even slow themselves (we are). This situation has been described in #35761.

This PR fixes the issue by not enforcing the ping timeout from a peer while downloading blocks from them. In order to do that, the ping timeout check is moved out of MaybeSendPing() (which was a slightly awkward place anyway, given the name of the function) and suspended until there are no longer blocks in flight with that peer (with a grace period, so that we don't disconnect immediately after the last block was received before the peer got a chance to send us the pong).

Note that during block download, there are still other timeouts:
- A dynamic timeout (BLOCK_DOWNLOAD_TIMEOUT_BASE / BLOCK_DOWNLOAD_TIMEOUT_PER_PEER) which will result in a timeout of 600s × (1 + 0.5×9) = 55 minutes per block when downloading from 10 peers in parallel
- the stalling logic which hits if the peer is much slower in comparison to other peers
- the socket inactivity check disconnects a peer that hasn't sent us anything at all in the last 20 minutes.
So the ping timeout didn't add much value anyway in that situation.

Fixes #35761

test: add functional test for pings during IBD

When the node has oustanding block requests, it will first
serve all of those before answering pings. If this happens
slowly because the requestor of the blocks has a slow connection,
they would disconnect our node due to a ping timeout, even though
it did nothing wrong.

e13760ad53

p2p: move ping timeout check into SendMessages

The check is relocated from MaybeSendPing to the timeout
section of SendMessages, right after the block download timeout.

This is in preparation for the following commit, which makes the ping timeout depend
on whether the peer is currently serving us blocks.
That requires CNodeState, which is guarded by cs_main and therefore not reachable from MaybeSendPing.

Moving it makes sense anyway, since timout decisions don't really
belong in a function called MaybeSendPing.

be32083ddb

p2p: Don't apply ping timeout while downloading blocks

If the peer is also running bitcoin core, they will prioritize
serving the blocks over answering pings. If this is slow due to
our own download speed, we could timeout the peer even though
they did nothing wrong.

Therefore suspend the check while downloading blocks.
If the peer is slow, we have other mechanisms (socket timeout,
block request timeout logic)
to disconnect them, so the ping timeout wasn't necessary anyway.

Once the last new block is received, give the peer a grace period to send
us the pong.

9e74c2f672

DrahtBot added the label P2P on Aug 25, 2026
DrahtBot commented at 3:59 PM on August 25, 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/36080.



Reviews

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

Type Reviewers

Concept ACK l0rinc

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:
- #35522 (refactor: Extract per-message helpers from SendMessages() (move-only) by pablomartin4btc)
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.
mzumsande force-pushed on Aug 25, 2026
DrahtBot added the label CI failed on Aug 25, 2026
test: check that the ping timeout is not enforced in IBD abeb912d45
mzumsande force-pushed on Aug 25, 2026
DrahtBot removed the label CI failed on Aug 25, 2026
in test/functional/p2p_ping_ibd.py:160 in abeb912d45
```
 154 | @@ -114,9 +155,60 @@ def test_pong_delay_ibd(self):
 155 |          peer.wait_until(lambda: "pong" in peer.last_message, timeout=120)
 156 |          assert_equal(peer.last_message["pong"].nonce, PING_NONCE)
 157 |          assert_equal(peer.blocks_received, NUM_GETDATA)
 158 | +        node.disconnect_p2ps()
 159 | +
 160 | +    def test_ping_timeout_ibd(self):
```
l0rinc commented at 3:36 PM on August 26, 2026:

abeb912 test: check that the ping timeout is not enforced in IBD:

nit: whenever there's a test after the fix, I have a hard time understanding what the behavior was before the fix, it's why I usually add a characterization test before the fix to document the previous behavior, showing that intermediary refactors don't update the test (i.e. aren't changing behavior) while the fix commit only does surgical changes to the assertions, documenting exactly how the fix changes the assumptions, see #35260

It would help with the review if most of the tests would be moved before the fix to help us understand both the before and after states.
l0rinc commented at 3:37 PM on August 26, 2026: contributor

Concept ACK

Type	Reviewers
Concept ACK	l0rinc

Contributors

Labels

Linked (view graph)

#35761 Unnecessary disconnections due to PONG delays during the initial block download

p2p: Suspend ping timeout while downloading blocks from a peer #36080

Code Coverage & Benchmarks

Reviews

Conflicts