klementtan commented at 9:15 AM on May 2, 2021: contributor

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.

klementtan force-pushed on May 2, 2021

DrahtBot added the label Tests on May 2, 2021

klementtan force-pushed on May 2, 2021

DrahtBot commented at 3:14 PM on May 2, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#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 commented at 9:31 AM on May 3, 2021: member

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

jonatack commented at 10:45 AM on May 3, 2021: member

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

Screenshot from 2021-05-03 12-10-02

klementtan commented at 1:46 PM on May 3, 2021: contributor

@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 commented at 3:00 PM on May 4, 2021: member

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).

in src/bitcoin-cli.cpp:877 in 380a6842db outdated

 872 | +{
 873 | +    if (!find_value(result, "error").isNull()) {
 874 | +        return;
 875 | +    }
 876 | +
 877 | +    std::string RESET = "\033[0m";

laanwj commented at 3:11 PM on May 4, 2021:

Are there still issues on windows with colorized text, or can basic ANSI color support be assumed all over the board now? (if not, you might want to put this in #ifndef WIN32, leave the strings empty if not—we do this for some python scripts)

klementtan commented at 3:27 PM on May 4, 2021:

hmmm I do not own a Windows computer and cannot test if ANSI color support works.

(if not, you might want to put this in #ifndef WIN32, leave the strings empty if not—we do this for some python scripts)

Am I right to say you are referring to something like this?

    std::string RESET = "";
    std::string GREEN = "";
    std::string BLUE = "";
    std::string YELLOW = "";
    std::string MAGENTA = "";
    std::string CYAN = "";
#ifndef WIN32
    RESET = "\033[0m";
    GREEN = "\x1B[32m";
    BLUE = "\x1B[34m";
    YELLOW = "\x1b[33m";
    MAGENTA = "\x1B[35m";
    CYAN = "\x1B[36m";
#endif

laanwj commented at 3:39 PM on May 4, 2021:

Yes, for example. I think I would personally do, to make it symmetrical:

#ifndef WIN32
    const std::string RESET = "\x1B[0m";
    const std::string GREEN = "\x1B[32m";
    const std::string BLUE = "\x1B[34m";
    const std::string YELLOW = "\x1B[33m";
    const std::string MAGENTA = "\x1B[35m";
    const std::string CYAN = "\x1B[36m";
#else
    const std::string RESET = "";
    const std::string GREEN = "";
    const std::string BLUE = "";
    const std::string YELLOW = "";
    const std::string MAGENTA = "";
    const std::string CYAN = "";
#endif

klementtan commented at 4:13 PM on May 4, 2021:

@laanwj Yeah you're right that looks better 👍. Updated the PR to add WIN32 check.

klementtan commented at 11:22 AM on May 7, 2021: contributor

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

laanwj commented at 12:55 PM on May 10, 2021: member

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

klementtan force-pushed on May 10, 2021

klementtan commented at 3:25 PM on May 10, 2021: contributor

@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?

klementtan force-pushed on May 11, 2021

klementtan force-pushed on May 12, 2021

laanwj commented at 12:47 PM on May 12, 2021: member

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.

in src/bitcoin-cli.cpp:892 in c271d17c29 outdated

 867 | @@ -867,6 +868,71 @@ static void GetWalletBalances(UniValue& result)
 868 |      result.pushKV("balances", balances);
 869 |  }
 870 |  
 871 | +static void ParseGetInfoResult(UniValue& result)

jonatack commented at 1:03 PM on May 12, 2021:

Could add Doxygen documentation for this new function.

klementtan commented at 5:51 PM on May 12, 2021:

Added Doxygen documentation for ParseGetInfoResult

jonatack commented at 7:57 AM on May 19, 2021:

Thanks for adding it. Suggestion:

 /**
- * ParseGetInfoResult takes in -getinfo result in UniValue object and parses it into a user friendly UniValue string to be
- * printed on the console.
- * [@param](/bitcoin-bitcoin/contributor/param/) result Reference to a UniValue object that contains all the -getinfo data.
+ * ParseGetInfoResult takes in -getinfo result in UniValue object and parses it
+ * into a user friendly UniValue string to be printed on the console.
+ *
+ * [@param](/bitcoin-bitcoin/contributor/param/)[out] result  Reference to UniValue result containing the -getinfo output.
  */

klementtan commented at 10:11 AM on May 19, 2021:

👍 updated it according to your suggestion.

in src/bitcoin-cli.cpp:925 in c271d17c29 outdated

 920 | +    }
 921 | +
 922 | +    if (!result["balances"].isNull()) {
 923 | +        result_string += strprintf("%sBalances%s\n", CYAN, RESET);
 924 | +
 925 | +        for (std::string wallet : result["balances"].getKeys()) {

jonatack commented at 1:07 PM on May 12, 2021:

Maybe reference to const here rather than copying

        for (const std::string& wallet : result["balances"].getKeys()) {

klementtan commented at 5:54 PM on May 12, 2021:

Updated to use string reference instead of copying

in src/bitcoin-cli.cpp:875 in c271d17c29 outdated

 867 | @@ -867,6 +868,71 @@ static void GetWalletBalances(UniValue& result)
 868 |      result.pushKV("balances", balances);
 869 |  }
 870 |  
 871 | +static void ParseGetInfoResult(UniValue& result)
 872 | +{
 873 | +    if (!find_value(result, "error").isNull()) {
 874 | +        return;
 875 | +    }

jonatack commented at 1:07 PM on May 12, 2021:

style nit, for a guard conditional at the top of the function this seems more compact to me, feel free to ignore

-    if (!find_value(result, "error").isNull()) {
-        return;
-    }
+    if (!find_value(result, "error").isNull()) return;

klementtan commented at 5:54 PM on May 12, 2021:

Followed your suggestion and updated to a single line guard clause

in src/bitcoin-cli.cpp:1108 in c271d17c29 outdated

1056 | @@ -991,6 +1057,11 @@ static int CommandLineRPC(int argc, char *argv[])
1057 |                  if (gArgs.IsArgSet("-getinfo") && !gArgs.IsArgSet("-rpcwallet")) {
1058 |                      GetWalletBalances(result); // fetch multiwallet balances and append to result
1059 |                  }
1060 | +
1061 | +                if (gArgs.IsArgSet("-getinfo")) {
1062 | +                    ParseGetInfoResult(result);
1063 | +                }
1064 | +

jonatack commented at 1:12 PM on May 12, 2021:

nit, maybe combine the two -getinfo checks, feel free to ignore

             if (error.isNull()) {
-                if (gArgs.IsArgSet("-getinfo") && !gArgs.IsArgSet("-rpcwallet")) {
-                    GetWalletBalances(result); // fetch multiwallet balances and append to result
-                }
-
                 if (gArgs.IsArgSet("-getinfo")) {
+                    if (!gArgs.IsArgSet("-rpcwallet")) {
+                        GetWalletBalances(result); // fetch multiwallet balances and append to result
+                    }
                     ParseGetInfoResult(result);
                 }

klementtan commented at 5:55 PM on May 12, 2021:

Followed your suggestion and combined the checks

in src/bitcoin-cli.cpp:1061 in c271d17c29 outdated

1056 | @@ -991,6 +1057,11 @@ static int CommandLineRPC(int argc, char *argv[])
1057 |                  if (gArgs.IsArgSet("-getinfo") && !gArgs.IsArgSet("-rpcwallet")) {
1058 |                      GetWalletBalances(result); // fetch multiwallet balances and append to result
1059 |                  }
1060 | +
1061 | +                if (gArgs.IsArgSet("-getinfo")) {

jonatack commented at 1:14 PM on May 12, 2021:

nit, after using if (gArgs.IsArgSet("-getinfo")) a year ago, I learned later that this does more robust value checking of the user input:

                if (gArgs.GetBoolArg("-getinfo", false)) {

klementtan commented at 5:55 PM on May 12, 2021:

Updated to use GetBoolArg instead of IsArgSet

jonatack commented at 1:17 PM on May 12, 2021: member

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

jonatack commented at 1:24 PM on May 12, 2021: member

Approach ACK

Screenshot from 2021-05-12 15-25-10

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 commented at 5:50 PM on May 12, 2021: contributor

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

klementtan force-pushed on May 12, 2021

jonatack commented at 9:05 PM on May 12, 2021: member

Test of f93b021472

Screenshot from 2021-05-12 22-48-59

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 commented at 9:36 PM on May 12, 2021: member

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 force-pushed on May 13, 2021

klementtan commented at 7:40 AM on May 13, 2021: contributor

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 commented at 2:27 PM on May 13, 2021: member

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 commented at 3:45 PM on May 13, 2021: contributor

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 :

klementtan force-pushed on May 13, 2021

klementtan requested review from jonatack on May 18, 2021

jonatack commented at 5:11 PM on May 18, 2021: member

Will re-test and review very soon.

in src/bitcoin-cli.cpp:927 in 3c866e5d79 outdated

 922 | +            result_string += strprintf("Unlocked until: %s\n", result["unlocked_until"].getValStr());
 923 | +        }
 924 | +        result_string += strprintf("Pay transaction fee: %s\n\n", result["paytxfee"].getValStr());
 925 | +    }
 926 | +    if (!result["balance"].isNull()) {
 927 | +        // \u20BF represents bitcoin emoji

jonatack commented at 8:17 AM on May 19, 2021:

This is clear from the BITCOIN_EMOJI constant, so I think you can drop this line.

klementtan commented at 10:13 AM on May 19, 2021:

Removed the comment

in test/functional/interface_bitcoin_cli.py:103 in 3c866e5d79 outdated

 103 |          assert_raises_process_error(1, "Could not connect to the server", self.nodes[0].cli('-rpcport=1').echo)
 104 |  
 105 |          self.log.info("Test connecting with non-existing RPC cookie file")
 106 | -        assert_raises_process_error(1, "Could not locate RPC credentials", self.nodes[0].cli('-rpccookiefile=does-not-exist', '-rpcpassword=').echo)
 107 | +        assert_raises_process_error(1, "Could not locate RPC credentials",
 108 | +                                    self.nodes[0].cli('-rpccookiefile=does-not-exist', '-rpcpassword=').echo)

jonatack commented at 8:25 AM on May 19, 2021:

The diff in the above 15 lines in the "Test -stdinrpcpass option" section is unrelated formatting changes. I checked that it doesn't change the code, but it's easier and safer to review without them and normally they shouldn't be added in this commit. I think they can probably be omitted.

klementtan commented at 10:14 AM on May 19, 2021:

My apologies for that 😅. Removed the unrelated formatting changes in this PR.

in test/functional/interface_bitcoin_cli.py:65 in 3c866e5d79 outdated

  56 | +            if key == 'Wallet' and value == '""':
  57 | +                # Set default wallet("") to empty string
  58 | +                value = ''
  59 | +            cli_get_info[key.strip()] = value.strip()
  60 | +        line_idx += 1
  61 | +    return cli_get_info

jonatack commented at 8:29 AM on May 19, 2021:

This cli_get_info_string_to_dict() function seems like a lot of workaround, would it be easier to drop this and test the stdout instead? I think we do that in other tests (feature_help.py, tool_wallet.py, etc). I guess it could be done later.

jonatack commented at 8:41 AM on May 19, 2021: member

I think the result is pretty good now, thanks for working on the suggestions! A few code review comments below and then I think it might be ready.

ACK 3c866e5d79519f70fedd04e03683ab51e754e9ab modulo the comments below

klementtan force-pushed on May 19, 2021

jonatack commented at 10:32 AM on May 19, 2021: member

ACK 6015d1d689c78dd5469cfde0963259422eef66df

One nice advantage of this change is that -getinfo would use less vertical screen space.

Screenshot from 2021-05-19 12-28-40

With no wallets loaded:

Screenshot from 2021-05-19 12-38-31

Consider removing or updating the screenshots in the PR description.

Possible follow-ups:

this may need a release note
maybe simplify the tests per #21832 (review)

klementtan commented at 10:42 AM on May 19, 2021: contributor

@jonatack Thanks for taking the time to review the PR and guide me. Will work on refactoring the test to simplify it.

jonatack commented at 10:44 AM on May 19, 2021: member

fixes: #17314

(This mention would auto-close the issue, I think. I don't know if it should be closed.)

in src/bitcoin-cli.cpp:914 in 6015d1d689 outdated

 909 | +        result["connections"]["total"].getValStr(),
 910 | +        RESET);
 911 | +    result_string += strprintf("Version: %s\n", result["version"].getValStr());
 912 | +    result_string += strprintf("Time offset: %s\n", result["timeoffset"].getValStr());
 913 | +    result_string += strprintf("Proxy: %s \n", result["proxy"].getValStr());
 914 | +    result_string += strprintf("Relay fee: %s\n\n", result["relayfee"].getValStr());

kiminuo commented at 8:37 PM on May 19, 2021:

Given that user should undestand this output, would it make sense to add a unit here (BTC/kB? / BTC/kvB?)?

klementtan commented at 1:57 AM on May 20, 2021:

Good suggestion! Updated to BTC/kB as per the unit in the code

in src/bitcoin-cli.cpp:912 in 6015d1d689 outdated

 907 | +        result["connections"]["in"].getValStr(),
 908 | +        result["connections"]["out"].getValStr(),
 909 | +        result["connections"]["total"].getValStr(),
 910 | +        RESET);
 911 | +    result_string += strprintf("Version: %s\n", result["version"].getValStr());
 912 | +    result_string += strprintf("Time offset: %s\n", result["timeoffset"].getValStr());

kiminuo commented at 8:51 PM on May 19, 2021:

Would it make sense to add s (seconds) as a time unit here? (At least I believe it is in seconds.)

klementtan commented at 1:58 AM on May 20, 2021:

Updated to add unit s. Yup you're right it is in seconds(code)

kiminuo commented at 9:04 PM on May 19, 2021: contributor

Concept ACK

Adding screenshots to your original post would be great.

Some thinking out loud: For me it's rather hard to say what keys are in result variable in ParseGetInfoResult. One way to solve that would be to introduce constants for strings like version, blocks, headers, verificationprogress, etc. so that an IDE can tell you where these JSON properties are used. This would help with refactorings too, I guess. The downside is that we would have more code. Feel free to ignore this note. If it resonates with you, it may be done in a follow-up PR. :)

jonatack commented at 10:00 PM on May 19, 2021: member

Some thinking out loud: For me it's rather hard to say what keys are in result variable in ParseGetInfoResult.

See the GetinfoRequestHandler class. I'm not sure the keys need to be hoisted out to a bunch of file-level static constants.

Maybe ParseGetInfoResult() could be a method in the GetinfoRequestHandler class for better encapsulation.

klementtan commented at 1:48 AM on May 20, 2021: contributor

Made the following changes:

Updated s/Time offset/Time offset (s) and s/Relay fee/ Relay fee (BTC/kB)
Update release notes.

Maybe ParseGetInfoResult() could be a method in the GetinfoRequestHandler class for better encapsulation. @jonatack I am not sure how to do that because GetWalletBalances takes in the return result of GetinfoRequestHandler and ParseGetInfoResult can only be called after GetWalletBalances is executed. Might need to refactor GetWalletBalances as well if we would like to encapsulate ParseGetInfoResult.

Will work on refactoring the test to simplify it.

I branch off from this branch in my own repo to implement testing against stdout(PR here) but I am not too sure if it actually simplifies the test because creating the expected stdout is quite complicated. @jonatack can I trouble you to take a look at it when you are free? If you agree with that rough approach I will merge it and you could review it here.

Adding screenshots to your original post would be great. @kiminuo My bad. I misunderstood previous comments and was under the impression that screenshots are not allowed in PR description. Updated OP to include PR description. @kiminuo thanks for reviewing the PR 🙇‍♂️

klementtan force-pushed on May 20, 2021

kiminuo commented at 6:16 AM on May 20, 2021: contributor

Updated s/Time offset/Time offset (s) and s/Relay fee/ Relay fee (BTC/kB)

Update release notes.

Looks good. Thanks for the patch.

"Pay transaction fee" should have a unit too, right?
"Proxy" is empty. Maybe "Proxy: N/A" when there is no proxy? Or hide it altogether when there is no proxy? I have not checked possible values but it feels strage to show "Proxy: <nothing here>".

Good work!

in src/bitcoin-cli.cpp:947 in 52ea193ad9 outdated

 914 | +    result_string += strprintf("Proxy: %s \n", result["proxy"].getValStr());
 915 | +    result_string += strprintf("Relay fee (%\\/kB): %s\n\n", CURRENCY_UNIT, result["relayfee"].getValStr());
 916 | +
 917 | +    if (!result["has_wallet"].isNull()) {
 918 | +        const std::string walletname = result["walletname"].getValStr();
 919 | +        result_string += strprintf("%sWallet: %s%s\n", MAGENTA, walletname.empty() ? "\"\"" : walletname, RESET);

kiminuo commented at 6:26 AM on May 20, 2021:

I think that instead of "", it is more common to use [default wallet].

At least these make me think so:

jonatack commented at 6:50 AM on May 20, 2021:

In the RPC/CLI the default wallet has been "".

kiminuo commented at 7:04 AM on May 20, 2021:

Is that user-friendly? I mean "" looks like an output for programmers and not for users. However, if it is a convention in CLI, then it's better to align with it.

One can possibly write also default wallet ("")

jonatack commented at 7:13 AM on May 20, 2021:

If you need to specify the default wallet in the CLI or RPC, you pass "" or just nothing, e.g.:

$ bitcoin-cli -signet -rpcwallet="" -getinfo
$ bitcoin-cli -signet -rpcwallet=  -getinfo

If you pass something else, it won't be recognized as the default wallet (unless there is some way I'm not aware of).

If you have a wallet named "default wallet" then it seems your suggestion would make things confusing.

kiminuo commented at 8:28 AM on May 20, 2021:

I see, that makes sense. Even though I was not suggesting to pass "default wallet" to a CLI command, just to make it clear that "" means a default wallet in the text output.

Anyway, all is good.

jonatack commented at 6:51 AM on May 20, 2021: member

@jonatack I am not sure how to do that because GetWalletBalances takes in the return result of GetinfoRequestHandler and ParseGetInfoResult can only be called after GetWalletBalances is executed. Might need to refactor GetWalletBalances as well if we would like to encapsulate ParseGetInfoResult.

Makes sense, and this can be in a follow-up.

in doc/release-notes.md:148 in 52ea193ad9 outdated

 140 | @@ -141,6 +141,7 @@ Tools and Utilities
 141 |    useful to see if the node knows enough addresses in a network to use options
 142 |    like `-onlynet=<network>` or to upgrade to current and future Tor releases
 143 |    that support Tor v3 addresses only.  (#21595)
 144 | +- Update `-getinfo` to return data in user-friendly format that also reduces vertical space. (#21832)

jonatack commented at 6:54 AM on May 20, 2021:

Would be good to insert a blank line before it.

klementtan commented at 7:28 AM on May 20, 2021:

Added blank line before it 👍🏻

in src/bitcoin-cli.cpp:915 in 52ea193ad9 outdated

 910 | +        result["connections"]["total"].getValStr(),
 911 | +        RESET);
 912 | +    result_string += strprintf("Version: %s\n", result["version"].getValStr());
 913 | +    result_string += strprintf("Time offset (s): %s\n", result["timeoffset"].getValStr());
 914 | +    result_string += strprintf("Proxy: %s \n", result["proxy"].getValStr());
 915 | +    result_string += strprintf("Relay fee (%\\/kB): %s\n\n", CURRENCY_UNIT, result["relayfee"].getValStr());

jonatack commented at 7:00 AM on May 20, 2021:

s/kB/kvB/ (virtual size is used now, see #21752)

klementtan commented at 7:28 AM on May 20, 2021:

Thanks I didn't know that! Changed the units to kvB for Relay fee and Pay transaction fee

in src/bitcoin-cli.cpp:21 in 52ea193ad9 outdated

  17 | @@ -18,6 +18,7 @@
  18 |  #include <util/system.h>
  19 |  #include <util/translation.h>
  20 |  #include <util/url.h>
  21 | +#include <policy/feerate.h>

jonatack commented at 7:02 AM on May 20, 2021:

sort

klementtan commented at 7:27 AM on May 20, 2021:

Thanks for spotting that! updated position of import

klementtan commented at 7:24 AM on May 20, 2021: contributor

"Pay transaction fee" should have a unit too, right?

"Proxy" is empty. Maybe "Proxy: N/A" when there is no proxy? Or hide it altogether when there is no proxy? I have not checked possible values but it feels strage to show "Proxy: <nothing here>".

Update the PR with the following changes:

If no Proxy then display N/A instead of an empty string
added unit for Pay transaction fee
Change all units to vkB

klementtan force-pushed on May 20, 2021

kiminuo commented at 9:07 AM on May 20, 2021: contributor

This is my output on Windows 10 with colors turned for the Win32 platform:

Windows Terminal running Powershell 7

(https://github.com/microsoft/terminal/ is not bundled with Windows by default and Powershell 7 is not bundled in Windows 10 by default too)

Note: Windows Terminal running PowerShell 5 works good too which only shows that Terminal is the thing that processes the colors.

Command prompt (good old cmd)

(bundled with Windows by default)

Note that the same command run in Powershell (without Windows Terminal) show the same output.

Going further

I'm not sure how to support colors on Windows properly in all scenarios but the following two pages gives useful intro:

Maybe somebody will pick this up in the future.

klementtan commented at 9:33 AM on May 20, 2021: contributor

@kiminuo Thanks a lot for testing! <strike>Unfortunately, I do not own a Windows computer to test this myself.

Does this mean the WIN32 check here is insufficient? </strike>

Will create a follow-up issue for this if this PR gets merged

in src/bitcoin-cli.cpp:926 in a9bfceee57 outdated

 921 | +
 922 | +        result_string += strprintf("Keypool size: %s\n", result["keypoolsize"].getValStr());
 923 | +        if (!result["unlocked_until"].isNull()) {
 924 | +            result_string += strprintf("Unlocked until: %s\n", result["unlocked_until"].getValStr());
 925 | +        }
 926 | +        result_string += strprintf("Pay transaction fee (%s/kvB): %s\n\n", CURRENCY_UNIT, result["paytxfee"].getValStr());

jonatack commented at 10:03 AM on May 20, 2021:

So that it is clear that this line refers to the -paytxfee configuration option without having to look in the code, and that it is actually a fee rate, maybe either s/Pay transaction fee/Paytxfee/ or

        result_string += strprintf("Transaction fee rate (-paytxfee) (%s/kvB): %s\n\n", CURRENCY_UNIT, result["paytxfee"].getValStr());

See also src/wallet/init.cpp:

argsman.AddArg("-paytxfee=<amt>", strprintf("Fee rate (in %s/kvB) to add to transactions you send (default: %s)",

klementtan commented at 10:45 AM on May 20, 2021:

Thanks! Updated to Paytxfee instead because Transaction fee rate (-paytxfee) seems a bit too long

in test/functional/interface_bitcoin_cli.py:131 in a9bfceee57 outdated

 148 | -            assert_equal(cli_get_info['paytxfee'], wallet_info['paytxfee'])
 149 | -            assert_equal(cli_get_info['relayfee'], network_info['relayfee'])
 150 | +            assert_equal(int(cli_get_info['Keypool size']), wallet_info['keypoolsize'])
 151 | +            assert_equal(int(cli_get_info['Unlocked until']), wallet_info['unlocked_until'])
 152 | +            assert_equal(Decimal(cli_get_info['Pay transaction fee (BTC/kvB)']), wallet_info['paytxfee'])
 153 | +            assert_equal(Decimal(cli_get_info['Relay fee (BTC/kvB)']), network_info['relayfee'])

jonatack commented at 10:07 AM on May 20, 2021:

maybe "Min tx relay fee rate" or "Minimum fee rate for transaction relay" or something like that...WDYT?

            assert_equal(Decimal(cli_get_info['Min tx relay fee rate (BTC/kvB)']), network_info['relayfee'])

(see the getnetworkinfo help)

klementtan commented at 10:46 AM on May 20, 2021:

Agreed 👍 ! Min tx relay fee rate is better wording. Updated to Relay fee to Min tx relay fee rate

klementtan force-pushed on May 20, 2021

klementtan commented at 10:59 AM on May 21, 2021: contributor

Updated the PR with the following changes:

Change Relay fee to Min tx relay fee rate
Change Pay transaction fee to Paytxfee

Cleaned up OP to add additional details on how to test as the PR comments have become rather long

klementtan force-pushed on May 21, 2021

jonatack commented at 8:04 AM on May 25, 2021: member

Will re-test shortly. This would be nice for the upcoming release.

klementtan force-pushed on May 25, 2021

jonatack commented at 9:20 PM on May 25, 2021: member

This is nicely improved after the feedback by @kiminuo.

Regarding the paytxfee and relay fee labels, I wonder if the longer versions wouldn't be more user-friendly and have the advantage of both clearly being fee rates (see screenshot), but I'm honestly not sure. Happy to re-ACK if you update.

Screenshot from 2021-05-25 23-13-01

ACK 7c0cd204d48772b5f41c13197f25e7c8f7a24d99

ghost commented at 9:57 PM on May 25, 2021: none

Concept ACK

klementtan force-pushed on May 26, 2021

klementtan commented at 1:17 AM on May 26, 2021: contributor

@jonatack Thanks for reviewing again!

Regarding the paytxfee and relay fee labels, I wonder if the longer versions wouldn't be more user-friendly and have the advantage of both clearly being fee rates (see screenshot)

Yup I agree. Having a longer version for paytxfee would allow for better standardization and user-friendliness. Have updated the PR to change Paytxfee (BTC/kVb) to Transaction fee rate (-paytxfee) (BTC/kvB).

Updated paytxfee label:

Can I also ask:

Is it harder to review if I constantly squash my changes compared to squashing it at the end when everything is finalized?
Should I remove screenshots and markdown toggles from the PR description because it would be added as the commit message after the PR has been merged?

jonatack commented at 1:04 PM on May 26, 2021: member

Diff-review-only re-ACK 36ddd44c9df3f1209be387c40b801c2fbab49672 per git diff 7c0cd20 36ddd44

Is it harder to review if I constantly squash my changes compared to squashing it at the end when everything is finalized?

Once the pull is complete and ready for more-or-less final review, I think it's preferable to squash immediately, as git diff (or git range-diff) allows easily diffing the changes, particularly when you are looking for final review ACKs which would be held up by waiting for you to squash (see https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits for more).

Should I remove screenshots and markdown toggles from the PR description because it would be added as the commit message after the PR has been merged?

Not sure about images, but markdown toggles maybe yes to not rely on markdown for it to make sense. At least avoid @-names and <strike>strike-through</strike> formatting; see https://jonatack.github.io/articles/how-to-contribute-pull-requests-to-bitcoin-core#pr-descriptions.

klementtan commented at 4:36 PM on May 26, 2021: contributor

but markdown toggles maybe yes to not rely on markdown for it to make sense @jonatack Ok got it! Updated the PR description to remove the markdown toggles.

jonatack commented at 10:52 AM on May 30, 2021: member

I think this is a nice improvement and it has an ACK by me and 3 Concept ACKs, if anyone else would like to review or test it.

ghost commented at 1:59 PM on May 30, 2021: none

nit: Could have created a "topic specific branch" for PR as mentioned in CONTRIBUTING.md#contributor-workflow
Compiled and tested on Ubuntu. Bitcoin symbol unicode character was missing. Maybe we can avoid using it. Either mention nothing because Bitcoin Core wallet will only be used for bitcoin and nothing else or write BTC after amount.

After running sudo apt install fonts-noto and relaunch terminal

nit: Pink color for wallet looks weird. Will be better if we could use some other color similar or same as color used for 'balance'.

Will compile for Windows and test it on command prompt and PowerShell by tomorrow.

klementtan force-pushed on May 30, 2021

klementtan commented at 4:27 PM on May 30, 2021: contributor

@prayank23 thanks a lot for reviewing ❤️

nit: Could have created a "topic specific branch" for PR as mentioned in CONTRIBUTING.md#contributor-workflow

My apologies for that. I didn't want to create a new PR with a "topic-specific branch" as other people had already started reviewing the PR.

Compiled and tested on Ubuntu. Bitcoin symbol unicode character was missing. Maybe we can avoid using it. Either mention nothing as balance in Bitcoin Core wallet will only be for Bitcoin and nothing else or write BTC after amount.

Yeah, that makes sense since the Bitcoin symbol isn't supported by default for all systems and it is just for novelty purposes. Removed the Bitcoin symbol in ef535b4

nit: Pink color for wallet looks weird. Will be better if we could use some other color similar or same as color used for 'balance'.

Hmmm I am not too sure about this as the only other color that is left in ANSI Colour Code is red and I am afraid that it would give a false sense of danger to the user.

Another approach would be to use bright cyan(\x1B[96m) for wallet(refer to screenshot below). However in my opinion, the difference between bright and non-bright color is too subtle and having a distinct color difference(ie using magenta) would be better.

klementtan commented at 4:31 PM on May 30, 2021: contributor

Made the following changes for a283d3d to ef535b4:

Removed bitcoin emoji as it is not supported by default on ubuntu

<summary> Screenshots </summary>

No wallet loaded:

Single wallet loaded

Multi wallet loaded

</details>

ghost commented at 4:44 PM on May 30, 2021: none

Hmmm I am not too sure about this as the only other color that is left in ANSI Colour Code is red and I am afraid that it would give a false sense of danger to the user.

Agree. Red works better for errors.

Another approach would be to use bright cyan(\x1B[96m) for wallet(refer to screenshot below). However in my opinion, the difference between bright and non-bright color is too subtle and having a distinct color difference(ie using magenta) would be better.

I am okay with this approach or pink is also fine if we don't have a better color.

unknown approved

unknown commented at 4:46 PM on May 30, 2021: none

tACK https://github.com/bitcoin/bitcoin/pull/21832/commits/ef535b48b618a40383498a35ec08a45992e51b58

DrahtBot added the label Needs rebase on May 31, 2021

klementtan force-pushed on May 31, 2021

klementtan commented at 12:24 PM on May 31, 2021: contributor

ef535b4 to 23c8c52 rebased to resolve conflict.

DrahtBot removed the label Needs rebase on May 31, 2021

in test/functional/interface_bitcoin_cli.py:124 in e94209b4c5 outdated

 137 |  
 138 |          if self.is_wallet_compiled():
 139 |              self.log.info("Test -getinfo and bitcoin-cli getwalletinfo return expected wallet info")
 140 | -            assert_equal(cli_get_info['balance'], BALANCE)
 141 | -            assert 'balances' not in cli_get_info.keys()
 142 | +            assert_equal(float(cli_get_info['Balance']), BALANCE)

kiminuo commented at 1:37 PM on May 31, 2021:

nit: Comparing of floats in general can lead to comparing errors in general. (Please see https://stackoverflow.com/questions/4548004/how-to-correctly-and-standardly-compare-floats)

So even though it may not be an issue now and here, it's still a question whether one should consider using Decimal comparison or even string comparison.

I don't know exact details here, so feel free to ignore this comment.

klementtan commented at 1:52 PM on May 31, 2021:

<strike>Yeah I agree. Comparing floats directly is not a great idea and we could do something like this. However, I think if we were to use that method a separate PR should be created to replace all float comparisons with it.</strike>

MarcoFalke commented at 11:37 AM on June 1, 2021:

The others in this file are already using Decimal, no? E.g. assert_equal(Decimal(cli_get_info['Balance']), amounts[1]).

klementtan commented at 12:30 PM on June 1, 2021:

<strike>Yup, they are all using python Decimal for the file. Correct if I am wrong but I think what @kiminuo meant is that using assert_equal for floats(float/Decimal) might lead to comparison errors and maybe another type of assertion should be used to check if the floats are "close enough".

However, I am and very new here, and might not know the nuance behind it.</strike>

klementtan commented at 12:42 PM on June 1, 2021:

Also changed float(cli_get_info['Balance']) to Decimal(cli_get_info['Balance'] to standardize with the rest of the tests.

MarcoFalke commented at 12:47 PM on June 1, 2021:

Decimal is not a float

klementtan commented at 1:59 PM on June 1, 2021:

My apologies I was confusing myself. Please ignore whatever I said

klementtan force-pushed on May 31, 2021

jonatack commented at 11:33 AM on June 1, 2021: member

Tested ACK 23c8c5284acb18d8e960184e0b2d425c8a304004

in src/bitcoin-cli.cpp:928 in 23c8c5284a outdated

 923 | +            result_string += strprintf("Unlocked until: %s\n", result["unlocked_until"].getValStr());
 924 | +        }
 925 | +        result_string += strprintf("Transaction fee rate (-paytxfee) (%s/kvB): %s\n\n", CURRENCY_UNIT, result["paytxfee"].getValStr());
 926 | +    }
 927 | +    if (!result["balance"].isNull()) {
 928 | +        result_string += strprintf("%sBalance%s: %s\n\n", CYAN, RESET, result["balance"].getValStr());

jonatack commented at 11:35 AM on June 1, 2021:

micro-nit

        result_string += strprintf("%sBalance:%s %s\n\n", CYAN, RESET, result["balance"].getValStr());

klementtan commented at 12:09 PM on June 1, 2021:

👍 updated this in 8233881

klementtan force-pushed on Jun 1, 2021

klementtan commented at 12:17 PM on June 1, 2021: contributor

23c8c52 to 8233881

Added color to : for Balance: and Warning:
Minor tests cleanup

in doc/release-notes.md:148 in 823388119b outdated

 144 | @@ -145,6 +145,8 @@ Tools and Utilities
 145 |    like `-onlynet=<network>` or to upgrade to current and future Tor releases
 146 |    that support Tor v3 addresses only.  (#21595)
 147 |  
 148 | +- Update `-getinfo` to return data in user-friendly format that also reduces vertical space. (#21832)

jonatack commented at 6:14 PM on June 1, 2021:

- Update `-getinfo` to return data in a user-friendly format that also reduces vertical space. (#21832)

klementtan commented at 6:37 PM on June 1, 2021:

👍 Resolved it in b634ff2

in test/functional/interface_bitcoin_cli.py:33 in 823388119b outdated

  27 | @@ -27,6 +28,41 @@
  28 |  WALLET_NOT_LOADED = 'Requested wallet does not exist or is not loaded'
  29 |  WALLET_NOT_SPECIFIED = 'Wallet file not specified'
  30 |  
  31 | +
  32 | +def cli_get_info_string_to_dict(cli_get_info_string):
  33 | +    # helper method to convert human readable -getinfo into a dictionary

jonatack commented at 6:15 PM on June 1, 2021:

    """Helper method to convert human-readable -getinfo output to a dictionary"""

klementtan commented at 6:37 PM on June 1, 2021:

👍 Resolved it in b634ff2

in test/functional/interface_bitcoin_cli.py:42 in 823388119b outdated

  37 | +    ansi_escape = re.compile(r'(\x9B|\x1B\[)[0-?]*[ -\/]*[@-~]')
  38 | +    while line_idx < len(lines):
  39 | +        # Remove ansi colour code
  40 | +        line = ansi_escape.sub('', lines[line_idx])
  41 | +        if "Balances" in line:
  42 | +            # When Balances appears in a line, all of the following line contains "balance: wallet" until an empty line

jonatack commented at 6:16 PM on June 1, 2021:

            # When "Balances" appears in a line, all of the following lines contain "balance: wallet" until an empty line

klementtan commented at 6:37 PM on June 1, 2021:

Good catch! Resolved it in b634ff2

jonatack commented at 6:19 PM on June 1, 2021: member

ACK 823388119bb1179e3667f86e82ab6817f87eb616

A few nits below, will re-ACK if you choose to address them.

klementtan force-pushed on Jun 1, 2021

klementtan commented at 6:40 PM on June 1, 2021: contributor

A few nits below, will re-ACK if you choose to address them. @jonatack thanks! Addressed the nits in 8233881 to b634ff2

kiminuo commented at 6:52 PM on June 1, 2021: contributor

tACK b634ff2

Good work!

jonatack commented at 8:23 PM on June 1, 2021: member

Code review re-ACK b634ff26ab8cf4bdffed1a75300ec63a9da22cf1 per git diff 8233881 b634ff2, documentation and code comment changes only

ghost commented at 8:54 PM on June 1, 2021: none

~~re-ACK https://github.com/bitcoin/bitcoin/commit/b634ff26ab8cf4bdffed1a75300ec63a9da22cf1~~

I couldn't' see any colors in PowerShell like #21832 (comment) :(

Maybe it works only with Microsoft Terminal.

<details><summary>Command Prompt</summary>

</details>

<details><summary>PowerShell</summary>

</details>

in src/bitcoin-cli.cpp:904 in b634ff26ab outdated

 891 | +    const std::string GREEN = "";
 892 | +    const std::string BLUE = "";
 893 | +    const std::string YELLOW = "";
 894 | +    const std::string MAGENTA = "";
 895 | +    const std::string CYAN = "";
 896 | +#endif

unknown commented at 11:27 PM on June 1, 2021:

const std::string RESET = "\x1B[0m";
const std::string GREEN = "\x1B[32m";
const std::string BLUE = "\x1B[34m"; 
const std::string YELLOW = "\x1B[33m";
const std::string MAGENTA = "\x1B[35m"; 
const std::string CYAN = "\x1B[36m";

If you remove the code that ignores ANSI escape sequence on Windows, we can see colors on Windows in Command Prompt and PowerShell as well

unknown changes_requested

unknown commented at 11:29 PM on June 1, 2021: none

This is in Command Prompt after making the changes:

Users will have to add one registry key so that ANSI escape sequence works:

reg add HKEY_CURRENT_USER\Console /v VirtualTerminalLevel /t REG_DWORD /d 0x00000001 /f

https://www.codeproject.com/Tips/5255355/How-to-Put-Color-on-Windows-Console

sipa commented at 12:17 AM on June 2, 2021: member

Maybe this should only be enabled when stdout is connected to a terminal (isatty(stdout))?

ghost commented at 1:23 AM on June 2, 2021: none

Yes this should only be enabled for output in terminal.

Although I am not sure about header files required for it to work on every OS. Maybe unistd.h for Linux and io.h for Windows

klementtan commented at 2:34 AM on June 2, 2021: contributor

@prayank23 thanks a lot for testing!

reg add HKEY_CURRENT_USER\Console /v VirtualTerminalLevel /t REG_DWORD /d 0x00000001 /f

If we go with that approach does it mean that bitcoin core will no longer work out of the box for Windows users? In my opinion not having colors for WIN32 would be better than having to run an additional step of entering the reg add ... command as an administrator. What do you think? :)

@sipa: Maybe this should only be enabled when stdout is connected to a terminal (isatty(stdout))?

Is the purpose of this to not return ANSI codes when -getinfo is executed outside of the terminal?

Side note: If the current handling of colored text is unsatisfactory I would prefer to remove all colored text support in this PR and create a separate PR for that. It seems that supporting colored text for every OS is quite complicated.

sipa commented at 2:45 AM on June 2, 2021: member

@klementtan Right, when you're sending output to a text file or through grep or something like that, you generally don't want colorization.

ghost commented at 2:54 AM on June 2, 2021: none

If we go with that approach does it mean that bitcoin core will no longer work out of the box for Windows users? In my opinion not having colors for WIN32 would be better than having to run an additional step of entering the reg add ... command as an administrator. What do you think? :)

Colors don't work on Windows with ANSI escape sequence by default. So making the changes that I suggested in #21832 (review) will change nothing but simplify code as we will use same code for Linux and Windows.

If any user is curious and wants to use colors we can always suggest making changes in registry key. Else we will have to use windows.h and different code for Windows. IMO there are very less users who use Bitcoin Core on Windows and want to see colors in command prompt. So asking few power users who are curious to make changes in registry, use same code with less lines for every OS will be a better approach.

Just tried running the code with suggested changes and registry key deleted. It doesn't work as expected. Not sure if supporting colors on Windows makes sense. You can ignore the suggested changes.

klementtan force-pushed on Jun 2, 2021

klementtan commented at 5:11 AM on June 2, 2021: contributor

Right, when you're sending output to a text file or through grep or something like that, you generally don't want colorization. @sipa Good idea! Added this in the latest commit.

b634ff2 to bc97f9a:

return colored text only when connected to the terminal.
No change in behaviour for normal cli usage

<details> <summary> Before b634ff2 </summary>

./src/bitcoin-cli -signet -rpcwallet="" -getinfo > temp.txt

temp.txt contents

[34mChain: signet[0m
Blocks: 40452
Headers: 40452
Verification progress: 0.9999998746026817
Difficulty: 0.002836490385148087

[32mNetwork: in 0, out 10, total 10[0m
Version: 219900
Time offset (s): 0
Proxy: N/A
Min tx relay fee rate (BTC/kvB): 0.00001000

[35mWallet: ""[0m
Keypool size: 1000
Transaction fee rate (-paytxfee) (BTC/kvB): 0.00000000

[36mBalance:[0m 11.30000000

[33mWarnings:[0m This is a pre-release test build - use at your own risk - do not use for mining or merchant applications

</details>

<details> <summary> After bc97f9a </summary>

./src/bitcoin-cli -signet -rpcwallet="" -getinfo > temp.txt

temp.txt contents

Chain: signet
Blocks: 40450
Headers: 40450
Verification progress: 0.9999923418449367
Difficulty: 0.002836490385148087

Network: in 0, out 10, total 10
Version: 219900
Time offset (s): 0
Proxy: N/A
Min tx relay fee rate (BTC/kvB): 0.00001000

Wallet: ""
Keypool size: 1000
Transaction fee rate (-paytxfee) (BTC/kvB): 0.00000000

Balance: 11.30000000

Warnings: This is a pre-release test build - use at your own risk - do not use for mining or merchant applications

</details>

bc97f9a to https://github.com/bitcoin/bitcoin/commit/84547644885098fc94a3c126a0464f3d08277f5f resolve linting issue

klementtan force-pushed on Jun 2, 2021

ghost commented at 9:32 AM on June 2, 2021: none

ACK https://github.com/bitcoin/bitcoin/commit/84547644885098fc94a3c126a0464f3d08277f5f

in src/bitcoin-cli.cpp:891 in 8454764488 outdated

 886 | +    std::string RESET = "";
 887 | +    std::string GREEN = "";
 888 | +    std::string BLUE = "";
 889 | +    std::string YELLOW = "";
 890 | +    std::string MAGENTA = "";
 891 | +    std::string CYAN = "";

jonatack commented at 4:28 PM on June 3, 2021:

style nit, std::string is default-initialized to ""

-    std::string RESET = "";
-    std::string GREEN = "";
-    std::string BLUE = "";
-    std::string YELLOW = "";
-    std::string MAGENTA = "";
-    std::string CYAN = "";
+    std::string RESET, GREEN, BLUE, YELLOW, MAGENTA, CYAN;

klementtan commented at 6:34 PM on June 3, 2021:

Agreed that is much cleaner 👍 . Updated to this in 7f253cf

jonatack commented at 4:35 PM on June 3, 2021: member

ACK 84547644885098fc94a3c126a0464f3d08277f5f

Tested on debian that piping the output to a .txt file does not write the special characters corresponding to the color codes, whereas with the previous versions like b634ff26a it did.

kiminuo commented at 5:44 PM on June 3, 2021: contributor

ACK 84547644885098fc94a3c126a0464f3d08277f5f

klementtan force-pushed on Jun 3, 2021

klementtan commented at 6:36 PM on June 3, 2021: contributor

84547644885098fc94a3c126a0464f3d08277f5f to 7f253cf resolved style issues. No change in any behaviour to -getinfo.

jonatack commented at 6:39 PM on June 3, 2021: member

Lightly retested and diff review re-ACK 7f253cf3459afa340412887765b62faff2f387f5 per git diff 8454764 7f253cf

ghost commented at 6:41 PM on June 3, 2021: none

ACK https://github.com/bitcoin/bitcoin/commit/7f253cf3459afa340412887765b62faff2f387f5

in src/bitcoin-cli.cpp:919 in 7f253cf345 outdated

 898 | +#endif
 899 | +
 900 | +    std::string result_string = strprintf("%sChain: %s%s\n", BLUE, result["chain"].getValStr(), RESET);
 901 | +    result_string += strprintf("Blocks: %s\n", result["blocks"].getValStr());
 902 | +    result_string += strprintf("Headers: %s\n", result["headers"].getValStr());
 903 | +    result_string += strprintf("Verification progress: %s\n", result["verificationprogress"].getValStr());

lsilva01 commented at 1:01 AM on June 4, 2021:

nit: instead of verificationprogress: 0.9999... or verificationprogress: 0.9977977097643339, this change shows verificationprogress: 100% and verificationprogress: 99.78% respectively

    result_string += strprintf("Verification progress: %.2f%%\n", result["verificationprogress"].get_real() * 100);

klementtan commented at 2:04 AM on June 4, 2021:

Thanks a lot for reviewing!

I actually considered changing it to 2 decimal places but was on the fence for it. My concern is that currently 0.01% of the current height(686181) is about 6 blocks and displaying only 2 decimal places could confuse the user into thinking that their node is stuck. Additionally, as time goes on and the best height increases, I feel that it would be better to have a more granular value for verification progress.

What do you think? If the others also feel that having 2 decimal places is better I would change it! 😄

theStack commented at 10:48 PM on June 12, 2021:

I think Isilva01's proposal was primarily to change to per-cent unit, and the number of decimal places is just an example. Personally I'd also prefer showing in %; how about with 4 decimal places? With that, even a total block height of 1000000 (still years away) each single block verified results in a change of the verification progress display.

klementtan commented at 3:48 AM on June 13, 2021:

Yeah that makes sense! % would be better than fractional from. Implemented this in c5625c2. Thanks!

in src/bitcoin-cli.cpp:933 in 7f253cf345 outdated

 902 | +    result_string += strprintf("Headers: %s\n", result["headers"].getValStr());
 903 | +    result_string += strprintf("Verification progress: %s\n", result["verificationprogress"].getValStr());
 904 | +    result_string += strprintf("Difficulty: %s\n\n", result["difficulty"].getValStr());
 905 | +
 906 | +    result_string += strprintf(
 907 | +        "%sNetwork: in %s, out %s, total %s%s\n",

lsilva01 commented at 1:08 AM on June 4, 2021:

nit: Maybe Connections is clearer. Network can refer to IPv4, Tor, I2P ...

klementtan commented at 2:09 AM on June 4, 2021:

Network is a "header" that is used to represent that the segment contains network-related information from getnetworkinfo. The connections information were moved up to the same line as Network as their values are quite self-explanatory and we will save vertical real estate with that. #21832 (comment) for more information.

lsilva01 approved

lsilva01 commented at 1:09 AM on June 4, 2021: contributor

Tested ACK https://github.com/bitcoin/bitcoin/pull/21832/commits/7f253cf3459afa340412887765b62faff2f387f5 on Ubuntu 20.04.

in src/bitcoin-cli.cpp:890 in 7f253cf345 outdated

 885 | +
 886 | +    std::string RESET, GREEN, BLUE, YELLOW, MAGENTA, CYAN;
 887 | +
 888 | +#ifndef WIN32
 889 | +    if (isatty(fileno(stdout))) {
 890 | +      // Only print colored text if OS is not WIN32 and stdout is connected to a terminal.

luke-jr commented at 9:24 PM on June 11, 2021:

Might be nice to have a way to force it on/off?

klementtan commented at 10:36 PM on June 11, 2021:

Are you suggesting a bitcon.conf option or CLI argument to allow WIN32 users to print ANSI color code and non-WIN32 users to not print ANSI color code? For the former, I am planning on making a follow-up PR to allow that but I am not too sure if there is a strong use case for the latter tho.

luke-jr commented at 12:08 AM on June 12, 2021:

eg, most GNU tools support a --color=always/never/auto

Often useful when you want to pipe coloured output to less or such

klementtan commented at 3:53 AM on June 12, 2021:

Good suggestion! Added -color option in https://github.com/bitcoin/bitcoin/commit/5a2a6b3bc5e2b1bd085f5a687f1992182b71ad62

klementtan force-pushed on Jun 12, 2021

klementtan commented at 4:14 AM on June 12, 2021: contributor

@luke-jr: Might be nice to have a way to force it on/off?

7f253cf to 5a2a6b3: Added -color=never/always/auto

always: Will always display color
never: Will never display color
auto: Will only display color if OS is not WIN32 and stdout connected to terminal

Tested scenarios

<details> <summary>-color not set</summary>

$ ./src/bitcoin-cli -signet -getinfo
$ ./src/bitcoin-cli -signet -getinfo | cat

</details>

<details> <summary>-color=always</summary>

$ ./src/bitcoin-cli -signet -color=always -getinfo
$ ./src/bitcoin-cli -signet -color=always -getinfo | cat

</details>

<details> <summary>-color=never</summary>

$ ./src/bitcoin-cli -signet -color=never -getinfo
$ ./src/bitcoin-cli -signet -color=never -getinfo | cat

</details>

<details> <summary>-color=auto</summary>

$ ./src/bitcoin-cli -signet -color=auto -getinfo
$ ./src/bitcoin-cli -signet -color=auto -getinfo | cat

</details>

theStack commented at 10:56 PM on June 12, 2021: member

Concept ACK Only quickly tested the "No wallet" scenario so far, looks very nice! 🎉 Planning to test with wallet(s) loaded and to code-review tomorrow.

klementtan force-pushed on Jun 13, 2021

klementtan commented at 3:46 AM on June 13, 2021: contributor

@theStack how about with 4 decimal places?

5a2a6b3 to c5625c2: Chanted Verification progress: to percentage with 4 dp.

<details> <summary> New screenshots </summary>

No wallet loaded:

Single wallet loaded

Multi wallet loaded

</details>

MarcoFalke referenced this in commit 9c1ec689f3 on Jun 13, 2021

theStack approved

theStack commented at 3:58 PM on June 13, 2021: member

Tested ACK c5625c2f8258b896a8dcf3d60646bf1f7bd0bbe5

Ran all three scenarios (no wallet, single wallet, multi-wallet) in OpenBSD 6.9 on mainnet and signet, works well and looks neat.

Now interactively watching the IBD progress via e.g.

watch -n 1 -d ./src/bitcoin-cli -getinfo

is quite a nice alternative to tail -f ~/.bitcoin/debug.log with less noise.

Some follow-up ideas I could imagine:

also display date/time of latest block verified (maybe even the delta to the current time, to immediately see how far one is behind during IBD, or how long ago the last block was received, respectively)
also show on-disk size of the block chain (getblockchaininfo returns a field "size_on_disk")
show whether pruning is enabled
verification progress bar?

klementtan commented at 4:21 PM on June 13, 2021: contributor

@theStack thanks for reviewing and testing. Yeah, I agree those are great ideas! I will update the issue with other improvement ideas after this PR gets merged. I would also like to refactor ParseGetInfoResult and GetWalletBalances into GetinfoRequestHandler.

sidhujag referenced this in commit 6972699a0e on Jun 13, 2021

klementtan force-pushed on Jun 14, 2021

achow101 commented at 8:49 PM on June 14, 2021: member

Code Review ACK d6211e9a49fda31c2e2792874e537858c1913af3

klementtan commented at 8:52 PM on June 14, 2021: contributor

c5625c2 to d6211e9 resolve typo in test log

theStack approved

theStack commented at 8:53 PM on June 14, 2021: member

re-ACK d6211e9a49fda31c2e2792874e537858c1913af3 🍸 (the only change since my last ACK was fixing a debug message in the test)

in src/bitcoin-cli.cpp:85 in d6211e9a49 outdated

  81 | @@ -77,6 +82,7 @@ static void SetupCliArgs(ArgsManager& argsman)
  82 |      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);
  83 |      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);
  84 |      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);
  85 | +    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);

kiminuo commented at 9:02 PM on June 14, 2021:

Nit: I would expect:

    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);

klementtan commented at 9:43 PM on June 14, 2021:

👍 Changed in 14cb2e0

in src/bitcoin-cli.cpp:899 in d6211e9a49 outdated

 894 | +    }
 895 | +#endif
 896 | +
 897 | +    if (gArgs.IsArgSet("-color")) {
 898 | +        // Override default should_colorize value if -color is set.
 899 | +        std::string color = gArgs.GetArg("-color", "auto");

kiminuo commented at 9:07 PM on June 14, 2021:

The idea is that when somebody passes -color=nonsense, then you consider the value to be auto (no warning or so), right?

klementtan commented at 9:09 PM on June 14, 2021:

Yup! If they don't enter a valid "when" value, it would be treated as the default colouring.

klementtan force-pushed on Jun 14, 2021

klementtan commented at 9:43 PM on June 14, 2021: contributor

d6211e9 to 14cb2e0: to re-trigger CI because of failing flaky test and s/cli/CLI for args options.

Apologies to previous ACKs

klementtan requested review from jonatack on Jun 15, 2021

ghost commented at 5:35 AM on June 15, 2021: none

1. Terminal (-color not set):

<details><summary>Single Wallet</summary>

</details>

<details><summary>Multi Wallet</summary>

</details>

2. -color=never

bitcoin-cli -getinfo -color=never
bitcoin-cli -getinfo -color=never > color_never.txt

<details><summary>Terminal and Redirection</summary>

</details>

3. Redirect (-color not set and always):

<details><summary>-color not set</summary>

</details>

bitcoin-cli -getinfo > color_notset.txt

<details><summary>-color=always</summary>

</details>

bitcoin-cli -getinfo -color=always > color_always.txt

unknown approved

unknown commented at 5:38 AM on June 15, 2021: none

re-ACK https://github.com/bitcoin/bitcoin/pull/21832/commits/14cb2e0fe1384f89b4c9ca70751e6676f438b7a5

Tested again with new changes -color and redirection. LGTM.

theStack approved

theStack commented at 7:50 AM on June 15, 2021: member

re-ACK 14cb2e0fe1384f89b4c9ca70751e6676f438b7a5 🗻

achow101 commented at 5:58 PM on June 15, 2021: member

re-ACK 14cb2e0fe1384f89b4c9ca70751e6676f438b7a5

lsilva01 approved

lsilva01 commented at 7:16 PM on June 15, 2021: contributor

re-ACK https://github.com/bitcoin/bitcoin/pull/21832/commits/14cb2e0fe1384f89b4c9ca70751e6676f438b7a5

in src/bitcoin-cli.cpp:85 in 14cb2e0fe1 outdated

  81 | @@ -77,6 +82,7 @@ static void SetupCliArgs(ArgsManager& argsman)
  82 |      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);
  83 |      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);
  84 |      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);
  85 | +    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);

jonatack commented at 6:53 AM on June 16, 2021:

nit, sort and missing space (see proposed improvement diff)

    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);

klementtan commented at 8:01 AM on June 16, 2021:

Thanks for the suggestion! will do this if I need to retouch or in a followup

klementtan commented at 1:09 PM on July 5, 2021:

Resolved this in 658ab9c. Thanks!

jonatack commented at 6:55 AM on June 16, 2021: member

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

@@ -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") {

</details>

klementtan requested review from laanwj on Jul 1, 2021

klementtan force-pushed on Jul 5, 2021

klementtan commented at 1:10 PM on July 5, 2021: contributor

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.

in test/functional/interface_bitcoin_cli.py:104 in 658ab9caa3 outdated

 100 | @@ -65,37 +101,43 @@ def run_test(self):
 101 |          self.log.info("Test -getinfo with arguments fails")
 102 |          assert_raises_process_error(1, "-getinfo takes no arguments", self.nodes[0].cli('-getinfo').help)
 103 |  
 104 | +        self.log.info("Test -getinfo with -color=never retuns no ANSI escape codes")

jonatack commented at 4:34 PM on July 5, 2021:

        self.log.info("Test -getinfo with -color=never returns no ANSI escape codes")

better yet, s/retuns no/does not return/

klementtan commented at 6:31 PM on July 5, 2021:

Good catch! Resolved this in d525177

in test/functional/interface_bitcoin_cli.py:107 in 658ab9caa3 outdated

 100 | @@ -65,37 +101,43 @@ def run_test(self):
 101 |          self.log.info("Test -getinfo with arguments fails")
 102 |          assert_raises_process_error(1, "-getinfo takes no arguments", self.nodes[0].cli('-getinfo').help)
 103 |  
 104 | +        self.log.info("Test -getinfo with -color=never retuns no ANSI escape codes")
 105 | +        assert "\u001b[0m" not in self.nodes[0].cli('-getinfo', '-color=never').send_cli()
 106 | +
 107 | +        self.log.info("Test -getinfo with -color=always retuns ANSI escape codes")

jonatack commented at 4:34 PM on July 5, 2021:

        self.log.info("Test -getinfo with -color=always returns ANSI escape codes")

klementtan commented at 6:31 PM on July 5, 2021:

Good catch! Resolved this in d525177

jonatack commented at 5:17 PM on July 5, 2021: member

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.

klementtan force-pushed on Jul 5, 2021

jonatack commented at 8:08 PM on July 5, 2021: member

re-ACK d525177aba60e996481007989422280f9c191f15 per git diff 658ab9c d525177

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

in src/bitcoin-cli.cpp:906 in d525177aba outdated

 901 | +        const std::string color{gArgs.GetArg("-color", DEFAULT_COLOR_SETTING)};
 902 | +        if (color == "always") {
 903 | +            should_colorize = true;
 904 | +        } else if (color == "never") {
 905 | +            should_colorize = false;
 906 | +        } else if (color != DEFAULT_COLOR_SETTING) {

theStack commented at 12:42 AM on July 8, 2021:

nit: I don't think using the constant is a good idea here. If the default color setting is ever changed, then passing "auto" would not work anymore.

        } else if (color != "auto") {

klementtan commented at 12:55 AM on July 8, 2021:

Good point resolved this in 62aaff1.

klementtan force-pushed on Jul 8, 2021

klementtan commented at 12:58 AM on July 8, 2021: contributor

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

theStack approved

theStack commented at 1:13 AM on July 8, 2021: member

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 commented at 10:53 AM on July 8, 2021: member

re-ACK 62aaff178a665d757b604ee4a90556242c1135d3 per git diff d525177 62aaff1

MarcoFalke removed the label Tests on Jul 21, 2021

MarcoFalke added the label Utils/log/libs on Jul 21, 2021

in doc/release-notes.md:79 in 62aaff178a outdated

 144 | @@ -145,6 +145,8 @@ Tools and Utilities
 145 |    like `-onlynet=<network>` or to upgrade to current and future Tor releases
 146 |    that support Tor v3 addresses only.  (#21595)
 147 |  
 148 | +- Update `-getinfo` to return data in a user-friendly format that also reduces vertical space. (#21832)
 149 | +

MarcoFalke commented at 9:45 AM on July 21, 2021:

Note to myself: Might have been better to merge before #22515.

MarcoFalke commented at 9:45 AM on July 21, 2021: member

Needs rebase

DrahtBot commented at 10:00 AM on July 21, 2021: member

🐙 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".

DrahtBot added the label Needs rebase on Jul 21, 2021

cli: Implement human readable -getinfo. a37e29d32f

klementtan force-pushed on Jul 21, 2021

theStack approved

theStack commented at 12:12 PM on July 21, 2021: member

re-ACK a37e29d32fde8c7b4143322deeef2a8a06114d43

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

jonatack commented at 12:26 PM on July 21, 2021: member

Congrats @klementtan, well-deserved.

theStack commented at 12:35 PM on July 21, 2021: member

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

MarcoFalke merged this on Jul 21, 2021

MarcoFalke closed this on Jul 21, 2021

sidhujag referenced this in commit aa9be1ec16 on Jul 23, 2021

DrahtBot locked this on Aug 16, 2022

cli: Improve -getinfo return format #21832

Overview

Return Format

Coloring

Screenshots

Reviewing Notes

Testing Scenarios

Implementation

Conflicts

Windows Terminal running Powershell 7

Command prompt (good old cmd)

Going further

Tested scenarios

1. Terminal (-color not set):

2. -color=never

3. Redirect (-color not set and always):