From 2574b7e177ef045e64f1dd48cb000640ff5103d3 Mon Sep 17 00:00:00 2001 From: Sergi Delgado Segura Date: Tue, 25 Jul 2023 11:50:47 -0400 Subject: [PATCH 1/5] net/rpc: Check all resolved addresses in ConnectNode rather than just one The current `addnode` rpc command has some edge cases in where it is possible to connect to the same node twice by combining ip and address requests. This can happen under two situations: The two commands are run one right after each other, in which case they will be processed under the same loop in `CConnman::ThreadOpenAddedConnections` without refreshing `vInfo`, so both will go trough. An example of this would be: ``` bitcoin-cli addnode "localhost:port" add ``` A node is added by IP using `addnode "add"` while the other is added by name using `addnode "onetry"` with an address that resolves to multiple IPs. In this case, we currently only check one of the resolved IPs (picked at random), instead of all the resolved ones, meaning this will only probabilistically fail/succeed. An example of this would be: ``` bitcoin-cli addnode "127.0.0.1:port" add [...] bitcoin-cli addnode "localhost:port" onetry ``` Both cases can be fixed by iterating over all resolved addresses in `CConnman::ConnectNode` instead of picking one at random --- src/net.cpp | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 13f44304240..0d4b3859c0f 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -464,21 +464,25 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo const uint16_t default_port{pszDest != nullptr ? GetDefaultPort(pszDest) : m_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; + } } } } From 94e8882d820969ddc83f24f4cbe1515a886da4ea Mon Sep 17 00:00:00 2001 From: Sergi Delgado Segura Date: Tue, 25 Jul 2023 15:29:00 -0400 Subject: [PATCH 2/5] rpc: Prevents adding the same ip more than once when formatted differently Currently it is possible to add the same node twice when formatting IPs in different, yet equivalent, manner. This applies to both ipv4 and ipv6, e.g: 127.0.0.1 = 127.1 | [::1] = [0:0:0:0:0:0:0:1] `addnode` will accept both and display both as connected (given they translate to the same IP). This will not result in multiple connections to the same node, but will report redundant info when querying `getaddednodeinfo` and populate `m_added_nodes` with redundant data. This can be avoided performing comparing the contents of `m_added_addr` and the address to be added as `CServices` instead of as strings. --- src/net.cpp | 5 ++++- test/functional/rpc_net.py | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 0d4b3859c0f..54a1eb64770 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3471,9 +3471,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, 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, GetDefaultPort(it.m_added_node)))) return false; } m_added_node_params.push_back(add); diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index 2c7f974d0bb..19fbca7fa93 100755 --- a/test/functional/rpc_net.py +++ b/test/functional/rpc_net.py @@ -215,8 +215,11 @@ class NetTest(BitcoinTestFramework): # 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 From 34b9ef443bc2655a85c8802edc5d5d48d792a286 Mon Sep 17 00:00:00 2001 From: Sergi Delgado Segura Date: Tue, 25 Jul 2023 15:47:36 -0400 Subject: [PATCH 3/5] net/rpc: Makes CConnman::GetAddedNodeInfo able to return only non-connected address on request `CConnman::GetAddedNodeInfo` is used both to get a list of addresses to manually connect to in `CConnman::ThreadOpenAddedConnections`, and to report about manually added connections in `getaddednodeinfo`. In both cases, all addresses added to `m_added_nodes` are returned, however the nodes we are already connected to are only relevant to the latter, in the former they are actively discarded. Parametrizes `CConnman::GetAddedNodeInfo` so we can ask for only addresses we are not connected to, to avoid passing useless information around. --- src/net.cpp | 30 +++++++++++++++++------------- src/net.h | 2 +- src/rpc/net.cpp | 2 +- src/test/fuzz/connman.cpp | 2 +- 4 files changed, 20 insertions(+), 16 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 54a1eb64770..db1847808df 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2804,7 +2804,7 @@ std::vector CConnman::GetCurrentBlockRelayOnlyConns() const return ret; } -std::vector CConnman::GetAddedNodeInfo() const +std::vector CConnman::GetAddedNodeInfo(bool include_connected) const { std::vector ret; @@ -2839,6 +2839,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; @@ -2847,6 +2850,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; @@ -2865,21 +2871,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); } // Retry every 60 seconds if a connection was attempted, otherwise two seconds if (!interruptNet.sleep_for(std::chrono::seconds(tried ? 60 : 2))) diff --git a/src/net.h b/src/net.h index 2f7b832fbaa..c7f2a0355ab 100644 --- a/src/net.h +++ b/src/net.h @@ -1201,7 +1201,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 96d06b6b9f0..dd7e5891cd3 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -487,7 +487,7 @@ static RPCHelpMan getaddednodeinfo() 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 0dab2a2e974..551e1c526dd 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -121,7 +121,7 @@ FUZZ_TARGET(connman, .init = initialize_connman) connman.SetTryNewOutboundPeer(fuzzed_data_provider.ConsumeBool()); }); } - (void)connman.GetAddedNodeInfo(); + (void)connman.GetAddedNodeInfo(fuzzed_data_provider.ConsumeBool()); (void)connman.GetExtraFullOutboundCount(); (void)connman.GetLocalServices(); (void)connman.GetMaxOutboundTarget(); From 4b834f649921aceb44d3e0b5a2ffd7847903f9f7 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Fri, 27 Oct 2023 18:53:13 -0600 Subject: [PATCH 4/5] Allow unit tests to access additional CConnman members that are otherwise private: - CConnman::m_nodes - CConnman::ConnectNodes() - CConnman::AlreadyConnectedToAddress() and update the #include headers per iwyu. --- src/test/util/net.cpp | 19 +++++++++++++++++-- src/test/util/net.h | 29 +++++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index bf5a653090b..e0404e33ed5 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_WITNESS)); + 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 497292542be..59c4ddb4b1a 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[]{ From 0420f99f429ce2382057e101859067f40de47be0 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Thu, 26 Oct 2023 15:54:18 -0600 Subject: [PATCH 5/5] Create net_peer_connection unit tests for initial partial unit test coverage of these CConnman class methods: - AddNode() - ConnectNode() - GetAddedNodeInfo() - AlreadyConnectedToAddress() - ThreadOpenAddedConnections() and of the GetAddedNodeInfo() call in RPC addnode. --- src/Makefile.test.include | 1 + src/test/net_peer_connection_tests.cpp | 147 +++++++++++++++++++++++++ 2 files changed, 148 insertions(+) create mode 100644 src/test/net_peer_connection_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index d66f5bf53a4..d73466c3c3e 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -111,6 +111,7 @@ BITCOIN_TESTS =\ test/miniscript_tests.cpp \ test/minisketch_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/test/net_peer_connection_tests.cpp b/src/test/net_peer_connection_tests.cpp new file mode 100644 index 00000000000..3d3f296d827 --- /dev/null +++ b/src/test/net_peer_connection_tests.cpp @@ -0,0 +1,147 @@ +// 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 +#include + +#include + +struct LogIPsTestingSetup : public TestingSetup { + LogIPsTestingSetup() + : TestingSetup{ChainType::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_WITNESS)); + node.fSuccessfullyConnected = true; + + connman.AddTestNode(node); +} + +BOOST_AUTO_TEST_CASE(test_addnode_getaddednodeinfo_and_connection_detection) +{ + auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman, Params()); + auto peerman = PeerManager::make(*connman, *m_node.addrman, nullptr, *m_node.chainman, *m_node.mempool, {}); + NodeId id{0}; + std::vector nodes; + + // Connect a localhost peer. + { + ASSERT_DEBUG_LOG("Added connection to 127.0.0.1:8333 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:8333"); + 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()