From 2ed51a699cca2e350b8ae303f2a2f4684fab2c7d Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 22 Apr 2025 12:15:32 +0200 Subject: [PATCH 1/3] net: get AlreadyConnectedToAddress() to iterate the nodes itself Previously `CConnman::AlreadyConnectedToAddress()` used two calls to `CConnman::FindNode()`. However that iterates the nodes two times which is inefficient and this was the only caller of `CConnman::FindNode(const CNetAddr&)`. So, drop that `FindNode()` method and iterate the nodes inside `CConnman::AlreadyConnectedToAddress()`. --- src/net.cpp | 19 +++++++------------ src/net.h | 1 - 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index fc0edc1a5c1..53a8301bd71 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -332,17 +332,6 @@ bool IsLocal(const CService& addr) return mapLocalHost.count(addr) > 0; } -CNode* CConnman::FindNode(const CNetAddr& ip) -{ - LOCK(m_nodes_mutex); - for (CNode* pnode : m_nodes) { - if (static_cast(pnode->addr) == ip) { - return pnode; - } - } - return nullptr; -} - CNode* CConnman::FindNode(const std::string& addrName) { LOCK(m_nodes_mutex); @@ -367,7 +356,13 @@ CNode* CConnman::FindNode(const CService& addr) bool CConnman::AlreadyConnectedToAddress(const CAddress& addr) { - return FindNode(static_cast(addr)) || FindNode(addr.ToStringAddrPort()); + const CNetAddr& net_addr{addr}; + const std::string str_addr{addr.ToStringAddrPort()}; + + LOCK(m_nodes_mutex); + return std::ranges::any_of(m_nodes, [&net_addr, &str_addr](CNode* node) { + return node->addr == net_addr || node->m_addr_name == str_addr; + }); } bool CConnman::CheckIncomingNonce(uint64_t nonce) diff --git a/src/net.h b/src/net.h index 9fdec52115e..19b35e8fd2b 100644 --- a/src/net.h +++ b/src/net.h @@ -1352,7 +1352,6 @@ private: uint64_t CalculateKeyedNetGroup(const CNetAddr& ad) const; - CNode* FindNode(const CNetAddr& ip); CNode* FindNode(const std::string& addrName); CNode* FindNode(const CService& addr); From c24e6d025ee847e66c39da521f39ac29ce7f0747 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 22 Apr 2025 12:32:34 +0200 Subject: [PATCH 2/3] net: avoid recursive m_nodes_mutex lock in DisconnectNode() Have `CConnman::DisconnectNode()` iterate `m_nodes` itself instead of using `FindNode()`. This avoids recursive mutex lock and drops the only caller of `FindNode()` which used the return value for something else than a boolean found/notfound. --- src/net.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/net.cpp b/src/net.cpp index 53a8301bd71..2870fd4d5a2 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3634,7 +3634,9 @@ void CConnman::GetNodeStats(std::vector& vstats) const bool CConnman::DisconnectNode(const std::string& strNode) { LOCK(m_nodes_mutex); - if (CNode* pnode = FindNode(strNode)) { + auto it = std::ranges::find_if(m_nodes, [&strNode](CNode* node) { return node->m_addr_name == strNode; }); + if (it != m_nodes.end()) { + CNode* pnode{*it}; LogDebug(BCLog::NET, "disconnect by address%s match, %s", (fLogIPs ? strprintf("=%s", strNode) : ""), pnode->DisconnectMsg(fLogIPs)); pnode->fDisconnect = true; return true; From cc8f239663a0874c307efa30815b40d9ef40badb Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 22 Apr 2025 15:17:36 +0200 Subject: [PATCH 3/3] net: change FindNode() to not return a node and rename it All callers of `CConnman::FindNode()` use its return value `CNode*` only as a boolean null/notnull. So change that method to return `bool`. This removes the dangerous pattern of handling a `CNode` object (the return value of `FindNode()`) without holding `CConnman::m_nodes_mutex` and without having that object's reference count incremented for the duration of the usage. Also rename the method to better describe what it does. --- src/net.cpp | 31 +++++++++---------------------- src/net.h | 19 +++++++++++++++++-- 2 files changed, 26 insertions(+), 24 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 2870fd4d5a2..c3162089e27 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -332,26 +332,16 @@ bool IsLocal(const CService& addr) return mapLocalHost.count(addr) > 0; } -CNode* CConnman::FindNode(const std::string& addrName) +bool CConnman::OutboundConnectedToStr(const std::string& str_addr) { LOCK(m_nodes_mutex); - for (CNode* pnode : m_nodes) { - if (pnode->m_addr_name == addrName) { - return pnode; - } - } - return nullptr; + return std::ranges::any_of(m_nodes, [&str_addr](CNode* node) { return node->m_addr_name == str_addr; }); } -CNode* CConnman::FindNode(const CService& addr) +bool CConnman::OutboundConnectedToService(const CService& addr) { LOCK(m_nodes_mutex); - for (CNode* pnode : m_nodes) { - if (static_cast(pnode->addr) == addr) { - return pnode; - } - } - return nullptr; + return std::ranges::any_of(m_nodes, [&addr](CNode* node) { return node->addr == addr; }); } bool CConnman::AlreadyConnectedToAddress(const CAddress& addr) @@ -399,10 +389,8 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo return nullptr; // Look for an existing connection - CNode* pnode = FindNode(static_cast(addrConnect)); - if (pnode) - { - LogPrintf("Failed to open new connection, already connected\n"); + if (OutboundConnectedToService(addrConnect)) { + LogPrintf("Failed to open new connection to %s, already connected", addrConnect.ToStringAddrPort()); return nullptr; } } @@ -432,9 +420,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)); - if (pnode) { + if (OutboundConnectedToService(addrConnect)) { LogPrintf("Not opening a connection to %s, already connected to %s\n", pszDest, addrConnect.ToStringAddrPort()); return nullptr; } @@ -2999,8 +2985,9 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai if (IsLocal(addrConnect) || banned_or_discouraged || AlreadyConnectedToAddress(addrConnect)) { return; } - } else if (FindNode(std::string(pszDest))) + } else if (OutboundConnectedToStr(pszDest)) { return; + } CNode* pnode = ConnectNode(addrConnect, pszDest, fCountFailure, conn_type, use_v2transport); diff --git a/src/net.h b/src/net.h index 19b35e8fd2b..dc0b6661af1 100644 --- a/src/net.h +++ b/src/net.h @@ -1352,8 +1352,23 @@ private: uint64_t CalculateKeyedNetGroup(const CNetAddr& ad) const; - CNode* FindNode(const std::string& addrName); - CNode* FindNode(const CService& addr); + /** + * Determine whether we're already connected to a given "address:port". + * Note that for inbound connections, the peer is likely using a random outbound + * port on their side, so this will likely not match any inbound connections. + * @param[in] str_addr String of the form "address:port", e.g. "1.2.3.4:8333". + * @return true if connected to addrName. + */ + bool OutboundConnectedToStr(const std::string& str_addr); + + /** + * Determine whether we're already connected to a given address:port. + * Note that for inbound connections, the peer is likely using a random outbound + * port on their side, so this will likely not match any inbound connections. + * @param[in] addr Address and port to check. + * @return true if connected to addr. + */ + bool OutboundConnectedToService(const CService& addr); /** * Determine whether we're already connected to a given address, in order to