Merge #19658: [rpc] Allow RPC to fetch all addrman records and add records to addrman

37a480e0cd94895b6051abef12d984ff74bdc4a3 [net] Add addpeeraddress RPC method (John Newbery)
ae8051bbd8377f2458ff1f167dc30c2d5f83e317 [test] Test that getnodeaddresses() can return all known addresses (John Newbery)
f26502e9fc8a669b30717525597e3f468eaecf79 [addrman] Specify max addresses and pct when calling GetAddresses() (John Newbery)

Pull request description:

  Currently addrman only allows a maximum of 1000 records or 23% of all records to be returned in a call to `GetAddr()`. Relax this limit and have the client specify the max records they want. For p2p, behaviour is unchanged (but the rate limiting is set inside net_processing, where it belongs). For RPC, `getnodeaddresses` can now return the complete addrman, which is helpful for testing and monitoring.

  Also add a test-only RPC `addpeeraddress`, which adds an IP address:port to addrman. This is helpful for testing (eg #18991).

ACKs for top commit:
  naumenkogs:
    utACK 37a480e0cd94895b6051abef12d984ff74bdc4a3
  laanwj:
    Code review and lightly manually tested ACK 37a480e0cd94895b6051abef12d984ff74bdc4a3

Tree-SHA512: f86dcd410aaebaf6e9ca18ce6f23556e5e4649c1325577213d873aa09967298e65ab2dc19a72670641ae92211a923afda1fe124a82e9d2c1cad73d478ef27fdc

10 files changed, 109 insertions, 60 deletions

diff --git a/src/addrman.cpp b/src/addrman.cpp
index 7aba340d9d..7636c6bad2 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -479,11 +479,15 @@ int CAddrMan::Check_()
 }
 #endif
 
-void CAddrMan::GetAddr_(std::vector<CAddress>& vAddr)
+void CAddrMan::GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, size_t max_pct)
 {
-    unsigned int nNodes = ADDRMAN_GETADDR_MAX_PCT * vRandom.size() / 100;
-    if (nNodes > ADDRMAN_GETADDR_MAX)
-        nNodes = ADDRMAN_GETADDR_MAX;
+    size_t nNodes = vRandom.size();
+    if (max_pct != 0) {
+        nNodes = max_pct * nNodes / 100;
+    }
+    if (max_addresses != 0) {
+        nNodes = std::min(nNodes, max_addresses);
+    }
 
     // gather a list of random nodes, skipping those of low quality
     for (unsigned int n = 0; n < vRandom.size(); n++) {
diff --git a/src/addrman.h b/src/addrman.h
index 9e742339db..ca045b91cd 100644
--- a/src/addrman.h
+++ b/src/addrman.h
@@ -153,12 +153,6 @@ public:
 //! how recent a successful connection should be before we allow an address to be evicted from tried
 #define ADDRMAN_REPLACEMENT_HOURS 4
 
-//! the maximum percentage of nodes to return in a getaddr call
-#define ADDRMAN_GETADDR_MAX_PCT 23
-
-//! the maximum number of nodes to return in a getaddr call
-#define ADDRMAN_GETADDR_MAX 1000
-
 //! Convenience
 #define ADDRMAN_TRIED_BUCKET_COUNT (1 << ADDRMAN_TRIED_BUCKET_COUNT_LOG2)
 #define ADDRMAN_NEW_BUCKET_COUNT (1 << ADDRMAN_NEW_BUCKET_COUNT_LOG2)
@@ -261,7 +255,7 @@ protected:
 #endif
 
     //! Select several addresses at once.
-    void GetAddr_(std::vector<CAddress> &vAddr) EXCLUSIVE_LOCKS_REQUIRED(cs);
+    void GetAddr_(std::vector<CAddress> &vAddr, size_t max_addresses, size_t max_pct) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! Mark an entry as currently-connected-to.
     void Connected_(const CService &addr, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs);
@@ -638,13 +632,13 @@ public:
     }
 
     //! Return a bunch of addresses, selected at random.
-    std::vector<CAddress> GetAddr()
+    std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct)
     {
         Check();
         std::vector<CAddress> vAddr;
         {
             LOCK(cs);
-            GetAddr_(vAddr);
+            GetAddr_(vAddr, max_addresses, max_pct);
         }
         Check();
         return vAddr;
diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp
index 26d9340768..ebdad5a4b8 100644
--- a/src/bench/addrman.cpp
+++ b/src/bench/addrman.cpp
@@ -98,7 +98,7 @@ static void AddrManGetAddr(benchmark::Bench& bench)
     FillAddrMan(addrman);
 
     bench.run([&] {
-        const auto& addresses = addrman.GetAddr();
+        const auto& addresses = addrman.GetAddr(2500, 23);
         assert(addresses.size() > 0);
     });
 }
diff --git a/src/net.cpp b/src/net.cpp
index 8c214dc05f..6c1980735c 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -2521,14 +2521,14 @@ void CConnman::MarkAddressGood(const CAddress& addr)
     addrman.Good(addr);
 }
 
-void CConnman::AddNewAddresses(const std::vector<CAddress>& vAddr, const CAddress& addrFrom, int64_t nTimePenalty)
+bool CConnman::AddNewAddresses(const std::vector<CAddress>& vAddr, const CAddress& addrFrom, int64_t nTimePenalty)
 {
-    addrman.Add(vAddr, addrFrom, nTimePenalty);
+    return addrman.Add(vAddr, addrFrom, nTimePenalty);
 }
 
-std::vector<CAddress> CConnman::GetAddresses()
+std::vector<CAddress> CConnman::GetAddresses(size_t max_addresses, size_t max_pct)
 {
-    std::vector<CAddress> addresses = addrman.GetAddr();
+    std::vector<CAddress> addresses = addrman.GetAddr(max_addresses, max_pct);
     if (m_banman) {
         addresses.erase(std::remove_if(addresses.begin(), addresses.end(),
                         [this](const CAddress& addr){return m_banman->IsDiscouraged(addr) || m_banman->IsBanned(addr);}),
@@ -2537,12 +2537,12 @@ std::vector<CAddress> CConnman::GetAddresses()
     return addresses;
 }
 
-std::vector<CAddress> CConnman::GetAddresses(Network requestor_network)
+std::vector<CAddress> CConnman::GetAddresses(Network requestor_network, size_t max_addresses, size_t max_pct)
 {
     const auto current_time = GetTime<std::chrono::microseconds>();
     if (m_addr_response_caches.find(requestor_network) == m_addr_response_caches.end() ||
         m_addr_response_caches[requestor_network].m_update_addr_response < current_time) {
-        m_addr_response_caches[requestor_network].m_addrs_response_cache = GetAddresses();
+        m_addr_response_caches[requestor_network].m_addrs_response_cache = GetAddresses(max_addresses, max_pct);
         m_addr_response_caches[requestor_network].m_update_addr_response = current_time + std::chrono::hours(21) + GetRandMillis(std::chrono::hours(6));
     }
     return m_addr_response_caches[requestor_network].m_addrs_response_cache;
diff --git a/src/net.h b/src/net.h
index f23878cd0c..7a8abb401d 100644
--- a/src/net.h
+++ b/src/net.h
@@ -51,11 +51,8 @@ static const bool DEFAULT_WHITELISTFORCERELAY = false;
 static const int TIMEOUT_INTERVAL = 20 * 60;
 /** Run the feeler connection loop once every 2 minutes or 120 seconds. **/
 static const int FEELER_INTERVAL = 120;
-/** The maximum number of new addresses to accumulate before announcing. */
-static const unsigned int MAX_ADDR_TO_SEND = 1000;
-// TODO: remove ADDRMAN_GETADDR_MAX and let the caller specify this limit with MAX_ADDR_TO_SEND.
-static_assert(MAX_ADDR_TO_SEND == ADDRMAN_GETADDR_MAX,
-    "Max allowed ADDR message size should be equal to the max number of records returned from AddrMan.");
+/** The maximum number of addresses from our addrman to return in response to a getaddr message. */
+static constexpr size_t MAX_ADDR_TO_SEND = 1000;
 /** Maximum length of incoming protocol messages (no message over 4 MB is currently acceptable). */
 static const unsigned int MAX_PROTOCOL_MESSAGE_LENGTH = 4 * 1000 * 1000;
 /** Maximum length of the user agent string in `version` message */
@@ -264,15 +261,15 @@ public:
     // Addrman functions
     void SetServices(const CService &addr, ServiceFlags nServices);
     void MarkAddressGood(const CAddress& addr);
-    void AddNewAddresses(const std::vector<CAddress>& vAddr, const CAddress& addrFrom, int64_t nTimePenalty = 0);
-    std::vector<CAddress> GetAddresses();
+    bool AddNewAddresses(const std::vector<CAddress>& vAddr, const CAddress& addrFrom, int64_t nTimePenalty = 0);
+    std::vector<CAddress> GetAddresses(size_t max_addresses, size_t max_pct);
     /**
      * Cache is used to minimize topology leaks, so it should
      * be used for all non-trusted calls, for example, p2p.
      * A non-malicious call (from RPC or a peer with addr permission) should
      * call the function without a parameter to avoid using the cache.
      */
-    std::vector<CAddress> GetAddresses(Network requestor_network);
+    std::vector<CAddress> GetAddresses(Network requestor_network, size_t max_addresses, size_t max_pct);
 
     // This allows temporarily exceeding m_max_outbound_full_relay, with the goal of finding
     // a peer that is better than all our current peers.
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 7188bd2154..3e06f1fff0 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -143,6 +143,8 @@ static constexpr unsigned int MAX_FEEFILTER_CHANGE_DELAY = 5 * 60;
 static constexpr uint32_t MAX_GETCFILTERS_SIZE = 1000;
 /** Maximum number of cf hashes that may be requested with one getcfheaders. See BIP 157. */
 static constexpr uint32_t MAX_GETCFHEADERS_SIZE = 2000;
+/** the maximum percentage of addresses from our addrman to return in response to a getaddr message. */
+static constexpr size_t MAX_PCT_ADDR_TO_SEND = 23;
 
 struct COrphanTx {
     // When modifying, adapt the copy of this definition in tests/DoS_tests.
@@ -3521,9 +3523,9 @@ void ProcessMessage(
         pfrom.vAddrToSend.clear();
         std::vector<CAddress> vAddr;
         if (pfrom.HasPermission(PF_ADDR)) {
-            vAddr = connman.GetAddresses();
+            vAddr = connman.GetAddresses(MAX_ADDR_TO_SEND, MAX_PCT_ADDR_TO_SEND);
         } else {
-            vAddr = connman.GetAddresses(pfrom.addr.GetNetwork());
+            vAddr = connman.GetAddresses(pfrom.addr.GetNetwork(), MAX_ADDR_TO_SEND, MAX_PCT_ADDR_TO_SEND);
         }
         FastRandomContext insecure_rand;
         for (const CAddress &addr : vAddr) {
diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp
index 66ace7263a..97d1c00369 100644
--- a/src/rpc/client.cpp
+++ b/src/rpc/client.cpp
@@ -173,6 +173,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "createwallet", 4, "avoid_reuse"},
     { "createwallet", 5, "descriptors"},
     { "getnodeaddresses", 0, "count"},
+    { "addpeeraddress", 1, "port"},
     { "stop", 0, "wait" },
 };
 // clang-format on
diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
index 09265bc480..0dfd560896 100644
--- a/src/rpc/net.cpp
+++ b/src/rpc/net.cpp
@@ -727,7 +727,7 @@ static UniValue getnodeaddresses(const JSONRPCRequest& request)
             RPCHelpMan{"getnodeaddresses",
                 "\nReturn known addresses which can potentially be used to find new nodes in the network\n",
                 {
-                    {"count", RPCArg::Type::NUM, /* default */ "1", "How many addresses to return. Limited to the smaller of " + ToString(ADDRMAN_GETADDR_MAX) + " or " + ToString(ADDRMAN_GETADDR_MAX_PCT) + "% of all known addresses."},
+                    {"count", RPCArg::Type::NUM, /* default */ "1", "The maximum number of addresses to return. Specify 0 to return all known addresses."},
                 },
                 RPCResult{
                     RPCResult::Type::ARR, "", "",
@@ -754,18 +754,16 @@ static UniValue getnodeaddresses(const JSONRPCRequest& request)
     int count = 1;
     if (!request.params[0].isNull()) {
         count = request.params[0].get_int();
-        if (count <= 0) {
+        if (count < 0) {
             throw JSONRPCError(RPC_INVALID_PARAMETER, "Address count out of range");
         }
     }
     // returns a shuffled list of CAddress
-    std::vector<CAddress> vAddr = node.connman->GetAddresses();
+    std::vector<CAddress> vAddr = node.connman->GetAddresses(count, /* max_pct */ 0);
     UniValue ret(UniValue::VARR);
 
-    int address_return_count = std::min<int>(count, vAddr.size());
-    for (int i = 0; i < address_return_count; ++i) {
+    for (const CAddress& addr : vAddr) {
         UniValue obj(UniValue::VOBJ);
-        const CAddress& addr = vAddr[i];
         obj.pushKV("time", (int)addr.nTime);
         obj.pushKV("services", (uint64_t)addr.nServices);
         obj.pushKV("address", addr.ToStringIP());
@@ -775,6 +773,54 @@ static UniValue getnodeaddresses(const JSONRPCRequest& request)
     return ret;
 }
 
+static UniValue addpeeraddress(const JSONRPCRequest& request)
+{
+    RPCHelpMan{"addpeeraddress",
+        "\nAdd the address of a potential peer to the address manager. This RPC is for testing only.\n",
+        {
+            {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The IP address of the peer"},
+            {"port", RPCArg::Type::NUM, RPCArg::Optional::NO, "The port of the peer"},
+        },
+        RPCResult{
+            RPCResult::Type::OBJ, "", "",
+            {
+                {RPCResult::Type::BOOL, "success", "whether the peer address was successfully added to the address manager"},
+            },
+        },
+        RPCExamples{
+            HelpExampleCli("addpeeraddress", "\"1.2.3.4\" 8333")
+    + HelpExampleRpc("addpeeraddress", "\"1.2.3.4\", 8333")
+        },
+    }.Check(request);
+
+    NodeContext& node = EnsureNodeContext(request.context);
+    if (!node.connman) {
+        throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled");
+    }
+
+    UniValue obj(UniValue::VOBJ);
+
+    std::string addr_string = request.params[0].get_str();
+    uint16_t port = request.params[1].get_int();
+
+    CNetAddr net_addr;
+    if (!LookupHost(addr_string, net_addr, false)) {
+        obj.pushKV("success", false);
+        return obj;
+    }
+    CAddress address = CAddress({net_addr, port}, ServiceFlags(NODE_NETWORK|NODE_WITNESS));
+    address.nTime = GetAdjustedTime();
+    // The source address is set equal to the address. This is equivalent to the peer
+    // announcing itself.
+    if (!node.connman->AddNewAddresses({address}, address)) {
+        obj.pushKV("success", false);
+        return obj;
+    }
+
+    obj.pushKV("success", true);
+    return obj;
+}
+
 void RegisterNetRPCCommands(CRPCTable &t)
 {
 // clang-format off
@@ -794,6 +840,7 @@ static const CRPCCommand commands[] =
     { "network",            "clearbanned",            &clearbanned,            {} },
     { "network",            "setnetworkactive",       &setnetworkactive,       {"state"} },
     { "network",            "getnodeaddresses",       &getnodeaddresses,       {"count"} },
+    { "hidden",             "addpeeraddress",         &addpeeraddress,         {"address", "port"} },
 };
 // clang-format on
 
diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp
index bc6b38c682..25fdd64568 100644
--- a/src/test/addrman_tests.cpp
+++ b/src/test/addrman_tests.cpp
@@ -392,7 +392,7 @@ BOOST_AUTO_TEST_CASE(addrman_getaddr)
     // Test: Sanity check, GetAddr should never return anything if addrman
     //  is empty.
     BOOST_CHECK_EQUAL(addrman.size(), 0U);
-    std::vector<CAddress> vAddr1 = addrman.GetAddr();
+    std::vector<CAddress> vAddr1 = addrman.GetAddr(/* max_addresses */ 0, /* max_pct */0);
     BOOST_CHECK_EQUAL(vAddr1.size(), 0U);
 
     CAddress addr1 = CAddress(ResolveService("250.250.2.1", 8333), NODE_NONE);
@@ -415,13 +415,15 @@ BOOST_AUTO_TEST_CASE(addrman_getaddr)
     BOOST_CHECK(addrman.Add(addr4, source2));
     BOOST_CHECK(addrman.Add(addr5, source1));
 
-    // GetAddr returns 23% of addresses, 23% of 5 is 1 rounded down.
-    BOOST_CHECK_EQUAL(addrman.GetAddr().size(), 1U);
+    BOOST_CHECK_EQUAL(addrman.GetAddr(/* max_addresses */ 0, /* max_pct */ 0).size(), 5U);
+    // Net processing asks for 23% of addresses. 23% of 5 is 1 rounded down.
+    BOOST_CHECK_EQUAL(addrman.GetAddr(/* max_addresses */ 2500, /* max_pct */ 23).size(), 1U);
 
     // Test: Ensure GetAddr works with new and tried addresses.
     addrman.Good(CAddress(addr1, NODE_NONE));
     addrman.Good(CAddress(addr2, NODE_NONE));
-    BOOST_CHECK_EQUAL(addrman.GetAddr().size(), 1U);
+    BOOST_CHECK_EQUAL(addrman.GetAddr(/* max_addresses */ 0, /* max_pct */ 0).size(), 5U);
+    BOOST_CHECK_EQUAL(addrman.GetAddr(/* max_addresses */ 2500, /* max_pct */ 23).size(), 1U);
 
     // Test: Ensure GetAddr still returns 23% when addrman has many addrs.
     for (unsigned int i = 1; i < (8 * 256); i++) {
@@ -436,7 +438,7 @@ BOOST_AUTO_TEST_CASE(addrman_getaddr)
         if (i % 8 == 0)
             addrman.Good(addr);
     }
-    std::vector<CAddress> vAddr = addrman.GetAddr();
+    std::vector<CAddress> vAddr = addrman.GetAddr(/* max_addresses */ 2500, /* max_pct */ 23);
 
     size_t percent23 = (addrman.size() * 23) / 100;
     BOOST_CHECK_EQUAL(vAddr.size(), percent23);
diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py
index 3336246c8b..ef5ccf7c6d 100755
--- a/test/functional/rpc_net.py
+++ b/test/functional/rpc_net.py
@@ -22,8 +22,6 @@ from test_framework.util import (
 from test_framework.mininode import P2PInterface
 import test_framework.messages
 from test_framework.messages import (
-    CAddress,
-    msg_addr,
     NODE_NETWORK,
     NODE_WITNESS,
 )
@@ -154,30 +152,34 @@ class NetTest(BitcoinTestFramework):
     def _test_getnodeaddresses(self):
         self.nodes[0].add_p2p_connection(P2PInterface())
 
-        # send some addresses to the node via the p2p message addr
-        msg = msg_addr()
+        # Add some addresses to the Address Manager over RPC. Due to the way
+        # bucket and bucket position are calculated, some of these addresses
+        # will collide.
         imported_addrs = []
-        for i in range(256):
-            a = "123.123.123.{}".format(i)
+        for i in range(10000):
+            first_octet = i >> 8
+            second_octet = i % 256
+            a = "{}.{}.1.1".format(first_octet, second_octet)
             imported_addrs.append(a)
-            addr = CAddress()
-            addr.time = 100000000
-            addr.nServices = NODE_NETWORK | NODE_WITNESS
-            addr.ip = a
-            addr.port = 8333
-            msg.addrs.append(addr)
-        self.nodes[0].p2p.send_and_ping(msg)
-
-        # obtain addresses via rpc call and check they were ones sent in before
-        REQUEST_COUNT = 10
-        node_addresses = self.nodes[0].getnodeaddresses(REQUEST_COUNT)
-        assert_equal(len(node_addresses), REQUEST_COUNT)
+            self.nodes[0].addpeeraddress(a, 8333)
+
+        # Obtain addresses via rpc call and check they were ones sent in before.
+        #
+        # Maximum possible addresses in addrman is 10000, although actual
+        # number will usually be less due to bucket and bucket position
+        # collisions.
+        node_addresses = self.nodes[0].getnodeaddresses(0)
+        assert_greater_than(len(node_addresses), 5000)
+        assert_greater_than(10000, len(node_addresses))
         for a in node_addresses:
-            assert_greater_than(a["time"], 1527811200) # 1st June 2018
+            assert_greater_than(a["time"], 1527811200)  # 1st June 2018
             assert_equal(a["services"], NODE_NETWORK | NODE_WITNESS)
             assert a["address"] in imported_addrs
             assert_equal(a["port"], 8333)
 
+        node_addresses = self.nodes[0].getnodeaddresses(1)
+        assert_equal(len(node_addresses), 1)
+
         assert_raises_rpc_error(-8, "Address count out of range", self.nodes[0].getnodeaddresses, -1)
 
         # addrman's size cannot be known reliably after insertion, as hash collisions may occur