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

pull jonatack wants to merge 4 commits into bitcoin:master from jonatack:netinfo-addr-statistics changing 1 files +31 −10
  1. jonatack commented at 3:01 pm on July 19, 2021: member

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

    0$ ./src/bitcoin-cli -netinfo help
1
2  txn      Time since last novel transaction received from the peer and accepted into our mempool, in minutes
3           "*" - the peer requested we not relay transactions to it (relaytxes is false)
4
5  addrp    Total number of addresses processed, excluding those dropped due to rate limiting
6           "." - we do not relay addresses to this peer (addr_relay_enabled is false)
7
8  addrl    Total number of addresses dropped due to rate limiting

    Screenshot from 2021-08-22 14-31-40

  2. lsilva01 approved
  3. lsilva01 commented at 5:57 pm on July 19, 2021: contributor
    Tested ACK https://github.com/bitcoin/bitcoin/pull/22501/commits/a5cc9e28f9e125a62168dcbe0b3484d098e56c1f on mainnet on Ubuntu 20.04.
  4. fanquake commented at 2:52 am on July 20, 2021: member

    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.

  5. laanwj added the label RPC/REST/ZMQ on Jul 20, 2021
  6. laanwj added the label Feature on Jul 20, 2021
  7. jonatack force-pushed on Jul 20, 2021
  8. jonatack commented at 1:30 pm on July 20, 2021: member

    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.

  9. jonatack commented at 1:32 pm on July 20, 2021: member

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

    0diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
1index 4c3cc22dfc..5a0371e3de 100644
2--- a/src/bitcoin-cli.cpp
3+++ b/src/bitcoin-cli.cpp
4@@ -592,7 +592,7 @@ public:
5         "                                  2 - Like 1 but with an address column\n"
6         "                                  3 - Like 1 but with a version column\n"
7         "                                  4 - Like 1 but with both address and version columns\n"
8-        "                                  5 - Like 4 but with additional statistics in the peers listing (addrl, addrp)\n"
9+        "                                  5 - Like 4 but with additional data for development and testing purposes\n"
  10. klementtan approved
  11. klementtan commented at 1:14 am on July 22, 2021: contributor

    Code Review and tested ACK a52c94cb8bf149116dd87e280d1306c72a428d3d

    image

  12. in src/bitcoin-cli.cpp:595 in a52c94cb8b outdated 
    592         "                                  0 - Connection counts and local addresses\n"
593         "                                  1 - Like 0 but with a peers listing (without address or version columns)\n"
594         "                                  2 - Like 1 but with an address column\n"
595         "                                  3 - Like 1 but with a version column\n"
596         "                                  4 - Like 1 but with both address and version columns\n"
597+        "                                  5 - Like 4 but with additional data for development and testing purposes\n"
    jarolrod commented at 3:30 am on July 22, 2021:

    The help messages for the other levels give you insight into what you’ll get before you use the level. This new help message for level 5 is kind of like a mystery box for users who are not familiar with the netinfo code – Look inside for additional data 🎁

    At this level we are introducing two new data fields addrl and addrp which are related to addresses and rate-limiting. I think at minimum we could just say that the additional data is address data, now it’s not such a mystery :)

    0        "                                  5 - Like 4 but with additional address data for development and testing purposes\n"
    jonatack commented at 3:27 pm on July 24, 2021:
    Dropped the additional level (thanks!)
  13. in src/bitcoin-cli.cpp:630 in a52c94cb8b outdated 
    617@@ -604,6 +618,8 @@ class NetinfoRequestHandler : public BaseRequestHandler
618         "           \".\" (to)   - we selected the peer as a high-bandwidth peer\n"
619         "           \"*\" (from) - the peer selected us as a high-bandwidth peer\n"
620         "  age      Duration of connection to the peer, in minutes\n"
621+        "  addrl    Total number of addresses dropped due to rate limiting\n"
    jarolrod commented at 3:30 am on July 22, 2021:

    because 39343849904ea53bb3e9c66521cfccdfd602f46e introduces the new fields and 22fa9112cecb3cfe8a299b7bba76267282e6d76f puts them to proper use by moving them into their new proper level, its kind of like ‘introduce then use’.

    Total non-blocker, but I think you could combine the first two commits into something like:

    cli: introduce netinfo level 5 with addr_processed and addr_rate_limited stats

    jonatack commented at 3:27 pm on July 24, 2021:
    Dropped the additional level (thanks!)
  14. jarolrod commented at 3:31 am on July 22, 2021: member

    ACK a52c94cb8bf149116dd87e280d1306c72a428d3d

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

    Notes on commits:

    • 39343849904ea53bb3e9c66521cfccdfd602f46e
      • 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
    • 22fa9112cecb3cfe8a299b7bba76267282e6d76f
    • f92bce8bf47c2bef63b365bcdff1de8225bcea9b
      • code review ack
    • a52c94cb8bf149116dd87e280d1306c72a428d3d
      • code review ack
  15. RandyMcMillan commented at 3:50 am on July 22, 2021: contributor

    tACK a52c94c

    macOS 11.4

    Screen Shot 2021-07-21 at 10 53 52 PM

    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”

    Screen Shot 2021-07-21 at 11 26 56 PM

    One request:

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

  16. sipa commented at 5:12 am on July 22, 2021: member
    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.
  17. jonatack force-pushed on Jul 24, 2021
  18. jonatack commented at 3:35 pm on July 24, 2021: member

    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

  19. jonatack renamed this:
    cli: add new address statistics to -netinfo
    cli: add getpeerinfo addr_{processed, rate_limited} data to -netinfo
    on Jul 24, 2021
  20. klementtan commented at 4:39 pm on July 25, 2021: contributor

    code review and tested reACK 936d8ff49beeac5e0c77a87618e021b09d55c7a1

    image

  21. MarcoFalke deleted a comment on Jul 25, 2021
  22. MarcoFalke removed the label RPC/REST/ZMQ on Jul 25, 2021
  23. MarcoFalke added the label Utils/log/libs on Jul 25, 2021
  24. practicalswift commented at 8:29 pm on July 25, 2021: contributor

    Concept ACK

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

  25. in src/bitcoin-cli.cpp:545 in ded9ba1b8e outdated 
    535@@ -528,6 +536,10 @@ class NetinfoRequestHandler : public BaseRequestHandler
536                     strprintf("%s%s", peer.is_bip152_hb_to ? "." : " ", peer.is_bip152_hb_from ? "*" : " "),
537                     m_max_age_length, // variable spacing
538                     peer.age,
539+                    m_max_addr_rate_limited_length, // variable spacing
540+                    peer.addr_rate_limited ? ToString(peer.addr_rate_limited) : "",
    vasild commented at 4:03 pm on July 27, 2021:

    Wouldn’t it be better to print 0 if the value is zero, instead of an empty string?

    0                    peer.addr_rate_limited,
    jonatack commented at 7:24 pm on July 27, 2021:

    Yes, that was done to optimise signal to noise. Examples follow; keep in mind that they are of much smaller nodes than typical longer-running ones that can have 125-250 peer connections.

    Screenshot from 2021-07-27 19-24-53

    Screenshot from 2021-07-27 19-24-08

  26. in src/bitcoin-cli.cpp:537 in 936d8ff49b outdated 
    533                     PingTimeToString(peer.ping),
534-                    peer.last_send == 0 ? "" : ToString(m_time_now - peer.last_send),
535-                    peer.last_recv == 0 ? "" : ToString(m_time_now - peer.last_recv),
536-                    peer.last_trxn == 0 ? "" : ToString((m_time_now - peer.last_trxn) / 60),
537-                    peer.last_blck == 0 ? "" : ToString((m_time_now - peer.last_blck) / 60),
538+                    peer.last_send ? ToString(m_time_now - peer.last_send) : "",
    vasild commented at 4:08 pm on July 27, 2021:

    nit: I guess it is a matter of personal taste, but I prefer to avoid treating integers as booleans. Somehow I think that the following convey more information: if (a == 0) / if (a == nullptr) / if (a == '\0') compared to just if (a) which I use only if a is bool indeed.

    Feel free to ignore.

    jonatack commented at 10:27 am on August 10, 2021:
    I agree there can be places where it’s good to be explicit/verbose with integer-to-boolean conversions, but they are well-defined behavior and the -netinfo class uses them extensively throughout (to good advantage IMO) to simplify the code.
  27. vasild approved
  28. vasild commented at 4:11 pm on July 27, 2021: member
    ACK 936d8ff49beeac5e0c77a87618e021b09d55c7a1
  29. Zero-1729 approved
  30. Zero-1729 commented at 6:17 pm on July 27, 2021: contributor

    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:

  31. jonatack commented at 7:27 pm on July 27, 2021: member
    Thanks @Zero-1729! A propos displaying zeroes, see #22501 (review).
  32. jarolrod commented at 3:04 am on July 28, 2021: member
    tACK 936d8ff49beeac5e0c77a87618e021b09d55c7a1
  33. Add addr_processed and addr_rate_limited stats to -netinfo 5eeea8e257
  34. Simplify a few conditionals in -netinfo 0a9ee3a2c7
  35. jonatack force-pushed on Aug 10, 2021
  36. jonatack commented at 10:36 am on August 10, 2021: member

    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
  37. jonatack renamed this:
    cli: add getpeerinfo addr_{processed, rate_limited} data to -netinfo
    netinfo: display addr_{processed, rate_limited, relay_enabled} and relaytxes data
    on Aug 10, 2021
  38. in src/bitcoin-cli.cpp:630 in 1df955273a outdated 
    626@@ -610,10 +627,14 @@ class NetinfoRequestHandler : public BaseRequestHandler
627         "  send     Time since last message sent to the peer, in seconds\n"
628         "  recv     Time since last message received from the peer, in seconds\n"
629         "  txn      Time since last novel transaction received from the peer and accepted into our mempool, in minutes\n"
630+        "           \".\" - the peer requested we not relay transactions to it (relaytxes is false)\n"
    Zero-1729 commented at 4:08 am on August 13, 2021:
    Minor nit: the hb column currently uses the ‘.’ and ‘*’ convention to indicate details regarding our preference and the remote peer’s respectively. The processed addresses column (addrp) also maintains this convention. I would suggest txn keep that consistency by using ‘*’ instead of ‘.’ to indicate the remote peer’s request regarding us relaying txs to them. @jonatack what do you think?
    jonatack commented at 10:16 am on August 13, 2021:
    Good point. I have been asking myself the same but hesitated because the * seemed a bit “loud” in the txn column. I agree that it’s better that the logic be coherent. Done, thanks for the thoughtful feedback!
  39. Zero-1729 approved
  40. Zero-1729 commented at 4:09 am on August 13, 2021: contributor

    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.

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

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

  41. Display peers in -netinfo that request we not relay transactions 
    i.e. peers where getpeerinfo#relaytxes is false
    3834e23b25
  42. Display peers in -netinfo that we don't relay addresses to 
    i.e. peers where getpeerinfo#addr_relay_enabled is false
    218862a018
  43. jonatack force-pushed on Aug 13, 2021
  44. jonatack commented at 10:32 am on August 13, 2021: member

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

     0diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
 1index f960737bfd..718ad5dcef 100644
 2--- a/src/bitcoin-cli.cpp
 3+++ b/src/bitcoin-cli.cpp
 4@@ -536,7 +536,7 @@ public:
 5                     PingTimeToString(peer.ping),
 6                     peer.last_send ? ToString(m_time_now - peer.last_send) : "",
 7                     peer.last_recv ? ToString(m_time_now - peer.last_recv) : "",
 8-                    peer.last_trxn ? ToString((m_time_now - peer.last_trxn) / 60) : peer.is_block_relay ? "." : "",
 9+                    peer.last_trxn ? ToString((m_time_now - peer.last_trxn) / 60) : peer.is_block_relay ? "*" : "",
10                     peer.last_blck ? ToString((m_time_now - peer.last_blck) / 60) : "",
11                     strprintf("%s%s", peer.is_bip152_hb_to ? "." : " ", peer.is_bip152_hb_from ? "*" : " "),
12                     m_max_addr_processed_length, // variable spacing
13@@ -627,7 +627,7 @@ public:
14         "  send     Time since last message sent to the peer, in seconds\n"
15         "  recv     Time since last message received from the peer, in seconds\n"
16         "  txn      Time since last novel transaction received from the peer and accepted into our mempool, in minutes\n"
17-        "           \".\" - the peer requested we not relay transactions to it (relaytxes is false)\n"
18+        "           \"*\" - the peer requested we not relay transactions to it (relaytxes is false)\n"
19         "  blk      Time since last novel block passing initial validity checks received from the peer, in minutes\n"
20         "  hb       High-bandwidth BIP152 compact block relay\n"
21         "           \".\" (to)   - we selected the peer as a high-bandwidth peer\n"
  45. Zero-1729 approved
  46. Zero-1729 commented at 11:11 am on August 13, 2021: contributor

    re-tACK 218862a01848f69d54380c780bb5eae6dfdb1416

    Tested on macOS v11.4

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

  47. klementtan commented at 2:25 am on August 16, 2021: contributor
    re crAck and tAck 218862a0 image
  48. vasild approved
  49. vasild commented at 11:29 am on August 20, 2021: member
    ACK 218862a01848f69d54380c780bb5eae6dfdb1416
  50. jonatack commented at 12:47 pm on August 22, 2021: member

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

    0<->   type   net  mping   ping send recv  txn  blk  hb addrp addrl  age  asmap   id address                                                             version
1out   full  ipv4    204    214    0    0    0          14958 31870 2334   6830  296 178.200.242.132:8333                                                70016/Satoshi:0.21.1/
  51. jarolrod commented at 3:47 am on August 24, 2021: member

    tACK 218862a01848f69d54380c780bb5eae6dfdb1416

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

    A note on 0a9ee3a2c787e97213a0456b0d6253c549b71e09: 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.

  52. jonatack commented at 8:34 am on August 26, 2021: member
    This has many ACKs, seems ready for merge?
  53. 0xB10C commented at 10:08 am on August 30, 2021: member
    Code review and tested ACK 218862a01848f69d54380c780bb5eae6dfdb1416
  54. sipa commented at 2:40 pm on September 2, 2021: member
    Concept ACK
  55. laanwj merged this on Sep 2, 2021
  56. laanwj closed this on Sep 2, 2021
  57. jonatack deleted the branch on Sep 2, 2021
  58. sidhujag referenced this in commit a3c7ca1355 on Sep 4, 2021
  59. DrahtBot locked this on Sep 2, 2022


jonatack lsilva01 fanquake klementtan jarolrod RandyMcMillan sipa practicalswift vasild Zero-1729 0xB10C

Labels
Feature Utils/log/libs

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-19 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me