util: Use locale independent ToString(…) instead of locale dependent strprintf(…) for low-level string formatting by practicalswift · Pull Request #18450 · bitcoin/bitcoin

practicalswift · 2020-03-27T15:52:00Z

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.

practicalswift · 2020-03-27T15:54:33Z

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

DrahtBot · 2020-03-27T23:44:55Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: Cleanup thread ctor calls #19064 (refactor: Cleanup thread ctor calls by hebasto)
The Zero Allocations project #18849 (The Zero Allocations project by jb55)
rpc: calculate fees in getblock using BlockUndo data #18772 (rpc: calculate fees in getblock using BlockUndo data by robot-visions)
net: Add NAT-PMP port forwarding support #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.

laanwj · 2020-04-01T12:28:40Z

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.

practicalswift · 2020-04-01T13:52:38Z

@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

practicalswift · 2020-04-01T14:09:52Z

@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 :)

practicalswift · 2020-04-01T14:26:19Z

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

maflcko · 2020-04-01T15:42:06Z

This needs rebase either way

practicalswift · 2020-04-01T18:28:37Z

@MarcoFalke Thanks! Rebased :)

src/net.cpp

practicalswift · 2020-04-07T11:17:50Z

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

luke-jr

utACK

maflcko · 2020-04-27T12:26:38Z

ACK 4cb9397088bf01bb5893bbcb705668894da118a6

jonatack

Code review ACK 4cb9397

practicalswift · 2020-05-12T14:20:04Z

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? :)

…strprintf(…) for low-level string formatting

practicalswift · 2020-05-21T07:27:50Z

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

practicalswift closed this Mar 27, 2020

practicalswift reopened this Mar 27, 2020

practicalswift force-pushed the locale-independence branch from fe276fd to 09eecba Compare March 27, 2020 16:09

DrahtBot added Mining P2P RPC/REST/ZMQ Tests Utils/log/libs Validation Wallet labels Mar 27, 2020

This was referenced Mar 28, 2020

Serve BIP 157 compact filters #16442

Closed

net: Add NAT-PMP port forwarding support #18077

Merged

maflcko added Refactoring and removed Mining P2P RPC/REST/ZMQ Tests Validation Wallet labels Apr 1, 2020

practicalswift force-pushed the locale-independence branch from 09eecba to ff7bcc7 Compare April 1, 2020 18:28

maflcko reviewed Apr 1, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

practicalswift force-pushed the locale-independence branch from ff7bcc7 to 7dcaf6e Compare April 2, 2020 21:36

practicalswift force-pushed the locale-independence branch from 7dcaf6e to 4cb9397 Compare April 2, 2020 21:37

DrahtBot mentioned this pull request Apr 9, 2020

[RFC] bitcoin-asmap utility #18573

Closed

luke-jr approved these changes Apr 23, 2020

View reviewed changes

DrahtBot mentioned this pull request May 2, 2020

The Zero Allocations project #18849

Closed

4 tasks

DrahtBot mentioned this pull request May 10, 2020

net: use CMessageHeader::HEADER_SIZE, add missing include #18931

Merged

practicalswift closed this May 12, 2020

jonatack reviewed May 12, 2020

View reviewed changes

practicalswift reopened this May 12, 2020

practicalswift force-pushed the locale-independence branch from 4cb9397 to 3948a06 Compare May 12, 2020 14:17

util: Use locale independent ToString(…) instead of locale dependent …

f808c23

…strprintf(…) for low-level string formatting

practicalswift force-pushed the locale-independence branch from 3948a06 to f808c23 Compare May 12, 2020 14:21

DrahtBot mentioned this pull request May 17, 2020

rpc: calculate fees in getblock using BlockUndo data #18772

Merged

maflcko mentioned this pull request May 19, 2020

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

Closed

DrahtBot mentioned this pull request May 20, 2020

Implement ADDRv2 support (part of BIP155) #19031

Closed

11 tasks

DrahtBot mentioned this pull request May 28, 2020

refactor: Cleanup thread ctor calls #19064

Merged

practicalswift closed this May 30, 2020

practicalswift deleted the locale-independence branch April 10, 2021 19:41

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Conversation

practicalswift commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented Apr 1, 2020

Uh oh!

practicalswift commented Apr 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Apr 1, 2020

Uh oh!

practicalswift commented Apr 1, 2020

Uh oh!

maflcko commented Apr 1, 2020

Uh oh!

practicalswift commented Apr 1, 2020

Uh oh!

Uh oh!

practicalswift commented Apr 7, 2020

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Apr 27, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented May 12, 2020

Uh oh!

practicalswift commented May 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

practicalswift commented Mar 27, 2020 •

edited

Loading

practicalswift commented Mar 27, 2020 •

edited

Loading

DrahtBot commented Mar 27, 2020 •

edited

Loading

practicalswift commented Apr 1, 2020 •

edited

Loading

practicalswift commented May 21, 2020 •

edited

Loading