p2p: protect manual evictions

willcl-ark commented at 1:28 pm on March 5, 2026: member

Closes: #5097

Manual peers (-addnode, -connect + RPC) currently get disconnected by IBD timeout logic for block stalling, block download timeout and headers sync timeout. This can conflict with explicit user intent to keep these peers connected (especially in privacy-focused -connect setups), and causes unnecessary disconnect/reconnect churn.

This PR changes that behavior: manual peers are not disconnected by those timeout paths and we try to re-assign their work to other peers to maintain progress.

Disconnecting manual peers automatically causes reconnect loops and other behavior that can violate user expectation for manually configured peers.

Simply “not disconnecting” (for block stalling) is not sufficient by itself: a manual peer can retain in-flight block assignments and delay overall IBD progress.

I considered taking an approach more like that from #32051, simply protecting peers from some disconnects. However without the reassignment the stalling effects are simply prolonged (which is worse, IMO).

This PR uses a different appraoch:

For manual peers in block-stalling/download-timeout paths:
- release all in-flight blocks from that peer,
- set a one-round recovery flag (m_stall_recovery) so scheduler ordering doesn’t immediately reassign those same blocks back to the same peer.

This attempts to both preserve manual connections and maintain download progress. It actively reassigns stalled work so IBD can continue, rather than just waiting longer. And, the one-round skip prevents immediate re-capture of freed blocks by the same manual peer due to per-peer send ordering.

Compared with simpler “don’t disconnect” approaches, this is more robust both in tests, and real-world slow-peer conditions.

I believe the changes do not increase remote attacker control because it only applies to manual peers (-addnode, -connect, addnode), which are explicitly chosen by the local operator. You were and still are free to add a malicious peer manually.

A malicious manually-added peer can now remain connected longer, but that peer was already granted privileged trust by operator configuration. And in fact, for block stalling/download timeout this PR reduces potential impact of a slow/malicious manual peer by releasing its in-flight blocks so other peers can fetch them.

Of course, if an operator relies only on bad manual peers, sync can still be delayed.

net: don't disconnect manual peers for block stalling

Manual connections (-addnode, -connect) represent explicit user intent
to maintain the connection, and are important for privacy in
-connect-only setups. Yet they get disconnected during IBD when the
block download window stalls.

Instead of disconnecting, release all in-flight blocks from the manual
peer so other peers can request them. Set m_stall_recovery to skip one
round of block download for this peer. Without this, the message
handler's random peer ordering can cause the manual peer's next
SendMessages call to re-request the same blocks via
FindNextBlocksToDownload before any other peer has a chance to claim
them, permanently stalling IBD when mocktime is frozen (and adding
unnecessary delay with real time).

This flakiness was observed first in functional tests with few peers
(intermittent failures), but would also occasionalyl happen with more
peers in a non-test scenario.

3d64f4b09d

willcl-ark renamed this:
~~Protect manual evictions~~
p2p: protect manual evictions
on Mar 5, 2026

DrahtBot commented at 1:29 pm on March 5, 2026: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	w0xlt, sedited, kevkevinpal

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34588 (refactor: decompose Peer struct into focused sub-components by w0xlt)
#34565 (refactor: extract BlockDownloadManager from PeerManagerImpl by w0xlt)
#34389 (net/log: standardize peer+addr log formatting via LogPeer by l0rinc)
#33854 (fix assumevalid is ignored during reindex by Eunovo)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

full nodes/support SegWit -> full nodes or support SegWit [the slash makes the phrase ambiguous; “or” clarifies that manual peers need not be full nodes nor support SegWit]

2026-03-06 22:38:15

DrahtBot added the label P2P on Mar 5, 2026

w0xlt commented at 3:30 pm on March 5, 2026: contributor

Concept ACK

in test/functional/test_framework/test_node.py:921 in 547f119668

916+
917+        def addnode_callback(address, port):
918+            if v2:
919+                self.addnode('%s:%d' % (address, port), "onetry", v2transport=True)
920+            else:
921+                self.addnode('%s:%d' % (address, port), "onetry")

kevkevinpal commented at 7:59 pm on March 5, 2026:

Might be able to do this instead. It seems redundant to have the two calls.

0self.addnode('%s:%d' % (address, port), "onetry", v2transport=v2)

jonatack commented at 2:51 am on March 6, 2026: member

Thanks for picking this up, Will.

sedited commented at 7:35 am on March 6, 2026: contributor

Concept ACK

kevkevinpal commented at 3:45 pm on March 6, 2026: contributor

Concept ACK

test: add add_manual_p2p_connection helper to TestNode

Add a helper for creating manual (addnode onetry) p2p connections,
mirroring the existing add_outbound_p2p_connection method. This will
be used by tests that verify manual peer behavior during IBD.

be002a701f

test: verify manual peers survive block stalling timeout

test that a manual (addnode) peer which stalls on a block, is not
disconnected when the stalling timeout fires.

6c2c3a451d

net: don't disconnect manual peers for block download timeout

Manual connections (-addnode, -connect) represent explicit user intent
to maintain the connection, and are important for privacy in
-connect-only setups. Yet they get disconnected during IBD when a
block download times out.

Apply the same release-and-skip strategy as the stalling path: release
all in-flight blocks and set m_stall_recovery so other peers get a
chance to pick them up.

a0d1bcdb69

test: verify manual peers survive block download timeout

Test the download timeout path in isolation by using only a manual
peer initially (no outbound peers). Without outbound peers, no
staller is identified, so only the download timeout fires (after
600s of mocktime). Verify non-disconnection, then add outbound
peers and advance time past the stalling timeout to verify IBD
completes.

baaf718bb1

net: don't disconnect manual peers for headers sync timeout

During IBD, a slow headers sync peer gets disconnected if other
preferred peers are available. The existing code already protects
NoBan peers by resetting their sync state instead of disconnecting.

Extend this protection to manual peers, which represent explicit
user intent to maintain the connection. Like NoBan peers, reset
their sync state so headers can be fetched from another peer
without dropping the manual connection.

896d44f601

test: verify manual peers survive headers sync timeout

test that a manually added peer which is used as initial sync peer is
not disconnected when the headers download timeout fires.

1c12e57ac2

doc: update addnode rpc help and add release note 7beac82aa1

willcl-ark force-pushed on Mar 6, 2026

p2p: protect manual evictions #34743

Reviews

Conflicts

LLM Linter (✨ experimental)