p2p: replace full outbound peers that don't support tx relay (take 2) #36372

fjahr commented at 5:19 PM on September 28, 2026: contributor

Picks up #28538

Outbound-full-relay slots should be used by peers that actually relay transactions, besides #28538 see #16418 and #28371 for the motivation. So try to replace peers that are not relaying txs with peers that do.

The approach differs from the original PR in a few details in response to some of the review comments there:

A replacement is connected before the peer is evicted. The original PR disconnected a non-tx-relaying peer once all full outbound slots were filled. Reviewers preferred having a replacement available first (https://github.com/bitcoin/bitcoin/pull/28538#issuecomment-1883481846). Now an extra outbound-full-relay connection is requested, like the extra network-specific peers, and the existing eviction of extra outbound peers prefers peers that don't relay txs.
Peers not relaying txs don't get m_protect. Otherwise they could stay forever (https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1443336121), and ignoring the protection was rejected in the same thread (https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1443312604).
An only peer on a network is not replaced. Otherwise the network-specific extra connections and this logic would keep replacing each other (https://github.com/bitcoin/bitcoin/pull/28538#issuecomment-1883481846). This also covers the -maxconnections=1 case (https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1417178196).

Nothing is changed in -blocksonly mode or during IBD. The rename commit and the functional test are taken from the original PR with some changes to match the new behavior.

refactor: rename ForEachNode and remove duplicate checks

Rename ForEachNode to ForEachFullyConnectedNode because it
has an inbuilt check that only executes the passed function
for fully connected nodes (i.e. if fSuccessfullyConnected is set
and fDisconnect isn't).

This means callers don't need to need to worry about that, so remove
those duplicate checks and related comments.

7ed4cbe6c6

p2p: Don't protect outbound peers not participating in tx relay

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

9798b32508

p2p: Replace outbound peers not participating in tx relay

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

8ac7e7897e

DrahtBot added the label P2P on Sep 28, 2026

DrahtBot commented at 5:19 PM on September 28, 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/36372
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36372.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36372#author-ralph

Reviews

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

Type	Reviewers
ACK	Bicaru20
Concept ACK	mzumsande

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

DrahtBot added the label CI failed on Sep 28, 2026

test: add test for interaction with -blocksonly peers

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

07e58de168

fjahr force-pushed on Sep 28, 2026

DrahtBot removed the label CI failed on Sep 28, 2026

mzumsande commented at 9:16 PM on September 28, 2026: contributor

Concept ACK, thanks for picking this up!

instagibbs commented at 7:46 PM on September 30, 2026: member

Just a conceptual question IIUC: we could be evicting a peer that was giving us useful work for a peer that claims to do useful work. Would we want to do more validation of those claims before evicting someone?

edit: do we have evidence this is a real problem? Obviously it would be problematic if nodes quickly started converting to blocksonly, but I don't see that happening fast. Can we crawl the network to see evidence of this?

fametrano commented at 11:49 AM on October 2, 2026: contributor

The "not evicted" checks in the first and third cases of the new p2p_blocksonly.py test can't fail. Each case connects one peer, and a node never evicts or replaces its only full-outbound peer on a network. So replacing peers in -blocksonly mode (dropping !m_opts.ignore_incoming_txs from replace_no_tx_relay) passes every test in the PR. Evicting below the outbound limit (GetExtraFullOutboundCount() >= 0) also passes this test, though the existing unit tests catch it. The second case works: disconnecting the peer instead of asking for a replacement fails it.

Two peers in each case fix this. In the first case, mocktime also has to pass MINIMUM_CONNECT_TIME (30 s), because a newer peer is never evicted. In the blocksonly case, use -maxconnections=2 and check that "setting replace no-tx-relay peer=true" is not logged. With that, both changes fail the test, and it passed 6 of 6 runs on 07e58de. Diff below.

I only looked at the new tests; I have not reviewed the C++ change.

--- a/test/functional/p2p_blocksonly.py
+++ b/test/functional/p2p_blocksonly.py
@@ -83,30 +83,41 @@
         self.log.info("Tests with outbound peers that set txrelay to false, as peers in blocksonly mode do")
         self.restart_node(0, ["-noblocksonly"])
         self.log.info("Check that we sustain full-relay outbound connections to blocksonly peers when not at the full outbound limit")
-        blocksonly_peer = self.nodes[0].add_outbound_p2p_connection(P2PInterface(), p2p_idx=1, txrelay=False)
-        # Trigger CheckForStaleTipAndEvictPeers, which is scheduled every 45s (EXTRA_PEER_CHECK_INTERVAL)
-        self.nodes[0].mockscheduler(46)
-        blocksonly_peer.sync_with_ping()
-        blocksonly_peer.peer_disconnect()
+        peers = self.add_blocksonly_peers()
+        self.trigger_eviction_check()
+        self.check_connected(peers)
 
         self.log.info("Check that we request a replacement instead of evicting full-relay outbound connections to blocksonly peers at the full outbound limit")
         # Simulate the situation of being at the max limit of full-outbound connections by setting -maxconnections to a low value
         self.restart_node(0, ["-noblocksonly", "-maxconnections=2"])
-        blocksonly_peer1 = self.nodes[0].add_outbound_p2p_connection(P2PInterface(), p2p_idx=1, txrelay=False)
-        blocksonly_peer2 = self.nodes[0].add_outbound_p2p_connection(P2PInterface(), p2p_idx=2, txrelay=False)
+        peers = self.add_blocksonly_peers()
         with self.nodes[0].assert_debug_log(["setting replace no-tx-relay peer=true"]):
-            self.nodes[0].mockscheduler(46)
-        blocksonly_peer1.sync_with_ping()
-        blocksonly_peer2.sync_with_ping()
-        self.nodes[0].disconnect_p2ps()
+            self.trigger_eviction_check()
+        self.check_connected(peers)
 
         self.log.info("Check that we don't evict full-relay outbound connections to blocksonly peers if we are blocksonly ourselves")
-        self.restart_node(0, ["-blocksonly", "-maxconnections=1"])
-        blocksonly_peer = self.nodes[0].add_outbound_p2p_connection(P2PInterface(), p2p_idx=1, txrelay=False)
+        self.restart_node(0, ["-blocksonly", "-maxconnections=2"])
+        peers = self.add_blocksonly_peers()
+        with self.nodes[0].assert_debug_log([], unexpected_msgs=["setting replace no-tx-relay peer=true"]):
+            self.trigger_eviction_check()
+        self.check_connected(peers)
+
+    def add_blocksonly_peers(self):
+        # Two peers, so that neither is kept only for being the sole peer on its network
+        return [self.nodes[0].add_outbound_p2p_connection(P2PInterface(), p2p_idx=i, txrelay=False) for i in (1, 2)]
+
+    def trigger_eviction_check(self):
+        # Pass MINIMUM_CONNECT_TIME (30s), so that a chosen peer is not kept for being new
+        self.nodes[0].setmocktime(int(time.time()) + 31)
+        # Trigger CheckForStaleTipAndEvictPeers, which is scheduled every 45s (EXTRA_PEER_CHECK_INTERVAL)
         self.nodes[0].mockscheduler(46)
-        blocksonly_peer.sync_with_ping()
-        blocksonly_peer.peer_disconnect()
 
+    def check_connected(self, peers):
+        for peer in peers:
+            peer.sync_with_ping()
+        self.nodes[0].disconnect_p2ps()
+        self.nodes[0].setmocktime(0)
+
     def blocks_relay_conn_tests(self):
         self.log.info('Tests with node in normal mode with block-relay-only connections')
         self.restart_node(0, ["-noblocksonly"])  # disables blocks only mode

</details>

in src/net.cpp:2493 in 07e58de168

2488 | +}
2489 | +
2490 | +void CConnman::SetReplaceNoTxRelayPeer(bool flag)
2491 | +{
2492 | +    if (m_replace_no_tx_relay_peer.exchange(flag) != flag) {
2493 | +        LogDebug(BCLog::NET, "setting replace no-tx-relay peer=%s", flag ? "true" : "false");

Bicaru20 commented at 10:32 AM on October 13, 2026:

nit:

        LogDebug(BCLog::NET, "setting replace no-tx-relay peer=%s\n", flag ? "true" : "false");

in src/test/denialofservice_tests.cpp:348 in 07e58de168

 343 | +    peerLogic->CheckForStaleTipAndEvictPeers();
 344 | +    for (size_t i = 0; i < vNodes.size(); ++i) {
 345 | +        BOOST_CHECK_EQUAL(vNodes[i]->fDisconnect, i == 1);
 346 | +    }
 347 | +    BOOST_CHECK(!connman->GetReplaceNoTxRelayPeer());
 348 | +

Bicaru20 commented at 11:10 AM on October 13, 2026:

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

    vNodes[1]->fDisconnect = false;
    AddRandomOutboundPeer(id, vNodes, *peerLogic, *connman, ConnectionType::OUTBOUND_FULL_RELAY);
    vNodes.back()->m_relays_txs = false;
    AddRandomOutboundPeer(id, vNodes, *peerLogic, *connman, ConnectionType::OUTBOUND_FULL_RELAY);
    vNodes.back()->m_relays_txs = true;
    clock += 1s;
    peerLogic->CheckForStaleTipAndEvictPeers();
    BOOST_CHECK(connman->GetReplaceNoTxRelayPeer());
    for (const CNode* node : vNodes) {
        BOOST_CHECK(!node->fDisconnect);
    }

Bicaru20 commented at 12:14 PM on October 13, 2026: contributor

Conecpt ACK 07e58de16805e66fc2bccc842c1c2edc4ee1f897 I left a suggestion to try to make the tests more robusts

With the new changes, if we are not in -blocksonly mode or in IBD, our node will be able to evict the nodes that do not relay transaction first. Also, these nodes will not be protected against eviction with the m_protect flag. Before evicting one of the nodes, it ensures a new peer has been found.

One question I have is what happens if we have a node that doesn't relay transactions and another one that does but has been silent for some time. In this case, if I have understood it correclty, it would evict first the peer that doesn't relay the tansactions. 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?

DrahtBot requested review from mzumsande on Oct 13, 2026