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.

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36080
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36080.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36080#author-ralph

Reviews

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

Type	Reviewers
ACK	maflcko, fjahr, danielabrozzoni, sedited
Concept ACK	l0rinc
Stale ACK	AndreaDiazCorreia

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 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35502.svg"></sub> (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.

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.

maflcko commented at 1:00 PM on September 29, 2026:

nit in the second commit 95dcf8f10dc83fd1d5048fc82027f53b3f83dba3: Obviously this is fine, but personally I am not a fan of expanding massive scopes (and locks) that span several unrelated topics over hundreds of lines.

For me, this makes it hard to review, because:

The git --function-context will pull in all tx getdata logic.
The large scope can hide shadowing issues.
The large lock scope can imply there are atomicity requirements that don't exist.

So I am thinking if it could make sense to move this to its own small lock scope. I don't think placement matters: It could be in the end after MaybeSendFeefilter, or early (before MaybeSendPing). I don't mind moving it out of MaybeSendPing, I just wonder if there is a better placement with a smaller (lock) scope, so that future code changes in this area are easier to review.

mzumsande commented at 2:22 PM on October 2, 2026:

The reason I did that was to avoid locking cs_main multiple times in SendMessages, which seemed to be the preexisting philosophy of that function. I checked that the refactor #35522 also doesn't change this. I feel that if taking/releasing the lock multiple times is not something we want to avoid, we could also just leave the check where it is in MaybeSendPing and add a lock there (even if it is a bit unexpected given the function name). So no strong opinion, but I didn't change it with the last push.

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 :)

mzumsande force-pushed on Sep 4, 2026

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

abeb912 to c551ff4: addressed feedback

in test/functional/p2p_ping_ibd.py:188 in c551ff40a4 outdated

 183 | +            while node.mocktime <= ping_start + TIMEOUT_INTERVAL:
 184 | +                node.bumpmocktime(TIMEOUT_INTERVAL // 5)
 185 | +                peer.serve_next_block()
 186 | +                self.wait_until(lambda: node.getblockcount() == start_height + peer.blocks_served)
 187 | +                assert peer.is_connected
 188 | +        assert_greater_than(node.getpeerinfo()[0]["pingwait"], TIMEOUT_INTERVAL)

danielabrozzoni commented at 4:01 PM on September 11, 2026:

I think here you should also wait for POST_BLOCK_PONG_GRACE + 1 seconds, and check that you don't get disconnected:

diff --git a/test/functional/p2p_ping_ibd.py b/test/functional/p2p_ping_ibd.py
index 0774bf8dc8..7f1ec2d4b5 100755
--- a/test/functional/p2p_ping_ibd.py
+++ b/test/functional/p2p_ping_ibd.py
@@ -186,6 +186,9 @@ class PingIBDTest(BitcoinTestFramework):
                 self.wait_until(lambda: node.getblockcount() == start_height + peer.blocks_served)
                 assert peer.is_connected
         assert_greater_than(node.getpeerinfo()[0]["pingwait"], TIMEOUT_INTERVAL)
+        node.bumpmocktime(POST_BLOCK_PONG_GRACE + 1)
+        peer.sync_with_ping()
+        assert peer.is_connected
 
         self.log.info("Serve the remaining blocks, so that nothing is in flight anymore")
         while peer.blocks_served < NUM_DOWNLOAD_BLOCKS:

This way you're checking that we're not disconnecting once the timeout elapsed and the POST_BLOCK_PONG_GRACE period passed if there are still blocks in flight.

If you don't add this check, you can mutate the original code like this, and the test will still pass:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index c339b471c5..734a41796d 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -6441,7 +6441,7 @@ bool PeerManagerImpl::SendMessages(CNode& node)
         // If the peer failed to answer our ping in time, disconnect due to timeout.
         // Skip the check while it is serving us blocks and let the block download timeout above govern
         // instead. Once the last new block was received, give the peer a grace period to answer the ping.
-        if (state.vBlocksInFlight.empty() &&
+        if (/*state.vBlocksInFlight.empty() &&*/
             now > NodeSeconds{node.m_last_block_time.load()} + POST_BLOCK_PONG_GRACE &&
             m_connman.ShouldRunInactivityChecks(node, now) &&
             peer.m_ping_nonce_sent &&

mzumsande commented at 2:00 PM on September 16, 2026:

good suggestion - done!

danielabrozzoni commented at 4:08 PM on September 11, 2026: member

Thanks for dealing with my AI-assisted review :) I only left an additional comment, I think the test can be made slightly more robust, by checking that the timeout and post block pong grace are only enforced if there are no blocks in flight, which kills one mutant.

mzumsande force-pushed on Sep 16, 2026

AndreaDiazCorreia commented at 5:00 AM on September 21, 2026: none

tested ACK 31280333dd4f9ef2ec818b1a9d25daaeb502659e

Built each commit. Between MaybeSendPing and the new location there are only two returns, and both already set fDisconnect, so no peer class gains or loses the timeout from the move alone. p2p_ping_ibd.py, --v2transport and p2p_ping.py pass at the tip, and the test from 56c194e655 passes against the 95dcf8f10d binary while only failing against the tip, so the relocation is behaviour-preserving.

I also tested it between two regtest nodes, no mocktime, with a local proxy limiting the serving side to 8 KiB/s while the receiver downloaded 25 blocks of ~946 kB. Before the fix the receiver disconnected after ~22 minutes at height 211, with 14 blocks still in flight:

2026-09-21T04:38:36.772959Z [net] ping timeout: 1200.019086s, disconnecting peer=0

With the fix it stayed connected, answered a ping 30m47s late and finished the 25 blocks in ~48 minutes.

One question (non-blocking): with a block in flight, is the block download timeout now the only bound, and does its window reset on every front-of-queue arrival? That would let a peer that never pongs but keeps delivering stay connected for hours. Seems right to me but I just want to confirm it is intended.

Part of the testing was AI-assisted (build and test orchestration, log collection). I checked the results myself.

DrahtBot requested review from l0rinc on Sep 21, 2026

DrahtBot requested review from danielabrozzoni on Sep 21, 2026

in test/functional/p2p_ping_ibd.py:147 in 56c194e655 outdated

 142 | +        # Since we don't answer the ping within the ping timeout interval, the peer would disconnect
 143 | +        # us if they don't relax the ping timeout rules while requesting blocks.
 144 | +        # Note that the reason for the stall is not us, but the slowness of the peer itself, so
 145 | +        # they would disconnect a good and fast peer that is not at fault.
 146 | +        node.bumpmocktime(TIMEOUT_INTERVAL + 60)
 147 | +        assert_equal(self.bytes_sent(node, "pong"), pong_bytes)

fjahr commented at 3:38 PM on September 28, 2026:

I am not sure I am getting this incorrectly but the peer is the one that would need their time bumped because that's the receiving side. But also running the check immediately feels racy. It seems to pong is being stalled by the stalling of blocks, so this can't actually fail as it is done now. I think the whole check can be removed since the assert just duplicates the one above if the timebump has no effect.

maflcko commented at 10:45 AM on September 29, 2026:

I had the same thought: This sub-test is mostly about the order of messages and that stalling can occur, not about exact timing. Seems fine to remove, but no strong opinion.

mzumsande commented at 2:20 PM on October 2, 2026:

good point, I removed it. I just left the comment about what would happen at the peer, the other added test covers the other side anyway.

fjahr commented at 4:03 PM on September 28, 2026: contributor

Code review ACK 31280333dd4f9ef2ec818b1a9d25daaeb502659e

Modulo my comment in the functional test, but it's also pretty harmless afaict, just would be good to address if you retouch.

in test/functional/p2p_ping_ibd.py:154 in 31280333dd

 149 | +        assert_equal(self.bytes_sent(node, "pong"), pong_bytes)
 150 | +
 151 | +        self.log.info("Check that the pong only arrives after all requested blocks were sent")
 152 | +        peer.last_message.pop("pong")
 153 | +        self.set_reading(peer, True)
 154 | +        peer.wait_until(lambda: "pong" in peer.last_message, timeout=120)

maflcko commented at 10:46 AM on September 29, 2026:

nit in the first commit: The timeout can probably be dropped? Sending 16MB should be faster than that even with sanitizers (and sanitizer come with an increased --timout-factor anyway).

mzumsande commented at 2:20 PM on October 2, 2026:

done

in test/functional/p2p_ping_ibd.py:190 in 31280333dd

 204 | -
 205 | -        self.log.info("Check that the peer is disconnected even though it is serving blocks")
 206 | -        # The peer keeps serving the blocks we ask for as fast as we request them, but we
 207 | -        # still disconnect it because it didn't get around to answering our ping. A later
 208 | -        # commit changes this behavior.
 209 | -        assert_greater_than(len(node.getpeerinfo()[0]["inflight"]), 0)

maflcko commented at 1:09 PM on September 29, 2026:

nit in the last commit 31280333dd4f9ef2ec818b1a9d25daaeb502659e: Not sure why this line is removed? It seems it should come right after the

        self.log.info("Check that the grace period doesn't apply while blocks are still in flight")
...> assert that blocks are still in flight here <...

mzumsande commented at 2:22 PM on October 2, 2026:

readded the line, it wasn't meant to be removed.

maflcko approved

maflcko commented at 1:14 PM on September 29, 2026: member

review ACK 31280333dd4f9ef2ec818b1a9d25daaeb502659e 🛐

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 31280333dd4f9ef2ec818b1a9d25daaeb502659e 🛐
5aW7uyMCJTscjwRcjliby8TfMCn7FltFsZy0R1a53FnKeNXBPpQfm6tERiTlLwGrxuTBCaF1JLVShaTlr4GbCQ==

</details>

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.

8b5bbfe1fc

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.

2b43c62d8e

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.

d83647e7c4

mzumsande force-pushed on Oct 2, 2026

mzumsande commented at 2:26 PM on October 2, 2026: contributor

3128033 to d83647e: addressed feedback

One question (non-blocking): with a block in flight, is the block download timeout now the only bound, and does its window reset on every front-of-queue arrival? That would let a peer that never pongs but keeps delivering stay connected for hours. Seems right to me but I just want to confirm it is intended.

yes, I think that is correct. There is still the socket inactivity check, but this will not fire if the peer keeps on sending blocks. But I don't think that this is a problem, blocks are more helpful for us than ping messages, which are not really important by themselves but a way to check responsibility in times when we don't exchanges more important messages.

maflcko commented at 2:30 PM on October 2, 2026: member

Only change is test nits

review ACK d83647e7c48a11dc54ad697342a4b7e3bce9061b 🌎

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK d83647e7c48a11dc54ad697342a4b7e3bce9061b 🌎
u8lrcdX5UP+MpTjsW0cLYHfcBM6Zj86Yu6pQBzXC7Zo2Cr+ebB4mswhEZtgofjNmW8AdY+KUtTXegtqfU3oMBw==

</details>

DrahtBot requested review from AndreaDiazCorreia on Oct 2, 2026

DrahtBot requested review from fjahr on Oct 2, 2026

fjahr commented at 2:41 PM on October 2, 2026: contributor

reACK d83647e7c48a11dc54ad697342a4b7e3bce9061b

danielabrozzoni approved

danielabrozzoni commented at 7:33 AM on October 4, 2026: member

ACK d83647e7c48a11dc54ad697342a4b7e3bce9061b

sedited approved

sedited commented at 8:32 AM on October 5, 2026: contributor

ACK d83647e7c48a11dc54ad697342a4b7e3bce9061b

sedited merged this on Oct 5, 2026

sedited closed this on Oct 5, 2026