cli: Improve -getinfo return format #21832

klementtan · 2021-05-02T09:15:41Z

Overview

Description This PR changes the return format of -getinfo

Rationale

make -getinfo more user-friendly
uses less vertical screen space.

Fixes: Issue 17314(Not linking issue to prevent it from closing)

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

Return Format

// Data from getblockchaininfo
Chain: [getblockchaininfo.chain] 
Blocks: [getblockchaininfo.blocks]
Headers: [getblockchaininfo.headers]
Verification progress: [getblockchaininfo.verificationprogress]
Difficulty: [getblockchaininfo.difficulty]

# Data from getnetworkinfo
Network: in [getnetworkinfo.connections_in], out [getnetworkinfo.connections_out], total [getnetworkinfo.connections]
Version: [getnetworkinfo.version]
Time offset (s): [getnetworkinfo.timeoffset]
Proxy: [getnetworkinfo.proxy] // "N/A" if no proxy
Min tx relay fee rate (BTC/kvB): [getnetworkinfo.relayfee]

# Data from getwalletinfo. Will only be present when a single wallet is loaded
Wallet: [getnetworkinfo.walletname] // "" if walletname is empty(default wallet)
Keypool size: [getnetworkinfo.keypoolsize]
Unlocked until: [getnetworkinfo.unlocked_until]
Transaction fee rate (-paytxfee) (BTC/kvB): [getnetworkinfo.paytxfee]

# Data from getbalances. Will only be present when a single wallet is loaded
Balance: [getbalances.mine.trusted]

# Data from listwallets then getbalances for each wallet. Will only be present for multiple wallets loaded
Balances
[getbalances.mine.trusted] [listwallets[i]] // Right justify on balance

Warnings: [getnetworkinfo.warnings]

Coloring

The following lines would be colored to better show the differences between the various -getinfo components. However, this will not apply for WIN32(ANSI code not supported) and users that connect the stdout to a non-terminal process.

Chain: ...: BLUE(\x1B[34m)
Network: ...: GREEN(\x1B[32m)
Wallet: ...: MAGENTA(\x1B[35m)
Balance: ... CYAN(\x1B[36m)
Balances: ... CYAN(\x1B[36m)
Warnings: ... Yellow(\x1B[33m)

Screenshots

No wallet loaded:

Single wallet loaded

Multi wallet loaded

Reviewing Notes

Testing Scenarios

1. No wallet loaded

When there no wallets are loaded(Unload wallet with ./src/bitcoin-cli unloadwallet "YOUR_WALLETNAME"
Test with ./src/bitcoin-cli -getinfo.
Should only display chain and network segment

2. Single wallet loaded

When only a single wallet loaded or -rpcwallet is set(Load wallet with ./src/bitcoin-cli loadwallet "YOUR_WALLETNAME")
Test with ./src/bitcoin-cli -rpcwallet="YOUR_WALLETNAME" -getinfo(Load wallet with ./src/bitcoin-cli loadwallet "YOUR_WALLETNAME")
Should only display chain, network, wallet and balance segment

3. Multiple wallet loaded

When there are multiple wallets loaded(Load wallet with ./src/bitcoin-cli loadwallet "YOUR_WALLETNAME")
Test with ./src/bitcoin-cli -getinfo
Should only display chain, network and balances segment

Implementation

Current Flow

GetinfoRequestHandler is instantiated
ConnectAndCallRPC is called with GetinfoRequestHandler. It returns UniValue::VOBJ result
If -rpcwallet wallet is not set, GetWalletBalances is called with a pointer to result as a parameter. It adds the balances for all the wallets to result

New Flow

GetinfoRequestHandler is instantiated
ConnectAndCallRPC is called with GetinfoRequestHandler. It returns UniValue::VOBJ result
If -rpcwallet wallet is not set, GetWalletBalances is called with a pointer to result as a parameter. It adds the balances for all the wallets to result
New ParseGetInfoResult is called with result as parameter. It converts results type from UniValue::VOBJ to UniValue::VSTR according to the format stated above.

DrahtBot · 2021-05-02T15:14:43Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

common: Disallow calling IsArgSet() on ALLOW_LIST options #17783 by ryanofsky

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot · 2021-05-03T09:31:13Z

🕵️ @jonatack has been requested to review this pull request as specified in the REVIEWERS file.

jonatack · 2021-05-03T10:45:02Z

Concept ACK. Quick test below on signet. Build is clean, only skimmed the code for now.

Ideas:

perhaps try displaying the balances before the wallet names, as the balances have a fixed length
maybe display the connections on one line connections: in 10, out 10, total 20...a good opportunity to save on vertical space, as getinfo was previously pruned down due to its output being considered too long
maybe s/Blockchain/Block chain/ (see https://github.com/bitcoinops/bitcoinops.github.io/blob/master/STYLE.md for a bitcoin writing guide)
add spaces between words like verificationprogress, relayfee, and timeoffset
maybe display "" for the default wallet

klementtan · 2021-05-03T13:46:23Z

@jonatack
Updated the PR according to your comments:

Swap position of wallet and balance and
Prefix balance value with ₿ emoji to remove any ambiguity of what it represents
display the connections on one line
changed Blockchain to Block chain
added space to verification progress, realy fee, time offset and keypool size(Should I also change paytxfee to pay tx fee?)
display "" for default wallet

Multiple wallet loaded

Single wallet loaded

laanwj · 2021-05-04T15:00:21Z

I think this is nice. It looks better, is more user friendly, and at the same time discourages parsing the output in scripts and such (which is good because it is not intended as a stable interface).

src/bitcoin-cli.cpp

klementtan · 2021-05-07T11:22:04Z

Added check to not print ansi colour code if running on WIN32.

laanwj · 2021-05-10T12:55:27Z

Looks good to me now. I think it makes sense to squash these commits before merge, as they incrementally change the same code.

klementtan · 2021-05-10T15:25:47Z

@laanwj Roger that. Squashed all the commits to a single commit.

Not sure why the last CI task is failing tho. Anyone have a clue?

laanwj · 2021-05-12T12:47:57Z

Thanks!

Not sure why the last CI task is failing tho. Anyone have a clue?

Seems to be solved now. We have transient issues with the CI quite often, unfortunately.

jonatack

Quick skim of the code, waiting on the build to test.

src/bitcoin-cli.cpp

jonatack · 2021-05-12T13:24:46Z

Approach ACK

Some questions (not sure they are good ideas!)

The Bitcoin symbols are neat but I'm not sure they are needed...or maybe instead add just one symbol in parentheses after "Balances" in the header, e.g. Balances (B)...or insert a space between them and the balance?

Should the field names be capitalized? e.g. s/verification progress/Verification progress/?

Maybe hoist "chain: signet" to the header and shorten it to chain? e.g. "Chain: signet", thereby removing a line...and if so, should that section be at the top, before the Network section?

klementtan · 2021-05-12T17:50:13Z

Updated the PR to according @jonatack' comments

Some questions (not sure they are good ideas!)
The Bitcoin symbols are neat but I'm not sure they are needed...or maybe instead add just one symbol in parentheses after "Balances" in the header, e.g. Balances (B)...or insert a space between them and the balance?
Should the field names be capitalized? e.g. s/verification progress/Verification progress/?
Maybe hoist "chain: signet" to the header and shorten it to chain? e.g. "Chain: signet", thereby removing a line...and if so, should that section be at the top, before the Network section?

Capitalized all field names. s/verification progress/Verification progress/
Changed unlocked_until to Unlocked until and paytxfee to Pay transaction fee
Changed ₿0.000: wallet to Balances(₿) ... 0.000: wallet
Combined chain: signet to Chain(signet)
Moved Chain to be the top of the response.
Updated cli_get_info_string_to_dict in test to a static method and changed string parsing method.

Single wallet loaded

Multi wallet loaded

jonatack · 2021-05-12T21:05:26Z

Test of f93b021472

What do you think about

s/Balance(₿):/Balance (₿):/ (insert a space)
extend the colors to the full line of Chain: <chain> and Balances (₿)
right-justify the balances to the largest balance, prefixing spaces to the smaller balances (-netinfo in this same file does it for some of the fields using strprintf)
maybe (not sure, might be too cluttered) hoisting the connections to the header: Network: in 17, out 22, total 49
in single-wallet mode, add the name: Wallet: <name>

jonatack · 2021-05-12T21:36:43Z

Oh, and if you need some sBTC (signet coins) for testing, here's a faucet: https://signetfaucet.com (I can send you some too, if you need).

klementtan · 2021-05-13T07:40:34Z

Updated ed7feb4 with the following changes:

Added space before (₿): s/Balance(₿):/Balance (₿):/ and s/Balances(₿):/Balances (₿):/
Combined Connections into Network header: Network: in <in>, out <out>, total <total>
Extended colors for Chain: <chain>, Balances (₿), Balance: (₿), Network: in <in>, out <out>, total <total>, Wallet: <name>
Right justify balances to the largest balance.

Single wallet

Multi wallet

Oh, and if you need some sBTC (signet coins) for testing, here's a faucet: https://signetfaucet.com (I can send you some too, if you need).

@jonatack I hard-coded a fake wallet to simulate and test a large balance wallet but if you don't mind sending me some sBTC to test it without hard-coding a wallet that would be great(address tb1q8rcmyqlphnpsemf8rgxpcwnlf0rvqnuqwhlzwf). Thanks!

jonatack · 2021-05-13T14:27:50Z

Sent you some signet coins (wow, the faucet used to be 10 sBTC and now it's max .01). Thanks for updating! Looking pretty good. I think the balances alignment should based on the decimal point (apologies, I was unclear) which might require padding spaces after the balance as well if there are fewer than 8 digits after the decimal point (I thought there were always 8). And maybe we can omit the : after the balances. WDYT? I'll re-test soon.

klementtan · 2021-05-13T15:45:56Z

Got it! Thanks for sending it to me and replying to me so quickly. My apologies the amount was hardcoded by me and I added fewer decimal places than what it would actually have. Here is the updated screenshot of what it would look like when one wallet contains "longer" balance than others

And maybe we can omit the : after the balances

Yup I agree <balance> <wallet> makes more sense as <balance> does not act as a key/field name.

Edit:

Updated PR to omit :

jonatack

Lightly-tested re-ACK 14cb2e0fe1384f89b4c9ca70751e6676f438b7a5 with suggested changes to the new -color help (feel free to ignore the code suggestions)

diff

@@ -52,6 +52,9 @@ static constexpr int8_t UNKNOWN_NETWORK{-1};
 /** Default number of blocks to generate for RPC generatetoaddress. */
 static const std::string DEFAULT_NBLOCKS = "1";
 
+/** Default -color setting. */
+static const std::string DEFAULT_COLOR_SETTING{"auto"};
+
 static void SetupCliArgs(ArgsManager& argsman)
 {
     SetupHelpOptions(argsman);
@@ -70,6 +73,7 @@ static void SetupCliArgs(ArgsManager& argsman)
     argsman.AddArg("-netinfo", "Get network peer connection information from the remote server. An optional integer argument from 0 to 4 can be passed for different peers listings (default: 0). Pass \"help\" for detailed help documentation.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
 
     SetupChainParamsBaseOptions(argsman);
+    argsman.AddArg("-color=<when>", strprintf("Color setting for CLI output (default: %s). Valid values: always, auto (add color codes when standard output is connected to a terminal and OS is not WIN32), never.", DEFAULT_COLOR_SETTING), ArgsManager::ALLOW_STRING, OptionsCategory::OPTIONS);
     argsman.AddArg("-named", strprintf("Pass named instead of positional arguments (default: %s)", DEFAULT_NAMED), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-rpcclienttimeout=<n>", strprintf("Timeout in seconds during HTTP requests, or 0 for no timeout. (default: %d)", DEFAULT_HTTP_CLIENT_TIMEOUT), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-rpcconnect=<ip>", strprintf("Send commands to node running on <ip> (default: %s)", DEFAULT_RPCCONNECT), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
@@ -82,7 +86,6 @@ static void SetupCliArgs(ArgsManager& argsman)
     argsman.AddArg("-stdin", "Read extra arguments from standard input, one per line until EOF/Ctrl-D (recommended for sensitive information such as passphrases). When combined with -stdinrpcpass, the first line from standard input is used for the RPC password.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-stdinrpcpass", "Read RPC password from standard input as a single line. When combined with -stdin, the first line from standard input is used for the RPC password. When combined with -stdinwalletpassphrase, -stdinrpcpass consumes the first line, and -stdinwalletpassphrase consumes the second.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-stdinwalletpassphrase", "Read wallet passphrase from standard input as a single line. When combined with -stdin, the first line from standard input is used for the wallet passphrase.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
-    argsman.AddArg("-color=<when>", "Enable key information from CLI response to be displayed with color. With \"-color=auto\", bitcoin-cli will only emit color codes when standard output is connected to a terminal and OS is not WIN32. \"when\" can be \"never\", \"always\", or \"auto\"(default).", ArgsManager::ALLOW_STRING, OptionsCategory::OPTIONS);
 }
 
 /** libevent event log callback */
@@ -895,8 +898,7 @@ static void ParseGetInfoResult(UniValue& result)
 #endif
 
     if (gArgs.IsArgSet("-color")) {
-        // Override default should_colorize value if -color is set.
-        std::string color = gArgs.GetArg("-color", "auto");
+        const std::string color{gArgs.GetArg("-color", DEFAULT_COLOR_SETTING)};
         if (color == "always") {
             should_colorize = true;
         } else if (color == "never") {

src/bitcoin-cli.cpp

klementtan · 2021-07-05T13:10:56Z

14cb2e0 to 658ab9c:

Resolved stylying nit
Add validation when invalid value added to -color

$ ./src/bitcoin-cli -signet -color=foo -getinfo
error: Invalid value for -color option. Valid values: always, auto, never.

jonatack

ACK 658ab9caa3423aa7f50ae7bd0ee7ebb50f0114b0 per git diff 14cb2e0 658ab9c and lightly re-tested rebased to master on each chain and with various wallet configs.

New -color option help

$ ./src/bitcoin-cli -h | grep -A1 color
  -color=<when>
       Color setting for CLI output (default: auto). Valid values: always, auto
       (add color codes when standard output is connected to a terminal
       and OS is not WIN32), never.

New error

$ ./src/bitcoin-cli -regtest -color=foo -getinfo
error: Invalid value for -color option. Valid values: always, auto, never.

test/functional/interface_bitcoin_cli.py

jonatack · 2021-07-05T20:08:29Z

re-ACK d525177aba60e996481007989422280f9c191f15 per git diff 658ab9c d525177

Only change is addressing feedback on 2 test logging touch-ups.

src/bitcoin-cli.cpp

klementtan · 2021-07-08T00:58:35Z

658ab9c to 62aaff1: resolved comments from reviewers regarding logging and better handling of -color option 🙏

theStack

re-ACK 62aaff178a665d757b604ee4a90556242c1135d3 🍪

Verified that the following changes since my previous ACK are OK: stricter -color argument handling with a corresponding test, plus minor style changes in the -color help and the test log messages 👌

jonatack · 2021-07-08T10:53:44Z

re-ACK 62aaff178a665d757b604ee4a90556242c1135d3 per git diff d525177 62aaff1

maflcko

Needs rebase

doc/release-notes.md

DrahtBot · 2021-07-21T10:00:35Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

theStack

re-ACK a37e29d

Verified via git range-diff 62aaff17...a37e29d3 that the latest change since my last ACK is only rebase-related.

jonatack · 2021-07-21T12:26:23Z

Congrats @klementtan, well-deserved.

theStack · 2021-07-21T12:35:56Z

This PR seems to be merged, but is still open. Another GitHub problem I guess? 🤔

klementtan force-pushed the master branch from 03080c3 to 58cff5d Compare May 2, 2021 09:28

DrahtBot added the Tests label May 2, 2021

klementtan force-pushed the master branch from 58cff5d to b42a4a3 Compare May 2, 2021 10:34

DrahtBot mentioned this pull request May 2, 2021

rpc: Add a -rpcwaittimeout parameter to limit time spent waiting #21056

Merged

laanwj reviewed May 4, 2021

View reviewed changes

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

klementtan force-pushed the master branch from 38283cc to 7464656 Compare May 10, 2021 15:24

klementtan force-pushed the master branch 2 times, most recently from d96b160 to c271d17 Compare May 12, 2021 06:11

jonatack reviewed May 12, 2021

View reviewed changes

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

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

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

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

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

klementtan force-pushed the master branch from a6b1bf8 to f93b021 Compare May 12, 2021 17:52

klementtan force-pushed the master branch 2 times, most recently from eb03366 to ed7feb4 Compare May 13, 2021 07:35

klementtan force-pushed the master branch from ed7feb4 to 3c866e5 Compare May 13, 2021 16:48

klementtan requested a review from jonatack May 18, 2021 16:45

jonatack reviewed Jun 16, 2021

View reviewed changes

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

DrahtBot mentioned this pull request Jun 17, 2021

common: Disallow calling IsArgSet() on ALLOW_LIST options #17783

Draft

klementtan requested a review from laanwj July 1, 2021 14:32

klementtan force-pushed the master branch from 14cb2e0 to 658ab9c Compare July 5, 2021 13:08

jonatack reviewed Jul 5, 2021

View reviewed changes

test/functional/interface_bitcoin_cli.py Outdated Show resolved Hide resolved

test/functional/interface_bitcoin_cli.py Outdated Show resolved Hide resolved

klementtan force-pushed the master branch 2 times, most recently from 9062edd to d525177 Compare July 5, 2021 18:30

theStack reviewed Jul 8, 2021

View reviewed changes

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

klementtan force-pushed the master branch from d525177 to 62aaff1 Compare July 8, 2021 00:55

theStack approved these changes Jul 8, 2021

View reviewed changes

maflcko added Utils/log/libs and removed Tests labels Jul 21, 2021

maflcko reviewed Jul 21, 2021

View reviewed changes

doc/release-notes.md Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Jul 21, 2021

cli: Implement human readable -getinfo.

a37e29d

klementtan force-pushed the master branch from 62aaff1 to a37e29d Compare July 21, 2021 11:28

theStack approved these changes Jul 21, 2021

View reviewed changes

maflcko merged commit cac38cd into bitcoin:master Jul 21, 2021

klementtan mentioned this pull request Jul 22, 2021

Re-thinking bitcoin-cli -getinfo #17314

Closed

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 23, 2021

Merge bitcoin#21832: cli: Improve -getinfo return format

aa9be1e

ghost mentioned this pull request Oct 2, 2021

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

Closed

jonatack mentioned this pull request Mar 22, 2022

add (none) in -getinfo Warnings: if no warning returned #24632

Merged

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

cli: Improve -getinfo return format #21832

cli: Improve -getinfo return format #21832

Uh oh!

Conversation

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

Uh oh!

Overview

Return Format

Coloring

Screenshots

Reviewing Notes

Testing Scenarios

Implementation

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

DrahtBot commented May 3, 2021

Uh oh!

jonatack commented May 3, 2021

Uh oh!

klementtan commented May 3, 2021

Uh oh!

laanwj commented May 4, 2021

Uh oh!

Uh oh!

klementtan commented May 7, 2021

Uh oh!

laanwj commented May 10, 2021

Uh oh!

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

Uh oh!

Uh oh!

laanwj commented May 12, 2021

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

klementtan commented May 12, 2021

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

jonatack commented May 13, 2021

Uh oh!

klementtan commented May 13, 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!

klementtan commented Jul 5, 2021

Uh oh!

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

Uh oh!

Uh oh!

jonatack commented Jul 5, 2021

Uh oh!

Uh oh!

klementtan commented Jul 8, 2021

Uh oh!

klementtan commented May 2, 2021 •

edited

Loading

DrahtBot commented May 2, 2021 •

edited

Loading

klementtan commented May 10, 2021 •

edited

Loading

jonatack commented May 12, 2021 •

edited

Loading

jonatack commented May 12, 2021 •

edited

Loading

jonatack commented May 12, 2021 •

edited

Loading

klementtan commented May 13, 2021 •

edited

Loading

klementtan commented May 13, 2021 •

edited

Loading

jonatack left a comment •

edited

Loading

theStack commented Jul 21, 2021 •

edited

Loading