cli: Add progress bar for -getinfo #22547

klementtan · 2021-07-25T16:28:53Z

Add a progress bar for the Verification progress attribute in -getinfo when verification progress < 99%.

Motivation:

Improve user-friendliness of -getinfo
Can be useful with watch -n 1 bitcoin-cli -getinfo(suggested by theStack below)
The progress bar is only display when are still syncing to tip(verification progress < 99%)

Reviewing

If your verification progress is > 99% you can restart the verification progress with

$ ./src/bitcoind -reindex
$./src/bitcoin-cli -getinfo

klementtan · 2021-07-25T16:29:33Z

Also address #17314

Alternative, more human-friendly, format besides JSON? Colors, progress bars 😄?

hebasto · 2021-07-25T16:32:32Z

Motivation: Improve user-friendliness of -getinfo

Not sure if bitcoin-cli is dedicated to casual users.

klementtan · 2021-07-25T18:31:09Z

Not sure if bitcoin-cli is dedicated to casual users.

@hebasto thanks for the feedback! I can see where you are coming from but from #17314 I was under the impression that we want to make bitcoin-cli more friendly to casual users. Will be happy to close this PR if this additional feature is deemed unnecessary.

Now that getinfo is no longer part of the RPC API, and client-side implementation is not subject to API stability, it's possible to start thinking about making it more user friendly.

ghost · 2021-07-25T20:41:19Z

Concept ACK. Will try today.

src/bitcoin-cli.cpp

jarolrod

~~I'm weak NACK on this.~~
I'm concept 0 on this

First, thanks for all your work on improving -getinfo 🥃

I think a good way to think about command line tools is how they fit into scripting work flows. We shouldn't add elements that make it harder for someone to extract the needed information as part of their workflow.

The progress bar introduced here is an example of an element that makes it slightly more difficult to extract the needed information; The verification progress value in this case.

Additionally, it is my opinion that the inclusion of this progress bar adds very little value. It doesn't give me any better insight into the verification progress than the number value does.

theStack · 2021-07-27T12:55:51Z

Concept ACK

Since "getinfo is no longer part of the RPC API, and client-side implementation is not subject to API stability" (quoting the description in #17314) , its output is meant only to be consumed by humans ("the eye"), not for machines and hence using it ever in a script would obviously be a bad idea that shouldn't be encouraged. For those purposes we have the RPC API.

I think CLI users deserves to have a nice looking output as well, I can imagine it's nice to see the verification progress also visually via watch -n 1 bitcoin-cli -getinfo :) that said, I agree of course that this PR is absolutely not high priority.

jonatack · 2021-07-27T12:56:51Z

No opinion yet about this change. Just mentioning that there is a distinction between client-side CLI commands and the server-side RPC API. IIRC part of the point of the client-side CLI, along with aggregating multiple RPC calls in some cases, is to be human-friendly and freer of API constraints. We warn about software depending on the client-side CLI (for stability it should use the RPC API instead), for instance ./src/bitcoin-cli -netinfo help states at the top: This human-readable interface will change regularly and is not intended to be a stable API. Now, -getinfo is IIRC the oldest CLI command, as indeed it previously was an RPC, but when it became a client-side command and now has been fully revamped in presentation, anyone who was depending on scraping it with a script may have received the message that it's better to use the RPC API instead.

jarolrod · 2021-07-27T15:33:44Z

Thanks @theStack @jonatack, retracted my NACK

Zero-1729 · 2021-07-27T18:36:15Z

Concept ACK, nice UX addition for the CLI.

Especially neat with watch -n 1 bitcoin-cli -getinfo as @theStack mentioned above.

jonatack · 2021-07-28T08:56:33Z

Ok, I tested and TBH find it distracting, somewhat blinding with the large patch of white, and inaccurate. It just adds noise here. Might make more sense in the GUI. If you do keep it, maybe a muted grey instead and not displayed if > 99%.

klementtan · 2021-07-28T14:08:55Z

Thanks, reviewers for the feedback and insights!

It seems there is a mixed response to this feature. I will leave this PR open for a few more days to get more feedback and close it afterward if there is still no strong demand for it.

If you do keep it, maybe a muted grey instead and not displayed if > 99%.

@jonatack thanks for the suggestion! Implemented it in e75ee3e to a94d991

Progress bar will only be displayed when verification progress < 99%
Change filled bar to make it less blinding

ghost · 2021-07-28T16:10:30Z

Progress bar will only be displayed when verification progress < 99%
Change filled bar to make it less blinding

No progress bar for >= 99% verification makes sense.

-\u2588
+\u2592

New Unicode for COMPLETE BAR looks good.

Tested with command: watch -c -t -n 1 bitcoin-cli -getinfo (Don't see colors using watch but the progress bar and other things LGTM.

tACK a94d991

theStack

Tested ACK a94d99199f17c667ba76fdb77c48ce6136afa81f 📊

Agree that the new (in)complete characters look way better and are less blinding, also displaying the bar only on <99% progress makes sense.

klementtan · 2021-07-31T10:50:47Z

@prayank23 Thanks for testing it! I think you're not seeing colour because your stdout is not connected to the terminal. You can get colour if you set -color=always

jonatack · 2021-07-31T17:27:00Z

This seems to be a nice improvement over the last version. Some code review feedback here: jonatack@b640049

Zero-1729 · 2021-07-31T18:12:42Z

tACK a94d99199f17c667ba76fdb77c48ce6136afa81f

Nice update. However, @klementtan I'd strongly suggest adding @jonatack's suggestions. For example, I just noticed the lack of space between the progress bar and the percentage value. Additionally, I think the other suggestions there would greatly help readability and simplify future code reviews.

Note: I also wasn't getting color from watch --color -n 1 so I prefixed bitcoin-cli -getinfo with unbuffer (from the expect package) to get it working.

jonatack

almost-ACK

src/bitcoin-cli.cpp

ghost · 2021-08-01T19:53:40Z

crACK 71c1556. Will test this week.

Changes that I see since last review:

Changed GetProgressBar() function from static to void. Reason is explained by @jonatack in this comment

-static std::string GetProgressBar(const double progress)
+void GetProgressBar(double progress, std::string& progress_bar)

-return progress_bar;

It is better to stop executing if progress is less than zero(min) or greater than 1(max) instead of printing empty string.

-if (progress < 0 || progress > 1) return "";
+if (progress < 0 || progress > 1) return;

Makes sense because std::string initializes an empty string ("") by default. In few other languages, devs prefer to initialize it to avoid null exceptions.

-std::string progress_bar = "";
+std::string ibd_progress_bar;

According to developer notes ++i should be used.

-for (int i = 0; i < progress / INCREMENT; i++) {
+for (int i = 0; i < progress / INCREMENT; ++i) {

-for (int i = 0; i < (1 - progress) / INCREMENT; i++) {
+for (int i = 0; i < (1 - progress) / INCREMENT; ++i) {

++i increments then return
i++ returns then increment

I had recently read one post on reddit and found it interesting because of 2 reasons:

a. Helped me understand different operators
b. This comment: i -=- 1

Add padding between progress bar and IBD progress

-verification_progress_bar = GetProgressBar(verification_progress);
+GetProgressBar(ibd_progress, ibd_progress_bar);
+ibd_progress_bar += " ";

klementtan · 2021-08-01T20:14:00Z

a94d991 ... dba61d0

Added whitespace between the progress bar and percentage
Used suggestions in jonatack/bitcoin@b640049

Note: I also wasn't getting color from watch --color -n 1 so I prefixed bitcoin-cli -getinfo with unbuffer (from the expect package) to get it working.

@Zero-1729 Thanks for testing! I think you will need to use watch -c -t -n 1 ./src/bitcoin-cli -signet -color=always -getinfo. Currently -getinfo will not print color if the stdout is not connected to a terminal(watch).

Zero-1729

re-tACK dba61d0cd1226fedaef194b6851c34674fda4bd6 🧪

LGTM, great work @klementtan!

Note: Yeah, appending -color=always to bitcoin-cli did the trick, no need for unbuffer again.

ghost · 2021-08-02T02:18:59Z

@prayank23 Thanks for testing it! I think you're not seeing colour because your stdout is not connected to the terminal. You can get colour if you set -color=always

Yes this works. I forgot the option that you added in last PR.

watch -c -t -n 1 bitcoin-cli -getinfo color=always

progress >= 0.99

progress < 0.99

ghost

tACK dba61d0

theStack

Some nits after the latest change:

there is no reason to remove the static attribute from GetProgressBar; the function still only needs to be visible in the same compilation unit
the new interface implictely assumes now that the passed string is already empty, as it only appends to progress_bar, but never initializes (could either clear the string in the beginning or assert that it is empty?)
I don't think returning a string by value necessarily leads to a copy -- this is a typical case where move semantics potentially kick in and a copy is avoided (see e.g. https://stackoverflow.com/questions/48160292/in-c11-does-returning-a-stdstring-in-a-function-move-or-copy-it); there are also techniques like copy elision and (N)RVO (https://www.fluentcpp.com/2016/11/28/return-value-optimizations/), though I'm not sure if we have any guarantees here with C++17; maybe someone deeper in C++ can shed some light here

Not any blockers though. Will re-test later.

jonatack · 2021-08-02T15:26:45Z

there is no reason to remove the static attribute from GetProgressBar; the function still only needs to be visible in the same compilation unit

Agree!

the new interface implictely assumes now that the passed string is already empty, as it only appends to progress_bar

IIUC this seems fine, insofar that one might want to append a progress bar to a result string that already has some content.

I don't think returning a string by value necessarily leads to a copy -- this is a typical case where move semantics potentially kick in and a copy is avoided (see e.g. stackoverflow.com/questions/48160292/in-c11-does-returning-a-stdstring-in-a-function-move-or-copy-it); there are also techniques like copy elision and (N)RVO (fluentcpp.com/2016/11/28/return-value-optimizations), though I'm not sure if we have any guarantees here with C++17; maybe someone deeper in C++ can shed some light here

I hadn't noticed this codebase depending on the compiler for it, though perhaps we already do somewhere.

Co-authored-by: jonatack <[email protected]>

theStack

re-ACK b851a92 🍹
(quickly tested watching verification from 0 to 100% on signet)

klementtan · 2021-08-02T18:57:42Z

dba61d0 to b851a92: Add static attribute to GetProgressBar

ghost · 2021-08-02T21:35:15Z

there is no reason to remove the static attribute from GetProgressBar; the function still only needs to be visible in the same compilation unit

Agree. It can be changed to non-static if required later.

reACK b851a92

Zero-1729

re-tACK b851a92 (re-tested, works as expected 🍾)

It's a clean push 🪄✨

$ git range-diff dba61d0...b851a92

1:  dba61d0cd ! 1:  b851a92c0 cli: Add progress bar for -getinfo
    ...
    -+void GetProgressBar(double progress, std::string& progress_bar)
    ++static void GetProgressBar(double progress, std::string& progress_bar)

jonatack

ACK b851a92

Tested by manually setting progress inside the new function with values between 0 and 1 and also with

$ ./src/bitcoind -signet -reindex -daemon
$ watch -c -t -n 1 ./src/bitcoin-cli -signet -color=always -getinfo

lsilva01

Tested ACK b851a92 on mainnet and signet on Ubuntu 20.04.

Good UX improvement.

shaavan

No strong opinion
Tested b851a92 on Ubuntu 20.04

This PR intends on adding a new progress bar in the output of command ./bitcoin-cli -signet -daemon. By adding new functions that create a string of greyscale Unicode characters to give a perception of the progress bar. The ratio of lighter and darker grey tones of these characters is a function of the ratio of the verification process that has been completed.

Tested the PR by the command bitcoind -signet -reindex followed by ./src/bitcoin-cli -signet -getinfo. The PR is successful in adding the changes it intends to add.

Attaching a screenshot to show the proper working of the progress bar in the result of -getinfo

I neither support nor disagree with the changes intended by this PR. Adding a progress bar does make the result of -getinfo command more eye-pleasing, but it doesn’t feel like an important or crucial addition to the result. As the purpose of bitcoin-cli is to be rather highly efficient than being eye-pleasing. If there is some way of adding the progress bar as a suboption of -getinfo then I am ACKed on that, because then the bitcoin-cli user has the choice to rather display the progress bar or not while calling -getinfo.

b851a92 cli: Add progress bar for -getinfo (klementtan) Pull request description: Add a progress bar for the `Verification progress` attribute in `-getinfo` when verification progress `< 99%`. ![image](https://user-images.githubusercontent.com/49265907/127897458-27d8aaa9-7893-4665-9c40-36389a8d9cbb.png) **Motivation**: * Improve user-friendliness of `-getinfo` * Can be useful with `watch -n 1 bitcoin-cli -getinfo`(suggested by theStack [below](bitcoin#22547 (comment))) * The progress bar is only display when are still syncing to tip(verification progress `< 99%`) **Reviewing** If your verification progress is `> 99%` you can restart the verification progress with ```shell $ ./src/bitcoind -reindex $./src/bitcoin-cli -getinfo ``` ACKs for top commit: prayank23: reACK bitcoin@b851a92 theStack: re-ACK b851a92 🍹 Zero-1729: re-tACK b851a92 (re-tested, works as expected 🍾) jonatack: ACK b851a92 lsilva01: Tested ACK bitcoin@b851a92 on mainnet and signet on Ubuntu 20.04. Tree-SHA512: 2046d812e3c4623c6cc3ed4c24f2daaa92ba12cd181fa21626b782743890c2373be3175cff1441a7ba37295b6d5818368deea90d483959875c22f7ad9b601a20

klementtan · 2021-08-15T09:38:41Z

@ShaMan239 Thanks for the detailed review! 👍

I agree that this feature is definitely not essential but from the feedback of other reviewers, it seems that it would make the user experience of using bitcoin-cli better.

adding the progress bar as a suboption

For now, I am not really inclined towards adding that because it would make -getinfo more complicated than necessary but if there is enough demand for it I would be more than happy to add it.

klementtan force-pushed the getinfo-progressbar branch from ae5c232 to 38cafb0 Compare July 25, 2021 16:48

laanwj added the RPC/REST/ZMQ label Jul 25, 2021

klementtan force-pushed the getinfo-progressbar branch from 38cafb0 to da3603c Compare July 25, 2021 19:33

ghost reviewed Jul 26, 2021

View reviewed changes

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

jarolrod reviewed Jul 26, 2021

View reviewed changes

klementtan force-pushed the getinfo-progressbar branch from da3603c to e75ee3e Compare July 27, 2021 14:31

klementtan force-pushed the getinfo-progressbar branch from e75ee3e to a94d991 Compare July 28, 2021 13:56

theStack approved these changes Jul 30, 2021

View reviewed changes

klementtan force-pushed the getinfo-progressbar branch 2 times, most recently from 029fb9a to 71c1556 Compare August 1, 2021 18:30

jonatack reviewed Aug 1, 2021

View reviewed changes

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

klementtan force-pushed the getinfo-progressbar branch from 71c1556 to dba61d0 Compare August 1, 2021 20:07

Zero-1729 approved these changes Aug 1, 2021

View reviewed changes

ghost approved these changes Aug 2, 2021

View reviewed changes

theStack reviewed Aug 2, 2021

View reviewed changes

cli: Add progress bar for -getinfo

b851a92

Co-authored-by: jonatack <[email protected]>

klementtan force-pushed the getinfo-progressbar branch from dba61d0 to b851a92 Compare August 2, 2021 16:58

theStack approved these changes Aug 2, 2021

View reviewed changes

Zero-1729 approved these changes Aug 2, 2021

View reviewed changes

jonatack reviewed Aug 3, 2021

View reviewed changes

lsilva01 approved these changes Aug 4, 2021

View reviewed changes

shaavan reviewed Aug 6, 2021

View reviewed changes

meshcollider merged commit ce09131 into bitcoin:master Aug 10, 2021

klementtan deleted the getinfo-progressbar branch August 11, 2021 08:33

klementtan mentioned this pull request Aug 12, 2021

Re-thinking bitcoin-cli -getinfo #17314

Closed

ghost mentioned this pull request Sep 5, 2021

Add fee rate distribution in -getinfo #22891

Closed

ghost mentioned this pull request Oct 2, 2021

cli: make -getinfo user friendly bitcoinknots/bitcoin#37

Closed

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

cli: Add progress bar for -getinfo #22547

cli: Add progress bar for -getinfo #22547

Uh oh!

Conversation

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

Uh oh!

Uh oh!

klementtan commented Jul 25, 2021

Uh oh!

hebasto commented Jul 25, 2021

Uh oh!

klementtan commented Jul 25, 2021

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

jarolrod 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!

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

Uh oh!

Uh oh!

jonatack commented Jul 27, 2021

Uh oh!

jarolrod commented Jul 27, 2021

Uh oh!

Zero-1729 commented Jul 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

ghost commented Jul 28, 2021 • edited by ghost 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!

klementtan commented Jul 31, 2021

Uh oh!

jonatack commented Jul 31, 2021

Uh oh!

Zero-1729 commented Jul 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

klementtan commented Aug 1, 2021

Uh oh!

Zero-1729 left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Aug 2, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

klementtan commented Aug 2, 2021

Uh oh!

ghost commented Aug 2, 2021

Uh oh!

klementtan commented Jul 25, 2021 •

edited

Loading

ghost commented Jul 25, 2021 •

edited by ghost

Loading

jarolrod left a comment •

edited

Loading

theStack commented Jul 27, 2021 •

edited

Loading

Zero-1729 commented Jul 27, 2021 •

edited

Loading

jonatack commented Jul 28, 2021 •

edited

Loading

klementtan commented Jul 28, 2021 •

edited

Loading

ghost commented Jul 28, 2021 •

edited by ghost

Loading

Zero-1729 commented Jul 31, 2021 •

edited

Loading

ghost commented Aug 1, 2021 •

edited by ghost

Loading

ghost commented Aug 2, 2021 •

edited by ghost

Loading

jonatack left a comment •

edited

Loading