Low level functions ScriptToAsmStr (core_io), i64tostr (strencodings) and itostr (strencodings) are locale dependent #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";
    std::cout << "[" << locale_string << "] i64tostr(12345678) == " 
        << i64tostr(12345678) << "\n";
    std::cout << "[" << locale_string << "] itostr(12345678) == " 
        << itostr(12345678) << "\n";
}

[C] ScriptToAsmStr(script, false) == OP_RETURN -2147483647
[C] i64tostr(12345678) == 12345678
[C] itostr(12345678) == 12345678
[de_DE] ScriptToAsmStr(script, false) == OP_RETURN -2.147.483.647
[de_DE] i64tostr(12345678) == 12.345.678
[de_DE] itostr(12345678) == 12.345.678

I can't reproduce with bitcoind or bitcoin-qt.

@MarcoFalke That's because the C++ locale is currently not reflecting LC_* which appears to be by pure luck and not thanks to a conscious decision: see the comments in PR [#18124](/bitcoin-bitcoin/18124/) for context. Please read the entire discussion to get all the nuances and gotchas (there are quite a few of them!) :)

The comments in the related PR is #18147 further highlights the confusion regarding what we assume to hold true with regards to locales in the project.

Note that in bitcoin-qt the C locale (setlocale) is reflecting LC_*, but the C++ locale (std::locale::global) is not.

If we consider ScriptToAsmStr, i64tostr and itostr to be safe as written now that implies that we assume that the C++ locale never being set to anything but C (the classic locale).

If we assume the C++ locale to always be C then we should state that explicitly and also assert it as runtime as suggested in #18124.

The current non-principled "whack-a-mole" approach to locale handling seems fragile at best :)

This one could be marked "good first issue": it should be trivial to fix.

@practicalswift "good first issue" is only for issues where one (and only one) uncontroversial solution exists.

Also i64tostr doesn't exist in this repo. I think the issue has already been solved?

@MarcoFalke Yes, i64tostr and itostr are gone, but ScriptToAsmStr is still doing strprintf("%d", CScriptNum(vch, false).getint()); which is 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

:)

yeah, and that is fixed by #18450