From 3a4d1a25cf949eb5f27d6dfd4e1b4a966b2cde75 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 22 Apr 2025 12:15:32 +0200 Subject: [PATCH 1/4] net: merge AlreadyConnectedToAddress() and FindNode(CNetAddr) `CConnman::AlreadyConnectedToAddress()` is the only caller of `CConnman::FindNode(CNetAddr)`, so merge the two in one function. The unit test that checked whether `AlreadyConnectedToAddress()` ignores the port is now unnecessary because now the function takes a `CNetAddr` argument. It has no access to the port. --- src/net.cpp | 16 +++------------- src/net.h | 6 ++---- src/test/net_peer_connection_tests.cpp | 11 ++--------- src/test/util/net.h | 2 +- 4 files changed, 8 insertions(+), 27 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 78e5706b851..e8d2819a88e 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -331,17 +331,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); @@ -364,9 +353,10 @@ CNode* CConnman::FindNode(const CService& addr) return nullptr; } -bool CConnman::AlreadyConnectedToAddress(const CAddress& addr) +bool CConnman::AlreadyConnectedToAddress(const CNetAddr& addr) const { - return FindNode(static_cast(addr)); + LOCK(m_nodes_mutex); + return std::ranges::any_of(m_nodes, [&addr](CNode* node) { return node->addr == addr; }); } bool CConnman::CheckIncomingNonce(uint64_t nonce) diff --git a/src/net.h b/src/net.h index afbcc52bd0f..52044fe1eab 100644 --- a/src/net.h +++ b/src/net.h @@ -1365,15 +1365,13 @@ private: uint64_t CalculateKeyedNetGroup(const CNetAddr& ad) const; - CNode* FindNode(const CNetAddr& ip); CNode* FindNode(const std::string& addrName); CNode* FindNode(const CService& addr); /** - * Determine whether we're already connected to a given address, in order to - * avoid initiating duplicate connections. + * Determine whether we're already connected to a given address. */ - bool AlreadyConnectedToAddress(const CAddress& addr); + bool AlreadyConnectedToAddress(const CNetAddr& addr) const; 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); diff --git a/src/test/net_peer_connection_tests.cpp b/src/test/net_peer_connection_tests.cpp index ba3224bca9d..f00aabf5bef 100644 --- a/src/test/net_peer_connection_tests.cpp +++ b/src/test/net_peer_connection_tests.cpp @@ -151,15 +151,8 @@ BOOST_FIXTURE_TEST_CASE(test_addnode_getaddednodeinfo_and_connection_detection, } BOOST_TEST_MESSAGE("\nCheck that all connected peers are correctly detected as connected"); - for (auto node : connman->TestNodes()) { - BOOST_CHECK(connman->AlreadyConnectedPublic(node->addr)); - } - - BOOST_TEST_MESSAGE("\nCheck that peers with the same addresses as connected peers but different ports are detected as connected."); - for (auto node : connman->TestNodes()) { - uint16_t changed_port = node->addr.GetPort() + 1; - CService address_with_changed_port{node->addr, changed_port}; - BOOST_CHECK(connman->AlreadyConnectedPublic(CAddress{address_with_changed_port, NODE_NONE})); + for (const auto& node : connman->TestNodes()) { + BOOST_CHECK(connman->AlreadyConnectedToAddressPublic(node->addr)); } // Clean up diff --git a/src/test/util/net.h b/src/test/util/net.h index a938ff1802a..f696cd53d68 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -89,7 +89,7 @@ struct ConnmanTestMsg : public CConnman { bool ReceiveMsgFrom(CNode& node, CSerializedNetMsg&& ser_msg) const; void FlushSendBuffer(CNode& node) const; - bool AlreadyConnectedPublic(const CAddress& addr) { return AlreadyConnectedToAddress(addr); }; + bool AlreadyConnectedToAddressPublic(const CNetAddr& addr) { return AlreadyConnectedToAddress(addr); }; CNode* ConnectNodePublic(PeerManager& peerman, const char* pszDest, ConnectionType conn_type) EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex); From 4268abae1a1d06f2c4bd26b85b3a491719217fae Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 22 Apr 2025 12:32:34 +0200 Subject: [PATCH 2/4] 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 | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index e8d2819a88e..6e95d6a3736 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3631,9 +3631,11 @@ void CConnman::GetNodeStats(std::vector& vstats) const bool CConnman::DisconnectNode(const std::string& strNode) { LOCK(m_nodes_mutex); - if (CNode* pnode = FindNode(strNode)) { - LogDebug(BCLog::NET, "disconnect by address%s match, %s", (fLogIPs ? strprintf("=%s", strNode) : ""), pnode->DisconnectMsg(fLogIPs)); - pnode->fDisconnect = true; + auto it = std::ranges::find_if(m_nodes, [&strNode](CNode* node) { return node->m_addr_name == strNode; }); + if (it != m_nodes.end()) { + CNode* node{*it}; + LogDebug(BCLog::NET, "disconnect by address%s match, %s", (fLogIPs ? strprintf("=%s", strNode) : ""), node->DisconnectMsg(fLogIPs)); + node->fDisconnect = true; return true; } return false; From 2a4450ccbbe30f6522c3108f136b2b867b2a87fe Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 22 Apr 2025 15:17:36 +0200 Subject: [PATCH 3/4] 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 6e95d6a3736..e0b5107fcff 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -331,26 +331,16 @@ bool IsLocal(const CService& addr) return mapLocalHost.count(addr) > 0; } -CNode* CConnman::FindNode(const std::string& addrName) +bool CConnman::AlreadyConnectedToHost(const std::string& host) const { 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, [&host](CNode* node) { return node->m_addr_name == host; }); } -CNode* CConnman::FindNode(const CService& addr) +bool CConnman::AlreadyConnectedToAddressPort(const CService& addr_port) const { 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_port](CNode* node) { return node->addr == addr_port; }); } bool CConnman::AlreadyConnectedToAddress(const CNetAddr& addr) const @@ -393,10 +383,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 (AlreadyConnectedToAddressPort(addrConnect)) { + LogInfo("Failed to open new connection to %s, already connected", addrConnect.ToStringAddrPort()); return nullptr; } } @@ -426,9 +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. - LOCK(m_nodes_mutex); - CNode* pnode = FindNode(static_cast(addrConnect)); - if (pnode) { + if (AlreadyConnectedToAddressPort(addrConnect)) { LogPrintf("Not opening a connection to %s, already connected to %s\n", pszDest, addrConnect.ToStringAddrPort()); return nullptr; } @@ -2996,8 +2982,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 (AlreadyConnectedToHost(pszDest)) { return; + } CNode* pnode = ConnectNode(addrConnect, pszDest, fCountFailure, conn_type, use_v2transport); diff --git a/src/net.h b/src/net.h index 52044fe1eab..44730419815 100644 --- a/src/net.h +++ b/src/net.h @@ -1365,8 +1365,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 "host: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] host String of the form "host[:port]", e.g. "localhost" or "localhost:8333" or "1.2.3.4:8333". + * @return true if connected to `host`. + */ + bool AlreadyConnectedToHost(const std::string& host) const; + + /** + * 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_port Address and port to check. + * @return true if connected to addr_port. + */ + bool AlreadyConnectedToAddressPort(const CService& addr_port) const; /** * Determine whether we're already connected to a given address. From 87e7f37918d42c28033e9f684db52f94eeed617b Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Wed, 24 Sep 2025 08:34:44 +0200 Subject: [PATCH 4/4] doc: clarify peer address in getpeerinfo and addnode RPC help The returned value in `getpeerinfo/addr` could be a hostname as well as an IP address and the `:port` part could be missing. It is displayed from `CNode::m_addr_name` which could have been set from RPC `addnode` where the argument is allowed to be a hostname and an optional port. --- src/rpc/net.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 3a0432d7a9e..da01a60b9ec 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -130,7 +130,7 @@ static RPCHelpMan getpeerinfo() { { {RPCResult::Type::NUM, "id", "Peer index"}, - {RPCResult::Type::STR, "addr", "(host:port) The IP address and port of the peer"}, + {RPCResult::Type::STR, "addr", "(host:port) The IP address/hostname optionally followed by :port of the peer"}, {RPCResult::Type::STR, "addrbind", /*optional=*/true, "(ip:port) Bind address of the connection to the peer"}, {RPCResult::Type::STR, "addrlocal", /*optional=*/true, "(ip:port) Local address as reported by the peer"}, {RPCResult::Type::STR, "network", "Network (" + Join(GetNetworkNames(/*append_unroutable=*/true), ", ") + ")"}, @@ -322,7 +322,7 @@ static RPCHelpMan addnode() strprintf("Addnode connections are limited to %u at a time", MAX_ADDNODE_CONNECTIONS) + " and are counted separately from the -maxconnections limit.\n", { - {"node", RPCArg::Type::STR, RPCArg::Optional::NO, "The address of the peer to connect to"}, + {"node", RPCArg::Type::STR, RPCArg::Optional::NO, "The IP address/hostname optionally followed by :port of the peer to connect to"}, {"command", RPCArg::Type::STR, RPCArg::Optional::NO, "'add' to add a node to the list, 'remove' to remove a node from the list, 'onetry' to try a connection to the node once"}, {"v2transport", RPCArg::Type::BOOL, RPCArg::DefaultHint{"set by -v2transport"}, "Attempt to connect using BIP324 v2 transport protocol (ignored for 'remove' command)"}, },