netinfo: add peer services column and outbound-only option

jonatack commented at 4:33 pm on September 19, 2024: member

Been using this since May 2023.

add a peer services column (considered displaying the p2p_v2 flag as “p” or “2”; proposing “2” here for continuity with the “v” column, but “p” is fine for me as well)
clarify in the help that “relaytxes” and “addr_relay_enabled” are from getpeerinfo
hoist (and rename) the max level constant to use in top-level help, to avoid overlooking to update the top-level help if the value of the constant changes (as caught by Larry Ruane in review below)
add an optional “outonly” (or “o”) argument for an outbound-only peer list, as suggested by Vasil Dimov in his review below. Several people have requested this, to keep the output within screen limits when running netinfo as a live dashboard (i.e. with watch) on a node with many peers. While doing this, also permit passing “h” for the help in addition to “help”.

DrahtBot commented at 4:33 pm on September 19, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30930.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux, tdb3, brunoerg, achow101
Stale ACK	1440000bytes, vasild

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

sipa commented at 4:36 pm on September 19, 2024: member

It’s possible to be connected to a peer that supports the v2 service using the v1 protocol.

jonatack commented at 4:48 pm on September 19, 2024: member

It’s possible to be connected to a peer that supports the v2 service using the v1 protocol.

Thanks! (I must be missing something.)

sipa commented at 4:52 pm on September 19, 2024: member

I meant this as a comment on:

remove the “v” transport protocol column, as that info is provided in the new peer services column

They represent different information. The “v” transport protocol column currently conveys whether or not you are right now speaking to that peer through the v1 or v2 protocol.

Peer services are about what a peer supports, not what protocol you’re speaking with it.

If you run with -v2transport=0, the transport protocol will be v1 with all peers, includes ones that support v2. Similarly, If a peer recently upgraded to support v2, but you didn’t know about that when you connected to them, you may be using v1 with them.

jonatack force-pushed on Sep 19, 2024

jonatack commented at 5:04 pm on September 19, 2024: member

@sipa I guess it may make sense to only return the “v” column if the server node is -v2transport=0?

sipa commented at 5:06 pm on September 19, 2024: member

@jonatack I don’t think so. It’s possible that the service flag and transport type differ even with -v2transport=1.

jonatack commented at 5:09 pm on September 19, 2024: member

@sipa Thank you. Dropped the commit that removed the “v” column.

 0<->   type   net   serv  v  mping   ping send recv  txn  blk  hb addrp addrl  age  asmap  id 
 1 in        onion    nwl  1    360    360    1    7   19             38         19          4 
 2 in        onion    nwl  1    372    472    1    6                  19         17         45 
 3 in        onion    nwl  1    499    593   25   25                   2          4         89 
 4 in        onion   nbwl  1    512    608    1   34                   .         18         26 
 5 in        onion nbwcl2  2    523    610    1    0   18             37         19         11 
 6 in        onion         1    565    789   82   81    *              .         11         71 
 7 in        onion    nwl  1    579    579   18  906                             15         57 
 8 in          i2p   nwl2  1    585   1200    6    2    6             27         17         47 
 9 in        onion   nwl2  2    603    885    1    2                  29         17         38 
10 in        onion   nwl2  2    611   2714    1    2    4             39         18         20 
11 in        onion   nbwl  1    615    615    1   27                   .          4         87 
12 in        onion   nwl2  2    621    669    0    3   10             35         18         25 
13 in        onion    nwl  1    626   2463    1    6    5             39     1   19          7 
14 in        onion    nwl  1    647    651    1    4                  25         17         42 
15 in        onion   nwl2  2    659   1151    1    0                  37         17         48 
16 in          i2p   nwcl  1    867    867    1    0                              0        100 
17 in          i2p  nwcl2  2    936   1369    0    0                  16         15         56 
18 in          i2p   nwl2  1    972   1716    1    4                  30         17         39 
19 in        onion   nwl2  2   1020   2064    1    1    0             36         18         30 
20 in          i2p  nwcl2  2   1093   1176    1    4                  12          7         80 
21 in          i2p   nbwl  1   1094  14142    6    0   14             31         17         49 
22 in          i2p   nbwl  1   1245   1614    1    4                  16          6         82 
23 in          i2p  nbwl2  2   1425 340508   37  103                   2         11         69 
24 in          i2p    nwl  1   1546   1863    1    0                  24         18         21 
25 in          i2p   nbwl  1   2202   2202   74   68                              1         97 
26out  block  ipv4    nwl  1     91    100   88   87    *   10  .      .         19   7843   0 
27out   full  ipv4    wl2  2    102    108    2   13    1           1023         19  20115  10 
28out manual  ipv4    nwl  1    172    188    1    9    3           1017         19  47605   9 
29out manual cjdns   nwl2  2    183    183    1    4    0           1020         19          1 
30out   full  ipv4   nwl2  2    221    297    2    2    2           1016         18  35432  36 
31out  block onion    nwl  1    614    818   79   79    *              .         19          2 
32out   full onion   nwl2  2    645    772    0    0    0           1043         18         22 
33out   full onion    nwl  1    677   3582    0    0    0           1036         18         32 
34out   full   i2p  nbwcl  1    678   2217    2    0    3           1031         18         15 
35out manual onion    wl2  2    692    926    4    1    0           1010         19          3 
36out manual   i2p  nwcl2  2    748    910    4    0    0           1021         18         16 
37out manual onion  nwcl2  2    786    797    1    0    1           1022         17         40 
38out   full onion    nwl  1    795    916    0    0    0           1034         18         28 
39out  block onion    nwl  1    965    965   55   55    *              .          0         99 
40out   full   i2p    nwl  1   1042   1102    0    0    4           1019         11         70 
41out   full  ipv4    nwl  1  28462 527279    0   17                1001         18   1351  33 
42                               ms     ms  sec  sec  min  min                  min

tdb3 commented at 2:20 am on September 20, 2024: contributor

Approach ACK

Nice improvement. Performed a quick initial code review and exercised the changes interactively running on signet. I plan to circle back.

in src/bitcoin-cli.cpp:397 in 51206c693a outdated

395-    bool IsAddressSelected() const { return m_details_level == 2 || m_details_level == 4; }
396-    bool IsVersionSelected() const { return m_details_level == 3 || m_details_level == 4; }
397+    bool OutboundOnlySelected() const { return m_details_level == 5; }
398+    bool DetailsRequested() const { return m_details_level > 0 && m_details_level <= MAX_DETAIL_LEVEL; }
399+    bool IsAddressSelected() const { return m_details_level == 2 || m_details_level >= 4; }
400+    bool IsVersionSelected() const { return m_details_level == 3 || m_details_level >= 4; }

tdb3 commented at 2:38 pm on September 20, 2024:

nit: If touching the file again, looks like this could be simplified.

0- bool IsVersionSelected() const { return m_details_level == 3 || m_details_level >= 4; }
1+ bool IsVersionSelected() const { return m_details_level >= 3; }

jonatack commented at 7:43 pm on October 2, 2024:

done

in src/bitcoin-cli.cpp:465 in 8ca614d795 outdated

457@@ -456,6 +458,16 @@ class NetinfoRequestHandler : public BaseRequestHandler
458         if (conn_type == "addr-fetch") return "addr";
459         return "";
460     }
461+    std::string FormatServices(const UniValue& services)
462+    {
463+        std::string str;
464+        for (size_t i = 0; i < services.size(); ++i) {
465+            if (services[i].isNull()) continue;

tdb3 commented at 3:06 pm on September 20, 2024:

Do we expect to receive a null in servicesnames among other entries? If we don’t, maybe this should be something like assert(!services[i].isNull()); so the issue can be noticed?

jonatack commented at 9:34 pm on September 23, 2024:

Yes, I often see peers with no services, both new connections still setting up and longer-lasting ones, where getpeerinfo returns

0    "services": "0000000000000000",
1    "servicesnames": [
2    ],

jonatack commented at 9:43 pm on September 23, 2024:

Two examples right now. Longer-lasting ones…

0<->   type   net    serv  v  mping   ping send recv  txn  blk  hb addrp addrl  age  asmap    id address                                                             version
1 in        onion     nwl  1                 10   10                   .          0        16818 127.0.0.1:63651                                                     70016/Satoshi:26.1.0/
2 in        cjdns   nwcl2  2    282    295    5    3    4            377        246        15919 [fccb:248:11a6:1042:bca:1218:f7ce:7d3d]:40608                       70016/Satoshi:28.0.0/
3 in        onion          1    293    530   20   20    *              .        241        15949 127.0.0.1:64109                                                     70016
4 in        onion     nwl  1    316    349    5    1    0            414        237        15971 127.0.0.1:64399                                                     70016/Satoshi:26.0.0/
5 in        onion     nwl  1    323  89873    1   31    *              .        209        16068 127.0.0.1:49870                                                     70016/Satoshi:26.0.0/
6 in        onion    nwl2  2    326    634    5    1    5            428        237        15972 127.0.0.1:64405                                                     70016/Satoshi:27.1.0/
7 in        onion    nwl2  2    327    349    0    2    9            325        160        16232 127.0.0.1:54108                                                     70016/Satoshi:27.1.0/
8 in        onion          1    336    785    5   12                   .        237        15970 127.0.0.1:64391                                                     70016/Satoshi:0.16.3/

…and fresh connections

0<->   type   net    serv  v  mping   ping send recv  txn  blk  hb addrp addrl  age  asmap    id address                                                             version
1 in          i2p                                       *              .          0        16826 yn6fuglkgeib4runvfkdmsokxkcnaecvo3x6x3rpdm5eywsalbxa.b32.i2p:0
2 in        onion                                       *              .          0        16827 127.0.0.1:63746

tdb3 commented at 9:55 pm on September 23, 2024:

Sorry, I wasn’t as clear as I could have been. It seems possible to have no services (e.g. servicesnames has no elements, in which case the for loop wouldn’t be entered, right?). What I meant to ask is if we think we’ll see servicesnames with some non-zero number of elements, with at least one element satisfying the isNull() check? In other words, does the continue ever actually happen?

jonatack commented at 10:43 pm on September 23, 2024:

Good point, SGTM to use a CHECK_NONFATAL there in lieu of continue.

jonatack commented at 7:43 pm on October 2, 2024:

Looking at it more, I don’t think it’s needed and won’t be hit, so removed that line for now. (Thanks!)

tdb3 commented at 3:36 pm on September 20, 2024: contributor

Left a couple small comments. Tested that using 5 does include only outbound peers by connecting two nodes locally.

jonatack force-pushed on Oct 2, 2024

jonatack commented at 8:35 pm on October 2, 2024: member

Updated to take @tdb3 review feedback (thanks!) and make minor adjustments.

 0diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
 1index 3207a5d9c97..000f7695de6 100644
 2--- a/src/bitcoin-cli.cpp
 3+++ b/src/bitcoin-cli.cpp
 4@@ -391,10 +391,10 @@ private:
 5         return UNKNOWN_NETWORK;
 6     }
 7     uint8_t m_details_level{0}; //!< Optional user-supplied arg to set dashboard details level
 8-    bool OutboundOnlySelected() const { return m_details_level == 5; }
 9     bool DetailsRequested() const { return m_details_level > 0 && m_details_level <= MAX_DETAIL_LEVEL; }
10     bool IsAddressSelected() const { return m_details_level == 2 || m_details_level >= 4; }
11-    bool IsVersionSelected() const { return m_details_level == 3 || m_details_level >= 4; }
12+    bool IsVersionSelected() const { return m_details_level >= 3; }
13+    bool OutboundOnlySelected() const { return m_details_level == 5; }
14     bool m_is_asmap_on{false};
15     size_t m_max_addr_length{0};
16     size_t m_max_addr_processed_length{5};
17@@ -407,8 +407,8 @@ private:
18         std::string sub_version;
19         std::string conn_type;
20         std::string network;
21-        std::string services;
22         std::string age;
23+        std::string services;
24         std::string transport_protocol_type;
25         double min_ping;
26         double ping;
27@@ -463,7 +463,6 @@ private:
28     {
29         std::string str;
30         for (size_t i = 0; i < services.size(); ++i) {
31-            if (services[i].isNull()) continue;
32             const std::string s{services[i].get_str()};
33             str += s == "NETWORK_LIMITED" ? 'l' : s == "P2P_V2" ? '2' : ToLower(s[0]);
34         }
35@@ -539,7 +538,7 @@ public:
36                 const bool is_addr_relay_enabled{peer["addr_relay_enabled"].isNull() ? false : peer["addr_relay_enabled"].get_bool()};
37                 const bool is_bip152_hb_from{peer["bip152_hb_from"].get_bool()};
38                 const bool is_bip152_hb_to{peer["bip152_hb_to"].get_bool()};
39-                m_peers.push_back({addr, sub_version, conn_type, NETWORK_SHORT_NAMES[network_id], services, age, transport, min_ping, ping, addr_processed, addr_rate_limited, last_blck, last_recv, last_send, last_trxn, peer_id, mapped_as, version, is_addr_relay_enabled, is_bip152_hb_from, is_bip152_hb_to, is_outbound, is_tx_relay});
40+                m_peers.push_back({addr, sub_version, conn_type, NETWORK_SHORT_NAMES[network_id], age, services, transport, min_ping, ping, addr_processed, addr_rate_limited, last_blck, last_recv, last_send, last_trxn, peer_id, mapped_as, version, is_addr_relay_enabled, is_bip152_hb_from, is_bip152_hb_to, is_outbound, is_tx_relay});
41                 m_max_addr_length = std::max(addr.length() + 1, m_max_addr_length);
42                 m_max_addr_processed_length = std::max(ToString(addr_processed).length(), m_max_addr_processed_length);
43                 m_max_addr_rate_limited_length = std::max(ToString(addr_rate_limited).length(), m_max_addr_rate_limited_length);
44@@ -684,13 +683,13 @@ public:
45         "           \"addr\"   - address fetch; short-lived connection for requesting addresses\n"
46         "  net      Network the peer connected through (\"ipv4\", \"ipv6\", \"onion\", \"i2p\", \"cjdns\", or \"npr\" (not publicly routable))\n"
47         "  serv     Services offered by the peer\n"
48-        "           \"n\" - NETWORK, can serve the full block chain\n"
49-        "           \"b\" - BLOOM, can handle bloom-filtered connections (see BIP 111)\n"
50-        "           \"w\" - WITNESS, can be asked for blocks and transactions with witness data (SegWit)\n"
51-        "           \"c\" - COMPACT_FILTERS, can handle basic block filter requests (see BIPs 157 and 158)\n"
52-        "           \"l\" - NETWORK_LIMITED, limited to serving only the last 288 blocks (~2 days)\n"
53-        "           \"2\" - P2P_V2, supports version 2 P2P transport protocol, as defined in BIP324\n"
54-        "           \"u\" - UNKNOWN, unrecognized bit flag\n"
55+        "           \"n\" - NETWORK: peer can serve the full block chain\n"
56+        "           \"b\" - BLOOM: peer can handle bloom-filtered connections (see BIP 111)\n"
57+        "           \"w\" - WITNESS: peer can be asked for blocks and transactions with witness data (SegWit)\n"
58+        "           \"c\" - COMPACT_FILTERS: peer can handle basic block filter requests (see BIPs 157 and 158)\n"
59+        "           \"l\" - NETWORK_LIMITED: peer limited to serving only the last 288 blocks (~2 days)\n"
60+        "           \"2\" - P2P_V2: peer supports version 2 P2P transport protocol, as defined in BIP 324\n"
61+        "           \"u\" - UNKNOWN: unrecognized bit flag\n"
62         "  v        Version of transport protocol used for the connection\n"
63         "  mping    Minimum observed ping time, in milliseconds (ms)\n"
64         "  ping     Last observed ping time, in milliseconds (ms)\n"

tdb3 approved

tdb3 commented at 11:06 pm on October 2, 2024: contributor

code review and light test ACK 683b558a020f1632b0a3cbdaa165adbd1423281c

Good catch with age/services

1440000bytes approved

1440000bytes commented at 9:40 pm on October 3, 2024: none

ACK https://github.com/bitcoin/bitcoin/pull/30930/commits/683b558a020f1632b0a3cbdaa165adbd1423281c

in src/bitcoin-cli.cpp:669 in 683b558a02 outdated

665@@ -648,6 +666,7 @@ class NetinfoRequestHandler : public BaseRequestHandler
666         "                                  2 - Like 1 but with an address column\n"
667         "                                  3 - Like 1 but with a version column\n"
668         "                                  4 - Like 1 but with both address and version columns\n"
669+        "                                  5 - Like 4 but limited to outbound peers only, to save screen space\n"

vasild commented at 4:16 pm on October 10, 2024:

Before these numbers somehow designated increasing verbosity and more columns. Whereas now the newly added “5” does not add more columns, but removes rows compared to “4”.

I am not sure how to improve that. It is a different dimension, maybe use a different option “outonly”. Then one could have either one of the 0-4 modes in “outonly” mode or in full mode (in + out).

(non-blocker, just something to consider)

jonatack commented at 4:52 pm on October 10, 2024:

I see. I think it’s simpler and more convenient for the user (i.e. me, so this could just be habit) to type 5 rather than 4, or vice versa.

I did maintain the “high details levels = maximum” behavior preferred by @laanwj in #21192 for values above 5.

jonatack commented at 4:55 pm on October 10, 2024:

Also, I find myself only using level 4 or 5. Perhaps other reviewers would prefer the option to apply, as you suggest, to all the other levels with an o / outonly argument. Happy to do that.

LarryRuane commented at 8:09 pm on October 11, 2024:

I like @vasild’s observation that having a separate option (instead of using level 5) allows 0-4 to be used in both modes – that’s very flexible.

However, I suggest that the new option be netinfo-outonly (instead of outonly) and it would imply netinfo (netinfo wouldn’t need to be specified) because it would be weird to have an option (outonly) that does nothing without netinfo. It would also be less convenient to have to specify both. This new netinfo-outonly option would take a level argument (0-4).

If both netinfo and netinfo-outonly are specified, the result should probably be outonly, since that was explicitly requested (but I don’t think it matters much either way).

jonatack commented at 8:54 pm on October 17, 2024:

Updated to take @vasild’s proposal. I believe it’s the best path of the 3 options discussed above.

laanwj commented at 8:06 am on October 20, 2024:

I did maintain the “high details levels = maximum” behavior preferred by laanwj in #21192 for values above 5.

Thank you 😄

vasild approved

vasild commented at 4:18 pm on October 10, 2024: contributor

ACK 683b558a020f1632b0a3cbdaa165adbd1423281c

in src/bitcoin-cli.cpp:467 in 683b558a02 outdated

458@@ -456,6 +459,15 @@ class NetinfoRequestHandler : public BaseRequestHandler
459         if (conn_type == "addr-fetch") return "addr";
460         return "";
461     }
462+    std::string FormatServices(const UniValue& services)
463+    {
464+        std::string str;
465+        for (size_t i = 0; i < services.size(); ++i) {
466+            const std::string s{services[i].get_str()};
467+            str += s == "NETWORK_LIMITED" ? 'l' : s == "P2P_V2" ? '2' : ToLower(s[0]);

LarryRuane commented at 5:35 pm on October 11, 2024:

683b558a020f1632b0a3cbdaa165adbd1423281c

0            str += (s == "NETWORK_LIMITED") ? 'l' : ((s == "P2P_V2") ? '2' : ToLower(s[0]));

(readability, nonblocking nit, if you happen to retouch)

jonatack commented at 8:56 pm on October 17, 2024:

Didn’t take, as those braces are unneeded due to the low precedence of the ternary operator, clang-format doesn’t prefer them, and they add clutter. Will add if people insist.

LarryRuane changes_requested

LarryRuane commented at 7:25 pm on October 11, 2024: contributor

I think you also should update line 93 (change “4” to “5”)

0argsman.AddArg("-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). Pass \"help\" for detailed help documentation.", ArgsManager::ALLOW_ANY, OptionsCategory::CLI_COMMANDS);

jonatack force-pushed on Oct 17, 2024

jonatack commented at 8:58 pm on October 17, 2024: member

I think you also should update line 93 (change “4” to “5”)

0argsman.AddArg("-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). Pass \"help\" for detailed help documentation.", ArgsManager::ALLOW_ANY, OptionsCategory::CLI_COMMANDS);

Good catch @LarryRuane – added 153b898a77f4c892731c220ce85d29c1cf880355 to ensure we don’t overlook updating this.

netinfo: add peer services column eef2a9d406

netinfo: clarify relaytxes and addr_relay_enabled help docs e7d307ce8c

netinfo: rename and hoist max level constant to use in top-level help

to avoid overlooking to update the top-level help if the value of the constant changes.

681ebcceca

jonatack force-pushed on Oct 17, 2024

jonatack commented at 9:35 pm on October 17, 2024: member

Updated to take @vasild’s suggestion and a fix for @LarryRuane’s catch (thanks!) and doc updates I overlooked.

  0diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
  1index 000f7695de6..6bda4cbe0c5 100644
  2--- a/src/bitcoin-cli.cpp
  3+++ b/src/bitcoin-cli.cpp
  4@@ -57,6 +57,7 @@ static const int DEFAULT_HTTP_CLIENT_TIMEOUT=900;
  5 static constexpr int DEFAULT_WAIT_CLIENT_TIMEOUT = 0;
  6 static const bool DEFAULT_NAMED=false;
  7 static const int CONTINUE_EXECUTION=-1;
  8+static constexpr uint8_t NETINFO_MAX_LEVEL{4};
  9 static constexpr int8_t UNKNOWN_NETWORK{-1};
 10 // See GetNetworkName() in netbase.cpp
 11 static constexpr std::array NETWORKS{"not_publicly_routable", "ipv4", "ipv6", "onion", "i2p", "cjdns", "internal"};
 12@@ -90,7 +91,7 @@ static void SetupCliArgs(ArgsManager& argsman)
 13                    ArgsManager::ALLOW_ANY, OptionsCategory::CLI_COMMANDS);
 14     argsman.AddArg("-addrinfo", "Get the number of addresses known to the node, per network and total, after filtering for quality and recency. The total number of addresses known to the node may be higher.", ArgsManager::ALLOW_ANY, OptionsCategory::CLI_COMMANDS);
 15     argsman.AddArg("-getinfo", "Get general information from the remote server. Note that unlike server-side RPC calls, the output of -getinfo is the result of multiple non-atomic requests. Some entries in the output may represent results from different states (e.g. wallet balance may be as of a different block from the chain state reported)", ArgsManager::ALLOW_ANY, OptionsCategory::CLI_COMMANDS);
 16-    argsman.AddArg("-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). Pass \"help\" for detailed help documentation.", ArgsManager::ALLOW_ANY, OptionsCategory::CLI_COMMANDS);
 17+    argsman.AddArg("-netinfo", strprintf("Get network peer connection information from the remote server. An optional argument from 0 to %d can be passed for different peers listings (default: 0). If a non-zero value is passed, an additional \"outonly\" (or \"o\") argument can be passed to see outbound peers only. Pass \"help\" (or \"h\") for detailed help documentation.", NETINFO_MAX_LEVEL), ArgsManager::ALLOW_ANY, OptionsCategory::CLI_COMMANDS);
 18 
 19     SetupChainParamsBaseOptions(argsman);
 20     argsman.AddArg("-color=<when>", strprintf("Color setting for CLI output (default: %s). Valid values: always, auto (add color codes when standard output is connected to a terminal and OS is not WIN32), never.", DEFAULT_COLOR_SETTING), ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS);
 21@@ -379,7 +380,6 @@ public:
 22 class NetinfoRequestHandler : public BaseRequestHandler
 23 {
 24 private:
 25-    static constexpr uint8_t MAX_DETAIL_LEVEL{5};
 26     std::array<std::array<uint16_t, NETWORKS.size() + 1>, 3> m_counts{{{}}}; //!< Peer counts by (in/out/total, networks/total)
 27     uint8_t m_block_relay_peers_count{0};
 28     uint8_t m_manual_peers_count{0};
 29@@ -391,10 +391,10 @@ private:
 30         return UNKNOWN_NETWORK;
 31     }
 32     uint8_t m_details_level{0}; //!< Optional user-supplied arg to set dashboard details level
 33-    bool DetailsRequested() const { return m_details_level > 0 && m_details_level <= MAX_DETAIL_LEVEL; }
 34-    bool IsAddressSelected() const { return m_details_level == 2 || m_details_level >= 4; }
 35-    bool IsVersionSelected() const { return m_details_level >= 3; }
 36-    bool OutboundOnlySelected() const { return m_details_level == 5; }
 37+    bool DetailsRequested() const { return m_details_level > 0 && m_details_level < 5; }
 38+    bool IsAddressSelected() const { return m_details_level == 2 || m_details_level == 4; }
 39+    bool IsVersionSelected() const { return m_details_level == 3 || m_details_level == 4; }
 40+    bool m_outbound_only_selected{false};
 41     bool m_is_asmap_on{false};
 42     size_t m_max_addr_length{0};
 43     size_t m_max_addr_processed_length{5};
 44@@ -478,9 +478,18 @@ public:
 45         if (!args.empty()) {
 46             uint8_t n{0};
 47             if (ParseUInt8(args.at(0), &n)) {
 48-                m_details_level = (n <= MAX_DETAIL_LEVEL) ? n : 4;
 49+                m_details_level = std::min(n, NETINFO_MAX_LEVEL);
 50             } else {
 51-                throw std::runtime_error(strprintf("invalid -netinfo argument: %s\nFor more information, run: bitcoin-cli -netinfo help", args.at(0)));
 52+                throw std::runtime_error(strprintf("invalid -netinfo level argument: %s\nFor more information, run: bitcoin-cli -netinfo help", args.at(0)));
 53+            }
 54+            if (args.size() > 1) {
 55+                if (std::string_view s{args.at(1)}; n && (s == "o" || s == "outonly")) {
 56+                    m_outbound_only_selected = true;
 57+                } else if (n) {
 58+                    throw std::runtime_error(strprintf("invalid -netinfo outonly argument: %s\nFor more information, run: bitcoin-cli -netinfo help", args.at(1)));
 59+                } else {
 60+                    throw std::runtime_error(strprintf("invalid -netinfo outonly argument: %s\nThe outonly argument is only valid for details levels higher than 0. For more information, run: bitcoin-cli -netinfo help", args.at(1)));
 61+                }
 62             }
 63         }
 64         UniValue result(UniValue::VARR);
 65@@ -515,7 +524,7 @@ public:
 66             ++m_counts.at(2).at(NETWORKS.size());           // total overall
 67             if (conn_type == "block-relay-only") ++m_block_relay_peers_count;
 68             if (conn_type == "manual") ++m_manual_peers_count;
 69-            if (!is_outbound && OutboundOnlySelected()) continue;
 70+            if (m_outbound_only_selected && !is_outbound) continue;
 71             if (DetailsRequested()) {
 72                 // Push data for this peer to the peers vector.
 73                 const int peer_id{peer["id"].getInt<int>()};
 74@@ -650,26 +659,28 @@ public:
 75     }
 76 
 77     const std::string m_help_doc{
 78-        "-netinfo level|\"help\" \n\n"
 79+        "-netinfo level|outonly|help\n\n"
 80         "Returns a network peer connections dashboard with information from the remote server.\n"
 81         "This human-readable interface will change regularly and is not intended to be a stable API.\n"
 82         "Under the hood, -netinfo fetches the data by calling getpeerinfo and getnetworkinfo.\n"
 83-        + strprintf("An optional integer argument from 0 to %d can be passed for different peers listings; %d to 255 are parsed as %d.\n", MAX_DETAIL_LEVEL, MAX_DETAIL_LEVEL, MAX_DETAIL_LEVEL) +
 84-        "Pass \"help\" to see this detailed help documentation.\n"
 85-        "If more than one argument is passed, only the first one is read and parsed.\n"
 86-        "Suggestion: use with the Linux watch(1) command for a live dashboard; see example below.\n\n"
 87+        + strprintf("An optional argument from 0 to %d can be passed for different peers listings; values above %d up to 255 are parsed as %d.\n", NETINFO_MAX_LEVEL, NETINFO_MAX_LEVEL, NETINFO_MAX_LEVEL) +
 88+        "If that argument is passed, an optional additional \"outonly\" argument may be passed to obtain the listing with outbound peers only.\n"
 89+        "Pass \"help\" or \"h\" to see this detailed help documentation.\n"
 90+        "If more than two arguments are passed, only the first two are read and parsed.\n"
 91+        "Suggestion: use -netinfo with the Linux watch(1) command for a live dashboard; see example below.\n\n"
 92         "Arguments:\n"
 93-        + strprintf("1. level (integer 0-%d, optional)  Specify the info level of the peers dashboard (default 0):\n", MAX_DETAIL_LEVEL) +
 94+        + strprintf("1. level (integer 0-%d, optional)  Specify the info level of the peers dashboard (default 0):\n", NETINFO_MAX_LEVEL) +
 95         "                                  0 - Peer counts for each reachable network as well as for block relay peers\n"
 96         "                                      and manual peers, and the list of local addresses and ports\n"
 97         "                                  1 - Like 0 but preceded by a peers listing (without address and version columns)\n"
 98         "                                  2 - Like 1 but with an address column\n"
 99         "                                  3 - Like 1 but with a version column\n"
100         "                                  4 - Like 1 but with both address and version columns\n"
101-        "                                  5 - Like 4 but limited to outbound peers only, to save screen space\n"
102-        "2. help (string \"help\", optional) Print this help documentation instead of the dashboard.\n\n"
103+        "2. outonly (\"outonly\" or \"o\", optional) Return the peers listing with outbound peers only, i.e. to save screen space\n"
104+        "                                        when a node has many inbound peers. Only valid if a level is passed.\n\n"
105+        "help (\"help\" or \"h\", optional) Print this help documentation instead of the dashboard.\n\n"
106         "Result:\n\n"
107-        + strprintf("* The peers listing in levels 1-%d displays all of the peers sorted by direction and minimum ping time:\n\n", MAX_DETAIL_LEVEL) +
108+        + strprintf("* The peers listing in levels 1-%d displays all of the peers sorted by direction and minimum ping time:\n\n", NETINFO_MAX_LEVEL) +
109         "  Column   Description\n"
110         "  ------   -----------\n"
111         "  <->      Direction\n"
112@@ -719,9 +730,11 @@ public:
113         "The same, preceded by a peers listing without address and version columns\n"
114         "> bitcoin-cli -netinfo 1\n\n"
115         "Full dashboard\n"
116-        + strprintf("> bitcoin-cli -netinfo %d\n\n", MAX_DETAIL_LEVEL) +
117+        + strprintf("> bitcoin-cli -netinfo %d\n\n", NETINFO_MAX_LEVEL) +
118+        "Full dashboard, but with outbound peers only\n"
119+        + strprintf("> bitcoin-cli -netinfo %d outonly\n\n", NETINFO_MAX_LEVEL) +
120         "Full live dashboard, adjust --interval or --no-title as needed (Linux)\n"
121-        + strprintf("> watch --interval 1 --no-title bitcoin-cli -netinfo %d\n\n", MAX_DETAIL_LEVEL) +
122+        + strprintf("> watch --interval 1 --no-title bitcoin-cli -netinfo %d\n\n", NETINFO_MAX_LEVEL) +
123         "See this help\n"
124         "> bitcoin-cli -netinfo help\n"};
125 };
126@@ -1245,7 +1258,7 @@ static int CommandLineRPC(int argc, char *argv[])
127         if (gArgs.IsArgSet("-getinfo")) {
128             rh.reset(new GetinfoRequestHandler());
129         } else if (gArgs.GetBoolArg("-netinfo", false)) {
130-            if (!args.empty() && args.at(0) == "help") {
131+            if (!args.empty() && (args.at(0) == "h" || args.at(0) == "help")) {
132                 tfm::format(std::cout, "%s\n", NetinfoRequestHandler().m_help_doc);
133                 return 0;
134             }

1440000bytes approved

1440000bytes commented at 0:13 am on October 18, 2024: none

ACK https://github.com/bitcoin/bitcoin/pull/30930/commits/17f8f03ec69b6dd3d61137a39b8f88201ec500dc

Changes since last review: #30930 (comment)

DrahtBot requested review from vasild on Oct 18, 2024

DrahtBot requested review from tdb3 on Oct 18, 2024

tdb3 approved

tdb3 commented at 1:36 pm on October 18, 2024: contributor

re ACK 17f8f03ec69b6dd3d61137a39b8f88201ec500dc

Code reviewed diff.

Ran sanity check using the new outonly arg:

 0$ build/src/bitcoin-cli -signet -netinfo 1
 1Bitcoin Core client v28.99.0-17f8f03ec69b signet - server 70016/Satoshi:28.99.0/
 2
 3<->   type   net   serv  v  mping   ping send recv  txn  blk  hb addrp addrl  age id 
 4 in          npr   nwl2  2      1      2   38   24    1              2          6 13 
 5out   full  ipv6    nwl  1     82     91   24   24         3  .   1001         25  6 
 6...
 7
 8$ build/src/bitcoin-cli -signet -netinfo 1 outonly
 9Bitcoin Core client v28.99.0-17f8f03ec69b signet - server 70016/Satoshi:28.99.0/
10
11<->   type   net   serv  v  mping   ping send recv  txn  blk  hb addrp addrl  age id 
12out   full  ipv6    nwl  1     82     91   26   26         3  .   1001         25  6 
13...

Compared the output of 17f8f03ec69b6dd3d61137a39b8f88201ec500dc to master (e8f72aefd20049eac81b150e7f0d33709acd18ed) for 1 through 4. Saw serv column and no unexpected differences.

Sanity checked that >4 acts as 4.

jonatack renamed this:
~~netinfo: add peer services column and outbound-only peers list~~
netinfo: add peer services column and outbound-only option
on Oct 18, 2024

in src/bitcoin-cli.cpp:491 in 17f8f03ec6 outdated

488+                if (std::string_view s{args.at(1)}; n && (s == "o" || s == "outonly")) {
489+                    m_outbound_only_selected = true;
490+                } else if (n) {
491+                    throw std::runtime_error(strprintf("invalid -netinfo outonly argument: %s\nFor more information, run: bitcoin-cli -netinfo help", args.at(1)));
492+                } else {
493+                    throw std::runtime_error(strprintf("invalid -netinfo outonly argument: %s\nThe outonly argument is only valid for details levels higher than 0. For more information, run: bitcoin-cli -netinfo help", args.at(1)));

vasild commented at 12:46 pm on October 25, 2024:

nit: since args.at(1) is repeated also in the throw statements, maybe:

0std::string_view arg1{args.at(1)};
1if (n && (arg1 == "o" || arg1 == "outonly")) {
2} else {
3    throw ... arg1 ...

jonatack commented at 1:10 pm on October 25, 2024:

done – good eye

in src/bitcoin-cli.cpp:662 in 17f8f03ec6 outdated

658@@ -632,25 +659,28 @@ class NetinfoRequestHandler : public BaseRequestHandler
659     }
660 
661     const std::string m_help_doc{
662-        "-netinfo level|\"help\" \n\n"
663+        "-netinfo level|outonly|help\n\n"

vasild commented at 12:53 pm on October 25, 2024:

nit: level|outonly|help reads as “exactly one of level, outonly or help”. I guess it should be

0-netinfo (level [outonly]) | help

or two lines:

0        "-netinfo \"help\"\n"
1        "-netinfo level [outonly]\n\n"

jonatack commented at 1:10 pm on October 25, 2024:

Thanks! Done.

vasild approved

vasild commented at 12:54 pm on October 25, 2024: contributor

ACK 17f8f03ec69b6dd3d61137a39b8f88201ec500dc

netinfo: allow setting an outbound-only peer list

by passing an additional argument of "outonly" or "o".

This has been requested in order to keep the output within screen limits when running -netinfo
as a live dashboard, i.e. with `watch`.

Also allow passing "h" in addition to "help" to see the help documentation.

87532fe558

jonatack force-pushed on Oct 25, 2024

jonatack commented at 1:27 pm on October 25, 2024: member

Updated the last commit to take @vasild’s feedback (thanks!) and slightly improve an error message on the same line, per the following 3-line diff.

 0diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
 1index 6bda4cbe0c5..f63dd22e621 100644
 2--- a/src/bitcoin-cli.cpp
 3+++ b/src/bitcoin-cli.cpp
 4@@ -486,9 +486,9 @@ public:
 5                 if (std::string_view s{args.at(1)}; n && (s == "o" || s == "outonly")) {
 6                     m_outbound_only_selected = true;
 7                 } else if (n) {
 8-                    throw std::runtime_error(strprintf("invalid -netinfo outonly argument: %s\nFor more information, run: bitcoin-cli -netinfo help", args.at(1)));
 9+                    throw std::runtime_error(strprintf("invalid -netinfo outonly argument: %s\nFor more information, run: bitcoin-cli -netinfo help", s));
10                 } else {
11-                    throw std::runtime_error(strprintf("invalid -netinfo outonly argument: %s\nThe outonly argument is only valid for details levels higher than 0. For more information, run: bitcoin-cli -netinfo help", args.at(1)));
12+                    throw std::runtime_error(strprintf("invalid -netinfo outonly argument: %s\nThe outonly argument is only valid for a level greater than 0 (the first argument). For more information, run: bitcoin-cli -netinfo help", s));
13                 }
14             }
15         }
16@@ -659,7 +659,7 @@ public:
17     }
18 
19     const std::string m_help_doc{
20-        "-netinfo level|outonly|help\n\n"
21+        "-netinfo (level [outonly]) | help\n\n"
22         "Returns a network peer connections dashboard with information from the remote server.\n"

in src/bitcoin-cli.cpp:465 in eef2a9d406 outdated

457@@ -456,6 +458,15 @@ class NetinfoRequestHandler : public BaseRequestHandler
458         if (conn_type == "addr-fetch") return "addr";
459         return "";
460     }
461+    std::string FormatServices(const UniValue& services)
462+    {
463+        std::string str;
464+        for (size_t i = 0; i < services.size(); ++i) {
465+            const std::string s{services[i].get_str()};

rkrux commented at 12:21 pm on October 28, 2024:

Can NODE_NONE ever be a value here?

https://github.com/bitcoin/bitcoin/blob/master/src/protocol.h#L312

jonatack commented at 3:16 pm on October 28, 2024:

I think so (but as an empty string, so no). Using this, I see empty service names while a peer connection is setting up, and often from one or two (usually inbound tor) peers. It looks like NODE_NONE would be returned to RPC getpeerinfo as an empty string by serviceFlagsToStr(), and the value from getpeerinfo is the input to this function here.

rkrux commented at 5:05 pm on October 28, 2024:

Thanks for clarifying, I was wondering whether the below line would cause NODE_NONE to show up as n but that should not be the case.

rkrux approved

rkrux commented at 12:23 pm on October 28, 2024: none

tACK 87532fe55856efc063cf81244800da37a015ba75

Successful make and functional tests. I used the -netinfo command with various verbosities and the outonly option. Also checked for verbosities greater than 4.

Asked a question for clarity.

DrahtBot requested review from tdb3 on Oct 28, 2024

DrahtBot requested review from vasild on Oct 28, 2024

DrahtBot requested review from 1440000bytes on Oct 28, 2024

tdb3 approved

tdb3 commented at 4:05 pm on October 28, 2024: contributor

cr re ACK 87532fe55856efc063cf81244800da37a015ba75

brunoerg commented at 9:31 pm on November 1, 2024: contributor

crACK 87532fe55856efc063cf81244800da37a015ba75

code looks great, but I didn’t test in practice yet.

achow101 commented at 8:43 pm on November 4, 2024: member

ACK 87532fe55856efc063cf81244800da37a015ba75

achow101 merged this on Nov 4, 2024

achow101 closed this on Nov 4, 2024

jonatack deleted the branch on Nov 4, 2024

netinfo: add peer services column and outbound-only option #30930

Code Coverage & Benchmarks

Reviews

Conflicts