cli: create -addrinfo #21595

jonatack · 2021-04-04T15:22:05Z

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.

$ bitcoin-cli --help | grep -A1 addrinfo
  -addrinfo
       Get the number of addresses known to the node, per network and total.

$ bitcoin-cli -addrinfo
{
  "addresses_known": {
    "ipv4": 14406,
    "ipv6": 2511,
    "torv2": 5563,
    "torv3": 2842,
    "i2p": 8,
    "total": 25330
  }
}

$ bitcoin-cli -addrinfo 1
error: -addrinfo takes no arguments

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

$ bitcoin-cli getnodeaddresses 0 | jq '.[] | (select(.address | contains(".onion")) | select(.address | length <= 22)) | .address' | wc -l
5563
$ bitcoin-cli getnodeaddresses 0 | jq '.[] | (select(.address | contains(".onion")) | select(.address | length > 22)) | .address' | wc -l
2842
$ bitcoin-cli getnodeaddresses 0 | jq '.[] | .address' | wc -l
25330

DrahtBot · 2021-04-04T17:05:09Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

lib: Add Taproot support to libconsensus #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.

practicalswift · 2021-04-04T21:29:58Z

Concept ACK: nice! :)

jarolrod

ACK 5f1ffbf

Tested on macOS 11.2.3

xyz@xyzs-MBP bitcoin % ./src/bitcoin-cli -testnet -addressinfo
{
  "addresses known": {
    "ipv4": 9774,
    "ipv6": 2211,
    "torv2": 0,
    "torv3": 0,
    "i2p": 0,
    "total": 11985
  }
}

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

jonatack · 2021-04-05T11:10:12Z

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.

jonatack · 2021-04-05T11:13:40Z

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.

laanwj · 2021-04-05T12:25:29Z

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.

jarolrod · 2021-04-05T17:37:14Z

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

promag

Code review ACK 5f1ffbf 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.

src/bitcoin-cli.cpp

jonatack · 2021-04-07T10:41:55Z

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 -netinfo...nodestats isn't bad though and is shorter. Hm...

jonatack · 2021-04-07T10:43:41Z

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.

jonatack · 2021-04-08T16:34:14Z

Rebased following the merge of rpc: add network field to getnodeaddresses #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

jarolrod

tACK c0986af

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:

error code: -28
error message:
Loading block index...

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

error: 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?

jonatack · 2021-04-12T19:47:31Z

Good find! I agree this isn't great, will look at fixing.

jonatack · 2021-04-12T20:17:59Z

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

@@ -256,6 +256,7 @@ public:
 
     UniValue ProcessReply(const UniValue& reply) override
     {
+        if (!reply["error"].isNull()) return reply;
         const std::vector<UniValue>& nodes{reply["result"].getValues()};

$ bitcoin-cli -addrinfo
error code: -28
error message:
Loading block index...

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

--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -264,7 +264,7 @@ public:
         // Count the number of peers we know by network, including torv2 versus torv3.
         std::array<uint64_t, m_networks_size> counts{{}};
         for (const UniValue& node : nodes) {
-        if (!nodes.empty() && nodes.at(0)["network"].isNull()) {
+        if (!nodes.empty() && nodes.at(0)["networ"].isNull()) {
             throw std::runtime_error("-addrinfo requires bitcoind server to be running v22.0 and up");
         }

laanwj · 2021-04-12T20:18:49Z

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.

jonatack · 2021-04-12T20:46:43Z

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.

jarolrod

tACK edf3167

Server starting up

error code: -28
error message:
Loading block index...

Server Running

{
  "addresses_known": {
    "ipv4": 15227,
    "ipv6": 3583,
    "torv2": 0,
    "torv3": 0,
    "i2p": 0,
    "total": 18810
  }
}

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

error: Could not connect to the server 127.0.0.1:18332

Make sure the bitcoind server is running and that you are connecting to the correct RPC port.

Tested network message with this patch

error: -addrinfo requires bitcoind server to be running v22.0 and up

jonatack · 2021-04-13T14:33:08Z

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

{
  "addresses_known": {
    "ipv4": 14398,
    "ipv6": 2509,
    "torv2": 5561,
    "torv3": 2842,
    "i2p": 8,
    "total": 25318
  }
}

jonatack · 2021-04-13T14:35:31Z

And one more i2p peer as well!

src/bitcoin-cli.cpp

Credit to João Barbosa (promag) for the suggestions. Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com>

laanwj · 2021-04-20T11:51:10Z

Tested ACK 06c4320

% cli -addrinfo
{
  "addresses_known": {
    "ipv4": 52404,
    "ipv6": 10827,
    "torv2": 4987,
    "torv3": 94,
    "i2p": 5,
    "total": 68317
  }
}

65f30e4 doc: add -addrinfo troubleshooting section to tor.md (Jon Atack) Pull request description: Follow-up to #21595. ACKs for top commit: jarolrod: ACK 65f30e4 practicalswift: ACK 65f30e4 0xB10C: ACK 65f30e4 theStack: ACK 65f30e4 Tree-SHA512: d17fa007106b8f877d2632c99273c663a24f025febe52faec9b197c561df808fd6a92bb27992ccbf5c3cc0d82058a8c4b82a2f1b99325f0ddfdac5ef703ac7d7

65f30e4 doc: add -addrinfo troubleshooting section to tor.md (Jon Atack) Pull request description: Follow-up to bitcoin#21595. ACKs for top commit: jarolrod: ACK 65f30e4 practicalswift: ACK 65f30e4 0xB10C: ACK 65f30e4 theStack: ACK 65f30e4 Tree-SHA512: d17fa007106b8f877d2632c99273c663a24f025febe52faec9b197c561df808fd6a92bb27992ccbf5c3cc0d82058a8c4b82a2f1b99325f0ddfdac5ef703ac7d7

Github-Pull: bitcoin#21595 Rebased-From: db4d2c2

Github-Pull: bitcoin#21595 Rebased-From: 5056a37

Github-Pull: bitcoin#21595 Rebased-From: edf3167

65f30e4 doc: add -addrinfo troubleshooting section to tor.md (Jon Atack) Pull request description: Follow-up to bitcoin#21595. ACKs for top commit: jarolrod: ACK 65f30e4 practicalswift: ACK 65f30e4 0xB10C: ACK 65f30e4 theStack: ACK 65f30e4 Tree-SHA512: d17fa007106b8f877d2632c99273c663a24f025febe52faec9b197c561df808fd6a92bb27992ccbf5c3cc0d82058a8c4b82a2f1b99325f0ddfdac5ef703ac7d7

DrahtBot added the RPC/REST/ZMQ label Apr 4, 2021

78051301012 approved these changes Apr 4, 2021

View reviewed changes

jonatack force-pushed the addressinfo branch from 5d2325d to 5f1ffbf Compare April 4, 2021 16:41

DrahtBot mentioned this pull request Apr 4, 2021

lib: Add Taproot support to libconsensus #21158

Closed

jarolrod reviewed Apr 5, 2021

View reviewed changes

promag suggested changes Apr 7, 2021

View reviewed changes

src/bitcoin-cli.cpp Outdated Show resolved Hide resolved

src/bitcoin-cli.cpp Outdated Show resolved Hide resolved

src/bitcoin-cli.cpp Outdated Show resolved Hide resolved

src/bitcoin-cli.cpp Outdated Show resolved Hide resolved

jonatack force-pushed the addressinfo branch from 5f1ffbf to d1dfa69 Compare April 8, 2021 16:34

jonatack changed the title ~~cli: create -addressinfo~~ cli: create -addrinfo Apr 8, 2021

jonatack added 3 commits April 9, 2021 09:02

cli: create AddrinfoRequestHandler class

db4d2c2

cli: add -addrinfo command

5056a37

doc: add cli -addrinfo release note

bb85cbc

jonatack force-pushed the addressinfo branch from d1dfa69 to c0986af Compare April 9, 2021 07:44

jarolrod reviewed Apr 12, 2021

View reviewed changes

addrinfo: raise helpfully on server error or incompatible server version

edf3167

jonatack force-pushed the addressinfo branch from c0986af to edf3167 Compare April 12, 2021 20:15

jarolrod reviewed Apr 12, 2021

View reviewed changes

promag reviewed Apr 19, 2021

View reviewed changes

src/bitcoin-cli.cpp Outdated Show resolved Hide resolved

cli: use C++17 std::array class template argument deduction (CTAD)

06c4320

Credit to João Barbosa (promag) for the suggestions. Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com>

laanwj merged commit 0180453 into bitcoin:master Apr 20, 2021

jonatack deleted the addressinfo branch April 20, 2021 12:38

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 20, 2021

Merge bitcoin#21595: cli: create -addrinfo

16a85e1

jonatack mentioned this pull request Apr 22, 2021

doc: add -addrinfo to tor docs #21753

Merged

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021

cli: create AddrinfoRequestHandler class

3429c9e

Github-Pull: bitcoin#21595 Rebased-From: db4d2c2

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021

cli: add -addrinfo command

ac5d632

Github-Pull: bitcoin#21595 Rebased-From: 5056a37

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021

addrinfo: raise helpfully on server error or incompatible server version

346231a

Github-Pull: bitcoin#21595 Rebased-From: edf3167

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

cli: create -addrinfo #21595

cli: create -addrinfo #21595

Uh oh!

Conversation

jonatack commented Apr 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Apr 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

practicalswift commented Apr 4, 2021

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Apr 5, 2021

Uh oh!

jonatack commented Apr 5, 2021

Uh oh!

laanwj commented Apr 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jarolrod commented Apr 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack commented Apr 7, 2021

Uh oh!

jonatack commented Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Apr 12, 2021

Uh oh!

jonatack commented Apr 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Apr 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Apr 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Apr 13, 2021

Uh oh!

jonatack commented Apr 13, 2021

Uh oh!

Uh oh!

laanwj commented Apr 20, 2021

Uh oh!

Uh oh!

jonatack commented Apr 4, 2021 •

edited

Loading

DrahtBot commented Apr 4, 2021 •

edited

Loading

laanwj commented Apr 5, 2021 •

edited

Loading

jarolrod commented Apr 5, 2021 •

edited

Loading

jonatack commented Apr 7, 2021 •

edited

Loading

jonatack commented Apr 8, 2021 •

edited

Loading

jonatack commented Apr 12, 2021 •

edited

Loading

laanwj commented Apr 12, 2021 •

edited

Loading

jonatack commented Apr 12, 2021 •

edited

Loading