From 07f4cebe5780f1039541d989e64b70eccc5b4eb5 Mon Sep 17 00:00:00 2001 From: Sergi Delgado Segura Date: Wed, 5 Jun 2024 12:56:09 -0400 Subject: [PATCH] refactor: move m_is_inbound out of CNodeState `m_is_inbound` cannot be changed throughout the life of a `Peer`. However, we are currently storing it in `CNodeState`, which requires locking `cs_main` in order to access it. This can be moved to the outside scope and only require `m_peer_mutex`. This is a refactor in preparation for Erlay reworks. --- src/net_processing.cpp | 42 ++++++++++++++++++------------------------ 1 file changed, 18 insertions(+), 24 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 6553393160..2fdd7987d1 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -224,6 +224,9 @@ struct Peer { /** Services this peer offered to us. */ std::atomic m_their_services{NODE_NONE}; + //! Whether this peer is an inbound connection + const bool m_is_inbound; + /** Protects misbehavior data members */ Mutex m_misbehavior_mutex; /** Whether this peer should be disconnected and marked as discouraged (unless it has NetPermissionFlags::NoBan permission). */ @@ -394,9 +397,10 @@ struct Peer { * timestamp the peer sent in the version message. */ std::atomic m_time_offset{0s}; - explicit Peer(NodeId id, ServiceFlags our_services) + explicit Peer(NodeId id, ServiceFlags our_services, bool is_inbound) : m_id{id} , m_our_services{our_services} + , m_is_inbound{is_inbound} {} private: @@ -476,11 +480,6 @@ struct CNodeState { //! Time of last new block announcement int64_t m_last_block_announcement{0}; - - //! Whether this peer is an inbound connection - const bool m_is_inbound; - - CNodeState(bool is_inbound) : m_is_inbound(is_inbound) {} }; class PeerManagerImpl final : public PeerManager @@ -1015,7 +1014,7 @@ class PeerManagerImpl final : public PeerManager bool IsBlockRequested(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** Have we requested this block from an outbound peer */ - bool IsBlockRequestedFromOutbound(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + bool IsBlockRequestedFromOutbound(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main, !m_peer_mutex); /** Remove this block from our tracked requested blocks. Called if: * - the block has been received from a peer @@ -1099,7 +1098,7 @@ class PeerManagerImpl final : public PeerManager * lNodesAnnouncingHeaderAndIDs, and keeping that list under a certain size by * removing the first element if necessary. */ - void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid) EXCLUSIVE_LOCKS_REQUIRED(cs_main, !m_peer_mutex); /** Stack of nodes which we have set to announce using compact blocks */ std::list lNodesAnnouncingHeaderAndIDs GUARDED_BY(cs_main); @@ -1302,8 +1301,8 @@ bool PeerManagerImpl::IsBlockRequestedFromOutbound(const uint256& hash) { for (auto range = mapBlocksInFlight.equal_range(hash); range.first != range.second; range.first++) { auto [nodeid, block_it] = range.first->second; - CNodeState& nodestate = *Assert(State(nodeid)); - if (!nodestate.m_is_inbound) return true; + PeerRef peer{GetPeerRef(nodeid)}; + if (peer && !peer->m_is_inbound) return true; } return false; @@ -1392,6 +1391,7 @@ void PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid) if (m_opts.ignore_incoming_txs) return; CNodeState* nodestate = State(nodeid); + PeerRef peer{GetPeerRef(nodeid)}; if (!nodestate || !nodestate->m_provides_cmpctblocks) { // Don't request compact blocks if the peer has not signalled support return; @@ -1404,15 +1404,15 @@ void PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid) lNodesAnnouncingHeaderAndIDs.push_back(nodeid); return; } - CNodeState *state = State(*it); - if (state != nullptr && !state->m_is_inbound) ++num_outbound_hb_peers; + PeerRef peer_ref{GetPeerRef(*it)}; + if (peer_ref && !peer_ref->m_is_inbound) ++num_outbound_hb_peers; } - if (nodestate->m_is_inbound) { + if (peer && 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) { - CNodeState *remove_node = State(lNodesAnnouncingHeaderAndIDs.front()); - if (remove_node != nullptr && !remove_node->m_is_inbound) { + PeerRef remove_peer{GetPeerRef(lNodesAnnouncingHeaderAndIDs.front())}; + if (remove_peer && !remove_peer->m_is_inbound) { // Put the HB outbound peer in the second slot, so that it // doesn't get removed. std::swap(lNodesAnnouncingHeaderAndIDs.front(), *std::next(lNodesAnnouncingHeaderAndIDs.begin())); @@ -1720,7 +1720,7 @@ void PeerManagerImpl::InitializeNode(const CNode& node, ServiceFlags our_service NodeId nodeid = node.GetId(); { LOCK(cs_main); // For m_node_states - m_node_states.emplace_hint(m_node_states.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(node.IsInboundConn())); + m_node_states.try_emplace(m_node_states.end(), nodeid); } { LOCK(m_tx_download_mutex); @@ -1731,7 +1731,7 @@ void PeerManagerImpl::InitializeNode(const CNode& node, ServiceFlags our_service our_services = static_cast(our_services | NODE_BLOOM); } - PeerRef peer = std::make_shared(nodeid, our_services); + PeerRef peer = std::make_shared(nodeid, our_services, node.IsInboundConn()); { LOCK(m_peer_mutex); m_peer_map.emplace_hint(m_peer_map.end(), nodeid, peer); @@ -1968,15 +1968,9 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati break; case BlockValidationResult::BLOCK_CACHED_INVALID: { - LOCK(cs_main); - CNodeState *node_state = State(nodeid); - if (node_state == nullptr) { - break; - } - // Discourage outbound (but not inbound) peers if on an invalid chain. // Exempt HB compact block peers. Manual connections are always protected from discouragement. - if (!via_compact_block && !node_state->m_is_inbound) { + if (peer && !via_compact_block && !peer->m_is_inbound) { if (peer) Misbehaving(*peer, message); return; }