rpc: Avoid unnecessary parsing roundtrip in number formatting, fix locale issue #6456

Merged
merged 3 commits into from Jul 24, 2015

Conversation

Projects
None yet
4 participants
@laanwj
Member

laanwj commented Jul 18, 2015

Three weakly related fixes to number handling after the univalue switch. These came to the surface while troubleshooting #6443.

Make ValueFromAmount always return 8 decimals

This is the format that was always returned to JSON clients. The difference was not noticed before, because VREAL values are post-processed by univalue.

By implementing the functionality directly it breaks the dependency of rpcserver on utilmoneystr. FormatMoney is now only used for debugging purposes.

To test, port over the formatting tests from util_tests.cpp to rpc_tests.cpp.

univalue: Avoid unnecessary roundtrip through double for numbers

JSON makes no distinction between numbers and reals, and our code doesn't need to do so either.

This removes VREAL, as well as its specific post-processing in UniValue::write. Non-monetary amounts do not need to be forcibly formatted with 8 decimals, so the extra roundtrip was unnecessary (and potentially loses precision).

util: use locale-independent parsing in ParseDouble

Use locale-indepent C++ based parsing instead of C's strtod, which checks for different input based on the user's locale.
Fixes #6443.

laanwj added some commits Jul 18, 2015

rpc: Make ValueFromAmount always return 8 decimals
This is the format that was always returned to JSON clients.
The difference was not noticed before, because VREAL values
are post-processed by univalue.

By implementing the functionality directly it breaks the dependency
of rpcserver on utilmoneystr. FormatMoney is now only used for debugging
purposes.

To test, port over the formatting tests from util_tests.cpp to
rpc_tests.cpp.
univalue: Avoid unnecessary roundtrip through double for numbers
JSON makes no distinction between numbers and reals, and our code
doesn't need to do so either.

This removes VREAL, as well as its specific post-processing in
`UniValue::write`. Non-monetary amounts do not need to be forcibly
formatted with 8 decimals, so the extra roundtrip was unnecessary
(and potentially loses precision).
util: use locale-independent parsing in ParseDouble
Use locale-indepent C++ based parsing instead of C's strtod,
which checks for different input based on the user's locale.
Fixes #6443.

@laanwj laanwj added the RPC/REST/ZMQ label Jul 18, 2015

@laanwj laanwj changed the title from rpc: Avoid unnecessary roundtrips in number parsing, fix locale issue to rpc: Avoid unnecessary parsing roundtrip in number formatting, fix locale issue Jul 18, 2015

@laanwj laanwj added the Bug label Jul 18, 2015

@dexX7

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Jul 18, 2015

Contributor

I can confirm this resolves #6443, thanks!

As far as I can see the output is as expected, except for the difficulty, returned by "getinfo", which is shown in e notation ("difficulty": 4.656542373906925e-10 vs. 0.00000000, as returned by Core 11.0). Edit: I wouldn't say this is bad though, as it offers a greater precision.

Contributor

dexX7 commented Jul 18, 2015

I can confirm this resolves #6443, thanks!

As far as I can see the output is as expected, except for the difficulty, returned by "getinfo", which is shown in e notation ("difficulty": 4.656542373906925e-10 vs. 0.00000000, as returned by Core 11.0). Edit: I wouldn't say this is bad though, as it offers a greater precision.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 20, 2015

Member

as returned by Core 11.0). Edit: I wouldn't say this is bad though, as it offers a greater precision

Exactly - the fixed 8-digit formatting is for clarity in monetary amounts.
Earlier it was applied to all numbers because JSON spirit had no other way.
But now we have full control over how values are rendered, and other floating point quantities such as bits, difficulty can be represented in full precision.

Member

laanwj commented Jul 20, 2015

as returned by Core 11.0). Edit: I wouldn't say this is bad though, as it offers a greater precision

Exactly - the fixed 8-digit formatting is for clarity in monetary amounts.
Earlier it was applied to all numbers because JSON spirit had no other way.
But now we have full control over how values are rendered, and other floating point quantities such as bits, difficulty can be represented in full precision.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 20, 2015

Member

Tested ACK.

If i understand this right, then there is a slightly API change. Every double/float (non-monetary) comes now with a precision of 16 instead of 8 (because of https://github.com/bitcoin/bitcoin/pull/6456/files#diff-0f1b401041a14398229cf7e31b6db7eeR89). Because we are using oss << std::setprecision(16) << val it will rewrite things like 0.00000000046565423739069247 into scientific notation. That is what @dexX7 has detected.

Member

jonasschnelli commented Jul 20, 2015

Tested ACK.

If i understand this right, then there is a slightly API change. Every double/float (non-monetary) comes now with a precision of 16 instead of 8 (because of https://github.com/bitcoin/bitcoin/pull/6456/files#diff-0f1b401041a14398229cf7e31b6db7eeR89). Because we are using oss << std::setprecision(16) << val it will rewrite things like 0.00000000046565423739069247 into scientific notation. That is what @dexX7 has detected.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 20, 2015

Member

Right. All numbers were 'cut' to %.8f notation, so it would have reported 0.00000000046565423739069247 as 0.00000000 instead of going to scientific notation.
E.g. a difficulty smaller than a "satoshi" was reported as 0. This makes no sense at all for non-monetary numbers. This is all within the limits of number representation in the JSON protocol.

Member

laanwj commented Jul 20, 2015

Right. All numbers were 'cut' to %.8f notation, so it would have reported 0.00000000046565423739069247 as 0.00000000 instead of going to scientific notation.
E.g. a difficulty smaller than a "satoshi" was reported as 0. This makes no sense at all for non-monetary numbers. This is all within the limits of number representation in the JSON protocol.

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jul 23, 2015

Contributor

ACK. Happy to see VREAL go away.

Contributor

jgarzik commented Jul 23, 2015

ACK. Happy to see VREAL go away.

@laanwj laanwj merged commit ec249d4 into bitcoin:master Jul 24, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jul 24, 2015

Merge pull request #6456
ec249d4 util: use locale-independent parsing in ParseDouble (Wladimir J. van der Laan)
7650449 univalue: Avoid unnecessary roundtrip through double for numbers (Wladimir J. van der Laan)
e061e27 rpc: Make ValueFromAmount always return 8 decimals (Wladimir J. van der Laan)

@str4d str4d referenced this pull request in zcash/zcash Feb 10, 2017

Merged

Add UniValue as subtree #2082

zkbot added a commit to zcash/zcash that referenced this pull request Feb 10, 2017

@str4d str4d referenced this pull request in zcash/zcash Feb 15, 2017

Merged

Bitcoin 0.12 RPC PRs 1 #2100

@dagurval dagurval referenced this pull request in bitcoinxt/bitcoinxt Mar 18, 2017

Merged

rpc: Accept strings in AmountFromValue #191

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment