ParseDouble() is faulty, depending on locale

dexX7 commented at 7:58 pm on July 15, 2015: contributor

I tested the current master, and when I used the QT client, then a lot of RPC calls returned errors with “JSON double out of range”. This wasn’t the case, when using bitcoind however, at least for me.

It turns out this seems to be another locale related issue: https://github.com/bitcoin/bitcoin/commit/7e98a3c642222edc0813ced945d4b6e548cb8ca8 added ParseDouble(), which uses strtod.

strtod is locale dependent, and with a locale, where the decial point isn’t ., the resulting numbers are unexpected, and ParseDouble() fails, due to it’s check, whether there are trailing characters left.

When using QT, then it looks like LC_NUMERIC (and friends) are based on the user’s locale, while bitcoind appears to use the "C" locale for all LC_xxxx.

The configuration option "-lang" seems to have no effect here.

laanwj added the label GUI on Jul 17, 2015

laanwj added the label RPC on Jul 17, 2015

laanwj commented at 7:37 pm on July 17, 2015: member

Can you try replacing in UniValue::write

0    case VREAL:
1        {
2            std::stringstream ss;
3            ss << std::showpoint << std::fixed << std::setprecision(8) << get_real();
4            s += ss.str();
5        }
6        break;
7    case VNUM:
8        s += val;
9        break;

with

0    case VREAL:
1    case VNUM:
2        s += val;
3        break;

There is no need to parse and re-format the value via a double when outputting…

get_real is only used in one place apart from that, prioritisetransaction, so if this is a problem with ParseDouble this should avoid your issue.

laanwj added this to the milestone 0.12.0 on Jul 17, 2015

dexX7 commented at 0:25 am on July 18, 2015: contributor

Can you try replacing in UniValue::write …

It looks like the numbers are parsed and re-formatted to normalize the values to %d.%8d:

 0{
 1  "version": 119900,
 2  "protocolversion": 70002,
 3  "walletversion": 60000,
 4  "balance": 11.56061, // <-- would be 11.56061000
 5  "blocks": 1046,
 6  "timeoffset": 0,
 7  "connections": 0,
 8  "proxy": "",
 9  "difficulty": 4.656542373906925e-10, // <-- would be 0.00000000
10  "testnet": false,
11  "keypoololdest": 1436225389,
12  "keypoolsize": 101,
13  "paytxfee": 0.0001, // <-- would be 0.00010000
14  "relayfee": 0.00001, // <-- would be 0.00001000
15  "errors": "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
16}

get_real is only used in one place apart from that, prioritisetransaction, so if this is a problem with ParseDouble this should avoid your issue.

Just to quickly clarify with an example: I didn’t mess with my locale settings, but I live in Europe, and as per system default, , is used as decimal point:

0double n;
1bool success = ParseDouble("1.2345", &n);
2
3// => n == 1.0
4// => success == false

The unit tests pass, because it seems that only the QT client is affected. The QT documenation actually addresses this pitfall and suggests to reset LC_NUMERIC, once the application is initialized. This works, though I think it would probably best to avoid the use of locale-dependent functions altogether, or reset the numeric locale directly in ParseDouble(), if strtod should be kept.

laanwj commented at 4:55 am on July 18, 2015: member

It looks like the numbers are parsed and re-formatted to normalize the values to %d.%8d:

If that kind of formatting is desired for non-monetary values, this can also be done in setFloat directly (avoiding a round-trip with potential loss of precision). My point was not that the above is a good fix, but it works around the issue to isolate the problem. Does it stop the issue from happening in qt if you do that?

Agree that a locale-independent alternative to strtod should be used in ParseDouble, but as said, this was not meant as a proper fix. My surprise was just that a problem in ParseDouble escalates to almost all JSON generated.

laanwj commented at 6:30 am on July 18, 2015: member

Working on a fix…

laanwj referenced this in commit ec249d4a1d on Jul 18, 2015

laanwj commented at 7:26 am on July 18, 2015: member

See #6456

laanwj referenced this in commit 6443184bd5 on Jul 23, 2015

laanwj referenced this in commit b6be90c7bb on Jul 23, 2015

laanwj closed this on Jul 24, 2015

DrahtBot locked this on Aug 18, 2022

ParseDouble() is faulty, depending on locale #6443