netinfo: user help and argument parsing improvements #20877

jonatack · 2021-01-07T16:21:35Z

A few updates, some per IRC discussion today at http://www.erisian.com.au/bitcoin-core-dev/log-2021-01-07.html#l-87 with respect to -netinfo:

enable -netinfo help to run without a remote server
warn in -netinfo help that -netinfo is not intended to be a stable API
improve the -netinfo invalid argument error message
make a performance improvement and simplification I noticed after the merge of cli -netinfo peer connections dashboard updates 🎄 ✨ #20764
update the -netinfo help doc following the merge of cli: Treat high detail levels as maximum in -netinfo #21192

How to test manually: 🔬 🧪 📈

check out and build this branch locally; if you need help, don't hesitate to refer to https://jonatack.github.io/articles/how-to-review-pull-requests-in-bitcoin-core#pull-down-the-code-locally or https://jonatack.github.io/articles/how-to-compile-bitcoin-core-and-run-the-tests
while it is compiling, look at the code changes
stop signet (if it is running) with ./src/bitcoin-cli -signet stop
once the build is completed, run ./src/bitcoin-cli -signet -netinfo help
the help should be printed even though the signet server is not running
near the top you should see the new warning, "This human-readable interface will change regularly and is not intended to be a stable API" as well as a bit more description about the integer argument values.
start signet with ./src/bitcoind -signet
test the improved invalid argument error message if you run ./src/bitcoin-cli -signet -netinfo 256 or ./src/bitcoin-cli -signet -netinfo a (valid values are from 0 to 255)
leave review feedback or ACK <commit hash> -- done 🍻

DrahtBot · 2021-01-07T19:51:45Z

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

Conflicts

No conflicts as of last run.

theStack

Concept ACK

Verified that the help shows now without bitcoind running and that bitcoin-cli -netinfo 5 throws an error. 🆗

Being still a git noob, but can anyone guide me on why the move detection with ignoring whitespace in the second commit doesn't work? I tried git show --color-moved-ws=ignore-all-space HEAD^ but no luck, still the whole diff of the helptext is shown. I ask as this could maybe also helpful for other reviewers.

jonatack · 2021-01-07T23:55:27Z

@theStack thanks for having a look. Here is what I used to diff the second commit.

~/.gitconfig

[diff]
  # Detect copies as well as renames
  renames = copies
	colorMoved = dimmed-zebra
	colorMovedWs = allow-indentation-change

command line result

theStack

@jonatack: Thanks, that gave me the missing clue: --color-moved-ws alone doesn't do much, it has to be combined with --color-moved (I should have RTFM(anpage) 😬). Applying that permanently by putting into .gitconfig is of course an interesting approach, will try.

ACK d05b4d57fde74eea71d033f1d0e5435380ad4708 🦘

jonatack · 2021-02-02T13:27:42Z

This has one ACK by @theStack a month ago (thanks!) Anyone else feel like reviewing?

Edit: added step-by-step manual testing info in the PR description.

theStack · 2021-02-02T13:48:26Z

@jonatack: Maybe restarting the CI also helps (the AppVeyor build seems to hang since the beginning)? I could imagine some reviewers only put PRs on their candidate list after they see the green check marks.

jonatack · 2021-02-02T14:08:03Z

@theStack thanks, good idea 🌻

jonatack · 2021-02-02T14:10:43Z

Oh, stopping/restarting doesn't seem to work. I guess I have to rebase.

jonatack · 2021-02-02T14:11:58Z

Rebased for appveyor.

michaelfolkson

ACK d05b4d57fde74eea71d033f1d0e5435380ad4708

Followed instructions to test in PR description. Those instructions could maybe be improved by instructing the restart of the Signet chain before the "test that an invalid integer argument raises" step. But very easy to follow, thanks

src/bitcoin-cli.cpp

jonatack · 2021-02-02T14:20:24Z

Followed instructions to test in PR description. Those instructions could maybe be improved by instructing the restart of the Signet chain before the "test that an invalid integer argument raises" step. But very easy to follow, thanks

Thanks! -- updated the instructions.

jonatack · 2021-02-02T15:54:29Z

Re-pushed at d05b4d57fde74ee to not invalidate the 2 ACKs (thanks!)

jonatack · 2021-02-05T16:51:58Z

Rebased following the merge of #20764 🍰

michaelfolkson · 2021-02-10T10:19:37Z

Re-ACK 965d9fc09872887314df34e1309604bcc79ff0fc

Followed testing instructions and skimmed over code.

src/bitcoin-cli.cpp

pinheadmz

Besides one take-it-or-leave it nit, this is great.

Built branch, tested help with and without bitcoind running. Attempted to call -netinfo with values from 0 through 5 and got expected results including the error. Read the help message and I will definitely not rely on this dashboard's output for any kind of stable application!

Tested on both signet and mainnet, including adding onion peers. Love the dsiplay!

Bitcoin Core v21.99.0-965d9fc09872 - 70016/Satoshi:21.99.0/

<->   type   net  mping   ping send recv  txn  blk  hb age id address                                                        version
out manual onion                 30   29                 0 19 kpgvmscirrdqpekbqjsvw5teanhatztpp2gl6eee4zkowvwfxwenqaid.onion
out   full  ipv4                 43   43                 0 18 188.37.24.190:8333
out   full  ipv4     60  35648   52   48         0       4  3 73.0.216.24:8333                                               70016/Satoshi:0.21.0/
out  block  ipv4     92     92   63    2         0       2 15 52.214.5.250:8333                                              70015/Satoshi:0.20.1/
out   full  ipv4    142   9772   53   50         0       4  0 87.206.227.194:8333                                            70016/Satoshi:0.21.0/
out   full  ipv4    191    191   63   20         0       2 12 54.168.144.78:8333                                             70015/Satoshi:0.18.1(Omni:0.8.0)/
out   full  ipv4    275    275  131    4                 4  1 180.181.159.200:8333                                           70015/Satoshi:0.19.0.1/
out   full  ipv4    321   6821   63   56         0       3  8 103.29.69.57:8333                                              70015/Satoshi:0.18.0(bitcore)/
out manual onion    618    618   65   26         1       3  4 rp7k2go3s5lyj3fnj6zn62ktarlrsft2ohlsxkyd7v3e3idqyptvread.onion 70016/Satoshi:21.99.0(Medea)/
out   full onion    618    618   60   26         0       2 13 d5uy3u2hkczvmzis.onion:8333                                    70015/Satoshi:0.20.1/
                     ms     ms  sec  sec  min  min     min

        ipv4    ipv6   onion   total   block  manual
in         0       0       0       0
out        7       0       3      10       1       2
total      7       0       3      10

Local addresses: n/a

ACK 965d9fc09872887314df34e1309604bcc79ff0fc

Show Signature

pinheadmz's public key is on keybase

jonatack · 2021-02-11T23:06:52Z

Thank you @theStack, @michaelfolkson, and @pinheadmz for the ACKs. Only change in latest push is to improve the error message per suggestion by @pinheadmz:

$ ./src/bitcoin-cli -netinfo foo
error: invalid -netinfo argument: foo
For more information, run: bitcoin-cli -netinfo help      <-- added this line

git diff 965d9fc 41a5ca6

diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index 43d3dc7da5..1c21856457 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -376,7 +376,7 @@ public:
             if (ParseUInt8(args.at(0), &n) && n < 5) {
                 m_details_level = n;
             } else {
-                throw std::runtime_error(strprintf("invalid -netinfo argument: %s", args.at(0)));
+                throw std::runtime_error(strprintf("invalid -netinfo argument: %s\nFor more information, run: bitcoin-cli -netinfo help", args.at(0)));
             }
         }
         UniValue result(UniValue::VARR);

pinheadmz · 2021-02-12T00:41:49Z

Confirmed diff is minimal, LGTM

ACK 41a5ca6a0341d132456408aa6271667e0a74e289

Show Signature

pinheadmz's public key is on keybase

that removes code and particularly this code from the loop of all peers: `m_is_i2p_on |= (network_id == NET_I2P);`

jonatack · 2021-02-17T14:48:11Z

Rebased and updated following the merge of #21192. This had tested ACK by @theStack, @michaelfolkson, and @pinheadmz (thanks!)

michaelfolkson · 2021-02-17T14:51:48Z

Re-ACK 7d3343f

pinheadmz · 2021-02-17T16:21:36Z

Built and tested locally, ran -netinfo with and without server, tried different arguments as integers and invalid arguments (strings) everything returned as expected.

RE-ACK 7d3343f

Show Signature

pinheadmz's public key is on keybase

jonatack mentioned this pull request Jan 7, 2021

doc: add -netinfo help #20829

Merged

DrahtBot mentioned this pull request Jan 7, 2021

cli -netinfo peer connections dashboard updates 🎄 ✨ #20764

Merged

theStack reviewed Jan 7, 2021

View reviewed changes

DrahtBot mentioned this pull request Jan 7, 2021

Extend support for nested commands to bitcoin-cli #20273

Closed

theStack approved these changes Jan 8, 2021

View reviewed changes

jonatack closed this Feb 2, 2021

jonatack reopened this Feb 2, 2021

jonatack force-pushed the netinfo-help-follow-ups branch from d05b4d5 to 1c40378 Compare February 2, 2021 14:12

michaelfolkson reviewed Feb 2, 2021

View reviewed changes

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

jonatack force-pushed the netinfo-help-follow-ups branch from 1c40378 to d05b4d5 Compare February 2, 2021 15:54

DrahtBot added the Needs rebase label Feb 5, 2021

jonatack force-pushed the netinfo-help-follow-ups branch from d05b4d5 to 965d9fc Compare February 5, 2021 16:15

DrahtBot removed the Needs rebase label Feb 5, 2021

pinheadmz reviewed Feb 11, 2021

View reviewed changes

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

pinheadmz approved these changes Feb 11, 2021

View reviewed changes

jonatack force-pushed the netinfo-help-follow-ups branch from 965d9fc to 41a5ca6 Compare February 11, 2021 22:59

DrahtBot mentioned this pull request Feb 16, 2021

cli: Treat high detail levels as maximum in -netinfo #21192

Merged

DrahtBot added the Needs rebase label Feb 17, 2021

jonatack added 5 commits February 17, 2021 14:55

cli: enable -netinfo help to run without a remote server

7afdd72

cli: warn in help that -netinfo is not intended to be a stable API

3732404

cli: improve -netinfo invalid argument error message

6b45ef3

cli: small -netinfo simplification and performance improvement

ef614bb

that removes code and particularly this code from the loop of all peers: `m_is_i2p_on |= (network_id == NET_I2P);`

cli: update -netinfo help doc following the merge of 882ce25

7d3343f

jonatack force-pushed the netinfo-help-follow-ups branch from 41a5ca6 to 7d3343f Compare February 17, 2021 14:10

DrahtBot removed the Needs rebase label Feb 17, 2021

laanwj merged commit cabe637 into bitcoin:master Mar 3, 2021

jonatack deleted the netinfo-help-follow-ups branch March 3, 2021 14:41

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 3, 2021

erge bitcoin#20877: netinfo: user help and argument parsing improvements

ba3bbb7

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

netinfo: user help and argument parsing improvements #20877

netinfo: user help and argument parsing improvements #20877

Uh oh!

Conversation

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

Uh oh!

Uh oh!

DrahtBot commented Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

theStack commented Feb 2, 2021

Uh oh!

jonatack commented Feb 2, 2021

Uh oh!

jonatack commented Feb 2, 2021

Uh oh!

jonatack commented Feb 2, 2021

Uh oh!

michaelfolkson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack commented Feb 2, 2021

Uh oh!

jonatack commented Feb 2, 2021

Uh oh!

jonatack commented Feb 5, 2021

Uh oh!

michaelfolkson commented Feb 10, 2021

Uh oh!

Uh oh!

pinheadmz 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 Feb 11, 2021

Uh oh!

pinheadmz commented Feb 12, 2021

Uh oh!

jonatack commented Feb 17, 2021

Uh oh!

michaelfolkson commented Feb 17, 2021

Uh oh!

pinheadmz commented Feb 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jonatack commented Jan 7, 2021 •

edited

Loading

DrahtBot commented Jan 7, 2021 •

edited

Loading

jonatack commented Jan 7, 2021 •

edited

Loading

jonatack commented Feb 2, 2021 •

edited

Loading

pinheadmz left a comment •

edited

Loading