Skip to content

Conversation

jonatack
Copy link
Member

@jonatack jonatack commented Oct 9, 2020

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:

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

@hebasto
Copy link
Member

hebasto commented Oct 9, 2020

Probably @ mentions should be removed from the OP.

@jonatack
Copy link
Member Author

jonatack commented Oct 9, 2020

Yes. Removed @ mentions.

@DrahtBot DrahtBot added the Docs label Oct 9, 2020
@0xB10C
Copy link
Contributor

0xB10C commented Oct 9, 2020

ACK f639a88

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

@practicalswift
Copy link
Contributor

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 10, 2020

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.

@Emzy
Copy link
Contributor

Emzy commented Oct 24, 2020

Tested ACK 43ee49f

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

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

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

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

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

@jonatack
Copy link
Member Author

jonatack commented Oct 25, 2020

Thanks for reviewing!

f3e30bf: 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.

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

Yes, it's already variable-width.

@hebasto
Copy link
Member

hebasto commented Oct 25, 2020

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

Yes, it's already variable-width.

Indeed :) Sorry for noise.

@jonatack
Copy link
Member Author

jonatack commented Oct 25, 2020

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

git diff 43ee49f 398045b

jon@purity:~/projects/bitcoin/bitcoin (netinfo-fixups)$ git diff 43ee49f bab058e
diff --git a/doc/release-notes.md b/doc/release-notes.md
index ccd66f9e85..65726f3d5d 100644
--- a/doc/release-notes.md
+++ b/doc/release-notes.md
@@ -207,11 +207,6 @@ Tools and Utilities
 
-- A new `bitcoin-cli -netinfo` command returns a network peer connections
-  dashboard that displays data from the `getpeerinfo` and `getnetworkinfo` RPCs
-  in a human-readable format. An optional integer argument from `0` to `4` may
-  be passed to see various levels of detail. (#19643)
-
index 14ac13632e..2054e991d5 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -14,11 +14,13 @@
 #include <rpc/request.h>
+#include <tinyformat.h>
 #include <util/strencodings.h>
 #include <util/system.h>
 #include <util/translation.h>
 #include <util/url.h>
 
+#include <algorithm>
 #include <functional>
 #include <memory>
 #include <stdio.h>
@@ -316,19 +317,19 @@ private:
     struct Peer {
-        int id;
-        int mapped_as;
-        int version;
+        std::string addr;
+        std::string sub_version;
+        std::string network;
+        std::string age;
+        double min_ping;
+        double ping;
         int64_t last_blck;
         int64_t last_recv;
         int64_t last_send;
         int64_t last_trxn;
-        double min_ping;
-        double ping;
-        std::string addr;
-        std::string age;
-        std::string network;
-        std::string sub_version;
+        int id;
+        int mapped_as;
+        int version;
         bool is_block_relay;
         bool is_outbound;
@@ -345,7 +346,7 @@ private:
         const double milliseconds{round(1000 * seconds)};
-        return milliseconds <= 999999 ? ToString(milliseconds) : "-";
+        return milliseconds > 999999 ? "-" : ToString(milliseconds);
     }
 
@@ -408,10 +409,10 @@ public:
                 const std::string sub_version{peer["subver"].get_str()};
-                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});
-                m_max_id_length = std::max(ToString(peer_id).length(), m_max_id_length);
+                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});
                 m_max_addr_length = std::max(addr.length() + 1, m_max_addr_length);
-                m_max_age_length = std::max(ToString(age).length(), m_max_age_length);
+                m_max_age_length = std::max(age.length(), m_max_age_length);
+                m_max_id_length = std::max(ToString(peer_id).length(), m_max_id_length);
                 m_is_asmap_on |= (mapped_as != 0);
@@ -464,11 +466,12 @@ public:
         if (local_addrs.empty()) {
             result += ": n/a\n";
         } else {
+            size_t max_addr_size{0};
             for (const UniValue& addr : local_addrs) {
-                m_max_addr_length = std::max(addr["address"].get_str().length() + 1, m_max_addr_length);
+                max_addr_size = std::max(addr["address"].get_str().length() + 1, max_addr_size);
             }
             for (const UniValue& addr : local_addrs) {
-                result += strprintf("\n%-*s    port %6i    score %6i", m_max_addr_length, addr["address"].get_str(), addr["port"].get_int(), addr["score"].get_int());
+                result += strprintf("\n%-*s    port %6i    score %6i", max_addr_size, addr["address"].get_str(), addr["port"].get_int(), addr["score"].get_int());
             }
         }

- 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
as it has a wide possible range and the new name ("age" instead of "uptime") is
much shorter.
@jonatack jonatack changed the title cli: -netinfo quick updates/fixups and release note cli: -netinfo quick updates/fixups for 0.21 Oct 28, 2020
@michaelfolkson
Copy link

michaelfolkson commented Oct 29, 2020

ACK 398045b

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

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

Potential improvement

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

@Emzy
Copy link
Contributor

Emzy commented Oct 29, 2020

Tested ACK 398045b

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

@jonatack
Copy link
Member Author

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 laanwj merged commit 2e24197 into bitcoin:master Oct 29, 2020
@jonatack jonatack deleted the netinfo-fixups branch October 29, 2020 11:24
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 29, 2020
398045b cli -netinfo: print oversized/extreme ping times as "-" (Jon Atack)
773f4c9 cli -netinfo: handle longer tor v3 local addresses (Jon Atack)
33e9874 cli -netinfo: make age column variable-width (Jon Atack)
f8a1c4d cli -netinfo: various quick updates and fixes (Jon Atack)

Pull request description:

  Quick fixups and updates for v0.21.0:

  - [x] handle larger BIP155 `addrv2` addresses
  - [x] add Signet chain
  - [x] add an additional space between the `net` and `mping` columns; add missing `tinyformat` and `algorithm` headers
  - [x] s/uptime/age/ per 0xB10C suggestion, and make the column auto-adjusting variable width
  - [x] 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:
  ```
  - A new `bitcoin-cli -netinfo` command returns a network peer connections
    dashboard that displays data from the `getpeerinfo` and `getnetworkinfo` RPCs
    in a human-readable format. An optional integer argument from `0` to `4` may
    be passed to see various levels of detail. (bitcoin#19643)
  ```

ACKs for top commit:
  michaelfolkson:
    ACK 398045b
  Emzy:
    Tested ACK 398045b

Tree-SHA512: 0625ee840141bafbfcaf8f1fce53f8f850ae91721b2bdad4279372da87c18a1fe3a214d90bfdbbabdf6da38d58290d7dd0f1109b4e2ca5d20cacf417d6ced0f9
@jonatack
Copy link
Member Author

Potential improvement

 -netinfo
       Get network peer connection information from the remote server. An
       optional integer argument from 0 to 4 can be passed **for a more
       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
Copy link
Member Author

jonatack commented Nov 2, 2020

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 pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Dec 21, 2021
Summary:
- 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

This is a backport of [[bitcoin/bitcoin#20115 | core#20115]] [1/4]
bitcoin/bitcoin@f8a1c4d

Test Plan: `ninja && src/bitcoind --daemon && sleep 5 && src/bitcoin-cli -netinfo 4 && src/bitcoin-cli stop`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D10707
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Dec 21, 2021
Summary:
It has a wide possible range and the new name ("age" instead of "uptime") is much shorter.

This is a backport of [[bitcoin/bitcoin#20115 | core#20115]] [2/4]
bitcoin/bitcoin@33e9874

Depends on D10707

Test Plan: `ninja && src/bitcoind --daemon && sleep 5 && src/bitcoin-cli -netinfo 4 && src/bitcoin-cli stop`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D10708
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Dec 21, 2021
Summary:
This is a backport of [[bitcoin/bitcoin#20115 | core#20115]] [3/4]
bitcoin/bitcoin@773f4c9

Depends on D10708

Test Plan: `ninja && src/bitcoind --daemon && sleep 5 && src/bitcoin-cli -netinfo 4 && src/bitcoin-cli stop`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D10709
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Dec 21, 2021
Summary:
This concludes backport of [[bitcoin/bitcoin#20115 | core#20115]] [4/4]
bitcoin/bitcoin@398045b

Depends on D10709

Test Plan: `ninja && src/bitcoind --daemon && sleep 5 && src/bitcoin-cli -netinfo 4 && src/bitcoin-cli stop`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D10710
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants