rpc, test: remove newline escape sequence from wallet warning fields

jonatack commented at 6:00 pm on February 21, 2023: contributor

This PR extends our test coverage to demonstrate the issue, then removes newline escape sequences printed in the wallet warning field in RPCs createwallet, loadwallet, unloadwallet, and restorewallet.

before

0$ ./src/bitcoin-cli -signet createwallet w1 false false "" false false
1{
2  "name": "w1",
3  "warning": "Empty string given as passphrase, wallet will not be encrypted.\nWallet created successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future."
4}

after

0$ ./src/bitcoin-cli -signet createwallet w2 false false "" false false
1{
2  "name": "w2",
3  "warning": "Empty string given as passphrase, wallet will not be encrypted. Wallet created successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future."
4}

Rationale: Our RPC is returning newline presentation elements embedded in JSON string field responses. These are a client-side concern, not a server-side one. If it is important for the RPC client to know how many warnings have been returned in order to display them on separate lines, then it may be a good idea after this to gradually replace the warning string field with a warnings JSON array of strings, as we do in several other RPCs. I’m happy to do that in a follow-up, as it would be orthogonal to this change: first the warnings field would be added and the warning field deprecated, then after a release or so the latter could possibly be removed. In the meantime, this is a simple and focused fix.

test: extend createwallet coverage for legacy and passphrase warnings 458c9f3bf8

DrahtBot commented at 6:00 pm on February 21, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, pinheadmz, vasild

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27279 (Add “warnings”, deprecate “warning” in {create,load,unload,restore}wallet by jonatack)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)

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

jonatack force-pushed on Feb 21, 2023

jonatack commented at 8:32 pm on February 21, 2023: contributor

Updated for restorewallet as well.

TheCharlatan commented at 8:33 pm on February 21, 2023: contributor

Concept ACK

I believe only createwallet appends additional warning sentences and is affected by the fix.

I think they all do, check the code in CWallet::Create. E.g. by setting weird mintxfee and maxapsfee values:

0./bitcoin-cli -signet restorewallet "w2" "w2"
1{
2  "name": "w2",
3  "warning": "-mintxfee is set very high! This is the minimum transaction fee you pay on every transaction.\n-maxapsfee is set very high! This is the maximum transaction fee you pay (in addition to the normal fee) to prioritize partial spend avoidance over regular coin selection."
4}

rpc: drop newline escape sequences from wallet warning messages

in RPCs createwallet, loadwallet, unloadwallet, and restorewallet.

f0391cd3ea

jonatack force-pushed on Feb 21, 2023

jonatack commented at 8:45 pm on February 21, 2023: contributor

Thanks @TheCharlatan, updated the OP and commit message.

TheCharlatan approved

TheCharlatan commented at 12:44 pm on February 22, 2023: contributor

ACK f0391cd3ea562d526db63996e7694ff4cd44b8b2

Was a bit hesitant about adding the messages to the functional tests, but I think testing the correct concatenation kind of makes sense.

pinheadmz commented at 4:19 pm on February 27, 2023: member

Code changes and tests look OK to me ~but I don’t understand the motivation, are newlines a problem in RPC responses?~ (RPC was literally printing “\n” in the output)

I found a few other similar mechanisms in other modules, dunno if this change should apply to them as well:

https://github.com/bitcoin/bitcoin/blob/82793f1984911774b111117f2e81d5f3b0bbec68/src/warnings.cpp#L55-L57

https://github.com/bitcoin/bitcoin/blob/82793f1984911774b111117f2e81d5f3b0bbec68/src/validation.cpp#L2565-L2569

pinheadmz approved

pinheadmz commented at 3:06 pm on March 1, 2023: member

ACK f0391cd3ea562d526db63996e7694ff4cd44b8b2

ryanofsky commented at 3:27 pm on March 1, 2023: contributor

-0. Showing \n on the terminal seems like a client side problem not a server side problem, and this server-side change makes it harder for other clients besides bitcoin-cli to display the warnings clearly on separate lines.

IMO, the ideal fix for this would be for bitcoin-cli to colorize and pretty-print the json when it detects stdout is a tty.

jonatack commented at 4:47 pm on March 1, 2023: contributor

@ryanofsky Adding newlines is a client-side presentation concern. If it is important for the RPC client to know how many warnings have been returned in order to display them on separate lines, it seems best after this to gradually replace the warning string field with a warnings JSON array of strings, as we do in several other RPCs. I’m happy to do that in a follow-up, as it would be orthogonal to this change: first the warnings field would be added and the warning field deprecated, then after a release or so the latter could possibly be removed. In the meantime, this is a simple and focused fix. @pinheadmz (Thanks!) Saw those for (maybe, maybe not) another pull; prefer to keep this small and focused.

fanquake commented at 11:19 am on March 8, 2023: member

Saw those for (maybe, maybe not) another pull; prefer to keep this small and focused.

If they are the same issue, what’s the reasoning for leaving them for another PR? Not sure how the PR becomes less focused by fixing more of the same problem.

vasild approved

vasild commented at 10:49 am on March 17, 2023: contributor

ACK f0391cd3ea562d526db63996e7694ff4cd44b8b2

The newline is presentation-specific and does not belong to the server output. For example it will not render as expected if displayed in HTML (where newlines are displayed as a single space). Then it should be <br/> instead of \n, or maybe \r\n for MacOS?

I am happy to review more of the same in another PR or a change that returns an array of warnings so that display engines can chose the best separator for them.

ryanofsky commented at 3:40 pm on March 17, 2023: contributor

The newline is presentation-specific and does not belong to the server output.

Again, for the record, I disagree. The RPC server does not return HTML strings or XML strings, it returns text strings, and the newline character has a very clear meaning in text strings.

Newline is a better delimiter than space if there are multiple warnings, so they are separable and coherent. Replacing newlines with spaces produces addled run-on paragraphs like “Wallet will not be encrypted. Wallet created successfully. The legacy wallet type…”).

Newlines are better than spaces for RPC clients that know how to output text, so it is unfortunate that the bitcoin-cli client doesn’t know which fields are text fields and dumps the warnings as JSON encoded text instead.

I do appreciate that bitcoin-cli specifically has a problem here. But I think teaching bitcoin-cli that the result[“warning”] field is a text field, or teaching it to colorize JSON output, or adding a result[“warnings”] list field would be better solutions that don’t hurt other clients for the sake of helping bitcoin-cli.

achow101 commented at 8:57 pm on March 17, 2023: member

I agree with @ryanofsky that this is a client side issue which should be fixed in the client and not the server. I also prefer that we remove warning and replace it with warnings as has been done in several other RPCs.

jonatack commented at 9:25 pm on March 17, 2023: contributor

I think we all agree with adding a warnings field and deprecating warning, then later removing warning, as follow-ups over time.

jonatack commented at 9:35 pm on March 17, 2023: contributor

Replacing newlines with spaces produces addled run-on paragraphs like “Wallet will not be encrypted. Wallet created successfully. The legacy wallet type…”).

The output would be the following, which seems fine to me: "Empty string given as passphrase, wallet will not be encrypted. Wallet created successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future."

maflcko commented at 9:38 am on March 18, 2023: member

If the documentation for the field is unclear, what about changing the help text over changing the code?

jonatack commented at 3:42 pm on March 18, 2023: contributor

Thanks for the reviews, everyone.

I’m not sure it makes sense to patch around this by adding code to our CLI to parse newline escape sequences, as that would be adding unneeded complexity for 4 messages instead of fixing the problem at the source in 4 LOC, adding complexity for something that’s probably going away anyway (here and/or replaced eventually by warnings), and it would only fix it for us and not for other clients.

Elaborating that last point, the 4 escape sequences were added in pulls like #15937 without warning for client software to parse for them. While it is within the JSON standard to use them, unless API clients do better QA testing than Bitcoin Core it seems doubtful that clients detected the need for and added newline-delimited JSON string handling for a handful of messages out of many returned by our API. After all, we didn’t, so perhaps it’s a high ask. It also seems clear that a paragraph of several sentences causes less user astonishment than the same paragraph with \n sequences thrown in. So it’s not unlikely that this PR solves the same issue for other clients as well.

For similar reasons, I’m not sure that adding documentation to doc/JSON-RPC-interface.md and/or to each of the warning field helps would be better than simply removing the sequences. By the time users become aware and take action, the need will be gone.

I’ll propose a warnings pull this weekend to also begin deprecating warning and maybe improve the warning helps, preferably with a fix merged for the 6-12 month interim.

ryanofsky commented at 3:07 pm on March 19, 2023: contributor

I’m not sure it makes sense to patch around this by adding code to our CLI to parse newline escape sequences

Yes agree no need for this as we already have a JSON parser. CLI could see simply if check result object has "warning" field which is a string, and if it does, write the string to stderr.

This would be really simple, and make warnings more obvious and readable, and be compatible with other bitcoin-cli improvements in the future.

jonatack commented at 3:26 pm on March 19, 2023: contributor

If these four remaining “warning” fields are behind a deprecation flag (opening a PR today), hopefully that avoids the need to do anything further, unless we want to backport a fix. I’ll look into that.

jonatack commented at 5:41 pm on March 29, 2023: contributor

Circling back for completeness to cover a few questions/suggestions.

I found a few other similar mechanisms in other modules, dunno if this change should apply to them as well

https://github.com/bitcoin/bitcoin/blob/82793f1984911774b111117f2e81d5f3b0bbec68/src/validation.cpp#L2565-L2569

This code appends warning messages to the UpdateTipLog() logging and looks correct.

https://github.com/bitcoin/bitcoin/blob/82793f1984911774b111117f2e81d5f3b0bbec68/src/warnings.cpp#L55-L57

The <hr> thematic break tag in GetWarnings is only used by the GUI, as the 3 RPC callers of GetWarnings pass verbose = false to it, so that tag isn’t returned to them. I tested it in the GUI with macOS with the following change, and it seems fine.

 0--- a/src/warnings.cpp
 1+++ b/src/warnings.cpp
 2@@ -47,10 +47,14 @@ bilingual_str GetWarnings(bool verbose)
 3         warnings_verbose.emplace_back(warnings_concise);
 4     }
 5 
 6-    if (fLargeWorkInvalidChainFound) {
 7-        warnings_concise = _("Warning: We do not appear to fully agree with our peers! You may need to upgrade, or other nodes may need to upgrade.");
 8-        warnings_verbose.emplace_back(warnings_concise);
 9-    }
10+    warnings_concise = _("Warning: We do not appear to fully agree with our peers! You may need to upgrade, or other nodes may need to upgrade.");
11+    warnings_verbose.emplace_back(warnings_concise);
12+
13+    warnings_concise = _("Warning: Low on disk space.");
14+    warnings_verbose.emplace_back(warnings_concise);
15+
16+    warnings_concise = _("Warning: System clock is off.");
17+    warnings_verbose.emplace_back(warnings_concise);
18 
19     if (verbose) {
20         return Join(warnings_verbose, Untranslated("<hr />"));

CLI could see simply if check result object has “warning” field which is a string, and if it does, write the string to stderr.

I am not sure, but this seems a little kludgey; more so if we’re going to remove these fields anyway. I think I prefer either the approach in this PR or documenting warning in the RPC helps more clearly as suggested in #27138 (comment).

Am addressing the feedback above and will post here when it’s ready.

jonatack commented at 1:00 pm on March 30, 2023: contributor

Based on the discussion above, #27279 is now ready. It adds a warnings field to these four RPCs and deprecates the warning fields. Its first commit implements the doc suggestion in #27138 (comment) as an alternative to this pull to see what reviewers prefer.

achow101 referenced this in commit 6a167325f0 on Apr 12, 2023

jonatack closed this on Apr 12, 2023

jonatack commented at 5:12 pm on April 12, 2023: contributor

Superceded by #27279. Thanks everyone!

bitcoin locked this on Apr 11, 2024

rpc, test: remove newline escape sequence from wallet warning fields #27138

Reviews

Conflicts