Don't go through double in AmountFromValue and ValueFromAmount #6239

pull laanwj wants to merge 3 commits into bitcoin:master from laanwj:2015_06_jsonrpc changing 6 files +45 −43

laanwj commented at 6:52 AM on June 5, 2015: member

My prime gripe with JSON spirit was that monetary values still had to be converted from and to floating point which can cause deviations (see #3759 and https://bitcoin.stackexchange.com/questions/22716/bitcoind-sendfrom-round-amount-error).

As UniValue stores internal values as strings, this is no longer necessary. By using fixed-point parsing and formatting already available, this avoids risky double-to-integer and integer-to-double conversions completely, and results in more elegant code to boot.

Also contains a related dead code cleanup commit for FormatMoney.
laanwj added the label RPC on Jun 5, 2015
laanwj force-pushed on Jun 5, 2015
jgarzik commented at 6:59 AM on June 5, 2015: contributor

Indeed - that was the intention RE stored as strings - great work @laanwj

"it works" tested ACK
in src/rpcserver.cpp:None in 94897e683a outdated
```
 135 | +        throw JSONRPCError(RPC_TYPE_ERROR, "Amount is not a number");
 136 | +    CAmount amount;
 137 | +    if (!ParseMoney(value.getValStr(), amount))
 138 |          throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount");
 139 | -    return nAmount;
 140 | +    if (!MoneyRange(amount))
```
laanwj commented at 7:40 AM on June 5, 2015:

I just noticed that there is a slight change of semantics here. The old check dAmount <= 0.0 || dAmount > 21000000.0 did not allow zero amounts, whereas MoneyRange allows nValue >= 0 && nValue <= MAX_MONEY.

Unsure what to do here. The no-brainer solution would be to restore the old behavior, however

There is one place in the source where this is worked around with an ugly hack: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpcwallet.cpp#L2172

The other places where AmountFromValue is used are sendtoaddress movecmd sendfrom sendmany

The alternative would be to handle zero-ness at the other call sites, which is feasible as they are few. It would be less surprising to handle specific constraints like this at the call side instead of the parser function. Whatever is decided there should be a test for this in rpc_parse_monetary_values.
Diapolo commented at 10:07 AM on June 5, 2015: none

Just wanted to mention that PaymentServer::verifyAmount() also uses MoneyRange().
laanwj commented at 10:26 AM on June 5, 2015: member

@diapolo True, but not relevant here, I'm not intending to change MoneyRange's semantics.
jonasschnelli commented at 2:59 PM on June 5, 2015: contributor

Nice! Tested and reviewed ACK.
laanwj commented at 4:08 PM on June 5, 2015: member

@jonasschnelli What do you think about the range issue mentioned in my comment above?
jonasschnelli commented at 6:54 PM on June 5, 2015: contributor

@laanwj: If we take a closer look at a AmountFromValue, than i think, CAmount can be 0 so AmountFromValue() should also be able to handle 0ness. Even, a single function must not/should not represent the behavior of the whole system (if someone would define our system behavior to reject 0 values everywhere). AmountFromValue is also used in createrawtransaction and there i could imaging usecases for 0 value in/outputs (to replace later by some hex/further manipulating, etc.).

Therefore ACK this solution (even if it's a slightly API change). And yes, adding a test for a 0 value would probably be a good idea.

Don't go through double in AmountFromValue and ValueFromAmount

My prime gripe with JSON spirit was that monetary values still had to be
converted from and to floating point which can cause deviations (see #3759
and https://bitcoin.stackexchange.com/questions/22716/bitcoind-sendfrom-round-amount-error).

As UniValue stores internal values as strings, this is no longer
necessary. This avoids risky double-to-integer and integer-to-double
conversions completely, and results in more elegant code to boot.

4b4b9a8de6

Get rid of fPlus argument to FormatMoney

It's never used with any other value than false, the default.

a04bdefbeb

Changes necessary now that zero values accepted in AmountFromValue

- Add an accept test for zero amounts, and a reject test for negative
  amounts
- Remove ugly hack in `settxfee` that is no longer necessary
- Do explicit zero checks in wallet RPC functions
- Don't add a check for zero amounts in `createrawtransaction` - this
  could be seen as a feature

7d8ffac186

laanwj force-pushed on Jun 6, 2015
laanwj commented at 7:48 AM on June 6, 2015: member

Thanks. Pushed a commit that adds the checks to caller sites where needed, and adds tests.
laanwj merged this on Jun 9, 2015
laanwj closed this on Jun 9, 2015
laanwj referenced this in commit 643114f539 on Jun 9, 2015
zkbot referenced this in commit eaaa5f625f on Feb 10, 2017
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#3759 Add option `-rpcamount` for monetary amounts as strings/satoshis in RPC