#36372 p2p: replace full outbound peers that don't support tx relay (take 2)
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36372 p2p: replace full outbound peers that don't support tx relay (take 2) | Waiting on author | 1 | Blocked | 62 + 134 tests | |||
|
|
|
| ||||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Replaces outbound full-relay peers that do not relay transactions with peers that do, operating only outside of initial block download and when not in -blocksonly mode. To prevent connection drops below outbound targets, the node initiates an extra full-relay outbound connection before evicting a non-relaying peer. Peers that do not relay transactions are excluded from chain-sync eviction protection, though the sole connection to any specific network is preserved.
Problem: Outbound full-relay slots can be occupied by peers that decline transaction relay (such as nodes running in -blocksonly mode). This degrades transaction broadcast and propagation topology without the node operator's knowledge, and nodes with feefilter mismatches or blocksonly peers can be left with too few transaction-relaying outbound connections.
Evidence of importance:
- Resurrects and reworks #28538 to address previous review feedback, motivated by longstanding open issues #16418 ('Ensure we have sufficient transaction-relay peers') and #28371 ('Seek more/different peers when ours all have too high feefilter').
Scope: Includes a cleanup commit renaming CConnman::ForEachNode to ForEachFullyConnectedNode and removing redundant disconnected-peer checks at call sites.
Risk of a mistake: medium (p2p). Bugs in peer eviction and connection rotation could cause connection instability, network churn, or unintended peer disconnects.
Changed since the previous assessment: Initial patch evaluation; the diff directly implements the strategy previously described without approach changes.
Reviewability
Paused · Waiting on author
Open objections from Bicaru20, fametrano, instagibbs with no author reply for 19 days; review would repeat what has been asked.
Waiting on (from the thread read):
- author: response to fametrano's test objection, instagibbs' questions, and Bicaru20's review comments 2026-10-02
Agreement
Blocked
objection that needs a change, nobody has replied (fametrano)
Thread notes: Added Bicaru20 with Concept ACK, a suggestion for unit test coverage in denialofservice_tests, and a question regarding eviction priority between inactive tx-relaying peers and active non-tx-relaying peers. Bicaru20's inline comment on logging format was treated as a nit.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| instagibbs (member) | question, approach | whether replacement peers should be validated to ensure they actually do useful work before evicting existing peers | open | answered | no | 2026-09-30: 'Would we want to do more validation of those claims before evicting someone?' |
| instagibbs (member) | question, usefulness | whether there is evidence on the network that non-tx-relaying peers are a real problem | open | answered | no | 2026-09-30: 'do we have evidence this is a real problem? ... Can we crawl the network to see evidence of this?' |
| fametrano (contributor) | objection, correctness | the functional test checks for non-eviction cannot fail because a node never evicts the only peer on a network | open | changed | no | 2026-10-02: 'The "not evicted" checks in the first and third cases of the new `p2p_blocksonly.py` test can't fail.' |
| Bicaru20 (contributor) | question, approach | whether active peers that do not relay transactions should be prioritized over peers that relay transactions but have been silent for some time | open | answered | no | 2026-10-13: 'Shouldn't we chose the peer that is active over the one that has been silent for some time even if the active peer is not relaying transactions?' |
| Bicaru20 (contributor) | suggestion, maintenance | add a test case to denialofservice_tests checking that a replacement is requested even when the candidate peer cannot be evicted yet because it is younger than MINIMUM_CONNECT_TIME | open | answered | no | 2026-10-13: 'I think it would be good to add a new test case here to check that a replecemnt is requested even when the candidate with `m_relays_txs=false` cannot be evicted yet because it is younger than MINIMUM_CONNECT_TIME' |
Support:
- mzumsande (member) (Concept ACK, 2026-09-28): thanks for picking this up! [not substantive]
- Bicaru20 (contributor) (Concept ACK, 2026-10-13) [read]: summarized PR behavior and left suggestion to make tests more robust [not substantive]
Participants:
- mzumsande (member, support, 1 statements 2026-09-28 to 2026-09-28): Concept ACK for picking up the PR
- instagibbs (member, question, 1 statements 2026-09-30 to 2026-09-30): asked whether replacement peer claims should be validated before evicting existing peers, and whether network data shows this is a real problem
- fametrano (contributor, objection, 1 statements 2026-10-02 to 2026-10-02): pointed out that non-eviction test cases in p2p_blocksonly.py cannot fail and provided a diff fixing them
- Bicaru20 (contributor, support, 3 statements 2026-10-13 to 2026-10-13): Concept ACK; suggested an additional test case and questioned eviction ordering for silent peers
Files
138 lines under test/bench/ci.
- src/test/denialofservice_tests.cpp +52/-0
- test/functional/p2p_outbound_eviction.py +48/-0
- src/net_processing.cpp +31/-11
- test/functional/p2p_blocksonly.py +29/-0
- src/net.cpp +21/-0
- src/net.h +10/-2
- test/functional/test_framework/p2p.py +4/-3
- src/test/fuzz/connman.cpp +1/-1
Card
The short text the ranking pass compares across PRs.
Replaces outbound full-relay peers that do not relay transactions with peers that do, ensuring nodes maintain expected transaction relay capacity and diversity. To avoid dropping below outbound connection targets, an extra outbound connection is established before evicting the non-relaying peer, and non-relaying peers are denied disconnect protection unless they are the sole connection on a reachable network. The replacement mechanism is disabled during initial block download and when running with -blocksonly, addressing issues #16418 and #28371 by resolving design feedback on #28538.
Data
thread read through 2026-10-13 (6 statements), head 07e58de168
labels: P2P
Categories
P2P
P2 · new feature
Improves transaction relay topology by ensuring outbound full-relay slots are not occupied by peers that decline transaction relay. Addresses longstanding network health issues (#16418, #28371) where nodes could be left with too few transaction-relaying outbound connections.
Membership: Carries the P2P label and modifies peer eviction logic and connection management in src/net.cpp and src/net_processing.cpp.
Factors: security/stability 1, bug 1, performance 0, user value 2, leverage 1