API versioning; represent and accept amounts as strings #431

cdhowie · 2011-07-26T03:23:10Z

This is a two-patch patch against 0.3.24 (since master has a lot of churn in rpc.cpp and I don't want to do all the work merging until I know that this patch has community support).

The first commit adds RPC versioning support. As things need to be changed, older clients still need to be supported in some way. This has been done by taking a numeric token from the URL and using that to determine the version. So if the API is accessed at http://localhost:8332/ then it will use version 0. If http://localhost:8332/bitcoin.v1 then it will use version 1. And of course, http://localhost:8332/bitcoin.v2 would be version 2. For the purposes of versioning, I am declaring versions 0 and 1 identical, to avoid confusion.

The second commit does two things:

Instead of requiring a JSON number, BTC amounts may now be sent as strings.
If the service is accessed using version 2 or higher, BTC amounts will be returned as strings instead of numbers.

This is an important change, because not all JSON libraries store numbers in a format with 16 decimal digits of precision. I've tested, and when I set bitcoind's paytxfee setting to 10000000.00000001, Python is unable to handle this many significant digits; it represents the number as 10000000.000000009. While not off by much, it does seem to indicate that representing amounts as pure numbers in JSON data may cause some clients to have accuracy issues.

By taking the amounts directly from int64s to strings, the client is free to convert the values to whatever high-precision format it wants (like the Decimal type in Python).

However, while many languages will convert strings to numbers automatically, many will not, so this change won't be backwards-compatible; hence the new API versioning feature.

Here are the effects of this branch, using Python:

>>> bitcoindv0 = jsonrpclib.Server("http://chris:testing@localhost:8332/")
>>> bitcoindv1 = jsonrpclib.Server("http://chris:testing@localhost:8332/bitcoin.v1")
>>> bitcoindv2 = jsonrpclib.Server("http://chris:testing@localhost:8332/bitcoin.v2")
>>> bitcoindv0.getinfo()[u'balance'];
0.10000000000000001
>>> bitcoindv1.getinfo()[u'balance'];
0.10000000000000001
>>> bitcoindv2.getinfo()[u'balance'];
u'0.10000000'

mikegogulski · 2011-07-26T05:53:09Z

If merged, I believe that f5f97fa would close #247

gavinandresen · 2011-07-26T07:22:06Z

This was discussed to death a few months ago.

10000000.000000009 will be correctly rounded to 10000000.00000001 by bitcoin, so there is no issue.

Going the other way, bitcoin always gives precise values.

If you want all of your json strings to look pretty, use the latest version of simplejson that fully supports Decimal JSON values; e.g.

import decimal
import simplejson
simplejson.dumps(decimal.Decimal('10000000.000000009'), use_decimal=True)
'10000000.000000009'

sipa · 2011-07-26T09:09:00Z

I definitely agree that if we ever make a backward-incompatible upgrade to the RPC protocol, amounts encoded as strings should be part of it. However, as each new protocol version introduces legacy for many versions to come, we should be careful about introducing new ones.

Maybe the time is indeed ripe for a new RPC version. In that case, i would prefer to do a larger overhaul, including renaming the calls to be more consistent.

I like the idea of putting version information in the URL, though.

genjix · 2011-07-26T13:13:00Z

Instead of converting the value to a string by yourself, you should use boost::lexical_cast as I'm doing here,

https://gitorious.org/intersango/bitcoind/commit/665294c486f1eba1bcd84025a42fa5e2886a89a9/diffs

cdhowie · 2011-07-26T20:07:28Z

10000000.000000009 will be correctly rounded to 10000000.00000001 by bitcoin, so there is no issue.

There is no information in the JSON specification regarding how numbers should be stored; a JSON implementation is free to use single-precision floating point, and that would not be a bug with that implementation. I repeat, we should not be conveying financial information using an encoded numeric format that does not require implementations to use any specific amount of precision.

…ngs if API version is >= 2

cdhowie · 2011-07-26T20:11:14Z

(Fixed conversions to double still occurring.)

gavinandresen · 2011-07-26T21:25:28Z

If you can find a reasonably-widely-used JSON implementation that cannot use double-precision floating point please send me a pointer. I'm strongly against adding 100 more lines of code because "there might theoretically be a bug on hardware or software that we haven't actually ever encountered and may or may not exist."

jgarzik · 2011-08-04T19:17:43Z

See #452

I think some amount of API versioning is fair, even outside this decimal-vs-float discussion.

alexwaters · 2011-09-30T08:05:35Z

The pull has become unmergeable (without conflicts), and will be closed in 15 days from this message if action is not taken.

To prevent closure, kindly rebase the pull to merge cleanly with the current codebase. If a time extension is needed, please respond to this comment or contact [email protected].

luke-jr · 2011-10-12T18:11:28Z

Amounts shouldn't be strings anyway, they should be Numbers.

cdhowie · 2011-10-12T21:02:46Z

Their type is numeric yes. If the devs are unwilling to incorporate an integral (satoshi-based) API then a less intrusive change would be to represent the numeric data as strings. In other words, in the order of best to worst:

Convey BTC amounts as integers, where the unit 1 is 0.00000001 BTC.
Convey BTC amounts as strings representing fractional numbers.
Convey BTC amounts as fractional numbers in a data type that is poorly-defined and imprecise enough to cause rounding errors.

3 is what we have now. Using strings is still better than what we have now, though I would strongly urge the BTC devs to adopt 1.

gavinandresen · 2011-10-12T21:22:59Z

Do you really want to have this argument AGAIN?

Responding to your 1/2/3:

JSON does not have an integer type. It just has Number.
JSON Numbers ARE strings as they are sent across the wire.
The JSON Number type on EVERY IMPLEMENTATION OF JSON THAT ANYBODY CAN FIND is precise enough to represent every single possible bitcoin amount without rounding errors.

549378c Activate recent new features on testnet (dexX7)

…lidate it 2bfabb3 blockheaders include block height, nodes validate it (Gregory Sanders) 8f6bedf functional framework support for CScriptNum decode (Gregory Sanders) Pull request description: The feature is enabled by default in custom chains; cannot be activated in legacy chains. Tree-SHA512: d033b7dd0d853e5693efda584494cf58ed7ff43ce32d1f5d44e4ee52907b989e434c8dbb28a0f843da8b799548924d2499e8759a96ed5a7191b95f22b29c7f6e

…n third-party link action 2ccde2f qt: hyphenate usage of third-party modifier (Jarol Rodriguez) 8177578 qt: ensure seperator when adding third-party transaction links (Jarol Rodriguez) a70a980 qt: improve text for open third-party tx url action (Jarol Rodriguez) 9980f4a qt, refactor: simplify third-party tx url action through overload (Jarol Rodriguez) Pull request description: [#4092](#4092) introduced the ability to open up a transaction in a block explorer. This improves the related code by simplifying the addition and connection of the action through an [overloaded](https://doc.qt.io/archives/qt-5.9/qmenu.html#addAction-5) `addAction` function and prepends action description text to the host, "Show in". The reason to add this text is to make it clear what the action does. It also creates a clearer mental correlation between a user doing the work to add the 3rd-party tx link and this new menu action popping up. This updates the setting text so that "third-party" is hyphenated. It should be hyphenated because it is being used as a modifier of both "URL" and "transaction URLs". Additionally, this fixes #431 by ensuring that the seperator will be added before creating action. Screenshots of visual changes: **Context menu actions** | master | pr | |--------------|--------| | <img width="248" alt="3pt-master" src="https://user-images.githubusercontent.com/23396902/134618354-00278ac6-5094-44ee-8ba7-fe648fdcb7d2.png"> | <img width="248" alt="3pt-pr" src="https://user-images.githubusercontent.com/23396902/134618364-ddb64269-e5ee-40af-a2a6-1922001b6f4e.png"> | **Setting text** (tooltip text containing usage of "third-party" is also properly hyphenated) | master | pr | |--------------|--------| | ![unnamed](https://user-images.githubusercontent.com/23396902/134854070-fb299ba5-3491-487f-b37f-c0cd96514353.png) | ![pr-hyphenate](https://user-images.githubusercontent.com/23396902/134854127-88630cc2-a178-4376-a569-f413f66eba0d.png) | ACKs for top commit: stratospher: tested ACK 2ccde2f. promag: Code view ACK 2ccde2f. hebasto: ACK 2ccde2f Tree-SHA512: 8dfcd539a1d41c8abf3c8208d150d1480d4ef81a008de826299e8bad1dfa6e3c49dc76d041c5946fafcf0b033eebb9b9fbd3d49ba6d8af93dd388c488e92f143

Add RPC API versioning capability

7f5ed22

Allow amounts to be specified as a string, and return amounts as stri…

aced653

…ngs if API version is >= 2

gavinandresen closed this Oct 12, 2011

jgarzik mentioned this pull request Oct 20, 2011

RPC: form basis of RPC API versioning, and declare current one v1 #452

Closed

zathras-crypto pushed a commit to zathras-crypto/omnicore that referenced this pull request Nov 16, 2016

Merge pull request bitcoin#431

39cdd9d

549378c Activate recent new features on testnet (dexX7)

Losangelosgenetics pushed a commit to Losangelosgenetics/bitcoin that referenced this pull request Mar 12, 2020

cmd.exe is not supported (closes bitcoin#431)

75765e9

rajarshimaitra pushed a commit to rajarshimaitra/bitcoin that referenced this pull request Aug 5, 2021

Fixes bitcoin#431

4d9774e

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

ghost deleted a comment Jan 10, 2022

API versioning; represent and accept amounts as strings #431

API versioning; represent and accept amounts as strings #431

Uh oh!

Conversation

cdhowie commented Jul 26, 2011

Uh oh!

mikegogulski commented Jul 26, 2011

Uh oh!

gavinandresen commented Jul 26, 2011

Uh oh!

sipa commented Jul 26, 2011

Uh oh!

genjix commented Jul 26, 2011

Uh oh!

cdhowie commented Jul 26, 2011

Uh oh!

cdhowie commented Jul 26, 2011

Uh oh!

gavinandresen commented Jul 26, 2011

Uh oh!

jgarzik commented Aug 4, 2011

Uh oh!

alexwaters commented Sep 30, 2011

Uh oh!

luke-jr commented Oct 12, 2011

Uh oh!

cdhowie commented Oct 12, 2011

Uh oh!

gavinandresen commented Oct 12, 2011

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants