From 0954ae669683a3179a362dd47cb6176cda914eb2 Mon Sep 17 00:00:00 2001 From: Eugene Siegel Date: Wed, 17 Jun 2026 09:39:20 -0400 Subject: [PATCH 1/2] net: move cmpctblocks fields from CNodeState to Peer Both m_requested_hb_cmpctblocks & m_provides_cmpctblocks were protected by cs_main which is unnecessary. Instead convert them to std::atomic as Mutex is not needed. This way we don't need to worry about potential lock inversion in the future or thread safety annotations. --- src/net_processing.cpp | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index c01f93c21aed..dca3a9eeff03 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -413,6 +413,11 @@ struct Peer { * timestamp the peer sent in the version message. */ std::atomic m_time_offset{0s}; + /** Whether this peer wants invs or cmpctblocks (when possible) for block announcements. */ + std::atomic m_requested_hb_cmpctblocks{false}; + /** Whether this peer will send us cmpctblocks if we request them. */ + std::atomic m_provides_cmpctblocks{false}; + explicit Peer(NodeId id, ServiceFlags our_services, bool is_inbound) : m_id{id} , m_our_services{our_services} @@ -452,10 +457,6 @@ struct CNodeState { std::chrono::microseconds m_downloading_since{0us}; //! Whether we consider this a preferred download peer. bool fPreferredDownload{false}; - /** Whether this peer wants invs or cmpctblocks (when possible) for block announcements. */ - bool m_requested_hb_cmpctblocks{false}; - /** Whether this peer will send us cmpctblocks if we request them. */ - bool m_provides_cmpctblocks{false}; /** State used to enforce CHAIN_SYNC_TIMEOUT and EXTRA_PEER_CHECK_INTERVAL logic. * @@ -1290,9 +1291,8 @@ void PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid) // compact block. if (m_opts.ignore_incoming_txs) return; - CNodeState* nodestate = State(nodeid); PeerRef peer{GetPeerRef(nodeid)}; - if (!nodestate || !nodestate->m_provides_cmpctblocks) { + if (!peer || !peer->m_provides_cmpctblocks) { // Don't request compact blocks if the peer has not signalled support return; } @@ -1307,7 +1307,7 @@ void PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid) PeerRef peer_ref{GetPeerRef(*it)}; if (peer_ref && !peer_ref->m_is_inbound) ++num_outbound_hb_peers; } - if (peer && peer->m_is_inbound) { + if (peer->m_is_inbound) { // If we're adding an inbound HB peer, make sure we're not removing // our last outbound HB peer in the process. if (lNodesAnnouncingHeaderAndIDs.size() >= 3 && num_outbound_hb_peers == 1) { @@ -2166,9 +2166,11 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha return; ProcessBlockAvailability(pnode->GetId()); CNodeState &state = *State(pnode->GetId()); + PeerRef peer{GetPeerRef(pnode->GetId())}; + if (!peer) return; // If the peer has, or we announced to them the previous block already, // but we don't think they have this one, go ahead and announce it - if (state.m_requested_hb_cmpctblocks && !PeerHasHeader(&state, pindex) && PeerHasHeader(&state, pindex->pprev)) { + if (peer->m_requested_hb_cmpctblocks && !PeerHasHeader(&state, pindex) && PeerHasHeader(&state, pindex->pprev)) { LogDebug(BCLog::NET, "%s sending header-and-ids %s to peer=%d\n", "PeerManager::NewPoWValidBlock", hashBlock.ToString(), pnode->GetId()); @@ -2917,7 +2919,7 @@ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, c } if (vGetData.size() > 0) { if (!m_opts.ignore_incoming_txs && - nodestate->m_provides_cmpctblocks && + peer.m_provides_cmpctblocks && vGetData.size() == 1 && mapBlocksInFlight.size() == 1 && last_header.pprev->IsValid(BLOCK_VALID_CHAIN)) { @@ -3942,10 +3944,8 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string // Only support compact block relay with witnesses if (sendcmpct_version != CMPCTBLOCKS_VERSION) return; - LOCK(cs_main); - CNodeState* nodestate = State(pfrom.GetId()); - nodestate->m_provides_cmpctblocks = true; - nodestate->m_requested_hb_cmpctblocks = sendcmpct_hb; + peer.m_provides_cmpctblocks = true; + peer.m_requested_hb_cmpctblocks = sendcmpct_hb; // save whether peer selects us as BIP152 high-bandwidth peer // (receiving sendcmpct(1) signals high-bandwidth, sendcmpct(0) low-bandwidth) pfrom.m_bip152_highbandwidth_from = sendcmpct_hb; @@ -5917,7 +5917,7 @@ bool PeerManagerImpl::SendMessages(CNode& node) LOCK(peer.m_block_inv_mutex); std::vector vHeaders; bool fRevertToInv = ((!peer.m_prefers_headers && - (!state.m_requested_hb_cmpctblocks || peer.m_blocks_for_headers_relay.size() > 1)) || + (!peer.m_requested_hb_cmpctblocks || peer.m_blocks_for_headers_relay.size() > 1)) || peer.m_blocks_for_headers_relay.size() > MAX_BLOCKS_TO_ANNOUNCE); const CBlockIndex *pBestIndex = nullptr; // last header queued for delivery ProcessBlockAvailability(node.GetId()); // ensure pindexBestKnownBlock is up-to-date @@ -5970,7 +5970,7 @@ bool PeerManagerImpl::SendMessages(CNode& node) } } if (!fRevertToInv && !vHeaders.empty()) { - if (vHeaders.size() == 1 && state.m_requested_hb_cmpctblocks) { + if (vHeaders.size() == 1 && peer.m_requested_hb_cmpctblocks) { // We only send up to 1 block as header-and-ids, as otherwise // probably means we're doing an initial-ish-sync or they're slow LogDebug(BCLog::NET, "%s sending header-and-ids %s to peer=%d\n", __func__, From 765f65971aa4fc6c7c0bdf971b4cdc771a5a296e Mon Sep 17 00:00:00 2001 From: Eugene Siegel Date: Wed, 17 Jun 2026 11:58:32 -0400 Subject: [PATCH 2/2] net: move fPreferredDownload to Peer, make atomic Also make the m_num_preferred_download_peers counter atomic and add an assert in FinalizeNode. The asserts for m_num_preferred_download_peers in FinalizeNode are still safe even without cs_main locking because of the implicit guarantee that ProcessMessage may not run for a peer at the same time as FinalizeNode. This has the nice benefit of being able to remove cs_main usage in some places. --- src/net_processing.cpp | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index dca3a9eeff03..6f2571d6931f 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -418,6 +418,9 @@ struct Peer { /** Whether this peer will send us cmpctblocks if we request them. */ std::atomic m_provides_cmpctblocks{false}; + /** Whether we consider this a preferred download peer. */ + std::atomic fPreferredDownload{false}; + explicit Peer(NodeId id, ServiceFlags our_services, bool is_inbound) : m_id{id} , m_our_services{our_services} @@ -455,8 +458,6 @@ struct CNodeState { std::list vBlocksInFlight; //! When the first entry in vBlocksInFlight started downloading. Don't care when vBlocksInFlight is empty. std::chrono::microseconds m_downloading_since{0us}; - //! Whether we consider this a preferred download peer. - bool fPreferredDownload{false}; /** State used to enforce CHAIN_SYNC_TIMEOUT and EXTRA_PEER_CHECK_INTERVAL logic. * @@ -851,7 +852,7 @@ class PeerManagerImpl final : public PeerManager int m_outbound_peers_with_protect_from_disconnect GUARDED_BY(cs_main) = 0; /** Number of preferable block download peers. */ - int m_num_preferred_download_peers GUARDED_BY(cs_main){0}; + std::atomic m_num_preferred_download_peers{0}; /** Stalling timeout for blocks in IBD */ std::atomic m_block_stalling_timeout{BLOCK_STALLING_TIMEOUT_DEFAULT}; @@ -1699,6 +1700,14 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) assert(peer != nullptr); m_wtxid_relay_peers -= peer->m_wtxid_relay; assert(m_wtxid_relay_peers >= 0); + m_num_preferred_download_peers -= peer->fPreferredDownload; + assert(m_num_preferred_download_peers >= 0); + { + LOCK(m_peer_mutex); + if (m_peer_map.empty()) { + assert(m_num_preferred_download_peers == 0); + } + } } CNodeState *state = State(nodeid); assert(state != nullptr); @@ -1722,7 +1731,6 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) m_txdownloadman.DisconnectedPeer(nodeid); } if (m_txreconciliation) m_txreconciliation->ForgetPeer(nodeid); - m_num_preferred_download_peers -= state->fPreferredDownload; m_peers_downloading_from -= (!state->vBlocksInFlight.empty()); assert(m_peers_downloading_from >= 0); m_outbound_peers_with_protect_from_disconnect -= state->m_chain_sync.m_protect; @@ -1733,7 +1741,6 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) if (m_node_states.empty()) { // Do a consistency check after the last peer is removed. assert(mapBlocksInFlight.empty()); - assert(m_num_preferred_download_peers == 0); assert(m_peers_downloading_from == 0); assert(m_outbound_peers_with_protect_from_disconnect == 0); assert(m_wtxid_relay_peers == 0); @@ -3778,10 +3785,8 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string // Potentially mark this peer as a preferred download peer. { - LOCK(cs_main); - CNodeState* state = State(pfrom.GetId()); - state->fPreferredDownload = (!pfrom.IsInboundConn() || pfrom.HasPermission(NetPermissionFlags::NoBan)) && !pfrom.IsAddrFetchConn() && CanServeBlocks(peer); - m_num_preferred_download_peers += state->fPreferredDownload; + peer.fPreferredDownload = (!pfrom.IsInboundConn() || pfrom.HasPermission(NetPermissionFlags::NoBan)) && !pfrom.IsAddrFetchConn() && CanServeBlocks(peer); + m_num_preferred_download_peers += peer.fPreferredDownload; } // Attempt to initialize address relay for outbound peers and use result @@ -3918,10 +3923,9 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string } { - LOCK2(::cs_main, m_tx_download_mutex); - const CNodeState* state = State(pfrom.GetId()); + LOCK(m_tx_download_mutex); m_txdownloadman.ConnectedPeer(pfrom.GetId(), node::TxDownloadConnectionInfo { - .m_preferred = state->fPreferredDownload, + .m_preferred = peer.fPreferredDownload, .m_relay_permissions = pfrom.HasPermission(NetPermissionFlags::Relay), .m_wtxid_relay = peer.m_wtxid_relay, }); @@ -5857,7 +5861,7 @@ bool PeerManagerImpl::SendMessages(CNode& node) // block download from this peer -- this mostly affects behavior while // in IBD (once out of IBD, we sync from all peers). bool sync_blocks_and_headers_from_peer = false; - if (state.fPreferredDownload) { + if (peer.fPreferredDownload) { sync_blocks_and_headers_from_peer = true; } else if (CanServeBlocks(peer) && !node.IsAddrFetchConn()) { // Typically this is an inbound peer. If we don't have any outbound @@ -6205,7 +6209,7 @@ bool PeerManagerImpl::SendMessages(CNode& node) if (state.fSyncStarted && peer.m_headers_sync_timeout < std::chrono::microseconds::max()) { // Detect whether this is a stalling initial-headers-sync peer if (m_chainman.m_best_header->Time() <= NodeClock::now() - 24h) { - if (current_time > peer.m_headers_sync_timeout && nSyncStarted == 1 && (m_num_preferred_download_peers - state.fPreferredDownload >= 1)) { + if (current_time > peer.m_headers_sync_timeout && nSyncStarted == 1 && (m_num_preferred_download_peers - peer.fPreferredDownload >= 1)) { // Disconnect a peer (without NetPermissionFlags::NoBan permission) if it is our only sync peer, // and we have others we could be using instead. // Note: If all our peers are inbound, then we won't