laantungir
/
bitcoin
mirror of https://github.com/bitcoin/bitcoin.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#32326: net: improve the interface around FindNode() and avoid a recursive mutex lock 

87e7f37918 doc: clarify peer address in getpeerinfo and addnode RPC help (Vasil Dimov)
2a4450ccbb net: change FindNode() to not return a node and rename it (Vasil Dimov)
4268abae1a net: avoid recursive m_nodes_mutex lock in DisconnectNode() (Vasil Dimov)
3a4d1a25cf net: merge AlreadyConnectedToAddress() and FindNode(CNetAddr) (Vasil Dimov)

Pull request description:

  `CConnman::FindNode()` would lock `m_nodes_mutex`, find the node in `m_nodes`, release the mutex and return the node. The current code is safe but it is a dangerous interface where a caller may end up using the node returned from `FindNode()` without owning `m_nodes_mutex` and without having that node's reference count incremented.

  Change `FindNode()` to return a boolean since all but one of its callers used its return value to check whether a node exists and did not do anything else with the return value.

  Remove a recursive lock on `m_nodes_mutex`.

  Rename `FindNode()` to better describe what it does.

ACKs for top commit:
  achow101:
    ACK 87e7f37918
  furszy:
    Code review ACK 87e7f37918
  hodlinator:
    re-ACK 87e7f37918

Tree-SHA512: 44fb64cd1226eca124ed1f447b4a1ebc42cc5c9e8561fc91949bbeaeaa7fa16fcfd664e85ce142e5abe62cb64197c178ca4ca93b3b3217b913e3c498d0b7d1c9
This commit is contained in:
Ava Chow 2025-10-01 14:17:22 -07:00
parent acc7f2a433 87e7f37918
commit 75353a0163
No known key found for this signature in database
GPG Key ID: 17565732E08E5E41
5 changed files with 40 additions and 55 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

53
src/net.cpp
View File

@ -331,42 +331,22 @@ bool IsLocal(const CService& addr)
return mapLocalHost.count(addr) > 0;
}
CNode* CConnman::FindNode(const CNetAddr& ip)
bool CConnman::AlreadyConnectedToHost(const std::string& host) const
{
LOCK(m_nodes_mutex);
for (CNode* pnode : m_nodes) {
if (static_cast<CNetAddr>(pnode->addr) == ip) {
return pnode;
}
}
return nullptr;
return std::ranges::any_of(m_nodes, [&host](CNode* node) { return node->m_addr_name == host; });
}
CNode* CConnman::FindNode(const std::string& addrName)
bool CConnman::AlreadyConnectedToAddressPort(const CService& addr_port) 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, [&addr_port](CNode* node) { return node->addr == addr_port; });
}
CNode* CConnman::FindNode(const CService& addr)
bool CConnman::AlreadyConnectedToAddress(const CNetAddr& addr) const
{
LOCK(m_nodes_mutex);
for (CNode* pnode : m_nodes) {
if (static_cast<CService>(pnode->addr) == addr) {
return pnode;
}
}
return nullptr;
}
bool CConnman::AlreadyConnectedToAddress(const CAddress& addr)
{
return FindNode(static_cast<CNetAddr>(addr));
return std::ranges::any_of(m_nodes, [&addr](CNode* node) { return node->addr == addr; });
}
bool CConnman::CheckIncomingNonce(uint64_t nonce)
@ -403,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<CService>(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;
}
}
@ -436,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<CService>(addrConnect));
if (pnode) {
if (AlreadyConnectedToAddressPort(addrConnect)) {
LogPrintf("Not opening a connection to %s, already connected to %s\n", pszDest, addrConnect.ToStringAddrPort());
return nullptr;
}
@ -3010,8 +2986,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);
@ -3651,9 +3628,11 @@ void CConnman::GetNodeStats(std::vector<CNodeStats>& 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;

25
src/net.h
View File

@ -1370,15 +1370,28 @@ 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 "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, in order to
* avoid initiating duplicate connections.
* 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 AlreadyConnectedToAddress(const CAddress& addr);
bool AlreadyConnectedToAddressPort(const CService& addr_port) const;
/**
* Determine whether we're already connected to a given address.
*/
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);

4
src/rpc/net.cpp
View File

@ -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)"},
},

11
src/test/net_peer_connection_tests.cpp
View File

@ -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

2
src/test/util/net.h
View File

@ -107,7 +107,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);