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

pull jonatack wants to merge 4 commits into bitcoin:master from jonatack:netinfo-fixups changing 1 files +40 −23
  1. 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)
  2. hebasto commented at 6:59 pm on October 9, 2020: member
    Probably @ mentions should be removed from the OP.
  3. jonatack commented at 6:59 pm on October 9, 2020: member
    Yes. Removed @ mentions.
  4. DrahtBot added the label Docs on Oct 9, 2020
  5. 0xB10C commented at 7:49 pm on October 9, 2020: member

    ACK f639a88e720eb30c170e84710341b9c64ccf4ce4

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

  6. practicalswift commented at 9:39 pm on October 9, 2020: contributor
    Concept ACK: I’m a big fan of -netinfo - thanks for improving!
  7. 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.

  8. 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!
  9. jonatack force-pushed on Oct 13, 2020
  10. DrahtBot added the label Needs rebase on Oct 15, 2020
  11. laanwj added this to the milestone 0.21.0 on Oct 15, 2020
  12. jonatack force-pushed on Oct 15, 2020
  13. DrahtBot removed the label Needs rebase on Oct 15, 2020
  14. Emzy commented at 1:27 pm on October 24, 2020: contributor

    Tested ACK 43ee49f2c42f078763be55d666161507a2d7707b

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

  15. 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!
  16. hebasto approved
  17. 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?~

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

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

  20. 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
  21. jonatack force-pushed on Oct 25, 2020
  22. 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         }
  23. 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
  24. 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
  25. jonatack force-pushed on Oct 25, 2020
  26. cli -netinfo: handle longer tor v3 local addresses 773f4c99c0
  27. cli -netinfo: print oversized/extreme ping times as "-" 398045ba8b
  28. jonatack force-pushed on Oct 25, 2020
  29. jonatack renamed this:
    cli: -netinfo quick updates/fixups and release note
    cli: -netinfo quick updates/fixups for 0.21
    on Oct 28, 2020
  30. 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).
  31. 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.

  32. 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 ;)

  33. laanwj merged this on Oct 29, 2020
  34. laanwj closed this on Oct 29, 2020
  35. jonatack deleted the branch on Oct 29, 2020
  36. sidhujag referenced this in commit 82dc2b27ac on Oct 29, 2020
  37. 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.

  38. 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.
  39. Fabcien referenced this in commit 8572692879 on Dec 21, 2021
  40. Fabcien referenced this in commit 185ed7f57b on Dec 21, 2021
  41. Fabcien referenced this in commit 01c1ed9974 on Dec 21, 2021
  42. Fabcien referenced this in commit 89fbef396c on Dec 21, 2021
  43. DrahtBot locked this on Feb 15, 2022


jonatack hebasto 0xB10C practicalswift DrahtBot Emzy michaelfolkson

Labels
Docs

Milestone
0.21.0

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-11-17 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me