Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
7 participants
@laanwj
Copy link
Member

commented Feb 27, 2014

See this report: https://bitcoin.stackexchange.com/questions/22716/bitcoind-sendfrom-round-amount-error

I've been unable to reproduce it in either master or 0.8.6. However, in theory this could happen on architectures that have imprecise floating point types. Even though bitcoind itself doesn't rely on floating point types anywhere, Spirit JSON does use doubles internally for parsing values like 123.456 (and so do we in ValueFromAmount / AmountFromValue).

My first attempt was to try to fix this and make the parser use a fixed-point or string representation for JSON numbers (ie https://github.com/bitcoin/bitcoin/blob/master/src/json/json_spirit_reader_template.h#L499 ), but turned out to be a deep rabbit hole and doesn't solve the issue at the client side.

So this is my solution: add an option -rpcstringamounts to represent monetary amounts as strings in RPC. With this option any monetary amount will be emitted as a string with fixed 8 decimals, very easy to parse. Also this changes the parsing to accept the same format (by using the existing ParseMoney function).

Example getinfo:

{
    ...
    "balance" : "0.29099995",
    ...
}

Commits should be self-documenting.

@laanwj laanwj added this to the 0.10.0 milestone Feb 27, 2014

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2014

BTW this same design can be trivially used to send monetary values via RPC in any desired way. For example, a mode that returns and takes satoshis as integer is just a matter of adding the necessary option parsing and updating ValueFromAmount and AmountFromValue.

Something like

--rpcamountsmode=double (default, legacy)
--rpcamountsmode=string
--rpcamountsmode=int
@jgarzik

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2014

@laanwj I was thinking the same thing, RE outputting satoshis rather than strings or decimal numbers.

@sipa

This comment has been minimized.

Copy link
Member

commented Mar 4, 2014

JSON has no native integer type. Outputting satoshi counts is no better than outputting Bitcoin amounts (they'll be interpreted as floating point in both case).

@jgarzik

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2014

@sipa The former is true -- no integer type -- but the latter is not.

Outputting satoshi amounts does not automatically imply interpretation as floating point. Most libs, including our own, provide integer or Decimal interpretations without an intermediate floating point conversion step.

The options should be more bitcoin-specific, though, and are really a matrix of four possibilities:

  1. JSON number, decimal (default)
  2. JSON number, satoshis
  3. JSON string, decimal
  4. JSON string, satoshis
@laanwj

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2014

Well I like strings (with the . in the right place), so I want to keep that option. I'm fine with adding more possibilities of course.

@jgarzik

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2014

-rpcamounttype=[number | string] --- JSON string or number
-rpcamountdecimal=[yes | no] --- satoshis, or no

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2014

Hmm... can we still combine it into one option somehow? I don't like two separate options affecting the monetary format, one in one place is confusing enough :-)

@jgarzik

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2014

-rpcamount=[number-decimal, number-satoshis, string-decimal, string-satoshis]

meh. Open to suggestions... it is a quad-state.

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2014

That looks good to me.

@luke-jr

This comment has been minimized.

Copy link
Member

commented Mar 4, 2014

s/decimal/btc/ (in case someone wants to do mbtc or ubtc in the future)?

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2014

@luke-jr Hmm... It was never my intention to add different units to the RPC. It's waay too easy to introduce thousandfold errors this way by having the client and server use a different setting.

Integer (in lowest possible denomination) and decimal (BTC proper) is enough IMO.

Confusion can be avoided with these four options by enforcing a . in decimal types and rejecting .. in the integer types (and rejecting strings when a number is expected and vice versa).

Edit: not that I'm against using btc instead of decimal here, it may be more consistent nevertheless.

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2014

OK implemented all four modes + added tests for them

  • number-btc: Format as number in decimal BTC (default)
  • number-satoshis: Format as number in satoshis
  • string-btc: Format as string in decimal BTC
  • string-satoshis: Format as string in satoshis
@sipa

This comment has been minimized.

Copy link
Member

commented Mar 10, 2014

If we're seriously thinking about making such pervasive changes to the RPC interface, I'd like to go all the way, and overhaul some other things:

  • Consistent method names.
  • Group method names based on the subsystem(s) they interfact with; for example prefix every wallet rpc with 'wallet.' (json-rpc allows arbitrary strings as method node), others with 'util.' (no subsystems), 'blockchain.' / 'validation.', ...
  • Stop using position arguments for optional flags, as they accumulate over time and become unwieldly. Instead either use a flags object (['prvikey', {'rescan': false}] instead of ['privkey', false]), or use JSON-RPC 2.0 named arguments (a larger change...).
@luke-jr

This comment has been minimized.

Copy link
Member

commented Mar 10, 2014

@sipa Those all sound like good ideas to me.

@davecgh

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2014

I would also like to see a full blown overhaul with proper REST versioning and grouped and prefixed method names along the same lines as what sipa suggests. I'm particularly keen on the idea of using named arguments. Positional arguments are not conducive to optional arguments at all. Having spent quite a bit of time on the RPC interface, I concur that the current positional design with optional parameters is already less than ideal.

Also, I would suggest that all numeric values in regards to BTC are in satoshi. First, this would make the API more consistent since you would know any time you're dealing with a bitcoin amount, it is satoshi and not some other variant like full BTC, mBTC, uBTC, etc. Second, it would eliminate the need for using a value with a decimal in it altogether. The client would then be responsible for converting to and from satoshi depending on its particular set of needs or user preferences.

@jgarzik

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2014

@sipa +1 to all that

@davecgh See existing pull req #2844 for a REST interface (though it is for something different, not an RPC replacement)

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2014

+1

  • JSON V2 may also be a good opportunity to remove getwork.
  • Regarding the wallet.* methods: it should be possible to specify which wallet they apply to in the case of multiwallet

laanwj added some commits Feb 27, 2014

FormatMoney: make right-trimming zeros optional, on by default
Currently the FormatMoney function always trims trailing zeros apart
from two if possible.
Add an option to avoid this trimming and always return a string with 8
decimals.
Move AmountFromValue and ValueFromAmount to rpcprotocol
Converting monetary amounts is part of the RPC protocol,
both the client and server need to share the same implementation.
Use ValueFromAmount in RPC client
No longer use doubles. This allows supporting alternative RPC
representations of monetary values.
Add option `-rpcamount` to set format for monetary amounts in RPC
Support this option at both the client side and server side. Both sides need
to be using the same value of this option, otherwise parse errors
will ensue.

Four modes are available:

- number-btc: Format as number in decimal BTC (default)
- number-satoshis: Format as number in satoshis
- string-btc: Format as string in decimal BTC
- string-satoshis: Format as string in satoshis

Also add tests for AmountFromValue / ValueFromAmount for all four modes.
@BitcoinPullTester

This comment has been minimized.

Copy link

commented May 28, 2014

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/f3f8460fe51be719231612a921dd37af638df46a for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2014

No longer going to maintain this, at some point I suppose we'll want to do a RPC overhaul and the money amounts should just be integers in satoshis.

@laanwj laanwj closed this Jun 12, 2014

@gdm85

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2015

JSON has no native integer type. Outputting satoshi counts is no better than outputting Bitcoin amounts (they'll be interpreted as floating point in both case).

@sipa luckily there are other implementations of JSON readers e.g. the world is not really always using JavaScript to parse JSON data. See also http://www.ecma-international.org/publications/files/ECMA-ST/ECMA-404.pdf (page 4):

JSON is agnostic about numbers. In any programming language, there can be a variety of number types of various capacities and complements, fixed or floating, binary or decimal. That can make interchange between different programming languages difficult.
JSON instead offers only the representation of numbers that humans use: a sequence of digits. All programming languages know how to make sense of digit sequences even if they disagree on internal representations. That is enough to allow interchange.

This is a closed/pretty old issue now, I don't want to do much necromancy but maybe the improvements proposed by @sipa can get their own separate PR and this one be kept about the single incremental improvement proposed in OP?

laanwj added a commit to laanwj/bitcoin that referenced this pull request Jun 5, 2015

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 bitcoin#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.

laanwj added a commit to laanwj/bitcoin that referenced this pull request Jun 6, 2015

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 bitcoin#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.