Merge bitcoin#28170: p2p: adaptive connections services flags

27f260a net: remove now unused global 'g_initial_block_download_completed' (furszy)
aff7d92 test: add coverage for peerman adaptive connections service flags (furszy)
6ed5360 net: peer manager, dynamically adjust desirable services flag (furszy)
9f36e59 net: move state dependent peer services flags (furszy)
f9ac96b net: decouple state independent service flags from desirable ones (furszy)
97df4e3 net: store best block tip time inside PeerManager (furszy)

Pull request description:

  Derived from bitcoin#28120 discussion.

  By relocating the peer desirable services flags into the peer manager, we
  allow the connections acceptance process to handle post-IBD potential
  stalling scenarios.

  The peer manager will be able to dynamically adjust the services flags
  based on the node's proximity to the tip (back and forth). Allowing the node
  to recover from the following post-IBD scenario:
  Suppose the node has successfully synced the chain, but later experienced
  dropped connections and remained inactive for a duration longer than the limited
  peers threshold (the timeframe within which limited peers can provide blocks). In
  such cases, upon reconnecting to the network, the node might only establish
  connections with limited peers, filling up all available outbound slots. Resulting
  in an inability to synchronize the chain (because limited peers will not provide
  blocks older than the `NODE_NETWORK_LIMITED_MIN_BLOCKS` threshold).

ACKs for top commit:
  achow101:
    ACK 27f260a
  vasild:
    ACK 27f260a
  naumenkogs:
    ACK 27f260a
  mzumsande:
    Light Code Review ACK 27f260a
  andrewtoth:
    ACK 27f260a

Tree-SHA512: 07befb9bcd0b60a4e7c45e4429c02e7b6c66244f0910f4b2ad97c9b98258b6f46c914660a717b5ed4ef4814d0dbfae6e18e6559fe9bec7d0fbc2034109200953

Author: achow101

src/Makefile.test.include

@@ -154,6 +154,7 @@ BITCOIN_TESTS =\
   test/net_tests.cpp \
   test/netbase_tests.cpp \
   test/orphanage_tests.cpp \
+  test/peerman_tests.cpp \
   test/pmt_tests.cpp \
   test/policyestimator_tests.cpp \
   test/pool_tests.cpp \

src/init.cpp

@@ -2511,13 +2511,15 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     // ********************************************************* Step 12: start node
 
     //// debug print
+    int64_t best_block_time{};
     {
         LOCK(cs_main);
         LogPrintf("block tree size = %u\n", chainman.BlockIndex().size());
         chain_active_height = chainman.ActiveChain().Height();
+        best_block_time = chainman.ActiveChain().Tip() ? chainman.ActiveChain().Tip()->GetBlockTime() : Params().GenesisBlock().GetBlockTime();
         if (tip_info) {
             tip_info->block_height = chain_active_height;
-            tip_info->block_time = chainman.ActiveChain().Tip() ? chainman.ActiveChain().Tip()->GetBlockTime() : Params().GenesisBlock().GetBlockTime();
+            tip_info->block_time = best_block_time
             tip_info->block_hash = chainman.ActiveChain().Tip() ? chainman.ActiveChain().Tip()->GetBlockHash() : Params().GenesisBlock().GetHash();
             tip_info->verification_progress = GuessVerificationProgress(Params().TxData(), chainman.ActiveChain().Tip());
         }
@@ -2527,7 +2529,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         }
     }
     LogPrintf("nBestHeight = %d\n", chain_active_height);
-    if (node.peerman) node.peerman->SetBestHeight(chain_active_height);
+    if (node.peerman) node.peerman->SetBestBlock(chain_active_height, std::chrono::seconds{best_block_time});
 
     // Map ports with UPnP or NAT-PMP.
     StartMapPort(args.GetBoolArg("-upnp", DEFAULT_UPNP), args.GetBoolArg("-natpmp", DEFAULT_NATPMP));

src/net.cpp

@@ -217,7 +217,7 @@ static std::vector<CAddress> ConvertSeeds(const std::vector<uint8_t> &vSeedsIn)
     while (!s.eof()) {
         CService endpoint;
         s >> endpoint;
-        CAddress addr{endpoint, GetDesirableServiceFlags(NODE_NONE)};
+        CAddress addr{endpoint, SeedsServiceFlags()};
         addr.nTime = rng.rand_uniform_delay(Now<NodeSeconds>() - one_week, -one_week);
         LogPrint(BCLog::NET, "Added hardcoded seed: %s\n", addr.ToStringAddrPort());
         vSeedsOut.push_back(addr);
@@ -2747,7 +2747,7 @@ void CConnman::ThreadDNSAddressSeed()
             AddAddrFetch(seed);
         } else {
             std::vector<CAddress> vAdd;
-            ServiceFlags requiredServiceBits = GetDesirableServiceFlags(NODE_NONE);
+            constexpr ServiceFlags requiredServiceBits{SeedsServiceFlags()};
             std::string host = strprintf("x%x.%s", requiredServiceBits, seed);
             CNetAddr resolveSource;
             if (!resolveSource.SetInternal(host)) {
@@ -3145,7 +3145,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, CDe
                 const CAddress addr = m_anchors.back();
                 m_anchors.pop_back();
                 if (!addr.IsValid() || IsLocal(addr) || !g_reachable_nets.Contains(addr) ||
-                    !HasAllDesirableServiceFlags(addr.nServices) ||
+                    !m_msgproc->HasAllDesirableServiceFlags(addr.nServices) ||
                     outbound_ipv46_peer_netgroups.count(m_netgroupman.GetGroup(addr))) continue;
                 addrConnect = addr;
                 LogPrint(BCLog::NET, "Trying to make an anchor connection to %s\n", addrConnect.ToStringAddrPort());
@@ -3220,7 +3220,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, CDe
             // for non-feelers, require all the services we'll want,
             // for feelers, only require they be a full node (only because most
             // SPV clients don't have a good address DB available)
-            if (!fFeeler && !HasAllDesirableServiceFlags(addr.nServices)) {
+            if (!fFeeler && !m_msgproc->HasAllDesirableServiceFlags(addr.nServices)) {
                 continue;
             } else if (fFeeler && !MayHaveUsefulAddressDB(addr.nServices)) {
                 continue;

src/net.h

@@ -1152,6 +1152,12 @@ class NetEventsInterface
     /** Handle removal of a peer (clear state) */
     virtual void FinalizeNode(const CNode& node) = 0;
 
+    /**
+     * Callback to determine whether the given set of service flags are sufficient
+     * for a peer to be "relevant".
+     */
+    virtual bool HasAllDesirableServiceFlags(ServiceFlags services) const = 0;
+
     /**
     * Process protocol messages received from a given node
     *

src/net_processing.cpp

@@ -161,6 +161,8 @@ static const unsigned int MAX_BLOCKS_TO_ANNOUNCE = 8;
 static const int MAX_UNCONNECTING_HEADERS = 10;
 /** Minimum blocks required to signal NODE_NETWORK_LIMITED */
 static const unsigned int NODE_NETWORK_LIMITED_MIN_BLOCKS = 288;
+/** Window, in blocks, for connecting to NODE_NETWORK_LIMITED peers */
+static const unsigned int NODE_NETWORK_LIMITED_ALLOW_CONN_BLOCKS = 144;
 /** Average delay between local address broadcasts */
 static constexpr auto AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL{24h};
 /** Average delay between peer address broadcasts */
@@ -615,6 +617,7 @@ class PeerManagerImpl final : public PeerManager
     /** Implement NetEventsInterface */
     void InitializeNode(CNode& node, ServiceFlags our_services) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
     void FinalizeNode(const CNode& node) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
+    bool HasAllDesirableServiceFlags(ServiceFlags services) const override;
     bool ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt) override
         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, !m_most_recent_block_mutex, g_msgproc_mutex);
     bool SendMessages(CNode* pto) override
@@ -634,7 +637,11 @@ class PeerManagerImpl final : public PeerManager
     void RelayTransaction(const uint256& txid) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
     void RelayRecoveredSig(const llmq::CRecoveredSig& sig, bool proactive_relay) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
     void RelayDSQ(const CCoinJoinQueue& queue) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
-    void SetBestHeight(int height) override { m_best_height = height; };
+    void SetBestBlock(int height, std::chrono::seconds time) override
+    {
+        m_best_height = height;
+        m_best_block_time = time;
+    };
     void Misbehaving(const NodeId pnode, const int howmuch, const std::string& message = "") override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
     void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv,
                         const std::chrono::microseconds time_received, const std::atomic<bool>& interruptMsgProc) override
@@ -664,6 +671,7 @@ class PeerManagerImpl final : public PeerManager
     void PeerAskPeersForTransaction(const uint256& txid) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
     size_t PeerGetRequestedObjectCount(NodeId nodeid) const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, ::cs_main);
     void PeerPostProcessMessage(MessageProcessingResult&& ret) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
+    ServiceFlags GetDesirableServiceFlags(ServiceFlags services) const override;
 
 private:
     void _RelayTransaction(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(cs_main, !m_peer_mutex);
@@ -830,6 +838,8 @@ class PeerManagerImpl final : public PeerManager
 
     /** The height of the best chain */
     std::atomic<int> m_best_height{-1};
+    /** The time of the best chain tip block */
+    std::atomic<std::chrono::seconds> m_best_block_time{0s};
 
     /** Next time to check for stale tip */
     std::chrono::seconds m_stale_tip_check_time GUARDED_BY(cs_main){0s};
@@ -1367,6 +1377,11 @@ bool PeerManagerImpl::TipMayBeStale()
     return m_last_tip_update.load() < GetTime<std::chrono::seconds>() - std::chrono::seconds{consensusParams.nPowTargetSpacing * 3} && mapBlocksInFlight.empty();
 }
 
+int64_t PeerManagerImpl::ApproximateBestBlockDepth() const
+{
+    return (GetTime<std::chrono::seconds>() - m_best_block_time.load()).count() / m_chainparams.GetConsensus().nPowTargetSpacing;
+}
+
 bool PeerManagerImpl::CanDirectFetch()
 {
     return m_chainman.ActiveChain().Tip()->GetBlockTime() > GetAdjustedTime() - m_chainparams.GetConsensus().nPowTargetSpacing * 20;
@@ -1812,6 +1827,23 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) {
     LogPrint(BCLog::NET, "Cleared nodestate for peer=%d\n", nodeid);
 }
 
+bool PeerManagerImpl::HasAllDesirableServiceFlags(ServiceFlags services) const
+{
+    // Shortcut for (services & GetDesirableServiceFlags(services)) == GetDesirableServiceFlags(services)
+    return !(GetDesirableServiceFlags(services) & (~services));
+}
+
+ServiceFlags PeerManagerImpl::GetDesirableServiceFlags(ServiceFlags services) const
+{
+    if (services & NODE_NETWORK_LIMITED) {
+        // Limited peers are desirable when we are close to the tip.
+        if (ApproximateBestBlockDepth() < NODE_NETWORK_LIMITED_ALLOW_CONN_BLOCKS) {
+            return ServiceFlags(NODE_NETWORK_LIMITED | NODE_WITNESS);
+        }
+    }
+    return ServiceFlags(NODE_NETWORK | NODE_WITNESS);
+}
+
 PeerRef PeerManagerImpl::GetPeerRef(NodeId id) const
 {
     READ_LOCK(m_peer_mutex);
@@ -2218,9 +2250,7 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha
  * in m_chainman.ActiveChain() to our peers.
  */
 void PeerManagerImpl::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload){
-    m_best_height = pindexNew->nHeight;
-
-    SetServiceFlagsIBDCache(!fInitialDownload);
+    SetBestBlock(pindexNew->nHeight, std::chrono::seconds{pindexNew->GetBlockTime()});
 
     // Don't relay inventory during initial block download.
     if (fInitialDownload) return;

src/net_processing.h

@@ -158,8 +158,8 @@ class PeerManager : public CValidationInterface, public NetEventsInterface, publ
     /** Relay recovered sigs to all interested peers */
     virtual void RelayRecoveredSig(const llmq::CRecoveredSig& sig, bool proactive_relay) = 0;
 
-    /** Set the best height */
-    virtual void SetBestHeight(int height) = 0;
+    /** Set the height of the best block and its time (seconds since epoch). */
+    virtual void SetBestBlock(int height, std::chrono::seconds time) = 0;
 
     /**
      * Increment peer's misbehavior score. If the new value surpasses DISCOURAGEMENT_THRESHOLD (specified on startup or by default), mark node to be discouraged, meaning the peer might be disconnected & added to the discouragement filter.
@@ -192,6 +192,28 @@ class PeerManager : public CValidationInterface, public NetEventsInterface, publ
     virtual void StopHandlers() = 0;
     virtual void InterruptHandlers() = 0;
     virtual void ScheduleHandlers(CScheduler& scheduler) = 0;
+    /**
+     * Gets the set of service flags which are "desirable" for a given peer.
+     *
+     * These are the flags which are required for a peer to support for them
+     * to be "interesting" to us, ie for us to wish to use one of our few
+     * outbound connection slots for or for us to wish to prioritize keeping
+     * their connection around.
+     *
+     * Relevant service flags may be peer- and state-specific in that the
+     * version of the peer may determine which flags are required (eg in the
+     * case of NODE_NETWORK_LIMITED where we seek out NODE_NETWORK peers
+     * unless they set NODE_NETWORK_LIMITED and we are out of IBD, in which
+     * case NODE_NETWORK_LIMITED suffices).
+     *
+     * Thus, generally, avoid calling with 'services' == NODE_NONE, unless
+     * state-specific flags must absolutely be avoided. When called with
+     * 'services' == NODE_NONE, the returned desirable service flags are
+     * guaranteed to not change dependent on state - ie they are suitable for
+     * use when describing peers which we know to be desirable, but for which
+     * we do not have a confirmed set of service flags.
+    */
+    virtual ServiceFlags GetDesirableServiceFlags(ServiceFlags services) const = 0;
 };
 
 #endif // BITCOIN_NET_PROCESSING_H

src/protocol.cpp

@@ -9,8 +9,6 @@
 
 #include <atomic>
 
-static std::atomic<bool> g_initial_block_download_completed(false);
-
 #define MAKE_MSG(var_name, p2p_name_str)   \
         const char* var_name=p2p_name_str; \
         static_assert(std::size(p2p_name_str) <= CMessageHeader::COMMAND_SIZE + 1, "p2p_name_str cannot be greater than COMMAND_SIZE"); // Includes +1 for null termination character.
@@ -248,18 +246,6 @@ bool CMessageHeader::IsCommandValid() const
     return true;
 }
 
-
-ServiceFlags GetDesirableServiceFlags(ServiceFlags services) {
-    if ((services & NODE_NETWORK_LIMITED) && g_initial_block_download_completed) {
-        return ServiceFlags(NODE_NETWORK_LIMITED);
-    }
-    return ServiceFlags(NODE_NETWORK);
-}
-
-void SetServiceFlagsIBDCache(bool state) {
-    g_initial_block_download_completed = state;
-}
-
 CInv::CInv()
 {
     type = 0;

src/protocol.h

@@ -347,43 +347,12 @@ enum ServiceFlags : uint64_t {
 std::vector<std::string> serviceFlagsToStr(uint64_t flags);
 
 /**
- * Gets the set of service flags which are "desirable" for a given peer.
- *
- * These are the flags which are required for a peer to support for them
- * to be "interesting" to us, ie for us to wish to use one of our few
- * outbound connection slots for or for us to wish to prioritize keeping
- * their connection around.
- *
- * Relevant service flags may be peer- and state-specific in that the
- * version of the peer may determine which flags are required (eg in the
- * case of NODE_NETWORK_LIMITED where we seek out NODE_NETWORK peers
- * unless they set NODE_NETWORK_LIMITED and we are out of IBD, in which
- * case NODE_NETWORK_LIMITED suffices).
- *
- * Thus, generally, avoid calling with peerServices == NODE_NONE, unless
- * state-specific flags must absolutely be avoided. When called with
- * peerServices == NODE_NONE, the returned desirable service flags are
- * guaranteed to not change dependent on state - ie they are suitable for
- * use when describing peers which we know to be desirable, but for which
- * we do not have a confirmed set of service flags.
- *
- * If the NODE_NONE return value is changed, contrib/seeds/makeseeds.py
- * should be updated appropriately to filter for the same nodes.
- */
-ServiceFlags GetDesirableServiceFlags(ServiceFlags services);
-
-/** Set the current IBD status in order to figure out the desirable service flags */
-void SetServiceFlagsIBDCache(bool status);
-
-/**
- * A shortcut for (services & GetDesirableServiceFlags(services))
- * == GetDesirableServiceFlags(services), ie determines whether the given
- * set of service flags are sufficient for a peer to be "relevant".
- */
-static inline bool HasAllDesirableServiceFlags(ServiceFlags services)
-{
-    return !(GetDesirableServiceFlags(services) & (~services));
-}
+ * State independent service flags.
+ * If the return value is changed, contrib/seeds/makeseeds.py
+ * should be updated appropriately to filter for nodes with
+ * desired service flags (compatible with our new flags).
+ */
+constexpr ServiceFlags SeedsServiceFlags() { return ServiceFlags(NODE_NETWORK | NODE_WITNESS); }
 
 /**
  * Checks if a peer with the given service flags may be capable of having a

src/test/fuzz/integer.cpp

@@ -211,7 +211,6 @@ FUZZ_TARGET(integer, .init = initialize_integer)
 
     {
         const ServiceFlags service_flags = (ServiceFlags)u64;
-        (void)HasAllDesirableServiceFlags(service_flags);
         (void)MayHaveUsefulAddressDB(service_flags);
     }
 

src/test/peerman_tests.cpp

@@ -0,0 +1,76 @@
+// Copyright (c) 2024-present The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or https://www.opensource.org/licenses/mit-license.php.
+
+#include <chainparams.h>
+#include <node/miner.h>
+#include <net_processing.h>
+#include <pow.h>
+#include <test/util/setup_common.h>
+#include <validation.h>
+
+#include <boost/test/unit_test.hpp>
+
+BOOST_FIXTURE_TEST_SUITE(peerman_tests, RegTestingSetup)
+
+/** Window, in blocks, for connecting to NODE_NETWORK_LIMITED peers */
+static constexpr int64_t NODE_NETWORK_LIMITED_ALLOW_CONN_BLOCKS = 144;
+
+static void mineBlock(const node::NodeContext& node, std::chrono::seconds block_time)
+{
+    auto curr_time = GetTime<std::chrono::seconds>();
+    SetMockTime(block_time); // update time so the block is created with it
+    CBlock block = node::BlockAssembler{node.chainman->ActiveChainstate(), nullptr}.CreateNewBlock(CScript() << OP_TRUE)->block;
+    while (!CheckProofOfWork(block.GetHash(), block.nBits, node.chainman->GetConsensus())) ++block.nNonce;
+    block.fChecked = true; // little speedup
+    SetMockTime(curr_time); // process block at current time
+    Assert(node.chainman->ProcessNewBlock(std::make_shared<const CBlock>(block), /*force_processing=*/true, /*min_pow_checked=*/true, nullptr));
+    SyncWithValidationInterfaceQueue(); // drain events queue
+}
+
+// Verifying when network-limited peer connections are desirable based on the node's proximity to the tip
+BOOST_AUTO_TEST_CASE(connections_desirable_service_flags)
+{
+    std::unique_ptr<PeerManager> peerman = PeerManager::make(*m_node.connman, *m_node.addrman, nullptr, *m_node.chainman, *m_node.mempool, {});
+    auto consensus = m_node.chainman->GetParams().GetConsensus();
+
+    // Check we start connecting to full nodes
+    ServiceFlags peer_flags{NODE_WITNESS | NODE_NETWORK_LIMITED};
+    BOOST_CHECK(peerman->GetDesirableServiceFlags(peer_flags) == ServiceFlags(NODE_NETWORK | NODE_WITNESS));
+
+    // Make peerman aware of the initial best block and verify we accept limited peers when we start close to the tip time.
+    auto tip = WITH_LOCK(::cs_main, return m_node.chainman->ActiveChain().Tip());
+    uint64_t tip_block_time = tip->GetBlockTime();
+    int tip_block_height = tip->nHeight;
+    peerman->SetBestBlock(tip_block_height, std::chrono::seconds{tip_block_time});
+
+    SetMockTime(tip_block_time + 1); // Set node time to tip time
+    BOOST_CHECK(peerman->GetDesirableServiceFlags(peer_flags) == ServiceFlags(NODE_NETWORK_LIMITED | NODE_WITNESS));
+
+    // Check we don't disallow limited peers connections when we are behind but still recoverable (below the connection safety window)
+    SetMockTime(GetTime<std::chrono::seconds>() + std::chrono::seconds{consensus.nPowTargetSpacing * (NODE_NETWORK_LIMITED_ALLOW_CONN_BLOCKS - 1)});
+    BOOST_CHECK(peerman->GetDesirableServiceFlags(peer_flags) == ServiceFlags(NODE_NETWORK_LIMITED | NODE_WITNESS));
+
+    // Check we disallow limited peers connections when we are further than the limited peers safety window
+    SetMockTime(GetTime<std::chrono::seconds>() + std::chrono::seconds{consensus.nPowTargetSpacing * 2});
+    BOOST_CHECK(peerman->GetDesirableServiceFlags(peer_flags) == ServiceFlags(NODE_NETWORK | NODE_WITNESS));
+
+    // By now, we tested that the connections desirable services flags change based on the node's time proximity to the tip.
+    // Now, perform the same tests for when the node receives a block.
+    RegisterValidationInterface(peerman.get());
+
+    // First, verify a block in the past doesn't enable limited peers connections
+    // At this point, our time is (NODE_NETWORK_LIMITED_ALLOW_CONN_BLOCKS + 1) * 10 minutes ahead the tip's time.
+    mineBlock(m_node, /*block_time=*/std::chrono::seconds{tip_block_time + 1});
+    BOOST_CHECK(peerman->GetDesirableServiceFlags(peer_flags) == ServiceFlags(NODE_NETWORK | NODE_WITNESS));
+
+    // Verify a block close to the tip enables limited peers connections
+    mineBlock(m_node, /*block_time=*/GetTime<std::chrono::seconds>());
+    BOOST_CHECK(peerman->GetDesirableServiceFlags(peer_flags) == ServiceFlags(NODE_NETWORK_LIMITED | NODE_WITNESS));
+
+    // Lastly, verify the stale tip checks can disallow limited peers connections after not receiving blocks for a prolonged period.
+    SetMockTime(GetTime<std::chrono::seconds>() + std::chrono::seconds{consensus.nPowTargetSpacing * NODE_NETWORK_LIMITED_ALLOW_CONN_BLOCKS + 1});
+    BOOST_CHECK(peerman->GetDesirableServiceFlags(peer_flags) == ServiceFlags(NODE_NETWORK | NODE_WITNESS));
+}
+
+BOOST_AUTO_TEST_SUITE_END()