Add -netinfo peer connections dashboard by jonatack · Pull Request #19643 · bitcoin/bitcoin

jonatack · 2020-08-02T16:38:08Z

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

$ ./src/bitcoin-cli -help | grep -A3 netinfo
  -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).

jonatack · 2020-08-02T17:51:26Z

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

ghost · 2020-08-02T18:21:00Z

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

DrahtBot · 2020-08-02T20:13:25Z

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

Conflicts

No conflicts as of last run.

0xB10C · 2020-08-03T10:00:05Z

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

practicalswift · 2020-08-03T15:51:22Z

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

0xB10C

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!

jnewbery

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.

hebasto

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

jonatack · 2020-08-06T10:54:16Z

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 · 2020-08-06T10:55:58Z

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!

laanwj · 2020-08-06T15:38:20Z

Concept and functionality ACK

ariard

Concept ACK

theStack · 2020-08-07T12:09:00Z

Concept ACK 👍

jonatack · 2020-08-09T15:51:13Z

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

jonatack · 2020-08-09T15:54:41Z

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 · 2020-08-31T15:06:58Z

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 · 2020-09-01T08:06:28Z

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

RandyMcMillan · 2020-09-01T23:15:54Z

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 · 2020-09-02T08:42:38Z

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 · 2020-09-02T09:27:30Z

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

practicalswift · 2020-09-02T14:42:18Z

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

vasild

ACK bf1f913

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

n-thumann · 2020-09-04T12:11:42Z

+        const UniValue& getpeerinfo{batch[ID_PEERINFO]["result"]};
+
+        for (const UniValue& peer : getpeerinfo.getValues()) {
+            const std::string addr{peer["addr"].get_str()};


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

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

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

@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 😃--and if merged, I'll be happy to make further improvements based on the state of the codebase at that time.

n-thumann

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

0xB10C · 2020-09-05T13:04:34Z

ACK bf1f913

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 · 2020-09-05T21:29:40Z

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

practicalswift · 2020-09-07T07:59:02Z

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! :)

maflcko · 2020-09-07T08:12:22Z

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 · 2020-09-07T12:41:46Z

fwiw: ce57bf6 (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

jonatack · 2020-09-15T07:44:16Z

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 net, rpc, cli: expose peer network in getpeerinfo; simplify/improve -netinfo #20002)
possibly integrate other getpeerinfo fields like addnode or your suggestions, or in the future, maybe bip152_hb_{to,from}, conn_type, etc.

jonatack · 2020-09-16T21:42:42Z

@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 · 2020-09-17T09:47:07Z

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 · 2020-09-20T10:43:03Z

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

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

398045b cli -netinfo: print oversized/extreme ping times as "-" (Jon Atack) 773f4c9 cli -netinfo: handle longer tor v3 local addresses (Jon Atack) 33e9874 cli -netinfo: make age column variable-width (Jon Atack) f8a1c4d cli -netinfo: various quick updates and fixes (Jon Atack) Pull request description: Quick fixups and updates for v0.21.0: - [x] handle larger BIP155 `addrv2` addresses - [x] add Signet chain - [x] add an additional space between the `net` and `mping` columns; add missing `tinyformat` and `algorithm` headers - [x] s/uptime/age/ per 0xB10C suggestion, and make the column auto-adjusting variable width - [x] display `-` for oversized mping/ping times like `1.17348e+06`, as reported by practicalswift Edit: removed the release note commit, as this PR was not merged before the notes were moved to the wiki. It's here: ``` - A new `bitcoin-cli -netinfo` command returns a network peer connections dashboard that displays data from the `getpeerinfo` and `getnetworkinfo` RPCs in a human-readable format. An optional integer argument from `0` to `4` may be passed to see various levels of detail. (#19643) ``` ACKs for top commit: michaelfolkson: ACK 398045b Emzy: Tested ACK 398045b Tree-SHA512: 0625ee840141bafbfcaf8f1fce53f8f850ae91721b2bdad4279372da87c18a1fe3a214d90bfdbbabdf6da38d58290d7dd0f1109b4e2ca5d20cacf417d6ced0f9

Summary: ``` 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 $ ./src/bitcoin-cli -help | grep -A3 netinfo -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). ``` Backport of [[bitcoin/bitcoin#19643 | core#19643]]. Depends on D9331 and D9333. Test Plan: watch ./src/bitcoin-cli -netinfo 4 Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9334

DrahtBot added P2P RPC/REST/ZMQ labels Aug 2, 2020

jonatack force-pushed the netinfo branch from 9f566b0 to 286adbb Compare August 2, 2020 20:02

DrahtBot mentioned this pull request Aug 2, 2020

net: change CNetAddr::ip to have flexible size #19628

Merged

jonatack force-pushed the netinfo branch from 286adbb to 9c41530 Compare August 2, 2020 21:02

This was referenced Aug 2, 2020

refactor: Cleanup thread ctor calls #19064

Merged

torcontrol: add -tortarget config #19043

Closed

jonatack force-pushed the netinfo branch from 9c41530 to 7e0727b Compare August 3, 2020 07:33

0xB10C reviewed Aug 4, 2020

View reviewed changes

Comment thread src/bitcoin-cli.cpp Outdated

Comment thread src/netaddress.h Outdated

Comment thread src/httpserver.cpp Outdated

Comment thread src/bitcoin-cli.cpp Outdated

Comment thread src/bitcoin-cli.cpp Outdated

0xB10C reviewed Aug 4, 2020

View reviewed changes

Comment thread src/bitcoin-cli.cpp Outdated

jnewbery reviewed Aug 6, 2020

View reviewed changes

Comment thread src/httpserver.cpp Outdated

Comment thread src/torcontrol.cpp Outdated

hebasto requested changes Aug 6, 2020

View reviewed changes

Comment thread src/bitcoin-cli.cpp Outdated

Comment thread src/bitcoin-cli.cpp Outdated

Comment thread src/netaddress.h Outdated

laanwj reviewed Aug 6, 2020

View reviewed changes

Comment thread src/bitcoin-cli.cpp Outdated

ariard reviewed Aug 6, 2020

View reviewed changes

Comment thread src/bitcoin-cli.cpp Outdated

DrahtBot mentioned this pull request Aug 8, 2020

refactor: make EncodeBase{32,64} consume Spans #19687

Merged

jonatack force-pushed the netinfo branch 2 times, most recently from d64d4fc to c456ffe Compare August 9, 2020 15:46

jonatack force-pushed the netinfo branch 2 times, most recently from e64170b to f0ff7cb Compare August 9, 2020 18:39

vasild reviewed Sep 2, 2020

View reviewed changes

Comment thread src/bitcoin-cli.cpp Outdated

cli -netinfo: display multiple levels of details

bf1f913

jonatack force-pushed the netinfo branch from 8587e78 to bf1f913 Compare September 2, 2020 14:25

vasild approved these changes Sep 2, 2020

View reviewed changes

n-thumann reviewed Sep 4, 2020

View reviewed changes

n-thumann approved these changes Sep 4, 2020

View reviewed changes

jonatack mentioned this pull request Sep 11, 2020

net: CNetAddr: add support to (un)serialize as ADDRv2 #19845

Merged

fanquake merged commit a336518 into bitcoin:master Sep 15, 2020

jonatack deleted the netinfo branch September 15, 2020 07:11

jonatack mentioned this pull request Sep 20, 2020

net, rpc: expose connection type in getpeerinfo #19883

Closed

ghost mentioned this pull request Sep 26, 2020

Not getting any ipv6 connections #20021

Closed

jonatack mentioned this pull request Oct 9, 2020

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

Merged

5 tasks

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Uh oh!

Conversation

jonatack commented Aug 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Aug 2, 2020

Uh oh!

ghost commented Aug 2, 2020

Uh oh!

DrahtBot commented Aug 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

0xB10C commented Aug 3, 2020

Uh oh!

practicalswift commented Aug 3, 2020

Uh oh!

0xB10C left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack commented Aug 6, 2020

Uh oh!

hebasto commented Aug 6, 2020

Uh oh!

Uh oh!

laanwj commented Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theStack commented Aug 7, 2020

Uh oh!

jonatack commented Aug 9, 2020

Uh oh!

jonatack commented Aug 9, 2020

Uh oh!

jonatack commented Aug 31, 2020

Uh oh!

practicalswift commented Sep 1, 2020

Uh oh!

RandyMcMillan commented Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xB10C commented Sep 2, 2020

Uh oh!

jonatack commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Sep 2, 2020

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

n-thumann Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n-thumann Sep 4, 2020

Choose a reason for hiding this comment

Uh oh!

jonatack commented Aug 2, 2020 •

edited

Loading

DrahtBot commented Aug 2, 2020 •

edited

Loading

laanwj commented Aug 6, 2020 •

edited

Loading

RandyMcMillan commented Sep 1, 2020 •

edited

Loading

jonatack commented Sep 2, 2020 •

edited

Loading

n-thumann Sep 4, 2020 •

edited

Loading

jonatack Sep 4, 2020 •

edited

Loading

0xB10C commented Sep 7, 2020 •

edited

Loading

jonatack commented Sep 15, 2020 •

edited

Loading

jonatack commented Sep 16, 2020 •

edited

Loading