merge bitcoin#28155: improves addnode / m_added_nodes logic

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 \

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<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())]};
+            Shuffle(resolved.begin(), resolved.end(), FastRandomContext());
-            addrConnect = CAddress{MaybeFlipIPv6toCJDNS(rnd), NODE_NONE};
+            // If the connection is made by name, it can be the case that the name resolves to more than one address.
-            if (!addrConnect.IsValid()) {
+            // We don't want to connect any more of them if we are already connected to one
-                LogPrint(BCLog::NET, "Resolver returned invalid address %s for %s\n", addrConnect.ToStringAddrPort(), pszDest);
+            for (const auto& r : resolved) {
-                return nullptr;
+                addrConnect = CAddress{MaybeFlipIPv6toCJDNS(r), NODE_NONE};
-            }
+                if (!addrConnect.IsValid()) {
-            // It is possible that we already have a connection to the IP/port pszDest resolved to.
+                    LogPrint(BCLog::NET, "Resolver returned invalid address %s for %s\n", addrConnect.ToStringAddrPort(), pszDest);
-            // In that case, drop the connection that was just created.
+                    return nullptr;
-            LOCK(m_nodes_mutex);
+                }
-            CNode* pnode = FindNode(static_cast<CService>(addrConnect));
+                // It is possible that we already have a connection to the IP/port pszDest resolved to.
-            if (pnode) {
+                // In that case, drop the connection that was just created.
-                LogPrintf("Failed to open new connection, already connected\n");
+                LOCK(m_nodes_mutex);
-                return nullptr;
+                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;
+                }
             }
         }
     }
@@ -3587,7 +3591,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;
 
@@ -3622,6 +3626,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;
@@ -3630,6 +3637,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;
@@ -3648,21 +3658,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 (!grant) {
+                // If we've used up our semaphore and need a new one, let's not wait here since while we are waiting
-                    // 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.
-                    // the addednodeinfo state might change.
+                break;
-                    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);
             }
+            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<CAddress> 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);

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

@@ -498,7 +498,7 @@ static RPCHelpMan getaddednodeinfo()
     const 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

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

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 <chainparams.h>
+#include <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/setup_common.h>
+#include <tinyformat.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{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<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.fSuccessfullyConnected = true;
+
+    connman.AddTestNode(node);
+}
+
+BOOST_AUTO_TEST_CASE(test_addnode_getaddednodeinfo_and_connection_detection)
+{
+    const auto& chainparams = Params();
+    auto connman = std::make_unique<ConnmanTestMsg>(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<CNode*> 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()

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

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