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

pull jnewbery wants to merge 3 commits into bitcoin:master from jnewbery:2020-07-addrman-get changing 10 files +109 −60
  1. jnewbery commented at 11:49 am on August 4, 2020: member

    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).

  2. jnewbery commented at 11:49 am on August 4, 2020: member
    cc @naumenkogs
  3. fanquake added the label RPC/REST/ZMQ on Aug 4, 2020
  4. fanquake added the label P2P on Aug 4, 2020
  5. naumenkogs commented at 1:41 pm on August 4, 2020: member

    I think this is a move in the right direction: GetAddr() should be more dynamic. It also seems to be useful for testing.

    Concept ACK.

  6. DrahtBot commented at 5:31 pm on August 4, 2020: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19238 (refactor: Replace RecursiveMutex with Mutex in CAddrMan by hebasto)
    • #17428 (p2p: Try to preserve outbound block-relay-only connections during restart by hebasto)
    • #16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  7. laanwj commented at 2:50 pm on August 5, 2020: member
    Concept ACK, I think more control over addrman via RPC is a good thing.
  8. in src/rpc/net.cpp:797 in fe36e5a2f1 outdated 
    792+    + HelpExampleRpc("getnodeaddresses", "\"1.2.3.4\", 8333")
793+        },
794+    }.Check(request);
795+
796+    NodeContext& node = EnsureNodeContext(request.context);
797+    if (!node.addrman) {
    mzumsande commented at 5:59 pm on August 8, 2020:
    This doesn’t compile since addrman is not a member of NodeContext.
    jnewbery commented at 10:06 pm on August 8, 2020:
    oops. This was from a previous version of the branch. Should be fixed now.
  9. mzumsande commented at 6:17 pm on August 8, 2020: member
    Concept ACK, the getaddr limits only make sense for P2P, not for RPC.
  10. jnewbery force-pushed on Aug 8, 2020
  11. jnewbery force-pushed on Aug 8, 2020
  12. jnewbery force-pushed on Aug 8, 2020
  13. in src/rpc/net.cpp:804 in 5bff6fdb50 outdated 
    799+    }
800+
801+    UniValue obj(UniValue::VOBJ);
802+
803+    std::string addr_string = request.params[0].get_str();
804+    uint16_t port = request.params[1].get_int();
    mzumsande commented at 6:03 pm on August 9, 2020:
    Should add addpeeraddress to vRPCConvertParams (rpc/client.cpp).
    jnewbery commented at 8:56 am on August 11, 2020:
    done
  14. in src/rpc/net.cpp:792 in 5bff6fdb50 outdated 
    787+                {RPCResult::Type::BOOL, "success", "whether the peer address waas successfully added to the address manager"},
788+            },
789+        },
790+        RPCExamples{
791+            HelpExampleCli("addpeeraddress", "\"1.2.3.4\" 8333")
792+    + HelpExampleRpc("getnodeaddresses", "\"1.2.3.4\", 8333")
    mzumsande commented at 6:03 pm on August 9, 2020:
    typo, getnodeaddresses->addpeeraddress
    jnewbery commented at 8:56 am on August 11, 2020:
    fixed
  15. in src/rpc/net.cpp:787 in 5bff6fdb50 outdated 
    782+            {"port", RPCArg::Type::NUM, RPCArg::Optional::NO, "The port of the peer"},
783+        },
784+        RPCResult{
785+            RPCResult::Type::OBJ, "", "",
786+            {
787+                {RPCResult::Type::BOOL, "success", "whether the peer address waas successfully added to the address manager"},
    mzumsande commented at 6:04 pm on August 9, 2020:
    typo: waas -> was
    jnewbery commented at 8:56 am on August 11, 2020:
    fixed
  16. in src/net.h:54 in 5bff6fdb50 outdated 
    50@@ -51,11 +51,8 @@ static const bool DEFAULT_WHITELISTFORCERELAY = false;
51 static const int TIMEOUT_INTERVAL = 20 * 60;
52 /** Run the feeler connection loop once every 2 minutes or 120 seconds. **/
53 static const int FEELER_INTERVAL = 120;
54-/** The maximum number of new addresses to accumulate before announcing. */
55-static const unsigned int MAX_ADDR_TO_SEND = 1000;
56-// TODO: remove ADDRMAN_GETADDR_MAX and let the caller specify this limit with MAX_ADDR_TO_SEND.
57-static_assert(MAX_ADDR_TO_SEND == ADDRMAN_GETADDR_MAX,
58-    "Max allowed ADDR message size should be equal to the max number of records returned from AddrMan.");
59+/** The maximum number of addresses from our addrman to return in a getaddr call. */
    mzumsande commented at 6:18 pm on August 9, 2020:
    “call” still sounds like rpc, maybe “return in response to a getaddr message”.
    jnewbery commented at 9:00 am on August 11, 2020:
    fixed
  17. in src/net_processing.cpp:146 in 5bff6fdb50 outdated 
    142@@ -143,6 +143,8 @@ static constexpr unsigned int MAX_FEEFILTER_CHANGE_DELAY = 5 * 60;
143 static constexpr uint32_t MAX_GETCFILTERS_SIZE = 1000;
144 /** Maximum number of cf hashes that may be requested with one getcfheaders. See BIP 157. */
145 static constexpr uint32_t MAX_GETCFHEADERS_SIZE = 2000;
146+/** the maximum percentage of addresses from our addrman to return in a getaddr call. */
    mzumsande commented at 6:19 pm on August 9, 2020:
    as for “call”, see above
    jnewbery commented at 9:00 am on August 11, 2020:
    fixed
  18. in test/functional/rpc_net.py:171 in 5bff6fdb50 outdated 
    182+        # Obtain addresses via rpc call and check they were ones sent in before.
183+        #
184+        # All addresses added above are in the same netgroup and so are assigned
185+        # to the same bucket. Maximum possible addresses in addrman is therefore
186+        # 64, although actual number will usually be slightly less due to
187+        # BucketPosition collisions.
    mzumsande commented at 6:34 pm on August 9, 2020:
    This comment became outdated.
    jnewbery commented at 9:02 am on August 11, 2020:
    fixed
  19. mzumsande commented at 6:41 pm on August 9, 2020: member
    I did a bit of testing via console, and everything worked as expected (after adding addpeeraddress to vRPCConvertParams). Added some minor issues/nits.
  20. jnewbery force-pushed on Aug 11, 2020
  21. jnewbery commented at 9:04 am on August 11, 2020: member
    Thanks for the review @mzumsande . All comments have been addressed.
  22. in src/addrman.cpp:485 in 81cbe5e87a outdated 
    484 {
485-    unsigned int nNodes = ADDRMAN_GETADDR_MAX_PCT * vRandom.size() / 100;
486-    if (nNodes > ADDRMAN_GETADDR_MAX)
487-        nNodes = ADDRMAN_GETADDR_MAX;
488+    unsigned int nNodes = vRandom.size();
489+    if (max_pct != 0) {
    luke-jr commented at 9:30 pm on August 11, 2020:
    Seems more logical to use >= 100 here?
    jnewbery commented at 7:58 am on August 12, 2020:
    I think both are fine. Calling with 0 to indicate no max seems fine to me too.
  23. in src/addrman.cpp:488 in 81cbe5e87a outdated 
    487-        nNodes = ADDRMAN_GETADDR_MAX;
488+    unsigned int nNodes = vRandom.size();
489+    if (max_pct != 0) {
490+        nNodes = max_pct * nNodes / 100;
491+    }
492+    if (max_addresses != 0) {
    luke-jr commented at 9:31 pm on August 11, 2020:
    Maybe just std::numeric_limits<int>::max()
    jnewbery commented at 7:58 am on August 12, 2020:
    as above
  24. in src/addrman.h:258 in 81cbe5e87a outdated 
    254@@ -261,7 +255,7 @@ friend class CAddrManTest;
255 #endif
256 
257     //! Select several addresses at once.
258-    void GetAddr_(std::vector<CAddress> &vAddr) EXCLUSIVE_LOCKS_REQUIRED(cs);
259+    void GetAddr_(std::vector<CAddress> &vAddr, unsigned int max_addresses, unsigned int max_pct) EXCLUSIVE_LOCKS_REQUIRED(cs);
    luke-jr commented at 9:32 pm on August 11, 2020:
    size_t might be more appropriate for max_addresses
    jnewbery commented at 8:16 am on August 12, 2020:
    ok, changed all of these to size_t
  25. in src/bench/addrman.cpp:101 in 81cbe5e87a outdated 
     97@@ -98,7 +98,7 @@ static void AddrManGetAddr(benchmark::Bench& bench)
 98     FillAddrMan(addrman);
 99 
100     bench.run([&] {
101-        const auto& addresses = addrman.GetAddr();
102+        const auto& addresses = addrman.GetAddr(2500, 23);  // Compatibility with ADDRMAN_GETADDR_MAX_PCT and ADDRMAN_GETADDR_MAX
    luke-jr commented at 9:33 pm on August 11, 2020:
    This comment is confusing. I assume it’s supposed to mean the original values thereof?
    jnewbery commented at 8:20 am on August 12, 2020:
    Yes, so the values used in the bench stay the same. I’ve removed the comment since it’s confusing.
  26. in src/rpc/net.cpp:730 in 81cbe5e87a outdated 
    726@@ -727,7 +727,7 @@ static UniValue getnodeaddresses(const JSONRPCRequest& request)
727             RPCHelpMan{"getnodeaddresses",
728                 "\nReturn known addresses which can potentially be used to find new nodes in the network\n",
729                 {
730-                    {"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."},
731+                    {"count", RPCArg::Type::NUM, /* default */ "1", "The max number of addresses to return. Specify 0 to return all known addresses."},
    luke-jr commented at 9:34 pm on August 11, 2020:
    “maximum”
    luke-jr commented at 9:34 pm on August 11, 2020:
    Would prefer -1 rather than 0 for no limit
    jnewbery commented at 8:20 am on August 12, 2020:
    done
    jnewbery commented at 8:21 am on August 12, 2020:
    as above
  27. in src/rpc/net.cpp:776 in 81cbe5e87a outdated 
    772@@ -775,6 +773,54 @@ static UniValue getnodeaddresses(const JSONRPCRequest& request)
773     return ret;
774 }
775 
776+static UniValue addpeeraddress(const JSONRPCRequest& request)
    luke-jr commented at 9:37 pm on August 11, 2020:
    IMO this should be moved to a separate PR, so not reviewing here.
    jnewbery commented at 8:21 am on August 12, 2020:
    I’m keeping it in this PR since it already has reviews and concept ACKs.
  28. luke-jr changes_requested
  29. [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.
    f26502e9fc
  30. [test] Test that getnodeaddresses() can return all known addresses ae8051bbd8
  31. [net] Add addpeeraddress RPC method 
    Allows addresses to be added to Address Manager for testing.
    37a480e0cd
  32. jnewbery commented at 8:24 am on August 12, 2020: member
    Thanks for the review @luke-jr . I’ve addressed all of your comments.
  33. jnewbery force-pushed on Aug 12, 2020
  34. naumenkogs commented at 10:05 am on August 12, 2020: member

    utACK 37a480e0cd94895b6051abef12d984ff74bdc4a3 I think both methods would be useful.

    You updated rpc_net.py test, but there’s also p2p_getaddr_cache.py and p2p_addr_relay.py which may benefit from new methods?

  35. jnewbery commented at 10:13 am on August 12, 2020: member

    there’s also p2p_getaddr_cache.py and p2p_addr_relay.py which may benefit from new methods?

    Of course. Those can be done in a follow-up.

  36. laanwj commented at 10:44 am on August 12, 2020: member
    Code review and lightly manually tested ACK 37a480e0cd94895b6051abef12d984ff74bdc4a3 I found the default of 1 a bit confusing (because the RPC name is plural), but it’s well documented so also no real problem with it.
  37. jnewbery commented at 11:19 am on August 12, 2020: member

    Thanks @laanwj

    I found the default of 1 a bit confusing (because the RPC name is plural), but it’s well documented so also no real problem with it.

    Yeah, I agree it’s a bit confusing, but I didn’t want to change the default in this PR.

  38. laanwj merged this on Aug 12, 2020
  39. laanwj closed this on Aug 12, 2020
  40. jnewbery deleted the branch on Aug 12, 2020
  41. random-zebra referenced this in commit 69979ce40b on Aug 12, 2021
  42. Fabcien referenced this in commit df5400977f on Sep 8, 2021
  43. Fabcien referenced this in commit 6d3b92de8b on Sep 8, 2021
  44. Fabcien referenced this in commit 95d394f4a6 on Sep 8, 2021
  45. DrahtBot locked this on Feb 15, 2022


jnewbery naumenkogs DrahtBot laanwj mzumsande luke-jr

Labels
RPC/REST/ZMQ P2P

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me