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

1. 
   practicalswift commented at 3:52 pm on March 27, 2020: contributor

   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:

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

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

   Fixes #18281.

2. practicalswift closed this on Mar 27, 2020

   ---

3. practicalswift reopened this on Mar 27, 2020

   ---

4. 
   practicalswift commented at 3:54 pm on March 27, 2020: contributor

   Rased on top of partly overlapping PR #18449 which I saw after submitting this PR :)
5. practicalswift force-pushed on Mar 27, 2020
6. DrahtBot added the label Mining on Mar 27, 2020
7. DrahtBot added the label P2P on Mar 27, 2020
8. DrahtBot added the label RPC/REST/ZMQ on Mar 27, 2020
9. DrahtBot added the label Tests on Mar 27, 2020
10. DrahtBot added the label Utils/log/libs on Mar 27, 2020
11. DrahtBot added the label Validation on Mar 27, 2020
12. DrahtBot added the label Wallet on Mar 27, 2020
13. 
    DrahtBot commented at 11:44 pm on March 27, 2020: member

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

    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.

14. 
    laanwj commented at 12:28 pm on April 1, 2020: member

    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.
15. 
    practicalswift commented at 1:52 pm on April 1, 2020: contributor

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

     0$ cat tinyformat-locale.cpp
     1#include "tinyformat.h"
     2
     3#include <iostream>
     4#include <locale>
     5
     6int main(void)
     7{
     8    std::cout << strprintf("money printer go %d brrr", 1000000) << std::endl;
     9    std::locale::global(std::locale("de_DE"));
    10    std::cout << strprintf("gelddrucker gehen %d brrr", 1000000) << std::endl;
    11}
    12$ clang++ -o tinyformat-locale tinyformat-locale.cpp
    13$ ./tinyformat-locale
    14money printer go 1000000 brrr
    15gelddrucker gehen 1.000.000 brrr
    

16. 
    practicalswift commented at 2:09 pm on April 1, 2020: contributor

    @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 :)
17. 
    practicalswift commented at 2:26 pm on April 1, 2020: contributor

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

18. 
    MarcoFalke commented at 3:42 pm on April 1, 2020: member

    This needs rebase either way
19. MarcoFalke removed the label Mining on Apr 1, 2020
20. MarcoFalke removed the label P2P on Apr 1, 2020
21. MarcoFalke removed the label RPC/REST/ZMQ on Apr 1, 2020
22. MarcoFalke removed the label Tests on Apr 1, 2020
23. MarcoFalke removed the label Validation on Apr 1, 2020
24. MarcoFalke removed the label Wallet on Apr 1, 2020
25. MarcoFalke added the label Refactoring on Apr 1, 2020
26. 
    practicalswift commented at 6:28 pm on April 1, 2020: contributor

    @MarcoFalke Thanks! Rebased :)
27. practicalswift force-pushed on Apr 1, 2020
28. in src/net.cpp:10 in ff7bcc757b outdated

     6@@ -7,19 +7,19 @@
     7 #include <config/bitcoin-config.h>
     8 #endif
     9 
    10-#include <net.h>
    

    ---

    ---

    

    MarcoFalke commented at 6:39 pm on April 1, 2020:

    the header of the own module is always included first on its own section

    ---

    

    practicalswift commented at 9:38 pm on April 2, 2020:

    Fixed!
29. practicalswift force-pushed on Apr 2, 2020
30. practicalswift force-pushed on Apr 2, 2020
31. 
    practicalswift commented at 11:17 am on April 7, 2020: contributor

    @laanwj Friendly ping: could you please clarify your review feedback? :)
32. luke-jr approved
33. 
    luke-jr commented at 0:06 am on April 23, 2020: member

    utACK
34. 
    MarcoFalke commented at 12:26 pm on April 27, 2020: member

    ACK 4cb9397088bf01bb5893bbcb705668894da118a6
35. practicalswift closed this on May 12, 2020

    ---

36. 
    jonatack commented at 9:10 am on May 12, 2020: member

    Code review ACK 4cb9397
37. practicalswift reopened this on May 12, 2020

    ---

38. practicalswift force-pushed on May 12, 2020
39. 
    practicalswift commented at 2:20 pm on May 12, 2020: contributor

    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:

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

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

40. util: Use locale independent ToString(…) instead of locale dependent strprintf(…) for low-level string formatting f808c232df
41. practicalswift force-pushed on May 12, 2020
42. 
    practicalswift commented at 7:27 am on May 21, 2020: contributor

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

43. practicalswift closed this on May 30, 2020

    ---

44. practicalswift deleted the branch on Apr 10, 2021
45. DrahtBot locked this on Aug 16, 2022

Contributors
practicalswift DrahtBot laanwj MarcoFalke luke-jr jonatack

Labels
Refactoring Utils/log/libs