netinfo: display addr_{processed, rate_limited, relay_enabled} and relaytxes data #22501

jonatack · 2021-07-19T15:01:06Z

Update CLI -netinfo to display the getpeerinfo addr_processed, addr_rate_limited, addr_relay_enabled and relaytxes data with auto-adjusting column widths.

$ ./src/bitcoin-cli -netinfo help

  txn      Time since last novel transaction received from the peer and accepted into our mempool, in minutes
           "*" - the peer requested we not relay transactions to it (relaytxes is false)

  addrp    Total number of addresses processed, excluding those dropped due to rate limiting
           "." - we do not relay addresses to this peer (addr_relay_enabled is false)

  addrl    Total number of addresses dropped due to rate limiting

lsilva01

Tested ACK a5cc9e2 on mainnet on Ubuntu 20.04.

fanquake · 2021-07-20T02:52:54Z

It looks like a5cc9e28f9e125a62168dcbe0b3484d098e56c1f should be part of 1893c907eac1e52294add6e059f192eb13702829 & ad5ea77d5dde3fb40dd4701ea0b92e445e060ef0 rather than a separate commit.

How is it determined when to add new information and/or verbosity levels? Given they are additive, it seems unsustainable to just keep adding new levels and more info.

jonatack · 2021-07-20T13:30:21Z

It looks like a5cc9e2 should be part of 1893c90 & ad5ea77 rather than a separate commit.

Done!

How is it determined when to add new information and/or verbosity levels? Given they are additive, it seems unsustainable to just keep adding new levels and more info.

Right, the data provided by getpeerinfo seems to be the upper bound, and apart from the bytes sent/received objects, there are only a handful of remaining fields. My current thinking is to orient levels 1-4 to node operators, and to use level 5 for data more suited to dev/testing. Open to feedback, of course.

jonatack · 2021-07-20T13:32:36Z

Combined the help into the first two commits as suggested by @fanquake and changed the help as follows: git diff a5cc9e2 a52c94c

diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index 4c3cc22dfc..5a0371e3de 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -592,7 +592,7 @@ public:
         "                                  2 - Like 1 but with an address column\n"
         "                                  3 - Like 1 but with a version column\n"
         "                                  4 - Like 1 but with both address and version columns\n"
-        "                                  5 - Like 4 but with additional statistics in the peers listing (addrl, addrp)\n"
+        "                                  5 - Like 4 but with additional data for development and testing purposes\n"

klementtan

Code Review and tested ACK a52c94c

src/bitcoin-cli.cpp

jarolrod

ACK a52c94c

Tested on macOS 11.3. Tested functionality and also tested each commit one by one rebased on current master.

Notes on commits:

3934384
- Code review ACK
- At this point the new addrl and addrp are available at any numbered detail level, as it should be at this point since we have not introduced a new level
22fa911
- tested that this commit makes the new fields available only at level 5, previously they were available at any level
- Couple of concerns, both are non-blockers for me
  1. Help message for new level: https://github.com/bitcoin/bitcoin/pull/22501/files#r674477509
  2. Combining the first two commits: https://github.com/bitcoin/bitcoin/pull/22501/files#r674477522
f92bce8
- code review ack
a52c94c
- code review ack

RandyMcMillan · 2021-07-22T03:50:55Z

tACK a52c94c

macOS 11.4

One nit:

The data repaints if it detects a window resize - but if the user uses <Command> + R to "reload" the window - the data doesn't automatically repaint. Not a huge issue. Note the macOS 11 Terminal has repurposed <Command> + R for "Allow mouse reporting" but apps like iTerm still use <Command> + R for "Reset"

One request:

If the terminal listened for up arrows and down arrows to change data info resolution - that would be awesome.

sipa · 2021-07-22T05:12:15Z

I'm really not convinced these numbers are sufficiently important to turn into a new field, unless the expectation is that many more things will be added to it.

jonatack · 2021-07-24T15:35:37Z

Thanks for the feedback everyone. Droppied the middle two commits; punting on adding a new level until or if there's a clearer need.

Diff: git range-diff 2b5563b a52c94c 936d8ff

klementtan · 2021-07-25T16:39:48Z

code review and tested reACK 936d8ff49beeac5e0c77a87618e021b09d55c7a1

practicalswift · 2021-07-25T20:29:47Z

Concept ACK

Thanks for improving this feature: -netinfo is great addition to the toolbox! :)

vasild

ACK 936d8ff49beeac5e0c77a87618e021b09d55c7a1

src/bitcoin-cli.cpp

Zero-1729

tACK 936d8ff49beeac5e0c77a87618e021b09d55c7a1

Tested on macOS v11.4

Minor code review nit: agree with @vasild regarding printing 0 instead of an empty string for addr_rate_limited. Below is the output with his code change suggestion:

jonatack · 2021-07-27T19:27:05Z

Thanks @Zero-1729! A propos displaying zeroes, see #22501 (comment).

jarolrod · 2021-07-28T03:04:06Z

tACK 936d8ff49beeac5e0c77a87618e021b09d55c7a1

jonatack · 2021-08-10T10:36:04Z

Thanks @klementtan, @vasild, @Zero-1729 and @jarolrod for the ACKs!

I've pushed an update for these reasons:

to indicate peers that requested we not relay transactions, i.e. relaytxes is false
to indicate peers we don't relay addresses to, i.e. addr_relay_enabled is false (this is a new getpeerinfo field and required rebasing here)
added null checking on these new nodestatestats addr fields to avoid UniValue JSON parsing errors
inversed the order of the addrp and addrl columns and moved them to the left of the age column, so that age is next to id
updated the PR title and description

src/bitcoin-cli.cpp

Zero-1729

re-tACK 1df9552

Tested on macOS v11.4

936d8ff -> 1df9552, clean update (still works as expected) 🧉

Updated docs look good:

Note: left a minor nit regarding txn.

$ src/bitcoin-cli -netinfo help
...
  txn      Time since last novel transaction received from the peer and accepted into our mempool, in minutes
           "." - the peer requested we not relay transactions to it (relaytxes is false)
  blk      Time since last novel block passing initial validity checks received from the peer, in minutes
  hb       High-bandwidth BIP152 compact block relay
           "." (to)   - we selected the peer as a high-bandwidth peer
           "*" (from) - the peer selected us as a high-bandwidth peer
  addrp    Total number of addresses processed, excluding those dropped due to rate limiting
           "." - we do not relay addresses to this peer (addr_relay_enabled is false)
  addrl    Total number of addresses dropped due to rate limiting
...

Personally love the new column arrangement; processed addresses (addrp) before rate-limit-dropped ones (addrl), followed by age before id:

i.e. peers where getpeerinfo#relaytxes is false

i.e. peers where getpeerinfo#addr_relay_enabled is false

jonatack · 2021-08-13T10:32:24Z

Updated per #22501 (comment) review feedback by @Zero-1729 (thanks!)

git diff 1df9552 218862a

diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index f960737bfd..718ad5dcef 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -536,7 +536,7 @@ public:
                     PingTimeToString(peer.ping),
                     peer.last_send ? ToString(m_time_now - peer.last_send) : "",
                     peer.last_recv ? ToString(m_time_now - peer.last_recv) : "",
-                    peer.last_trxn ? ToString((m_time_now - peer.last_trxn) / 60) : peer.is_block_relay ? "." : "",
+                    peer.last_trxn ? ToString((m_time_now - peer.last_trxn) / 60) : peer.is_block_relay ? "*" : "",
                     peer.last_blck ? ToString((m_time_now - peer.last_blck) / 60) : "",
                     strprintf("%s%s", peer.is_bip152_hb_to ? "." : " ", peer.is_bip152_hb_from ? "*" : " "),
                     m_max_addr_processed_length, // variable spacing
@@ -627,7 +627,7 @@ public:
         "  send     Time since last message sent to the peer, in seconds\n"
         "  recv     Time since last message received from the peer, in seconds\n"
         "  txn      Time since last novel transaction received from the peer and accepted into our mempool, in minutes\n"
-        "           \".\" - the peer requested we not relay transactions to it (relaytxes is false)\n"
+        "           \"*\" - the peer requested we not relay transactions to it (relaytxes is false)\n"
         "  blk      Time since last novel block passing initial validity checks received from the peer, in minutes\n"
         "  hb       High-bandwidth BIP152 compact block relay\n"
         "           \".\" (to)   - we selected the peer as a high-bandwidth peer\n"

Zero-1729

re-tACK 218862a

Tested on macOS v11.4

1df9552 -> 218862a, clean update 🧼 (LGTM)

klementtan · 2021-08-16T02:25:54Z

re crAck and tAck 218862a

vasild

ACK 218862a

jonatack · 2021-08-22T12:47:11Z

Thanks for all the reviews! I updated the PR description with a current screenshot. One peer seems to be spamming addresses a bit:

<->   type   net  mping   ping send recv  txn  blk  hb addrp addrl  age  asmap   id address                                                             version
out   full  ipv4    204    214    0    0    0          14958 31870 2334   6830  296 178.200.242.132:8333                                                70016/Satoshi:0.21.1/

jarolrod

tACK 218862a

tested on Ubuntu 20.04 and macOS 12. PR works nicely.

A note on 0a9ee3a: While not necessary for the goals of this PR; the usage of the conditional operator and reordering of the logic is a welcome simplification, it's certainly neater.

jonatack · 2021-08-26T08:34:50Z

This has many ACKs, seems ready for merge?

0xB10C · 2021-08-30T10:08:05Z

Code review and tested ACK 218862a

sipa · 2021-09-02T14:40:33Z

Concept ACK

…relay_enabled} and relaytxes data 218862a Display peers in -netinfo that we don't relay addresses to (Jon Atack) 3834e23 Display peers in -netinfo that request we not relay transactions (Jon Atack) 0a9ee3a Simplify a few conditionals in -netinfo (Jon Atack) 5eeea8e Add addr_processed and addr_rate_limited stats to -netinfo (Jon Atack) Pull request description: Update CLI -netinfo to display the getpeerinfo `addr_processed`, `addr_rate_limited`, `addr_relay_enabled` and `relaytxes` data with auto-adjusting column widths. ``` $ ./src/bitcoin-cli -netinfo help txn Time since last novel transaction received from the peer and accepted into our mempool, in minutes "*" - the peer requested we not relay transactions to it (relaytxes is false) addrp Total number of addresses processed, excluding those dropped due to rate limiting "." - we do not relay addresses to this peer (addr_relay_enabled is false) addrl Total number of addresses dropped due to rate limiting ``` ![Screenshot from 2021-08-22 14-31-40](https://user-images.githubusercontent.com/2415484/130355514-f6fd4f21-79d6-463b-9791-de01ebef20b1.png) ACKs for top commit: 0xB10C: Code review and tested ACK 218862a Zero-1729: re-tACK 218862a vasild: ACK 218862a jarolrod: tACK 218862a Tree-SHA512: bb9da4bdd71859b234f6e4c2c46257a57ef0d0e0b363d2b8fded128bcaa28132f64a0a4651c622e1de1e3b7c05c7587a4369e9e79799895884fda9745c63409d

lsilva01 approved these changes Jul 19, 2021

View reviewed changes

laanwj added RPC/REST/ZMQ Feature labels Jul 20, 2021

jonatack force-pushed the netinfo-addr-statistics branch from a5cc9e2 to a52c94c Compare July 20, 2021 13:21

klementtan approved these changes Jul 22, 2021

View reviewed changes

jarolrod reviewed Jul 22, 2021

View reviewed changes

src/bitcoin-cli.cpp Outdated Show resolved Hide resolved

jarolrod reviewed Jul 22, 2021

View reviewed changes

src/bitcoin-cli.cpp Outdated Show resolved Hide resolved

jarolrod reviewed Jul 22, 2021

View reviewed changes

jonatack force-pushed the netinfo-addr-statistics branch from a52c94c to 936d8ff Compare July 24, 2021 15:26

jonatack changed the title ~~cli: add new address statistics to -netinfo~~ cli: add getpeerinfo addr_{processed, rate_limited} data to -netinfo Jul 24, 2021

bitcoin deleted a comment from achirawi Jul 25, 2021

maflcko added Utils/log/libs and removed RPC/REST/ZMQ labels Jul 25, 2021

vasild approved these changes Jul 27, 2021

View reviewed changes

src/bitcoin-cli.cpp Outdated Show resolved Hide resolved

src/bitcoin-cli.cpp Outdated Show resolved Hide resolved

Zero-1729 approved these changes Jul 27, 2021

View reviewed changes

jonatack added 2 commits August 10, 2021 12:17

Add addr_processed and addr_rate_limited stats to -netinfo

5eeea8e

Simplify a few conditionals in -netinfo

0a9ee3a

jonatack force-pushed the netinfo-addr-statistics branch from 936d8ff to 1df9552 Compare August 10, 2021 10:29

jonatack changed the title ~~cli: add getpeerinfo addr_{processed, rate_limited} data to -netinfo~~ netinfo: display addr_{processed, rate_limited, relay_enabled} and relaytxes data Aug 10, 2021

Zero-1729 reviewed Aug 13, 2021

View reviewed changes

src/bitcoin-cli.cpp Outdated Show resolved Hide resolved

Zero-1729 approved these changes Aug 13, 2021

View reviewed changes

jonatack added 2 commits August 13, 2021 12:19

Display peers in -netinfo that request we not relay transactions

3834e23

i.e. peers where getpeerinfo#relaytxes is false

Display peers in -netinfo that we don't relay addresses to

218862a

i.e. peers where getpeerinfo#addr_relay_enabled is false

jonatack force-pushed the netinfo-addr-statistics branch from 1df9552 to 218862a Compare August 13, 2021 10:28

Zero-1729 approved these changes Aug 13, 2021

View reviewed changes

Mszare mentioned this pull request Aug 16, 2021

properly deprecate mempool entry RPC fee fields #22682

Closed

jonatack mentioned this pull request Aug 19, 2021

net/net processing: Move tx inventory into net_processing #21160

Merged

vasild approved these changes Aug 20, 2021

View reviewed changes

jarolrod reviewed Aug 24, 2021

View reviewed changes

jonatack mentioned this pull request Aug 31, 2021

v22.0 testing #22634

Closed

laanwj merged commit a70768d into bitcoin:master Sep 2, 2021

jonatack deleted the netinfo-addr-statistics branch September 2, 2021 15:21

bitcoin locked as resolved and limited conversation to collaborators Sep 2, 2022

netinfo: display addr_{processed, rate_limited, relay_enabled} and relaytxes data #22501

netinfo: display addr_{processed, rate_limited, relay_enabled} and relaytxes data #22501

Uh oh!

Conversation

jonatack commented Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lsilva01 left a comment • 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!

fanquake commented Jul 20, 2021

Uh oh!

jonatack commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Jul 20, 2021

Uh oh!

klementtan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

RandyMcMillan commented Jul 22, 2021

Uh oh!

sipa commented Jul 22, 2021

Uh oh!

jonatack commented Jul 24, 2021

Uh oh!

klementtan commented Jul 25, 2021

Uh oh!

practicalswift commented Jul 25, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Zero-1729 left a comment • 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!

jonatack commented Jul 27, 2021

Uh oh!

jarolrod commented Jul 28, 2021

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Zero-1729 left a comment • 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!

jonatack commented Aug 13, 2021

Uh oh!

Zero-1729 left a comment

Choose a reason for hiding this comment

Uh oh!

klementtan commented Aug 16, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Aug 22, 2021

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Aug 26, 2021

Uh oh!

0xB10C commented Aug 30, 2021

Uh oh!

sipa commented Sep 2, 2021

Uh oh!

Reviewers

Assignees

jonatack commented Jul 19, 2021 •

edited

Loading

lsilva01 left a comment •

edited

Loading

jonatack commented Jul 20, 2021 •

edited

Loading

Zero-1729 left a comment •

edited

Loading

jonatack commented Aug 10, 2021 •

edited

Loading

Zero-1729 left a comment •

edited

Loading