From f26502e9fc8a669b30717525597e3f468eaecf79 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 23 Jul 2020 15:34:40 +0100 Subject: [PATCH 1/3] [addrman] Specify max addresses and pct when calling GetAddresses() CAddrMan.GetAddr() would previously limit the number and percentage of addresses returned (to ADDRMAN_GETADDR_MAX (1000) and ADDRMAN_GETADDR_MAX_PCT (23) respectively). Instead, make it the callers responsibility to specify the maximum addresses and percentage they want returned. For net_processing, the maximums are MAX_ADDR_TO_SEND (1000) and MAX_PCT_ADDR_TO_SEND (23). For rpc/net, the maximum is specified by the client. --- src/addrman.cpp | 12 ++++++++---- src/addrman.h | 12 +++--------- src/bench/addrman.cpp | 2 +- src/net.cpp | 8 ++++---- src/net.h | 11 ++++------- src/net_processing.cpp | 6 ++++-- src/rpc/net.cpp | 10 ++++------ src/test/addrman_tests.cpp | 12 +++++++----- 8 files changed, 35 insertions(+), 38 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 7aba340d9d9..7636c6bad26 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -479,11 +479,15 @@ int CAddrMan::Check_() } #endif -void CAddrMan::GetAddr_(std::vector& vAddr) +void CAddrMan::GetAddr_(std::vector& 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 9e742339db0..ca045b91cda 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 &vAddr) EXCLUSIVE_LOCKS_REQUIRED(cs); + void GetAddr_(std::vector &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 GetAddr() + std::vector GetAddr(size_t max_addresses, size_t max_pct) { Check(); std::vector 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 26d93407682..ebdad5a4b8f 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 9c72c62df98..931c08a71e5 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2528,9 +2528,9 @@ void CConnman::AddNewAddresses(const std::vector& vAddr, const CAddres addrman.Add(vAddr, addrFrom, nTimePenalty); } -std::vector CConnman::GetAddresses() +std::vector CConnman::GetAddresses(size_t max_addresses, size_t max_pct) { - std::vector addresses = addrman.GetAddr(); + std::vector 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);}), @@ -2539,12 +2539,12 @@ std::vector CConnman::GetAddresses() return addresses; } -std::vector CConnman::GetAddresses(Network requestor_network) +std::vector CConnman::GetAddresses(Network requestor_network, size_t max_addresses, size_t max_pct) { const auto current_time = GetTime(); 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 1c558ee810a..015c1eca0fc 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 */ @@ -254,14 +251,14 @@ public: void SetServices(const CService &addr, ServiceFlags nServices); void MarkAddressGood(const CAddress& addr); void AddNewAddresses(const std::vector& vAddr, const CAddress& addrFrom, int64_t nTimePenalty = 0); - std::vector GetAddresses(); + std::vector 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 GetAddresses(Network requestor_network); + std::vector 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 c503a97be5d..1530da8d7f1 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. @@ -3476,9 +3478,9 @@ void ProcessMessage( pfrom.vAddrToSend.clear(); std::vector 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/net.cpp b/src/rpc/net.cpp index 9981ea35dff..3a67bb717fc 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 vAddr = node.connman->GetAddresses(); + std::vector vAddr = node.connman->GetAddresses(count, /* max_pct */ 0); UniValue ret(UniValue::VARR); - int address_return_count = std::min(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()); diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index bc6b38c6826..25fdd645688 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 vAddr1 = addrman.GetAddr(); + std::vector 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 vAddr = addrman.GetAddr(); + std::vector vAddr = addrman.GetAddr(/* max_addresses */ 2500, /* max_pct */ 23); size_t percent23 = (addrman.size() * 23) / 100; BOOST_CHECK_EQUAL(vAddr.size(), percent23); From ae8051bbd8377f2458ff1f167dc30c2d5f83e317 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 23 Jul 2020 16:21:14 +0100 Subject: [PATCH 2/3] [test] Test that getnodeaddresses() can return all known addresses --- test/functional/rpc_net.py | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index 3336246c8b4..6173e658b09 100755 --- a/test/functional/rpc_net.py +++ b/test/functional/rpc_net.py @@ -168,16 +168,24 @@ class NetTest(BitcoinTestFramework): 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) + # Obtain addresses via rpc call and check they were ones sent in before. + # + # All addresses added above are in the same netgroup and so are assigned + # to the same bucket. Maximum possible addresses in addrman is therefore + # 64, although actual number will usually be slightly less due to + # BucketPosition collisions. + node_addresses = self.nodes[0].getnodeaddresses(0) + assert_greater_than(len(node_addresses), 50) + assert_greater_than(65, 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 From 37a480e0cd94895b6051abef12d984ff74bdc4a3 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 23 Jul 2020 18:10:35 +0100 Subject: [PATCH 3/3] [net] Add addpeeraddress RPC method Allows addresses to be added to Address Manager for testing. --- src/net.cpp | 4 ++-- src/net.h | 2 +- src/rpc/client.cpp | 1 + src/rpc/net.cpp | 49 ++++++++++++++++++++++++++++++++++++++ test/functional/rpc_net.py | 32 ++++++++++--------------- 5 files changed, 66 insertions(+), 22 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 931c08a71e5..3fd3f19b5c5 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2523,9 +2523,9 @@ void CConnman::MarkAddressGood(const CAddress& addr) addrman.Good(addr); } -void CConnman::AddNewAddresses(const std::vector& vAddr, const CAddress& addrFrom, int64_t nTimePenalty) +bool CConnman::AddNewAddresses(const std::vector& vAddr, const CAddress& addrFrom, int64_t nTimePenalty) { - addrman.Add(vAddr, addrFrom, nTimePenalty); + return addrman.Add(vAddr, addrFrom, nTimePenalty); } std::vector CConnman::GetAddresses(size_t max_addresses, size_t max_pct) diff --git a/src/net.h b/src/net.h index 015c1eca0fc..fa2299f0124 100644 --- a/src/net.h +++ b/src/net.h @@ -250,7 +250,7 @@ public: // Addrman functions void SetServices(const CService &addr, ServiceFlags nServices); void MarkAddressGood(const CAddress& addr); - void AddNewAddresses(const std::vector& vAddr, const CAddress& addrFrom, int64_t nTimePenalty = 0); + bool AddNewAddresses(const std::vector& vAddr, const CAddress& addrFrom, int64_t nTimePenalty = 0); std::vector GetAddresses(size_t max_addresses, size_t max_pct); /** * Cache is used to minimize topology leaks, so it should diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 66ace7263a7..97d1c003696 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 3a67bb717fc..abd02bd2c3a 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -773,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 @@ -792,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/test/functional/rpc_net.py b/test/functional/rpc_net.py index 6173e658b09..ef5ccf7c6de 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,29 +152,25 @@ 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) + self.nodes[0].addpeeraddress(a, 8333) # Obtain addresses via rpc call and check they were ones sent in before. # - # All addresses added above are in the same netgroup and so are assigned - # to the same bucket. Maximum possible addresses in addrman is therefore - # 64, although actual number will usually be slightly less due to - # BucketPosition collisions. + # 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), 50) - assert_greater_than(65, len(node_addresses)) + 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)