From 9f36e591c551ec2e58a6496334541bfdae8fdfe5 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 19 Dec 2023 09:50:37 -0300 Subject: [PATCH] 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. --- src/net.cpp | 4 ++-- src/net.h | 6 ++++++ src/net_processing.cpp | 16 ++++++++++++++++ src/net_processing.h | 23 +++++++++++++++++++++++ src/protocol.cpp | 10 ++-------- src/protocol.h | 34 +--------------------------------- src/test/fuzz/integer.cpp | 1 - 7 files changed, 50 insertions(+), 44 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index cb9f1582065..6575441cadd 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2637,7 +2637,7 @@ void CConnman::ThreadOpenConnections(const std::vector 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 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 547e032ba61..78414340374 100644 --- a/src/net.h +++ b/src/net.h @@ -1005,6 +1005,12 @@ public: /** 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 * diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 3abedf3e7e9..3e09b543365 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& 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& 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 f3aa2c84c62..f8d7a8f5115 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 27a0a2ffc18..694c13a7a88 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 a3c472d0982..e19560e33be 100644 --- a/src/protocol.h +++ b/src/protocol.h @@ -310,29 +310,6 @@ enum ServiceFlags : uint64_t { */ std::vector 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 @@ -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 9b97958a5d1..2577f9e97a7 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); }