Merge bitcoin/bitcoin#21843: p2p, rpc: enable GetAddr, GetAddresses, and getnodeaddresses by network

ce6bca88e8 doc: release note for getnodeaddresses by network (Jon Atack)
3f89c0e990 test: improve getnodeaddresses coverage, test by network (Jon Atack)
6c98c09991 rpc: enable filtering getnodeaddresses by network (Jon Atack)
80ba294854 p2p: allow CConnman::GetAddresses() by network, add doxygen (Jon Atack)
a49f3ddbba p2p: allow CAddrMan::GetAddr() by network, add doxygen (Jon Atack)
c38981e748 p2p: pull time call out of loop in CAddrMan::GetAddr_() (João Barbosa)
d35ddca91e p2p: enable CAddrMan::GetAddr_() by network, add doxygen (Jon Atack)

Pull request description:

  This patch allows passing a network argument to CAddrMan::GetAddr(), CConnman::GetAddresses(), and rpc getnodeaddresses to return only addresses of that network.

  It also contains a performance optimisation by promag.

ACKs for top commit:
  laanwj:
    Code review and lightly tested ACK ce6bca88e8
  vasild:
    ACK ce6bca88e8

Tree-SHA512: 40e700d97091248429c73cbc0639a1f03ab7288e636a7b9026ad253e9708253c6b2ec98e7d9fb2d56136c0f762313dd648915ac98d723ee330d713813a43f99d

doc/release-notes.md

@@ -108,6 +108,12 @@ Updated RPCs
   Respectively, these new fields indicate the duration of a ban and the time remaining until a ban expires,
   both in seconds. Additionally, the `ban_created` field is repositioned to come before `banned_until`. (#21602)
 
+- The `getnodeaddresses` RPC now returns a "network" field indicating the
+  network type (ipv4, ipv6, onion, or i2p) for each address.  (#21594)
+
+- `getnodeaddresses` now also accepts a "network" argument (ipv4, ipv6, onion,
+  or i2p) to return only addresses of the specified network.  (#21843)
+
 Changes to Wallet or GUI related RPCs can be found in the GUI or Wallet section below.
 
 New RPCs
@@ -130,9 +136,6 @@ Changes to Wallet or GUI related settings can be found in the GUI or Wallet sect
 
 - Passing an invalid `-rpcauth` argument now cause bitcoind to fail to start.  (#20461)
 
-- The `getnodeaddresses` RPC now returns a "network" field indicating the
-  network type (ipv4, ipv6, onion, or i2p) for each address.  (#21594)
-
 Tools and Utilities
 -------------------
 

src/addrman.cpp

@@ -7,9 +7,11 @@
 
 #include <hash.h>
 #include <logging.h>
+#include <netaddress.h>
 #include <serialize.h>
 
 #include <cmath>
+#include <optional>
 
 int CAddrInfo::GetTriedBucket(const uint256& nKey, const std::vector<bool> &asmap) const
 {
@@ -481,7 +483,7 @@ int CAddrMan::Check_()
 }
 #endif
 
-void CAddrMan::GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, size_t max_pct)
+void CAddrMan::GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, size_t max_pct, std::optional<Network> network)
 {
     size_t nNodes = vRandom.size();
     if (max_pct != 0) {
@@ -492,6 +494,7 @@ void CAddrMan::GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, size
     }
 
     // gather a list of random nodes, skipping those of low quality
+    const int64_t now{GetAdjustedTime()};
     for (unsigned int n = 0; n < vRandom.size(); n++) {
         if (vAddr.size() >= nNodes)
             break;
@@ -501,8 +504,14 @@ void CAddrMan::GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, size
         assert(mapInfo.count(vRandom[n]) == 1);
 
         const CAddrInfo& ai = mapInfo[vRandom[n]];
-        if (!ai.IsTerrible())
+
-            vAddr.push_back(ai);
+        // Filter by network (optional)
+        if (network != std::nullopt && ai.GetNetClass() != network) continue;
+
+        // Filter for quality
+        if (ai.IsTerrible(now)) continue;
+
+        vAddr.push_back(ai);
     }
 }
 

src/addrman.h

@@ -20,6 +20,7 @@
 #include <hash.h>
 #include <iostream>
 #include <map>
+#include <optional>
 #include <set>
 #include <stdint.h>
 #include <streams.h>
@@ -278,8 +279,15 @@ protected:
     int Check_() EXCLUSIVE_LOCKS_REQUIRED(cs);
 #endif
 
-    //! Select several addresses at once.
+    /**
-    void GetAddr_(std::vector<CAddress> &vAddr, size_t max_addresses, size_t max_pct) EXCLUSIVE_LOCKS_REQUIRED(cs);
+     * Return all or many randomly selected addresses, optionally by network.
+     *
+     * @param[out] vAddr         Vector of randomly selected addresses from vRandom.
+     * @param[in] max_addresses  Maximum number of addresses to return (0 = all).
+     * @param[in] max_pct        Maximum percentage of addresses to return (0 = all).
+     * @param[in] network        Select only addresses of this network (nullopt = all).
+     */
+    void GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, size_t max_pct, std::optional<Network> network) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     /** We have successfully connected to this peer. Calling this function
      *  updates the CAddress's nTime, which is used in our IsTerrible()
@@ -715,14 +723,20 @@ public:
         return addrRet;
     }
 
-    //! Return a bunch of addresses, selected at random.
+    /**
-    std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct)
+     * Return all or many randomly selected addresses, optionally by network.
+     *
+     * @param[in] max_addresses  Maximum number of addresses to return (0 = all).
+     * @param[in] max_pct        Maximum percentage of addresses to return (0 = all).
+     * @param[in] network        Select only addresses of this network (nullopt = all).
+     */
+    std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network)
     {
         Check();
         std::vector<CAddress> vAddr;
         {
             LOCK(cs);
-            GetAddr_(vAddr, max_addresses, max_pct);
+            GetAddr_(vAddr, max_addresses, max_pct, network);
         }
         Check();
         return vAddr;

src/bench/addrman.cpp

@@ -7,6 +7,7 @@
 #include <random.h>
 #include <util/time.h>
 
+#include <optional>
 #include <vector>
 
 /* A "source" is a source address from which we have received a bunch of other addresses. */
@@ -98,7 +99,7 @@ static void AddrManGetAddr(benchmark::Bench& bench)
     FillAddrMan(addrman);
 
     bench.run([&] {
-        const auto& addresses = addrman.GetAddr(2500, 23);
+        const auto& addresses = addrman.GetAddr(/* max_addresses */ 2500, /* max_pct */ 23, /* network */ std::nullopt);
         assert(addresses.size() > 0);
     });
 }

src/net.cpp

@@ -16,6 +16,7 @@
 #include <crypto/sha256.h>
 #include <i2p.h>
 #include <net_permissions.h>
+#include <netaddress.h>
 #include <netbase.h>
 #include <node/ui_interface.h>
 #include <protocol.h>
@@ -2669,9 +2670,9 @@ CConnman::~CConnman()
     Stop();
 }
 
-std::vector<CAddress> CConnman::GetAddresses(size_t max_addresses, size_t max_pct) const
+std::vector<CAddress> CConnman::GetAddresses(size_t max_addresses, size_t max_pct, std::optional<Network> network) const
 {
-    std::vector<CAddress> addresses = addrman.GetAddr(max_addresses, max_pct);
+    std::vector<CAddress> addresses = addrman.GetAddr(max_addresses, max_pct, network);
     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);}),
@@ -2691,7 +2692,7 @@ std::vector<CAddress> CConnman::GetAddresses(CNode& requestor, size_t max_addres
     auto r = m_addr_response_caches.emplace(cache_id, CachedAddrResponse{});
     CachedAddrResponse& cache_entry = r.first->second;
     if (cache_entry.m_cache_entry_expiration < current_time) { // If emplace() added new one it has expiration 0.
-        cache_entry.m_addrs_response_cache = GetAddresses(max_addresses, max_pct);
+        cache_entry.m_addrs_response_cache = GetAddresses(max_addresses, max_pct, /* network */ std::nullopt);
         // Choosing a proper cache lifetime is a trade-off between the privacy leak minimization
         // and the usefulness of ADDR responses to honest users.
         //

src/net.h

@@ -923,7 +923,14 @@ public:
     };
 
     // Addrman functions
-    std::vector<CAddress> GetAddresses(size_t max_addresses, size_t max_pct) const;
+    /**
+     * Return all or many randomly selected addresses, optionally by network.
+     *
+     * @param[in] max_addresses  Maximum number of addresses to return (0 = all).
+     * @param[in] max_pct        Maximum percentage of addresses to return (0 = all).
+     * @param[in] network        Select only addresses of this network (nullopt = all).
+     */
+    std::vector<CAddress> GetAddresses(size_t max_addresses, size_t max_pct, std::optional<Network> network) const;
     /**
      * Cache is used to minimize topology leaks, so it should
      * be used for all non-trusted calls, for example, p2p.

src/net_processing.cpp

@@ -3586,7 +3586,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         pfrom.vAddrToSend.clear();
         std::vector<CAddress> vAddr;
         if (pfrom.HasPermission(NetPermissionFlags::Addr)) {
-            vAddr = m_connman.GetAddresses(MAX_ADDR_TO_SEND, MAX_PCT_ADDR_TO_SEND);
+            vAddr = m_connman.GetAddresses(MAX_ADDR_TO_SEND, MAX_PCT_ADDR_TO_SEND, /* network */ std::nullopt);
         } else {
             vAddr = m_connman.GetAddresses(pfrom, MAX_ADDR_TO_SEND, MAX_PCT_ADDR_TO_SEND);
         }

src/rpc/net.cpp

@@ -28,6 +28,8 @@
 #include <version.h>
 #include <warnings.h>
 
+#include <optional>
+
 #include <univalue.h>
 
 const std::vector<std::string> CONNECTION_TYPE_DOC{
@@ -851,6 +853,7 @@ static RPCHelpMan getnodeaddresses()
                 "\nReturn known addresses, which can potentially be used to find new nodes in the network.\n",
                 {
                     {"count", RPCArg::Type::NUM, RPCArg::Default{1}, "The maximum number of addresses to return. Specify 0 to return all known addresses."},
+                    {"network", RPCArg::Type::STR, RPCArg::DefaultHint{"all networks"}, "Return only addresses of the specified network. Can be one of: " + Join(GetNetworkNames(), ", ") + "."},
                 },
                 RPCResult{
                     RPCResult::Type::ARR, "", "",
@@ -867,7 +870,10 @@ static RPCHelpMan getnodeaddresses()
                 },
                 RPCExamples{
                     HelpExampleCli("getnodeaddresses", "8")
-            + HelpExampleRpc("getnodeaddresses", "8")
+                    + HelpExampleCli("getnodeaddresses", "4 \"i2p\"")
+                    + HelpExampleCli("-named getnodeaddresses", "network=onion count=12")
+                    + HelpExampleRpc("getnodeaddresses", "8")
+                    + HelpExampleRpc("getnodeaddresses", "4, \"i2p\"")
                 },
         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 {
@@ -877,8 +883,13 @@ static RPCHelpMan getnodeaddresses()
     const int count{request.params[0].isNull() ? 1 : request.params[0].get_int()};
     if (count < 0) throw JSONRPCError(RPC_INVALID_PARAMETER, "Address count out of range");
 
+    const std::optional<Network> network{request.params[1].isNull() ? std::nullopt : std::optional<Network>{ParseNetwork(request.params[1].get_str())}};
+    if (network == NET_UNROUTABLE) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Network not recognized: %s", request.params[1].get_str()));
+    }
+
     // returns a shuffled list of CAddress
-    const std::vector<CAddress> vAddr{connman.GetAddresses(count, /* max_pct */ 0)};
+    const std::vector<CAddress> vAddr{connman.GetAddresses(count, /* max_pct */ 0, network)};
     UniValue ret(UniValue::VARR);
 
     for (const CAddress& addr : vAddr) {

src/test/addrman_tests.cpp

@@ -12,6 +12,7 @@
 
 #include <boost/test/unit_test.hpp>
 
+#include <optional>
 #include <string>
 
 class CAddrManTest : public CAddrMan
@@ -392,7 +393,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(/* max_addresses */ 0, /* max_pct */0);
+    std::vector<CAddress> vAddr1 = addrman.GetAddr(/* max_addresses */ 0, /* max_pct */ 0, /* network */ std::nullopt);
     BOOST_CHECK_EQUAL(vAddr1.size(), 0U);
 
     CAddress addr1 = CAddress(ResolveService("250.250.2.1", 8333), NODE_NONE);
@@ -415,15 +416,15 @@ BOOST_AUTO_TEST_CASE(addrman_getaddr)
     BOOST_CHECK(addrman.Add(addr4, source2));
     BOOST_CHECK(addrman.Add(addr5, source1));
 
-    BOOST_CHECK_EQUAL(addrman.GetAddr(/* max_addresses */ 0, /* max_pct */ 0).size(), 5U);
+    BOOST_CHECK_EQUAL(addrman.GetAddr(/* max_addresses */ 0, /* max_pct */ 0, /* network */ std::nullopt).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);
+    BOOST_CHECK_EQUAL(addrman.GetAddr(/* max_addresses */ 2500, /* max_pct */ 23, /* network */ std::nullopt).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(/* max_addresses */ 0, /* max_pct */ 0).size(), 5U);
+    BOOST_CHECK_EQUAL(addrman.GetAddr(/* max_addresses */ 0, /* max_pct */ 0, /* network */ std::nullopt).size(), 5U);
-    BOOST_CHECK_EQUAL(addrman.GetAddr(/* max_addresses */ 2500, /* max_pct */ 23).size(), 1U);
+    BOOST_CHECK_EQUAL(addrman.GetAddr(/* max_addresses */ 2500, /* max_pct */ 23, /* network */ std::nullopt).size(), 1U);
 
     // Test: Ensure GetAddr still returns 23% when addrman has many addrs.
     for (unsigned int i = 1; i < (8 * 256); i++) {
@@ -438,7 +439,7 @@ BOOST_AUTO_TEST_CASE(addrman_getaddr)
         if (i % 8 == 0)
             addrman.Good(addr);
     }
-    std::vector<CAddress> vAddr = addrman.GetAddr(/* max_addresses */ 2500, /* max_pct */ 23);
+    std::vector<CAddress> vAddr = addrman.GetAddr(/* max_addresses */ 2500, /* max_pct */ 23, /* network */ std::nullopt);
 
     size_t percent23 = (addrman.size() * 23) / 100;
     BOOST_CHECK_EQUAL(vAddr.size(), percent23);

src/test/fuzz/addrman.cpp

@@ -60,7 +60,10 @@ FUZZ_TARGET_INIT(addrman, initialize_addrman)
                 (void)addr_man.Select(fuzzed_data_provider.ConsumeBool());
             },
             [&] {
-                (void)addr_man.GetAddr(fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096), fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096));
+                (void)addr_man.GetAddr(
+                    /* max_addresses */ fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096),
+                    /* max_pct */ fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096),
+                    /* network */ std::nullopt);
             },
             [&] {
                 const std::optional<CAddress> opt_address = ConsumeDeserializable<CAddress>(fuzzed_data_provider);

src/test/fuzz/connman.cpp

@@ -71,10 +71,16 @@ FUZZ_TARGET_INIT(connman, initialize_connman)
                 (void)connman.ForNode(fuzzed_data_provider.ConsumeIntegral<NodeId>(), [&](auto) { return fuzzed_data_provider.ConsumeBool(); });
             },
             [&] {
-                (void)connman.GetAddresses(fuzzed_data_provider.ConsumeIntegral<size_t>(), fuzzed_data_provider.ConsumeIntegral<size_t>());
+                (void)connman.GetAddresses(
+                    /* max_addresses */ fuzzed_data_provider.ConsumeIntegral<size_t>(),
+                    /* max_pct */ fuzzed_data_provider.ConsumeIntegral<size_t>(),
+                    /* network */ std::nullopt);
             },
             [&] {
-                (void)connman.GetAddresses(random_node, fuzzed_data_provider.ConsumeIntegral<size_t>(), fuzzed_data_provider.ConsumeIntegral<size_t>());
+                (void)connman.GetAddresses(
+                    /* requestor */ random_node,
+                    /* max_addresses */ fuzzed_data_provider.ConsumeIntegral<size_t>(),
+                    /* max_pct */ fuzzed_data_provider.ConsumeIntegral<size_t>());
             },
             [&] {
                 (void)connman.GetDeterministicRandomizer(fuzzed_data_provider.ConsumeIntegral<uint64_t>());

test/functional/rpc_net.py

@@ -187,43 +187,54 @@ class NetTest(BitcoinTestFramework):
     def test_getnodeaddresses(self):
         self.log.info("Test getnodeaddresses")
         self.nodes[0].add_p2p_connection(P2PInterface())
+        services = NODE_NETWORK | NODE_WITNESS
 
-        # Add some addresses to the Address Manager over RPC. Due to the way
+        # Add an IPv6 address to the address manager.
-        # bucket and bucket position are calculated, some of these addresses
+        ipv6_addr = "1233:3432:2434:2343:3234:2345:6546:4534"
-        # will collide.
+        self.nodes[0].addpeeraddress(address=ipv6_addr, port=8333)
+
+        # Add 10,000 IPv4 addresses to the address manager. Due to the way bucket
+        # and bucket positions are calculated, some of these addresses will collide.
         imported_addrs = []
         for i in range(10000):
             first_octet = i >> 8
             second_octet = i % 256
-            a = "{}.{}.1.1".format(first_octet, second_octet)  # IPV4
+            a = f"{first_octet}.{second_octet}.1.1"
             imported_addrs.append(a)
             self.nodes[0].addpeeraddress(a, 8333)
 
-        # Obtain addresses via rpc call and check they were ones sent in before.
+        # Fetch the addresses via the RPC and test the results.
-        #
+        assert_equal(len(self.nodes[0].getnodeaddresses()), 1)  # default count is 1
-        # Maximum possible addresses in addrman is 10000, although actual
+        assert_equal(len(self.nodes[0].getnodeaddresses(count=2)), 2)
-        # number will usually be less due to bucket and bucket position
+        assert_equal(len(self.nodes[0].getnodeaddresses(network="ipv4", count=8)), 8)
-        # collisions.
+
-        node_addresses = self.nodes[0].getnodeaddresses(0)
+        # Maximum possible addresses in AddrMan is 10000. The actual number will
+        # usually be less due to bucket and bucket position collisions.
+        node_addresses = self.nodes[0].getnodeaddresses(0, "ipv4")
         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_equal(a["services"], NODE_NETWORK | NODE_WITNESS)
+            assert_equal(a["services"], services)
             assert a["address"] in imported_addrs
             assert_equal(a["port"], 8333)
             assert_equal(a["network"], "ipv4")
 
-        node_addresses = self.nodes[0].getnodeaddresses(1)
+        # Test the IPv6 address.
-        assert_equal(len(node_addresses), 1)
+        res = self.nodes[0].getnodeaddresses(0, "ipv6")
+        assert_equal(len(res), 1)
+        assert_equal(res[0]["address"], ipv6_addr)
+        assert_equal(res[0]["network"], "ipv6")
+        assert_equal(res[0]["port"], 8333)
+        assert_equal(res[0]["services"], services)
 
+        # Test for the absence of onion and I2P addresses.
+        for network in ["onion", "i2p"]:
+            assert_equal(self.nodes[0].getnodeaddresses(0, network), [])
+
+        # Test invalid arguments.
         assert_raises_rpc_error(-8, "Address count out of range", self.nodes[0].getnodeaddresses, -1)
-
+        assert_raises_rpc_error(-8, "Network not recognized: Foo", self.nodes[0].getnodeaddresses, 1, "Foo")
-        # addrman's size cannot be known reliably after insertion, as hash collisions may occur
-        # so only test that requesting a large number of addresses returns less than that
-        LARGE_REQUEST_COUNT = 10000
-        node_addresses = self.nodes[0].getnodeaddresses(LARGE_REQUEST_COUNT)
-        assert_greater_than(LARGE_REQUEST_COUNT, len(node_addresses))
 
 
 if __name__ == '__main__':