net: constrain the memory usage of manually added nodes

Each node keeps a registry of manually added nodes (through the
addnode parameter, rpc call or UI) but there are currently no
limits imposed on that usage, which is a bit sloppy and can lead
to situations where memory is being used for storing addresses
that are never connected to, because the maximum number of
connections used for addnode entries is hardcoded as 8. This
could prevent smaller systems that host nodes (like those
running on an ARM SoC) to optimally use the available memory.

This enhancement limits the addnode functionality as follows:

1. Whenever over 799 nodes are added to the registry, require
   the user to remove an entry before a new one can be added
2. Disallow very large addresses (more than 256 characters).
   This limit provides for at least 4 levels of subdomains as
   specified under RFC1035.

See https://datatracker.ietf.org/doc/html/rfc1035#section-2.3.1

qa/pull-tester/rpc-tests.py

@@ -171,6 +171,7 @@ testScripts = [
     'p2p-policy.py',
     'wallet_create_tx.py',
     'liststucktransactions.py',
+    'addnode.py',
 ]
 if ENABLE_ZMQ:
     testScripts.append('zmq_test.py')

qa/rpc-tests/addnode.py

@@ -0,0 +1,67 @@
+#!/usr/bin/env python3
+# Copyright (c) 2021 The Dogecoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Addnode QA test.
+# Tests the addnode command
+"""
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import *
+
+class AddnodeTest(BitcoinTestFramework):
+
+    def __init__(self):
+        super().__init__()
+        self.setup_clean_chain = True
+        self.num_nodes = 1
+        self.counter = 0
+
+    def setup_network(self, split=False):
+        self.nodes = []
+        self.nodes.append(start_node(0, self.options.tmpdir, []))
+
+        self.is_network_split=False
+        self.sync_all()
+
+    def get_next_dummy_address(self):
+        self.counter += 1
+        assert self.counter < 256 ** 2  # Don't allow the returned ip addresses to wrap.
+        return f"123.123.{self.counter // 256}.{self.counter % 256}:{10000 + self.counter}"
+
+    def run_test(self):
+        node = self.nodes[0]
+        # mine a block
+        node.generate(1)
+
+        # add one address
+        first_addr = self.get_next_dummy_address()
+        node.addnode(first_addr, 'add')
+
+        # try adding it again
+        try:
+            node.addnode(first_addr, 'add')
+            raise AssertionError("Must reject an existing addnode")
+        except JSONRPCException as e:
+            assert("Error: Unable to add node" in e.error["message"])
+
+        # add 799 valid addresses - must not throw an error
+        for i in range((8 * 100) - 1):
+            node.addnode(self.get_next_dummy_address(), 'add')
+
+        # add one more address
+        try:
+            node.addnode(self.get_next_dummy_address(), 'add')
+            raise AssertionError("Must reject when more than 800 addnode entries exist")
+        except JSONRPCException as e:
+            assert("Error: Unable to add node" in e.error["message"])
+
+        # try adding a large string domain
+        try:
+            node.addnode(f'{"manybigstr" * 25}.domain:10020', 'add')
+            raise AssertionError("Must reject large strings")
+        except JSONRPCException as e:
+            assert("Error: Node address is invalid" in e.error["message"])
+
+if __name__ == '__main__':
+    AddnodeTest().main()

src/net.cpp

@@ -2492,6 +2492,13 @@ std::vector<CAddress> CConnman::GetAddresses()
 bool CConnman::AddNode(const std::string& strNode)
 {
     LOCK(cs_vAddedNodes);
+
+    // We only allow 100x the amount of total connections, to protect against
+    // script errors that fill up memory of the node with addresses
+    if (vAddedNodes.size() >= MAX_ADDNODE_CONNECTIONS * 100) {
+      return false;
+    }
+
     for(std::vector<std::string>::const_iterator it = vAddedNodes.begin(); it != vAddedNodes.end(); ++it) {
         if (strNode == *it)
             return false;

src/qt/peerdialog.cpp

@@ -34,6 +34,10 @@ QString PeerTools::ManagePeer(QString type, QString peer)
 {
     std::string peerAddress = peer.toStdString();
 
+    if (peerAddress.size() > 256) {
+      return tr("Error: Node address is invalid");
+    }
+
     if(!g_connman)
         return tr("Error: Peer-to-peer functionality missing or disabled");
 
@@ -47,7 +51,7 @@ QString PeerTools::ManagePeer(QString type, QString peer)
     if (type == "add")
     {
         if(!g_connman->AddNode(peerAddress))
-            return tr("Error: Node already added");
+            return tr("Error: Unable to add node");
     }
     else if(type == "remove")
     {

src/rpc/net.cpp

@@ -247,6 +247,10 @@ UniValue addnode(const JSONRPCRequest& request)
 
     string strNode = request.params[0].get_str();
 
+    if (strNode.size() > 256) {
+      throw JSONRPCError(RPC_CLIENT_NODE_ADDRESS_INVALID, "Error: Node address is invalid");
+    }
+
     if (strCommand == "onetry")
     {
         CAddress addr;
@@ -257,7 +261,7 @@ UniValue addnode(const JSONRPCRequest& request)
     if (strCommand == "add")
     {
         if(!g_connman->AddNode(strNode))
-            throw JSONRPCError(RPC_CLIENT_NODE_ALREADY_ADDED, "Error: Node already added");
+            throw JSONRPCError(RPC_CLIENT_NODE_ALREADY_ADDED, "Error: Unable to add node");
     }
     else if(strCommand == "remove")
     {

src/rpc/protocol.h

@@ -64,6 +64,7 @@ enum RPCErrorCode
     RPC_CLIENT_NODE_NOT_CONNECTED   = -29, //!< Node to disconnect not found in connected nodes
     RPC_CLIENT_INVALID_IP_OR_SUBNET = -30, //!< Invalid IP/Subnet
     RPC_CLIENT_P2P_DISABLED         = -31, //!< No valid connection manager instance found
+    RPC_CLIENT_NODE_ADDRESS_INVALID = -32, //!< Node address provided is not valid
 
     //! Wallet errors
     RPC_WALLET_ERROR                = -4,  //!< Unspecified problem with wallet (key not found etc.)