net: prefer NODE_BLAKE2B for every outbound slot, and stop churning

src/net.cpp

@@ -2552,69 +2552,74 @@ int CConnman::GetExtraBlockRelayCount() const
     return std::max(block_relay_peers - m_max_outbound_block_relay, 0);
 }
 
-bool CConnman::DemoteToStaleOutbound(CNode& node, unsigned int max_stale)
+CConnman::StaleOutboundRoom CConnman::CheckStaleOutboundRoom(ConnectionType conn_type, unsigned int max_stale, const CNode* exclude) const
 {
-    // The version handler rejects a redundant VERSION before the stale gate, so
-    // a peer is never demoted twice; assert that rather than guarding for it.
-    Assert(!node.m_is_stale_outbound);
-    // m_nodes_mutex guards grantOutbound and m_network_conn_counts, and lets us
-    // count peers without racing the socket handler. The stale count is derived
-    // from the flag here rather than kept in a separate counter, so it can never
-    // drift out of sync with the connections it describes. num_stale is unsigned
-    // to match max_stale (-maxstaleoutbound); inbound_equiv is int to match
-    // m_max_inbound, so neither comparison trips -Wsign-compare.
-    LOCK(m_nodes_mutex);
-    if (node.fDisconnect) return false;
-    const ConnectionType conn_type{node.m_conn_type};
-    unsigned int num_stale{0};
+    AssertLockHeld(m_nodes_mutex);
+    // The stale count is derived from the flag rather than kept in a separate
+    // counter, so it can never drift out of sync with the connections it
+    // describes. num_stale is unsigned to match max_stale (-maxstaleoutbound);
+    // inbound_equiv is int to match m_max_inbound, so neither comparison trips
+    // -Wsign-compare.
+    StaleOutboundRoom room;
     int inbound_equiv{0};
     int same_target{0};
     for (const CNode* pnode : m_nodes) {
-        if (pnode->fDisconnect) continue;
+        if (pnode == exclude || pnode->fDisconnect) continue;
         // A demoted stale peer draws on the inbound budget, like a real inbound.
         if (pnode->m_is_stale_outbound) {
-            ++num_stale;
+            ++room.num_stale;
             ++inbound_equiv;
         } else if (pnode->IsInboundConn()) {
             ++inbound_equiv;
         }
-        // Peers filling this outbound target, preferred or stale alike. A demoted
-        // peer keeps its connection type, so this counts both, and node itself
-        // is still in m_nodes here, so it counts towards its own target too.
+        // Peers filling this outbound target, preferred or stale alike: a demoted
+        // peer keeps its connection type.
         if (pnode->m_conn_type == conn_type) ++same_target;
     }
-    if (num_stale >= max_stale) {
-        LogDebug(BCLog::NET, "peer lacks NODE_BLAKE2B and already have %u stale outbound peers (limit %u), %s\n",
-                 num_stale, max_stale, node.DisconnectMsg(fLogIPs));
-        node.fDisconnect = true;
-        return false;
+    Assume(conn_type == ConnectionType::OUTBOUND_FULL_RELAY || conn_type == ConnectionType::BLOCK_RELAY);
+    const int max_same_target{conn_type == ConnectionType::OUTBOUND_FULL_RELAY ? m_max_outbound_full_relay : m_max_outbound_block_relay};
+    if (room.num_stale >= max_stale) {
+        room.refusal = strprintf("already have %u stale outbound peers (limit %u)", room.num_stale, max_stale);
+    } else if (same_target >= max_same_target) {
+        // Tolerating a stale peer is only worthwhile while it fills a gap in the
+        // outbound target. Once that target is met, by preferred peers, already
+        // tolerated stale ones, or a mix, another stale peer buys us nothing.
+        room.refusal = strprintf("the outbound target is already full (%d/%d)", same_target, max_same_target);
+    } else if (inbound_equiv >= m_max_inbound) {
+        // Demoting releases the outbound slot, which we then refill toward the
+        // outbound target, so the peer must fit the inbound budget or a later
+        // outbound connection would push us past -maxconnections.
+        room.refusal = "no room within -maxconnections";
     }
-    // Tolerating a stale peer is only worthwhile while it fills a gap in the
-    // outbound target. Once that target is met, by preferred peers, already
-    // tolerated stale ones, or a mix, another stale peer buys us nothing.
-    // same_target includes node, so compare with > and report the rest.
-    const int max_same_target{node.IsFullOutboundConn() ? m_max_outbound_full_relay : m_max_outbound_block_relay};
-    if (same_target > max_same_target) {
-        LogDebug(BCLog::NET, "peer lacks NODE_BLAKE2B and the outbound target is already full (%d/%d), %s\n",
-                 same_target - 1, max_same_target, node.DisconnectMsg(fLogIPs));
-        node.fDisconnect = true;
-        return false;
-    }
-    // Demoting releases the outbound slot, which we then refill toward the
-    // outbound target, so the peer must fit the inbound budget or a later
-    // outbound connection would push us past -maxconnections.
-    if (inbound_equiv >= m_max_inbound) {
-        LogDebug(BCLog::NET, "peer lacks NODE_BLAKE2B and no room within -maxconnections, %s\n",
-                 node.DisconnectMsg(fLogIPs));
+    return room;
+}
+
+bool CConnman::CanTolerateStaleOutbound(ConnectionType conn_type, unsigned int max_stale) const
+{
+    LOCK(m_nodes_mutex);
+    return !CheckStaleOutboundRoom(conn_type, max_stale, /*exclude=*/nullptr).refusal.has_value();
+}
+
+bool CConnman::DemoteToStaleOutbound(CNode& node, unsigned int max_stale)
+{
+    // The version handler rejects a redundant VERSION before the stale gate, so
+    // a peer is never demoted twice; assert that rather than guarding for it.
+    Assert(!node.m_is_stale_outbound);
+    // m_nodes_mutex guards grantOutbound and m_network_conn_counts, and lets us
+    // count peers without racing the socket handler.
+    LOCK(m_nodes_mutex);
+    if (node.fDisconnect) return false;
+    const StaleOutboundRoom room{CheckStaleOutboundRoom(node.m_conn_type, max_stale, &node)};
+    if (room.refusal) {
+        LogDebug(BCLog::NET, "peer lacks NODE_BLAKE2B and %s, %s\n", *room.refusal, node.DisconnectMsg(fLogIPs));
         node.fDisconnect = true;
         return false;
     }
     node.m_is_stale_outbound = true;
     node.grantOutbound.Release();
     if (node.IsManualOrFullOutboundConn()) --m_network_conn_counts[node.addr.GetNetwork()];
-    ++num_stale;
     LogDebug(BCLog::NET, "connected to stale outbound peer (%u/%u), %s\n",
-             num_stale, max_stale, node.ConnectionTypeAsString());
+             room.num_stale + 1, max_stale, node.ConnectionTypeAsString());
     return true;
 }
 
@@ -2898,6 +2903,18 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, Spa
             continue;
         }
 
+        // Only a NODE_BLAKE2B peer can fill a persistent outbound slot: any other
+        // is demoted to stale at the handshake and stops counting towards the
+        // target, so prefer one for every slot of it. The extra peers opened past
+        // the target are the same two types, and one without the bit is refused
+        // outright there because the target is full, so prefer one for those too.
+        const bool prefer_blake2b{conn_type == ConnectionType::OUTBOUND_FULL_RELAY || conn_type == ConnectionType::BLOCK_RELAY};
+        // Falling back to a peer without it after enough tries is what bootstraps
+        // a node that can find none, but that only helps while the peer would be
+        // kept: once it would be dropped at the handshake instead, we would
+        // reconnect to the network twice a second for nothing.
+        const bool blake2b_required{prefer_blake2b && !m_msgproc->CanTolerateStaleOutbound(conn_type)};
+
         addrman.ResolveCollisions();
 
         const auto current_time{NodeClock::now()};
@@ -2970,8 +2987,11 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, Spa
                 continue;
             }
 
-            // only consider very recently tried nodes after 30 failed attempts
-            if (current_time - addr_last_try < 10min && nTries < 30) {
+            // only consider very recently tried nodes after 30 failed attempts,
+            // unless we need NODE_BLAKE2B: an address that claims it but never
+            // completes a handshake is never corrected by SetServices, so
+            // relaxing this would re-dial it every pass
+            if (current_time - addr_last_try < 10min && (nTries < 30 || blake2b_required)) {
                 continue;
             }
 
@@ -2984,15 +3004,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, Spa
                 continue;
             }
 
-            // Prefer NODE_BLAKE2B peers for the first outbound-full-relay slots
-            // so the node quickly has peers that can serve the header chain past
-            // the hard fork. #368 demotes any non-BLAKE2B full-outbound peer to
-            // stale, so nOutboundFullRelay already counts only fork-capable ones.
-            // Fall back to any desirable peer after enough tries so a node that
-            // cannot yet find one still bootstraps.
-            if (conn_type == ConnectionType::OUTBOUND_FULL_RELAY &&
-                nOutboundFullRelay < SEED_OUTBOUND_CONNECTION_THRESHOLD &&
-                !(addr.nServices & NODE_BLAKE2B) && nTries < 30) {
+            if (prefer_blake2b && !(addr.nServices & NODE_BLAKE2B) && (nTries < 30 || blake2b_required)) {
                 continue;
             }
 

src/net.h

@@ -1069,6 +1069,12 @@ class NetEventsInterface
      */
     virtual bool HasAllDesirableServiceFlags(ServiceFlags services) const = 0;
 
+    /**
+     * Callback to determine whether one more outbound peer of this type that
+     * lacks NODE_BLAKE2B would be kept rather than dropped at the handshake.
+     */
+    virtual bool CanTolerateStaleOutbound(ConnectionType conn_type) const = 0;
+
     /**
     * Process protocol messages received from a given node
     *
@@ -1261,16 +1267,18 @@ class CConnman
     // Count the number of block-relay-only peers we have over our limit.
     int GetExtraBlockRelayCount() const;
 
+    /** NetEventsInterface::CanTolerateStaleOutbound, given the -maxstaleoutbound
+     *  budget the caller enforces. */
+    bool CanTolerateStaleOutbound(ConnectionType conn_type, unsigned int max_stale) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);
+
     /** Demote an outbound peer that did not advertise NODE_BLAKE2B to an
      *  additional connection: give up its automatic outbound semaphore slot (so
      *  we keep looking for a preferred peer) and stop counting it as our outbound
      *  coverage of its network. A demoted peer draws on the inbound budget, so it
-     *  is kept only while fewer than max_stale are already tolerated, its outbound
-     *  target is not already filled (by preferred or stale peers alike), and the
-     *  inbound budget has room; otherwise this sets fDisconnect and returns false
-     *  without demoting. Does its own logging. Must not be called on an already-
-     *  demoted peer (Assert): the version handler guarantees this by rejecting
-     *  redundant VERSION messages. */
+     *  is kept only while CheckStaleOutboundRoom finds room for it; otherwise this
+     *  sets fDisconnect and returns false without demoting. Does its own logging.
+     *  Must not be called on an already-demoted peer (Assert): the version handler
+     *  guarantees this by rejecting redundant VERSION messages. */
     bool DemoteToStaleOutbound(CNode& node, unsigned int max_stale) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);
 
     bool AddNode(const AddedNodeParams& add) EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex);
@@ -1365,6 +1373,17 @@ class CConnman
     //! in case of no limit, it will always return 0
     std::chrono::seconds GetMaxOutboundTimeLeftInCycle_() const EXCLUSIVE_LOCKS_REQUIRED(m_total_bytes_sent_mutex);
 
+    struct StaleOutboundRoom {
+        //! Unset if another stale outbound peer fits, else why not, for logging.
+        std::optional<std::string> refusal;
+        unsigned int num_stale{0};
+    };
+
+    /** Counts that decide whether another stale outbound peer fits.
+     *  `exclude`, when set, is skipped while counting, so a peer that has already
+     *  connected is measured the same way as one that has not. */
+    StaleOutboundRoom CheckStaleOutboundRoom(ConnectionType conn_type, unsigned int max_stale, const CNode* exclude) const EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex);
+
     bool BindListenPort(const CService& bindAddr, bilingual_str& strError, NetPermissionFlags permissions);
     bool Bind(const CService& addr, unsigned int flags, NetPermissionFlags permissions);
     bool InitBinds(const Options& options);

src/net_processing.cpp

@@ -493,6 +493,7 @@ class PeerManagerImpl final : public PeerManager
     void InitializeNode(const CNode& node, ServiceFlags our_services) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_tx_download_mutex);
     void FinalizeNode(const CNode& node) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_headers_presync_mutex, !m_tx_download_mutex);
     bool HasAllDesirableServiceFlags(ServiceFlags services) const override;
+    bool CanTolerateStaleOutbound(ConnectionType conn_type) const override;
     bool ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt) override
         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_most_recent_block_mutex, !m_headers_presync_mutex, g_msgproc_mutex, !m_tx_download_mutex);
     bool SendMessages(CNode* pto) override
@@ -1655,6 +1656,11 @@ bool PeerManagerImpl::HasAllDesirableServiceFlags(ServiceFlags services) const
     return !(GetDesirableServiceFlags(services) & (~services));
 }
 
+bool PeerManagerImpl::CanTolerateStaleOutbound(ConnectionType conn_type) const
+{
+    return m_connman.CanTolerateStaleOutbound(conn_type, m_opts.maxstaleoutbound);
+}
+
 ServiceFlags PeerManagerImpl::GetDesirableServiceFlags(ServiceFlags services) const
 {
     if (services & NODE_NETWORK_LIMITED) {

src/test/denialofservice_tests.cpp

@@ -489,6 +489,65 @@ BOOST_FIXTURE_TEST_CASE(stale_outbound_respects_maxconnections, OutboundTest)
     connman->ClearTestNodes();
 }
 
+//! ThreadOpenConnections asks CanTolerateStaleOutbound before opening a
+//! connection precisely so it does not open one the handshake would drop, so the
+//! answer has to match what DemoteToStaleOutbound then does. The candidate is in
+//! m_nodes for one and not the other, which is what the exclude argument of
+//! CheckStaleOutboundRoom is for; check they agree on each refusal.
+BOOST_FIXTURE_TEST_CASE(stale_outbound_room_predicts_demotion, OutboundTest)
+{
+    NodeId id{0};
+    auto connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman, Params());
+    auto peerLogic = PeerManager::make(*connman, *m_node.addrman, nullptr, *m_node.chainman, *m_node.mempool, *m_node.warnings, {});
+
+    CConnman::Options options;
+    options.m_max_automatic_connections = 20;
+    connman->Init(options);
+    std::vector<CNode*> vNodes;
+
+    // Ask whether one more stale peer of this type fits, then connect one and
+    // demote it. A refused peer keeps its outbound slot and is disconnected, so
+    // it drops out of the counts either way and the two stay comparable.
+    const auto ask_then_demote = [&](ConnectionType conn_type, unsigned int max_stale) {
+        const bool predicted{connman->CanTolerateStaleOutbound(conn_type, max_stale)};
+        AddRandomOutboundPeer(id, vNodes, *peerLogic, *connman, conn_type);
+        BOOST_CHECK_EQUAL(predicted, connman->DemoteToStaleOutbound(*vNodes.back(), max_stale));
+        return predicted;
+    };
+
+    // A budget of zero tolerates nothing, which is what makes a node started with
+    // -maxstaleoutbound=0 dial NODE_BLAKE2B peers only.
+    BOOST_CHECK(!ask_then_demote(ConnectionType::OUTBOUND_FULL_RELAY, 0));
+
+    // With room, both agree the peer is kept, up to the block-relay target.
+    BOOST_CHECK(ask_then_demote(ConnectionType::BLOCK_RELAY, 1000));
+    BOOST_CHECK(ask_then_demote(ConnectionType::BLOCK_RELAY, 1000));
+    // Which stale peers now fill, so another buys nothing.
+    BOOST_CHECK(!ask_then_demote(ConnectionType::BLOCK_RELAY, 1000));
+
+    // Those two also spend a -maxstaleoutbound budget of 2, whatever the target.
+    BOOST_CHECK(!ask_then_demote(ConnectionType::OUTBOUND_FULL_RELAY, 2));
+    BOOST_CHECK(ask_then_demote(ConnectionType::OUTBOUND_FULL_RELAY, 1000));
+
+    // Fill the inbound budget the stale peers already tolerated above draw on,
+    // counted rather than hardcoded so reordering the cases cannot silently turn
+    // the check below into one of the other two refusals.
+    const int tolerated{static_cast<int>(std::count_if(vNodes.begin(), vNodes.end(),
+        [](const CNode* n) -> bool { return n->m_is_stale_outbound; }))};
+    const int max_inbound = options.m_max_automatic_connections
+        - (MAX_OUTBOUND_FULL_RELAY_CONNECTIONS + MAX_BLOCK_RELAY_ONLY_CONNECTIONS + MAX_FEELER_CONNECTIONS);
+    BOOST_REQUIRE(tolerated < max_inbound);
+    for (int i = tolerated; i < max_inbound; ++i) {
+        AddRandomOutboundPeer(id, vNodes, *peerLogic, *connman, ConnectionType::INBOUND);
+    }
+    BOOST_CHECK(!ask_then_demote(ConnectionType::OUTBOUND_FULL_RELAY, 1000));
+
+    for (const CNode* node : vNodes) {
+        peerLogic->FinalizeNode(*node);
+    }
+    connman->ClearTestNodes();
+}
+
 BOOST_AUTO_TEST_CASE(peer_discouragement)
 {
     LOCK(NetEventsInterface::g_msgproc_mutex);

test/functional/p2p_blake2b_outbound_preference.py

@@ -1,13 +1,16 @@
 #!/usr/bin/env python3
-"""ThreadOpenConnections prefers NODE_BLAKE2B peers for the first outbound
-full-relay slots (SEED_OUTBOUND_CONNECTION_THRESHOLD), so a node quickly gains
-peers that can serve the header chain past the BLAKE2b hard fork. It falls back
-to any desirable peer after enough tries so a node with none yet still bootstraps.
+"""ThreadOpenConnections prefers NODE_BLAKE2B peers when filling an outbound
+target, so a node quickly gains peers that can serve the header chain past the
+BLAKE2b hard fork. It falls back to any desirable peer after enough tries, while
+one that lacks the bit would still be tolerated as a stale peer, so a node that
+can find none still bootstraps.
 
 The addresses are unreachable (connections go through an unreachable proxy and
-never complete), so we observe the addresses the node attempts in debug.log. A
-fresh node with no anchors always opens OUTBOUND_FULL_RELAY first, so every
-attempt exercises the preference.
+never complete), so we observe the addresses the node attempts in debug.log and
+no peer is ever demoted, which is what keeps the fallback in play here. A fresh
+node with no anchors always opens OUTBOUND_FULL_RELAY first, so every attempt
+exercises the preference. p2p_blake2b_outbound_slots.py covers what happens once
+connections complete and the stale budget runs out.
 
 Case A: mix of NODE_BLAKE2B (250.x) and non-HF (251.x) -> the first slots are HF.
 Case B: only non-HF (251.x)                            -> attempted anyway (fallback).