rpc: Add test-only RPC getaddrmaninfo for new/tried table address count #27511

pull stratospher wants to merge 2 commits into bitcoin:master from stratospher:getaddrmaninfo changing 3 files +74 −0
  1. stratospher commented at 4:28 pm on April 21, 2023: contributor

    implements #26907. split off from #26988 to keep RPC, CLI discussions separate.

    This PR introduces a new RPC getaddrmaninfowhich returns the count of addresses in the new/tried table of a node’s addrman broken down by network type. This would be useful for users who want to see the distribution of addresses from different networks across new/tried table in the addrman.

     0$ getaddrmaninfo
 1
 2Result:
 3{                   (json object) json object with network type as keys
 4  "network" : {     (json object) The network (ipv4, ipv6, onion, i2p, cjdns)
 5    "new" : n,      (numeric) number of addresses in new table
 6    "tried" : n,    (numeric) number of addresses in tried table
 7    "total" : n     (numeric) total number of addresses in both new/tried tables from a network
 8  },
 9  ...
10}

    additional context from original PR

    1. network coverage tests were skipped because there’s a small chance that addresses from different networks could hash to the same bucket and cause count of different network addresses in the tests to fail. see #26988 (review).
    2. #26988 uses this RPC in -addrinfo CLI. Slight preference for keeping the RPC hidden since this info will mostly be useful to only super users. see #26988 (review).
  2. DrahtBot commented at 4:28 pm on April 21, 2023: contributor

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

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK 0xB10C, brunoerg, theStack, willcl-ark, achow101
    Concept ACK jonatack
    Stale ACK ryanofsky, mzumsande, amitiuttarwar

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #26988 (cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency by stratospher)

    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.

  3. DrahtBot added the label RPC/REST/ZMQ on Apr 21, 2023
  4. jonatack commented at 5:00 pm on April 21, 2023: contributor
    This PR is based on master from two months age. You may need to rebase it to current master for the (tidy) CI to be green.
  5. jonatack commented at 5:13 pm on April 21, 2023: contributor

    Tested Concept ACK. It might be good to have the breakdown of pre/post IsTerrible filtering (RPC getnodeaddresses and CLI -addrinfo return post) – see the large differences between them.

  6. in src/rpc/net.cpp:1056 in c505611f08 outdated 
     999+                              if (network == NET_UNROUTABLE || network == NET_INTERNAL) continue;
1000+                              UniValue obj(UniValue::VOBJ);
1001+                              obj.pushKV("new", node.addrman->Size(network, true));
1002+                              obj.pushKV("tried", node.addrman->Size(network, false));
1003+                              obj.pushKV("total", node.addrman->Size(network));
1004+                              ret.pushKV(GetNetworkName(network), obj);
    jonatack commented at 5:17 pm on April 21, 2023:
    Maybe a final “totals” field with the totals for each?
    stratospher commented at 2:23 pm on May 15, 2023:
    done. liked the idea.
  7. DrahtBot added the label CI failed on Apr 21, 2023
  8. brunoerg commented at 2:15 pm on April 26, 2023: contributor
    Concept ACK
  9. brunoerg approved
  10. brunoerg commented at 2:31 pm on April 26, 2023: contributor

    tACK c505611f08a850acb97dea9cc03b36aa468929ca

    just tested it with my “clearnet only” node, results:

     0➜  bitcoin-core-dev git:(27511-stratospher) ✗ ./src/bitcoin-cli getaddrmaninfo
 1{
 2  "ipv4": {
 3    "new": 28765,
 4    "tried": 277,
 5    "total": 29042
 6  },
 7  "ipv6": {
 8    "new": 7245,
 9    "tried": 67,
10    "total": 7312
11  },
12  "onion": {
13    "new": 0,
14    "tried": 0,
15    "total": 0
16  },
17  "i2p": {
18    "new": 0,
19    "tried": 0,
20    "total": 0
21  },
22  "cjdns": {
23    "new": 0,
24    "tried": 0,
25    "total": 0
26  }
27}
  11. in test/functional/rpc_net.py:377 in c505611f08 outdated 
    339+        assert "getaddrmaninfo" in node.help("getaddrmaninfo")
340+
341+        # current count of ipv4 addresses in addrman is {'new':1, 'tried':1}
342+        res = node.getaddrmaninfo()
343+        assert_equal(res["ipv4"]["new"], 1)
344+        assert_equal(res["ipv4"]["tried"], 1)
    kevkevinpal commented at 1:50 pm on April 29, 2023:
    Do you think we can add total to this test aswell?
    stratospher commented at 2:24 pm on May 15, 2023:
    yes! done now.
    Jones098 commented at 3:40 am on August 11, 2023:
    Thanks all
  12. mzumsande commented at 6:16 pm on May 12, 2023: contributor

    Code Review ACK c505611f08a850acb97dea9cc03b36aa468929ca, I would use this rpc a lot in my work.

    I also think that a totals field would be useful.

    I don’t think that a split-up with respect to “terrible” would be necessary, because in my opinion “terrible” is an internal property used only in address relay / addrman collisions that is not so critical to expose because it isn’t used for the main purpose of addrman: selecting peers for outbound connections

  13. jonatack commented at 6:24 pm on May 12, 2023: contributor

    “terrible” is an internal property

    It’s used in RPC getnodeaddresses and CLI -addrinfo to return only non-IsTerrible peers to users. (I meant pre and post that filtering in the suggestion above).

  14. mzumsande commented at 6:39 pm on May 12, 2023: contributor

    It’s used in RPC getnodeaddresses and CLI -addrinfo to return only non-IsTerrible peers to users. (I meant pre and post that filtering in the suggestion above).

    in my opinion unfortunately. I think this was mostly coincidental, because #13152 chose to reuse an addrman function used only for GETADDR answers before. Then #21595 used that output without any mentioning of that restriction, and the doc was only adjusted a year later in #24370. I think that ideally we never should have exposed the terrible filter in the first place.

  15. fanquake commented at 10:30 am on May 15, 2023: member

    I think that ideally we never should have exposed the terrible filter in the first place. @mzumsande do you want to propose some changes to remove its exposure?

  16. jonatack commented at 12:41 pm on May 15, 2023: contributor

    The author and the numerous reviewers of #13152 were aware of the relationship to getaddr (see the PR description and review discussion), as well as mention of IsTerrible in #13152#pullrequestreview-121943454.

    I don’t see it as unfortunate; as a frequent user of getnodeaddresses and -addrinfo it seems useful that the data doesn’t include terrible/non-recent (and banned/discouraged) peers.

    But guessing whether it was coincidental is beside the point. What is relevant is that changing those calls to return different data would break user space, including any tracking over time using that data.

    Adding data is fine, but let’s not needlessly break user space.

  17. jonatack commented at 12:50 pm on May 15, 2023: contributor
    @stratospher do you plan to repush after rebase to current master for the CI and perhaps with the totals?
  18. stratospher force-pushed on May 15, 2023
  19. mzumsande commented at 2:31 pm on May 15, 2023: contributor

    @mzumsande do you want to propose some changes to remove its exposure?

    no, I don’t want to spend time on discussions on whether some change breaks user space or not. I’d be happy to have an alternative way to query the actual, unfiltered numbers per network, which are much more relevant to my use cases as a developer.

    I don’t see it as unfortunate; as a frequent user of getnodeaddresses and -addrinfo it seems useful that the data doesn’t include terrible/non-recent (and banned/discouraged) peers.

    For me, it makes no sense that these commands might withhold our very next outbound peer - which would be picked with no disadvantage over non-terrible peers if it, for example, just has an older timestamp. Or, when dealing with a peers.dat that is >30 days old, that these commands won’t give any results at all anymore, even though the peers.dat is still perfectly useful and able to find peers (many nodes are reachable under the same address for way longer than a month).

  20. jonatack commented at 3:34 pm on May 15, 2023: contributor

    I’d be happy to have an alternative way to query the actual, unfiltered numbers per network

    I think that would be useful, too, perhaps by passing an argument to getnodeaddresses and -addrinfo, and/or returning both pre and post filter in -addrinfo.

  21. DrahtBot removed the label CI failed on May 15, 2023
  22. stratospher commented at 4:28 pm on May 15, 2023: contributor

    so sorry for the delay and thank you for the suggestions! the CI is all green now. i’ve updated the PR to include:

    • addrman new/tried/total count for “all networks”
    • test coverage for “all networks” and for total (total refers to both new/tried table count)

    I’d want this RPC to be the alternative way to access the actual count of all addresses in the addrman.

  23. mzumsande commented at 2:36 pm on May 16, 2023: contributor
    Tested ACK 7d7ee5e0f4254495349ea68f3d0127c0c336fd12
  24. DrahtBot requested review from brunoerg on May 16, 2023
  25. fanquake commented at 4:43 pm on May 16, 2023: member

    no, I don’t want to spend time on discussions on whether some change breaks user space or not. @mzumsande fair enough. I don’t quite understand what “user space” means, when it has been used in comments here (and in other PRs), as we are not the Linux Kernel, but I assume it’s a somewhat less direct way of saying backwards compatibility/continuity?

    In any case, as far as I’m aware, we offer no special “backwards compatiblity”, or other, guarantees for our CLI tools, and I don’t really think we should. Doing so would put us in an awkward position, of having our RPCs backwards compatibility requirements, extended to those tools (consumers, which just happen to exist in this codebase).

    We don’t want to be in position where we can’t change an RPC, to stop it doing something possibly incorrect/illogical, just because a CLI tool happens to call that RPC. These tools shouldn’t get any special treatment, and, as far as I’m concerned, should just adapt to any RPC changes we might make, like every other consumer (internal or external) of that RPC.

  26. DrahtBot added the label CI failed on May 16, 2023
  27. DrahtBot removed the label CI failed on May 17, 2023
  28. ryanofsky commented at 7:10 pm on June 9, 2023: contributor
    The code change here seems very simple, but since this PR does add a new RPC it would be useful to have more concept ACKs to know whether this is appropriate to merge. I noticed a number of contributors added github reactions to some comments here, so perhaps more of the people following the discussion could add concept acks or mention any reservations
  29. in src/rpc/net.cpp:973 in 332cf61a5c outdated 
    966@@ -967,6 +967,53 @@ static RPCHelpMan addpeeraddress()
967     };
968 }
969 
970+static RPCHelpMan getaddrmaninfo()
971+{
972+    return RPCHelpMan{"getaddrmaninfo",
973+                      "\nReturns the number of addresses in the `new` and `tried` tables and their sum for all networks. This RPC is for testing only.\n",
    ryanofsky commented at 3:06 pm on July 20, 2023:

    In commit “rpc: Introduce getaddrmaninfo for count of addresses stored in new/tried table” (332cf61a5cdf838a20117280dc2d24659a4b4a5c)

    It seems inaccurate to say “RPC is for testing only” when PR description says “This would be useful for users who want to see the distribution of addresses from different networks across new/tried table in the addrman” which makes it sound like this is useful for troubleshooting or monitoring.

    If the RPC is not intended just to test the software, I think it would also be nice if documentation gave a little hint of what the new and tried are tables so the functionality is discoverable.

    I asked ChatGPT to write a documentation for a getaddrmaninfo method and it went a little overboard but maybe the new/tried descriptions are useful:

    getaddrmaninfo Method Documentation

    Method Name: getaddrmaninfo

    Description: The getaddrmaninfo method is a custom JSON-RPC method available in bitcoind, which provides high-level information about the Address Manager (addrman) module. The Address Manager is responsible for managing and organizing the addresses of network nodes in the Bitcoin peer-to-peer network.

    Parameters: None

    Result:

    • new_addresses: An integer representing the current number of addresses stored in the “new” table of the Address Manager. These are addresses that have been recently discovered but have not yet been successfully connected to by the node.

    • tried_addresses: An integer representing the current number of addresses stored in the “tried” table of the Address Manager. These are addresses of known nodes that the node has successfully connected to in the past and are considered accessible peers.

    Note: The Address Manager plays a crucial role in maintaining a healthy and well-connected peer-to-peer network. The “new” table stores potential new peers that are periodically tested for connectivity to increase the node’s network reach. The “tried” table contains addresses of reliable and accessible peers with whom the node has established successful connections.

    Usage: The getaddrmaninfo method allows users to monitor the state of the Address Manager. By calling this method, users can retrieve the current counts of addresses in the “new” and “tried” tables, providing valuable insights into the node’s ongoing efforts to discover new peers and maintain connections to known reliable nodes.

    Example Output:

    0{
1  "new_addresses": 256,
2  "tried_addresses": 1024
3}

    In this example, the node currently has 256 addresses in the “new” table and 1024 addresses in the “tried” table. These counts reflect the node’s active efforts to discover new peers (new addresses) and the established connections with known reliable peers (tried addresses) in the Bitcoin network.

    stratospher commented at 5:57 pm on July 20, 2023:

    “RPC is for testing only” was used since this is a hidden RPC.

    I think it would also be nice if documentation gave a little hint of what the new and tried are tables so the functionality is discoverable.

    liked the ChatGPT documentation! will update the PR to use some of this.

  30. ryanofsky approved
  31. ryanofsky commented at 3:17 pm on July 20, 2023: contributor
    Code review ACK 7d7ee5e0f4254495349ea68f3d0127c0c336fd12
  32. stratospher force-pushed on Jul 24, 2023
  33. stratospher commented at 10:32 am on July 24, 2023: contributor

    updated the PR to include better docs inspired from #27511 (review). left “RPC is for testing only” as it is since all hidden RPCs have that line in the help. getaddrmaninfo’s help looks like this now:

     0Provides high-level information about the node's address manager by returning the number of addresses in the `new` and `tried` tables and their sum for all networks.
 1This RPC is for testing only.
 2
 3Result:
 4{                   (json object) json object with network type as keys
 5  "network" : {     (json object) the network (ipv4, ipv6, onion, i2p, cjdns)
 6    "new" : n,      (numeric) number of addresses in new table. The new table contains addresses of potential new peers that are periodically tested for connectivity to increase the node's network reach.
 7    "tried" : n,    (numeric) number of addresses in tried table. The tried table contains addresses of peers with whom the node has successfully connected to in the past.
 8    "total" : n     (numeric) total number of addresses in both new/tried tables from a network
 9  },
10  ...
11}
12
13Examples:
14> bitcoin-cli getaddrmaninfo 
15> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "getaddrmaninfo", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:8332/
  34. in src/rpc/net.cpp:984 in 6265bf90b7 outdated 
    979+                              {
980+                                      {RPCResult::Type::OBJ, "network", "the network (" + Join(GetNetworkNames(), ", ") + ")",
981+                                       {
982+                                               {RPCResult::Type::NUM, "new", "number of addresses in new table. The new table contains addresses of potential new peers that are periodically tested for connectivity to increase the node's network reach."},
983+                                               {RPCResult::Type::NUM, "tried", "number of addresses in tried table. The tried table contains addresses of peers with whom the node has successfully connected to in the past."},
984+                                               {RPCResult::Type::NUM, "total", "total number of addresses in both new/tried tables from a network"},
    amitiuttarwar commented at 5:46 am on July 30, 2023: 
    0                                               {RPCResult::Type::NUM, "total", "total number of addresses in both new/tried tables"},

    having the clause “from a network” only on total field (vs also on new and tried) is confusing. a bit redundant anyways since these fields are all nested within the networks

    stratospher commented at 4:34 am on August 11, 2023:
    done
  35. in src/rpc/net.cpp:982 in 6265bf90b7 outdated 
    977+                      RPCResult{
978+                              RPCResult::Type::OBJ_DYN, "", "json object with network type as keys",
979+                              {
980+                                      {RPCResult::Type::OBJ, "network", "the network (" + Join(GetNetworkNames(), ", ") + ")",
981+                                       {
982+                                               {RPCResult::Type::NUM, "new", "number of addresses in new table. The new table contains addresses of potential new peers that are periodically tested for connectivity to increase the node's network reach."},
    amitiuttarwar commented at 5:49 am on July 30, 2023: 
    0                                               {RPCResult::Type::NUM, "new", "number of addresses in the new table, which represent potential peers."},

    the fact that they are periodically tested seems like an implementation detail of addrman and feels a little confusing here.

    amitiuttarwar commented at 6:11 am on July 30, 2023:
    reading the chatGPT suggestion, if you want more detail, I think it’d make more sense to use the part that says they are recently discovered but have not yet been successfully connected to.
    fanquake commented at 3:40 pm on July 31, 2023:
    Maybe relevant here, I think it’s easier to just avoid (explicitly) copy-pasting ChatGPT or similar output into the project, for now, see #28175.
    stratospher commented at 4:33 am on August 11, 2023:
    done. added some more detail to the part about new table.
  36. in src/rpc/net.cpp:983 in 6265bf90b7 outdated 
    978+                              RPCResult::Type::OBJ_DYN, "", "json object with network type as keys",
979+                              {
980+                                      {RPCResult::Type::OBJ, "network", "the network (" + Join(GetNetworkNames(), ", ") + ")",
981+                                       {
982+                                               {RPCResult::Type::NUM, "new", "number of addresses in new table. The new table contains addresses of potential new peers that are periodically tested for connectivity to increase the node's network reach."},
983+                                               {RPCResult::Type::NUM, "tried", "number of addresses in tried table. The tried table contains addresses of peers with whom the node has successfully connected to in the past."},
    amitiuttarwar commented at 5:52 am on July 30, 2023: 
    0                                               {RPCResult::Type::NUM, "tried", "number of addresses in the tried table, which represent peers with whom the node has successfully connected to in the past."},

    if you take the previous suggestion, I’d update this too for consistency

    stratospher commented at 4:34 am on August 11, 2023:
    done
  37. in test/functional/rpc_net.py:378 in 69abfd3db1 outdated 
    340+
341+        # current count of ipv4 addresses in addrman is {'new':1, 'tried':1}
342+        res = node.getaddrmaninfo()
343+        assert_equal(res["ipv4"]["new"], 1)
344+        assert_equal(res["ipv4"]["tried"], 1)
345+        assert_equal(res["ipv4"]["total"], 2)
    amitiuttarwar commented at 5:56 am on July 30, 2023:
    could also check that the values for other networks are 0
    stratospher commented at 4:34 am on August 11, 2023:
    done
  38. amitiuttarwar commented at 6:09 am on July 30, 2023: contributor

    tested ACK 69abfd3db1

    all my feedback is very optional, code looks fine as is. I saw the previous review comments & update around wording suggestions & don’t think it’s worth much churn just for that, but left my language thoughts just for consideration.

    agreed that it makes most sense to return just the totals here, and that generall the IsTerrible info is an internal implementation detail that would ideally not be exposed to the users.

    I considered the approach of the RPC endpoint exclusively returning results for networks that pass the IsReachable test, but it is possible for addrman to have addresses for currently unreachable networks based on previous configurations, so checking all networks seems better.

    that said, a reasonable possibility would be to remove the results for networks where the total is 0.

  39. DrahtBot requested review from mzumsande on Jul 30, 2023
  40. DrahtBot requested review from ryanofsky on Jul 30, 2023
  41. stratospher force-pushed on Aug 11, 2023
  42. stratospher force-pushed on Aug 11, 2023
  43. stratospher commented at 4:57 am on August 11, 2023: contributor

    thanks @amitiuttarwar! addressed #27511 (comment) to have better documentation for new/tried table. getaddrmaninfo’s help looks like this now:

     0getaddrmaninfo
 1
 2Provides information about the node's address manager by returning the number of addresses in the `new` and `tried` tables and their sum for all networks.
 3This RPC is for testing only.
 4
 5Result:
 6{                   (json object) json object with network type as keys
 7  "network" : {     (json object) the network (ipv4, ipv6, onion, i2p, cjdns)
 8    "new" : n,      (numeric) number of addresses in the new table, which represent potential peers the node discovers but hasn't yet successfully connected to.
 9    "tried" : n,    (numeric) number of addresses in the tried table, which represent peers with whom the node has successfully connected to in the past.
10    "total" : n     (numeric) total number of addresses in both new/tried tables
11  },
12  ...
13}
14
15Examples:
16> bitcoin-cli getaddrmaninfo 
17> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "getaddrmaninfo", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:8332/

    that said, a reasonable possibility would be to remove the results for networks where the total is 0.

    i think i’ll keep it as it is for now since removing it wouldn’t make much of a difference as it’s just 5 networks + 1 total.

    there was also a discussion in #26988 (comment) (which reworks a CLI using this RPC) whether to keep this RPC hidden/public. it was kept as a hidden RPC because:

    1. a normal user wouldn’t be interested in the new/tried table breakdown of addresses.
    2. -generate CLI currently uses a hidden RPC (generatetoaddress) too.

    If ever sufficient people end up being interested in the CLI rework and also feel that we’d need the RPC to be public in the CLI (despite being used by super users mostly), we could make it public in the CLI rework PR (#26982).

    So yeah, i feel it’s better to keep this RPC hidden for now since this info will mostly be useful to only super users. Would like to know what others think.

  44. stratospher force-pushed on Aug 15, 2023
  45. amitiuttarwar commented at 4:16 am on August 16, 2023: contributor

    ACK 6d7fd9fb9b38be75b2bce643a99cc584837a902b

    keeping it as a hidden RPC makes sense to me

  46. luke-jr referenced this in commit ebe4b67766 on Aug 16, 2023
  47. luke-jr referenced this in commit 74478f0c67 on Aug 16, 2023
  48. in src/rpc/net.cpp:983 in a903f2529e outdated 
    978+                              RPCResult::Type::OBJ_DYN, "", "json object with network type as keys",
979+                              {
980+                                      {RPCResult::Type::OBJ, "network", "the network (" + Join(GetNetworkNames(), ", ") + ")",
981+                                       {
982+                                               {RPCResult::Type::NUM, "new", "number of addresses in the new table, which represent potential peers the node has discovered but hasn't yet successfully connected to."},
983+                                               {RPCResult::Type::NUM, "tried", "number of addresses in the tried table, which represent peers with whom the node has successfully connected to in the past."},
    mzumsande commented at 8:50 pm on August 21, 2023:
    grammar nit: would drop “with whom”
    stratospher commented at 5:26 am on August 22, 2023:
    done
  49. mzumsande commented at 8:53 pm on August 21, 2023: contributor
    ACK 6d7fd9fb9b38be75b2bce643a99cc584837a902b
  50. stratospher force-pushed on Aug 22, 2023
  51. mzumsande commented at 2:30 pm on August 22, 2023: contributor
    ACK 3803efb, only change was to improve the RPC doc.
  52. DrahtBot requested review from amitiuttarwar on Aug 22, 2023
  53. DrahtBot added the label Needs rebase on Aug 24, 2023
  54. stratospher force-pushed on Aug 27, 2023
  55. DrahtBot removed the label Needs rebase on Aug 27, 2023
  56. DrahtBot added the label CI failed on Aug 27, 2023
  57. DrahtBot removed the label CI failed on Aug 28, 2023
  58. luke-jr referenced this in commit 31484b440b on Aug 29, 2023
  59. luke-jr referenced this in commit cad1f31039 on Aug 29, 2023
  60. mzumsande commented at 9:48 pm on August 31, 2023: contributor
    ACK e4f9b4097e8fd2e3aeb9bf0683cd0d9c1199319d (only rebase)
  61. amitiuttarwar commented at 7:04 pm on September 1, 2023: contributor
    ACK e4f9b4097e8fd2e3aeb9bf0683cd0d9c1199319d (rebase + small wording update)
  62. DrahtBot removed review request from amitiuttarwar on Sep 1, 2023
  63. willcl-ark requested review from 0xB10C on Sep 18, 2023
  64. in src/rpc/net.cpp:1062 in e4f9b4097e outdated 
    1057+                          }
1058+                          UniValue obj(UniValue::VOBJ);
1059+                          obj.pushKV("new", node.addrman->Size(std::nullopt, true));
1060+                          obj.pushKV("tried", node.addrman->Size(std::nullopt, false));
1061+                          obj.pushKV("total", node.addrman->Size());
1062+                          ret.pushKV("all networks", obj);
    0xB10C commented at 3:31 pm on September 18, 2023:

    I think having spaces in JSON keys could cause problems for some software. Why not use all_networks (or sum, all, …).

    0                          ret.pushKV("all_networks", obj);
    stratospher commented at 0:03 am on September 20, 2023:
    true! done.
  65. 0xB10C commented at 3:33 pm on September 18, 2023: contributor
    Reviewed the changes and manually tested the RPC. Looks good besides maybe renaming all networks to all_networks.
  66. 0xB10C commented at 3:42 pm on September 18, 2023: contributor
    It came up in a discussion that a follow-up to this PR could add a list of addresses (with source, time added, bucket, position, network, …) to this RPC with a verbose flag.
  67. in test/functional/rpc_net.py:373 in e4f9b4097e outdated 
    368+        self.log.debug("Test that getaddrmaninfo is a hidden RPC")
369+        # It is hidden from general help, but its detailed help may be called directly.
370+        assert "getaddrmaninfo" not in node.help()
371+        assert "getaddrmaninfo" in node.help("getaddrmaninfo")
372+
373+        # current count of ipv4 addresses in addrman is {'new':1, 'tried':1}
    willcl-ark commented at 4:30 pm on September 18, 2023:
    nit: if you re-touch could add a debug log line what part of the new functionality we are testing as on L368
    stratospher commented at 0:03 am on September 20, 2023:
    done.
  68. willcl-ark approved
  69. willcl-ark commented at 4:37 pm on September 18, 2023: contributor

    ACK e4f9b4097e

    I would like to be a user of this data. Like @0xB10C I would be interested in seeing a followup to this which exposed even more granular details, to permit monitoring of the node’s addrman without the need for tracepoints or logging.

    Happy to re-ACK if the JSON key gets updated.

  70. brunoerg approved
  71. brunoerg commented at 4:50 pm on September 18, 2023: contributor

    ACK e4f9b4097e8fd2e3aeb9bf0683cd0d9c1199319d

    Just tested it again and got expected result. I agree on JSON key getting updated and can reACK it as soon as it gets updated.

  72. rpc: Introduce getaddrmaninfo for count of addresses stored in new/tried table c8eb8dae51
  73. stratospher force-pushed on Sep 19, 2023
  74. brunoerg approved
  75. brunoerg commented at 11:05 am on September 19, 2023: contributor

    reACK 0bff92a3b34fea764efdfc474e4d69bb7da0882e

    edit: needs to update the functional tests

  76. DrahtBot requested review from amitiuttarwar on Sep 19, 2023
  77. DrahtBot requested review from willcl-ark on Sep 19, 2023
  78. DrahtBot requested review from mzumsande on Sep 19, 2023
  79. in test/functional/rpc_net.py:379 in 0bff92a3b3 outdated 
    374+        self.log.info("Test that count of addresses in addrman match expected values")
375+        res = node.getaddrmaninfo()
376+        assert_equal(res["ipv4"]["new"], 1)
377+        assert_equal(res["ipv4"]["tried"], 1)
378+        assert_equal(res["ipv4"]["total"], 2)
379+        assert_equal(res["all networks"]["new"], 1)
    brunoerg commented at 11:08 am on September 19, 2023: 
    0        assert_equal(res["all_networks"]["new"], 1)
    stratospher commented at 0:04 am on September 20, 2023:
    done.
  80. DrahtBot added the label CI failed on Sep 19, 2023
  81. test: add functional test for getaddrmaninfo 28bac81a34
  82. stratospher force-pushed on Sep 19, 2023
  83. DrahtBot removed the label CI failed on Sep 19, 2023
  84. 0xB10C commented at 11:10 pm on September 19, 2023: contributor
    ACK 28bac81a346c0b68273fa73af924f7096cb3f41d
  85. DrahtBot requested review from brunoerg on Sep 19, 2023
  86. brunoerg approved
  87. brunoerg commented at 9:05 am on September 20, 2023: contributor

    reACK 28bac81a346c0b68273fa73af924f7096cb3f41d

    CI failure seems unrelated.

  88. in src/rpc/net.cpp:1046 in c8eb8dae51 outdated 
    1041+                      [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
1042+                      {
1043+                          NodeContext& node = EnsureAnyNodeContext(request.context);
1044+                          if (!node.addrman) {
1045+                              throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Address manager functionality missing or disabled");
1046+                          }
    theStack commented at 11:40 am on September 20, 2023:
    nit: to deduplicate code, could introduce a helper function EnsureAddrman in rpc/server_util.cpp (similar to the already existing EnsureConnman and EnsurePeerman) and use that here and in the addpeeraddress RPC.
    stratospher commented at 2:14 pm on October 3, 2023:
    done in #28565.
  89. theStack approved
  90. theStack commented at 11:44 am on September 20, 2023: contributor
    Code-review ACK 28bac81a346c0b68273fa73af924f7096cb3f41d
  91. willcl-ark commented at 12:10 pm on September 20, 2023: contributor
    reACK 28bac81a346c0b68273fa73af924f7096cb3f41d
  92. DrahtBot removed review request from willcl-ark on Sep 20, 2023
  93. achow101 commented at 12:19 pm on September 20, 2023: member
    ACK 28bac81a346c0b68273fa73af924f7096cb3f41d
  94. achow101 merged this on Sep 20, 2023
  95. achow101 closed this on Sep 20, 2023
  96. in src/rpc/net.cpp:1029 in c8eb8dae51 outdated 
    1024+                      "This RPC is for testing only.\n",
1025+                      {},
1026+                      RPCResult{
1027+                              RPCResult::Type::OBJ_DYN, "", "json object with network type as keys",
1028+                              {
1029+                                      {RPCResult::Type::OBJ, "network", "the network (" + Join(GetNetworkNames(), ", ") + ")",
    0xB10C commented at 0:46 am on September 24, 2023:

    nit: This is missing the all_networks key in the RPC help.

    ~Can add a fix for this in #28523 as this touches the line.~

    0xB10C commented at 12:19 pm on September 25, 2023:
    ~included in #28523~ Not touching getaddrmaninfo anymore in #28523.
    stratospher commented at 5:27 am on October 3, 2023:
    done in #28565
  97. Frank-GER referenced this in commit da7ea225a7 on Sep 25, 2023
  98. sidhujag referenced this in commit f2c8dfcc9a on Sep 26, 2023
  99. 0xB10C referenced this in commit fbc29f2ead on Sep 27, 2023
  100. 0xB10C referenced this in commit 1f5c8dc5a0 on Sep 27, 2023
  101. stratospher referenced this in commit 0538579abd on Oct 3, 2023
  102. stratospher referenced this in commit b62f7d678f on Oct 3, 2023


stratospher DrahtBot jonatack brunoerg kevkevinpal Jones098 mzumsande fanquake ryanofsky amitiuttarwar 0xB10C willcl-ark theStack achow101


amitiuttarwar ryanofsky mzumsande

Labels
RPC/REST/ZMQ

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-07-05 19:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me