I'm not sure whether it's better to crash than start with a "wrong" locale. We're already avoiding using locale-dependent functions in pretty much all important places.

Edit: also, why are our assumptions different for bitcoin-qt and bitcoind?

@laanwj I think you misunderstand the patch. Note that currently there is no way for a user to make bitcoind or bitcoin-qt start with the "wrong" locale ("wrong" as in that the assertions in this PR does not hold).

This is just a sanity check that makes sure that the assumption we know to be true today remain true also in the future. Like ECC_InitSanityCheck, Random_SanityCheck, etc it tests that our assumptions hold true also in practice at run-time. What risks do you see?

This is just my personal opinion but I think this adds some quite ugly code into our already convoluted initialization process, using weird functions such as setlocale. I'd prefer to avoid it and focus on getting rid of locale dependent functions where it's a problem.

If we decide to do this, I'd prefer to add the common checks to the other sanity checks and not distinguish between bitcoind.cpp/qt/bitcoin.cpp.

@laanwj How would you suggest checking for the assumed locales in a less ugly way?

How do you suggest reading the C locale without using setlocale? (Please note that the C locale is not the same thing as the C++ locale.)

If we decide to do this, I'd prefer to add the common checks to the other sanity checks and not distinguish between bitcoind.cpp/qt/bitcoin.cpp.

But we're making different assumptions for bitcoind and bitcoin-qt, no?

As the code is written we apparently want the C locale to be user-defined in bitcoin-qt but not in bitcoind. That is how it is done now in practice at least.

Perhaps I should have included a few examples in the PR title. The gotcha list could be made very long, but this is a C locale gotcha example :)

$ cling
[cling]$ #include <clocale>
[cling]$ #include <string>
[cling]$ std::string current_locale = {setlocale(LC_ALL, nullptr)}
(std::string &) "C"
[cling]$ std::to_string(1.23)
(std::string) "1.230000"
[cling]$ setlocale(LC_ALL, "de_DE");
[cling]$ std::to_string(1.23)
(std::string) "1,230000"

Perhaps surprisingly std::to_string cares about the C locale and not the global C++ locale.

And a C++ locale gotcha:

$ cling
[cling]$ #include <locale>
[cling]$ #include <sstream>
[cling]$ std::locale{}.name()
(std::string) "C"
[cling]$ std::ostringstream oss1;
[cling]$ oss1 << 1000000;
[cling]$ std::string s1 = oss1.str()
(std::string &) "1000000"
[cling]$ std::locale::global(std::locale("de_DE"));
[cling]$ std::ostringstream oss2;
[cling]$ oss2 << 1000000;
[cling]$ std::string s2 = oss2.str()
(std::string &) "1.000.000"

More C locale WTF:

$ uname -s
Darwin
$ cat poc.cpp
#include <iostream>
#include <locale>

int main(void) {
    setlocale(LC_ALL, "");
    std::cout << std::isspace(133) << ' ' << std::isspace(154) << ' ' << std::isspace(160);
    std::cout << '\n';
}
$ clang++ -o poc poc.cpp
$ ./poc
1 0 1
$ LC_ALL=en_US ./poc
1 0 1
$ LC_ALL=C ./poc
0 0 0
$ LC_ALL=ru_RU.KOI8-R ./poc # an "interesting" locale
0 1 0

Let me know when I've made my case sufficiently clear :)

@practicalswift You're making the case that locales are annoying, but we already knew that.

I think what @laanwj means is that he'd rather just avoid any locale-dependent functionality in the code (to make the locale settings irrelevant) as opposed to forcing the locale to a known value (correct me if I'm wrong of course).

-0 I agree with @laanwj let's remove locale dependence rather than reduce the set of configurations we can run against.

@Empact

-0 I agree with @laanwj let's remove locale dependence rather than reduce the set of configurations we can run against.

I don't follow. What user configuration was possible before this PR that is not possible after this PR? Can you give a specific example?

@sipa

I think what @laanwj means is that he'd rather just avoid any locale-dependent functionality in the code (to make the locale settings irrelevant) as opposed to forcing the locale to a known value (correct me if I'm wrong of course).

Note that this PR simply documents and asserts restrictions we already have in place. More specifically it does not disallow any configuration that is currently allowed. (Please correct me if you think I'm wrong.)

I've worked a lot on removing the use of locale-dependent functions and I fail to see how that effort would be in conflict with documenting and asserting the currently made locale assumptions.

It is my understanding the following are the only possible effective locale combinations before and after this PR:

Is that your understanding as well?

Perhaps the suggestion is that we might want to change the current locale logic and allow for bitcoind to run with a C locale (setlocale) or global C++ locale (std::locale) other than the classic locale in the future? That sounds really risky. The mere thought of an accidental fork/network split made possible by a locale dependency bug in combination with the fact that we would allow running bitcoind which an effective non-classic C or C++ locale (think zh_CN!) gives me nightmares :)

Again, please correct me if my reasoning is wrong. I could be missing something :)

Recommended reading to better understand the motivation behind this PR: "Differences between the C Locale and the C++ Locales"

No, what I'm trying to say is simply that if we don't have locale-dependent functions, this should be unnecessary complexity.

@sipa

It is my understanding the following are the only possible effective locale combinations before and after this PR:

Is that your understanding as well?

I don't understand what you mean by "effective locale". The locale shouldn't matter, so for all we care they're both always effectively equal to "foobar".

@sipa Sorry for my sloppy wording: with "effective locale" I simply meant the current locale in use (the C locale and the global C++ locale).

More specifically the following:

const std::locale current_cpp_locale;
const std::string current_c_locale{setlocale(LC_ALL, nullptr)};

I claim the following is guaranteed to hold true no matter user configuration:

To make it even more explicit - it is my understanding that the following assertions are guaranteed to hold for the entire lifetime of the processes regardless of user configuration and/or what the user has in his/hers LC_ALL:

For bitcoind:

assert(std::locale{}.name() == "C");
const std::string current_c_locale = {setlocale(LC_ALL, nullptr)};
assert(current_c_locale == "C");

std::ostringstream oss1;
oss1 << 1000000;
assert(oss1.str() == "1000000");
assert(std::to_string(1.23) == "1.230000");

For bitcoin-qt:

assert(std::locale{}.name() == "C");

std::ostringstream oss1;
oss1 << 1000000;
assert(oss1.str() == "1000000");

Is that your understanding as well? :)

Before this PR, those assertions obviously don't hold? I'm very confused.

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• Avoid using locale-dependent boost trim functions in RPCAuthorized(…) and bitcoin-tx #18539 (Avoid using locale-dependent boost trim functions in RPCAuthorized(…) and bitcoin-tx by practicalswift)
• Replace uses of boost::trim* with locale-independent alternatives #18130 (Replace uses of boost::trim* with locale-independent alternatives by Empact)

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.

Before this PR, those assertions obviously don't hold? I'm very confused.

Then I'm confused by your confusion :)

What assertion are you claiming not to hold before this PR? The bitcoind or the bitcoin-qt assertions or all of them? A single counter-example would settle this. What user configuration should I run to make the assertions fail?

Very interesting: it seems like one of us will walk away from here with a "TIL moment" :) Very exciting regardless of outcome! :)

FWIW:

$ git diff
diff --git a/src/logging.h b/src/logging.h
index b2fde1b9e..6b3bc5032 100644
--- a/src/logging.h
+++ b/src/logging.h
@@ -10,9 +10,12 @@
 #include <tinyformat.h>
 
 #include <atomic>
+#include <clocale>
 #include <cstdint>
 #include <list>
+#include <locale>
 #include <mutex>
+#include <sstream>
 #include <string>
 #include <vector>
 
@@ -162,6 +165,15 @@ bool GetLogCategory(BCLog::LogFlags& flag, const std::string& str);
 template <typename... Args>
 static inline void LogPrintf(const char* fmt, const Args&... args)
 {
+    std::ostringstream oss1;
+    oss1 << 1000000;
+    assert(oss1.str() == "1000000");
+    assert(std::to_string(1.23) == "1.230000");
+
+    assert(std::locale{}.name() == "C");
+    std::string current_locale = {setlocale(LC_ALL, nullptr)};
+    assert(current_locale == "C");
+
     if (LogInstance().Enabled()) {
         std::string log_msg;
         try {
$ make
$ LC_ALL=de_DE src/bitcoind
… note the absence of assertion failures …

std::to_string(1.23) won't always be "1.230000" with the current code, if LC_ALL isn't set to C?

@sipa

As you probably know the equivalent of std::setlocale(LC_ALL, "C"); is always executed before main is run.

I think you're incorrectly assuming that we are doing std::setlocale(LC_ALL, ""); in the bitcoind code (which would set the locale to whatever the user has set the environment variable LC_ALL to) .

Could that be the case?

No, I'm not assuming any setlocale call. I'm saying that before this PR, there is no such call, and thus std::to_string(1.23) right now won't be guaranteed to be "1.230000".

@sipa

Sounds like you're incorrectly assuming that setlocale(LC_ALL, nullptr); sets the locale. Could that be the case? Note that setlocale(LC_ALL, nullptr); is not equivalent to setlocale(LC_ALL, "");.

I claim that std::to_string(1.23) right now (prior to this PR) is guaranteed to be "1.230000" during the entire lifetime of the bitcoind process regardless of the user's configuration and/or his/her LC_ALL environment variable.

If you still don't think that is the case then I kindly ask you to provide a counter-example in the form of a configuration and/or diff. I would love to be proved wrong and walk away with a TIL experience :)

FWIW:

SYNOPSIS
       #include <locale.h>

       char *setlocale(int category, const char *locale);

DESCRIPTION
       The setlocale() function is used to set or query the program's current locale.

       If  locale is not NULL, the program's current locale is modified according
       to the arguments.

       […]

       If locale is NULL, the current locale is only queried, not modified.

       […]

       On startup of the main program, the portable "C" locale is selected as default.
       A program may be made portable to all locales by calling:

           setlocale(LC_ALL, "");

       after program initialization […]

Aha, I was indeed making the assumption that setlocale(?, nullptr) was changing the locale.

In that case, I have another concern: doesn't this break running bitcoind with locale env variables set to anything but LC_ALL?

@sipa

In that case, I have another concern: doesn't this break running bitcoind with locale env variables set to anything but LC_ALL?

It sounds like you're incorrectly assuming that any of the LC_* environment variables will somehow control the C and C++ locales used without any explicit call to setlocale(LC_*, "") being made in the user code. Could that be the case?

As you probably know the equivalent of setlocale(LC_ALL, "C") is called automatically in all applications before main.

Thus when the execution of main begins the C locale and the global C++ locale are both guaranteed to equal to the classic locale (a.k.a. the "C" locale).

Without any explicit call to setlocale(LC_*, "") in the code the default "C" locale will be in use throughout the process lifetime no matter what the LC_* environment variables are set to.

Let's demonstrate this with a small test program:

#include <clocale>
#include <cstdlib>
#include <iostream>
#include <locale>
#include <sstream>
#include <string>

int main(void)
{
    std::cout << "C locale at beginning of main: " << std::string{setlocale(LC_ALL, nullptr)} << "\n";
    std::cout << "C++ locale at beginning of main: " << std::locale{}.name() << "\n\n";
#if OPT_IN_TO_LOCALIZATION_USING_SET_LOCALE
    setlocale(LC_ALL, "");
#endif
#if OPT_IN_TO_LOCALIZATION_USING_STD_LOCALE_GLOBAL
    std::locale::global(std::locale(""));
#endif
    std::ostringstream oss;
    oss << 1000000;
    std::cout << "std::ostringstream oss; oss << 1000000; oss.str() equals " << oss.str() << "\n";
    std::cout << "std::to_string(1.23) equals " << std::to_string(1.23) << "\n\n";
    std::cout << "C locale at end of main: " << std::string{setlocale(LC_ALL, nullptr)} << "\n";
    std::cout << "C++ locale at end of main: " << std::locale{}.name() << "\n\n";
    std::cout << "setlocale(LC_COLLATE,  nullptr) (read-only operation) = " << std::string{setlocale(LC_COLLATE, nullptr)} << "\n";
    std::cout << "setlocale(LC_CTYPE,    nullptr) (read-only operation) = " << std::string{setlocale(LC_CTYPE, nullptr)} << "\n";
    std::cout << "setlocale(LC_MESSAGES, nullptr) (read-only operation) = " << std::string{setlocale(LC_MESSAGES, nullptr)} << "\n";
    std::cout << "setlocale(LC_MONETARY, nullptr) (read-only operation) = " << std::string{setlocale(LC_MONETARY, nullptr)} << "\n";
    std::cout << "setlocale(LC_NUMERIC,  nullptr) (read-only operation) = " << std::string{setlocale(LC_NUMERIC, nullptr)} << "\n";
    std::cout << "setlocale(LC_TIME,     nullptr) (read-only operation) = " << std::string{setlocale(LC_TIME, nullptr)} << "\n";
}

Let's try with LC_ALL=de_DE ./l8n without any opt-in to locale dependence:

$ clang++ -o l8n l8n.cpp
$ LC_ALL=de_DE ./l8n
C locale at beginning of main: C
C++ locale at beginning of main: C

std::ostringstream oss; oss << 1000000; oss.str() equals 1000000
std::to_string(1.23) equals 1.230000

C locale at end of main: C
C++ locale at end of main: C

setlocale(LC_COLLATE,  nullptr) (read-only operation) = C
setlocale(LC_CTYPE,    nullptr) (read-only operation) = C
setlocale(LC_MESSAGES, nullptr) (read-only operation) = C
setlocale(LC_MONETARY, nullptr) (read-only operation) = C
setlocale(LC_NUMERIC,  nullptr) (read-only operation) = C
setlocale(LC_TIME,     nullptr) (read-only operation) = C

Let's try with LC_NUMERIC=de_DE ./l8n without any opt-in to locale dependence:

$ clang++ -o l8n l8n.cpp
$ LC_NUMERIC=de_DE ./l8n
C locale at beginning of main: C
C++ locale at beginning of main: C

std::ostringstream oss; oss << 1000000; oss.str() equals 1000000
std::to_string(1.23) equals 1.230000

C locale at end of main: C
C++ locale at end of main: C

setlocale(LC_COLLATE,  nullptr) (read-only operation) = C
setlocale(LC_CTYPE,    nullptr) (read-only operation) = C
setlocale(LC_MESSAGES, nullptr) (read-only operation) = C
setlocale(LC_MONETARY, nullptr) (read-only operation) = C
setlocale(LC_NUMERIC,  nullptr) (read-only operation) = C
setlocale(LC_TIME,     nullptr) (read-only operation) = C

Let's try LC_ALL=de_DE ./l8n with opt-in to locale dependence using set_locale:

$ clang++ -DOPT_IN_TO_LOCALIZATION_USING_SET_LOCALE -o l8n l8n.cpp
$ LC_ALL=de_DE ./l8n
C locale at beginning of main: C
C++ locale at beginning of main: C

std::ostringstream oss; oss << 1000000; oss.str() equals 1000000
std::to_string(1.23) equals 1,230000

C locale at end of main: de_DE
C++ locale at end of main: C

setlocale(LC_COLLATE,  nullptr) (read-only operation) = de_DE
setlocale(LC_CTYPE,    nullptr) (read-only operation) = de_DE
setlocale(LC_MESSAGES, nullptr) (read-only operation) = de_DE
setlocale(LC_MONETARY, nullptr) (read-only operation) = de_DE
setlocale(LC_NUMERIC,  nullptr) (read-only operation) = de_DE
setlocale(LC_TIME,     nullptr) (read-only operation) = de_DE

Let's try LC_ALL=de_DE ./l8n with opt-in to locale dependence using std::locale::global:

$ clang++ -DOPT_IN_TO_LOCALIZATION_USING_STD_LOCALE_GLOBAL -o l8n l8n.cpp
$ LC_ALL=de_DE ./l8n
C locale at beginning of main: C
C++ locale at beginning of main: C

std::ostringstream oss; oss << 1000000; oss.str() equals 1.000.000
std::to_string(1.23) equals 1,230000

C locale at end of main: de_DE
C++ locale at end of main: de_DE

setlocale(LC_COLLATE,  nullptr) (read-only operation) = de_DE
setlocale(LC_CTYPE,    nullptr) (read-only operation) = de_DE
setlocale(LC_MESSAGES, nullptr) (read-only operation) = de_DE
setlocale(LC_MONETARY, nullptr) (read-only operation) = de_DE
setlocale(LC_NUMERIC,  nullptr) (read-only operation) = de_DE
setlocale(LC_TIME,     nullptr) (read-only operation) = de_DE

QED? :)

Oh, I'm indeed learning. I assumed that the LC_ environment variables automatically affect all programs.

So what is this testing? It seems to verify things that are guaranteed by the language?

@sipa The only thing guaranteed by the language is that a C++ program has the C and C++ locales set to the classic locale when entering main. After that the user code is free to mess with the locale as it seen fit: hence the need to verify that the assumption we make hold true :)

There are quite a few gotchas that are worth guarding against in my opinion:

A common gotcha is that setlocale(LC_ALL, "") (write) where setlocale(LC_ALL, nullptr) (read) was intended.

Another common gotcha is libraries calling setlocale(LC_ALL, "") without the application author being aware of it which of course messes with the locale of the entire application in unexpected ways.

I think we have that problem today in the form of Qt which runs std::setlocale(LC_ALL, "") as part of its setup process. That is why the reported locale issues historically have popped up in bitcoin-qt and not in bitcoind AFAICT.

To make things even more unpredictable some versions of Qt messes with all locale categories whereas other versions only messes with a subset of locale categories. I believe a major locale handling change was made in Qt 4.4 for example.

Some of our stuff, like bitcoin-qt and bitcoin-wallet should use locales...

@luke-jr

We produce the following five binaries:

• bitcoin-cli
• bitcoin-qt
• bitcoin-tx
• bitcoin-wallet
• bitcoind

Which of them should be localized and which should not be localized?

@luke-jr Could you clarify your comment by answering the last question? :)

@practicalswift Purely user-facing graphical displays should be localized

@MarcoFalke bitcoin-qt is the only of the five binaries producing purely user-facing graphical displays AFAICT, so bitcoin-qt is thus the only application that should be localized. Correct? :)

bitcoin-qt is the only of the five binaries producing purely user-facing graphical displays AFAICT, so bitcoin-qt is thus the only application that should be localized. Correct? :)

I think it's wrong to think about binaries being localized or not.

The user-interfacing code in bitcoin-qt should/may be localized, but other code in bitcoin-qt (e.g. the RPC code in it) still shouldn't.

bitcoin-qt is the only of the five binaries producing purely user-facing graphical displays AFAICT, so bitcoin-qt is thus the only application that should be localized. Correct? :)

I think it's wrong to think about binaries being localized or not.

The user-interfacing code in bitcoin-qt should/may be localized, but other code in bitcoin-qt (e.g. the RPC code in it) still shouldn't.

I think our statements can be combined to a meaningful policy:

• Code in bitcoin-cli, bitcoin-tx, bitcoin-wallet and bitcoind must not be localized.
• Purely user-facing graphical displays in bitcoin-qt should/may be localized, but all other code in bitcoin-qt must not be localized.

Or alternatively:

• Code in the project must not be localized with the exception of purely user-facing graphical displays in Qt related code which should/may be localized.

Sounds good? :)

@practicalswift That sounds right; I don't think any of that is related to the changes in this PR.

Closing this PR: consensus opinion seems to be that the locale assumptions we're are making implicitly are not worth asserting explicitly :)