From 37c097defdabdc78fd1c0a206f28cfc94f7ad746 Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 21 Oct 2025 16:45:23 -0500 Subject: [PATCH 1/6] perf: convert m_nodes_mutex from recursive mutex to mutex --- src/net.cpp | 72 +++++++++++++++------- src/net.h | 131 ++++++++++++++++++++++------------------- src/net_processing.cpp | 22 +++---- 3 files changed, 132 insertions(+), 93 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 7035045eb5483..3017fa2aeb50d 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -338,9 +338,9 @@ bool IsLocal(const CService& addr) return mapLocalHost.count(addr) > 0; } -CNode* CConnman::FindNode(const CNetAddr& ip, bool fExcludeDisconnecting) +CNode* CConnman::FindNodeLocked(const CNetAddr& ip, bool fExcludeDisconnecting) { - LOCK(m_nodes_mutex); + AssertLockHeld(m_nodes_mutex); for (CNode* pnode : m_nodes) { if (fExcludeDisconnecting && pnode->fDisconnect) { continue; @@ -352,9 +352,9 @@ CNode* CConnman::FindNode(const CNetAddr& ip, bool fExcludeDisconnecting) return nullptr; } -CNode* CConnman::FindNode(const std::string& addrName, bool fExcludeDisconnecting) +CNode* CConnman::FindNodeLocked(const std::string& addrName, bool fExcludeDisconnecting) { - LOCK(m_nodes_mutex); + AssertLockHeld(m_nodes_mutex); for (CNode* pnode : m_nodes) { if (fExcludeDisconnecting && pnode->fDisconnect) { continue; @@ -366,9 +366,9 @@ CNode* CConnman::FindNode(const std::string& addrName, bool fExcludeDisconnectin return nullptr; } -CNode* CConnman::FindNode(const CService& addr, bool fExcludeDisconnecting) +CNode* CConnman::FindNodeLocked(const CService& addr, bool fExcludeDisconnecting) { - LOCK(m_nodes_mutex); + AssertLockHeld(m_nodes_mutex); for (CNode* pnode : m_nodes) { if (fExcludeDisconnecting && pnode->fDisconnect) { continue; @@ -380,6 +380,24 @@ CNode* CConnman::FindNode(const CService& addr, bool fExcludeDisconnecting) return nullptr; } +CNode* CConnman::FindNode(const CNetAddr& ip, bool fExcludeDisconnecting) +{ + LOCK(m_nodes_mutex); + return FindNodeLocked(ip, fExcludeDisconnecting); +} + +CNode* CConnman::FindNode(const std::string& addrName, bool fExcludeDisconnecting) +{ + LOCK(m_nodes_mutex); + return FindNodeLocked(addrName, fExcludeDisconnecting); +} + +CNode* CConnman::FindNode(const CService& addr, bool fExcludeDisconnecting) +{ + LOCK(m_nodes_mutex); + return FindNodeLocked(addr, fExcludeDisconnecting); +} + bool CConnman::AlreadyConnectedToAddress(const CAddress& addr) { return FindNode(addr.ToStringAddrPort()); @@ -458,7 +476,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo // It is possible that we already have a connection to the IP/port pszDest resolved to. // In that case, drop the connection that was just created. LOCK(m_nodes_mutex); - CNode* pnode = FindNode(static_cast(addrConnect)); + CNode* pnode = FindNodeLocked(static_cast(addrConnect)); if (pnode) { LogPrintf("Not opening a connection to %s, already connected to %s\n", pszDest, addrConnect.ToStringAddrPort()); return nullptr; @@ -3415,7 +3433,8 @@ void CConnman::ThreadOpenMasternodeConnections(CDeterministicMNManager& dmnman, MasternodeProbeConn isProbe = MasternodeProbeConn::IsNotConnection; - const auto getPendingQuorumNodes = [&]() EXCLUSIVE_LOCKS_REQUIRED(cs_vPendingMasternodes) { + const auto getPendingQuorumNodes = [&]() EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex, cs_vPendingMasternodes) { + AssertLockHeld(m_nodes_mutex); AssertLockHeld(cs_vPendingMasternodes); std::vector ret; for (const auto& group : masternodeQuorumNodes) { @@ -3428,20 +3447,21 @@ void CConnman::ThreadOpenMasternodeConnections(CDeterministicMNManager& dmnman, if (connectedNodes.count(addr2) && !connectedProRegTxHashes.count(proRegTxHash)) { // we probably connected to it before it became a masternode // or maybe we are still waiting for mnauth - (void)ForNode(addr2, [&](CNode* pnode) { - if (pnode->nTimeFirstMessageReceived.load() != 0s && GetTime() - pnode->nTimeFirstMessageReceived.load() > 5s) { - // clearly not expecting mnauth to take that long even if it wasn't the first message - // we received (as it should normally), disconnect - LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- dropping non-mnauth connection to %s, service=%s\n", _func_, proRegTxHash.ToString(), addr2.ToStringAddrPort()); - pnode->fDisconnect = true; - return true; - } - return false; - }); + // Use FindNodeLocked since we already hold m_nodes_mutex + CNode* pnode = FindNodeLocked(addr2); + if (pnode && pnode->nTimeFirstMessageReceived.load() != 0s && GetTime() - pnode->nTimeFirstMessageReceived.load() > 5s) { + // clearly not expecting mnauth to take that long even if it wasn't the first message + // we received (as it should normally), disconnect + LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- dropping non-mnauth connection to %s, service=%s\n", _func_, proRegTxHash.ToString(), addr2.ToStringAddrPort()); + pnode->fDisconnect = true; + } // either way - it's not ready, skip it for now continue; } - if (!connectedNodes.count(addr2) && !IsMasternodeOrDisconnectRequested(addr2) && !connectedProRegTxHashes.count(proRegTxHash)) { + // Check if node is masternode or disconnect requested using FindNodeLocked + CNode* existingNode = FindNodeLocked(addr2); + bool isDisconnectRequested = existingNode && (existingNode->m_masternode_connection || existingNode->fDisconnect); + if (!connectedNodes.count(addr2) && !isDisconnectRequested && !connectedProRegTxHashes.count(proRegTxHash)) { int64_t lastAttempt = mn_metaman.GetMetaInfo(dmn->proTxHash)->GetLastOutboundAttempt(); // back off trying connecting to an address if we already tried recently if (nANow - lastAttempt < chainParams.LLMQConnectionRetryTimeout()) { @@ -3454,7 +3474,8 @@ void CConnman::ThreadOpenMasternodeConnections(CDeterministicMNManager& dmnman, return ret; }; - const auto getPendingProbes = [&]() EXCLUSIVE_LOCKS_REQUIRED(cs_vPendingMasternodes) { + const auto getPendingProbes = [&]() EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex, cs_vPendingMasternodes) { + AssertLockHeld(m_nodes_mutex); AssertLockHeld(cs_vPendingMasternodes); std::vector ret; for (auto it = masternodePendingProbes.begin(); it != masternodePendingProbes.end(); ) { @@ -3490,7 +3511,14 @@ void CConnman::ThreadOpenMasternodeConnections(CDeterministicMNManager& dmnman, if (!vPendingMasternodes.empty()) { auto dmn = mnList.GetValidMN(vPendingMasternodes.front()); vPendingMasternodes.erase(vPendingMasternodes.begin()); - if (dmn && !connectedNodes.count(dmn->pdmnState->netInfo->GetPrimary()) && !IsMasternodeOrDisconnectRequested(dmn->pdmnState->netInfo->GetPrimary())) { + // Check if we should connect to this masternode + // We already hold m_nodes_mutex here, so check fDisconnect and m_masternode_connection directly + bool shouldConnect = true; + if (dmn && !connectedNodes.count(dmn->pdmnState->netInfo->GetPrimary())) { + CNode* pnode = FindNodeLocked(dmn->pdmnState->netInfo->GetPrimary()); + shouldConnect = !pnode || (!pnode->m_masternode_connection && !pnode->fDisconnect); + } + if (dmn && shouldConnect) { LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- opening pending masternode connection to %s, service=%s\n", _func_, dmn->proTxHash.ToString(), dmn->pdmnState->netInfo->GetPrimary().ToStringAddrPort()); return dmn; } @@ -4485,7 +4513,7 @@ void CConnman::GetNodeStats(std::vector& vstats) const bool CConnman::DisconnectNode(const std::string& strNode) { LOCK(m_nodes_mutex); - if (CNode* pnode = FindNode(strNode)) { + if (CNode* pnode = FindNodeLocked(strNode)) { LogPrint(BCLog::NET_NETCONN, "disconnect by address%s matched peer=%d; disconnecting\n", (fLogIPs ? strprintf("=%s", strNode) : ""), pnode->GetId()); pnode->fDisconnect = true; return true; diff --git a/src/net.h b/src/net.h index 9e0fb5d8b5b85..d3b1ae0b86ef8 100644 --- a/src/net.h +++ b/src/net.h @@ -1244,8 +1244,8 @@ friend class CNode; EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !m_added_nodes_mutex, !m_addr_fetches_mutex, !mutexMsgProc); void StopThreads(); - void StopNodes() EXCLUSIVE_LOCKS_REQUIRED(!cs_mapSocketToNode, !cs_sendable_receivable_nodes); - void Stop() EXCLUSIVE_LOCKS_REQUIRED(!cs_mapSocketToNode, !cs_sendable_receivable_nodes) + void StopNodes() EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !cs_mapSocketToNode, !cs_sendable_receivable_nodes); + void Stop() EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !cs_mapSocketToNode, !cs_sendable_receivable_nodes) { StopThreads(); StopNodes(); @@ -1274,13 +1274,13 @@ friend class CNode; const char* strDest, ConnectionType conn_type, bool use_v2transport, MasternodeConn masternode_connection = MasternodeConn::IsNotConnection, MasternodeProbeConn masternode_probe_connection = MasternodeProbeConn::IsNotConnection) - EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex, !mutexMsgProc, !cs_mapSocketToNode); + EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_unused_i2p_sessions_mutex, !mutexMsgProc, !cs_mapSocketToNode); void OpenMasternodeConnection(const CAddress& addrConnect, bool use_v2transport, MasternodeProbeConn probe = MasternodeProbeConn::IsConnection) - EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex, !mutexMsgProc, !cs_mapSocketToNode); - bool CheckIncomingNonce(uint64_t nonce); + EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_unused_i2p_sessions_mutex, !mutexMsgProc, !cs_mapSocketToNode); + bool CheckIncomingNonce(uint64_t nonce) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); // alias for thread safety annotations only, not defined - RecursiveMutex& GetNodesMutex() const LOCK_RETURNED(m_nodes_mutex); + Mutex& GetNodesMutex() const LOCK_RETURNED(m_nodes_mutex); struct CFullyConnectedOnly { bool operator() (const CNode* pnode) const { @@ -1296,37 +1296,37 @@ friend class CNode; constexpr static const CAllNodes AllNodes{}; - bool ForNode(NodeId id, std::function cond, std::function func); - bool ForNode(const CService& addr, std::function cond, std::function func); + bool ForNode(NodeId id, std::function cond, std::function func) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + bool ForNode(const CService& addr, std::function cond, std::function func) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); template - bool ForNode(const CService& addr, Callable&& func) + bool ForNode(const CService& addr, Callable&& func) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { return ForNode(addr, FullyConnectedOnly, func); } template - bool ForNode(NodeId id, Callable&& func) + bool ForNode(NodeId id, Callable&& func) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { return ForNode(id, FullyConnectedOnly, func); } using NodeFn = std::function; - bool IsConnected(const CService& addr, std::function cond) + bool IsConnected(const CService& addr, std::function cond) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { return ForNode(addr, cond, [](CNode* pnode){ return true; }); } - bool IsMasternodeOrDisconnectRequested(const CService& addr); + bool IsMasternodeOrDisconnectRequested(const CService& addr) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); void PushMessage(CNode* pnode, CSerializedNetMsg&& msg) EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc, !m_total_bytes_sent_mutex); template - bool ForEachNodeContinueIf(const Condition& cond, Callable&& func) + bool ForEachNodeContinueIf(const Condition& cond, Callable&& func) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { LOCK(m_nodes_mutex); for (auto&& node : m_nodes) @@ -1337,13 +1337,13 @@ friend class CNode; }; template - bool ForEachNodeContinueIf(Callable&& func) + bool ForEachNodeContinueIf(Callable&& func) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { return ForEachNodeContinueIf(FullyConnectedOnly, func); } template - bool ForEachNodeContinueIf(const Condition& cond, Callable&& func) const + bool ForEachNodeContinueIf(const Condition& cond, Callable&& func) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { LOCK(m_nodes_mutex); for (const auto& node : m_nodes) @@ -1354,13 +1354,13 @@ friend class CNode; }; template - bool ForEachNodeContinueIf(Callable&& func) const + bool ForEachNodeContinueIf(Callable&& func) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { return ForEachNodeContinueIf(FullyConnectedOnly, func); } template - void ForEachNode(const Condition& cond, Callable&& func) + void ForEachNode(const Condition& cond, Callable&& func) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { LOCK(m_nodes_mutex); for (auto&& node : m_nodes) { @@ -1369,13 +1369,13 @@ friend class CNode; } }; - void ForEachNode(const NodeFn& fn) + void ForEachNode(const NodeFn& fn) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { ForEachNode(FullyConnectedOnly, fn); } template - void ForEachNode(const Condition& cond, Callable&& func) const + void ForEachNode(const Condition& cond, Callable&& func) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { LOCK(m_nodes_mutex); for (auto&& node : m_nodes) { @@ -1384,13 +1384,13 @@ friend class CNode; } }; - void ForEachNode(const NodeFn& fn) const + void ForEachNode(const NodeFn& fn) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { ForEachNode(FullyConnectedOnly, fn); } template - void ForEachNodeThen(const Condition& cond, Callable&& pre, CallableAfter&& post) + void ForEachNodeThen(const Condition& cond, Callable&& pre, CallableAfter&& post) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { LOCK(m_nodes_mutex); for (auto&& node : m_nodes) { @@ -1401,13 +1401,13 @@ friend class CNode; }; template - void ForEachNodeThen(Callable&& pre, CallableAfter&& post) + void ForEachNodeThen(Callable&& pre, CallableAfter&& post) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { ForEachNodeThen(FullyConnectedOnly, pre, post); } template - void ForEachNodeThen(const Condition& cond, Callable&& pre, CallableAfter&& post) const + void ForEachNodeThen(const Condition& cond, Callable&& pre, CallableAfter&& post) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { LOCK(m_nodes_mutex); for (auto&& node : m_nodes) { @@ -1418,7 +1418,7 @@ friend class CNode; }; template - void ForEachNodeThen(Callable&& pre, CallableAfter&& post) const + void ForEachNodeThen(Callable&& pre, CallableAfter&& post) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { ForEachNodeThen(FullyConnectedOnly, pre, post); } @@ -1454,14 +1454,15 @@ friend class CNode; // return a value less than (num_outbound_connections - num_outbound_slots) // in cases where some outbound connections are not yet fully connected, or // not yet fully disconnected. - int GetExtraFullOutboundCount() const; + int GetExtraFullOutboundCount() const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); // Count the number of block-relay-only peers we have over our limit. - int GetExtraBlockRelayCount() const; + int GetExtraBlockRelayCount() const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); bool AddNode(const AddedNodeParams& add) EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex); bool RemoveAddedNode(const std::string& node) EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex); bool AddedNodesContain(const CAddress& addr) const EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex); - std::vector GetAddedNodeInfo(bool include_connected) const EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex); + std::vector GetAddedNodeInfo(bool include_connected) const + EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex, !m_nodes_mutex); /** * Attempts to open a connection. Currently only used from tests. @@ -1477,29 +1478,29 @@ friend class CNode; * - Max connection capacity for type is filled */ bool AddConnection(const std::string& address, ConnectionType conn_type, bool use_v2transport) - EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex, !mutexMsgProc, !cs_mapSocketToNode); + EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_unused_i2p_sessions_mutex, !mutexMsgProc, !cs_mapSocketToNode); bool AddPendingMasternode(const uint256& proTxHash); void SetMasternodeQuorumNodes(Consensus::LLMQType llmqType, const uint256& quorumHash, const Uint256HashSet& proTxHashes); - void SetMasternodeQuorumRelayMembers(Consensus::LLMQType llmqType, const uint256& quorumHash, const Uint256HashSet& proTxHashes); + void SetMasternodeQuorumRelayMembers(Consensus::LLMQType llmqType, const uint256& quorumHash, const Uint256HashSet& proTxHashes) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); bool HasMasternodeQuorumNodes(Consensus::LLMQType llmqType, const uint256& quorumHash) const; Uint256HashSet GetMasternodeQuorums(Consensus::LLMQType llmqType) const; // also returns QWATCH nodes - std::vector GetMasternodeQuorumNodes(Consensus::LLMQType llmqType, const uint256& quorumHash) const; + std::vector GetMasternodeQuorumNodes(Consensus::LLMQType llmqType, const uint256& quorumHash) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); void RemoveMasternodeQuorumNodes(Consensus::LLMQType llmqType, const uint256& quorumHash); bool IsMasternodeQuorumNode(const CNode* pnode, const CDeterministicMNList& tip_mn_list) const; bool IsMasternodeQuorumRelayMember(const uint256& protxHash); void AddPendingProbeConnections(const std::set& proTxHashes); - size_t GetNodeCount(ConnectionDirection) const; + size_t GetNodeCount(ConnectionDirection) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); std::map getNetLocalAddresses() const; size_t GetMaxOutboundNodeCount(); size_t GetMaxOutboundOnionNodeCount(); - void GetNodeStats(std::vector& vstats) const; - bool DisconnectNode(const std::string& node); - bool DisconnectNode(const CSubNet& subnet); - bool DisconnectNode(const CNetAddr& addr); - bool DisconnectNode(NodeId id); + void GetNodeStats(std::vector& vstats) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + bool DisconnectNode(const std::string& node) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + bool DisconnectNode(const CSubNet& subnet) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + bool DisconnectNode(const CNetAddr& addr) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + bool DisconnectNode(NodeId id) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); //! Used to convey which local services we are offering peers during node //! connection. @@ -1544,7 +1545,8 @@ friend class CNode; { public: explicit NodesSnapshot(const CConnman& connman, std::function cond = AllNodes, - bool shuffle = false); + bool shuffle = false) + EXCLUSIVE_LOCKS_REQUIRED(!connman.m_nodes_mutex); ~NodesSnapshot(); const std::vector& Nodes() const @@ -1579,16 +1581,19 @@ friend class CNode; bool InitBinds(const Options& options); void ThreadOpenAddedConnections() - EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex, !m_unused_i2p_sessions_mutex, !m_reconnections_mutex, !mutexMsgProc, !cs_mapSocketToNode); + EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex, !m_nodes_mutex, !m_reconnections_mutex, + !m_unused_i2p_sessions_mutex, !mutexMsgProc, !cs_mapSocketToNode); void AddAddrFetch(const std::string& strDest) EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex); void ProcessAddrFetch() - EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_unused_i2p_sessions_mutex, !mutexMsgProc, !cs_mapSocketToNode); + EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_nodes_mutex, !m_unused_i2p_sessions_mutex, + !mutexMsgProc, !cs_mapSocketToNode); void ThreadOpenConnections(const std::vector connect, CDeterministicMNManager& dmnman) - EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_added_nodes_mutex, !m_nodes_mutex, !m_unused_i2p_sessions_mutex, !m_reconnections_mutex, !mutexMsgProc, !cs_mapSocketToNode); - void ThreadMessageHandler() EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc); - void ThreadI2PAcceptIncoming(CMasternodeSync& mn_sync) EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc, !cs_mapSocketToNode); + EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_added_nodes_mutex, !m_nodes_mutex, !m_reconnections_mutex, + !m_unused_i2p_sessions_mutex, !mutexMsgProc, !cs_mapSocketToNode); + void ThreadMessageHandler() EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !mutexMsgProc); + void ThreadI2PAcceptIncoming(CMasternodeSync& mn_sync) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !mutexMsgProc, !cs_mapSocketToNode); void AcceptConnection(const ListenSocket& hListenSocket, CMasternodeSync& mn_sync) - EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc, !cs_mapSocketToNode); + EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !mutexMsgProc, !cs_mapSocketToNode); /** * Create a `CNode` object from a socket that has just been accepted and add the node to @@ -1602,11 +1607,12 @@ friend class CNode; NetPermissionFlags permission_flags, const CAddress& addr_bind, const CAddress& addr, - CMasternodeSync& mn_sync) EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc, !cs_mapSocketToNode); + CMasternodeSync& mn_sync) + EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !mutexMsgProc, !cs_mapSocketToNode); void DisconnectNodes() EXCLUSIVE_LOCKS_REQUIRED(!m_reconnections_mutex, !m_nodes_mutex); - void NotifyNumConnectionsChanged(CMasternodeSync& mn_sync); - void CalculateNumConnectionsChangedStats(); + void NotifyNumConnectionsChanged(CMasternodeSync& mn_sync) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + void CalculateNumConnectionsChangedStats() EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); /** Return true if the peer is inactive and should be disconnected. */ bool InactivityCheck(const CNode& node) const; @@ -1620,21 +1626,21 @@ friend class CNode; /** * Check connected and listening sockets for IO readiness and process them accordingly. */ - void SocketHandler(CMasternodeSync& mn_sync) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc, !cs_mapSocketToNode, !cs_sendable_receivable_nodes); + void SocketHandler(CMasternodeSync& mn_sync) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_total_bytes_sent_mutex, !mutexMsgProc, !cs_mapSocketToNode, !cs_sendable_receivable_nodes); /** * Do the read/write for connected sockets that are ready for IO. * @param[in] events_per_sock Sockets that are ready for IO. */ void SocketHandlerConnected(const Sock::EventsPerSock& events_per_sock) - EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc, !cs_sendable_receivable_nodes, !cs_mapSocketToNode, !cs_sendable_receivable_nodes); + EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_total_bytes_sent_mutex, !mutexMsgProc, !cs_sendable_receivable_nodes, !cs_mapSocketToNode, !cs_sendable_receivable_nodes); /** * Accept incoming connections, one from each read-ready listening socket. * @param[in] events_per_sock Sockets that are ready for IO. */ void SocketHandlerListening(const Sock::EventsPerSock& events_per_sock, CMasternodeSync& mn_sync) - EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc, !cs_mapSocketToNode); + EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !mutexMsgProc, !cs_mapSocketToNode); void ThreadSocketHandler(CMasternodeSync& mn_sync) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc, !m_nodes_mutex, !m_reconnections_mutex, !cs_mapSocketToNode, !cs_sendable_receivable_nodes); @@ -1645,18 +1651,23 @@ friend class CNode; uint64_t CalculateKeyedNetGroup(const CAddress& ad) const; - CNode* FindNode(const CNetAddr& ip, bool fExcludeDisconnecting = true); - CNode* FindNode(const std::string& addrName, bool fExcludeDisconnecting = true); - CNode* FindNode(const CService& addr, bool fExcludeDisconnecting = true); + CNode* FindNode(const CNetAddr& ip, bool fExcludeDisconnecting = true) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + CNode* FindNode(const std::string& addrName, bool fExcludeDisconnecting = true) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + CNode* FindNode(const CService& addr, bool fExcludeDisconnecting = true) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + + // Internal versions that require the lock to be held + CNode* FindNodeLocked(const CNetAddr& ip, bool fExcludeDisconnecting = true) EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex); + CNode* FindNodeLocked(const std::string& addrName, bool fExcludeDisconnecting = true) EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex); + CNode* FindNodeLocked(const CService& addr, bool fExcludeDisconnecting = true) EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex); /** * Determine whether we're already connected to a given address, in order to * avoid initiating duplicate connections. */ - bool AlreadyConnectedToAddress(const CAddress& addr); + bool AlreadyConnectedToAddress(const CAddress& addr) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); - bool AttemptToEvictConnection(); - CNode* ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type, bool use_v2transport) EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex); + bool AttemptToEvictConnection() EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + CNode* ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type, bool use_v2transport) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_unused_i2p_sessions_mutex); void AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNetAddr &addr) const; void DeleteNode(CNode* pnode); @@ -1666,7 +1677,7 @@ friend class CNode; /** (Try to) send data from node's vSendMsg. Returns (bytes_sent, data_left). */ std::pair SocketSendData(CNode& node) const EXCLUSIVE_LOCKS_REQUIRED(node.cs_vSend); - size_t SocketRecvData(CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc); + size_t SocketRecvData(CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !mutexMsgProc); void DumpAddresses(); @@ -1683,7 +1694,7 @@ friend class CNode; /** * Return vector of current BLOCK_RELAY peers. */ - std::vector GetCurrentBlockRelayOnlyConns() const; + std::vector GetCurrentBlockRelayOnlyConns() const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); /** * Search for a "preferred" network, a reachable network to which we @@ -1695,7 +1706,7 @@ friend class CNode; * * @return bool Whether a preferred network was found. */ - bool MaybePickPreferredNetwork(std::optional& network); + bool MaybePickPreferredNetwork(std::optional& network) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); // Whether the node should be passed out in ForEach* callbacks static bool NodeFullyConnected(const CNode* pnode); @@ -1735,7 +1746,7 @@ friend class CNode; mutable Mutex m_added_nodes_mutex; std::vector m_nodes GUARDED_BY(m_nodes_mutex); std::list m_nodes_disconnected; - mutable RecursiveMutex m_nodes_mutex; + mutable Mutex m_nodes_mutex; std::atomic nLastNodeId{0}; unsigned int nPrevNodeCount{0}; @@ -1934,7 +1945,7 @@ friend class CNode; std::list m_reconnections GUARDED_BY(m_reconnections_mutex); /** Attempt reconnections, if m_reconnections non-empty. */ - void PerformReconnections() EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc, !m_reconnections_mutex, !m_unused_i2p_sessions_mutex, !cs_mapSocketToNode); + void PerformReconnections() EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !mutexMsgProc, !m_reconnections_mutex, !m_unused_i2p_sessions_mutex, !cs_mapSocketToNode); /** * Cap on the size of `m_unused_i2p_sessions`, to ensure it does not diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 63d5aac9657b0..ea3800bdaab1a 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1299,23 +1299,23 @@ void PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid) } m_connman.ForNode(nodeid, [this](CNode* pfrom) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { AssertLockHeld(::cs_main); - if (lNodesAnnouncingHeaderAndIDs.size() >= 3) { - // As per BIP152, we only get 3 of our peers to announce - // blocks using compact encodings. - m_connman.ForNode(lNodesAnnouncingHeaderAndIDs.front(), [this](CNode* pnodeStop){ - m_connman.PushMessage(pnodeStop, CNetMsgMaker(pnodeStop->GetCommonVersion()).Make(NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION)); - // save BIP152 bandwidth state: we select peer to be low-bandwidth - pnodeStop->m_bip152_highbandwidth_to = false; - return true; - }); - lNodesAnnouncingHeaderAndIDs.pop_front(); - } m_connman.PushMessage(pfrom, CNetMsgMaker(pfrom->GetCommonVersion()).Make(NetMsgType::SENDCMPCT, /*high_bandwidth=*/true, /*version=*/CMPCTBLOCKS_VERSION)); // save BIP152 bandwidth state: we select peer to be high-bandwidth pfrom->m_bip152_highbandwidth_to = true; lNodesAnnouncingHeaderAndIDs.push_back(pfrom->GetId()); return true; }); + if (lNodesAnnouncingHeaderAndIDs.size() > 3) { + // As per BIP152, we only get 3 of our peers to announce + // blocks using compact encodings. + m_connman.ForNode(lNodesAnnouncingHeaderAndIDs.front(), [this](CNode* pnodeStop){ + m_connman.PushMessage(pnodeStop, CNetMsgMaker(pnodeStop->GetCommonVersion()).Make(NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION)); + // save BIP152 bandwidth state: we select peer to be low-bandwidth + pnodeStop->m_bip152_highbandwidth_to = false; + return true; + }); + lNodesAnnouncingHeaderAndIDs.pop_front(); + } } bool PeerManagerImpl::TipMayBeStale() From 63cf92d88ec56f1eaa52a3629da9288246ac3291 Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 21 Oct 2025 22:10:25 -0500 Subject: [PATCH 2/6] perf: convert m_nodes_mutex from mutex to shared mutex --- src/net.cpp | 289 ++++++++++++++++++++++++++++++---------------------- src/net.h | 103 +++++++++++++++---- src/sync.h | 5 + 3 files changed, 255 insertions(+), 142 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 3017fa2aeb50d..0ce860e45c4e4 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -338,10 +338,10 @@ bool IsLocal(const CService& addr) return mapLocalHost.count(addr) > 0; } -CNode* CConnman::FindNodeLocked(const CNetAddr& ip, bool fExcludeDisconnecting) +// Internal FindNodeLocked - const versions do the actual work +const CNode* CConnman::FindNodeLocked(const CNetAddr& ip, bool fExcludeDisconnecting) const { - AssertLockHeld(m_nodes_mutex); - for (CNode* pnode : m_nodes) { + for (const CNode* pnode : m_nodes) { if (fExcludeDisconnecting && pnode->fDisconnect) { continue; } @@ -352,10 +352,9 @@ CNode* CConnman::FindNodeLocked(const CNetAddr& ip, bool fExcludeDisconnecting) return nullptr; } -CNode* CConnman::FindNodeLocked(const std::string& addrName, bool fExcludeDisconnecting) +const CNode* CConnman::FindNodeLocked(const std::string& addrName, bool fExcludeDisconnecting) const { - AssertLockHeld(m_nodes_mutex); - for (CNode* pnode : m_nodes) { + for (const CNode* pnode : m_nodes) { if (fExcludeDisconnecting && pnode->fDisconnect) { continue; } @@ -366,10 +365,9 @@ CNode* CConnman::FindNodeLocked(const std::string& addrName, bool fExcludeDiscon return nullptr; } -CNode* CConnman::FindNodeLocked(const CService& addr, bool fExcludeDisconnecting) +const CNode* CConnman::FindNodeLocked(const CService& addr, bool fExcludeDisconnecting) const { - AssertLockHeld(m_nodes_mutex); - for (CNode* pnode : m_nodes) { + for (const CNode* pnode : m_nodes) { if (fExcludeDisconnecting && pnode->fDisconnect) { continue; } @@ -380,32 +378,53 @@ CNode* CConnman::FindNodeLocked(const CService& addr, bool fExcludeDisconnecting return nullptr; } -CNode* CConnman::FindNode(const CNetAddr& ip, bool fExcludeDisconnecting) +const CNode* CConnman::FindNodeLocked(NodeId id, bool fExcludeDisconnecting) const { - LOCK(m_nodes_mutex); - return FindNodeLocked(ip, fExcludeDisconnecting); + for (const CNode* pnode : m_nodes) { + if (fExcludeDisconnecting && pnode->fDisconnect) { + continue; + } + if (pnode->GetId() == id) { + return pnode; + } + } + return nullptr; } -CNode* CConnman::FindNode(const std::string& addrName, bool fExcludeDisconnecting) +CNode* CConnman::FindNodeLockedMutable(const CNetAddr& ip, bool fExcludeDisconnecting) { - LOCK(m_nodes_mutex); - return FindNodeLocked(addrName, fExcludeDisconnecting); + AssertLockHeld(m_nodes_mutex); + return const_cast(FindNodeLocked(ip, fExcludeDisconnecting)); } -CNode* CConnman::FindNode(const CService& addr, bool fExcludeDisconnecting) +CNode* CConnman::FindNodeLockedMutable(const std::string& addrName, bool fExcludeDisconnecting) { - LOCK(m_nodes_mutex); - return FindNodeLocked(addr, fExcludeDisconnecting); + AssertLockHeld(m_nodes_mutex); + return const_cast(FindNodeLocked(addrName, fExcludeDisconnecting)); } -bool CConnman::AlreadyConnectedToAddress(const CAddress& addr) +CNode* CConnman::FindNodeLockedMutable(const CService& addr, bool fExcludeDisconnecting) { - return FindNode(addr.ToStringAddrPort()); + AssertLockHeld(m_nodes_mutex); + return const_cast(FindNodeLocked(addr, fExcludeDisconnecting)); } -bool CConnman::CheckIncomingNonce(uint64_t nonce) +CNode* CConnman::FindNodeLockedMutable(NodeId id, bool fExcludeDisconnecting) { - LOCK(m_nodes_mutex); + AssertLockHeld(m_nodes_mutex); + return const_cast(FindNodeLocked(id, fExcludeDisconnecting)); +} + + +bool CConnman::AlreadyConnectedToAddress(const CAddress& addr) const +{ + READ_LOCK(m_nodes_mutex); + return FindNodeLocked(addr.ToStringAddrPort()) != nullptr; +} + +bool CConnman::CheckIncomingNonce(uint64_t nonce) const +{ + READ_LOCK(m_nodes_mutex); for (const CNode* pnode : m_nodes) { if (!pnode->fSuccessfullyConnected && !pnode->IsInboundConn() && pnode->GetLocalNonce() == nonce) return false; @@ -438,8 +457,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo } // Look for an existing connection - CNode* pnode = FindNode(static_cast(addrConnect)); - if (pnode) + if (WithNodeShared(static_cast(addrConnect), [](const CNode*){ return true; })) { LogPrintf("Failed to open new connection, already connected\n"); return nullptr; @@ -475,9 +493,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo } // It is possible that we already have a connection to the IP/port pszDest resolved to. // In that case, drop the connection that was just created. - LOCK(m_nodes_mutex); - CNode* pnode = FindNodeLocked(static_cast(addrConnect)); - if (pnode) { + if (WithNodeExclusive(static_cast(addrConnect), [&](CNode* /*pnode*/){ return true; })) { LogPrintf("Not opening a connection to %s, already connected to %s\n", pszDest, addrConnect.ToStringAddrPort()); return nullptr; } @@ -655,7 +671,7 @@ bool CNode::IsConnectedThroughPrivacyNet() const #undef X #define X(name) stats.name = name -void CNode::CopyStats(CNodeStats& stats) +void CNode::CopyStats(CNodeStats& stats) const { stats.nodeid = this->GetId(); X(addr); @@ -1801,6 +1817,56 @@ std::pair CConnman::SocketSendData(CNode& node) const return {nSentSize, data_left}; } +std::vector CConnman::GetEvictionCandidates() const +{ + std::vector vEvictionCandidates; + READ_LOCK(m_nodes_mutex); + + for (const CNode* node : m_nodes) { + if (node->fDisconnect) + continue; + + if (m_active_masternode) { + // This handles eviction protected nodes. Nodes are always protected for a short time after the connection + // was accepted. This short time is meant for the VERSION/VERACK exchange and the possible MNAUTH that might + // follow when the incoming connection is from another masternode. When a message other than MNAUTH + // is received after VERSION/VERACK, the protection is lifted immediately. + bool isProtected = GetTime() - node->m_connected < INBOUND_EVICTION_PROTECTION_TIME; + if (node->nTimeFirstMessageReceived.load() != 0s && !node->fFirstMessageIsMNAUTH) { + isProtected = false; + } + // if MNAUTH was valid, the node is always protected (and at the same time not accounted when + // checking incoming connection limits) + if (!node->GetVerifiedProRegTxHash().IsNull()) { + isProtected = true; + } + if (isProtected) { + continue; + } + } + + NodeEvictionCandidate candidate{ + .id = node->GetId(), + .m_connected = node->m_connected, + .m_min_ping_time = node->m_min_ping_time, + .m_last_block_time = node->m_last_block_time, + .m_last_tx_time = node->m_last_tx_time, + .fRelevantServices = node->m_has_all_wanted_services, + .m_relay_txs = node->m_relays_txs.load(), + .fBloomFilter = node->m_bloom_filter_loaded.load(), + .nKeyedNetGroup = node->nKeyedNetGroup, + .prefer_evict = node->m_prefer_evict, + .m_is_local = node->addr.IsLocal(), + .m_network = node->ConnectedThroughNetwork(), + .m_noban = node->HasPermission(NetPermissionFlags::NoBan), + .m_conn_type = node->m_conn_type, + }; + vEvictionCandidates.push_back(candidate); + } + + return vEvictionCandidates; +} + /** Try to find a connection to evict when the node is full. * Extreme care must be taken to avoid opening the node to attacker * triggered network partitioning. @@ -1811,65 +1877,16 @@ std::pair CConnman::SocketSendData(CNode& node) const */ bool CConnman::AttemptToEvictConnection() { - std::vector vEvictionCandidates; - { - LOCK(m_nodes_mutex); - - for (const CNode* node : m_nodes) { - if (node->fDisconnect) - continue; - - if (m_active_masternode) { - // This handles eviction protected nodes. Nodes are always protected for a short time after the connection - // was accepted. This short time is meant for the VERSION/VERACK exchange and the possible MNAUTH that might - // follow when the incoming connection is from another masternode. When a message other than MNAUTH - // is received after VERSION/VERACK, the protection is lifted immediately. - bool isProtected = GetTime() - node->m_connected < INBOUND_EVICTION_PROTECTION_TIME; - if (node->nTimeFirstMessageReceived.load() != 0s && !node->fFirstMessageIsMNAUTH) { - isProtected = false; - } - // if MNAUTH was valid, the node is always protected (and at the same time not accounted when - // checking incoming connection limits) - if (!node->GetVerifiedProRegTxHash().IsNull()) { - isProtected = true; - } - if (isProtected) { - continue; - } - } - - NodeEvictionCandidate candidate{ - .id = node->GetId(), - .m_connected = node->m_connected, - .m_min_ping_time = node->m_min_ping_time, - .m_last_block_time = node->m_last_block_time, - .m_last_tx_time = node->m_last_tx_time, - .fRelevantServices = node->m_has_all_wanted_services, - .m_relay_txs = node->m_relays_txs.load(), - .fBloomFilter = node->m_bloom_filter_loaded.load(), - .nKeyedNetGroup = node->nKeyedNetGroup, - .prefer_evict = node->m_prefer_evict, - .m_is_local = node->addr.IsLocal(), - .m_network = node->ConnectedThroughNetwork(), - .m_noban = node->HasPermission(NetPermissionFlags::NoBan), - .m_conn_type = node->m_conn_type, - }; - vEvictionCandidates.push_back(candidate); - } - } + std::vector vEvictionCandidates = GetEvictionCandidates(); const std::optional node_id_to_evict = SelectNodeToEvict(std::move(vEvictionCandidates)); if (!node_id_to_evict) { return false; } - LOCK(m_nodes_mutex); - for (CNode* pnode : m_nodes) { - if (pnode->GetId() == *node_id_to_evict) { - LogPrint(BCLog::NET_NETCONN, "selected %s connection for eviction peer=%d; disconnecting\n", pnode->ConnectionTypeAsString(), pnode->GetId()); - pnode->fDisconnect = true; - return true; - } - } - return false; + return WithNodeExclusive(*node_id_to_evict, [](CNode* pnode){ + LogPrint(BCLog::NET_NETCONN, "selected %s connection for eviction peer=%d; disconnecting\n", pnode->ConnectionTypeAsString(), pnode->GetId()); + pnode->fDisconnect = true; + return true; + }).value_or(false); } void CConnman::AcceptConnection(const ListenSocket& hListenSocket, CMasternodeSync& mn_sync) { @@ -1919,8 +1936,9 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, NetPermissions::AddFlag(permission_flags, NetPermissionFlags::NoBan); } + { - LOCK(m_nodes_mutex); + READ_LOCK(m_nodes_mutex); for (const CNode* pnode : m_nodes) { if (pnode->IsInboundConn()) { nInbound++; @@ -1929,7 +1947,6 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, } } } - } std::string strDropped; @@ -2078,7 +2095,7 @@ bool CConnman::AddConnection(const std::string& address, ConnectionType conn_typ } // no default case, so the compiler can warn about missing cases // Count existing connections - int existing_connections = WITH_LOCK(m_nodes_mutex, + int existing_connections = WITH_READ_LOCK(m_nodes_mutex, return std::count_if(m_nodes.begin(), m_nodes.end(), [conn_type](CNode* node) { return node->m_conn_type == conn_type; });); // Max connections of specified type already exist @@ -2219,11 +2236,7 @@ void CConnman::DisconnectNodes() void CConnman::NotifyNumConnectionsChanged(CMasternodeSync& mn_sync) { - size_t nodes_size; - { - LOCK(m_nodes_mutex); - nodes_size = m_nodes.size(); - } + size_t nodes_size = WITH_READ_LOCK(m_nodes_mutex, return m_nodes.size();); // If we had zero connections before and new connections now or if we just dropped // to zero connections reset the sync process if its outdated. @@ -2391,7 +2404,8 @@ void CConnman::SocketHandler(CMasternodeSync& mn_sync) bool only_poll = [this]() { // Check if we have work to do and thus should avoid waiting for events - LOCK2(m_nodes_mutex, cs_sendable_receivable_nodes); + READ_LOCK(m_nodes_mutex); // We acquire this to avoid the pointers stored in mapSendableNodes and mapReceivableNodes being invalidated by ThreadSocketHandler + LOCK(cs_sendable_receivable_nodes); if (!mapReceivableNodes.empty()) { return true; } @@ -2721,7 +2735,7 @@ void CConnman::ThreadDNSAddressSeed() int nRelevant = 0; { - LOCK(m_nodes_mutex); + READ_LOCK(m_nodes_mutex); for (const CNode* pnode : m_nodes) { if (pnode->fSuccessfullyConnected && !pnode->IsFullOutboundConn() && !pnode->m_masternode_probe_connection) ++nRelevant; } @@ -2846,7 +2860,7 @@ int CConnman::GetExtraFullOutboundCount() const { int full_outbound_peers = 0; { - LOCK(m_nodes_mutex); + READ_LOCK(m_nodes_mutex); for (const CNode* pnode : m_nodes) { // don't count outbound masternodes if (pnode->m_masternode_connection) { @@ -2864,7 +2878,7 @@ int CConnman::GetExtraBlockRelayCount() const { int block_relay_peers = 0; { - LOCK(m_nodes_mutex); + READ_LOCK(m_nodes_mutex); for (const CNode* pnode : m_nodes) { if (pnode->fSuccessfullyConnected && !pnode->fDisconnect && pnode->IsBlockOnlyConn()) { ++block_relay_peers; @@ -2889,7 +2903,7 @@ std::unordered_set CConnman::GetReachableEmptyNetworks() const bool CConnman::MultipleManualOrFullOutboundConns(Network net) const { - AssertLockHeld(m_nodes_mutex); + AssertSharedLockHeld(m_nodes_mutex); return m_network_conn_counts[net] > 1; } @@ -2898,7 +2912,7 @@ bool CConnman::MaybePickPreferredNetwork(std::optional& network) std::array nets{NET_IPV4, NET_IPV6, NET_ONION, NET_I2P, NET_CJDNS}; Shuffle(nets.begin(), nets.end(), FastRandomContext()); - LOCK(m_nodes_mutex); + READ_LOCK(m_nodes_mutex); for (const auto net : nets) { if (g_reachable_nets.Contains(net) && m_network_conn_counts[net] == 0 && addrman.Size(net) != 0) { network = net; @@ -3021,7 +3035,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe std::set> outbound_ipv46_peer_netgroups; if (!Params().AllowMultipleAddressesFromGroup()) { - LOCK(m_nodes_mutex); + READ_LOCK(m_nodes_mutex); for (const CNode* pnode : m_nodes) { if (pnode->IsFullOutboundConn() && !pnode->m_masternode_connection) nOutboundFullRelay++; if (pnode->IsBlockOnlyConn()) nOutboundBlockRelay++; @@ -3060,8 +3074,8 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe std::set setConnectedMasternodes; { - LOCK(m_nodes_mutex); - for (CNode* pnode : m_nodes) { + READ_LOCK(m_nodes_mutex); + for (const CNode* pnode : m_nodes) { auto verifiedProRegTxHash = pnode->GetVerifiedProRegTxHash(); if (!verifiedProRegTxHash.IsNull()) { setConnectedMasternodes.emplace(verifiedProRegTxHash); @@ -3290,7 +3304,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe std::vector CConnman::GetCurrentBlockRelayOnlyConns() const { std::vector ret; - LOCK(m_nodes_mutex); + READ_LOCK(m_nodes_mutex); for (const CNode* pnode : m_nodes) { if (pnode->IsBlockRelayOnly()) { ret.push_back(pnode->addr); @@ -3316,7 +3330,7 @@ std::vector CConnman::GetAddedNodeInfo(bool include_connected) co std::map mapConnected; std::map> mapConnectedByName; { - LOCK(m_nodes_mutex); + READ_LOCK(m_nodes_mutex); for (const CNode* pnode : m_nodes) { if (pnode->addr.IsValid()) { mapConnected[pnode->addr] = pnode->IsInboundConn(); @@ -3447,19 +3461,22 @@ void CConnman::ThreadOpenMasternodeConnections(CDeterministicMNManager& dmnman, if (connectedNodes.count(addr2) && !connectedProRegTxHashes.count(proRegTxHash)) { // we probably connected to it before it became a masternode // or maybe we are still waiting for mnauth - // Use FindNodeLocked since we already hold m_nodes_mutex - CNode* pnode = FindNodeLocked(addr2); - if (pnode && pnode->nTimeFirstMessageReceived.load() != 0s && GetTime() - pnode->nTimeFirstMessageReceived.load() > 5s) { + // Use shared access since we already hold m_nodes_mutex + bool slowHandshake = false; + if (const CNode* pnode = FindNodeLocked(addr2)) { + slowHandshake = pnode->nTimeFirstMessageReceived.load() != 0s && GetTime() - pnode->nTimeFirstMessageReceived.load() > 5s; + } + if (slowHandshake) { // clearly not expecting mnauth to take that long even if it wasn't the first message // we received (as it should normally), disconnect LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- dropping non-mnauth connection to %s, service=%s\n", _func_, proRegTxHash.ToString(), addr2.ToStringAddrPort()); - pnode->fDisconnect = true; + if (CNode* p2 = FindNodeLockedMutable(addr2)) { p2->fDisconnect = true; } } // either way - it's not ready, skip it for now continue; } // Check if node is masternode or disconnect requested using FindNodeLocked - CNode* existingNode = FindNodeLocked(addr2); + const CNode* existingNode = FindNodeLocked(addr2); bool isDisconnectRequested = existingNode && (existingNode->m_masternode_connection || existingNode->fDisconnect); if (!connectedNodes.count(addr2) && !isDisconnectRequested && !connectedProRegTxHashes.count(proRegTxHash)) { int64_t lastAttempt = mn_metaman.GetMetaInfo(dmn->proTxHash)->GetLastOutboundAttempt(); @@ -3515,8 +3532,11 @@ void CConnman::ThreadOpenMasternodeConnections(CDeterministicMNManager& dmnman, // We already hold m_nodes_mutex here, so check fDisconnect and m_masternode_connection directly bool shouldConnect = true; if (dmn && !connectedNodes.count(dmn->pdmnState->netInfo->GetPrimary())) { - CNode* pnode = FindNodeLocked(dmn->pdmnState->netInfo->GetPrimary()); - shouldConnect = !pnode || (!pnode->m_masternode_connection && !pnode->fDisconnect); + bool anyBad = false; + if (const CNode* pnode = FindNodeLocked(dmn->pdmnState->netInfo->GetPrimary())) { + anyBad = pnode->m_masternode_connection || pnode->fDisconnect; + } + shouldConnect = !anyBad; } if (dmn && shouldConnect) { LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- opening pending masternode connection to %s, service=%s\n", _func_, dmn->proTxHash.ToString(), dmn->pdmnState->netInfo->GetPrimary().ToStringAddrPort()); @@ -3556,7 +3576,7 @@ void CConnman::ThreadOpenMasternodeConnections(CDeterministicMNManager& dmnman, OpenMasternodeConnection(CAddress(connectToDmn->pdmnState->netInfo->GetPrimary(), NODE_NETWORK), /*use_v2transport=*/GetLocalServices() & NODE_P2P_V2, isProbe); // should be in the list now if connection was opened - bool connected = ForNode(connectToDmn->pdmnState->netInfo->GetPrimary(), CConnman::AllNodes, [&](CNode* pnode) { + bool connected = ForNode(connectToDmn->pdmnState->netInfo->GetPrimary(), CConnman::AllNodes, [&](const CNode* pnode) { if (pnode->fDisconnect) { return false; } @@ -3606,7 +3626,7 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai if (addrConnect.GetPort() == GetListenPort() && IsLocal(addrConnect)) { return; } - } else if (FindNode(std::string(pszDest))) + } else if (WithNodeShared(std::string(pszDest), [](const CNode*){ return true; })) return; LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- connecting to %s\n", __func__, getIpStr()); @@ -4378,7 +4398,8 @@ Uint256HashSet CConnman::GetMasternodeQuorums(Consensus::LLMQType llmqType) cons std::vector CConnman::GetMasternodeQuorumNodes(Consensus::LLMQType llmqType, const uint256& quorumHash) const { - LOCK2(m_nodes_mutex, cs_vPendingMasternodes); + READ_LOCK(m_nodes_mutex); + LOCK(cs_vPendingMasternodes); auto it = masternodeQuorumNodes.find(std::make_pair(llmqType, quorumHash)); if (it == masternodeQuorumNodes.end()) { return {}; @@ -4459,7 +4480,7 @@ void CConnman::AddPendingProbeConnections(const std::set &proTxHashes) size_t CConnman::GetNodeCount(ConnectionDirection flags) const { - LOCK(m_nodes_mutex); + READ_LOCK(m_nodes_mutex); int nNum = 0; for (const auto& pnode : m_nodes) { @@ -4498,9 +4519,9 @@ size_t CConnman::GetMaxOutboundOnionNodeCount() void CConnman::GetNodeStats(std::vector& vstats) const { vstats.clear(); - LOCK(m_nodes_mutex); + READ_LOCK(m_nodes_mutex); vstats.reserve(m_nodes.size()); - for (CNode* pnode : m_nodes) { + for (const CNode* pnode : m_nodes) { if (pnode->fDisconnect) { continue; } @@ -4512,13 +4533,11 @@ void CConnman::GetNodeStats(std::vector& vstats) const bool CConnman::DisconnectNode(const std::string& strNode) { - LOCK(m_nodes_mutex); - if (CNode* pnode = FindNodeLocked(strNode)) { + return WithNodeExclusive(strNode, [&](CNode* pnode){ LogPrint(BCLog::NET_NETCONN, "disconnect by address%s matched peer=%d; disconnecting\n", (fLogIPs ? strprintf("=%s", strNode) : ""), pnode->GetId()); pnode->fDisconnect = true; return true; - } - return false; + }).value_or(false); } bool CConnman::DisconnectNode(const CSubNet& subnet) @@ -4798,6 +4817,8 @@ void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg) } } + + bool CConnman::ForNode(const CService& addr, std::function cond, std::function func) { CNode* found = nullptr; @@ -4811,6 +4832,32 @@ bool CConnman::ForNode(const CService& addr, std::function cond, std::function func) const +{ + CNode* found = nullptr; + READ_LOCK(m_nodes_mutex); + for (auto&& pnode : m_nodes) { + if((CService)pnode->addr == addr) { + found = pnode; + break; + } + } + return found != nullptr && cond(found) && func(found); +} + +bool CConnman::ForNode(NodeId id, std::function cond, std::function func) const +{ + CNode* found = nullptr; + READ_LOCK(m_nodes_mutex); + for (auto&& pnode : m_nodes) { + if(pnode->GetId() == id) { + found = pnode; + break; + } + } + return found != nullptr && cond(found) && func(found); +} + bool CConnman::ForNode(NodeId id, std::function cond, std::function func) { CNode* found = nullptr; @@ -4825,7 +4872,7 @@ bool CConnman::ForNode(NodeId id, std::function cond, } bool CConnman::IsMasternodeOrDisconnectRequested(const CService& addr) { - return ForNode(addr, AllNodes, [](CNode* pnode){ + return ForNode(addr, AllNodes, [](const CNode* pnode){ return pnode->m_masternode_connection || pnode->fDisconnect; }); } @@ -4834,7 +4881,7 @@ CConnman::NodesSnapshot::NodesSnapshot(const CConnman& connman, std::function #include #include +#include #include #include @@ -61,6 +62,7 @@ class CMasternodeSync; class CNode; class CScheduler; struct bilingual_str; +struct NodeEvictionCandidate; /** Default for -whitelistrelay. */ static const bool DEFAULT_WHITELISTRELAY = true; @@ -730,9 +732,9 @@ class CNode /** Messages still to be fed to m_transport->SetMessageToSend. */ std::deque vSendMsg GUARDED_BY(cs_vSend); std::atomic nSendMsgSize{0}; - Mutex cs_vSend; + mutable Mutex cs_vSend; Mutex m_sock_mutex; - Mutex cs_vRecv; + mutable Mutex cs_vRecv; uint64_t nRecvBytes GUARDED_BY(cs_vRecv){0}; @@ -755,7 +757,7 @@ class CNode const bool m_inbound_onion; std::atomic nNumWarningsSkipped{0}; std::atomic nVersion{0}; - Mutex m_subver_mutex; + mutable Mutex m_subver_mutex; /** * cleanSubVer is a sanitized string of the user agent byte array we read * from the wire. This cleaned string can safely be logged or displayed. @@ -1027,7 +1029,7 @@ class CNode void CloseSocketDisconnect(CConnman* connman) EXCLUSIVE_LOCKS_REQUIRED(!m_sock_mutex); - void CopyStats(CNodeStats& stats) EXCLUSIVE_LOCKS_REQUIRED(!m_subver_mutex, !m_addr_local_mutex, !cs_vSend, !cs_vRecv, !cs_mnauth); + void CopyStats(CNodeStats& stats) const EXCLUSIVE_LOCKS_REQUIRED(!m_subver_mutex, !m_addr_local_mutex, !cs_vSend, !cs_vRecv, !cs_mnauth); std::string ConnectionTypeAsString() const { return ::ConnectionTypeAsString(m_conn_type); } @@ -1277,10 +1279,10 @@ friend class CNode; EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_unused_i2p_sessions_mutex, !mutexMsgProc, !cs_mapSocketToNode); void OpenMasternodeConnection(const CAddress& addrConnect, bool use_v2transport, MasternodeProbeConn probe = MasternodeProbeConn::IsConnection) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_unused_i2p_sessions_mutex, !mutexMsgProc, !cs_mapSocketToNode); - bool CheckIncomingNonce(uint64_t nonce) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + bool CheckIncomingNonce(uint64_t nonce) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); // alias for thread safety annotations only, not defined - Mutex& GetNodesMutex() const LOCK_RETURNED(m_nodes_mutex); + SharedMutex& GetNodesMutex() const LOCK_RETURNED(m_nodes_mutex); struct CFullyConnectedOnly { bool operator() (const CNode* pnode) const { @@ -1297,7 +1299,9 @@ friend class CNode; constexpr static const CAllNodes AllNodes{}; bool ForNode(NodeId id, std::function cond, std::function func) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); - bool ForNode(const CService& addr, std::function cond, std::function func) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + bool ForNode(NodeId id, std::function cond, std::function func) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + bool ForNode(const CService& addr, std::function cond, std::function func) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + bool ForNode(const CService& addr, std::function cond, std::function func) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); template bool ForNode(const CService& addr, Callable&& func) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) @@ -1345,7 +1349,7 @@ friend class CNode; template bool ForEachNodeContinueIf(const Condition& cond, Callable&& func) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { - LOCK(m_nodes_mutex); + READ_LOCK(m_nodes_mutex); for (const auto& node : m_nodes) if (cond(node)) if(!func(node)) @@ -1377,7 +1381,7 @@ friend class CNode; template void ForEachNode(const Condition& cond, Callable&& func) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { - LOCK(m_nodes_mutex); + READ_LOCK(m_nodes_mutex); for (auto&& node : m_nodes) { if (cond(node)) func(node); @@ -1409,7 +1413,7 @@ friend class CNode; template void ForEachNodeThen(const Condition& cond, Callable&& pre, CallableAfter&& post) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { - LOCK(m_nodes_mutex); + READ_LOCK(m_nodes_mutex); for (auto&& node : m_nodes) { if (cond(node)) pre(node); @@ -1535,7 +1539,7 @@ friend class CNode; /** Return true if we should disconnect the peer for failing an inactivity check. */ bool ShouldRunInactivityChecks(const CNode& node, std::chrono::seconds now) const; - bool MultipleManualOrFullOutboundConns(Network net) const EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex); + bool MultipleManualOrFullOutboundConns(Network net) const SHARED_LOCKS_REQUIRED(m_nodes_mutex); /** * RAII helper to atomically create a copy of `m_nodes` and add a reference @@ -1651,21 +1655,78 @@ friend class CNode; uint64_t CalculateKeyedNetGroup(const CAddress& ad) const; - CNode* FindNode(const CNetAddr& ip, bool fExcludeDisconnecting = true) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); - CNode* FindNode(const std::string& addrName, bool fExcludeDisconnecting = true) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); - CNode* FindNode(const CService& addr, bool fExcludeDisconnecting = true) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + const CNode* FindNodeLocked(const CNetAddr& ip, bool fExcludeDisconnecting = true) const SHARED_LOCKS_REQUIRED(m_nodes_mutex); + const CNode* FindNodeLocked(const std::string& addrName, bool fExcludeDisconnecting = true) const SHARED_LOCKS_REQUIRED(m_nodes_mutex); + const CNode* FindNodeLocked(const CService& addr, bool fExcludeDisconnecting = true) const SHARED_LOCKS_REQUIRED(m_nodes_mutex); + const CNode* FindNodeLocked(NodeId id, bool fExcludeDisconnecting = true) const SHARED_LOCKS_REQUIRED(m_nodes_mutex); + + // Mutable find helpers for callers that already hold the exclusive lock + CNode* FindNodeLockedMutable(const CNetAddr& ip, bool fExcludeDisconnecting = true) EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex); + CNode* FindNodeLockedMutable(const std::string& addrName, bool fExcludeDisconnecting = true) EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex); + CNode* FindNodeLockedMutable(const CService& addr, bool fExcludeDisconnecting = true) EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex); + CNode* FindNodeLockedMutable(NodeId id, bool fExcludeDisconnecting = true) EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex); + + // Type-agnostic node matching helpers + static inline bool NodeMatches(const CNode* p, const CService& addr) + { + return static_cast(p->addr) == addr; + } + static inline bool NodeMatches(const CNode* p, const CNetAddr& ip) + { + return static_cast(p->addr) == ip; + } + static inline bool NodeMatches(const CNode* p, const std::string& addrName) + { + return p->m_addr_name == addrName; + } + static inline bool NodeMatches(const CNode* p, const NodeId id) + { + return p->GetId() == id; + } + + template + const CNode* FindNodeLockedBy(const Key& key, bool fExcludeDisconnecting = true) const SHARED_LOCKS_REQUIRED(m_nodes_mutex) + { + for (const CNode* pnode : m_nodes) { + if (fExcludeDisconnecting && pnode->fDisconnect) continue; + if (NodeMatches(pnode, key)) return pnode; + } + return nullptr; + } + + // Callback helpers with explicit lock semantics (templated on key type) + // Lambda-based shared accessor returning optional result (nullopt = not found) + template + std::optional> WithNodeShared(const Key& key, Callable&& fn) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) + { + READ_LOCK(m_nodes_mutex); + if (const CNode* p = FindNodeLockedBy(key)) return std::optional>{fn(p)}; + return std::nullopt; + } - // Internal versions that require the lock to be held - CNode* FindNodeLocked(const CNetAddr& ip, bool fExcludeDisconnecting = true) EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex); - CNode* FindNodeLocked(const std::string& addrName, bool fExcludeDisconnecting = true) EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex); - CNode* FindNodeLocked(const CService& addr, bool fExcludeDisconnecting = true) EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex); + // Fast existence check under shared lock + template + bool ExistsNodeShared(const Key& key) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) + { + READ_LOCK(m_nodes_mutex); + return FindNodeLockedBy(key) != nullptr; + } + + template + std::optional> WithNodeExclusive(const Key& key, Callable&& fn) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) + { + LOCK(m_nodes_mutex); + if (const CNode* cp = FindNodeLockedBy(key)) return std::optional>{fn(const_cast(cp))}; + return std::nullopt; + } /** * Determine whether we're already connected to a given address, in order to * avoid initiating duplicate connections. */ - bool AlreadyConnectedToAddress(const CAddress& addr) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + bool AlreadyConnectedToAddress(const CAddress& addr) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + std::vector GetEvictionCandidates() const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); bool AttemptToEvictConnection() EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); CNode* ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type, bool use_v2transport) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_unused_i2p_sessions_mutex); void AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNetAddr &addr) const; @@ -1746,12 +1807,12 @@ friend class CNode; mutable Mutex m_added_nodes_mutex; std::vector m_nodes GUARDED_BY(m_nodes_mutex); std::list m_nodes_disconnected; - mutable Mutex m_nodes_mutex; + mutable SharedMutex m_nodes_mutex; std::atomic nLastNodeId{0}; unsigned int nPrevNodeCount{0}; // Stores number of full-tx connections (outbound and manual) per network - std::array m_network_conn_counts GUARDED_BY(m_nodes_mutex) = {}; + std::array m_network_conn_counts GUARDED_BY(m_nodes_mutex) = {}; // TODO consider moving this to seperate mutex std::vector vPendingMasternodes; mutable RecursiveMutex cs_vPendingMasternodes; diff --git a/src/sync.h b/src/sync.h index 8ca945cca76f4..e22fb3c62ad78 100644 --- a/src/sync.h +++ b/src/sync.h @@ -161,6 +161,11 @@ using SharedMutex = SharedAnnotatedMixin; #define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs) +// For shared locks, we use the same internal function but disable thread safety analysis +// because the function signature requires exclusive lock but we only have shared lock +inline void AssertSharedLockHeldInline(const char* name, const char* file, int line, SharedMutex* cs) NO_THREAD_SAFETY_ANALYSIS { AssertLockHeldInternal(name, file, line, cs); } +#define AssertSharedLockHeld(cs) (AssertSharedLockHeldInline(#cs, __FILE__, __LINE__, &cs)) + inline void AssertLockNotHeldInline(const char* name, const char* file, int line, Mutex* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) { AssertLockNotHeldInternal(name, file, line, cs); } inline void AssertLockNotHeldInline(const char* name, const char* file, int line, RecursiveMutex* cs) LOCKS_EXCLUDED(cs) { AssertLockNotHeldInternal(name, file, line, cs); } inline void AssertLockNotHeldInline(const char* name, const char* file, int line, GlobalMutex* cs) LOCKS_EXCLUDED(cs) { AssertLockNotHeldInternal(name, file, line, cs); } From 7121c3cc6829720c3eafbddecb7dc599c97324cd Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 21 Oct 2025 22:26:17 -0500 Subject: [PATCH 3/6] code review fixes --- src/net.cpp | 2 +- src/net.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 0ce860e45c4e4..5ed1037179d2d 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -419,7 +419,7 @@ CNode* CConnman::FindNodeLockedMutable(NodeId id, bool fExcludeDisconnecting) bool CConnman::AlreadyConnectedToAddress(const CAddress& addr) const { READ_LOCK(m_nodes_mutex); - return FindNodeLocked(addr.ToStringAddrPort()) != nullptr; + return FindNodeLocked(static_cast(addr)) != nullptr; } bool CConnman::CheckIncomingNonce(uint64_t nonce) const diff --git a/src/net.h b/src/net.h index 73cbb80e6737f..15ddd9c4e3228 100644 --- a/src/net.h +++ b/src/net.h @@ -1637,7 +1637,7 @@ friend class CNode; * @param[in] events_per_sock Sockets that are ready for IO. */ void SocketHandlerConnected(const Sock::EventsPerSock& events_per_sock) - EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_total_bytes_sent_mutex, !mutexMsgProc, !cs_sendable_receivable_nodes, !cs_mapSocketToNode, !cs_sendable_receivable_nodes); + EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_total_bytes_sent_mutex, !mutexMsgProc, !cs_sendable_receivable_nodes, !cs_mapSocketToNode); /** * Accept incoming connections, one from each read-ready listening socket. From 420f6148048754ce5d22cd32584abbce0ec64f13 Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 21 Oct 2025 22:35:48 -0500 Subject: [PATCH 4/6] style: clean up whitespace in net.cpp --- src/net.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 5ed1037179d2d..aa6eb382e60a8 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1936,7 +1936,7 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, NetPermissions::AddFlag(permission_flags, NetPermissionFlags::NoBan); } - + { READ_LOCK(m_nodes_mutex); for (const CNode* pnode : m_nodes) { @@ -2405,7 +2405,7 @@ void CConnman::SocketHandler(CMasternodeSync& mn_sync) bool only_poll = [this]() { // Check if we have work to do and thus should avoid waiting for events READ_LOCK(m_nodes_mutex); // We acquire this to avoid the pointers stored in mapSendableNodes and mapReceivableNodes being invalidated by ThreadSocketHandler - LOCK(cs_sendable_receivable_nodes); + LOCK(cs_sendable_receivable_nodes); if (!mapReceivableNodes.empty()) { return true; } @@ -4817,8 +4817,6 @@ void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg) } } - - bool CConnman::ForNode(const CService& addr, std::function cond, std::function func) { CNode* found = nullptr; From c1b3047d66ab30b274ce7172f22ee62d637a5ee2 Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 22 Oct 2025 12:12:45 -0500 Subject: [PATCH 5/6] perf: only acquire exclusive locks on m_nodes_mutex if we mutate m_nodes; CNode is internally thread safe --- src/net.cpp | 36 +++++++++++++----------------------- src/net.h | 27 +++------------------------ src/test/util/net.h | 2 +- 3 files changed, 17 insertions(+), 48 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index aa6eb382e60a8..9b4a07a556906 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -493,7 +493,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo } // It is possible that we already have a connection to the IP/port pszDest resolved to. // In that case, drop the connection that was just created. - if (WithNodeExclusive(static_cast(addrConnect), [&](CNode* /*pnode*/){ return true; })) { + if (ExistsNodeShared(static_cast(addrConnect))) { LogPrintf("Not opening a connection to %s, already connected to %s\n", pszDest, addrConnect.ToStringAddrPort()); return nullptr; } @@ -2616,7 +2616,7 @@ size_t CConnman::SocketRecvData(CNode *pnode) { bool notify = false; if (!pnode->ReceiveMsgBytes(Span(pchBuf, nBytes), notify)) { - LOCK(m_nodes_mutex); + READ_LOCK(m_nodes_mutex); // is this here for lock ordering? pnode->CloseSocketDisconnect(this); } RecordBytesRecv(nBytes); @@ -2631,7 +2631,7 @@ size_t CConnman::SocketRecvData(CNode *pnode) if (!pnode->fDisconnect) { LogPrint(BCLog::NET, "socket closed for peer=%d\n", pnode->GetId()); } - LOCK(m_nodes_mutex); + READ_LOCK(m_nodes_mutex); // is this here for lock ordering? pnode->fOtherSideDisconnected = true; // avoid lingering pnode->CloseSocketDisconnect(this); } @@ -2644,7 +2644,7 @@ size_t CConnman::SocketRecvData(CNode *pnode) if (!pnode->fDisconnect){ LogPrint(BCLog::NET, "socket recv error for peer=%d: %s\n", pnode->GetId(), NetworkErrorString(nErr)); } - LOCK(m_nodes_mutex); + READ_LOCK(m_nodes_mutex); // is this here for lock ordering? pnode->fOtherSideDisconnected = true; // avoid lingering pnode->CloseSocketDisconnect(this); } @@ -4543,7 +4543,7 @@ bool CConnman::DisconnectNode(const std::string& strNode) bool CConnman::DisconnectNode(const CSubNet& subnet) { bool disconnected = false; - LOCK(m_nodes_mutex); + READ_LOCK(m_nodes_mutex); for (CNode* pnode : m_nodes) { if (subnet.Match(pnode->addr)) { LogPrint(BCLog::NET_NETCONN, "disconnect by subnet%s matched peer=%d; disconnecting\n", (fLogIPs ? strprintf("=%s", subnet.ToString()) : ""), pnode->GetId()); @@ -4561,15 +4561,11 @@ bool CConnman::DisconnectNode(const CNetAddr& addr) bool CConnman::DisconnectNode(NodeId id) { - LOCK(m_nodes_mutex); - for(CNode* pnode : m_nodes) { - if (id == pnode->GetId()) { - LogPrint(BCLog::NET_NETCONN, "disconnect by id peer=%d; disconnecting\n", pnode->GetId()); - pnode->fDisconnect = true; - return true; - } - } - return false; + return WithNodeExclusive(id, [&](CNode* pnode){ + LogPrint(BCLog::NET_NETCONN, "disconnect by id peer=%d; disconnecting\n", pnode->GetId()); + pnode->fDisconnect = true; + return true; + }).value_or(false); } void CConnman::RecordBytesRecv(uint64_t bytes) @@ -4819,8 +4815,8 @@ void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg) bool CConnman::ForNode(const CService& addr, std::function cond, std::function func) { + READ_LOCK(m_nodes_mutex); CNode* found = nullptr; - LOCK(m_nodes_mutex); for (auto&& pnode : m_nodes) { if((CService)pnode->addr == addr) { found = pnode; @@ -4832,14 +4828,8 @@ bool CConnman::ForNode(const CService& addr, std::function cond, std::function func) const { - CNode* found = nullptr; READ_LOCK(m_nodes_mutex); - for (auto&& pnode : m_nodes) { - if((CService)pnode->addr == addr) { - found = pnode; - break; - } - } + const CNode* found = FindNodeLockedBy(addr, false); return found != nullptr && cond(found) && func(found); } @@ -4859,7 +4849,7 @@ bool CConnman::ForNode(NodeId id, std::function cond, bool CConnman::ForNode(NodeId id, std::function cond, std::function func) { CNode* found = nullptr; - LOCK(m_nodes_mutex); + READ_LOCK(m_nodes_mutex); for (auto&& pnode : m_nodes) { if(pnode->GetId() == id) { found = pnode; diff --git a/src/net.h b/src/net.h index 15ddd9c4e3228..d06e0c845cf7c 100644 --- a/src/net.h +++ b/src/net.h @@ -1319,7 +1319,7 @@ friend class CNode; bool IsConnected(const CService& addr, std::function cond) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { - return ForNode(addr, cond, [](CNode* pnode){ + return ForNode(addr, cond, [](const CNode* pnode){ return true; }); } @@ -1332,7 +1332,7 @@ friend class CNode; template bool ForEachNodeContinueIf(const Condition& cond, Callable&& func) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { - LOCK(m_nodes_mutex); + READ_LOCK(m_nodes_mutex); for (auto&& node : m_nodes) if (cond(node)) if(!func(node)) @@ -1363,16 +1363,6 @@ friend class CNode; return ForEachNodeContinueIf(FullyConnectedOnly, func); } - template - void ForEachNode(const Condition& cond, Callable&& func) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) - { - LOCK(m_nodes_mutex); - for (auto&& node : m_nodes) { - if (cond(node)) - func(node); - } - }; - void ForEachNode(const NodeFn& fn) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { ForEachNode(FullyConnectedOnly, fn); @@ -1393,17 +1383,6 @@ friend class CNode; ForEachNode(FullyConnectedOnly, fn); } - template - void ForEachNodeThen(const Condition& cond, Callable&& pre, CallableAfter&& post) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) - { - LOCK(m_nodes_mutex); - for (auto&& node : m_nodes) { - if (cond(node)) - pre(node); - } - post(); - }; - template void ForEachNodeThen(Callable&& pre, CallableAfter&& post) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { @@ -1715,7 +1694,7 @@ friend class CNode; template std::optional> WithNodeExclusive(const Key& key, Callable&& fn) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { - LOCK(m_nodes_mutex); + READ_LOCK(m_nodes_mutex); if (const CNode* cp = FindNodeLockedBy(key)) return std::optional>{fn(const_cast(cp))}; return std::nullopt; } diff --git a/src/test/util/net.h b/src/test/util/net.h index ab1d958546c89..a52a3ec753d10 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -41,7 +41,7 @@ struct ConnmanTestMsg : public CConnman { std::vector TestNodes() { - LOCK(m_nodes_mutex); + READ_LOCK(m_nodes_mutex); return m_nodes; } From 9b22095c81e5d2b787e44bc1eb55695d5d3b4c62 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 29 Oct 2025 16:57:52 +0300 Subject: [PATCH 6/6] Various suggestions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Major Improvements ✅ 1. Simplified FindNode API // REMOVED the complex FindNodeLocked/FindNodeLockedMutable distinction - const CNode* FindNodeLocked(...) const SHARED_LOCKS_REQUIRED(m_nodes_mutex); - CNode* FindNodeLockedMutable(...) EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex); // REPLACED with simple template-based approach template const CNode* FindNode(const Key& key, ...) const SHARED_LOCKS_REQUIRED(m_nodes_mutex); template CNode* FindNodeMutable(const Key& key, ...) SHARED_LOCKS_REQUIRED(m_nodes_mutex); Analysis: ✅ Much better! - Generic template handles all key types (CNetAddr, CService, NodeId, string) - Both require only SHARED_LOCKS_REQUIRED - correctly reflects that finding in the vector only needs shared lock - Mutable vs const is about pointer type, not lock type - Eliminates ~80 lines of duplicate code 2. Fixed getPendingProbes Lock Annotation // BEFORE: Incorrectly required m_nodes_mutex - const auto getPendingProbes = [&]() EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex, cs_vPendingMasternodes) { - AssertLockHeld(m_nodes_mutex); // AFTER: Only requires cs_vPendingMasternodes (correct!) + const auto getPendingProbes = [&]() EXCLUSIVE_LOCKS_REQUIRED(cs_vPendingMasternodes) { Analysis: ✅ Correct! This fixes the issue we identified - getPendingProbes doesn't access m_nodes. 3. Fixed getPendingQuorumNodes Lock Annotation // BEFORE: Required exclusive lock - const auto getPendingQuorumNodes = [&]() EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex, cs_vPendingMasternodes) { - AssertLockHeld(m_nodes_mutex); // AFTER: Only requires shared lock (correct!) + const auto getPendingQuorumNodes = [&]() SHARED_LOCKS_REQUIRED(m_nodes_mutex) + EXCLUSIVE_LOCKS_REQUIRED(cs_vPendingMasternodes) { + AssertSharedLockHeld(m_nodes_mutex); Analysis: ✅ Correct! - Now properly uses SHARED_LOCKS_REQUIRED since it only reads from m_nodes - Uses AssertSharedLockHeld instead of AssertLockHeld - Writes to pnode->fDisconnect are safe (atomic) 4. Simplified getConnectToDmn Locking // BEFORE: Held exclusive lock - LOCK2(m_nodes_mutex, cs_vPendingMasternodes); // AFTER: Only holds shared lock for m_nodes + READ_LOCK(m_nodes_mutex); + LOCK(cs_vPendingMasternodes); Analysis: ✅ Excellent optimization! - Only needs shared lock since it's just reading from m_nodes - Reduces contention significantly 5. Improved Logic in getPendingQuorumNodes // BEFORE: Complex TOCTOU pattern - if (const CNode* pnode = FindNodeLocked(addr2)) { - slowHandshake = pnode->nTimeFirstMessageReceived.load() != 0s && ...; - } - if (slowHandshake) { - if (CNode* p2 = FindNodeLockedMutable(addr2)) { p2->fDisconnect = true; } - } // AFTER: Simplified single lookup + CNode* pnode = FindNodeMutable(addr2); + if (pnode && pnode->m_masternode_connection) { + continue; + } + if (connectedNodes.count(addr2)) { + bool slow_handshake = pnode && pnode->nTimeFirstMessageReceived.load() != 0s && ...; + if (slow_handshake) { + pnode->fDisconnect = true; // Direct access, no second lookup + } Analysis: ✅ Much cleaner! - Eliminates TOCTOU issue by using single pointer - More readable logic flow - Early connectedProRegTxHashes check avoids unnecessary work 6. Renamed Helper Methods for Clarity // BEFORE: Confusing names - WithNodeShared() - WithNodeExclusive() - ExistsNodeShared() // AFTER: Clear intent + WithNodeMutable() // Callback gets mutable CNode* + ExistsNode() // Simple existence check Analysis: ✅ Better naming! - WithNodeMutable clearly indicates you get a mutable pointer - No need for "Shared" suffix - the shared lock is an implementation detail - The lock type is in the function signature for type safety 7. Proper AssertSharedLockHeld Implementation // BEFORE: Hacky workaround with NO_THREAD_SAFETY_ANALYSIS - inline void AssertSharedLockHeldInline(...) NO_THREAD_SAFETY_ANALYSIS { - AssertLockHeldInternal(...); - } // AFTER: Proper template specialization + template + void AssertSharedLockHeldInternal(...) SHARED_LOCKS_REQUIRED(cs) { ... } + template void AssertSharedLockHeldInternal(..., SharedMutex*); Analysis: ✅ Much better! - Proper template implementation - Correct thread safety annotations - No need for NO_THREAD_SAFETY_ANALYSIS hack Remaining Observations ⚠️ FindNodeMutable with Shared Lock template CNode* FindNodeMutable(const Key& key, ...) SHARED_LOCKS_REQUIRED(m_nodes_mutex) Question: This returns a mutable CNode* under shared lock. Is this intentional? Answer: ✅ Yes, this is correct! - The shared lock protects the m_nodes container, not the CNode objects - CNode has internal thread safety - Returning mutable pointer allows modifying atomic members like fDisconnect - The "Mutable" in the name documents this is intentional ✅ IsMasternodeOrDisconnectRequested Now Const - bool IsMasternodeOrDisconnectRequested(const CService& addr); + bool IsMasternodeOrDisconnectRequested(const CService& addr) const; Analysis: ✅ Correct - it's a read-only operation. Summary of Improvements | Issue | Status | Notes | |---------------------------------------|---------|-----------------------------------| | Unnecessary FindNodeLocked variants | ✅ Fixed | Replaced with clean templates | | getPendingProbes incorrect lock | ✅ Fixed | Removed m_nodes_mutex requirement | | getPendingQuorumNodes wrong lock type | ✅ Fixed | Changed to SHARED_LOCKS_REQUIRED | | TOCTOU in masternode connection | ✅ Fixed | Single pointer lookup | | AssertSharedLockHeld hack | ✅ Fixed | Proper template implementation | | Confusing method names | ✅ Fixed | Better naming convention | | Over-locking in getConnectToDmn | ✅ Fixed | Changed to READ_LOCK | Reviewed-by: Claude (Anthropic AI Assistant) --- src/net.cpp | 195 +++++++++++++-------------------------------------- src/net.h | 43 +++++------- src/sync.cpp | 9 +++ src/sync.h | 10 +-- 4 files changed, 82 insertions(+), 175 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 9b4a07a556906..ea4f08246e13a 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -338,88 +338,10 @@ bool IsLocal(const CService& addr) return mapLocalHost.count(addr) > 0; } -// Internal FindNodeLocked - const versions do the actual work -const CNode* CConnman::FindNodeLocked(const CNetAddr& ip, bool fExcludeDisconnecting) const -{ - for (const CNode* pnode : m_nodes) { - if (fExcludeDisconnecting && pnode->fDisconnect) { - continue; - } - if (static_cast(pnode->addr) == ip) { - return pnode; - } - } - return nullptr; -} - -const CNode* CConnman::FindNodeLocked(const std::string& addrName, bool fExcludeDisconnecting) const -{ - for (const CNode* pnode : m_nodes) { - if (fExcludeDisconnecting && pnode->fDisconnect) { - continue; - } - if (pnode->m_addr_name == addrName) { - return pnode; - } - } - return nullptr; -} - -const CNode* CConnman::FindNodeLocked(const CService& addr, bool fExcludeDisconnecting) const -{ - for (const CNode* pnode : m_nodes) { - if (fExcludeDisconnecting && pnode->fDisconnect) { - continue; - } - if (static_cast(pnode->addr) == addr) { - return pnode; - } - } - return nullptr; -} - -const CNode* CConnman::FindNodeLocked(NodeId id, bool fExcludeDisconnecting) const -{ - for (const CNode* pnode : m_nodes) { - if (fExcludeDisconnecting && pnode->fDisconnect) { - continue; - } - if (pnode->GetId() == id) { - return pnode; - } - } - return nullptr; -} - -CNode* CConnman::FindNodeLockedMutable(const CNetAddr& ip, bool fExcludeDisconnecting) -{ - AssertLockHeld(m_nodes_mutex); - return const_cast(FindNodeLocked(ip, fExcludeDisconnecting)); -} - -CNode* CConnman::FindNodeLockedMutable(const std::string& addrName, bool fExcludeDisconnecting) -{ - AssertLockHeld(m_nodes_mutex); - return const_cast(FindNodeLocked(addrName, fExcludeDisconnecting)); -} - -CNode* CConnman::FindNodeLockedMutable(const CService& addr, bool fExcludeDisconnecting) -{ - AssertLockHeld(m_nodes_mutex); - return const_cast(FindNodeLocked(addr, fExcludeDisconnecting)); -} - -CNode* CConnman::FindNodeLockedMutable(NodeId id, bool fExcludeDisconnecting) -{ - AssertLockHeld(m_nodes_mutex); - return const_cast(FindNodeLocked(id, fExcludeDisconnecting)); -} - - bool CConnman::AlreadyConnectedToAddress(const CAddress& addr) const { READ_LOCK(m_nodes_mutex); - return FindNodeLocked(static_cast(addr)) != nullptr; + return FindNode(static_cast(addr)) != nullptr; } bool CConnman::CheckIncomingNonce(uint64_t nonce) const @@ -457,8 +379,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo } // Look for an existing connection - if (WithNodeShared(static_cast(addrConnect), [](const CNode*){ return true; })) - { + if (ExistsNode(static_cast(addrConnect))) { LogPrintf("Failed to open new connection, already connected\n"); return nullptr; } @@ -493,7 +414,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo } // It is possible that we already have a connection to the IP/port pszDest resolved to. // In that case, drop the connection that was just created. - if (ExistsNodeShared(static_cast(addrConnect))) { + if (ExistsNode(static_cast(addrConnect))) { LogPrintf("Not opening a connection to %s, already connected to %s\n", pszDest, addrConnect.ToStringAddrPort()); return nullptr; } @@ -1882,7 +1803,7 @@ bool CConnman::AttemptToEvictConnection() if (!node_id_to_evict) { return false; } - return WithNodeExclusive(*node_id_to_evict, [](CNode* pnode){ + return WithNodeMutable(*node_id_to_evict, [](CNode* pnode){ LogPrint(BCLog::NET_NETCONN, "selected %s connection for eviction peer=%d; disconnecting\n", pnode->ConnectionTypeAsString(), pnode->GetId()); pnode->fDisconnect = true; return true; @@ -3447,52 +3368,53 @@ void CConnman::ThreadOpenMasternodeConnections(CDeterministicMNManager& dmnman, MasternodeProbeConn isProbe = MasternodeProbeConn::IsNotConnection; - const auto getPendingQuorumNodes = [&]() EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex, cs_vPendingMasternodes) { - AssertLockHeld(m_nodes_mutex); + const auto getPendingQuorumNodes = [&]() SHARED_LOCKS_REQUIRED(m_nodes_mutex) EXCLUSIVE_LOCKS_REQUIRED(cs_vPendingMasternodes) { + AssertSharedLockHeld(m_nodes_mutex); AssertLockHeld(cs_vPendingMasternodes); std::vector ret; for (const auto& group : masternodeQuorumNodes) { for (const auto& proRegTxHash : group.second) { + if (connectedProRegTxHashes.count(proRegTxHash)) { + continue; + } auto dmn = mnList.GetMN(proRegTxHash); if (!dmn) { continue; } const auto addr2 = dmn->pdmnState->netInfo->GetPrimary(); - if (connectedNodes.count(addr2) && !connectedProRegTxHashes.count(proRegTxHash)) { + CNode* pnode = FindNodeMutable(addr2); + if (pnode && pnode->m_masternode_connection) { + // node is masternode, skip it + continue; + } + if (connectedNodes.count(addr2)) { // we probably connected to it before it became a masternode // or maybe we are still waiting for mnauth - // Use shared access since we already hold m_nodes_mutex - bool slowHandshake = false; - if (const CNode* pnode = FindNodeLocked(addr2)) { - slowHandshake = pnode->nTimeFirstMessageReceived.load() != 0s && GetTime() - pnode->nTimeFirstMessageReceived.load() > 5s; - } - if (slowHandshake) { + bool slow_handshake = pnode && pnode->nTimeFirstMessageReceived.load() != 0s && + GetTime() - pnode->nTimeFirstMessageReceived.load() > 5s; + if (slow_handshake) { // clearly not expecting mnauth to take that long even if it wasn't the first message // we received (as it should normally), disconnect - LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- dropping non-mnauth connection to %s, service=%s\n", _func_, proRegTxHash.ToString(), addr2.ToStringAddrPort()); - if (CNode* p2 = FindNodeLockedMutable(addr2)) { p2->fDisconnect = true; } + LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- dropping non-mnauth connection to %s, service=%s\n", + _func_, proRegTxHash.ToString(), addr2.ToStringAddrPort()); + pnode->fDisconnect = true; } // either way - it's not ready, skip it for now continue; } - // Check if node is masternode or disconnect requested using FindNodeLocked - const CNode* existingNode = FindNodeLocked(addr2); - bool isDisconnectRequested = existingNode && (existingNode->m_masternode_connection || existingNode->fDisconnect); - if (!connectedNodes.count(addr2) && !isDisconnectRequested && !connectedProRegTxHashes.count(proRegTxHash)) { - int64_t lastAttempt = mn_metaman.GetMetaInfo(dmn->proTxHash)->GetLastOutboundAttempt(); - // back off trying connecting to an address if we already tried recently - if (nANow - lastAttempt < chainParams.LLMQConnectionRetryTimeout()) { - continue; - } - ret.emplace_back(dmn); + // back off connecting to an address if we already tried recently + int64_t last_attempt = mn_metaman.GetMetaInfo(dmn->proTxHash)->GetLastOutboundAttempt(); + if (nANow - last_attempt < chainParams.LLMQConnectionRetryTimeout()) { + continue; } + // all checks passed + ret.emplace_back(dmn); } } return ret; }; - const auto getPendingProbes = [&]() EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex, cs_vPendingMasternodes) { - AssertLockHeld(m_nodes_mutex); + const auto getPendingProbes = [&]() EXCLUSIVE_LOCKS_REQUIRED(cs_vPendingMasternodes) { AssertLockHeld(cs_vPendingMasternodes); std::vector ret; for (auto it = masternodePendingProbes.begin(); it != masternodePendingProbes.end(); ) { @@ -3523,24 +3445,23 @@ void CConnman::ThreadOpenMasternodeConnections(CDeterministicMNManager& dmnman, auto getConnectToDmn = [&]() -> CDeterministicMNCPtr { // don't hold lock while calling OpenMasternodeConnection as cs_main is locked deep inside - LOCK2(m_nodes_mutex, cs_vPendingMasternodes); + READ_LOCK(m_nodes_mutex); + LOCK(cs_vPendingMasternodes); if (!vPendingMasternodes.empty()) { auto dmn = mnList.GetValidMN(vPendingMasternodes.front()); vPendingMasternodes.erase(vPendingMasternodes.begin()); // Check if we should connect to this masternode - // We already hold m_nodes_mutex here, so check fDisconnect and m_masternode_connection directly - bool shouldConnect = true; + // We already hold m_nodes_mutex here, so check m_masternode_connection directly if (dmn && !connectedNodes.count(dmn->pdmnState->netInfo->GetPrimary())) { - bool anyBad = false; - if (const CNode* pnode = FindNodeLocked(dmn->pdmnState->netInfo->GetPrimary())) { - anyBad = pnode->m_masternode_connection || pnode->fDisconnect; + if (const CNode* pnode = FindNode(dmn->pdmnState->netInfo->GetPrimary())) { + if (!pnode->m_masternode_connection) { + LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- opening pending masternode connection to %s, service=%s\n", + _func_, dmn->proTxHash.ToString(), + dmn->pdmnState->netInfo->GetPrimary().ToStringAddrPort()); + return dmn; + } } - shouldConnect = !anyBad; - } - if (dmn && shouldConnect) { - LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- opening pending masternode connection to %s, service=%s\n", _func_, dmn->proTxHash.ToString(), dmn->pdmnState->netInfo->GetPrimary().ToStringAddrPort()); - return dmn; } } @@ -3626,8 +3547,9 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai if (addrConnect.GetPort() == GetListenPort() && IsLocal(addrConnect)) { return; } - } else if (WithNodeShared(std::string(pszDest), [](const CNode*){ return true; })) + } else if (ExistsNode(std::string(pszDest))) { return; + } LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- connecting to %s\n", __func__, getIpStr()); CNode* pnode = ConnectNode(addrConnect, pszDest, fCountFailure, conn_type, use_v2transport); @@ -4533,7 +4455,7 @@ void CConnman::GetNodeStats(std::vector& vstats) const bool CConnman::DisconnectNode(const std::string& strNode) { - return WithNodeExclusive(strNode, [&](CNode* pnode){ + return WithNodeMutable(strNode, [&](CNode* pnode){ LogPrint(BCLog::NET_NETCONN, "disconnect by address%s matched peer=%d; disconnecting\n", (fLogIPs ? strprintf("=%s", strNode) : ""), pnode->GetId()); pnode->fDisconnect = true; return true; @@ -4561,7 +4483,7 @@ bool CConnman::DisconnectNode(const CNetAddr& addr) bool CConnman::DisconnectNode(NodeId id) { - return WithNodeExclusive(id, [&](CNode* pnode){ + return WithNodeMutable(id, [&](CNode* pnode){ LogPrint(BCLog::NET_NETCONN, "disconnect by id peer=%d; disconnecting\n", pnode->GetId()); pnode->fDisconnect = true; return true; @@ -4816,50 +4738,33 @@ void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg) bool CConnman::ForNode(const CService& addr, std::function cond, std::function func) { READ_LOCK(m_nodes_mutex); - CNode* found = nullptr; - for (auto&& pnode : m_nodes) { - if((CService)pnode->addr == addr) { - found = pnode; - break; - } - } + CNode* found = FindNodeMutable(addr, false); return found != nullptr && cond(found) && func(found); } bool CConnman::ForNode(const CService& addr, std::function cond, std::function func) const { READ_LOCK(m_nodes_mutex); - const CNode* found = FindNodeLockedBy(addr, false); + const CNode* found = FindNode(addr, false); return found != nullptr && cond(found) && func(found); } -bool CConnman::ForNode(NodeId id, std::function cond, std::function func) const +bool CConnman::ForNode(NodeId id, std::function cond, std::function func) { - CNode* found = nullptr; READ_LOCK(m_nodes_mutex); - for (auto&& pnode : m_nodes) { - if(pnode->GetId() == id) { - found = pnode; - break; - } - } + CNode* found = FindNodeMutable(id, false); return found != nullptr && cond(found) && func(found); } -bool CConnman::ForNode(NodeId id, std::function cond, std::function func) +bool CConnman::ForNode(NodeId id, std::function cond, std::function func) const { - CNode* found = nullptr; READ_LOCK(m_nodes_mutex); - for (auto&& pnode : m_nodes) { - if(pnode->GetId() == id) { - found = pnode; - break; - } - } + const CNode* found = FindNode(id, false); return found != nullptr && cond(found) && func(found); } -bool CConnman::IsMasternodeOrDisconnectRequested(const CService& addr) { +bool CConnman::IsMasternodeOrDisconnectRequested(const CService& addr) const +{ return ForNode(addr, AllNodes, [](const CNode* pnode){ return pnode->m_masternode_connection || pnode->fDisconnect; }); diff --git a/src/net.h b/src/net.h index d06e0c845cf7c..b2e457fef01d8 100644 --- a/src/net.h +++ b/src/net.h @@ -1324,7 +1324,7 @@ friend class CNode; }); } - bool IsMasternodeOrDisconnectRequested(const CService& addr) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + bool IsMasternodeOrDisconnectRequested(const CService& addr) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); void PushMessage(CNode* pnode, CSerializedNetMsg&& msg) EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc, !m_total_bytes_sent_mutex); @@ -1634,17 +1634,6 @@ friend class CNode; uint64_t CalculateKeyedNetGroup(const CAddress& ad) const; - const CNode* FindNodeLocked(const CNetAddr& ip, bool fExcludeDisconnecting = true) const SHARED_LOCKS_REQUIRED(m_nodes_mutex); - const CNode* FindNodeLocked(const std::string& addrName, bool fExcludeDisconnecting = true) const SHARED_LOCKS_REQUIRED(m_nodes_mutex); - const CNode* FindNodeLocked(const CService& addr, bool fExcludeDisconnecting = true) const SHARED_LOCKS_REQUIRED(m_nodes_mutex); - const CNode* FindNodeLocked(NodeId id, bool fExcludeDisconnecting = true) const SHARED_LOCKS_REQUIRED(m_nodes_mutex); - - // Mutable find helpers for callers that already hold the exclusive lock - CNode* FindNodeLockedMutable(const CNetAddr& ip, bool fExcludeDisconnecting = true) EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex); - CNode* FindNodeLockedMutable(const std::string& addrName, bool fExcludeDisconnecting = true) EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex); - CNode* FindNodeLockedMutable(const CService& addr, bool fExcludeDisconnecting = true) EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex); - CNode* FindNodeLockedMutable(NodeId id, bool fExcludeDisconnecting = true) EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex); - // Type-agnostic node matching helpers static inline bool NodeMatches(const CNode* p, const CService& addr) { @@ -1664,8 +1653,9 @@ friend class CNode; } template - const CNode* FindNodeLockedBy(const Key& key, bool fExcludeDisconnecting = true) const SHARED_LOCKS_REQUIRED(m_nodes_mutex) + const CNode* FindNode(const Key& key, bool fExcludeDisconnecting = true) const SHARED_LOCKS_REQUIRED(m_nodes_mutex) { + AssertSharedLockHeld(m_nodes_mutex); for (const CNode* pnode : m_nodes) { if (fExcludeDisconnecting && pnode->fDisconnect) continue; if (NodeMatches(pnode, key)) return pnode; @@ -1673,30 +1663,33 @@ friend class CNode; return nullptr; } + template + CNode* FindNodeMutable(const Key& key, bool fExcludeDisconnecting = true) SHARED_LOCKS_REQUIRED(m_nodes_mutex) + { + AssertSharedLockHeld(m_nodes_mutex); + for (CNode* pnode : m_nodes) { + if (fExcludeDisconnecting && pnode->fDisconnect) continue; + if (NodeMatches(pnode, key)) return pnode; + } + return nullptr; + } + // Callback helpers with explicit lock semantics (templated on key type) // Lambda-based shared accessor returning optional result (nullopt = not found) template - std::optional> WithNodeShared(const Key& key, Callable&& fn) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) + std::optional> WithNodeMutable(const Key& key, Callable&& fn) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { READ_LOCK(m_nodes_mutex); - if (const CNode* p = FindNodeLockedBy(key)) return std::optional>{fn(p)}; + if (CNode* p = FindNodeMutable(key)) return std::optional>{fn(p)}; return std::nullopt; } // Fast existence check under shared lock template - bool ExistsNodeShared(const Key& key) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) - { - READ_LOCK(m_nodes_mutex); - return FindNodeLockedBy(key) != nullptr; - } - - template - std::optional> WithNodeExclusive(const Key& key, Callable&& fn) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) + bool ExistsNode(const Key& key) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex) { READ_LOCK(m_nodes_mutex); - if (const CNode* cp = FindNodeLockedBy(key)) return std::optional>{fn(const_cast(cp))}; - return std::nullopt; + return FindNode(key) != nullptr; } /** diff --git a/src/sync.cpp b/src/sync.cpp index d1c06376dc0c1..1747b69f6fee1 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -287,6 +287,15 @@ template void AssertLockHeldInternal(const char*, const char*, int, Mutex*); template void AssertLockHeldInternal(const char*, const char*, int, RecursiveMutex*); template void AssertLockHeldInternal(const char*, const char*, int, SharedMutex*); +template +void AssertSharedLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) +{ + if (LockHeld(cs)) return; + tfm::format(std::cerr, "Assertion failed: shared lock %s not held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld()); + abort(); +} +template void AssertSharedLockHeldInternal(const char*, const char*, int, SharedMutex*); + template void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) { diff --git a/src/sync.h b/src/sync.h index e22fb3c62ad78..3a4220c40c356 100644 --- a/src/sync.h +++ b/src/sync.h @@ -61,6 +61,8 @@ void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, c template void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(cs); template +void AssertSharedLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) SHARED_LOCKS_REQUIRED(cs); +template void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) LOCKS_EXCLUDED(cs); void DeleteLock(void* cs); bool LockStackEmpty(); @@ -79,6 +81,8 @@ inline void CheckLastCritical(void* cs, std::string& lockname, const char* guard template inline void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(cs) {} template +inline void AssertSharedLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) SHARED_LOCKS_REQUIRED(cs) {}; +template void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) LOCKS_EXCLUDED(cs) {} inline void DeleteLock(void* cs) {} inline bool LockStackEmpty() { return true; } @@ -160,11 +164,7 @@ class GlobalMutex : public Mutex { }; using SharedMutex = SharedAnnotatedMixin; #define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs) - -// For shared locks, we use the same internal function but disable thread safety analysis -// because the function signature requires exclusive lock but we only have shared lock -inline void AssertSharedLockHeldInline(const char* name, const char* file, int line, SharedMutex* cs) NO_THREAD_SAFETY_ANALYSIS { AssertLockHeldInternal(name, file, line, cs); } -#define AssertSharedLockHeld(cs) (AssertSharedLockHeldInline(#cs, __FILE__, __LINE__, &cs)) +#define AssertSharedLockHeld(cs) AssertSharedLockHeldInternal(#cs, __FILE__, __LINE__, &cs) inline void AssertLockNotHeldInline(const char* name, const char* file, int line, Mutex* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) { AssertLockNotHeldInternal(name, file, line, cs); } inline void AssertLockNotHeldInline(const char* name, const char* file, int line, RecursiveMutex* cs) LOCKS_EXCLUDED(cs) { AssertLockNotHeldInternal(name, file, line, cs); }