cli: Improve -getinfo return format #21832

pull klementtan wants to merge 1 commits into bitcoin:master from klementtan:master changing 3 files +211 −45
  1. 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

     0// Data from getblockchaininfo
 1Chain: [getblockchaininfo.chain] 
 2Blocks: [getblockchaininfo.blocks]
 3Headers: [getblockchaininfo.headers]
 4Verification progress: [getblockchaininfo.verificationprogress]
 5Difficulty: [getblockchaininfo.difficulty]
 6
 7# Data from getnetworkinfo
 8Network: in [getnetworkinfo.connections_in], out [getnetworkinfo.connections_out], total [getnetworkinfo.connections]
 9Version: [getnetworkinfo.version]
10Time offset (s): [getnetworkinfo.timeoffset]
11Proxy: [getnetworkinfo.proxy] // "N/A" if no proxy
12Min tx relay fee rate (BTC/kvB): [getnetworkinfo.relayfee]
13
14# Data from getwalletinfo. Will only be present when a single wallet is loaded
15Wallet: [getnetworkinfo.walletname] // "" if walletname is empty(default wallet)
16Keypool size: [getnetworkinfo.keypoolsize]
17Unlocked until: [getnetworkinfo.unlocked_until]
18Transaction fee rate (-paytxfee) (BTC/kvB): [getnetworkinfo.paytxfee]
19
20# Data from getbalances. Will only be present when a single wallet is loaded
21Balance: [getbalances.mine.trusted]
22
23# Data from listwallets then getbalances for each wallet. Will only be present for multiple wallets loaded
24Balances
25[getbalances.mine.trusted] [listwallets[i]] // Right justify on balance
26
27Warnings: [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: image

    Single wallet loaded image

    Multi wallet loaded image

    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

    1. GetinfoRequestHandler is instantiated
    2. ConnectAndCallRPC is called with GetinfoRequestHandler. It returns UniValue::VOBJ result
    3. 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

    1. GetinfoRequestHandler is instantiated
    2. ConnectAndCallRPC is called with GetinfoRequestHandler. It returns UniValue::VOBJ result
    3. 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
    4. New ParseGetInfoResult is called with result as parameter. It converts results type from UniValue::VOBJ to UniValue::VSTR according to the format stated above.
  2. klementtan force-pushed on May 2, 2021
  3. DrahtBot added the label Tests on May 2, 2021
  4. klementtan force-pushed on May 2, 2021
  5. 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:

    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.

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

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

  8. klementtan commented at 1:46 pm on May 3, 2021: contributor

    @jonatack Updated the PR according to your comments:

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

    Multiple wallet loaded image

    Single wallet loaded image

  9. 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).
  10. 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?

     0    std::string RESET = "";
 1    std::string GREEN = "";
 2    std::string BLUE = "";
 3    std::string YELLOW = "";
 4    std::string MAGENTA = "";
 5    std::string CYAN = "";
 6#ifndef WIN32
 7    RESET = "\033[0m";
 8    GREEN = "\x1B[32m";
 9    BLUE = "\x1B[34m";
10    YELLOW = "\x1b[33m";
11    MAGENTA = "\x1B[35m";
12    CYAN = "\x1B[36m";
13#endif
    laanwj commented at 3:39 pm on May 4, 2021:

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

     0#ifndef WIN32
 1    const std::string RESET = "\x1B[0m";
 2    const std::string GREEN = "\x1B[32m";
 3    const std::string BLUE = "\x1B[34m";
 4    const std::string YELLOW = "\x1B[33m";
 5    const std::string MAGENTA = "\x1B[35m";
 6    const std::string CYAN = "\x1B[36m";
 7#else
 8    const std::string RESET = "";
 9    const std::string GREEN = "";
10    const std::string BLUE = "";
11    const std::string YELLOW = "";
12    const std::string MAGENTA = "";
13    const std::string CYAN = "";
14#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.
  11. klementtan commented at 11:22 am on May 7, 2021: contributor
    Added check to not print ansi colour code if running on WIN32.
  12. 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.
  13. klementtan force-pushed on May 10, 2021
  14. 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?

  15. klementtan force-pushed on May 11, 2021
  16. klementtan force-pushed on May 12, 2021
  17. 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.

  18. 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:

    0 /**
1- * ParseGetInfoResult takes in -getinfo result in UniValue object and parses it into a user friendly UniValue string to be
2- * printed on the console.
3- * [@param](/bitcoin-bitcoin/contributor/param/) result Reference to a UniValue object that contains all the -getinfo data.
4+ * ParseGetInfoResult takes in -getinfo result in UniValue object and parses it
5+ * into a user friendly UniValue string to be printed on the console.
6+ *
7+ * [@param](/bitcoin-bitcoin/contributor/param/)[out] result  Reference to UniValue result containing the -getinfo output.
8  */
    klementtan commented at 10:11 am on May 19, 2021:
    👍 updated it according to your suggestion.
  19. 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

    0        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
  20. 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

    0-    if (!find_value(result, "error").isNull()) {
1-        return;
2-    }
3+    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
  21. 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

     0             if (error.isNull()) {
 1-                if (gArgs.IsArgSet("-getinfo") && !gArgs.IsArgSet("-rpcwallet")) {
 2-                    GetWalletBalances(result); // fetch multiwallet balances and append to result
 3-                }
 4-
 5                 if (gArgs.IsArgSet("-getinfo")) {
 6+                    if (!gArgs.IsArgSet("-rpcwallet")) {
 7+                        GetWalletBalances(result); // fetch multiwallet balances and append to result
 8+                    }
 9                     ParseGetInfoResult(result);
10                 }
    klementtan commented at 5:55 pm on May 12, 2021:
    Followed your suggestion and combined the checks
  22. 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:

    0                if (gArgs.GetBoolArg("-getinfo", false)) {
    klementtan commented at 5:55 pm on May 12, 2021:
    Updated to use GetBoolArg instead of IsArgSet
  23. jonatack commented at 1:17 pm on May 12, 2021: member
    Quick skim of the code, waiting on the build to test.
  24. 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?

  25. 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?

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

    Single wallet loaded image

    Multi wallet loaded image

  26. klementtan force-pushed on May 12, 2021
  27. 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>
  28. 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).
  29. klementtan force-pushed on May 13, 2021
  30. klementtan force-pushed on May 13, 2021
  31. klementtan commented at 7:40 am on May 13, 2021: contributor

    Updated ed7feb4 with the following changes:

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

    Single wallet image Multi wallet image

    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!

  32. 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.
  33. 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

    image

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

  34. klementtan force-pushed on May 13, 2021
  35. klementtan requested review from jonatack on May 18, 2021
  36. jonatack commented at 5:11 pm on May 18, 2021: member
    Will re-test and review very soon.
  37. 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
  38. 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.
  39. 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.
  40. 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

  41. klementtan force-pushed on May 19, 2021
  42. 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)
  43. 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.
  44. 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.)

  45. 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
  46. 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)
  47. 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. :)

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

  49. 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) image
    • 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 🙇‍♂️

  50. klementtan force-pushed on May 20, 2021
  51. 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) image

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

  52. 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.:

    0$ bitcoin-cli -signet -rpcwallet="" -getinfo
1$ 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.

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

  54. 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 👍🏻
  55. 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
  56. 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
  57. 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: ”.

    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

    image

  58. klementtan force-pushed on May 20, 2021
  59. klementtan force-pushed on May 20, 2021
  60. 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

    image

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

    image

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

  61. klementtan commented at 9:33 am on May 20, 2021: contributor

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

    Does this mean the WIN32 check here is insufficient?

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

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

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

    See also src/wallet/init.cpp:

    0argsman.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
  63. 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?

    0            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
  64. klementtan force-pushed on May 20, 2021
  65. 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

  66. klementtan force-pushed on May 21, 2021
  67. jonatack commented at 8:04 am on May 25, 2021: member
    Will re-test shortly. This would be nice for the upcoming release.
  68. klementtan force-pushed on May 25, 2021
  69. 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

  70. ghost commented at 9:57 pm on May 25, 2021: none 
    0Concept ACK
  71. klementtan force-pushed on May 26, 2021
  72. 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: image

    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?
  73. 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-through formatting; see https://jonatack.github.io/articles/how-to-contribute-pull-requests-to-bitcoin-core#pr-descriptions.

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

  75. 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.
  76. ghost commented at 1:59 pm on May 30, 2021: none

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

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

    image

    After running sudo apt install fonts-noto and relaunch terminal

    image

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

  77. klementtan force-pushed on May 30, 2021
  78. klementtan force-pushed on May 30, 2021
  79. klementtan force-pushed on May 30, 2021
  80. klementtan commented at 4:27 pm on May 30, 2021: contributor

    @prayank23 thanks a lot for reviewing ❤️

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

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

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

    image

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

    No wallet loaded: image

    Single wallet loaded image

    Multi wallet loaded image

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

  83. unknown approved
  85. DrahtBot added the label Needs rebase on May 31, 2021
  86. klementtan force-pushed on May 31, 2021
  87. klementtan commented at 12:24 pm on May 31, 2021: contributor
    ef535b4 to 23c8c52 rebased to resolve conflict.
  88. DrahtBot removed the label Needs rebase on May 31, 2021
  89. 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:
    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.
    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:

    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.

    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
  90. klementtan force-pushed on May 31, 2021
  91. jonatack commented at 11:33 am on June 1, 2021: member
    Tested ACK 23c8c5284acb18d8e960184e0b2d425c8a304004
  92. 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

    0        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
  93. klementtan force-pushed on Jun 1, 2021
  94. klementtan commented at 12:17 pm on June 1, 2021: contributor

    23c8c52 to 8233881

    • Added color to : for Balance: and Warning:
    • Minor tests cleanup
  95. 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: 
    0- 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
  96. 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: 
    0    """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
  97. 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: 
    0            # 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
  98. 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.

  99. klementtan force-pushed on Jun 1, 2021
  100. 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

  101. kiminuo commented at 6:52 pm on June 1, 2021: contributor

    tACK b634ff2

    Good work!

  102. 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
  103. 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.

    image

    image

  104. 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: 
    0const std::string RESET = "\x1B[0m";
1const std::string GREEN = "\x1B[32m";
2const std::string BLUE = "\x1B[34m"; 
3const std::string YELLOW = "\x1B[33m";
4const std::string MAGENTA = "\x1B[35m"; 
5const 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

  105. unknown changes_requested
  106. unknown commented at 11:29 pm on June 1, 2021: none

    This is in Command Prompt after making the changes:

    image

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

    0reg 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

  107. sipa commented at 0:17 am on June 2, 2021: member
    Maybe this should only be enabled when stdout is connected to a terminal (isatty(stdout))?
  108. ghost commented at 1:23 am on June 2, 2021: none

    Yes this should only be enabled for output in terminal.

    Related link: https://stackoverflow.com/questions/2027484/determine-whether-process-output-is-being-redirected-in-c-c

    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

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

  110. 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.
  111. 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.

  112. klementtan force-pushed on Jun 2, 2021
  113. 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
    0./src/bitcoin-cli -signet -rpcwallet="" -getinfo > temp.txt

    temp.txt contents

     0Chain: signet
 1Blocks: 40452
 2Headers: 40452
 3Verification progress: 0.9999998746026817
 4Difficulty: 0.002836490385148087
 5
 6Network: in 0, out 10, total 10
 7Version: 219900
 8Time offset (s): 0
 9Proxy: N/A
10Min tx relay fee rate (BTC/kvB): 0.00001000
11
12Wallet: ""
13Keypool size: 1000
14Transaction fee rate (-paytxfee) (BTC/kvB): 0.00000000
15
16Balance: 11.30000000
17
18Warnings: This is a pre-release test build - use at your own risk - do not use for mining or merchant applications

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

    temp.txt contents

     0Chain: signet
 1Blocks: 40450
 2Headers: 40450
 3Verification progress: 0.9999923418449367
 4Difficulty: 0.002836490385148087
 5
 6Network: in 0, out 10, total 10
 7Version: 219900
 8Time offset (s): 0
 9Proxy: N/A
10Min tx relay fee rate (BTC/kvB): 0.00001000
11
12Wallet: ""
13Keypool size: 1000
14Transaction fee rate (-paytxfee) (BTC/kvB): 0.00000000
15
16Balance: 11.30000000
17
18Warnings: This is a pre-release test build - use at your own risk - do not use for mining or merchant applications

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

  114. klementtan force-pushed on Jun 2, 2021
  116. 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 ""

    0-    std::string RESET = "";
1-    std::string GREEN = "";
2-    std::string BLUE = "";
3-    std::string YELLOW = "";
4-    std::string MAGENTA = "";
5-    std::string CYAN = "";
6+    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
  117. 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.

  118. kiminuo commented at 5:44 pm on June 3, 2021: contributor
    ACK 84547644885098fc94a3c126a0464f3d08277f5f
  119. klementtan force-pushed on Jun 3, 2021
  120. klementtan commented at 6:36 pm on June 3, 2021: contributor
    84547644885098fc94a3c126a0464f3d08277f5f to 7f253cf resolved style issues. No change in any behaviour to -getinfo.
  121. jonatack commented at 6:39 pm on June 3, 2021: member
    Lightly retested and diff review re-ACK 7f253cf3459afa340412887765b62faff2f387f5 per git diff 8454764 7f253cf
  123. 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

    0    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!
  124. 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.
  125. lsilva01 approved
  126. 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.
  127. 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 0: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
  128. klementtan force-pushed on Jun 12, 2021
  129. 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

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

    image

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

    image

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

    image

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

    image

  130. 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.
  131. klementtan force-pushed on Jun 13, 2021
  132. 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.

    No wallet loaded: image

    Single wallet loaded image

    Multi wallet loaded image

  133. MarcoFalke referenced this in commit 9c1ec689f3 on Jun 13, 2021
  134. theStack approved
  135. 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.

    0watch -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?
  136. 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.
  137. sidhujag referenced this in commit 6972699a0e on Jun 13, 2021
  138. klementtan force-pushed on Jun 14, 2021
  139. achow101 commented at 8:49 pm on June 14, 2021: member
    Code Review ACK d6211e9a49fda31c2e2792874e537858c1913af3
  140. klementtan commented at 8:52 pm on June 14, 2021: contributor
    c5625c2 to d6211e9 resolve typo in test log
  141. theStack approved
  142. 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)
  143. 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:

    0    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
  144. 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.
  145. klementtan force-pushed on Jun 14, 2021
  146. 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

  147. klementtan requested review from jonatack on Jun 15, 2021
  148. ghost commented at 5:35 am on June 15, 2021: none

    1. Terminal (-color not set):

    image

    image

    2. -color=never

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

    image

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

    image

    0bitcoin-cli -getinfo > color_notset.txt

    image

    0bitcoin-cli -getinfo -color=always > color_always.txt
  149. unknown approved
  150. 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.

  151. theStack approved
  152. theStack commented at 7:50 am on June 15, 2021: member
    re-ACK 14cb2e0fe1384f89b4c9ca70751e6676f438b7a5 🗻
  153. achow101 commented at 5:58 pm on June 15, 2021: member
    re-ACK 14cb2e0fe1384f89b4c9ca70751e6676f438b7a5
  154. lsilva01 approved
  156. 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)

    0    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!
  157. 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)

     0@@ -52,6 +52,9 @@ static constexpr int8_t UNKNOWN_NETWORK{-1};
 1 /** Default number of blocks to generate for RPC generatetoaddress. */
 2 static const std::string DEFAULT_NBLOCKS = "1";
 3 
 4+/** Default -color setting. */
 5+static const std::string DEFAULT_COLOR_SETTING{"auto"};
 6+
 7 static void SetupCliArgs(ArgsManager& argsman)
 8 {
 9     SetupHelpOptions(argsman);
10@@ -70,6 +73,7 @@ static void SetupCliArgs(ArgsManager& argsman)
11     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);
12 
13     SetupChainParamsBaseOptions(argsman);
14+    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);
15     argsman.AddArg("-named", strprintf("Pass named instead of positional arguments (default: %s)", DEFAULT_NAMED), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
16     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);
17     argsman.AddArg("-rpcconnect=<ip>", strprintf("Send commands to node running on <ip> (default: %s)", DEFAULT_RPCCONNECT), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
18@@ -82,7 +86,6 @@ static void SetupCliArgs(ArgsManager& argsman)
19     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);
20     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);
21     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);
22-    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);
23 }
24 
25 /** libevent event log callback */
26@@ -895,8 +898,7 @@ static void ParseGetInfoResult(UniValue& result)
27 #endif
28 
29     if (gArgs.IsArgSet("-color")) {
30-        // Override default should_colorize value if -color is set.
31-        std::string color = gArgs.GetArg("-color", "auto");
32+        const std::string color{gArgs.GetArg("-color", DEFAULT_COLOR_SETTING)};
33         if (color == "always") {
34             should_colorize = true;
35         } else if (color == "never") {
  158. klementtan requested review from laanwj on Jul 1, 2021
  159. klementtan force-pushed on Jul 5, 2021
  160. 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
    0$ ./src/bitcoin-cli -signet -color=foo -getinfo
1error: Invalid value for -color option. Valid values: always, auto, never.
  161. 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: 
    0        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
  162. 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: 
    0        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
  163. 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

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

    New error

    0$ ./src/bitcoin-cli -regtest -color=foo -getinfo
1error: Invalid value for -color option. Valid values: always, auto, never.
  164. klementtan force-pushed on Jul 5, 2021
  165. klementtan force-pushed on Jul 5, 2021
  166. 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.

  167. 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 0: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.

    0        } else if (color != "auto") {
    klementtan commented at 0:55 am on July 8, 2021:
    Good point resolved this in 62aaff1.
  168. klementtan force-pushed on Jul 8, 2021
  169. klementtan commented at 0:58 am on July 8, 2021: contributor
    658ab9c to 62aaff1: resolved comments from reviewers regarding logging and better handling of -color option 🙏
  170. theStack approved
  171. 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 👌

  172. jonatack commented at 10:53 am on July 8, 2021: member
    re-ACK 62aaff178a665d757b604ee4a90556242c1135d3 per git diff d525177 62aaff1
  173. MarcoFalke removed the label Tests on Jul 21, 2021
  174. MarcoFalke added the label Utils/log/libs on Jul 21, 2021
  175. 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.
  176. MarcoFalke commented at 9:45 am on July 21, 2021: member
    Needs rebase
  177. 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”.

  178. DrahtBot added the label Needs rebase on Jul 21, 2021
  179. cli: Implement human readable -getinfo. a37e29d32f
  180. klementtan force-pushed on Jul 21, 2021
  181. theStack approved
  182. 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.

  183. jonatack commented at 12:26 pm on July 21, 2021: member
    Congrats @klementtan, well-deserved.
  184. 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? 🤔
  185. MarcoFalke merged this on Jul 21, 2021
  186. MarcoFalke closed this on Jul 21, 2021
  187. sidhujag referenced this in commit aa9be1ec16 on Jul 23, 2021
  188. DrahtBot locked this on Aug 16, 2022


klementtan DrahtBot jonatack laanwj kiminuo ghost MarcoFalke sipa lsilva01 theStack luke-jr achow101


laanwj

Labels
Utils/log/libs Needs rebase

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me