Merge bitcoin/bitcoin#28155: net: improves addnode / m_added_nodes logic

0420f99f42 Create net_peer_connection unit tests (Jon Atack)
4b834f6499 Allow unit tests to access additional CConnman members (Jon Atack)
34b9ef443b net/rpc: Makes CConnman::GetAddedNodeInfo able to return only non-connected address on request (Sergi Delgado Segura)
94e8882d82 rpc: Prevents adding the same ip more than once when formatted differently (Sergi Delgado Segura)
2574b7e177 net/rpc: Check all resolved addresses in ConnectNode rather than just one (Sergi Delgado Segura)

Pull request description:

  ## Rationale

  Currently, `addnode` has a couple of corner cases that allow it to either connect to the same peer more than once, hence wasting outbound connection slots, or add redundant information to `m_added_nodes`, hence making Bitcoin iterate through useless data on a regular basis.

  ### Connecting to the same node more than once

  In general, connecting to the same node more than once is something we should try to prevent. Currently, this is possible via `addnode` in two different ways:

  1. Calling `addnode` more than once in a short time period, using two equivalent but distinct addresses
  2. Calling `addnode add` using an IP, and `addnode onetry` after with an address that resolved to the same IP

  For the former, the issue boils down to `CConnman::ThreadOpenAddedConnections` calling `CConnman::GetAddedNodeInfo` once, and iterating over the result to open connections (`CConman::OpenNetworkConnection`) on the same loop for all addresses.`CConnman::ConnectNode` only checks a single address, at random, when resolving from a hostname, and uses it to check whether we are already connected to it.

  An example to test this would be calling:

  ```
  bitcoin-cli addnode "127.0.0.1:port" add
  bitcoin-cli addnode "localhost:port" add
  ```

  And check how it allows us to perform both connections some times, and some times it fails.

  The latter boils down to the same issue, but takes advantage of `onetry` bypassing the `CConnman::ThreadOpenAddedConnections` logic and calling `CConnman::OpenNetworkConnection` straightaway. A way to test this would be:

  ```
  bitcoin-cli addnode "127.0.0.1:port" add
  bitcoin-cli addnode "localhost:port" onetry
  ```

  ### Adding the same peer with two different, yet equivalent, addresses

  The current implementation of `addnode` is pretty naive when checking what data is added to `m_added_nodes`. Given the collection stores strings, the checks at `CConnman::AddNode()` basically check wether the exact provided string is already in the collection. If so, the data is rejected, otherwise, it is accepted. However, ips can be formatted in several ways that would bypass those checks.

  Two examples would be `127.0.0.1` being equal to `127.1` and `[::1]` being equal to `[0:0:0:0:0:0:0:1]`. Adding any pair of these will be allowed by the rpc command, and both will be reported as connected by `getaddednodeinfo`, given they map to the same `CService`.

  This is less severe than the previous issue, since even tough both nodes are reported as connected by `getaddednodeinfo`, there is only a single connection to them (as properly reported by `getpeerinfo`). However, this adds redundant data to `m_added_nodes`, which is undesirable.

  ### Parametrize `CConnman::GetAddedNodeInfo`
  Finally, this PR also parametrizes `CConnman::GetAddedNodeInfo` so it returns either all added nodes info, or only info about the nodes we are **not** connected to. This method is used both for `rpc`, in `getaddednodeinfo`, in which we are reporting all data to the user, so the former applies, and to check what nodes we are not connected to, in `CConnman::ThreadOpenAddedConnections`, in which we are currently returning more data than needed and then actively filtering using `CService.fConnected()`

ACKs for top commit:
  jonatack:
    re-ACK 0420f99f42
  kashifs:
    > > tACK [0420f9](0420f99f42)
  sr-gi:
    > > > tACK [0420f9](0420f99f42)
  mzumsande:
    Tested ACK 0420f99f42

Tree-SHA512: a3a10e748c12d98d439dfb193c75bc8d9486717cda5f41560f5c0ace1baef523d001d5e7eabac9fa466a9159a30bb925cc1327c2d6c4efb89dcaf54e176d1752

src/Makefile.test.include

@@ -112,6 +112,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 \

src/net.cpp

@@ -417,21 +417,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<CService> resolved{Lookup(pszDest, default_port, fNameLookup && !HaveNameProxy(), 256)};
+        std::vector<CService> 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<CService>(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<CService>(addrConnect));
+                if (pnode) {
+                    LogPrintf("Not opening a connection to %s, already connected to %s\n", pszDest, addrConnect.ToStringAddrPort());
+                    return nullptr;
+                }
             }
         }
     }
@@ -2753,7 +2757,7 @@ std::vector<CAddress> CConnman::GetCurrentBlockRelayOnlyConns() const
     return ret;
 }
 
-std::vector<AddedNodeInfo> CConnman::GetAddedNodeInfo() const
+std::vector<AddedNodeInfo> CConnman::GetAddedNodeInfo(bool include_connected) const
 {
     std::vector<AddedNodeInfo> ret;
 
@@ -2788,6 +2792,9 @@ std::vector<AddedNodeInfo> 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;
@@ -2796,6 +2803,9 @@ std::vector<AddedNodeInfo> 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;
@@ -2814,21 +2824,19 @@ void CConnman::ThreadOpenAddedConnections()
     while (true)
     {
         CSemaphoreGrant grant(*semAddnode);
-        std::vector<AddedNodeInfo> vInfo = GetAddedNodeInfo();
+        std::vector<AddedNodeInfo> 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)))
@@ -3424,9 +3432,12 @@ std::vector<CAddress> 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);

src/net.h

@@ -1184,7 +1184,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<AddedNodeInfo> GetAddedNodeInfo() const EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex);
+    std::vector<AddedNodeInfo> GetAddedNodeInfo(bool include_connected) const EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex);
 
     /**
      * Attempts to open a connection. Currently only used from tests.

src/rpc/net.cpp

@@ -500,7 +500,7 @@ static RPCHelpMan getaddednodeinfo()
     NodeContext& node = EnsureAnyNodeContext(request.context);
     const CConnman& connman = EnsureConnman(node);
 
-    std::vector<AddedNodeInfo> vInfo = connman.GetAddedNodeInfo();
+    std::vector<AddedNodeInfo> vInfo = connman.GetAddedNodeInfo(/*include_connected=*/true);
 
     if (!request.params[0].isNull()) {
         bool found = false;

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();

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 <chainparams.h>
+#include <compat/compat.h>
+#include <net.h>
+#include <net_processing.h>
+#include <netaddress.h>
+#include <netbase.h>
+#include <netgroup.h>
+#include <node/connection_types.h>
+#include <protocol.h>
+#include <random.h>
+#include <test/util/logging.h>
+#include <test/util/net.h>
+#include <test/util/random.h>
+#include <test/util/setup_common.h>
+#include <tinyformat.h>
+#include <util/chaintype.h>
+#include <version.h>
+
+#include <algorithm>
+#include <cstdint>
+#include <memory>
+#include <optional>
+#include <string>
+#include <vector>
+
+#include <boost/test/unit_test.hpp>
+
+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<CNode*>& nodes, PeerManager& peerman, ConnmanTestMsg& connman, ConnectionType conn_type, bool onion_peer = false, std::optional<std::string> 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<ConnmanTestMsg>(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<CNode*> 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()

src/test/util/net.cpp

@@ -4,11 +4,15 @@
 
 #include <test/util/net.h>
 
-#include <chainparams.h>
-#include <node/eviction.h>
 #include <net.h>
 #include <net_processing.h>
+#include <netaddress.h>
 #include <netmessagemaker.h>
+#include <node/connection_types.h>
+#include <node/eviction.h>
+#include <protocol.h>
+#include <random.h>
+#include <serialize.h>
 #include <span.h>
 
 #include <vector>
@@ -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<NodeEvictionCandidate> GetRandomNodeEvictionCandidates(int n_candidates, FastRandomContext& random_context)
 {
     std::vector<NodeEvictionCandidate> candidates;

src/test/util/net.h

@@ -6,16 +6,30 @@
 #define BITCOIN_TEST_UTIL_NET_H
 
 #include <compat/compat.h>
-#include <node/eviction.h>
-#include <netaddress.h>
 #include <net.h>
+#include <net_permissions.h>
+#include <net_processing.h>
+#include <netaddress.h>
+#include <node/connection_types.h>
+#include <node/eviction.h>
+#include <sync.h>
 #include <util/sock.h>
 
+#include <algorithm>
 #include <array>
 #include <cassert>
+#include <chrono>
+#include <cstdint>
 #include <cstring>
 #include <memory>
 #include <string>
+#include <unordered_map>
+#include <vector>
+
+class FastRandomContext;
+
+template <typename C>
+class Span;
 
 struct ConnmanTestMsg : public CConnman {
     using CConnman::CConnman;
@@ -25,6 +39,12 @@ struct ConnmanTestMsg : public CConnman {
         m_peer_connect_timeout = timeout;
     }
 
+    std::vector<CNode*> 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[]{

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