Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli: create -addrinfo #21595

Merged
merged 5 commits into from Apr 20, 2021
Merged

cli: create -addrinfo #21595

merged 5 commits into from Apr 20, 2021

Conversation

jonatack
Copy link
Contributor

@jonatack jonatack commented Apr 4, 2021

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
Copy link
Contributor

DrahtBot commented Apr 4, 2021

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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
Copy link
Contributor

Concept ACK: nice! :)

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

jonatack commented Apr 5, 2021

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
Copy link
Contributor Author

jonatack commented Apr 5, 2021

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
Copy link
Member

laanwj commented Apr 5, 2021

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
Copy link
Member

jarolrod commented Apr 5, 2021

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

Copy link
Member

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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
Copy link
Contributor Author

jonatack commented Apr 7, 2021

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
Copy link
Contributor Author

jonatack commented Apr 7, 2021

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
Copy link
Contributor Author

jonatack commented Apr 8, 2021

  • 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

@jonatack jonatack changed the title cli: create -addressinfo cli: create -addrinfo Apr 8, 2021
Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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

@jonatack
Copy link
Contributor Author

jonatack commented Apr 12, 2021

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
Copy link
Member

laanwj commented Apr 12, 2021

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
Copy link
Contributor Author

jonatack commented Apr 12, 2021

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.

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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
Copy link
Contributor Author

And one more i2p peer as well!

src/bitcoin-cli.cpp Outdated Show resolved Hide resolved
Credit to João Barbosa (promag) for the suggestions.

Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com>
@laanwj
Copy link
Member

laanwj commented Apr 20, 2021

Tested ACK 06c4320

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

@laanwj laanwj merged commit 0180453 into bitcoin:master Apr 20, 2021
@jonatack 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
laanwj added a commit that referenced this pull request May 5, 2021
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
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 5, 2021
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
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Mar 5, 2022
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Mar 5, 2022
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Mar 5, 2022
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants