Add -netinfo peer connections dashboard

jonatack commented at 4:38 pm on August 2, 2020: member

This PR is inspired by laanwj’s python script mentioned in #19405, which it turns out I ended up using every day and extending because I got hooked on using it to monitor Bitcoin peer connections.

For the full experience, run ./src/bitcoin-cli -netinfo 4

On Linux, try it with watch watch ./src/bitcoin-cli -netinfo 4

Help doc

0$ ./src/bitcoin-cli -help | grep -A3 netinfo
1  -netinfo
2       Get network peer connection information from the remote server. An
3       optional integer argument from 0 to 4 can be passed for different
4       peers listings (default: 0).

DrahtBot added the label P2P on Aug 2, 2020

DrahtBot added the label RPC/REST/ZMQ on Aug 2, 2020

jonatack commented at 5:51 pm on August 2, 2020: member

@sumBTC you might find this useful–I’ve been using it to observe your issue #19500.

ghost commented at 6:21 pm on August 2, 2020: none

@jonatack Ah was wondering what you were using. Nice to know this kind of information will soon be part of bitcoin-cli.

jonatack force-pushed on Aug 2, 2020

DrahtBot commented at 8:13 pm on August 2, 2020: member

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

Conflicts

No conflicts as of last run.

jonatack force-pushed on Aug 2, 2020

jonatack force-pushed on Aug 3, 2020

0xB10C commented at 10:00 am on August 3, 2020: member

Concept ACK. Especially on the detailed peers listing as getpeerinfo formatted for humans.

practicalswift commented at 3:51 pm on August 3, 2020: contributor

Wow, this is really neat!

As a pure terminal user I love the bitcoin-cli -netinfo t output – that is terminal usability at its finest!

Concept ACK

in src/bitcoin-cli.cpp:329 in 7e0727b738 outdated

320+    /** Whether a peer is an IPv6 connection.
321+     * @returns true if addr starts with '['. */
322+    bool IsAddrIPv6(const std::string& addr) const { return addr.front() == '['; }
323+
324+    /** Whether a peer is an outbound onion connection.
325+     * @returns true if addr contains ".onion". */

0xB10C commented at 9:36 pm on August 4, 2020:

just for me to understand: Why do you check if the address contains “.onion” and not ends with it (as it’s done in src/netaddress.cpp)?

jonatack commented at 3:03 pm on August 9, 2020:

AFAICT the outbound onion addrs returned by getpeerinfo don’t end with .onion but with .onion:<port id>

in src/netaddress.h:20 in 7e0727b738 outdated

15@@ -16,6 +16,9 @@
16 #include <string>
17 #include <vector>
18 
19+static const std::string ONION{".onion"};
20+static const std::string LOCALHOST{"127.0.0.1"};

0xB10C commented at 10:36 pm on August 4, 2020:

nit: This is only the IPv4 localhost. Maybe:

0static const std::string LOCALHOST_IPV4{"127.0.0.1"};

jonatack commented at 3:03 pm on August 9, 2020:

done

in src/httpserver.cpp:171 in 7e0727b738 outdated

167@@ -167,7 +168,7 @@ static bool InitHTTPAllowList()
168     rpc_allow_subnets.clear();
169     CNetAddr localv4;
170     CNetAddr localv6;
171-    LookupHost("127.0.0.1", localv4, false);
172+    LookupHost(LOCALHOST, localv4, false);
173     LookupHost("::1", localv6, false);

0xB10C commented at 10:39 pm on August 4, 2020:

Does it make sense to use a LOCALHOST_IPV6 constant here as well? (even if you would otherwise not touch it)

jonatack commented at 3:03 pm on August 9, 2020:

done

in src/bitcoin-cli.cpp:415 in 7e0727b738 outdated

410+        // Report 1: peer connections sorted by direction and minimum ping time
411+        if (m_verbose) {
412+            result += "Peer connections sorted by direction and min ping\n  id  <->  relay   conn  min ping    ping     asmap  address\n";
413+            std::sort(peers.begin(), peers.end());
414+            for (const m_peer& peer : peers) {
415+                result += strprintf("%4i  %3s  %5s  %5s  %-8d  %-8d  %6s  %s\n", peer.id, peer.is_outbound ? "out" : "in", peer.is_block_relay ? "block" : "full", ConnTypeEnumToString(peer.conn_type), peer.min_ping, peer.ping, peer.mapped_as == 0 ? "" : ToString(peer.mapped_as), peer.addr);

0xB10C commented at 11:05 pm on August 4, 2020:

Probably needs a unit behind the min ping and ping values.

0xB10C commented at 11:57 pm on August 4, 2020:

Longer peer ids as found on long running nodes cause the table columns to become misaligned.

0Peer connections sorted by direction and min ping
1  id  <->  relay   conn  min ping    ping     asmap  address
213417   in   full   ipv4  0.005165  0.005495          XXX.XXX.XX.XX:XXXXX
313715  out  block   ipv4  0.011632  0.013025          XX.XXX.XX.XXX:XXXX
4   5  out   full   ipv4  0.014787  0.016915          XXX.XX.XX.XX:XXXX

jonatack commented at 3:04 pm on August 9, 2020:

done by adding units in a line at the bottom to save on horizontal space

jonatack commented at 3:04 pm on August 9, 2020:

done; the spacing is now dynamically based on the size of the largest peer id

jonatack commented at 3:05 pm on August 9, 2020:

also moved the id column next to the version and address ones

in src/bitcoin-cli.cpp:429 in 7e0727b738 outdated

399+                if (is_block_relay) block_relay_o += 1;
400+            }
401+            if (m_verbose) {
402+                const int peer_id{peer["id"].get_int()};
403+                const double min_ping{peer["minping"].isNull() ? 0 : peer["minping"].get_real()};
404+                const double ping{peer["pingtime"].isNull() ? 0 : peer["pingtime"].get_real()};

0xB10C commented at 11:08 pm on August 4, 2020:

I found ping times displayed in seconds to be somewhat uninutive. I’d personally prefer milliseconds.

(Had pings under one second to most of my peers. This might be different for other users.)

jonatack commented at 3:04 pm on August 9, 2020:

done for min ping and ping

0xB10C commented at 11:51 pm on August 4, 2020: member

Ran unit tests and the extend functional tests.

Mainly played around with the verbose mode on testnet without inbound peers. Works great with watch!

Did not see anything in the asmap column. Guess that’s due to me not using the asmap feature.

Tested the bitcoin-cli build from this PR with -netinfo 1 on a v0.20.0 Bitcoin Core node and they seem to be compatible. That’s super awesome!

in src/httpserver.cpp:9 in 7e0727b738 outdated

5@@ -6,6 +6,7 @@
6 
7 #include <chainparamsbase.h>
8 #include <compat.h>
9+#include <netaddress.h> // For explicitness; already included via netbase.h

jnewbery commented at 10:24 am on August 6, 2020:

no need for this inline comment

hebasto commented at 10:33 am on August 6, 2020:

https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#source-code-organization

jonatack commented at 10:38 am on August 6, 2020:

@jnewbery agreed, I added it for reviewers so they wouldn’t wonder why this was added ;) will remove @hebasto if you are referring to Every .cpp and .h file should #include every header file it directly uses classes, functions or other definitions from, even if those headers are already included indirectly through other headers.… yes, this is why I added the #include. Are you posting the link for a different reason?

jnewbery commented at 10:42 am on August 6, 2020:

I added it for reviewers so they wouldn’t wonder why this was added

You can leave github comments on your own PR for that. If we added a code comment “// already included via …. " to all includes that were already included indirectly, then most of the includes would have that comment.

jonatack commented at 10:46 am on August 6, 2020:

Agreed

hebasto commented at 10:46 am on August 6, 2020:

Are you posting the link for a different reason?

No, you point the reason exactly :)

jonatack commented at 3:05 pm on August 9, 2020:

done

in src/torcontrol.cpp:9 in 7e0727b738 outdated

5@@ -6,6 +6,7 @@
6 #include <chainparams.h>
7 #include <torcontrol.h>
8 #include <util/strencodings.h>
9+#include <netaddress.h> // For explicitness; already included via netbase.h

jnewbery commented at 10:27 am on August 6, 2020:

remove inline comment

hebasto commented at 10:33 am on August 6, 2020:

https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#source-code-organization

jonatack commented at 3:05 pm on August 9, 2020:

done

jnewbery commented at 10:31 am on August 6, 2020: member

Concept ACK. This is very cool. I’d normally be against adding this kind of functionality to bitcoin-cli for reasons of scope creep and bloat, but this seems really useful, and it’s very neatly encapsulated in the NetinfoRequestHandler class. I’m looking forward to playing around with it.

in src/bitcoin-cli.cpp:61 in 7e0727b738 outdated

58@@ -58,6 +59,7 @@ static void SetupCliArgs(ArgsManager& argsman)
59     argsman.AddArg("-getinfo", "Get general information from the remote server. Note that unlike server-side RPC calls, the results of -getinfo is the result of multiple non-atomic requests. Some entries in the result 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::OPTIONS);
60     SetupChainParamsBaseOptions(argsman);
61     argsman.AddArg("-named", strprintf("Pass named instead of positional arguments (default: %s)", DEFAULT_NAMED), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
62+    argsman.AddArg("-netinfo", "Get network peer connection information from the remote server. An optional boolean argument can be passed for a detailed peers listing (default: false).", ArgsManager::ALLOW_BOOL, OptionsCategory::OPTIONS);

hebasto commented at 10:38 am on August 6, 2020:

7e0727b73841d5f2abc96cd2fac50de74546a281

For Bitcoin Core it seems conventional to use -netinfo=<...>, no?

jonatack commented at 3:06 pm on August 9, 2020:

Unless I’m confused, I think -option=<...> would be for config args, not CLI ones. See -generate.

in src/bitcoin-cli.cpp:342 in 7e0727b738 outdated

337+        switch (t) {
338+        case m_conn_type::ipv4: return "ipv4";
339+        case m_conn_type::ipv6: return "ipv6";
340+        case m_conn_type::onion: return "onion";
341+        default: return "invalid";
342+        };

hebasto commented at 10:40 am on August 6, 2020:

7e0727b73841d5f2abc96cd2fac50de74546a281

Mind following convention about switch statement on an enumeration?

jonatack commented at 3:06 pm on August 9, 2020:

Thanks! – done

in src/netaddress.h:19 in 56b156dc16 outdated

15@@ -16,6 +16,9 @@
16 #include <string>
17 #include <vector>
18 
19+static const std::string ONION{".onion"};

hebasto commented at 10:43 am on August 6, 2020:

56b156dc161b766e4aaae1ca07c719a3c599c4cb

0static const std::string ONION_DOMAIN{".onion"};

jonatack commented at 3:06 pm on August 9, 2020:

done

hebasto changes_requested

hebasto commented at 10:44 am on August 6, 2020: member

Concept ACK, but not sure about min ping data usefulness though.

jonatack commented at 10:54 am on August 6, 2020: member

Concept ACK, but not sure about min ping data usefulness though.

Thanks for having a look. min ping is an inbound eviction criterium and I look at it more than ping; also mulling adding a human-readable conntime column and maybe a couple others (last send/recv, addnode).

hebasto commented at 10:55 am on August 6, 2020: member

Concept ACK, but not sure about min ping data usefulness though.

Thanks for having a look. min ping is an inbound eviction criterium and I look at it more than ping; also mulling adding a human-readable conntime column and maybe a couple others (last send/recv, addnode).

Great!

in src/bitcoin-cli.cpp:12 in 7e0727b738 outdated

 8@@ -9,6 +9,7 @@
 9 
10 #include <chainparamsbase.h>
11 #include <clientversion.h>
12+#include <netaddress.h>

laanwj commented at 3:37 pm on August 6, 2020:

It seems netaddress.cpp is part of LIBBITCOIN_COMMON, which is not linked into bitcoin-cli. So you’re not currently allowed to use this here.

As you are only including this header to get a few constants, ONION and LOCALHOST, might make sense to factor them out to another header? or even just duplicate them?

jonatack commented at 3:03 pm on August 9, 2020:

Thanks! I forgot to look at the makefile. Moved them to httpserver.h, which seems to be allowed, IIUC.

jonatack commented at 3:10 pm on August 9, 2020:

Oops, A new circular dependency in the form of “httpserver -> netbase -> netaddress -> httpserver appears to have been introduced. Will create a header, I guess.

Edit: placed them in util/url.h

Edit 2: removed the constants.

laanwj commented at 3:38 pm on August 6, 2020: member

Concept and functionality ACK

in src/bitcoin-cli.cpp:343 in 7e0727b738 outdated

311+        std::string addr;
312+        m_conn_type conn_type;
313+        bool is_outbound;
314+        bool is_block_relay;
315+        bool operator<(const m_peer& rhs) const { return std::tie(is_outbound, min_ping) < std::tie(rhs.is_outbound, rhs.min_ping); }
316+    };

ariard commented at 5:32 pm on August 6, 2020:

I think adding user agent in verbose mode is worthy. Beyond mode of display, how do you see the data split between this new RPC and getpeerinfo ? Static-set-at-once-at-connection-opening versus dynamic-messages-processed ?

jonatack commented at 10:58 am on August 9, 2020:

By user agent, do you mean getpeerinfo.subver? aka cleanSubVer in net.h

jonatack commented at 3:06 pm on August 9, 2020:

added version+subver together in a column

ariard commented at 5:32 pm on August 6, 2020: member

Concept ACK

theStack commented at 12:09 pm on August 7, 2020: member

Concept ACK :+1:

jonatack force-pushed on Aug 9, 2020

jonatack commented at 3:51 pm on August 9, 2020: member

Took most all of the feedback and also added lastsend and lastrecv in addition to the requested version column.

jonatack commented at 3:54 pm on August 9, 2020: member

Did not see anything in the asmap column. Guess that’s due to me not using the asmap feature.

Updated to not display the asmap column unless it is being used.

jonatack force-pushed on Aug 9, 2020

jonatack commented at 7:48 pm on August 9, 2020: member

What I am using at the moment is this version with additional columns for the time since the last tx (in sec) and the last block (in min), done by adding nLastBlockTime and nLastTXTime to getpeerinfo, because these are possible criteria for inbound peer evictions. Maybe for a follow-up.

  0diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
  1index 32d9cb9d3d..3b14806bea 100644
  2--- a/src/bitcoin-cli.cpp
  3+++ b/src/bitcoin-cli.cpp
  4@@ -307,8 +307,10 @@ private:
  5         int mapped_as;
  6         int version;
  7         int64_t conn_time;
  8+        int64_t last_block;
  9         int64_t last_recv;
 10         int64_t last_send;
 11+        int64_t last_tx;
 12         double min_ping;
 13         double ping;
 14         std::string addr;
 15@@ -410,11 +412,13 @@ public:
 16                 const int version{peer["version"].get_int()};
 17                 const std::string sub_version{peer["subver"].get_str()};
 18                 const int64_t conn_time{peer["conntime"].get_int64()};
 19+                const int64_t last_block{peer["last_block"].get_int64()};
 20                 const int64_t last_recv{peer["lastrecv"].get_int64()};
 21                 const int64_t last_send{peer["lastsend"].get_int64()};
 22+                const int64_t last_tx{peer["last_tx"].get_int64()};
 23                 const double min_ping{peer["minping"].isNull() ? 0 : peer["minping"].get_real()};
 24                 const double ping{peer["pingtime"].isNull() ? 0 : peer["pingtime"].get_real()};
 25-                peers.push_back({peer_id, mapped_as, version, conn_time, last_recv, last_send, min_ping, ping, addr, sub_version, conn_type, is_block_relay, !is_inbound});
 26+                peers.push_back({peer_id, mapped_as, version, conn_time, last_block, last_recv, last_send, last_tx, min_ping, ping, addr, sub_version, conn_type, is_block_relay, !is_inbound});
 27                 is_asmap_on |= (mapped_as != 0);
 28                 max_peer_id_length = std::max(int(ToString(peer_id).length()), max_peer_id_length);
 29                 max_version_length = std::max(int((ToString(version) + sub_version).length()), max_version_length);
 30@@ -425,12 +429,12 @@ public:
 31         // Report 1: detailed peer connections sorted by direction and minimum ping time
 32         if (m_verbose) {
 33             std::sort(peers.begin(), peers.end());
 34-            result += "Peer connections sorted by direction and min ping\n<-> relay  conn   time minping   ping lastsend lastrecv ";
 35+            result += "Peer connections sorted by direction and min ping\n<-> relay  conn   time minping   ping lastsend lastrecv lasttx lastblk ";
 36             if (is_asmap_on) result += "asmap ";
 37             result += strprintf("%*s %-*s address\n", max_peer_id_length, "id", max_version_length, "version");
 38             for (const m_peer& peer : peers) {
 39                 result += strprintf(
 40-                    "%3s %5s %5s%7s%8d%7d %8s %8s%*i %*s %-*s %s\n",
 41+                    "%3s %5s %5s%7s%8d%7d %8s %8s%7s%8s%*i %*s %-*s %s\n",
 42                     peer.is_outbound ? "out" : "in",
 43                     peer.is_block_relay ? "block" : "full",
 44                     ConnTypeEnumToString(peer.conn_type),
 45@@ -439,6 +443,8 @@ public:
 46                     round(1000 * peer.ping),
 47                     peer.last_send == 0 ? "" : ToString(time_now - peer.last_send),
 48                     peer.last_recv == 0 ? "" : ToString(time_now - peer.last_recv),
 49+                    peer.last_tx == 0 ? "" : ToString(time_now - peer.last_tx),
 50+                    peer.last_block == 0 ? "" : ToString(time_now - peer.last_block),
 51                     is_asmap_on ? 6 : 0, // variable spacing
 52                     is_asmap_on && peer.mapped_as != 0 ? ToString(peer.mapped_as) : "",
 53                     max_peer_id_length, // variable spacing
 54@@ -447,7 +453,7 @@ public:
 55                     ToString(peer.version) + peer.sub_version,
 56                     peer.addr);
 57             }
 58-            result += "                   min      ms     ms      sec      sec\n\n";
 59+            result += "                   min      ms     ms      sec      sec    sec     sec\n\n";
 60         }
 61         // Report 2: peer connections summary
 62         total_i = ipv4_i + ipv6_i + onion_i;
 63diff --git a/src/net.cpp b/src/net.cpp
 64index 0c56cddbdc..15f2b6044c 100644
 65--- a/src/net.cpp
 66+++ b/src/net.cpp
 67@@ -530,6 +530,8 @@ void CNode::copyStats(CNodeStats &stats, const std::vector<bool> &m_asmap)
 68     X(nLastRecv);
 69     X(nTimeConnected);
 70     X(nTimeOffset);
 71+    X(nLastBlockTime);
 72+    X(nLastTXTime);
 73     stats.addrName = GetAddrName();
 74     X(nVersion);
 75     {
 76diff --git a/src/net.h b/src/net.h
 77index 17d8fda372..4ecfcce223 100644
 78--- a/src/net.h
 79+++ b/src/net.h
 80@@ -594,6 +594,8 @@ public:
 81     int64_t m_ping_usec;
 82     int64_t m_ping_wait_usec;
 83     int64_t m_min_ping_usec;
 84+    int64_t nLastBlockTime;
 85+    int64_t nLastTXTime;
 86     CAmount minFeeFilter;
 87     // Our address, as reported by the peer
 88     std::string addrLocal;
 89diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
 90index 9981ea35df..533d424611 100644
 91--- a/src/rpc/net.cpp
 92+++ b/src/rpc/net.cpp
 93@@ -169,10 +169,13 @@ static UniValue getpeerinfo(const JSONRPCRequest& request)
 94         obj.pushKV("relaytxes", stats.fRelayTxes);
 95         obj.pushKV("lastsend", stats.nLastSend);
 96         obj.pushKV("lastrecv", stats.nLastRecv);
 97+        obj.pushKV("last_block", stats.nLastBlockTime);
 98+        obj.pushKV("last_tx", stats.nLastTXTime);
 99         obj.pushKV("bytessent", stats.nSendBytes);
100         obj.pushKV("bytesrecv", stats.nRecvBytes);
101         obj.pushKV("conntime", stats.nTimeConnected);
102         obj.pushKV("timeoffset", stats.nTimeOffset);

Output

 0Peer connections sorted by direction and min ping
 1<-> relay  conn   time minping   ping lastsend lastrecv lasttx lastblk  asmap   id version                                    address
 2 in  full  ipv6    902      47     58        4       25                 14061   11 70015/bitnodes.earn.com:0.1/               [2a03:b0c0:2:d0::4bc:2001]:21160
 3 in  full  ipv6    492      53     66        4       49                 34878  995 70002/dsn.tm.kit.edu/bitcoin:0.9.99/       [2a00:1398:4:2a03:921b:eff:fe35:aed9]:41110
 4 in block  ipv6     55      53     69        1        1                 14061 2092 70012/bitcoinj:0.15.6/                     [2a03:b0c0:3:f0::3a:8000]:38318
 5 in  full  ipv6    902      54    120        4       25                 34878   12 70002/dsn.tm.kit.edu/bitcoin:0.9.99/       [2a00:1398:4:2a03:4e52:62ff:fe22:6c13]:41222
 6 in  full  ipv6    900      57     74        4       27                 24940   28 70015/bitnodes.io:0.1/                     [2a01:4f8:10a:37ee::2]:60418
 7 in  full  ipv6    839     131    239        4        2    188          12637  154 70015/Satoshi:0.18.1/                      [2a0b:f4c2:2::1]:4237
 8 in  full  ipv6      1     214    214        5       72                 16509 2240 70015/Satoshi:0.19.1/                      [2600:1f16:625:e00:ab19:5fe3:f155:1371]:53146
 9 in  full  ipv6      1     226    226        5        7                 16509 2239 70015/Satoshi:0.19.1/                      [2406:da18:f7c:4351:5729:102:998c:d41a]:35564
10 in  full  ipv6      9     239    273        5       92                 54098 2204 70015/bitcoinj:0.15-SNAPSHOT/              [2604:d500:4:1::2]:29044
11 in  full  ipv6      2     262    322        5       25                 54098 2237 70015/bitcoinj:0.13.4/Bitcoin Wallet:4.46/ [2604:d500:4:1::2]:37601
12 in  full  ipv6      0     277    277        5       15                 54098 2241 70015/Satoshi:0.10.1/                      [2604:d500:4:1::2]:36310
13 in  full  ipv6      2     286    286        5       25                 54098 2236 70015/bitcoinj:0.14.4/Bitcoin:1.075/       [2604:d500:4:1::2]:41908
14 in  full onion    783     297   1357        4       50                        286 70015/bitnodes.io:0.1/                     127.0.0.1:59452
15 in block onion    614     616    998       19       19                        702 70015/Satoshi:0.20.0/                      127.0.0.1:37594
16 in  full onion     77   58745 157842        5       23                       2038 70015/Satoshi:0.19.1/                      127.0.0.1:33374
17out  full  ipv4     92      57     99        2        9     13          43350 2006 70015/Satoshi:0.18.0/                      109.201.140.9:8333
18out  full  ipv4    901      67    249        2        7     10      18  24940   21 70015/Satoshi:0.19.99/                     94.130.10.158:8333
19out  full  ipv4    352      68    130        2        2      2     105  12876 1357 70015/Satoshi:0.19.0.1/                    51.158.27.215:8333
20out block  ipv4    901     129    201       76       76            277  13768   22 70015/Satoshi:0.19.1/                      64.227.13.20:8333
21out block  ipv4    901     203    280       67       67            839  14061   25 70015/Satoshi:0.17.2/                      165.22.223.100:8333
22out  full  ipv4    281     211    248        3        3      3          63949 1527 70015/Satoshi:0.17.1/                      173.230.157.253:8333
23out  full  ipv4    638     309    391        2       30     30     247 132203  637 70015/Satoshi:0.18.0/                      129.226.125.10:8333
24out  full onion     23     461    789        2       13     13                2163 70015/Satoshi:0.20.99/                     XXXXXXXXy7q47OID.onion:8333
25out  full onion    902     545   1003        3        5     35                  20 70015/Satoshi:0.19.0.1/                    XXXXXXXXuomUx6ln.onion:8333
26out  full onion    223     705   1261        1        1    327                1706 70015/Satoshi:0.19.1/                      XXXXXXXXmexaT7zd.onion:8333
27out  full onion    903     724   1267        4        4   2414                   8 70015/Satoshi:0.20.0/                      XXXXXXXX5pans2i4.onion:8333
28                   min      ms     ms      sec      sec    sec     min
29
30Inbound and outbound peer connections
31in:  ipv4   0  |  ipv6  12  |  onion   3  |  total  15  (2 block-relay)
32out: ipv4   7  |  ipv6   0  |  onion   4  |  total  11  (2 block-relay)
33all: 26
34
35Local addresses
36XXXX:XXX:53c:a200:bb54:3be5:c3d0:9ce5  |  port  8333  |  score   1505
37XXXXXXXXrn5pknnd.onion                 |  port  8333  |  score    235

fanquake deleted a comment on Aug 10, 2020

jonatack force-pushed on Aug 10, 2020

in src/netaddress.cpp:106 in 5cdce7a075 outdated

102@@ -102,7 +103,7 @@ bool CNetAddr::SetInternal(const std::string &name)
103  */
104 bool CNetAddr::SetSpecial(const std::string &strName)
105 {
106-    if (strName.size()>6 && strName.substr(strName.size() - 6, 6) == ".onion") {
107+    if (strName.size()>6 && strName.substr(strName.size() - 6, 6) == ONION_DOMAIN) {

fjahr commented at 4:45 pm on August 10, 2020:

ONION_EXTENSION or ONION_SUFFIX would be a more accurate name here imo

luke-jr commented at 8:09 pm on August 11, 2020:

ONION_TLD?

jonatack commented at 4:42 pm on August 13, 2020:

Dropped the commit adding the constants.

in src/bitcoin-cli.cpp:325 in 324aa5eaf9 outdated

320+    };
321+
322+    bool m_verbose{false}; //!< Whether user requested verbose -netinfo report
323+
324+    /** Whether a peer is an IPv6 connection.
325+     * @returns true if addr starts with '['. */

fjahr commented at 4:50 pm on August 10, 2020:

I don’t think a comment should recite the implementation details of the code. For this level of detail, ppl can read the code. Same with the two functions below.

fjahr commented at 5:49 pm on August 10, 2020:

I would expect something like this instead:

0     * [@returns](/bitcoin-bitcoin/contributor/returns/) true if addr is identified as IPv6. */

jonatack commented at 4:43 pm on August 13, 2020:

Removed the Doxygen comments.

in src/bitcoin-cli.cpp:357 in 324aa5eaf9 outdated

352+
353+    UniValue PrepareRequest(const std::string& method, const std::vector<std::string>& args) override
354+    {
355+        if (!args.empty()) {
356+            const std::string arg{ToLower(args.at(0))};
357+            m_verbose = (arg == "true" || arg == "t" || arg == "1");

fjahr commented at 5:38 pm on August 10, 2020:

I think agreement is now to not allow “1” instead of “true” if it can be avoided. Came up here: #19544 (review)

jonatack commented at 4:36 pm on August 13, 2020:

Interesting, thanks. That may possibly apply more to the RPC API than to the CLI, but ok, done.

fjahr commented at 5:46 pm on August 10, 2020: member

Concept ACK

in src/torcontrol.cpp:32 in 324aa5eaf9 outdated

28@@ -28,7 +29,7 @@
29 #include <event2/thread.h>
30 
31 /** Default control port */
32-const std::string DEFAULT_TOR_CONTROL = "127.0.0.1:9051";
33+const std::string DEFAULT_TOR_CONTROL = LOCALHOST_IPV4 + ":9051";

luke-jr commented at 8:08 pm on August 11, 2020:

Not sure about initialisation order here.

jonatack commented at 4:44 pm on August 13, 2020:

Not sure about initialisation order here.

Was working fine for me the past weeks, but point taken. Dropped the commit adding the constants to simplify things.

jonatack force-pushed on Aug 13, 2020

jonatack commented at 4:56 pm on August 13, 2020: member

Thanks everyone for reviewing and the 9-10 Concept ACKs.

To simplify things, I dropped the commit that added the constants, took the other feedback, and made a number of further improvements. As there were no full ACKs and almost no feedback on the code itself, and to make the changes easy to review, I organised the 200 lines of changes into step-by-step hygienic commits to hopefully garner some final ACKs.

At this point, I’d like to defer any further changes to follow-ups, as there are more improvements I’d propose as well as a release note if this is merged. So looking for ACKs now and hoping to see this in master soon.

in src/bitcoin-cli.cpp:302 in bf1c49a2a0 outdated

297+class NetinfoRequestHandler : public BaseRequestHandler
298+{
299+private:
300+    bool IsAddrIPv6(const std::string& addr) const
301+    {
302+        return addr.front() == '[';

laanwj commented at 12:06 pm on August 14, 2020:

This is undefined behavior if addr is empty, you might want to guard against that.

jonatack commented at 12:11 pm on August 14, 2020:

This is undefined behavior if addr is empty, you might want to guard against that.

Thanks! will fix

jonatack force-pushed on Aug 14, 2020

jonatack commented at 12:36 pm on August 14, 2020: member

Updated with a non-empty check per git diff bf1c49a f63cecc (thanks!)

in src/bitcoin-cli.cpp:355 in f63ceccca3 outdated

350+
351+    UniValue PrepareRequest(const std::string& method, const std::vector<std::string>& args) override
352+    {
353+        if (!args.empty()) {
354+            const std::string arg{ToLower(args.at(0))};
355+            m_verbose = (arg == "true" || arg == "t");

laanwj commented at 2:24 pm on August 14, 2020:

This uses a different boolean argument parsing than the rest of the software, which doesn’t interpret “1” as true, for example. And it doesn’t raise a parse error for invalid values, but simply ignores them (and assumes them to be false). I understand that it’s not possible to use GetBoolArg here because the arguments are passed in, but it’d be nice to be consistent.

Alternatively we could make this optional argument “verbose”/“v” instead of “true”, to not confuse it with boolean parsing at all. I don’t know.

jonatack commented at 3:46 pm on August 14, 2020:

Yes, new context. As this is very simple, doesn’t wrap any RPC arguments and will only be used by humans, I began with the simplest (and probably least annoying option to users) of not raising parsing errors.

vasild commented at 12:49 pm on August 17, 2020:

IMO -netinfo verbose is clearer than -netinfo true. My wish list:

change it to -netinfo verbose
make a reusable function that takes a string and returns Optional<bool>. It can support true/false, t/f, yes/no, y/n, 1/0.
leave it as is (silently interpreting e.g. truw as false :/)

jonatack commented at 7:16 pm on August 23, 2020:

I’m considering proposing replacing the boolean arg with an integer one in the follow-up, to allow choosing from a number of degrees of detail rather than just two.

laanwj commented at 2:52 pm on August 14, 2020: member

Tested and code review ACK f63ceccca31aa624819b429d015f8f1bf2daf49f, my comment above is just a small nit. Tested it on a busy node and it works great.

0xB10C commented at 4:53 pm on August 14, 2020: member

Tested on multiple nodes and light code review ACK f63ceccca31aa624819b429d015f8f1bf2daf49f.

Awesome that this is backwards compatible with e.g. Bitcoin Core v0.18 as well!

fjahr commented at 5:52 pm on August 14, 2020: member

tested ACK f63ceccca31aa624819b429d015f8f1bf2daf49f

Reviewed changes since last review per git diff 324aa5e f63cecc. Tested on a testnet node.

luke-jr referenced this in commit 42a3b8da56 on Aug 15, 2020

luke-jr referenced this in commit 2001104735 on Aug 15, 2020

luke-jr referenced this in commit 93230035e8 on Aug 15, 2020

luke-jr referenced this in commit a2a4e3d3b7 on Aug 15, 2020

luke-jr referenced this in commit f0cca1bd67 on Aug 15, 2020

luke-jr referenced this in commit cda2c22017 on Aug 15, 2020

luke-jr referenced this in commit dfbf649360 on Aug 15, 2020

luke-jr referenced this in commit 5575bc1cbf on Aug 15, 2020

luke-jr referenced this in commit 07a8897dec on Aug 15, 2020

in src/bitcoin-cli.cpp:430 in f63ceccca3 outdated

425+        std::string result{strprintf("%s %s - %i%s\n\n", PACKAGE_NAME, FormatFullVersion(), networkinfo["protocolversion"].get_int(), networkinfo["subversion"].get_str())};
426+
427+        // Report detailed peer connections list sorted by direction and minimum ping time.
428+        if (m_verbose) {
429+            std::sort(peers.begin(), peers.end());
430+            result += "Peer connections sorted by direction and min ping\n<-> relay  conn minping   ping lastsend lastrecv uptime ";

vasild commented at 1:23 pm on August 17, 2020:

Is net more suitable here than conn? The values printed are e.g. ipv4, ipv6.

We have minping and ping. It is unclear whether ping is the last ping or average ping. The output from ping(1) contains something like

0min/avg/max/mdev = 367.149/488.941/628.650/107.507 ms

jonatack commented at 4:02 pm on August 17, 2020:

ping is pingtime in getpeerinfo: this list is essentially getpeerinfo for humans with sometimes-shorter field names to save horizontal space.

jonatack commented at 4:26 pm on August 17, 2020:

I’m open to net for a follow-up if people prefer that. I agree it may be better.

in src/bitcoin-cli.cpp:306 in f63ceccca3 outdated

301+    {
302+        return !addr.empty() && addr.front() == '[';
303+    }
304+    bool IsInboundOnion(int mapped_as, const std::string& addr, const std::string& addr_local) const
305+    {
306+        return mapped_as == 0 && addr.find("127.0.0.1") == 0 && addr_local.find(".onion") != std::string::npos;

vasild commented at 1:29 pm on August 17, 2020:

The TOR proxy could be running on another machine, not necessary on 127.0.0.1. Isn’t addr_local.find(".onion") sufficient?

jonatack commented at 4:16 pm on August 17, 2020:

Given that this dashboard shows all of the peer addresses, AFAICT I’ve found this to be reliable as-is over the past month WRT onions.

Edit: have still not seen an issue with this.

in src/bitcoin-cli.cpp:310 in f63ceccca3 outdated

305+    {
306+        return mapped_as == 0 && addr.find("127.0.0.1") == 0 && addr_local.find(".onion") != std::string::npos;
307+    }
308+    bool IsOutboundOnion(const std::string& addr) const
309+    {
310+        return addr.find(".onion") != std::string::npos;

vasild commented at 1:29 pm on August 17, 2020:

This and the one above would match e.g. “www.onionfoo.com”. Or is addr always numeric for IP networks?

jonatack commented at 4:23 pm on August 17, 2020:

I haven’t seen this to be an issue yet. Happy to look at a proposal.

vasild commented at 7:55 am on August 18, 2020:

I see this for an IPv4 connection to www.onionfoo.com:8345, mistakenly labelled as “conn=onion” instead of “conn=ipv4”.

0<-> relay  conn minping   ping lastsend lastrecv uptime id version               address
1out  full onion       4      4        0        0      0  0 70015/Satoshi:0.20.0/ www.onionfoo.com:8345

0        const char* suffix = ".onion";
1        const size_t suffix_len = 6;
2        return addr.length() > suffix_len &&
3               addr.compare(addr.length() - suffix_len, std::string::npos, suffix) == 0;

jonatack commented at 7:13 pm on August 23, 2020:

In more than a month of running this and also the original python script, I still have not seen a case like this and have not found the node in question on the network. Seems like a pretty pathological edge case but happy to look at tightening the detection in the follow-up.

jonatack commented at 4:49 pm on August 28, 2020:

I have still not yet encountered the described edge case above, but it should be noted that the proposal to replace find with “string ends with” causes many outbound onions to not be detected when they end with .onion:<port>, e.g. .onion:8333 – nearly half of the onions I’m connected to as I write this.

jonatack commented at 8:11 pm on August 28, 2020:

This seems to cover everything:

0bool IsOutboundOnion(const std::string& addr, int mapped_as) const
1{
2    const size_t onion_len{ONION.length()};
3    const size_t pos{addr.rfind(ONION)};
4    return mapped_as == 0 && addr.size() > onion_len &&
5        (pos == addr.size() - onion_len || pos == addr.find_last_of(":") - onion_len);
6}

in src/bitcoin-cli.cpp:386 in f63ceccca3 outdated

381+            const bool is_inbound{peer["inbound"].get_bool()};
382+            m_conn_type conn_type{m_conn_type::ipv4};
383+            if (is_inbound) {
384+                if (IsAddrIPv6(addr)) {
385+                    conn_type = m_conn_type::ipv6;
386+                    ipv6_i += 1;

vasild commented at 1:38 pm on August 17, 2020:

nit: here and in other places: consider ++foo instead of foo += 1.

jonatack commented at 7:09 pm on August 23, 2020:

Noted for a follow-up.

in src/bitcoin-cli.cpp:466 in f63ceccca3 outdated

461+        result += strprintf("all: %i\n", total_i + total_o);
462+
463+        // Report local addresses, ports, and scores.
464+        result += "\nLocal addresses";
465+        const UniValue& local_addrs{networkinfo["localaddresses"]};
466+        for (const UniValue& addr : local_addrs.getValues()) {

vasild commented at 3:20 pm on August 17, 2020:

nit: ditch unnecessary local variable (but maybe keep it if you decide to check whether local_addrs is empty)

0        for (const UniValue& addr : networkinfo["localaddresses"].getValues()) {

in src/bitcoin-cli.cpp:489 in f63ceccca3 outdated

459+        result += strprintf("in:  ipv4 %3i  |  ipv6 %3i  |  onion %3i  |  total %3i  (%i block-relay)\n", ipv4_i, ipv6_i, onion_i, total_i, block_relay_i);
460+        result += strprintf("out: ipv4 %3i  |  ipv6 %3i  |  onion %3i  |  total %3i  (%i block-relay)\n", ipv4_o, ipv6_o, onion_o, total_o, block_relay_o);
461+        result += strprintf("all: %i\n", total_i + total_o);
462+
463+        // Report local addresses, ports, and scores.
464+        result += "\nLocal addresses";

vasild commented at 3:22 pm on August 17, 2020:

Given that the Local addresses header may be followed by nothing, what about skipping it if there are no local addresses or printing something like none or n/a below it in that case?

jonatack commented at 4:22 pm on August 17, 2020:

Skipping it might violate the principle of least surprise but no strong opinion on what would be best or if any change is needed.

vasild commented at 8:00 am on August 18, 2020:

I see this:

0$ ./src/bitcoin-cli -netinfo 
1Bitcoin Core v0.20.99.0-f63ceccca - 70015/Satoshi:0.20.0/
2
3Inbound and outbound peer connections
4in:  ...
5out: ...
6all: ...
7
8Local addresses
9$

I guess it happens when the node does not have an incoming connections and --externalip= is not provided to bitcoind.

pinheadmz commented at 4:06 pm on August 19, 2020:

Same here, behind a firewall with no port forwarding, my local addresses is empty. Not sure the best way to handle it visually. Perhaps even just an empty newline.

in src/bitcoin-cli.cpp:314 in f63ceccca3 outdated

309+    {
310+        return addr.find(".onion") != std::string::npos;
311+    }
312+    bool m_verbose{false}; //!< Whether user requested verbose -netinfo report
313+
314+    enum struct m_conn_type {

vasild commented at 3:27 pm on August 17, 2020:

This and m_peer are types, not member variables. I think they should be named ConnType and Peer.

jonatack commented at 4:20 pm on August 17, 2020:

I hesitated on this. Will update in a follow-up.

in src/bitcoin-cli.cpp:453 in f63ceccca3 outdated

426+
427+        // Report detailed peer connections list sorted by direction and minimum ping time.
428+        if (m_verbose) {
429+            std::sort(peers.begin(), peers.end());
430+            result += "Peer connections sorted by direction and min ping\n<-> relay  conn minping   ping lastsend lastrecv uptime ";
431+            if (is_asmap_on) result += " asmap ";

vasild commented at 3:52 pm on August 17, 2020:

Does it make sense to position this optional column as the last one? The “target audience” for this is humans, but maybe somebody will write a script that parses it and it will get bricked occasionally by the presence or absence of the asmap column if it is in the middle.

Also for humans - if somebody gets used to search for certain information at a certain position on the screen, e.g. id, it would be annoying if it gets shifted to the right by the occasional presence of an extra column.

jonatack commented at 4:08 pm on August 17, 2020:

I gradually adjusted this over the past month of using it continually. Generally I don’t think people change frequently between using asmap or not (based on my recent twitter usage polls, I suspect very few are using asmap yet). I initially wrote this using only asmap, and only realized that it would be annoying when @0xB10C mentioned it was just empty without it. When I did change to test it, it was better to not display it than have an empty column and wasted horizontal space. Thus, the current proposal.

jonatack commented at 7:02 pm on August 23, 2020:

maybe somebody will write a script that parses it and it will get bricked occasionally by the presence or absence of the asmap column if it is in the middle.

Scripts should consume the RPC API, not the CLI.

in src/bitcoin-cli.cpp:420 in f63ceccca3 outdated

415+                const double ping{peer["pingtime"].isNull() ? 0 : peer["pingtime"].get_real()};
416+                peers.push_back({peer_id, mapped_as, version, conn_time, last_recv, last_send, min_ping, ping, addr, sub_version, conn_type, is_block_relay, !is_inbound});
417+
418+                is_asmap_on |= (mapped_as != 0);
419+                max_peer_id_length = std::max(int(ToString(peer_id).length()), max_peer_id_length);
420+                max_version_length = std::max(int((ToString(version) + sub_version).length()), max_version_length);

vasild commented at 3:53 pm on August 17, 2020:

std::string::length() returns size_t and if our variables max_peer_id_length and max_version_length are defined as size_t, then there would not be a need for the typecast to int.

jonatack commented at 4:28 pm on August 17, 2020:

Good idea; noting this style nit for the follow-up.

vasild commented at 3:57 pm on August 17, 2020: member

Approach ACK

vasild commented at 4:03 pm on August 17, 2020: member

Don’t consider my review as a blocker - no stopper issues. All except #19643 (review) can be ignored and that one can be addressed in this or a followup PR.

jonatack commented at 4:18 pm on August 17, 2020: member

Thanks for the detailed review @vasild. As I wrote above in #19643 (comment), I’d like to defer any further code style or edge case changes to follow-ups, as there are more improvements I’d propose, including using #19731 if it is merged.

With 3 tested ACKs, 9 Concept ACKs and an Approach ACK (which for a CLI PR is miraculous), it’s also better to not invalidate the 3 tested ACKs.

I’ve been using this continually for a number of weeks and it works great in practice.

vasild commented at 7:07 am on August 18, 2020: member

To be explicit that I am ok with this as is:

ACK f63ceccca

The worthwhile suggestions I mentioned can be addressed in a follow up.

fanquake commented at 11:09 am on August 18, 2020: member

With 3 tested ACKs, 9 Concept ACKs and an Approach ACK (which for a CLI PR is miraculous), it’s also better to not invalidate the 3 tested ACKs.

I just want to comment on “it’s also better to not invalidate the 3 tested ACKs.”. If this was a taproot PR, some consensus or in some other way critical code, or had accumulated review from a number of less-active but more-knowledgeable for some portion of the code-base reviewers I might agree. However, this is a few hundred lines, adding a new, “cool” feature to a binary which isn’t bitcoind. I don’t think there’s any problem with “invalidating ACKs” here to fix bugs or outstanding issues, and it looks like there might be a couple, or even just fixing this up so that newly added code is following the developer/style guidelines, which really should always be the case.

Speaking generally, I think, “we’ll fix bugs/issues in a follow up”, isn’t the right mindset for this project, not only because I don’t think ACKs are so precious that we’d rather merge suboptimal/buggy code, but also because I don’t want to discourage reviewers from actually reviewing, or have them change how they review based on how many ACKs a PR already has (not saying this is happening right now). Also, when comments are left with follow ups, care has to be taken to make sure they all end up accounted for, and/or the reviewers that left them know they should also be following up in another PR. It’s easy for something to get lost in GitHub’s ever collapsing GUI.

Regardless of the above, I’m a ~0 on this change. I agree with @jnewbery’s comment about this being scope creep. It seems now that we’ve now got to maintain a kind of “GUI” in a cli tool. This also seems like the sort of feature that has the potential to be constantly tweaked/redone to the liking of whoever is modifying this code. Hopefully we don’t end up having to maintain a bunch of tests / some sort of backwards-compatibility for a cli-tool based GUI. I think in general if we can ship tools with nice APIs, so that end-users can write scripts & tools to consume the data, that’s I think that’s a win (hopefully they’ll even share/publish them). Ideally we won’t have to bring too much functionality like this into our tools.

jonatack commented at 12:04 pm on August 18, 2020: member

@fanquake Much could be said and debated concerning the project norms, I suppose, but an issue or gist may be a better place for that. I’m trying to follow the norms as best I’ve been able to observe them and would prefer to keep the discussion here to review/testing of the PR.

practicalswift commented at 3:09 pm on August 18, 2020: contributor

Tested ACK f63ceccca31aa624819b429d015f8f1bf2daf49f

Very nice addition @jonatack - I’ve been using it for a while and I’m already hooked: I can’t wait for it to land in master :)

laanwj commented at 9:14 am on August 19, 2020: member

Agree that it’s not always bad to invalidate ACKs. I’ll happily retest this if it’s needed.

Regardless of the above, I’m a ~0 on this change. I agree with @jnewbery’s comment about this being scope creep. It seems now that we’ve now got to maintain a kind of “GUI” in a cli tool

I don’t disagree with your point but at least we’ve managed to keep this fully client-side.

I’m very wary for server-side scope creep, but don’t have a problem with making our client more user friendly with localized changes.

We have a GUI too. I don’t much of a problem with having a nice command-line tool. I kind of like Rust’s philosophy in that regard (“command line is also an UI”). But if people disagree and think having a simple as possible cli is more important than feel free to close #17314.

Edit: so I kind of see the conflict, on one hand bitcoin-cli is for usage in scripts, on the other it’s a user-facing tool. It’s important to document what is what, that the output for these kind of convenience tools is not a stable API meant for machine parsing.

pinheadmz commented at 4:23 pm on August 19, 2020: member

Concept ACK, tested ACK, approach ACK.

I think this is very cool feature and I tried it out on clearnet and onion. I find the output of getpeerinfo too big or verbose to be helpful without a grep and this makes the data nice and tidy.

None of the style nits are game-changers for me.

 0$ src/bitcoin-cli -datadir=/Volumes/Serenity/bitcoin -netinfo true
 1Bitcoin Core v0.20.99.0-f63ceccca - 70016/Satoshi:0.20.99/
 2
 3Peer connections sorted by direction and min ping
 4<-> relay  conn minping   ping lastsend lastrecv uptime id version                              address
 5out  full onion     384    388        9        8     32  5 70015/Satoshi:0.20.1/                4iczunxx6krcaqbp.onion:8333
 6out  full onion     385    468        9        8     32  3 70015/Satoshi:0.20.0/                oah44d75fd3sbyf3.onion:8333
 7out  full onion     424    561        9        8     32  4 70015/Satoshi:0.18.1/                sjx3sqpsxxn73ihg.onion:8333
 8out  full onion     438    654       13        4     32  0 70015/Satoshi:0.19.1/Knots:20200304/ gx3z4jk7bug3ctka.onion:8333
 9out  full onion     476    574       10       10     32  7 70015/Satoshi:0.18.1/                xjc5ref2kcqxkf6i.onion:8333
10out  full onion     479    619        9        8     32  2 70015/Satoshi:0.20.0/                e5pvabgdvrhznfx5.onion:8333
11out block onion     550    552        4        3     31  9 70015/Satoshi:0.20.0/                yba4brm555denlt7.onion:8333
12out block onion     594    927        4        5     32  8 70015/Satoshi:0.20.0/                uvaqn6udr2vmhxni.onion:8333
13out  full onion     602    755       13       13     32  6 70015/Satoshi:0.20.0/                bzvptsceamcojin4.onion:8333
14out  full onion     692    822       13       12     32  1 70015/Satoshi:0.20.1/                qfuccrxvh5i526g4.onion:8333
15                     ms     ms      sec      sec    min
16
17Inbound and outbound peer connections
18in:  ipv4   0  |  ipv6   0  |  onion   0  |  total   0  (0 block-relay)
19out: ipv4   0  |  ipv6   0  |  onion  10  |  total  10  (2 block-relay)
20all: 10
21
22Local addresses

practicalswift commented at 6:44 pm on August 19, 2020: contributor

@jonatack A minor UI nit if you choose to touch this PR: when testing this PR I noticed that the address column sometimes ends up outside the visible part of the terminal due to very long version strings (these can be 256 chars IIRC).

I suggest printing the address column (which is guaranteed to be short) before the version column. That way all column headers are guaranteed to be visible :)

In other words …

0<-> relay  conn minping   ping lastsend lastrecv uptime id address                     version
1out  full onion     438    654       13        4     32  0 l0ckdwncyph3rpnk.onion:8333 70015/Satoshi:0.19.1/Knots:20200304/Libertarian-Cypherpunk-Anarchists-Supporting-COVID-19-Lockdowns/

… instead of …

0<-> relay  conn minping   ping lastsend lastrecv uptime id version                                                                                              address
1out  full onion     438    654       13        4     32  0 70015/Satoshi:0.19.1/Knots:20200304/Libertarian-Cypherpunk-Anarchists-Supporting-COVID-19-Lockdowns/ l0ckdwncyph3rpnk.onion:8333

Makes sense? :)

(Details in the screen capture above edited to protect the privacy of our users.)

MarcoFalke removed the label P2P on Aug 21, 2020

MarcoFalke removed the label RPC/REST/ZMQ on Aug 21, 2020

MarcoFalke added the label Scripts and tools on Aug 21, 2020

jonatack commented at 7:19 pm on August 23, 2020: member

@practicalswift thanks for the feedback. Worth a try. See also #19643 (review); higher integer values e.g. -netinfo 2 could be passed for more/wider columns.

laanwj referenced this in commit 7f609f68d8 on Aug 24, 2020

sidhujag referenced this in commit 29ed917b69 on Aug 24, 2020

practicalswift commented at 8:41 am on August 26, 2020: contributor

@jonatack When running src/bitcoin-cli -netinfo t I observed a peer with a reported minping of "8601.05568e+06". Could be an uninitialized read?

jonatack commented at 4:09 pm on August 27, 2020: member

@practicalswift A couple of times I saw ephemeral very long minping times, but IIRC it happens only around initial connection of a peer. Not sure. It’s rare.

The nMinPingUsecTime is initialized in net.h to int64_t max value

0    // Best measured round-trip time.
1    std::atomic<int64_t> nMinPingUsecTime{std::numeric_limits<int64_t>::max()};

(per 93ff1b9041a828 net: correctly initialize nMinPingUsecTime, was left uninitialized in CNode. The correct initialization for a minimum-until-now is int64_t’s max value, so initialize it to that.)

The only place it is updated is in net_processing.cpp in ProcessMessage::PONG

0const auto ping_end = time_received;
1../..
2            const auto ping_time = ping_end - pfrom.m_ping_start.load();
3            if (ping_time.count() > 0) {
4                // Successful ping time measurement, replace previous
5                pfrom.nPingUsecTime = count_microseconds(ping_time);
6                pfrom.nMinPingUsecTime = std::min(pfrom.nMinPingUsecTime.load(), count_microseconds(ping_time));
7            } else {
8                // This should never happen

Offhand, I don’t see any uninitialized reads, maybe more a possible lag or initial high value.

jonatack commented at 4:11 pm on August 27, 2020: member

If -netinfo helps us detect things like this, all the better.

jonatack force-pushed on Aug 29, 2020

jonatack commented at 8:31 pm on August 29, 2020: member

I’ve pushed the latest version for people to use/test/review.

Under the hood:

nits addressed
tweaked the onion detection helpers; the change makes no observable difference in my testing but in theory it might better handle a couple of edge cases

User-facing:

changed the optional boolean arg to an integer one between 0 and 4 (default: 0) to enable getting reports with varying details & horizontal sizes
added the new getpeerinfo last_block and last_transaction fields merged in #19731
added a bitcoind server version check and explanatory error message if < v0.21
added total ipv4/ipv6/onion connections to the default report and simplified its output format
grouped all the time columns by increasing time units (ms, ms, sec, sec, min, min)
shortened the column names to gain horizontal space and make room for future additions, while attempting to anticipate the width of possible values to display
the header and footer of the detailed peer connections list are not displayed if there are no connection
the local addresses report displays “n/a” if there are none

As before, this is encapsulated in one easy-to-maintain class in the CLI. Thanks to being in the CLI, you don’t need to restart your node to test it; just building and running the command is enough. I did need to rebase on master to be able to use #19731 and this now targets master/v0.21. To use -netinfo on a node not running on current master, drop the last 4 commits.

I hope this update is useful for people to observe/better understand their peer connections and for reviewing/testing P2P PRs.

jonatack force-pushed on Aug 30, 2020

jonatack renamed this:
~~Add `-netinfo` peer connections dashboard~~
Add -netinfo peer connections dashboard
on Aug 30, 2020

jonatack force-pushed on Aug 30, 2020

cli: create initial -netinfo option, NetinfoRequestHandler class 12242b17a5

cli: add ipv6 and onion address type detection helpers 54799b66b4

jonatack force-pushed on Aug 30, 2020

practicalswift commented at 8:52 pm on August 30, 2020: contributor

@jonatack

Usability nit: Wouldn’t it be nice to have last_trxn, last_blck and conn_time in the same unit (minutes since last event) to make them easily comparable? :)

RandyMcMillan commented at 6:48 am on August 31, 2020: contributor

Looks good on alpine 3.12!

Screen Shot 2020-08-31 at 2 43 50 AM

jonatack commented at 7:06 am on August 31, 2020: member

@practicalswift different frequency timescales really, at least for me, last txns are often only a few seconds ago, whereas last blocks are more on a minutes timescale.

Here’s a screenshot with last txn in min: many are under one minute ago, so lots of zero values:

Screenshot from 2020-08-31 08-47-55

and the same with last txn in sec, which seems to be the more informative timescale:

Screenshot from 2020-08-31 08-47-50

WDYT? Don’t hesitate to share a screenshot if it’s different for you. I’d be especially interested to see one of a really busy node with many connections. @RandyMcMillan nice! :+1:

practicalswift commented at 8:37 am on August 31, 2020: contributor

@jonatack Yes, transactions are received more frequently, but my experience is that minute granularity would be enough: it doesn’t really matter to me if the last transaction was received 29 or 59 seconds ago – knowing that I received the last transaction it under a minute ago is fine.

The nice thing about using the same unit is that one can easily reason about it along the lines of “oh, I’ve been connected to this node for 629 minutes but the last transaction was 500 minutes ago” (instead of “oh, I’ve been connected to this node for 629 minutes but the last transaction was 30000 seconds ago – is that good or bad? I guess I’ll have to bring up bc :))

But as said: this is a UI nit and should not block progress on this PR. FWIW I love this feature and use it literally daily :)

jonatack commented at 8:50 am on August 31, 2020: member

The nice thing about using the same unit is that one can easily reason about it along the lines of “oh, I’ve been connected to this node for 629 minutes but the last transaction was 500 minutes ago” (instead of “oh, I’ve been connected to this node for 629 minutes but the last transaction was 30000 seconds ago – is that good or bad? I guess I’ll have to bring up bc :))

You may be right. That would also allow removing a couple of horizontal spaces from that column and align the columns better. I’ll give it a try. Edit: Yes, I’m hooked on using this, too :)

RandyMcMillan commented at 8:52 am on August 31, 2020: contributor

It may be a good idea to check for extraneous arguments… And (quiet) no response if it isn’t an expected value

Is a security hole? IDK - overkill? not sure…

root@stats:~/bitcoin/src# ./bitcoin-cli -netinfo ~~234 root@stats:~~/bitcoin/src# ./bitcoin-cli -netinfo ~~• root@stats:~~/bitcoin/src# ./bitcoin-cli -netinfo ~~%^&% root@stats:~~/bitcoin/src# ./bitcoin-cli -netinfo §¶•ªº

Screen Shot 2020-08-31 at 4 41 55 AM

jonatack force-pushed on Aug 31, 2020

jonatack commented at 9:08 am on August 31, 2020: member

@RandyMcMillan AFAICT if the arg is unparseable, either an error is returned or the default of 0 is used. @practicalswift Done!

RandyMcMillan commented at 9:33 am on August 31, 2020: contributor

Another possible option would be in a -connect the report could **************** obfuscate the ip addresses and/or toggle dns resolution

jonatack force-pushed on Aug 31, 2020

jonatack commented at 10:43 am on August 31, 2020: member

Re-pushed to fix the server version (200000 -> 209900) and re-verified that all commits build and run.

Another possible option would be in a -connect the report could **************** obfuscate the ip addresses and/or toggle dns resolution

I’m not completely parsing, could you explain this more? (you can also run ./src/bitcoin-cli -netinfo 1 or ./src/bitcoin-cli -netinfo 3 to not see the addresses)

practicalswift commented at 12:29 pm on August 31, 2020: contributor

Some more UI nits. As always with nits feel free to ignore or tackle in a follow-up PR :)

After having used this feature for a while I feel that the five modes of operation might be a case of giving the user too many knobs to tweak/configure.

Currently these are five modes of operation AFAICT:

src/bitcoin-cli -netinfo 0: Executive summary
src/bitcoin-cli -netinfo 1: Connection details (columns: standard columns), executive summary
src/bitcoin-cli -netinfo 2: Connection details (columns: standard columns plus address), executive summary
src/bitcoin-cli -netinfo 3: Connection details (columns: standard columns plus version), executive summary
src/bitcoin-cli -netinfo 4: Connection details (columns: standard columns plus address and version), executive summary

Some potential ways to reduce the number of choices:

If we always printed the executive summary before the detailed view, then users could simply pipe to less or head if he/she is only interested in the executive summary. (Putting the executive summary before the detailed drill-down feels like a reasonable choice in itself: start with the summary, then the details.)

That would remove the need for -netinfo 0.

If we always printed also the address and version (like -netinfo 4), then in the very rare case that a user don’t want to see all columns he/she could simply use cut -b, awk or similar Unix tools to tailor what columns he/she needs. That is probably easier than having to remember which of the arbitrarily numbered 1, 2, 3 or 4 that corresponds to ones preferred column setup :)

We would then simply need one mode of operation (src/bitcoin-cli -netinfo) instead of five modes :)

For me personally that would be preferable given my use cases, but perhaps I’m missing use cases that I’m not having where all five knobs would be good to have :)

cli: tally peer connections by type a3653c159e

cli: start dashboard report with chain and version header 19377b2fd2

cli: create inbound/outbound peer connections report d3f77b736e

cli: create local addresses, ports, and scores report c227100919

cli: add NetType enum struct and NetTypeEnumToString() 3a0ab93e1c

cli: create vector of Peer structs for peers data f5edd66e5d

cli: create peer connections report sorted by dir, minping ce57bf6cc0

cli: add -netinfo server version check and error message 644be659ab

cli: add getpeerinfo last_{block,transaction} to -netinfo 4e2f2ddd64

cli: change -netinfo optional arg from bool to int 077b3ac928

jonatack commented at 3:01 pm on August 31, 2020: member

Thanks for the feedback. I think it’s worthwhile to keep this simple enough that users don’t need to know bash/awk/piping. The default summary is printed last so that busy nodes having many peers will still see it without scrolling up. The use case for being able to leaving off the wide address and/or version columns is for running -netinfo in a less wide buffer window.

jonatack force-pushed on Aug 31, 2020

jonatack commented at 3:06 pm on August 31, 2020: member

Further updates, per git diff 1492754 8587e78

No longer display peer version when value from getpeerinfo is 0
No longer display peer minping or ping when value from getpeerinfo is null
If user enters an integer greater than 4, parse it as the default of 0 rather than 4
Show the chain name when testnet or regtest

practicalswift commented at 8:06 am on September 1, 2020: contributor

Tested ACK 8587e78221492ee26aafbbc332453f7639912a41 – patch looks correct and is limited to src/bitcoin-cli.cpp

RandyMcMillan commented at 11:15 pm on September 1, 2020: contributor

Awesome stuff @jonatack - I was wondering if it would be possible to add some flags to make it automatically report after each new block (or some other arbitrary time interval) - the idea would be to generate a report that could be easily read into another service such as statsd. -netinfo -p (for polling) -I (interval in seconds) -bn=true (triggered by a blocknotify event)

Thanks!

0xB10C commented at 8:42 am on September 2, 2020: member

I was wondering if it would be possible to add some flags to make it automatically report after each new block (or some other arbitrary time interval)

I’ve been using watch bitcoin-cli -netinfo for a dashboard-like view of the report updated every few seconds. I don’t see the need for adding a refreshing, dashboard-like view (and the complexity that comes with it) here.

the idea would be to generate a report that could be easily read into another service such as statsd.

I wouldn’t use the bitcoin-cli -netinfo interface for this. The interface is designed to be developer-readable and not machine-readable. Under the hood -netinfo just calls getnetworkinfo and getpeerinfo. Calling these RPCs directly is probably more suited for your use-case. They are designed to be machine-readable and changes to these RPCs are documented in the release notes. Changes to -netinfo (which could break your statsd importing) will most likely not be documented.

jonatack commented at 9:27 am on September 2, 2020: member

@0xB10C thanks, I couldn’t have said it better. Edit: added your watch suggestion to the PR description.

If we want this in the next release (and feature freeze isn’t far away now), it’s probably best to stabilize this PR on what we have now. It has gone through several rounds of review with lots of good feedback and updates. I’d propose to try to have this merged in its current state and consider further changes (e.g. possible future getpeerinfo fields like conn_type or bip152_hb_{to/from}) after that.

in src/bitcoin-cli.cpp:319 in 8587e78221 outdated

314+        return mapped_as == 0 && onion_pos != std::string::npos && addr_len > ONION_LEN &&
315+               (onion_pos == addr_len - ONION_LEN || onion_pos == addr.find_last_of(":") - ONION_LEN);
316+    }
317+    uint8_t m_details_level{0}; //!< Optional user-supplied arg to set dashboard details level
318+    bool DetailsRequested() const { return m_details_level > 0 && m_details_level < 5; }
319+    bool IsAddressSelected() const { return m_details_level == 2 || m_details_level == 4; }

vasild commented at 9:49 am on September 2, 2020:

Wouldn’t it be surprising to have some details disappear when the verbosity level is increased?

The description says an increasingly detailed peers listing but IsAddressSelected() will be true for details=2 and false for details=3.

jonatack commented at 1:34 pm on September 2, 2020:

Going from details level 2 to 3, it is indeed increasingly detailed as the address is replaced by 2 fields combined in one, version and sub-version. That said, I don’t mind improving it if there is a good suggestion.

jonatack commented at 1:39 pm on September 2, 2020:

(As there may be people who prefer to see one or the other of these two wider fields, but not necessarily both for reasons of window size or interest, and since it isn’t complicated to do–and is done–it’s good to allow people the choice.)

practicalswift commented at 1:53 pm on September 2, 2020:

FWIW I also found it confusing that increasing what I thought was a numeric verbosity level removed columns :)

I think the expected command-line UI for verbosity switches are one of these two:

Increase verbosity like for curl: curl -v, curl -vv, curl -vvv, etc.
Specify exactly what you want like for ps: ps -o comm,label

jonatack commented at 1:55 pm on September 2, 2020:

Concrete suggestion for the help?

jonatack commented at 2:03 pm on September 2, 2020:

How about “An optional integer argument from 0 to 4 can be passed for different peers listings (default: 0)”

practicalswift commented at 2:11 pm on September 2, 2020:

Yes, that is probably good enough for now. Making the verbosity levels more in tune with how other command-line tools work can be done in a follow-up PR: no need to do it here :)

jonatack commented at 2:21 pm on September 2, 2020:

Passing args like for curl or ps would be indeed cool, but yes, overkill for now and maybe hurt this being merged by making it too large.

jonatack commented at 2:25 pm on September 2, 2020:

Done. Thanks for the feedback. The help seems better now.

cli -netinfo: display multiple levels of details bf1f913c44

jonatack force-pushed on Sep 2, 2020

practicalswift commented at 2:42 pm on September 2, 2020: contributor

ACK bf1f913c4405cba35c8f99ec07b407940eb955b6 – patch looks correct and is limited to src/bitcoin-cli.cpp

vasild approved

vasild commented at 7:03 pm on September 2, 2020: member

ACK bf1f913

It is easier to change the doc to conform to the code :-D

in src/bitcoin-cli.cpp:398 in bf1f913c44

393+        bool is_asmap_on{false};
394+        std::vector<Peer> peers;
395+        const UniValue& getpeerinfo{batch[ID_PEERINFO]["result"]};
396+
397+        for (const UniValue& peer : getpeerinfo.getValues()) {
398+            const std::string addr{peer["addr"].get_str()};

n-thumann commented at 12:11 pm on September 4, 2020:

peer["addr"] can also contain hostnames (bitcoin-cli addnode "foo.bar" onetry) or IPv6 addresses without square brackets (bitcoin-cli addnode "2001:db8::1337" onetry). In theses cases the IsAddrIPv6 will not recognize them properly and they will be always marked as IPv4 🤔.

n-thumann commented at 12:13 pm on September 4, 2020:

Maybe, if there are no square brackets, ask inet_pton if it´s a valid IPv6 address? But what if there´s a hostname?

n-thumann commented at 2:32 pm on September 4, 2020:

Dug a little bit deeper: Adding a new field to getpeerinfo seems to be a better solution IMO. The actual remote address (that is IPv6 with square brackets or resolved hostname) is already stored here, but only addrName is passed back as addr instead. I would suggest to return both addrName & addr so that the former contains the hostname (or ordinary IP address) and the latter the actual resolved remote address. This would at least solve the hostname/IPv6 problems, not sure how that effects onion addresses. Let me know what you think and if I should prepare a PR :)

jonatack commented at 2:50 pm on September 4, 2020:

@n-thumann Thanks for having a look. I don’t plan to make further changes for edge cases unless they can be addressed within the scope of this PR (but am willing to update if they can be!)

The net address code is currently being deeply changed for BIP155 addrv2. This PR has seen a lot of review and updates, and if we begin gating it on changes in code outside this PR, we’ll risk missing the next feature freeze (October 15 deadline for merge). I think this is very useable in its current state–if you start using it, good luck doing without it afterward :smiley:–and if merged, I’ll be happy to make further improvements based on the state of the codebase at that time.

n-thumann approved

n-thumann commented at 4:59 pm on September 4, 2020: contributor

tACK, works as expected! Minor bug in edge cases is tolerable ✌️

0xB10C commented at 1:04 pm on September 5, 2020: member

ACK bf1f913c4405cba35c8f99ec07b407940eb955b6

I tested with a testnet node having connections both via clearnet and tor.

nit: I think age is a bit less ambiguous than uptime. uptime could be confused with the actual uptime of the remote node and not the time since the connection was established.

jonatack commented at 9:29 pm on September 5, 2020: member

@0xB10C thanks! s/uptime/age/ sgtm for next update/follow-up.

practicalswift commented at 7:59 am on September 7, 2020: contributor

I think this src/bitcoin-cli.cpp-only PR is ready to merge with four ACK:s (@vasild @0xB10C @practicalswift @n-thumann) and six Concept ACK:s (@jnewbery @hebasto @laanwj @ariard @theStack @RandyMcMillan).

Thanks @jonatack for coming up with this nifty utility. I’m using it daily!

Looking forward to being able to use it directly from master! :)

MarcoFalke commented at 8:12 am on September 7, 2020: member

Concept ACK, haven’t reviewed/tested at all, but this might come in handy when testing pull requests on the live network.

bitcoin-cli isn’t a stable interface, so we can change or remove this at will if issues come up in the future.

0xB10C commented at 12:41 pm on September 7, 2020: member

fwiw: ce57bf6cc0cdaf8233fd8a20e0d1c5b69d17d2a3 (leaving out the last three commits) can be used with bitcoind versions before v0.20.99.

edit: archiving this here: https://github.com/0xB10C/bitcoin/tree/jonatacks-netinfo-pre-v0.20.99

fanquake merged this on Sep 15, 2020

fanquake closed this on Sep 15, 2020

jonatack deleted the branch on Sep 15, 2020

jonatack commented at 7:44 am on September 15, 2020: member

Next steps checklist

Easy, do in next commit (done in #20115):

release note
s/uptime/age/ per @0xB10C suggestion
add one additional space between the net and mping columns
accomodate variable size of addrv2 addresses
handle occasional large mping/ping times like 1.17348e+06 per @practicalswift feedback
add new signet chain
display reachable networks

Medium term:

expose CNetAddr::GetNetClass or similar to getpeerinfo to know peers’ net types and remove our helper functions that guess it (proposed in #20002)
possibly integrate other getpeerinfo fields like addnode or your suggestions, or in the future, maybe bip152_hb_{to,from}, conn_type, etc.

sidhujag referenced this in commit 7c9738a42a on Sep 15, 2020

jonatack commented at 9:42 pm on September 16, 2020: member

@practicalswift A couple of times I saw ephemeral very long minping times @practicalswift I’m still seeing these (both mping and ping), so added it to the checklist above.

jnewbery commented at 9:47 am on September 17, 2020: member

I’ve been using this to test another PR and it’s very helpful. Thanks @jonatack !

One thing I’ve noticed is that connections are inaccurately classified as blocks-only on first connection, I think because all connections start out as fRelayTxes set to false. It’d be better to explicitly use the connection type.

jonatack commented at 10:43 am on September 20, 2020: member

Here’s a branch with some of the updates, including using the connection type:

https://github.com/jonatack/bitcoin/commits/netinfo-2

laanwj referenced this in commit 2e24197117 on Oct 29, 2020

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

Fabcien referenced this in commit 02e6486e5d on Mar 22, 2021

DrahtBot locked this on Aug 16, 2022

Add -netinfo peer connections dashboard #19643

Conflicts

Next steps checklist