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

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

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, danielabrozzoni

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

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 outdated

 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.

mzumsande commented at 4:35 PM on September 4, 2026:

moved some of the test into the first commit. The grace period test coverage makes no sense there though.

l0rinc commented at 3:37 PM on August 26, 2026: contributor

Concept ACK

in src/net_processing.cpp:821 in be32083ddb outdated

 817 | @@ -818,10 +818,7 @@ class PeerManagerImpl final : public PeerManager
 818 |      /** Send a version message to a peer */
 819 |      void PushNodeVersion(CNode& pnode, const Peer& peer);
 820 |  
 821 | -    /** Send a ping message every PING_INTERVAL or if requested via RPC (peer.m_ping_queued is true).
 822 | -     *  May mark the peer to be disconnected if a ping has timed out.
 823 | -     *  We use mockable time for ping timeouts, so setmocktime may cause pings
 824 | -     *  to time out. */
 825 | +    /** Send a ping message every PING_INTERVAL or if requested via RPC (peer.m_ping_queued is true). */

danielabrozzoni commented at 3:19 PM on August 31, 2026:

(Comment placed in random location) nit: In commit message be32083ddb17d5bc1667a56fe7f3b7a82f7e0e68, there's a typo: timout -> timeout

mzumsande commented at 4:35 PM on September 4, 2026:

fixed

in test/functional/p2p_ping_ibd.py:109 in e13760ad53

 104 | +        # us if they don't relax the ping timeout rules while requesting blocks.
 105 | +        # Note that the reason for the stall is not us, but the slowness of the peer itself, so
 106 | +        # they would disconnect a good and fast peer that is not at fault.
 107 | +        node.bumpmocktime(TIMEOUT_INTERVAL + 60)
 108 | +        assert_equal(self.bytes_sent(node, "pong"), pong_bytes)
 109 | +        assert peer.is_connected

danielabrozzoni commented at 4:45 PM on August 31, 2026:

In e13760ad536f15a3280ae555c65ff922cf820478: I don't understand what's the purpose of this check; if the peer was a Bitcoin Core node, we wouldn't be connected anymore (this is the first commit, so there's still ping timeout). The peer is still connected only because P2PInterface has no ping timeout.

mzumsande commented at 4:35 PM on September 4, 2026:

makes sense, I removed the check.

in src/net_processing.cpp:6445 in 9e74c2f672 outdated

6438 | @@ -6437,7 +6439,11 @@ bool PeerManagerImpl::SendMessages(CNode& node)
6439 |              }
6440 |          }
6441 |          // If the peer failed to answer our ping in time, disconnect due to timeout.
6442 | -        if (m_connman.ShouldRunInactivityChecks(node, now) &&
6443 | +        // Skip the check while it is serving us blocks and let the block download timeout above govern
6444 | +        // instead. Once the last new block was received, give the peer a grace period to answer the ping.
6445 | +        if (state.vBlocksInFlight.empty() &&
6446 | +            now > NodeSeconds{node.m_last_block_time.load()} + POST_BLOCK_PONG_GRACE &&

danielabrozzoni commented at 1:44 PM on September 2, 2026:

Opus 5 flagged this - I think it's true in theory, but I'm not sure in practice if it can ever happen, there's a lot of details around block relay that I don't really know and I'm trying to understand :)

You're using m_last_block_time, but it gets updated only upon receiving a block we didn't already have, which means that a peer that sends us block that we already had would get a shorter post block pong grace, or none at all.

https://github.com/bitcoin/bitcoin/blob/dc0395c5858a1d55239b82a834e5075cf2069219/src/net_processing.cpp#L3676-L3681

Suppose peerA has been sending us blocks, and not responding to our pings in the meantime. We only miss one block, the tip. Then, either of this happens:

peerB pushes one or more unsolicited BLOCK message with the tip
peerB is one of our high bandwidth compact block peers, and sends us the tip, unsolicited, via CMPCTBLOCK. (I think this particular condition can't happen if we were doing IBD with peerA, but it can happen that we have compact block peers if we were synced and then feel behind by a few blocks)

In any case, peerB sends us the last block in peerA's queue. Then peerA will be given a shorter post block pong grace, or none at all, if the last new block they sent was more than 1min ago. This is because peerA sending us the last block didn't update m_last_block_time, since the block wasn't new to us.

mzumsande commented at 4:38 PM on September 4, 2026:

I also don't think that this is a real issue in practice, because the original problem exists mostly during IBD, where bandwidth is spread over 10 peers and 16 blocks are queued at the same time. In IBD, we don't request a single block from multiple peers at the same time, so this wouldn't happen. Even if this could happen in theory as a one-off after catching up with the tip as in your scenario, it's a one-off and not a systematic problem as the status quo.

in src/net_processing.cpp:6439 in be32083ddb outdated

6435 | @@ -6453,6 +6436,17 @@ bool PeerManagerImpl::SendMessages(CNode& node)
6436 |                  return true;
6437 |              }
6438 |          }
6439 | +        // If the peer failed to answer our ping in time, disconnect due to timeout.

danielabrozzoni commented at 6:31 PM on September 2, 2026:

Surfaced using Opus 5, I checked manually and I think it's correct:

You were previously checking whether to disconnect for inactivity first, and only then deciding whether to send a new ping, based on peer.m_ping_queued and whether enough time passed from the last ping. Now you're doing the opposite, and what might happen is:

peer hasn't been responding to our ping, it's time to disconnect it
the user calls the ping RPC
in MaybeSendPing, peer.m_ping_queued is true, so pingSend gets set to true, and peer.m_ping_start = now and peer.m_ping_nonce_sent get reset.
When we reach the disconnection check here, m_ping_start will be fresh, and we won't disconnect.

The chances are of course slim, and I'm not sure what's the best way to fix...

You can save a snapshot of m_ping_queued and m_ping_start before MaybeSendPing is called, and here evaluate against those - not super clean, but will work
In MaybeSendPing you could honor the user's ping request only if there's not an outstanding ping, but I don't think that's correct
Maybe you can move MaybeSendPing down here, but I'm sure it has other implications...
You can document that this might happen and move on, the chances are slim, and if it happens it's not too big of a deal imho :)

mzumsande commented at 4:42 PM on September 4, 2026:

this feels like one of those fringe KI findings that I think don't really need to be addressed: As far as I can see, a user can meddle with pings already today on master - if they send a ping rpc every 19 minutes, an unresponsive peer will never be disconnected due to missing pings. For the particular issue here, they would need to time their ping exactly, which seems very unlikely. But I don't even think we need to fix the behavior on master.

danielabrozzoni commented at 6:46 PM on September 2, 2026: member

Concept ACK! I am slowly going through it and learning more about block relay :)

test: add functional test for pings during IBD

When the node has outstanding 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.

The second subtest documents the current behavior from the other side:
we disconnect a peer that is busy serving us the blocks we requested
from it, just because it didn't answer our ping in time. A later commit
changes this.

56c194e655

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 timeout decisions don't really
belong in a function called MaybeSendPing.

95dcf8f10d

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.

Update the functional test added earlier accordingly: the peer is no
longer disconnected while it is serving blocks, and only times out once
the grace period after the last block has passed.

c551ff40a4

mzumsande force-pushed on Sep 4, 2026

mzumsande commented at 4:43 PM on September 4, 2026: contributor

abeb912 to c551ff4: addressed feedback