Merge #8113: Rework addnode behaviour

1a5a4e6 Randomize name lookup result in ConnectSocketByName (Pieter Wuille)
f9f5cfc Prevent duplicate connections where one is by name and another by ip (Pieter Wuille)
1111b80 Rework addnode behaviour (Pieter Wuille)

src/net.cpp

@@ -400,6 +400,26 @@ CNode* ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure
             return NULL;
         }
 
+        if (pszDest && addrConnect.IsValid()) {
+            // 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, and return the existing CNode instead.
+            // Also store the name we used to connect in that CNode, so that future FindNode() calls to that
+            // name catch this early.
+            CNode* pnode = FindNode((CService)addrConnect);
+            if (pnode)
+            {
+                pnode->AddRef();
+                {
+                    LOCK(cs_vNodes);
+                    if (pnode->addrName.empty()) {
+                        pnode->addrName = std::string(pszDest);
+                    }
+                }
+                CloseSocket(hSocket);
+                return pnode;
+            }
+        }
+
         addrman.Attempt(addrConnect, fCountFailure);
 
         // Add node
@@ -1659,68 +1679,79 @@ void ThreadOpenConnections()
     }
 }
 
-void ThreadOpenAddedConnections()
+std::vector<AddedNodeInfo> GetAddedNodeInfo()
 {
-    {
+    std::vector<AddedNodeInfo> ret;
-        LOCK(cs_vAddedNodes);
-        vAddedNodes = mapMultiArgs["-addnode"];
-    }
 
-    if (HaveNameProxy()) {
-        while(true) {
     std::list<std::string> lAddresses(0);
     {
         LOCK(cs_vAddedNodes);
+        ret.reserve(vAddedNodes.size());
         BOOST_FOREACH(const std::string& strAddNode, vAddedNodes)
             lAddresses.push_back(strAddNode);
     }
-            BOOST_FOREACH(const std::string& strAddNode, lAddresses) {
+
-                CAddress addr;
+
-                CSemaphoreGrant grant(*semOutbound);
+    // Build a map of all already connected addresses (by IP:port and by name) to inbound/outbound and resolved CService
-                OpenNetworkConnection(addr, false, &grant, strAddNode.c_str());
+    std::map<CService, bool> mapConnected;
-                MilliSleep(500);
+    std::map<std::string, std::pair<bool, CService>> mapConnectedByName;
+    {
+        LOCK(cs_vNodes);
+        for (const CNode* pnode : vNodes) {
+            if (pnode->addr.IsValid()) {
+                mapConnected[pnode->addr] = pnode->fInbound;
             }
-            MilliSleep(120000); // Retry every 2 minutes
+            if (!pnode->addrName.empty()) {
+                mapConnectedByName[pnode->addrName] = std::make_pair(pnode->fInbound, static_cast<const CService&>(pnode->addr));
             }
         }
-
-    for (unsigned int i = 0; true; i++)
-    {
-        std::list<std::string> lAddresses(0);
-        {
-            LOCK(cs_vAddedNodes);
-            BOOST_FOREACH(const std::string& strAddNode, vAddedNodes)
-                lAddresses.push_back(strAddNode);
     }
 
-        std::list<std::vector<CService> > lservAddressesToAdd(0);
     BOOST_FOREACH(const std::string& strAddNode, lAddresses) {
-            std::vector<CService> vservNode(0);
+        CService service(strAddNode, Params().GetDefaultPort());
-            if(Lookup(strAddNode.c_str(), vservNode, Params().GetDefaultPort(), fNameLookup, 0))
+        if (service.IsValid()) {
-                lservAddressesToAdd.push_back(vservNode);
+            // strAddNode is an IP:port
+            auto it = mapConnected.find(service);
+            if (it != mapConnected.end()) {
+                ret.push_back(AddedNodeInfo{strAddNode, service, true, it->second});
+            } else {
+                ret.push_back(AddedNodeInfo{strAddNode, CService(), false, false});
+            }
+        } else {
+            // strAddNode is a name
+            auto it = mapConnectedByName.find(strAddNode);
+            if (it != mapConnectedByName.end()) {
+                ret.push_back(AddedNodeInfo{strAddNode, it->second.second, true, it->second.first});
+            } else {
+                ret.push_back(AddedNodeInfo{strAddNode, CService(), false, false});
             }
-        // Attempt to connect to each IP for each addnode entry until at least one is successful per addnode entry
-        // (keeping in mind that addnode entries can have many IPs if fNameLookup)
-        {
-            LOCK(cs_vNodes);
-            BOOST_FOREACH(CNode* pnode, vNodes)
-                for (std::list<std::vector<CService> >::iterator it = lservAddressesToAdd.begin(); it != lservAddressesToAdd.end(); it++)
-                    BOOST_FOREACH(const CService& addrNode, *(it))
-                        if (pnode->addr == addrNode)
-                        {
-                            it = lservAddressesToAdd.erase(it);
-                            it--;
-                            break;
         }
     }
-        BOOST_FOREACH(std::vector<CService>& vserv, lservAddressesToAdd)
+
+    return ret;
+}
+
+void ThreadOpenAddedConnections()
+{
+    {
+        LOCK(cs_vAddedNodes);
+        vAddedNodes = mapMultiArgs["-addnode"];
+    }
+
+    for (unsigned int i = 0; true; i++)
     {
+        std::vector<AddedNodeInfo> vInfo = GetAddedNodeInfo();
+        for (const AddedNodeInfo& info : vInfo) {
+            if (!info.fConnected) {
                 CSemaphoreGrant grant(*semOutbound);
-            /* We want -addnode to work even for nodes that don't provide all
+                // If strAddedNode is an IP/port, decode it immediately, so
-             * wanted services, so pass in nServices=NODE_NONE to CAddress. */
+                // OpenNetworkConnection can detect existing connections to that IP/port.
-            OpenNetworkConnection(CAddress(vserv[i % vserv.size()], NODE_NONE), false, &grant);
+                CService service(info.strAddedNode, Params().GetDefaultPort());
+                OpenNetworkConnection(CAddress(service, NODE_NONE), false, &grant, info.strAddedNode.c_str(), false);
                 MilliSleep(500);
             }
+        }
+
         MilliSleep(120000); // Retry every 2 minutes
     }
 }

src/net.h

@@ -823,4 +823,14 @@ public:
 /** Return a timestamp in the future (in microseconds) for exponentially distributed events. */
 int64_t PoissonNextSend(int64_t nNow, int average_interval_seconds);
 
+struct AddedNodeInfo
+{
+    std::string strAddedNode;
+    CService resolvedAddress;
+    bool fConnected;
+    bool fInbound;
+};
+
+std::vector<AddedNodeInfo> GetAddedNodeInfo();
+
 #endif // BITCOIN_NET_H

src/netbase.cpp

@@ -621,10 +621,10 @@ bool ConnectSocketByName(CService &addr, SOCKET& hSocketRet, const char *pszDest
     proxyType nameProxy;
     GetNameProxy(nameProxy);
 
-    CService addrResolved;
+    std::vector<CService> addrResolved;
-    if (Lookup(strDest.c_str(), addrResolved, port, fNameLookup && !HaveNameProxy())) {
+    if (Lookup(strDest.c_str(), addrResolved, port, fNameLookup && !HaveNameProxy(), 256)) {
-        if (addrResolved.IsValid()) {
+        if (addrResolved.size() > 0) {
-            addr = addrResolved;
+            addr = addrResolved[GetRand(addrResolved.size())];
             return ConnectSocket(addr, hSocketRet, nTimeout);
         }
     }

src/rpc/net.cpp

@@ -271,25 +271,22 @@ UniValue getaddednodeinfo(const UniValue& params, bool fHelp)
 {
     if (fHelp || params.size() < 1 || params.size() > 2)
         throw runtime_error(
-            "getaddednodeinfo dns ( \"node\" )\n"
+            "getaddednodeinfo dummy ( \"node\" )\n"
             "\nReturns information about the given added node, or all added nodes\n"
             "(note that onetry addnodes are not listed here)\n"
-            "If dns is false, only a list of added nodes will be provided,\n"
-            "otherwise connected information will also be available.\n"
             "\nArguments:\n"
-            "1. dns        (boolean, required) If false, only a list of added nodes will be provided, otherwise connected information will also be available.\n"
+            "1. dummy      (boolean, required) Kept for historical purposes but ignored\n"
             "2. \"node\"   (string, optional) If provided, return information about this specific node, otherwise all nodes are returned.\n"
             "\nResult:\n"
             "[\n"
             "  {\n"
-            "    \"addednode\" : \"192.168.0.201\",   (string) The node ip address\n"
+            "    \"addednode\" : \"192.168.0.201\",   (string) The node ip address or name (as provided to addnode)\n"
             "    \"connected\" : true|false,          (boolean) If connected\n"
-            "    \"addresses\" : [\n"
+            "    \"addresses\" : [                    (list of objects) Only when connected = true\n"
             "       {\n"
-            "         \"address\" : \"192.168.0.201:8333\",  (string) The bitcoin server host and port\n"
+            "         \"address\" : \"192.168.0.201:8333\",  (string) The bitcoin server IP and port we're connected to\n"
             "         \"connected\" : \"outbound\"           (string) connection, inbound or outbound\n"
             "       }\n"
-            "       ,...\n"
             "     ]\n"
             "  }\n"
             "  ,...\n"
@@ -300,83 +297,35 @@ UniValue getaddednodeinfo(const UniValue& params, bool fHelp)
             + HelpExampleRpc("getaddednodeinfo", "true, \"192.168.0.201\"")
         );
 
-    bool fDns = params[0].get_bool();
+    std::vector<AddedNodeInfo> vInfo = GetAddedNodeInfo();
 
-    list<string> laddedNodes(0);
+    if (params.size() == 2) {
-    if (params.size() == 1)
+        bool found = false;
-    {
+        for (const AddedNodeInfo& info : vInfo) {
-        LOCK(cs_vAddedNodes);
+            if (info.strAddedNode == params[1].get_str()) {
-        BOOST_FOREACH(const std::string& strAddNode, vAddedNodes)
+                vInfo.assign(1, info);
-            laddedNodes.push_back(strAddNode);
+                found = true;
-    }
-    else
-    {
-        string strNode = params[1].get_str();
-        LOCK(cs_vAddedNodes);
-        BOOST_FOREACH(const std::string& strAddNode, vAddedNodes) {
-            if (strAddNode == strNode)
-            {
-                laddedNodes.push_back(strAddNode);
                 break;
             }
         }
-        if (laddedNodes.size() == 0)
+        if (!found) {
             throw JSONRPCError(RPC_CLIENT_NODE_NOT_ADDED, "Error: Node has not been added.");
         }
-
-    UniValue ret(UniValue::VARR);
-    if (!fDns)
-    {
-        BOOST_FOREACH (const std::string& strAddNode, laddedNodes) {
-            UniValue obj(UniValue::VOBJ);
-            obj.push_back(Pair("addednode", strAddNode));
-            ret.push_back(obj);
-        }
-        return ret;
     }
 
-    list<pair<string, vector<CService> > > laddedAddreses(0);
+    UniValue ret(UniValue::VARR);
-    BOOST_FOREACH(const std::string& strAddNode, laddedNodes) {
-        vector<CService> vservNode(0);
-        if(Lookup(strAddNode.c_str(), vservNode, Params().GetDefaultPort(), fNameLookup, 0))
-            laddedAddreses.push_back(make_pair(strAddNode, vservNode));
-        else
-        {
-            UniValue obj(UniValue::VOBJ);
-            obj.push_back(Pair("addednode", strAddNode));
-            obj.push_back(Pair("connected", false));
-            UniValue addresses(UniValue::VARR);
-            obj.push_back(Pair("addresses", addresses));
-            ret.push_back(obj);
-        }
-    }
 
-    LOCK(cs_vNodes);
+    for (const AddedNodeInfo& info : vInfo) {
-    for (list<pair<string, vector<CService> > >::iterator it = laddedAddreses.begin(); it != laddedAddreses.end(); it++)
-    {
         UniValue obj(UniValue::VOBJ);
-        obj.push_back(Pair("addednode", it->first));
+        obj.push_back(Pair("addednode", info.strAddedNode));
-
+        obj.push_back(Pair("connected", info.fConnected));
         UniValue addresses(UniValue::VARR);
-        bool fConnected = false;
+        if (info.fConnected) {
-        BOOST_FOREACH(const CService& addrNode, it->second) {
+            UniValue address(UniValue::VOBJ);
-            bool fFound = false;
+            address.push_back(Pair("address", info.resolvedAddress.ToString()));
-            UniValue node(UniValue::VOBJ);
+            address.push_back(Pair("connected", info.fInbound ? "inbound" : "outbound"));
-            node.push_back(Pair("address", addrNode.ToString()));
+            addresses.push_back(address);
-            BOOST_FOREACH(CNode* pnode, vNodes) {
-                if (pnode->addr == addrNode)
-                {
-                    fFound = true;
-                    fConnected = true;
-                    node.push_back(Pair("connected", pnode->fInbound ? "inbound" : "outbound"));
-                    break;
-                }
-            }
-            if (!fFound)
-                node.push_back(Pair("connected", "false"));
-            addresses.push_back(node);
         }
-        obj.push_back(Pair("connected", fConnected));
         obj.push_back(Pair("addresses", addresses));
         ret.push_back(obj);
     }