netinfo: user help and argument parsing improvements

jonatack commented at 4:21 pm on January 7, 2021: member

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 #20764
update the -netinfo help doc following the merge of #21192

How to test manually: :microscope: :test_tube: :chart_with_upwards_trend:

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

DrahtBot commented at 7:51 pm on January 7, 2021: member

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

Conflicts

No conflicts as of last run.

theStack commented at 8:46 pm on January 7, 2021: member

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 commented at 11:55 pm on January 7, 2021: member

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

~/.gitconfig

0[diff]
1  # Detect copies as well as renames
2  renames = copies
3	colorMoved = dimmed-zebra
4	colorMovedWs = allow-indentation-change

command line result

Screenshot from 2021-01-08 00-40-43

theStack approved

theStack commented at 0:54 am on January 8, 2021: member

@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) :grimacing:). Applying that permanently by putting into .gitconfig is of course an interesting approach, will try.

ACK d05b4d57fde74eea71d033f1d0e5435380ad4708 🦘

jonatack commented at 1:27 pm on February 2, 2021: member

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 commented at 1:48 pm on February 2, 2021: member

@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 closed this on Feb 2, 2021

jonatack reopened this on Feb 2, 2021

jonatack commented at 2:08 pm on February 2, 2021: member

@theStack thanks, good idea :sunflower:

in src/bitcoin-cli.cpp:318 in d05b4d57fd outdated

308@@ -309,8 +309,7 @@ class NetinfoRequestHandler : public BaseRequestHandler
309         }
310         return UNKNOWN_NETWORK;
311     }
312-    uint8_t m_details_level{0};      //!< Optional user-supplied arg to set dashboard details level
313-    bool m_is_help_requested{false}; //!< Optional user-supplied arg to print help documentation

michaelfolkson commented at 2:09 pm on February 2, 2021:

Why is this bool removed? What was it doing before such that it is no longer needed now?

jonatack commented at 2:19 pm on February 2, 2021:

Yes, it is no longer needed. Before, m_is_help_requested stored if the user passed “help” between PrepareRequest() and ProcessReply() inside the NetinfoRequestHandler class. Now, that logic happens outside of the class at line ~869.

jonatack commented at 2:10 pm on February 2, 2021: member

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

jonatack commented at 2:11 pm on February 2, 2021: member

Rebased for appveyor.

jonatack force-pushed on Feb 2, 2021

michaelfolkson commented at 2:13 pm on February 2, 2021: contributor

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

jonatack commented at 2:20 pm on February 2, 2021: member

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 force-pushed on Feb 2, 2021

jonatack commented at 3:54 pm on February 2, 2021: member

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

DrahtBot added the label Needs rebase on Feb 5, 2021

jonatack force-pushed on Feb 5, 2021

jonatack commented at 4:51 pm on February 5, 2021: member

Rebased following the merge of #20764 :cake:

DrahtBot removed the label Needs rebase on Feb 5, 2021

michaelfolkson commented at 10:19 am on February 10, 2021: contributor

Re-ACK 965d9fc09872887314df34e1309604bcc79ff0fc

Followed testing instructions and skimmed over code.

in src/bitcoin-cli.cpp:379 in 965d9fc098 outdated

377+            if (ParseUInt8(args.at(0), &n) && n < 5) {
378                 m_details_level = n;
379-            } else if (args.at(0) == "help") {
380-                m_is_help_requested = true;
381             } else {
382                 throw std::runtime_error(strprintf("invalid -netinfo argument: %s", args.at(0)));

pinheadmz commented at 9:11 pm on February 11, 2021:

optional nit: include valid range (0..4) or something in error?

jonatack commented at 11:00 pm on February 11, 2021:

done, updated to:

0$ ./src/bitcoin-cli -netinfo foo
1error: invalid -netinfo argument: foo
2For more information, run: bitcoin-cli -netinfo help

pinheadmz approved

pinheadmz commented at 9:20 pm on February 11, 2021: member

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!

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

ACK 965d9fc09872887314df34e1309604bcc79ff0fc

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 965d9fc09872887314df34e1309604bcc79ff0fc
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmAlnvoACgkQ5+KYS2KJ
 7yTp6kw//afOJ89VzlHAafYMlwR58qHYiyziMnIeJQnIJULMibA7EEQg480dNyNUu
 8LiZFb6sPSnFaw8nlbEy08XxcTfmwJ4CvqP/DOp29N8iNIXKarPLrHfonOaOTmbAY
 9snTp/UC6frdB0U532CBeY3UgNEjXdQg6XmKMFDevkVm0Gv8eehskEeMPnsDh7+L4
107I3gs88ROZRuK5iDv08R3UD8U90JKpHFnJ4oVaI1xZGekvuG0rJYF1TzhqiTdGVH
11NwCUfy1jqTQvTEAAabeh5oMGVSxxGlVj8Vfd9wPyHuk6nFUZ7J/rgeIajitD7rhQ
12JX3xvW5rLcky+lFXcEZm9tRh2/znZGg286ImP+E1TWACmbePV1pX5kH5x9Uy0rMP
13qguCksUh1xAnuXxt265nOaX9KiK1CW70hVynxvAc0UDWph+k5hElZpq8S5vLgqFF
14qFz22B3GhaQAJ3k8oi1pEIVRJOaWyhhf1Keq+tvwbIuxf7SIOP+lWIctNQ4LkaCK
1585P78/EIraEgBRtDuFxV6ljBa0xk1xaX+CbxFLsdT4fZew8kWnXlzoBMx0uhk4Dx
167Z6fY7XJuwr83O8gPER22LP0DRw+GFIyvSeGJTx2lPkKuHsgumxOSlUGa3WWgNiE
17C2FBqoGD8ya5Ty2Aio5cH/h/W0aMWFDToPvk/+8lrHzfDh7wclw=
18=LaQW
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

jonatack force-pushed on Feb 11, 2021

jonatack commented at 11:06 pm on February 11, 2021: member

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

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

git diff 965d9fc 41a5ca6

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

pinheadmz commented at 0:41 am on February 12, 2021: member

Confirmed diff is minimal, LGTM

ACK 41a5ca6a0341d132456408aa6271667e0a74e289

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 41a5ca6a0341d132456408aa6271667e0a74e289
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmAlzrYACgkQ5+KYS2KJ
 7yTr5gw/+KxPNflTAOaMuG2MNcmRsWFfznO93T8WXHyB7nO0ejCil7DuPViVDVmK1
 8nARGht0n9qXs+gQKwGphnWu6GxTuLL+lHfbEXUH63hgMgZYl9xWipL/cVLywtu2g
 9AUwJI7S0lOHV7H/cxzmWmlT12h1Jr1qGIwLUoib48LrcAw1bgJ3r1llRYRsEJ1fu
10heZHgXvpB45Y/ZSChyzAe3w1ABD/ylsrkubilas7WYiFAkL7qRBOBw77HiFcUqjh
11z12HkoSbzsW30u5ddDhGg9smeUBzZzI6frJfn4FiFIQP+UukR+BZmqhKtixcpHfY
127kvA5q2YISxkcdmdth6SjEsWUcinfi4Q1AnKkvZ7BNwH+g4OKRZlq4DbHYzMzTGv
13GtGJvo66BR05dysjuf1fXVLuoxmksQaYKN3G71QwvkI3yvgUw4SU4hBZwZwcoZqe
14g5CncCcLpG8uCWN9VARG6JB0sMh8/n+ASC1MjJZwhwXEGxemdAnLwwPHuBM0005X
15G86K/5eVoxMbJEGUlsQkwCoN35MMZ6jogAgsaDC7tRUoOaPPreoKIu1CLfVF8GlO
16ioRQ9aF7A2amfSQBxBMieSxnOjuQisBO1CJavTl88nF5nRwC7Eqz2xckvk7GCeh+
170f63HqOZuXXIpV900twIBQv27ckZM0zdCHgOAJAh4TK1ncpqGzU=
18=hGOF
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

DrahtBot added the label Needs rebase on Feb 17, 2021

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

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

cli: improve -netinfo invalid argument error message 6b45ef3233

cli: small -netinfo simplification and performance improvement

that removes code and particularly this code from the loop of all peers:

`m_is_i2p_on |= (network_id == NET_I2P);`

ef614bb408

cli: update -netinfo help doc following the merge of 882ce251 7d3343fb8e

jonatack force-pushed on Feb 17, 2021

jonatack commented at 2:48 pm on February 17, 2021: member

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

michaelfolkson commented at 2:51 pm on February 17, 2021: contributor

Re-ACK 7d3343fb8e775527e6afc2b183a62ad76e62347e

DrahtBot removed the label Needs rebase on Feb 17, 2021

pinheadmz commented at 4:21 pm on February 17, 2021: member

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 7d3343fb8e775527e6afc2b183a62ad76e62347e

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 7d3343fb8e775527e6afc2b183a62ad76e62347e
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmAtQkUACgkQ5+KYS2KJ
 7yTpxlA//eUHLA/V4il2vPwrA5rBvwkL97kT6I4MHSoJYF4kx+27tZORPQ/1l/1OO
 8ybzrZoazhY2XRZ3vrc+AOEyI1l6d+DPElczCvdE4in3Q6fPogLd0/Dg+EBiyuUPO
 99yAVIOcGTemSQz7Xvf7lQvpFqrrcHc+Jami/PTLYf8UjnaVfK5cHSyyu+WyJzG9H
10P4iuFYm1Q5dnbTRzRHCQEuyzG2iO0OVHmx5ryEorgT+BbZKREk1pwD4BEDceH/CH
11LpahEC4OQPT+GUhK7mto3OPzY+O8A7ZA2A8mcZXDXgMBr+TignOwIg9Zd7IvvGWl
12dYkmyBpz18JAxQF4SFFoYkdAQI7cvkcnRnsdokAndv1h8G1VFq1FiGLG/UJL1hZJ
13dRcA8Zptfr7AuXfO/HJukVKr+jZZUtDgeeXvutdJ5p9bLzqPIaU/vCgBg3fEUZiG
14pRcXqEjgN4hI3/3uYSEBbYzprdUzR83vuoQLZ1+wsXbSSCkkqblewQRcA3OF3sl1
15rhQSZvOupy99w6kQBmrIs0prELQX2fDqcD0rl+iI1VyZwH/o6d6cGDxWUrRiPHCR
16phwu2IFjnQ67XoonAEGoQ26SKyzzPO/k/B/Nt6nojna3ZiQ/EYd4scdlTbcM69mG
17/CY9ueyK/8yUla0iELYByBUXLLLRP3OO+KbGL4Yghs0f/LVu2e8=
18=uXWM
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

laanwj merged this on Mar 3, 2021

laanwj closed this on Mar 3, 2021

jonatack deleted the branch on Mar 3, 2021

sidhujag referenced this in commit ba3bbb7519 on Mar 3, 2021

DrahtBot locked this on Aug 16, 2022

netinfo: user help and argument parsing improvements #20877

Conflicts