p2p: Evict outbound connections to -blocksonly peers when at full outbound maximum

mzumsande · mzumsande · commit 1393bf67b26c · 2023-12-14T14:09:50.000-05:00
...unless we are in -blocksonly mode ourselve or in ibd.
If we had too many outbound peers not participating in tx relay, we would run
into problems with getting own transaction relayed. This would also be
bad for privacy and fee estimation.

We evict non-txrelay peers only when all full outbound slots
are filled to have peers in hypothetical situations where
-blocksonly peers are the only ones available.

Idea for this approach by ajtowns.
diff --git a/src/net.cpp b/src/net.cpp
@@ -2347,12 +2347,13 @@ void CConnman::StartExtraBlockRelayPeers()
 }
 
 // Return the number of peers we have over our outbound connection limit
+// Returns 0 if we are at the limit, and a negative number if below.
 // Exclude peers that are marked for disconnect, or are going to be
 // disconnected soon (eg ADDR_FETCH and FEELER)
 // Also exclude peers that haven't finished initial connection handshake yet
 // (so that we don't decide we're over our desired connection limit, and then
 // evict some peer that has finished the handshake)
-int CConnman::GetExtraFullOutboundCount() const
+int CConnman::GetFullOutboundDelta() const
 {
     int full_outbound_peers = 0;
     {
@@ -2363,7 +2364,7 @@ int CConnman::GetExtraFullOutboundCount() const
             }
         }
     }
-    return std::max(full_outbound_peers - m_max_outbound_full_relay, 0);
+    return full_outbound_peers - m_max_outbound_full_relay;
 }
 
 int CConnman::GetExtraBlockRelayCount() const
diff --git a/src/net.h b/src/net.h
@@ -1173,7 +1173,8 @@ class CConnman
     // return a value less than (num_outbound_connections - num_outbound_slots)
     // in cases where some outbound connections are not yet fully connected, or
     // not yet fully disconnected.
-    int GetExtraFullOutboundCount() const;
+    // Returns 0 if we are at the target, and a negative number if below.
+    int GetFullOutboundDelta() const;
     // Count the number of block-relay-only peers we have over our limit.
     int GetExtraBlockRelayCount() const;
 
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
@@ -5183,7 +5183,8 @@ void PeerManagerImpl::EvictExtraOutboundPeers(std::chrono::seconds now)
     }
 
     // Check whether we have too many outbound-full-relay peers
-    if (m_connman.GetExtraFullOutboundCount() > 0) {
+    int full_outbound_delta{m_connman.GetFullOutboundDelta()};
+    if (full_outbound_delta > 0) {
         // If we have more outbound-full-relay peers than we target, disconnect one.
         // Pick the outbound-full-relay peer that least recently announced
         // us a new block, with ties broken by choosing the more recent
@@ -5241,6 +5242,27 @@ void PeerManagerImpl::EvictExtraOutboundPeers(std::chrono::seconds now)
             }
         }
     }
+    // If we are at the max full outbound limit but not above, check if any peers
+    // don't support tx relay (they might be in -blocksonly mode) - if so,
+    // evict the newest of them (highest Node Id) so we can fill the slot with a tx-relaying outbound peer
+    // This is not done if we are in -blocksonly mode ourselves or still in IBD,
+    // because then we don't care if our peer supports tx relay.
+    else if (full_outbound_delta == 0 && !m_opts.ignore_incoming_txs && !m_chainman.IsInitialBlockDownload()) {
+        std::optional<NodeId> node_to_evict;
+        m_connman.ForEachNode([&node_to_evict](CNode* node) {
+            if (!node->IsFullOutboundConn() || node->m_relays_txs) return;
+            if (!node_to_evict.has_value() || *node_to_evict < node->GetId()) {
+                node_to_evict = node->GetId();
+            }
+        });
+        if (node_to_evict.has_value()) {
+            LogPrint(BCLog::NET, "disconnecting full outbound peer not participating in tx relay: peer=%d\n", *node_to_evict);
+            m_connman.ForNode(*node_to_evict, [](CNode* node) {
+                node->fDisconnect = true;
+                return true;
+            });
+        }
+    }
 }
 
 void PeerManagerImpl::CheckForStaleTipAndEvictPeers()
diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp
@@ -123,7 +123,7 @@ FUZZ_TARGET(connman, .init = initialize_connman)
             });
     }
     (void)connman.GetAddedNodeInfo(fuzzed_data_provider.ConsumeBool());
-    (void)connman.GetExtraFullOutboundCount();
+    (void)connman.GetFullOutboundDelta();
     (void)connman.GetLocalServices();
     (void)connman.GetMaxOutboundTarget();
     (void)connman.GetMaxOutboundTimeframe();

Original file line number	Diff line number	Diff line change
`@@ -2347,12 +2347,13 @@ void CConnman::StartExtraBlockRelayPeers()`
`2347`	`2347`	`}`
`2348`	`2348`
`2349`	`2349`	`// Return the number of peers we have over our outbound connection limit`
	`2350`	`+// Returns 0 if we are at the limit, and a negative number if below.`
`2350`	`2351`	`// Exclude peers that are marked for disconnect, or are going to be`
`2351`	`2352`	`// disconnected soon (eg ADDR_FETCH and FEELER)`
`2352`	`2353`	`// Also exclude peers that haven't finished initial connection handshake yet`
`2353`	`2354`	`// (so that we don't decide we're over our desired connection limit, and then`
`2354`	`2355`	`// evict some peer that has finished the handshake)`
`2355`		`-int CConnman::GetExtraFullOutboundCount() const`
	`2356`	`+int CConnman::GetFullOutboundDelta() const`
`2356`	`2357`	`{`
`2357`	`2358`	`int full_outbound_peers = 0;`
`2358`	`2359`	`{`
`@@ -2363,7 +2364,7 @@ int CConnman::GetExtraFullOutboundCount() const`
`2363`	`2364`	`}`
`2364`	`2365`	`}`
`2365`	`2366`	`}`
`2366`		`- return std::max(full_outbound_peers - m_max_outbound_full_relay, 0);`
	`2367`	`+ return full_outbound_peers - m_max_outbound_full_relay;`
`2367`	`2368`	`}`
`2368`	`2369`
`2369`	`2370`	`int CConnman::GetExtraBlockRelayCount() const`
Original file line number	Diff line number	Diff line change
`@@ -123,7 +123,7 @@ FUZZ_TARGET(connman, .init = initialize_connman)`
`123`	`123`	`});`
`124`	`124`	`}`
`125`	`125`	`(void)connman.GetAddedNodeInfo(fuzzed_data_provider.ConsumeBool());`
`126`		`- (void)connman.GetExtraFullOutboundCount();`
	`126`	`+ (void)connman.GetFullOutboundDelta();`
`127`	`127`	`(void)connman.GetLocalServices();`
`128`	`128`	`(void)connman.GetMaxOutboundTarget();`
`129`	`129`	`(void)connman.GetMaxOutboundTimeframe();`