cli: create -addrinfo #21595

pull jonatack wants to merge 5 commits into bitcoin:master from jonatack:addressinfo changing 2 files +73 −11
  1. jonatack commented at 3:22 pm on April 4, 2021: member

    While looking at issue #21351, it turned out that the problem was a lack of tor v3 addresses known to the node. It became clear (e.g. #21351 (comment)) that a CLI command returning the number of addresses the node knows per network (with a tor v2 / v3 breakdown) would be very helpful. This patch adds that.

    -addrinfo is useful to see if your node knows enough addresses in a network to use options like -onlynet=<network>, or to upgrade to the upcoming tor release that no longer supports tor v2, for which you’ll need to be sure your node knows enough tor v3 peers.

     0$ bitcoin-cli --help | grep -A1 addrinfo
     1  -addrinfo
     2       Get the number of addresses known to the node, per network and total.
     3
     4$ bitcoin-cli -addrinfo
     5{
     6  "addresses_known": {
     7    "ipv4": 14406,
     8    "ipv6": 2511,
     9    "torv2": 5563,
    10    "torv3": 2842,
    11    "i2p": 8,
    12    "total": 25330
    13  }
    14}
    15
    16$ bitcoin-cli -addrinfo 1
    17error: -addrinfo takes no arguments
    

    This can be manually tested, for example, with commands like this:

    0$ bitcoin-cli getnodeaddresses 0 | jq '.[] | (select(.address | contains(".onion")) | select(.address | length <= 22)) | .address' | wc -l
    15563
    2$ bitcoin-cli getnodeaddresses 0 | jq '.[] | (select(.address | contains(".onion")) | select(.address | length > 22)) | .address' | wc -l
    32842
    4$ bitcoin-cli getnodeaddresses 0 | jq '.[] | .address' | wc -l
    525330
    
  2. DrahtBot added the label RPC/REST/ZMQ on Apr 4, 2021
  3. 78051301012 approved
  4. jonatack force-pushed on Apr 4, 2021
  5. DrahtBot commented at 5:05 pm on April 4, 2021: 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:

    • #21158 (lib: Add Taproot support to libconsensus by jrawsthorne)

    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.

  6. practicalswift commented at 9:29 pm on April 4, 2021: contributor
    Concept ACK: nice! :)
  7. jarolrod commented at 0:29 am on April 5, 2021: member

    ACK 5f1ffbfa3fccb145a6724ef33f8de92d6b652a31

    Tested on macOS 11.2.3

     0xyz@xyzs-MBP bitcoin % ./src/bitcoin-cli -testnet -addressinfo
     1{
     2  "addresses known": {
     3    "ipv4": 9774,
     4    "ipv6": 2211,
     5    "torv2": 0,
     6    "torv3": 0,
     7    "i2p": 0,
     8    "total": 11985
     9  }
    10}
    

    Question: Why make this a CLI command and not an RPC command? Seems useful as an RPC command.

  8. jonatack commented at 11:10 am on April 5, 2021: member

    Thanks!

    Question: Why make this a CLI command and not an RPC command? Seems useful as an RPC command.

    It could, or a third path could be to add a boolean totals arg to the getnodeaddresses RPC to toggle a “totals mode” (that said, it might be a bit obscure/hard to discover).

    Generally, CLI commands batch or wrap one or multiple RPC commands and are easier to change and improve without worrying about API stability. For example, this might evolve to returning other address info or statistics. In this case where only one RPC is wrapped, instead of batching multiple RPCs like -getinfo, -netinfo and -generate do, I don’t have a strong opinion on the RPC vs CLI question.

  9. jonatack commented at 11:13 am on April 5, 2021: member
    Not sure, but it’s maybe more flexible to start life as a CLI and then become an RPC if there’s a good reason, than vice-versa.
  10. laanwj commented at 12:25 pm on April 5, 2021: member

    Concept ACK

    Question: Why make this a CLI command and not an RPC command? Seems useful as an RPC command.

    Minimalism. I think it’s a good choice to put functionality in the client that can be done client-side. Adding things server-side adds maintenance burden, means having to commit to a certain (machine parseable) interface, etc. And this is clearly functionality aimed at end users. Also agree with

    Not sure, but it’s maybe more flexible to start life as a CLI and then become an RPC if there’s a good reason, than vice-versa.

    Right, it doesn’t rule out making it a server-side RPC either. This is just the path of least resistance.

  11. jarolrod commented at 5:37 pm on April 5, 2021: member
    @laanwj @jonatack Thanks for answering my question. Maybe this information, when a command should be a CLI or RPC command, should be added to the developer-notes.
  12. in src/bitcoin-cli.cpp:617 in 57eb71ce0f outdated
    612+        for (uint8_t i = 0; i < m_networks_size; ++i) {
    613+            addresses.pushKV(m_networks.at(i), m_counts.at(i));
    614+            m_total += m_counts.at(i);
    615+        }
    616+        addresses.pushKV("total", m_total);
    617+        result.pushKV("addresses known", addresses);
    


    promag commented at 8:17 am on April 7, 2021:

    57eb71ce0f27c0353ea0742a696c6d04ce048e15

    IMO avoid spaces in keys.


    jonatack commented at 10:36 am on April 7, 2021:
    Good point, will snakecase it if this remains in JSON format. I wonder if it should drop the JSON format (like -netinfo) for the client side.

    jonatack commented at 4:21 pm on April 8, 2021:
    done
  13. in src/bitcoin-cli.cpp:579 in 57eb71ce0f outdated
    574+{
    575+private:
    576+    static constexpr uint8_t m_networks_size{5};
    577+    const std::array<std::string, m_networks_size> m_networks{{"ipv4", "ipv6", "torv2", "torv3", "i2p"}};
    578+    std::array<uint64_t, m_networks_size> m_counts{{}}; // Address counts by network
    579+    uint64_t m_total{0}; // Total address count
    


    promag commented at 8:21 am on April 7, 2021:

    57eb71ce0f27c0353ea0742a696c6d04ce048e15

    No need to have m_networks and m_total as members.


    jonatack commented at 4:23 pm on April 8, 2021:
    Changed m_counts and m_total to be local variables instead (counts and total).
  14. in src/bitcoin-cli.cpp:576 in 57eb71ce0f outdated
    568@@ -569,6 +569,56 @@ class NetinfoRequestHandler : public BaseRequestHandler
    569         "> bitcoin-cli -netinfo help\n"};
    570 };
    571 
    572+/** Process addressinfo requests */
    573+class AddressinfoRequestHandler : public BaseRequestHandler
    574+{
    575+private:
    576+    static constexpr uint8_t m_networks_size{5};
    


    promag commented at 8:22 am on April 7, 2021:

    57eb71ce0f27c0353ea0742a696c6d04ce048e15

    Drop, and use m_networks size?


    jonatack commented at 4:40 pm on April 8, 2021:
    I would if could omit the template arguments.
  15. in src/bitcoin-cli.cpp:577 in 57eb71ce0f outdated
    568@@ -569,6 +569,56 @@ class NetinfoRequestHandler : public BaseRequestHandler
    569         "> bitcoin-cli -netinfo help\n"};
    570 };
    571 
    572+/** Process addressinfo requests */
    573+class AddressinfoRequestHandler : public BaseRequestHandler
    574+{
    575+private:
    576+    static constexpr uint8_t m_networks_size{5};
    577+    const std::array<std::string, m_networks_size> m_networks{{"ipv4", "ipv6", "torv2", "torv3", "i2p"}};
    


    promag commented at 8:24 am on April 7, 2021:

    57eb71ce0f27c0353ea0742a696c6d04ce048e15

    I think you can omit template arguments, see https://stackoverflow.com/a/61371266/1978589.


    jonatack commented at 10:34 am on April 7, 2021:
    Thanks, will give it a try in the next push.

    jonatack commented at 4:23 pm on April 8, 2021:
    Couldn’t get that to compile (clang 11).
  16. promag changes_requested
  17. promag commented at 8:40 am on April 7, 2021: member

    Code review ACK 5f1ffbfa3fccb145a6724ef33f8de92d6b652a31 with some suggestions.

    But I’m not fond of this approach. This is fetching a list from the server and doing an aggregation on the client, this can be done by other means. I would be in favor of implementing this on the server so we get it right for all clients, GUI included.

    I’m also not fond of the name -addressinfo as it looks like something specific to an address only, I would name it -nodestats or something like that.

  18. jonatack commented at 10:41 am on April 7, 2021: member

    I would be in favor of implementing this on the server so we get it right for all clients, GUI included.

    The GUI is a good point, and performance could also be better (not sure if it would be noticeable) as an RPC.

    I’m also not fond of the name -addressinfo as it looks like something specific to an address only, I would name it -nodestats or something like that.

    I proposed -addressinfo in line with CLI commands -getinfo and -netinfonodestats isn’t bad though and is shorter. Hm…

  19. jonatack commented at 10:43 am on April 7, 2021: member
    Note to self: as a CLI this needs a server version check like the one in -netinfo to gracefully raise if the server is earlier than v22, due to this depending on the network field in getnodeaddresses.
  20. jonatack commented at 4:34 pm on April 8, 2021: member
    • Rebased following the merge of #21594
    • Took review feedback where applicable
    • Re “-nodestats”, I couldn’t get command names beginning with “-node” to work (e.g. -nodestats, -nodeinfo); it compiles, but when run: “Error parsing command line arguments: Invalid parameter -nodestats”…not sure why. OTOH, -peerstats does work.
    • Shortened the command from -addressinfo to -addrinfo, as shorter is better for humans typing manually
    • Added the server version check which raises with a helpful message
    • Improved the release note
  21. jonatack force-pushed on Apr 8, 2021
  22. jonatack renamed this:
    cli: create -addressinfo
    cli: create -addrinfo
    on Apr 8, 2021
  23. cli: create AddrinfoRequestHandler class db4d2c282a
  24. cli: add -addrinfo command 5056a37624
  25. doc: add cli -addrinfo release note bb85cbc4f7
  26. jonatack force-pushed on Apr 9, 2021
  27. jarolrod commented at 7:44 pm on April 12, 2021: member

    tACK c0986af3c1a7eba860f9375674f03aaa365e4f72

    Tested on Ubuntu and macOS 11.2.3

    I think the change from -addressinfo to -addrinfo is more appropriate as -addressinfo does sound like you are trying to get information about a specific address.

    one NIT: The behavior when the server is initiating is a bit different than other CLI commands like -getinfo and -netinfo. When you call either of these while the server is starting up you’ll get something like this:

    0error code: -28
    1error message:
    2Loading block index...
    

    When calling this command, -addrinfo, while the server is starting you’ll get an error about the output:

    0error: JSON value is not an object or array as expected
    

    I don’t know how important this is, but maybe we should keep this behavior consistent?

  28. jonatack commented at 7:47 pm on April 12, 2021: member
    Good find! I agree this isn’t great, will look at fixing.
  29. addrinfo: raise helpfully on server error or incompatible server version edf3167151
  30. jonatack force-pushed on Apr 12, 2021
  31. jonatack commented at 8:17 pm on April 12, 2021: member

    Thanks again for finding that @jarolrod. Fixed in the last commit per git diff c0986af edf3167. Much better.

    0@@ -256,6 +256,7 @@ public:
    1 
    2     UniValue ProcessReply(const UniValue& reply) override
    3     {
    4+        if (!reply["error"].isNull()) return reply;
    5         const std::vector<UniValue>& nodes{reply["result"].getValues()};
    
    0$ bitcoin-cli -addrinfo
    1error code: -28
    2error message:
    3Loading block index...
    

    Note that the server version message can be tested by building and running with a change like

    0--- a/src/bitcoin-cli.cpp
    1+++ b/src/bitcoin-cli.cpp
    2@@ -264,7 +264,7 @@ public:
    3         // Count the number of peers we know by network, including torv2 versus torv3.
    4         std::array<uint64_t, m_networks_size> counts{{}};
    5         for (const UniValue& node : nodes) {
    6-        if (!nodes.empty() && nodes.at(0)["network"].isNull()) {
    7+        if (!nodes.empty() && nodes.at(0)["networ"].isNull()) {
    8             throw std::runtime_error("-addrinfo requires bitcoind server to be running v22.0 and up");
    9         }
    
  32. laanwj commented at 8:18 pm on April 12, 2021: member

    I would be in favor of implementing this on the server so we get it right for all clients, GUI included.

    I’m not 100% convinced this is worth adding server side. Also not against it, but I don’t think we should add a lot of “would be nice” RPC commands. Yes, doing it client side is slower but how often are you going to call this ?

    As for the GUI I wonder if we can share some things between bitcoin-cli and GUI debug console.

  33. jonatack commented at 8:46 pm on April 12, 2021: member
    Right, and the fields returned here will probably change…more BIP155 networks may be on the way…and we may want to replace “torv2” and “torv3” with “onion” a year from now, IDK…adding fields is easier than removing or reorganizing them, but we’ll have more flexibility to do needed updates as a CLI.
  34. jarolrod commented at 8:49 pm on April 12, 2021: member

    tACK edf3167151f7a6d08cf733b4e230e2d745819ac8

    Server starting up

    0error code: -28
    1error message:
    2Loading block index...
    

    Server Running

    0{
    1  "addresses_known": {
    2    "ipv4": 15227,
    3    "ipv6": 3583,
    4    "torv2": 0,
    5    "torv3": 0,
    6    "i2p": 0,
    7    "total": 18810
    8  }
    9}
    

    Server shutting down, this message is consistent across other CLI commands

    0error: Could not connect to the server 127.0.0.1:18332
    1
    2Make sure the bitcoind server is running and that you are connecting to the correct RPC port.
    

    Tested network message with this patch

    0error: -addrinfo requires bitcoind server to be running v22.0 and up
    
  35. jonatack commented at 2:33 pm on April 13, 2021: member

    I’m at 2842 torv3 peers known now, up from ~1800 ten days ago and still finding more…good to see.

    0{
    1  "addresses_known": {
    2    "ipv4": 14398,
    3    "ipv6": 2509,
    4    "torv2": 5561,
    5    "torv3": 2842,
    6    "i2p": 8,
    7    "total": 25318
    8  }
    9}
    
  36. jonatack commented at 2:35 pm on April 13, 2021: member
    And one more i2p peer as well!
  37. in src/bitcoin-cli.cpp:238 in edf3167151 outdated
    229@@ -228,6 +230,61 @@ class BaseRequestHandler
    230     virtual UniValue ProcessReply(const UniValue &batch_in) = 0;
    231 };
    232 
    233+/** Process addrinfo requests */
    234+class AddrinfoRequestHandler : public BaseRequestHandler
    235+{
    236+private:
    237+    static constexpr uint8_t m_networks_size{5};
    238+    const std::array<std::string, m_networks_size> m_networks{{"ipv4", "ipv6", "torv2", "torv3", "i2p"}};
    


    promag commented at 11:57 pm on April 19, 2021:

    Does the following work?

    0static constexpr std::array m_networks{"ipv4", "ipv6", "torv2", "torv3", "i2p"};
    

    jonatack commented at 8:40 am on April 20, 2021:
    Thanks! I think I hadn’t dropped the type specifier, only the size arg. Took your suggestions and updated the file in 06c43201a714b0426cc68b2fd5c681e5df10af99.

    promag commented at 9:59 am on April 20, 2021:
    Cool 👍
  38. cli: use C++17 std::array class template argument deduction (CTAD)
    Credit to João Barbosa (promag) for the suggestions.
    
    Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com>
    06c43201a7
  39. laanwj commented at 11:51 am on April 20, 2021: member

    Tested ACK 06c43201a714b0426cc68b2fd5c681e5df10af99

     0% cli -addrinfo
     1{
     2  "addresses_known": {
     3    "ipv4": 52404,
     4    "ipv6": 10827,
     5    "torv2": 4987,
     6    "torv3": 94,
     7    "i2p": 5,
     8    "total": 68317
     9  }
    10}
    
  40. laanwj merged this on Apr 20, 2021
  41. laanwj closed this on Apr 20, 2021

  42. jonatack deleted the branch on Apr 20, 2021
  43. sidhujag referenced this in commit 16a85e1b23 on Apr 20, 2021
  44. laanwj referenced this in commit 23109cc548 on May 5, 2021
  45. sidhujag referenced this in commit 35f953ac31 on May 5, 2021
  46. luke-jr referenced this in commit 3429c9e7af on Jun 27, 2021
  47. luke-jr referenced this in commit ac5d632658 on Jun 27, 2021
  48. luke-jr referenced this in commit 346231afff on Jun 27, 2021
  49. PastaPastaPasta referenced this in commit bcd543c738 on Mar 5, 2022
  50. PastaPastaPasta referenced this in commit 62ff6a56af on Mar 5, 2022
  51. PastaPastaPasta referenced this in commit d3a8c28bac on Mar 5, 2022
  52. DrahtBot locked this on Aug 16, 2022

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-10-04 19:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me