util: Use locale independent ToString(…) instead of locale dependent strprintf(…) for low-level string formatting #18450

Use locale independent ToString(…) instead of locale dependent strprintf(…) for low-level string formatting.

Context: See sipas comment in #18147 (comment).

Example taken from #18281:

Low level functions ScriptToAsmStr (core_io), i64tostr (strencodings) and itostr (strencodings) are locale dependent:

const std::vector<uint8_t> b{0x6a, 0x4, 0xff, 0xff, 0xff, 0xff};
const CScript script{b.begin(), b.end()};
for (const std::string& locale_string : {"C", "de_DE"}) {
    std::locale::global(std::locale(locale_string));
    std::cout << "[" << locale_string << "] ScriptToAsmStr(script, false) == " 
        << ScriptToAsmStr(script, false) << "\n";
}

[C] ScriptToAsmStr(script, false) == OP_RETURN -2147483647
[de_DE] ScriptToAsmStr(script, false) == OP_RETURN -2.147.483.647

Fixes #18281.

Rased on top of partly overlapping PR #18449 which I saw after submitting this PR :)

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #19064 (refactor: Cleanup thread ctor calls by hebasto)
• #18849 (The Zero Allocations project by jb55)
• #18772 (rpc: calculate fees in getblock using BlockUndo data by robot-visions)
• #18077 (net: Add NAT-PMP port forwarding support by hebasto)

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.

strprintf should not be locale dependent in the first place. It's never acceptable for it to be locale dependent. I think this is the wrong fix.

@laanwj I'm not sure I follow TBH.

I'm not sure if you are claiming that 1.) tinyformat.h (strprintf) isn't locale dependent, 2.) that you wish that tinyformat.h wasn't locale dependent, or 3.) if you are suggesting that we should assert that the global C++ locale is set in a way that strprintf isn't locale dependent in practice? :)

In other words: what would be the "correct" fix in your opinion? :)

FWIW:

$ cat tinyformat-locale.cpp
#include "tinyformat.h"

#include <iostream>
#include <locale>

int main(void)
{
    std::cout << strprintf("money printer go %d brrr", 1000000) << std::endl;
    std::locale::global(std::locale("de_DE"));
    std::cout << strprintf("gelddrucker gehen %d brrr", 1000000) << std::endl;
}
$ clang++ -o tinyformat-locale tinyformat-locale.cpp
$ ./tinyformat-locale
money printer go 1000000 brrr
gelddrucker gehen 1.000.000 brrr

@laanwj Also, is the claim about "wrong fix" referring to the tinyformat fix or all fixes in this PR? Please clarify - the review was a bit vague :)

Please also see #18432 which makes tinyformat.h (strprintf) locale independent. Perhaps that is the preferred fix?

I don't have any preference myself: I just want to kill this bug class once and for all :)

This needs rebase either way

@MarcoFalke Thanks! Rebased :)

@laanwj Friendly ping: could you please clarify your review feedback? :)

utACK

ACK 4cb9397088bf01bb5893bbcb705668894da118a6

Code review ACK 4cb9397

Re-opened thanks to renewed interest in the form of @jonatack's review and ACK :)

Unfortunately I had to rebase to resolve a conflict, so the three ACKs collected are now stale.

When rebasing I also added this case which makes this change complete across the code base:

diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index dda00f1fe..c6a6a7e8d 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -4222,7 +4222,7 @@ static UniValue upgradewallet(const JSONRPCRequest& request)
         "\nUpgrade the wallet. Upgrades to the latest version if no version number is specified\n"
         "New keys may be generated and a new wallet backup will need to be made.",
         {
-            {"version", RPCArg::Type::NUM, /* default */ strprintf("%d", FEATURE_LATEST), "The version number to upgrade to. Default is the latest wallet version"}
+            {"version", RPCArg::Type::NUM, /* default */ ToString(FEATURE_LATEST), "The version number to upgrade to. Default is the latest wallet version"}
         },
         RPCResults{},
         RPCExamples{

@luke-jr @MarcoFalke @jonatack - would you mind re-reviewing the rebased version and re-ACK if everything still looks good? :)

@laanwj Friendly ping: can you clarify your review feedback as requested above?

Generally I don't think it is fair to leave NACK style comments if one is unwilling to follow up when asked for clarification regarding the rationale for said NACK.