doc: Document differences in bitcoind and bitcoin-qt locale handling

practicalswift commented at 3:25 pm on April 29, 2020: contributor

Document differences in bitcoind and bitcoin-qt locale handling.

Since this seems to be the root cause to the locale dependency issues we’ve seen over the years I thought it was worth documenting :)

Note that 1.) QLocale (used by Qt), 2.) C locale (used by locale-sensitive C standard library functions/POSIX functions and some parts of the C++ standard library such as std::to_string) and 3.) C++ locale (used by the C++ input/output library) are three separate things. This comment is about the perhaps surprising interference with the C locale (2) that takes place as part of the Qt initialization.

DrahtBot added the label Docs on Apr 29, 2020

DrahtBot added the label GUI on Apr 29, 2020

hebasto commented at 2:22 am on May 3, 2020: member

Concept ACK.

Mind adding a link: https://doc.qt.io/qt-5/qcoreapplication.html#locale-settings or https://doc.qt.io/qt-5/qcoreapplication.html ?

I’m confused a bit with wording “surprisingly” as such behavior is mentioned in Qt docs :)

UPDATE: https://stackoverflow.com/questions/34877942/is-qt-forcing-the-system-locale could be relevant. Related to #18147.

practicalswift force-pushed on May 3, 2020

practicalswift commented at 5:59 am on May 3, 2020: contributor

@hebasto Thanks a lot for reviewing! Good points: I’ve adjusted accordingly. Please re-review :)

hebasto commented at 0:07 am on May 4, 2020: member

@practicalswift Why do you think that the BitcoinApplication constructor is a proper place to document locale issues? Did you consider other options, e.g., doc/ directory?

practicalswift commented at 1:57 pm on May 4, 2020: contributor

@hebasto I put the comment in the ctor where the QApplication call is made and thus where the implicit setlocale invocation is made, but I don’t have any strong opinion about where to put it. You suggestion is putting it in the developer notes instead? I can do that :)

I’m fine as long as we have it documented somewhere so that future generations of Bitcoin Core developers don’t have to wonder where the heck the “invisible” setlocale call is made that is the root cause of all locale issues :)

hebasto commented at 1:59 pm on May 4, 2020: member

@hebasto I put the comment in the ctor where the QApplication call is made and thus where the implicit setlocale invocation is made, but I don’t have any strong opinion about where to put it. You suggestion is putting it in the developer notes instead? I can do that :)

It seems better to get a broader consensus :)

DrahtBot commented at 7:17 pm on May 29, 2020: member

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

Conflicts

No conflicts as of last run.

DrahtBot added the label Needs rebase on Jun 2, 2020

practicalswift force-pushed on Jun 2, 2020

DrahtBot removed the label Needs rebase on Jun 2, 2020

fanquake added this to the "Blockers" column in a project

in src/qt/bitcoin.cpp:206 in 52d83bf9cc outdated

200@@ -201,6 +201,36 @@ BitcoinApplication::BitcoinApplication(interfaces::Node& node):
201     returnValue(0),
202     platformStyle(nullptr)
203 {
204+    // Note about QApplication(...)'s impact on locale dependent non-Qt functions:
205+    //
206+    // Note that Qt runs setlocale(LC_ALL, "") on initialization. This

jonatack commented at 10:57 am on July 3, 2020:

Can remove “Note that” and begin with “Qt runs”.

in src/qt/bitcoin.cpp:204 in 52d83bf9cc outdated

200@@ -201,6 +201,36 @@ BitcoinApplication::BitcoinApplication(interfaces::Node& node):
201     returnValue(0),
202     platformStyle(nullptr)
203 {
204+    // Note about QApplication(...)'s impact on locale dependent non-Qt functions:

jonatack commented at 10:57 am on July 3, 2020:

s/locale dependent/locale-dependent/

in src/qt/bitcoin.cpp:210 in 52d83bf9cc outdated

205+    //
206+    // Note that Qt runs setlocale(LC_ALL, "") on initialization. This
207+    // installs the locale specified by the user's LC_ALL (or LC_*) environment
208+    // variable as the new C locale.
209+    //
210+    // In contrast bitcoind does not opt-in to localization -- no call to

jonatack commented at 10:58 am on July 3, 2020:

s/In contrast/In contrast,/

s/opt-in/opt in/ (with the hyphen it is an adjectif; here you are using it as a verb)

in src/qt/bitcoin.cpp:214 in 52d83bf9cc outdated

209+    //
210+    // In contrast bitcoind does not opt-in to localization -- no call to
211+    // setlocale(LC_ALL, "") is made and the environment variables LC_* are
212+    // thus ignored.
213+    //
214+    // This results in situation where bitcoind is guaranteed to be running

jonatack commented at 11:00 am on July 3, 2020:

s/situation/situations/

jonatack commented at 11:07 am on July 3, 2020: member

Concept ACK.

Perhaps add a paragraph similar to the second one in your PR description.

I’m not sure, but this might be a bit long for the developer notes. Maybe either in the codebase like here, or in its own file in /doc, the main thing being that a developer git grepping on the topic finds it easily.

Some suggestions below; feel free to pick and choose or ignore.

practicalswift force-pushed on Jul 3, 2020

practicalswift commented at 12:10 pm on July 3, 2020: contributor

@jonatack Thanks for excellent and detailed copy review! ❤️ Updated accordingly. @jonatack @hebasto I’ve now added a small note in the developer notes that refers to the more detailed description in the code comment. Looks good? :)

practicalswift commented at 9:20 pm on July 9, 2020: contributor

The CI failure is most likely unrelated. Can someone restart the Cirrus CI job? :)

fjahr commented at 8:35 pm on July 15, 2020: member

Concept ACK

FWIW, I agree with @hebasto and would only put a much shorter version of the comment in the code. My perspective is that a lot of people look at src/qt/bitcoin.cpp but only very few of them care about localization. While it may be the best place to put it technically, it feels oddly specific there. A short hint and the link to the docs would be enough to let people know what is happening. The long-form could go into the developer notes or another place in the docs IMO.

practicalswift force-pushed on Jul 29, 2020

practicalswift commented at 5:19 pm on July 29, 2020: contributor

@hebasto @jonatack @fjahr Thanks a lot for reviewing. Feedback addressed: the documentation has now been moved to a more appropriate home (test/lint/lint-locale-dependence.sh). Should be ready for final review :)

hebasto approved

hebasto commented at 1:07 pm on July 30, 2020: member

ACK 26fa24996e18b9ecaa1b21fcfd049af6c468d59d, I have reviewed the code and it looks OK, I agree it can be merged.

fjahr commented at 1:25 pm on July 30, 2020: member

ACK 26fa24996e18b9ecaa1b21fcfd049af6c468d59d

Restarted failing CI job.

practicalswift commented at 4:21 pm on August 3, 2020: contributor

I think this one should be ready for merge now :)

practicalswift commented at 5:58 am on August 14, 2020: contributor

@fanquake I think two ACK:s (hebasto, fjahr) and one Concept ACK (jonatack) should make this totally uncontroversial documentation only change ready for merge. What do you think? :)

practicalswift commented at 7:35 pm on August 26, 2020: contributor

Ready for merge? :)

MarcoFalke commented at 2:59 pm on August 27, 2020: member

Since this seems to be the root cause to the locale dependency issues we’ve seen over the years I thought it was worth documenting :)

Do you have one example handy where the different handling was the root cause for an issue?

Also I am wondering what the motivation here is. While this difference in handling may be theoretically correct, we should never obverse it in practice. If we did, it would be a bug, as any locale dependent functions should be avoided.

practicalswift commented at 7:25 pm on August 27, 2020: contributor

@MarcoFalke

Do you have one example handy where the different handling was the root cause for an issue?

There are plenty but look at #6443: bug in Bitcoin Qt due to Qt’s highly surprising “unsolicited” setlocale call on initialization. Not a bug in bitcoind due to the discussed difference.

The thing is that literally all locale bugs when using POSIX functions have been due to this difference. If it wasn’t for the setlocale call which Qt does as part of initialization we wouldn’t have seen a single POSIX locale bug.

We could literally kill the locale dependency bug class for good in Bitcoin Core by simply following the recommendation of the Qt documentation:

On Unix/Linux Qt is configured to use the system locale settings by default. This can cause a conflict when using POSIX functions, for instance, when converting between data types such as floats and strings, since the notation may differ between locales. To get around this problem, call the POSIX function setlocale(LC_NUMERIC,“C”) right after initializing QApplication, QGuiApplication or QCoreApplication to reset the locale that is used for number formatting to “C”-locale. [https://doc.qt.io/qt-5/qcoreapplication.html]

(Aside: AFAICT there is not a single place in our code base where we want the POSIX functions to use a locale other than C when parsing or formatting numbers. (Obviously we want Qt to be localized but that is handled by QLocale.))

So, why documenting this?

A new developer coming to the project doing git grep setlocale won’t see a single place (outside of the fuzz tests) where we opt in to POSIX locale dependence.

He or she might thus incorrectly assume that we don’t risk running in to POSIX locale bugs.

If this PR is merged he/she will hopefully fully understand why we run such risk despite the apparent lack of setlocale call in our code base.

While this difference in handling may be theoretically correct, we should never obverse it in practice.

If consensus critical code would use a POSIX locale dependent function the worst case scenario would be a chain split between bitcoind clients and a subset of Bitcoin Qt clients (depending on their locale). For bitcoind the POSIX locale is guaranteed to be C whereas it can be whatever funky locale the user chooses in Bitcoin Qt.

Such a worst-case chain split would certainly be observable in practice :)

If we did, it would be a bug, as any locale dependent functions should be avoided.

I share the long-term goal of removing all usages of locale dependent functions, but I don’t see how that would be in conflict with wanting to document how locales are currently used in our project, and the risks such use entails :)

Should I perhaps drop or rephrase the developer note comment?

As long as we document this somehow and somewhere in a greppable way in our project I’m happy :)

MarcoFalke commented at 2:05 pm on August 28, 2020: member

Ok, sounds reasonable. About the dev notes: I see them mostly as instructions on how we write code. “Avoid using locale dependent functions if possible” is already mentioned and the qt documentation seems a bit misplaced.

Concept ACK

doc: Document differences in bitcoind and bitcoin-qt locale handling ca185cf5a1

practicalswift force-pushed on Aug 29, 2020

practicalswift commented at 1:57 am on August 29, 2020: contributor

@MarcoFalke Thanks for reviewing! No longer touching the developer notes. Should hopefully be ready for merge :)

hebasto approved

hebasto commented at 7:49 am on August 29, 2020: member

re-ACK ca185cf5a14b16d61814d7172284bc8efcd28b69

MarcoFalke merged this on Aug 29, 2020

MarcoFalke closed this on Aug 29, 2020

sidhujag referenced this in commit 085db7ad6b on Aug 30, 2020

fanquake removed this from the "Blockers" column in a project

practicalswift deleted the branch on Apr 10, 2021

Fabcien referenced this in commit 59661fd89e on Sep 17, 2021

DrahtBot locked this on Aug 18, 2022

doc: Document differences in bitcoind and bitcoin-qt locale handling #18817

Conflicts