diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 6b6594f7da1b..c277a47c0b4a 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -417,6 +417,14 @@ 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}; + + /** Whether we consider this a preferred download peer. */ + std::atomic m_preferred_download{false}; + explicit Peer(NodeId id, ServiceFlags our_services, bool is_inbound) : m_id{id} , m_our_services{our_services} @@ -454,12 +462,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}; - /** 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. * @@ -583,7 +585,7 @@ class PeerManagerImpl final : public PeerManager void BlockChecked(const std::shared_ptr& block, const BlockValidationState& state) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock) override - EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex); + EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex, !m_peer_mutex); /** Implement NetEventsInterface */ void InitializeNode(const CNode& node, ServiceFlags our_services) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_tx_download_mutex); @@ -932,7 +934,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}; @@ -1380,9 +1382,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; } @@ -1397,7 +1398,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) { @@ -1790,6 +1791,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->m_preferred_download; + 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); @@ -1813,7 +1822,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; @@ -1824,7 +1832,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); @@ -2230,6 +2237,8 @@ void PeerManagerImpl::BlockDisconnected(const std::shared_ptr &blo */ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock) { + AssertLockNotHeld(m_peer_mutex); + auto pcmpctblock = std::make_shared(*pblock, FastRandomContext().rand64()); LOCK(cs_main); @@ -2265,9 +2274,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()); @@ -3129,7 +3140,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)) { @@ -3988,12 +3999,8 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string MakeAndPushMessage(pfrom, NetMsgType::VERACK); // 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.m_preferred_download = (!pfrom.IsInboundConn() || pfrom.HasPermission(NetPermissionFlags::NoBan)) && !pfrom.IsAddrFetchConn() && CanServeBlocks(peer); + m_num_preferred_download_peers += peer.m_preferred_download; // Attempt to initialize address relay for outbound peers and use result // to decide whether to send GETADDR, so that we don't send it to @@ -4129,10 +4136,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.m_preferred_download, .m_relay_permissions = pfrom.HasPermission(NetPermissionFlags::Relay), .m_wtxid_relay = peer.m_wtxid_relay, }); @@ -4162,10 +4168,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; @@ -4782,13 +4786,9 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string return; } - { - LOCK(cs_main); - const CNodeState *nodestate = State(pfrom.GetId()); - if (!nodestate->m_provides_cmpctblocks) { - LogDebug(BCLog::CMPCTBLOCK, "%s sent us a compact block despite never having sent us a SENDCMPCT!", pfrom.LogPeer()); - return; - } + if (!peer.m_provides_cmpctblocks) { + LogDebug(BCLog::CMPCTBLOCK, "%s sent us a compact block despite never having sent us a SENDCMPCT!", pfrom.LogPeer()); + return; } CBlockHeaderAndShortTxIDs cmpctblock; @@ -6110,7 +6110,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.m_preferred_download) { 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 @@ -6170,7 +6170,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 @@ -6223,7 +6223,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__, @@ -6455,7 +6455,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.m_preferred_download >= 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 diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index 00d69fb11a52..f27983161299 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -209,7 +209,7 @@ bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, return false; } // Decide the TxRequestTracker parameters for this announcement: - // - "preferred": if fPreferredDownload is set (= outbound, or NetPermissionFlags::NoBan permission) + // - "preferred": if m_preferred_download is set (= outbound, or NetPermissionFlags::NoBan permission) // - "reqtime": current time plus delays for: // - NONPREF_PEER_TX_DELAY for announcements from non-preferred connections // - TXID_RELAY_DELAY for txid announcements while wtxid peers are available diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 70aeb4266621..af85f55f59c2 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -367,7 +367,7 @@ def setup_network(self): # # If all nodes are in IBD (clean chain from genesis), node0 is assumed to be the source of blocks (miner). To # ensure block propagation, all nodes will establish outgoing connections toward node0. - # See fPreferredDownload in net_processing. + # See m_preferred_download in net_processing. # # If further outbound connections are needed, they can be added at the beginning of the test with e.g. # self.connect_nodes(1, 2) diff --git a/test/functional/wallet_listsinceblock.py b/test/functional/wallet_listsinceblock.py index 0517c2aacde1..1637b1fb7a5b 100755 --- a/test/functional/wallet_listsinceblock.py +++ b/test/functional/wallet_listsinceblock.py @@ -31,7 +31,7 @@ def skip_test_if_missing_module(self): def run_test(self): # All nodes are in IBD from genesis, so they'll need the miner (node2) to be an outbound connection, or have - # only one connection. (See fPreferredDownload in net_processing) + # only one connection. (See m_preferred_download in net_processing) self.connect_nodes(1, 2) self.generate(self.nodes[2], COINBASE_MATURITY + 1)