From 6cf206ca0eed48290fb1a07e288053005ee6741e Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 25 Oct 2024 20:39:03 +0000 Subject: [PATCH] merge bitcoin#28155: improves addnode / m_added_nodes logic --- src/Makefile.test.include | 1 + src/net.cpp | 67 ++++++----- src/net.h | 2 +- src/rpc/net.cpp | 2 +- src/test/fuzz/connman.cpp | 2 +- src/test/net_peer_connection_tests.cpp | 149 +++++++++++++++++++++++++ src/test/util/net.cpp | 19 +++- src/test/util/net.h | 29 ++++- test/functional/rpc_net.py | 5 +- 9 files changed, 240 insertions(+), 36 deletions(-) create mode 100644 src/test/net_peer_connection_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 1a4cc323e2..1a3f538118 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -134,6 +134,7 @@ BITCOIN_TESTS =\ test/minisketch_tests.cpp \ test/miner_tests.cpp \ test/multisig_tests.cpp \ + test/net_peer_connection_tests.cpp \ test/net_peer_eviction_tests.cpp \ test/net_tests.cpp \ test/netbase_tests.cpp \ diff --git a/src/net.cpp b/src/net.cpp index e4f0d26b27..fa685813af 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -523,21 +523,25 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo const uint16_t default_port{pszDest != nullptr ? Params().GetDefaultPort(pszDest) : Params().GetDefaultPort()}; if (pszDest) { - const std::vector resolved{Lookup(pszDest, default_port, fNameLookup && !HaveNameProxy(), 256)}; + std::vector resolved{Lookup(pszDest, default_port, fNameLookup && !HaveNameProxy(), 256)}; if (!resolved.empty()) { - const CService& rnd{resolved[GetRand(resolved.size())]}; - addrConnect = CAddress{MaybeFlipIPv6toCJDNS(rnd), NODE_NONE}; - if (!addrConnect.IsValid()) { - LogPrint(BCLog::NET, "Resolver returned invalid address %s for %s\n", addrConnect.ToStringAddrPort(), pszDest); - return nullptr; - } - // 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) { - LogPrintf("Failed to open new connection, already connected\n"); - return nullptr; + Shuffle(resolved.begin(), resolved.end(), FastRandomContext()); + // If the connection is made by name, it can be the case that the name resolves to more than one address. + // We don't want to connect any more of them if we are already connected to one + for (const auto& r : resolved) { + addrConnect = CAddress{MaybeFlipIPv6toCJDNS(r), NODE_NONE}; + if (!addrConnect.IsValid()) { + LogPrint(BCLog::NET, "Resolver returned invalid address %s for %s\n", addrConnect.ToStringAddrPort(), pszDest); + return nullptr; + } + // 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) { + LogPrintf("Not opening a connection to %s, already connected to %s\n", pszDest, addrConnect.ToStringAddrPort()); + return nullptr; + } } } } @@ -3587,7 +3591,7 @@ std::vector CConnman::GetCurrentBlockRelayOnlyConns() const return ret; } -std::vector CConnman::GetAddedNodeInfo() const +std::vector CConnman::GetAddedNodeInfo(bool include_connected) const { std::vector ret; @@ -3622,6 +3626,9 @@ std::vector CConnman::GetAddedNodeInfo() const // strAddNode is an IP:port auto it = mapConnected.find(service); if (it != mapConnected.end()) { + if (!include_connected) { + continue; + } addedNode.resolvedAddress = service; addedNode.fConnected = true; addedNode.fInbound = it->second; @@ -3630,6 +3637,9 @@ std::vector CConnman::GetAddedNodeInfo() const // strAddNode is a name auto it = mapConnectedByName.find(addr.m_added_node); if (it != mapConnectedByName.end()) { + if (!include_connected) { + continue; + } addedNode.resolvedAddress = it->second.second; addedNode.fConnected = true; addedNode.fInbound = it->second.first; @@ -3648,21 +3658,19 @@ void CConnman::ThreadOpenAddedConnections() while (true) { CSemaphoreGrant grant(*semAddnode); - std::vector vInfo = GetAddedNodeInfo(); + std::vector vInfo = GetAddedNodeInfo(/*include_connected=*/false); bool tried = false; for (const AddedNodeInfo& info : vInfo) { - if (!info.fConnected) { - if (!grant) { - // If we've used up our semaphore and need a new one, let's not wait here since while we are waiting - // the addednodeinfo state might change. - break; - } - tried = true; - CAddress addr(CService(), NODE_NONE); - OpenNetworkConnection(addr, false, std::move(grant), info.m_params.m_added_node.c_str(), ConnectionType::MANUAL, info.m_params.m_use_v2transport); - if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) return; - grant = CSemaphoreGrant(*semAddnode, /*fTry=*/true); + if (!grant) { + // If we've used up our semaphore and need a new one, let's not wait here since while we are waiting + // the addednodeinfo state might change. + break; } + tried = true; + CAddress addr(CService(), NODE_NONE); + OpenNetworkConnection(addr, false, std::move(grant), info.m_params.m_added_node.c_str(), ConnectionType::MANUAL, info.m_params.m_use_v2transport); + if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) return; + grant = CSemaphoreGrant(*semAddnode, /*fTry=*/true); } // See if any reconnections are desired. PerformReconnections(); @@ -4549,9 +4557,12 @@ std::vector CConnman::GetAddresses(CNode& requestor, size_t max_addres bool CConnman::AddNode(const AddedNodeParams& add) { + const CService resolved(LookupNumeric(add.m_added_node, Params().GetDefaultPort(add.m_added_node))); + const bool resolved_is_valid{resolved.IsValid()}; + LOCK(m_added_nodes_mutex); for (const auto& it : m_added_node_params) { - if (add.m_added_node == it.m_added_node) return false; + if (add.m_added_node == it.m_added_node || (resolved_is_valid && resolved == LookupNumeric(it.m_added_node, Params().GetDefaultPort(it.m_added_node)))) return false; } m_added_node_params.push_back(add); diff --git a/src/net.h b/src/net.h index dfad5aa462..245e5bc0db 100644 --- a/src/net.h +++ b/src/net.h @@ -1481,7 +1481,7 @@ public: 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); - std::vector GetAddedNodeInfo() const EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex); + std::vector GetAddedNodeInfo(bool include_connected) const EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex); /** * Attempts to open a connection. Currently only used from tests. diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 491c0af391..ecb1441ee6 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -498,7 +498,7 @@ static RPCHelpMan getaddednodeinfo() const NodeContext& node = EnsureAnyNodeContext(request.context); const CConnman& connman = EnsureConnman(node);; - std::vector vInfo = connman.GetAddedNodeInfo(); + std::vector vInfo = connman.GetAddedNodeInfo(/*include_connected=*/true); if (!request.params[0].isNull()) { bool found = false; diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index e82b40532c..6fa704323f 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -124,7 +124,7 @@ FUZZ_TARGET_INIT(connman, initialize_connman) connman.SetTryNewOutboundPeer(fuzzed_data_provider.ConsumeBool()); }); } - (void)connman.GetAddedNodeInfo(); + (void)connman.GetAddedNodeInfo(fuzzed_data_provider.ConsumeBool()); (void)connman.GetExtraFullOutboundCount(); (void)connman.GetLocalServices(); assert(connman.GetMaxOutboundTarget() == max_outbound_limit); diff --git a/src/test/net_peer_connection_tests.cpp b/src/test/net_peer_connection_tests.cpp new file mode 100644 index 0000000000..ef87fc4045 --- /dev/null +++ b/src/test/net_peer_connection_tests.cpp @@ -0,0 +1,149 @@ +// Copyright (c) 2023-present The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include + +#include + +struct LogIPsTestingSetup : public TestingSetup { + LogIPsTestingSetup() + : TestingSetup{CBaseChainParams::MAIN, /*extra_args=*/{"-logips"}} {} +}; + +BOOST_FIXTURE_TEST_SUITE(net_peer_connection_tests, LogIPsTestingSetup) + +static CService ip(uint32_t i) +{ + struct in_addr s; + s.s_addr = i; + return CService{CNetAddr{s}, Params().GetDefaultPort()}; +} + +/** Create a peer and connect to it. If the optional `address` (IP/CJDNS only) isn't passed, a random address is created. */ +static void AddPeer(NodeId& id, std::vector& nodes, PeerManager& peerman, ConnmanTestMsg& connman, ConnectionType conn_type, bool onion_peer = false, std::optional address = std::nullopt) +{ + CAddress addr{}; + + if (address.has_value()) { + addr = CAddress{MaybeFlipIPv6toCJDNS(LookupNumeric(address.value(), Params().GetDefaultPort())), NODE_NONE}; + } else if (onion_peer) { + auto tor_addr{g_insecure_rand_ctx.randbytes(ADDR_TORV3_SIZE)}; + BOOST_REQUIRE(addr.SetSpecial(OnionToString(tor_addr))); + } + + while (!addr.IsLocal() && !addr.IsRoutable()) { + addr = CAddress{ip(g_insecure_rand_ctx.randbits(32)), NODE_NONE}; + } + + BOOST_REQUIRE(addr.IsValid()); + + const bool inbound_onion{onion_peer && conn_type == ConnectionType::INBOUND}; + + nodes.emplace_back(new CNode{++id, + /*sock=*/nullptr, + addr, + /*nKeyedNetGroupIn=*/0, + /*nLocalHostNonceIn=*/0, + CAddress{}, + /*addrNameIn=*/"", + conn_type, + /*inbound_onion=*/inbound_onion}); + CNode& node = *nodes.back(); + node.SetCommonVersion(PROTOCOL_VERSION); + + peerman.InitializeNode(node, ServiceFlags(NODE_NETWORK)); + node.fSuccessfullyConnected = true; + + connman.AddTestNode(node); +} + +BOOST_AUTO_TEST_CASE(test_addnode_getaddednodeinfo_and_connection_detection) +{ + const auto& chainparams = Params(); + auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman); + auto peerman = PeerManager::make(chainparams, *connman, *m_node.addrman, nullptr, + *m_node.chainman, *m_node.mempool, *m_node.mn_metaman, *m_node.mn_sync, + *m_node.govman, *m_node.sporkman, /* mn_activeman = */ nullptr, m_node.dmnman, + m_node.cj_ctx, m_node.llmq_ctx, /* ignore_incoming_txs = */ false); + NodeId id{0}; + std::vector nodes; + + // Connect a localhost peer. + { + ASSERT_DEBUG_LOG("Added connection to 127.0.0.1:9999 peer=1"); + AddPeer(id, nodes, *peerman, *connman, ConnectionType::MANUAL, /*onion_peer=*/false, /*address=*/"127.0.0.1"); + BOOST_REQUIRE(nodes.back() != nullptr); + } + + // Call ConnectNode(), which is also called by RPC addnode onetry, for a localhost + // address that resolves to multiple IPs, including that of the connected peer. + // The connection attempt should consistently fail due to the check in ConnectNode(). + for (int i = 0; i < 10; ++i) { + ASSERT_DEBUG_LOG("Not opening a connection to localhost, already connected to 127.0.0.1:9999"); + BOOST_CHECK(!connman->ConnectNodePublic(*peerman, "localhost", ConnectionType::MANUAL)); + } + + // Add 3 more peer connections. + AddPeer(id, nodes, *peerman, *connman, ConnectionType::OUTBOUND_FULL_RELAY); + AddPeer(id, nodes, *peerman, *connman, ConnectionType::BLOCK_RELAY, /*onion_peer=*/true); + AddPeer(id, nodes, *peerman, *connman, ConnectionType::INBOUND); + + BOOST_TEST_MESSAGE("Call AddNode() for all the peers"); + for (auto node : connman->TestNodes()) { + BOOST_CHECK(connman->AddNode({/*m_added_node=*/node->addr.ToStringAddrPort(), /*m_use_v2transport=*/true})); + BOOST_TEST_MESSAGE(strprintf("peer id=%s addr=%s", node->GetId(), node->addr.ToStringAddrPort())); + } + + BOOST_TEST_MESSAGE("\nCall AddNode() with 2 addrs resolving to existing localhost addnode entry; neither should be added"); + BOOST_CHECK(!connman->AddNode({/*m_added_node=*/"127.0.0.1", /*m_use_v2transport=*/true})); + BOOST_CHECK(!connman->AddNode({/*m_added_node=*/"127.1", /*m_use_v2transport=*/true})); + + BOOST_TEST_MESSAGE("\nExpect GetAddedNodeInfo to return expected number of peers with `include_connected` true/false"); + BOOST_CHECK_EQUAL(connman->GetAddedNodeInfo(/*include_connected=*/true).size(), nodes.size()); + BOOST_CHECK(connman->GetAddedNodeInfo(/*include_connected=*/false).empty()); + + BOOST_TEST_MESSAGE("\nPrint GetAddedNodeInfo contents:"); + for (const auto& info : connman->GetAddedNodeInfo(/*include_connected=*/true)) { + BOOST_TEST_MESSAGE(strprintf("\nadded node: %s", info.m_params.m_added_node)); + BOOST_TEST_MESSAGE(strprintf("connected: %s", info.fConnected)); + if (info.fConnected) { + BOOST_TEST_MESSAGE(strprintf("IP address: %s", info.resolvedAddress.ToStringAddrPort())); + BOOST_TEST_MESSAGE(strprintf("direction: %s", info.fInbound ? "inbound" : "outbound")); + } + } + + BOOST_TEST_MESSAGE("\nCheck that all connected peers are correctly detected as connected"); + for (auto node : connman->TestNodes()) { + BOOST_CHECK(connman->AlreadyConnectedPublic(node->addr)); + } + + // Clean up + for (auto node : connman->TestNodes()) { + peerman->FinalizeNode(*node); + } + connman->ClearTestNodes(); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index ee29613fa0..de8eac2523 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -4,11 +4,15 @@ #include -#include -#include #include #include +#include #include +#include +#include +#include +#include +#include #include #include @@ -98,6 +102,17 @@ bool ConnmanTestMsg::ReceiveMsgFrom(CNode& node, CSerializedNetMsg&& ser_msg) co return complete; } +CNode* ConnmanTestMsg::ConnectNodePublic(PeerManager& peerman, const char* pszDest, ConnectionType conn_type) +{ + CNode* node = ConnectNode(CAddress{}, pszDest, /*fCountFailure=*/false, conn_type, /*use_v2transport=*/true); + if (!node) return nullptr; + node->SetCommonVersion(PROTOCOL_VERSION); + peerman.InitializeNode(*node, ServiceFlags(NODE_NETWORK)); + node->fSuccessfullyConnected = true; + AddTestNode(*node); + return node; +} + std::vector GetRandomNodeEvictionCandidates(int n_candidates, FastRandomContext& random_context) { std::vector candidates; diff --git a/src/test/util/net.h b/src/test/util/net.h index ef2f3df2c1..4c2be00e49 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -6,16 +6,30 @@ #define BITCOIN_TEST_UTIL_NET_H #include -#include -#include #include +#include +#include +#include +#include +#include +#include #include +#include #include #include +#include +#include #include #include #include +#include +#include + +class FastRandomContext; + +template +class Span; struct ConnmanTestMsg : public CConnman { using CConnman::CConnman; @@ -25,6 +39,12 @@ struct ConnmanTestMsg : public CConnman { m_peer_connect_timeout = timeout; } + std::vector TestNodes() + { + LOCK(m_nodes_mutex); + return m_nodes; + } + void AddTestNode(CNode& node) { LOCK(m_nodes_mutex); @@ -56,6 +76,11 @@ 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); }; + + CNode* ConnectNodePublic(PeerManager& peerman, const char* pszDest, ConnectionType conn_type) + EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex); }; constexpr ServiceFlags ALL_SERVICE_FLAGS[]{ diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index 5cc1dc617b..47787b633a 100755 --- a/test/functional/rpc_net.py +++ b/test/functional/rpc_net.py @@ -239,8 +239,11 @@ class NetTest(DashTestFramework): # add a node (node2) to node0 ip_port = "127.0.0.1:{}".format(p2p_port(2)) self.nodes[0].addnode(node=ip_port, command='add') + # try to add an equivalent ip + ip_port2 = "127.1:{}".format(p2p_port(2)) + assert_raises_rpc_error(-23, "Node already added", self.nodes[0].addnode, node=ip_port2, command='add') # check that the node has indeed been added - added_nodes = self.nodes[0].getaddednodeinfo(ip_port) + added_nodes = self.nodes[0].getaddednodeinfo() assert_equal(len(added_nodes), 1) assert_equal(added_nodes[0]['addednode'], ip_port) # check that node cannot be added again