cli: -netinfo quick updates/fixups for 0.21

jonatack commented at 6:56 pm on October 9, 2020: member

Quick fixups and updates for v0.21.0:

handle larger BIP155 addrv2 addresses
add Signet chain
add an additional space between the net and mping columns; add missing tinyformat and algorithm headers
s/uptime/age/ per 0xB10C suggestion, and make the column auto-adjusting variable width
display - for oversized mping/ping times like 1.17348e+06, as reported by practicalswift

Edit: removed the release note commit, as this PR was not merged before the notes were moved to the wiki. It’s here:

0- A new `bitcoin-cli -netinfo` command returns a network peer connections
1  dashboard that displays data from the `getpeerinfo` and `getnetworkinfo` RPCs
2  in a human-readable format. An optional integer argument from `0` to `4` may
3  be passed to see various levels of detail. (#19643)

hebasto commented at 6:59 pm on October 9, 2020: member

Probably @ mentions should be removed from the OP.

jonatack commented at 6:59 pm on October 9, 2020: member

Yes. Removed @ mentions.

DrahtBot added the label Docs on Oct 9, 2020

0xB10C commented at 7:49 pm on October 9, 2020: member

ACK f639a88e720eb30c170e84710341b9c64ccf4ce4

Build, ran functional & unit tests, used -netinfo on testnet and signet.

practicalswift commented at 9:39 pm on October 9, 2020: contributor

Concept ACK: I’m a big fan of -netinfo - thanks for improving!

DrahtBot commented at 2:37 am on October 10, 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:

#19064 (refactor: Cleanup thread ctor calls by hebasto)
#16439 (cli/gui: support “@height” in place of blockhash for getblock on client side by ajtowns)

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.

in src/bitcoin-cli.cpp:461 in 8b0aa43e29 outdated

496+        // Report reachable networks and local addresses info.
497+        result += "\nnetworks:";
498+        for (const UniValue& network : networkinfo["networks"].getValues()) {
499+            const std::string name{network["name"].get_str()};
500+            if (!name.empty() && network["reachable"].get_bool()) result += " " + name;
501+        }

0xB10C commented at 7:27 am on October 13, 2020:

This produces the following output with bitcoin-cli -testnet -netinfo for me

 0Bitcoin Core v0.20.99.0-8b0aa43e2 testnet - 70016/Satoshi:0.20.99/
 1
 2        ipv4    ipv6   onion   total  block-relay
 3in         0       0       0       0       0
 4out        9       0       1      10       2
 5total      9       0       1      10       2
 6
 7networks: ipv4 ipv6 onion - local addresses:
 82002:b10c:b10c:0:1234::5057     port  18333    score      1
 92002:b10c:b10c:0:1234::33ed     port  18333    score      1
10b10cabcdefaqqqhf.onion          port  18333    score     13

I’m not sure if this adds that much in its current form. Someone, who didn’t look at the code, wouldn’t know that ipv4 ipv6 onion is dynamically constructed from networkinfo and not just a constant. Additionally, it’s for 1) quite hidden and 2) not self-explanatory that these are the reachable networks.

Something like the following might be better:

0reachable networks: ipv4 ipv6 onion
1local addresses:
22002:b10c:b10c:0:1234::5057     port  18333    score      1
32002:b10c:b10c:0:1234::33ed     port  18333    score      1
4b10cabcdefaqqqhf.onion          port  18333    score     13

jonatack commented at 8:53 am on October 13, 2020:

Thanks for the testing and feedback, @0xB10C. I’m not sure it would add much either. Dropped the commit to not invalidate your previous ACK. Thanks!

jonatack force-pushed on Oct 13, 2020

DrahtBot added the label Needs rebase on Oct 15, 2020

laanwj added this to the milestone 0.21.0 on Oct 15, 2020

jonatack force-pushed on Oct 15, 2020

DrahtBot removed the label Needs rebase on Oct 15, 2020

Emzy commented at 1:27 pm on October 24, 2020: contributor

Tested ACK 43ee49f2c42f078763be55d666161507a2d7707b

Build and used -netinfo on mainnet, testnet and signet.

in src/bitcoin-cli.cpp:427 in 43ee49f2c4 outdated

421@@ -419,13 +422,13 @@ class NetinfoRequestHandler : public BaseRequestHandler
422         // Report detailed peer connections list sorted by direction and minimum ping time.
423         if (DetailsRequested() && !m_peers.empty()) {
424             std::sort(m_peers.begin(), m_peers.end());
425-            result += "Peer connections sorted by direction and min ping\n<-> relay   net  mping   ping send recv  txn  blk    age ";
426+            result += strprintf("Peer connections sorted by direction and min ping\n<-> relay   net  mping   ping send recv  txn  blk %*s ", m_max_age_length, "age");

hebasto commented at 12:26 pm on October 25, 2020:

43ee49f2c42f078763be55d666161507a2d7707b

#include <tinyformat.h> ?

jonatack commented at 4:03 pm on October 25, 2020:

Added the tinyformat header, and also algorithm, which was missing too. Thanks!

hebasto approved

hebasto commented at 12:34 pm on October 25, 2020: member

ACK 43ee49f2c42f078763be55d666161507a2d7707b, tested on Linux Mint 20 (x86_64).

f3e30bf1b2231ba3abb65f59be1c5d923ca3dc91: isn’t the https://github.com/bitcoin-core/bitcoin-devwiki/wiki/0.21.0-Release-Notes-Draft designed for this purpose?

~~43ee49f2c42f078763be55d666161507a2d7707b: does id column require variable-width too?~~

jonatack commented at 12:49 pm on October 25, 2020: member

Thanks for reviewing!

f3e30bf1b2231ba3abb65f59be1c5d923ca3dc91: isn’t the https://github.com/bitcoin-core/bitcoin-devwiki/wiki/0.21.0-Release-Notes-Draft designed for this purpose?

Yes, that wiki was not yet created when the PR was opened. I’m not sure what the process is in this case. Edit: dropped the release note.

43ee49f2c42f078763be55d666161507a2d7707b: does id column require variable-width too?

Yes, it’s already variable-width.

hebasto commented at 12:51 pm on October 25, 2020: member

43ee49f: does id column require variable-width too?

Yes, it’s already variable-width.

Indeed :) Sorry for noise.

in src/bitcoin-cli.cpp:414 in 43ee49f2c4 outdated

410                 const std::string sub_version{peer["subver"].get_str()};
411-                m_peers.push_back({peer_id, mapped_as, version, conn_time, last_blck, last_recv, last_send, last_trxn, min_ping, ping, addr, network, sub_version, is_block_relay, is_outbound});
412+                m_peers.push_back({peer_id, mapped_as, version, last_blck, last_recv, last_send, last_trxn, min_ping, ping, addr, age, network, sub_version, is_block_relay, is_outbound});
413                 m_max_id_length = std::max(ToString(peer_id).length(), m_max_id_length);
414                 m_max_addr_length = std::max(addr.length() + 1, m_max_addr_length);
415+                m_max_age_length = std::max(ToString(age).length(), m_max_age_length);

jonatack commented at 1:33 pm on October 25, 2020:

Hm, age is already a string.

0                m_max_age_length = std::max(age.length(), m_max_age_length);

jonatack commented at 2:41 pm on October 25, 2020:

fixed

jonatack force-pushed on Oct 25, 2020

jonatack commented at 2:45 pm on October 25, 2020: member

Thank you @Emzy, @hebasto, @0xB10C and @practicalswift for reviewing and testing. Updated per git diff 43ee49f 398045b to add algorithm and tinyformat headers, remove an unneeded ToString type conversion, order the Peer struct members by decreasing memory size, fix a bug I found in the local address size handling by adding a local max_addr_size variable, and drop the release note (copied it to the PR description).

 0jon@purity:~/projects/bitcoin/bitcoin (netinfo-fixups)$ git diff 43ee49f bab058e
 1diff --git a/doc/release-notes.md b/doc/release-notes.md
 2index ccd66f9e85..65726f3d5d 100644
 3--- a/doc/release-notes.md
 4+++ b/doc/release-notes.md
 5@@ -207,11 +207,6 @@ Tools and Utilities
 6 
 7-- A new `bitcoin-cli -netinfo` command returns a network peer connections
 8-  dashboard that displays data from the `getpeerinfo` and `getnetworkinfo` RPCs
 9-  in a human-readable format. An optional integer argument from `0` to `4` may
10-  be passed to see various levels of detail. (#19643)
11-
12index 14ac13632e..2054e991d5 100644
13--- a/src/bitcoin-cli.cpp
14+++ b/src/bitcoin-cli.cpp
15@@ -14,11 +14,13 @@
16 #include <rpc/request.h>
17+#include <tinyformat.h>
18 #include <util/strencodings.h>
19 #include <util/system.h>
20 #include <util/translation.h>
21 #include <util/url.h>
22 
23+#include <algorithm>
24 #include <functional>
25 #include <memory>
26 #include <stdio.h>
27@@ -316,19 +317,19 @@ private:
28     struct Peer {
29-        int id;
30-        int mapped_as;
31-        int version;
32+        std::string addr;
33+        std::string sub_version;
34+        std::string network;
35+        std::string age;
36+        double min_ping;
37+        double ping;
38         int64_t last_blck;
39         int64_t last_recv;
40         int64_t last_send;
41         int64_t last_trxn;
42-        double min_ping;
43-        double ping;
44-        std::string addr;
45-        std::string age;
46-        std::string network;
47-        std::string sub_version;
48+        int id;
49+        int mapped_as;
50+        int version;
51         bool is_block_relay;
52         bool is_outbound;
53@@ -345,7 +346,7 @@ private:
54         const double milliseconds{round(1000 * seconds)};
55-        return milliseconds <= 999999 ? ToString(milliseconds) : "-";
56+        return milliseconds > 999999 ? "-" : ToString(milliseconds);
57     }
58 
59@@ -408,10 +409,10 @@ public:
60                 const std::string sub_version{peer["subver"].get_str()};
61-                m_peers.push_back({peer_id, mapped_as, version, last_blck, last_recv, last_send, last_trxn, min_ping, ping, addr, age, network, sub_version, is_block_relay, is_outbound});
62-                m_max_id_length = std::max(ToString(peer_id).length(), m_max_id_length);
63+                m_peers.push_back({addr, sub_version, network, age, min_ping, ping, last_blck, last_recv, last_send, last_trxn, peer_id, mapped_as, version, is_block_relay, is_outbound});
64                 m_max_addr_length = std::max(addr.length() + 1, m_max_addr_length);
65-                m_max_age_length = std::max(ToString(age).length(), m_max_age_length);
66+                m_max_age_length = std::max(age.length(), m_max_age_length);
67+                m_max_id_length = std::max(ToString(peer_id).length(), m_max_id_length);
68                 m_is_asmap_on |= (mapped_as != 0);
69@@ -464,11 +466,12 @@ public:
70         if (local_addrs.empty()) {
71             result += ": n/a\n";
72         } else {
73+            size_t max_addr_size{0};
74             for (const UniValue& addr : local_addrs) {
75-                m_max_addr_length = std::max(addr["address"].get_str().length() + 1, m_max_addr_length);
76+                max_addr_size = std::max(addr["address"].get_str().length() + 1, max_addr_size);
77             }
78             for (const UniValue& addr : local_addrs) {
79-                result += strprintf("\n%-*s    port %6i    score %6i", m_max_addr_length, addr["address"].get_str(), addr["port"].get_int(), addr["score"].get_int());
80+                result += strprintf("\n%-*s    port %6i    score %6i", max_addr_size, addr["address"].get_str(), addr["port"].get_int(), addr["score"].get_int());
81             }
82         }

cli -netinfo: various quick updates and fixes

- add new signet chain

- update change "uptime" column name to "age" per suggestion by 0xB10C (Timo)

- add an additional digit to mping field width

- change m_networks_size from size_t to uint8_t, as size_t was a holdover
  from m_networks_size being defined as size_t m_networks.size() in a draft

- order Peer struct members by decreasing memory size

f8a1c4d946

cli -netinfo: make age column variable-width

as it has a wide possible range and the new name ("age" instead of "uptime") is
much shorter.

33e987452f

jonatack force-pushed on Oct 25, 2020

cli -netinfo: handle longer tor v3 local addresses 773f4c99c0

cli -netinfo: print oversized/extreme ping times as "-" 398045ba8b

jonatack force-pushed on Oct 25, 2020

jonatack renamed this:
~~cli: -netinfo quick updates/fixups and release note~~
cli: -netinfo quick updates/fixups for 0.21
on Oct 28, 2020

michaelfolkson commented at 0:21 am on October 29, 2020: contributor

ACK 398045ba8b3694931069f88ec95553b3207dd1a6

Light code review, tested on MacOS Catalina with Signet and all 0-4 arguments.

A couple of (very) minor non-urgent observations not relevant for 0.21 (but maybe for a future follow up PR)

I expected that as the arguments increased this would strictly increase the amount of the data displayed. Between 2 and 3 it replaces “address” with “version” only to display both “address” and “version” for 4. Was there particular thinking around this ie some users would want only one of these and not both?
I suspect the help message for this tool could be sharpened up a touch.

Currently

0 -netinfo
1       Get network peer connection information from the remote server. An
2       optional integer argument from 0 to 4 can be passed for **different
3       peers listings** (default: 0).

Potential improvement

0 -netinfo
1       Get network peer connection information from the remote server. An
2       optional integer argument from 0 to 4 can be passed **for a more
3       detailed dashboard** (default: 0).

Emzy commented at 0:30 am on October 29, 2020: contributor

Tested ACK 398045ba8b3694931069f88ec95553b3207dd1a6

Build and used -netinfo on mainnet, testnet and signet on Ubuntu 20.04.1 LTS.

jonatack commented at 0:42 am on October 29, 2020: member

Thanks for testing and the feedback!

I expected that as the arguments increased this would strictly increase the amount of the data displayed. Between 2 and 3 it replaces “address” with “version” only to display both “address” and “version” for 4. Was there particular thinking around this ie some users would want only one of these and not both?

Yes, flexibility for different buffer/window widths. Especially with the new Tor v3 addresses, which are much longer.

The “version” column actually combines two getpeerinfo fields into one: version and sub-version. So in a way, it’s more info ;)

laanwj merged this on Oct 29, 2020

laanwj closed this on Oct 29, 2020

jonatack deleted the branch on Oct 29, 2020

sidhujag referenced this in commit 82dc2b27ac on Oct 29, 2020

jonatack commented at 8:53 pm on October 29, 2020: member

Potential improvement

0 -netinfo
1       Get network peer connection information from the remote server. An
2       optional integer argument from 0 to 4 can be passed **for a more
3       detailed dashboard** (default: 0).

Thanks @michaelfolkson, I’ll do this in the next -netinfo update to use the connection_type field in the place of the first two columns.

jonatack commented at 4:46 pm on November 2, 2020: member

Added the release note in the PR description to the wiki at https://github.com/bitcoin-core/bitcoin-devwiki/wiki/0.21.0-Release-Notes-Draft.

Fabcien referenced this in commit 8572692879 on Dec 21, 2021

Fabcien referenced this in commit 185ed7f57b on Dec 21, 2021

Fabcien referenced this in commit 01c1ed9974 on Dec 21, 2021

Fabcien referenced this in commit 89fbef396c on Dec 21, 2021

DrahtBot locked this on Feb 15, 2022

cli: -netinfo quick updates/fixups for 0.21 #20115

Conflicts