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

[jonatack]

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

$ ./src/bitcoin-cli -signet createwallet w1 false false "" false false
{
  "name": "w1",
  "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."
}

after

$ ./src/bitcoin-cli -signet createwallet w2 false false "" false false
{
  "name": "w2",
  "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."
}

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.

[DrahtBot]

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]

Updated for restorewallet as well.

[TheCharlatan]

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:

./bitcoin-cli -signet restorewallet "w2" "w2"
{
  "name": "w2",
  "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."
}

[jonatack]

Thanks @TheCharlatan, updated the OP and commit message.

[TheCharlatan]

ACK f0391cd

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

[pinheadmz]

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:

bitcoin/src/warnings.cpp

Lines 55 to 57 in 82793f1

	if (verbose) {
	return Join(warnings_verbose, Untranslated("<hr />"));
	}

bitcoin/src/validation.cpp

Lines 2565 to 2569 in 82793f1

	static void AppendWarning(bilingual_str& res, const bilingual_str& warn)
	{
	if (!res.empty()) res += Untranslated(", ");
	res += warn;
	}

[pinheadmz]

ACK f0391cd

[ryanofsky]

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

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

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]

ACK f0391cd

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]

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]

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]

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

[jonatack]

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]

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

[jonatack]

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]

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]

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]

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

bitcoin/src/validation.cpp

Lines 2565 to 2569 in 82793f1

	static void AppendWarning(bilingual_str& res, const bilingual_str& warn)
	{
	if (!res.empty()) res += Untranslated(", ");
	res += warn;
	}

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

bitcoin/src/warnings.cpp

Lines 55 to 57 in 82793f1

	if (verbose) {
	return Join(warnings_verbose, Untranslated("<hr />"));
	}

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.

diff

--- a/src/warnings.cpp
+++ b/src/warnings.cpp
@@ -47,10 +47,14 @@ bilingual_str GetWarnings(bool verbose)
         warnings_verbose.emplace_back(warnings_concise);
     }
 
-    if (fLargeWorkInvalidChainFound) {
-        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.");
-        warnings_verbose.emplace_back(warnings_concise);
-    }
+    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.");
+    warnings_verbose.emplace_back(warnings_concise);
+
+    warnings_concise = _("Warning: Low on disk space.");
+    warnings_verbose.emplace_back(warnings_concise);
+
+    warnings_concise = _("Warning: System clock is off.");
+    warnings_verbose.emplace_back(warnings_concise);
 
     if (verbose) {
         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]

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.

[jonatack]

Superceded by #27279. Thanks everyone!