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

ghost · 2022-03-21T23:53:25Z

Adds (none) in warnings when no warnings returned by -getinfo

Reviewers can test this by making the following change in /src/warnings.cpp:

bilingual_str GetWarnings(bool verbose)
{
    bilingual_str warnings_concise;
    std::vector<bilingual_str> warnings_verbose;

    LOCK(g_warnings_mutex);

    // Pre-release build warning
    if (!CLIENT_VERSION_IS_RELEASE) {
-        warnings_concise = _("This is a pre-release test build - use at your own risk - do not use for mining or merchant applications");;
+        warnings_concise = _("");;

Before this pull request:

$ bitcoin-cli -getinfo
Chain: regtest
Blocks: 0
Headers: 0
Verification progress: 100.0000%
Difficulty: 4.656542373906925e-10

Network: in 0, out 0, total 0
Version: 239900
Time offset (s): 0
Proxies: n/a
Min tx relay fee rate (BTC/kvB): 0.00001000

Warnings:

After this pull request:

$ bitcoin-cli -getinfo
Chain: regtest
Blocks: 0
Headers: 0
Verification progress: 100.0000%
Difficulty: 4.656542373906925e-10

Network: in 0, out 0, total 0
Version: 239900
Time offset (s): 0
Proxies: n/a
Min tx relay fee rate (BTC/kvB): 0.00001000

Warnings: (none)

jessebarton

Concept ACK 1249aa6

Confirmed Warning is being thrown even if there are no Warnings
The version is showing 220000 I think because of the version of bitcoind im running, I could be wrong though.

Blocks: 728446
Headers: 728446
Verification progress: 99.9998%
Difficulty: 27452707696466.39

Network: in 8, out 10, total 18
Version: 220000
Time offset (s): 0
Proxies: n/a
Min tx relay fee rate (BTC/kvB): 0.00001000

Warnings:

Compiled and tested fix with no issues with the desired outcome of not throwing empty warnings.

Chain: main
Blocks: 728448
Headers: 728448
Verification progress: 99.9999%
Difficulty: 27452707696466.39

Network: in 15, out 10, total 25
Version: 220000
Time offset (s): 0
Proxies: n/a
Min tx relay fee rate (BTC/kvB): 0.00001000

ghost · 2022-03-22T07:58:12Z

The version is showing 220000 I think because of the version of bitcoind im running, I could be wrong though.

@jessebarton I see Version: 230000 when running RC2. It prints server version from getnetworkinfo so you are right that its 220000 because of bitcoind in your output.

jonatack · 2022-03-22T09:17:20Z

src/bitcoin-cli.cpp

+    if (result["warnings"].getValStr()!="")
+    {
+        result_string += strprintf("%sWarnings:%s %s", YELLOW, RESET, result["warnings"].getValStr());
+    }


suggestions

don't add extra line breaks when no warning is present

avoid running getValStr() twice

use empty()

clang-format of brackets in the conditional

--- a/src/bitcoin-cli.cpp +++ b/src/bitcoin-cli.cpp @@ -1038,7 +1038,7 @@ static void ParseGetInfoResult(UniValue& result) result_string += strprintf("Transaction fee rate (-paytxfee) (%s/kvB): %s\n\n", CURRENCY_UNIT, result["paytxfee"].getValStr()); } if (!result["balance"].isNull()) { - result_string += strprintf("%sBalance:%s %s\n\n", CYAN, RESET, result["balance"].getValStr()); + result_string += strprintf("%sBalance:%s %s\n", CYAN, RESET, result["balance"].getValStr()); } if (!result["balances"].isNull()) { @@ -1056,13 +1056,13 @@ static void ParseGetInfoResult(UniValue& result) result["balances"][wallet].getValStr(), wallet.empty() ? "\"\"" : wallet); } - result_string += "\n"; } - if (result["warnings"].getValStr()!="") - { - result_string += strprintf("%sWarnings:%s %s", YELLOW, RESET, result["warnings"].getValStr()); + const std::string warnings{result["warnings"].getValStr()}; + if (!warnings.empty()) { + result_string += strprintf("\n%sWarnings:%s %s", YELLOW, RESET, warnings); } + result.setStr(result_string); }

With this suggestion, I think there is still an extra line break after the balance/balances, if you can look into it.

@jonatack thanks for review and suggestion. Made changes, tested and it looks good. Let me know if there are any issues with line breaks:

With warning: https://pastebin.com/raw/eN3SGjiX
Without warning: https://pastebin.com/raw/frfnqfvU

As I mentioned, there is still an extra line, and the commits should be squashed, as the extra line is created by the changes here.

Done. Removed extra lines: https://pastebin.com/raw/bZk4YNj5 and squashed commits.

jonatack · 2022-03-22T09:28:23Z

Concept ACK. Previously, -getinfo returned a JSON-RPC-like response with the "warnings" field returned by getnetworkinfo after calling GetWarnings() in src/warnings.{h,cpp}. With #21832, -getinfo was converted to a human-readable format without taking into account that the field, when empty, could appear odd. This was likely overlooked because in pre-release development the tester will always see Warnings: This is a pre-release test build - use at your own risk - do not use for mining or merchant applications.

jonatack · 2022-03-23T13:10:19Z

Thanks for updating.

It looks like there is now (a) an extra line between "Balances" and the balances, and (b) a line missing before the warnings, with both one balance and multiple balances. Example:

₿ ./src/bitcoin-cli -signet -getinfo

.../...

Min tx relay fee rate (BTC/kvB): 0.00001000

Balances

  1.10997653 default settings
  0.00000000 disable private keys and blank
  0.00000000 disable private keys
  0.00000000 encrypted blank descriptor
  0.00000000 blank
498.77584341 ""
Warnings: This is a pre-release test build - use at your own risk - do not use for mining or merchant applications

Could you please update the PR and commit titles to include "-getinfo"? i.e. something like print -getinfo warnings only if warnings are present

theStack · 2022-03-25T12:58:10Z

Concept ACK

ghost · 2022-03-31T23:18:25Z

It looks like there is now (a) an extra line between "Balances" and the balances, and (b) a line missing before the warnings, with both one balance and multiple balances. Example:

Sorry, missed this. Will do it this weekend.

Could you please update the PR and commit titles to include "-getinfo"? i.e. something like print -getinfo warnings only if warnings are present

Done

DrahtBot · 2022-04-05T04:36:15Z

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

Conflicts

No conflicts as of last run.

laanwj · 2022-04-06T11:01:18Z

I don't know about leaving it out entirely. It might be reassuring to the user that there are, explicitly, no warnings. Or less reassuring (but important) that there could be warnings, but are none. Could format it different like Warnings: (none) or something like that if the empty field bothers you.

jonatack · 2022-04-07T07:05:01Z

I don't know about leaving it out entirely. It might be reassuring to the user that there are, explicitly, no warnings. Or less reassuring (but important) that there could be warnings, but are none. Could format it different like Warnings: (none) or something like that if the empty field bothers you.

Good point (and probably a simpler change to implement).

ghost · 2022-04-08T04:01:42Z

I don't know about leaving it out entirely. It might be reassuring to the user that there are, explicitly, no warnings. Or less reassuring (but important) that there could be warnings, but are none. Could format it different like Warnings: (none) or something like that if the empty field bothers you.

Done in last commit. Updated pull request title and description.

jonatack · 2022-04-08T10:36:18Z

src/bitcoin-cli.cpp

-    result_string += strprintf("%sWarnings:%s %s", YELLOW, RESET, result["warnings"].getValStr());
+    const std::string warnings{result["warnings"].getValStr()};
+    if (warnings.empty()) {
+        result_string += strprintf("%sWarnings: %s(none)", YELLOW, RESET);


Suggested change

result_string += strprintf("%sWarnings: %s(none)", YELLOW, RESET);

result_string += strprintf("%sWarnings:%s (none)", YELLOW, RESET);

alternatively, could simplify

- if (warnings.empty()) { - result_string += strprintf("%sWarnings: %s(none)", YELLOW, RESET); - } else { - result_string += strprintf("%sWarnings:%s %s", YELLOW, RESET, warnings); - } + result_string += strprintf("%sWarnings:%s %s", YELLOW, RESET, warnings.empty() ? "(none)" : warnings);

Done in last commit.

jonatack · 2022-04-08T16:06:12Z

ACK 0cea7b1

laanwj · 2022-04-13T19:47:27Z

Tested ACK 0cea7b1

…ng returned 0cea7b1 print `(none)` if no warnings in -getinfo (/dev/fd0) Pull request description: Adds `(none)` in warnings when no warnings returned by -getinfo Reviewers can test this by making the following change in `/src/warnings.cpp`: ```diff bilingual_str GetWarnings(bool verbose) { bilingual_str warnings_concise; std::vector<bilingual_str> warnings_verbose; LOCK(g_warnings_mutex); // Pre-release build warning if (!CLIENT_VERSION_IS_RELEASE) { - warnings_concise = _("This is a pre-release test build - use at your own risk - do not use for mining or merchant applications");; + warnings_concise = _("");; ``` Before this pull request: ``` $ bitcoin-cli -getinfo Chain: regtest Blocks: 0 Headers: 0 Verification progress: 100.0000% Difficulty: 4.656542373906925e-10 Network: in 0, out 0, total 0 Version: 239900 Time offset (s): 0 Proxies: n/a Min tx relay fee rate (BTC/kvB): 0.00001000 Warnings: ``` After this pull request: ```diff $ bitcoin-cli -getinfo Chain: regtest Blocks: 0 Headers: 0 Verification progress: 100.0000% Difficulty: 4.656542373906925e-10 Network: in 0, out 0, total 0 Version: 239900 Time offset (s): 0 Proxies: n/a Min tx relay fee rate (BTC/kvB): 0.00001000 Warnings: (none) ``` ACKs for top commit: jonatack: ACK 0cea7b1 laanwj: Tested ACK 0cea7b1 Tree-SHA512: a12499d11ff84bc954db354f968eb1f5ee4999d8b80581fe0bdf604732b2e2f608cb5c35c4ca8cb5a430f3991954a6207f0758302618662e6b9505044cf2dc95

DrahtBot added the Utils/log/libs label Mar 22, 2022

jessebarton approved these changes Mar 22, 2022

View reviewed changes

fanquake requested a review from jonatack March 22, 2022 08:54

jonatack reviewed Mar 22, 2022

View reviewed changes

ghost changed the title ~~print Warnings: only if warning returned~~ print -getinfo Warnings: only if warning returned Mar 31, 2022

DrahtBot mentioned this pull request Apr 5, 2022

Refactor: Improve API design of ScriptToUniv, TxToUniv etc to return the UniValue instead of mutating a parameter/reference #23507

Closed

ghost changed the title ~~print -getinfo Warnings: only if warning returned~~ add (none) in -getinfo Warnings: if no warning returned Apr 8, 2022

jonatack reviewed Apr 8, 2022

View reviewed changes

print (none) if no warnings in -getinfo

0cea7b1

laanwj merged commit 3bbc46d into bitcoin:master Apr 13, 2022

bitcoin locked and limited conversation to collaborators Apr 13, 2023

	result_string += strprintf("%sWarnings: %s(none)", YELLOW, RESET);
	result_string += strprintf("%sWarnings:%s (none)", YELLOW, RESET);

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

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

Uh oh!

Conversation

ghost commented Mar 21, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jessebarton left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Mar 22, 2022

Uh oh!

jonatack Mar 22, 2022

Choose a reason for hiding this comment

Uh oh!

ghost Mar 22, 2022

Choose a reason for hiding this comment

Uh oh!

jonatack Mar 22, 2022

Choose a reason for hiding this comment

Uh oh!

ghost Mar 22, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack commented Mar 22, 2022

Uh oh!

jonatack commented Mar 23, 2022

Uh oh!

theStack commented Mar 25, 2022

Uh oh!

ghost commented Mar 31, 2022

Uh oh!

DrahtBot commented Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Apr 7, 2022

Uh oh!

ghost commented Apr 8, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack Apr 8, 2022

Choose a reason for hiding this comment

Uh oh!

ghost Apr 8, 2022

Choose a reason for hiding this comment

Uh oh!

jonatack commented Apr 8, 2022

Uh oh!

laanwj commented Apr 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

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

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

ghost commented Mar 21, 2022 •

edited by ghost

Loading

jessebarton left a comment •

edited

Loading

ghost Mar 22, 2022 •

edited by ghost

Loading

DrahtBot commented Apr 5, 2022 •

edited

Loading

laanwj commented Apr 6, 2022 •

edited

Loading

ghost commented Apr 8, 2022 •

edited by ghost

Loading