net: move state dependent peer services flags

No behavior change. Just an intermediate refactoring.

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

In the follow-up commit(s), the desirable service flags will be
dynamically adjusted to detect post-IBD stalling scenarios (such
as a +48-hour inactive node that must prefer full node connections
instead of limited peer connections because they cannot provide
historical blocks). Additionally, this encapsulation enable us
to customize the connections decision-making process based on
new user's configurations in the future.

7 files changed, 50 insertions, 44 deletions

diff --git a/src/net.cpp b/src/net.cpp
index cb9f158206..6575441cad 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -2637,7 +2637,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
                 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());
@@ -2703,7 +2703,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
             // 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;
diff --git a/src/net.h b/src/net.h
index 547e032ba6..7841434037 100644
--- a/src/net.h
+++ b/src/net.h
@@ -1006,6 +1006,12 @@ public:
     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
     *
     * @param[in]   pnode           The node which we have received messages from.
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 3abedf3e7e..3e09b54336 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -499,6 +499,7 @@ public:
     /** 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, !m_headers_presync_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, !m_headers_presync_mutex, g_msgproc_mutex);
     bool SendMessages(CNode* pto) override
@@ -523,6 +524,7 @@ public:
                         const std::chrono::microseconds time_received, const std::atomic<bool>& interruptMsgProc) override
         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, !m_most_recent_block_mutex, !m_headers_presync_mutex, g_msgproc_mutex);
     void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) override;
+    ServiceFlags GetDesirableServiceFlags(ServiceFlags services) const override;
 
 private:
     /** Consider evicting an outbound peer based on the amount of time they've been behind our tip */
@@ -1668,6 +1670,20 @@ 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 && GetServicesFlagsIBDCache()) {
+        return ServiceFlags(NODE_NETWORK_LIMITED | NODE_WITNESS);
+    }
+    return ServiceFlags(NODE_NETWORK | NODE_WITNESS);
+}
+
 PeerRef PeerManagerImpl::GetPeerRef(NodeId id) const
 {
     LOCK(m_peer_mutex);
diff --git a/src/net_processing.h b/src/net_processing.h
index f3aa2c84c6..f8d7a8f511 100644
--- a/src/net_processing.h
+++ b/src/net_processing.h
@@ -110,6 +110,29 @@ public:
 
     /** This function is used for testing the stale tip eviction logic, see denialofservice_tests.cpp */
     virtual void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) = 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
diff --git a/src/protocol.cpp b/src/protocol.cpp
index 27a0a2ffc1..694c13a7a8 100644
--- a/src/protocol.cpp
+++ b/src/protocol.cpp
@@ -125,18 +125,12 @@ 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 | NODE_WITNESS);
-    }
-    return ServiceFlags(NODE_NETWORK | NODE_WITNESS);
-}
-
 void SetServiceFlagsIBDCache(bool state) {
     g_initial_block_download_completed = state;
 }
 
+bool GetServicesFlagsIBDCache() { return g_initial_block_download_completed; }
+
 CInv::CInv()
 {
     type = 0;
diff --git a/src/protocol.h b/src/protocol.h
index a3c472d098..e19560e33b 100644
--- a/src/protocol.h
+++ b/src/protocol.h
@@ -311,29 +311,6 @@ 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.
- */
-ServiceFlags GetDesirableServiceFlags(ServiceFlags services);
-
-/**
  * State independent service flags.
  * If the return value is changed, contrib/seeds/makeseeds.py
  * should be updated appropriately to filter for nodes with
@@ -343,16 +320,7 @@ constexpr ServiceFlags SeedsServiceFlags() { return ServiceFlags(NODE_NETWORK |
 
 /** 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));
-}
+bool GetServicesFlagsIBDCache();
 
 /**
  * Checks if a peer with the given service flags may be capable of having a
diff --git a/src/test/fuzz/integer.cpp b/src/test/fuzz/integer.cpp
index 9b97958a5d..2577f9e97a 100644
--- a/src/test/fuzz/integer.cpp
+++ b/src/test/fuzz/integer.cpp
@@ -213,7 +213,6 @@ FUZZ_TARGET(integer, .init = initialize_integer)
 
     {
         const ServiceFlags service_flags = (ServiceFlags)u64;
-        (void)HasAllDesirableServiceFlags(service_flags);
         (void)MayHaveUsefulAddressDB(service_flags);
     }