Bitcoin Forum

Bitcoin => Development & Technical Discussion => Topic started by: Luke-Jr on February 20, 2011, 01:36:21 AM



Title: 0.3.21: Base units for JSON-RPC (Please test!)
Post by: Luke-Jr on February 20, 2011, 01:36:21 AM
There have been 3 threads about the 1000000 base unit rounding problem now, and all 3 have had a general consensus that base units should be used by all protocols and internals.

Gavin made a valid point about a buggy JSON-RPC library that would try to fit anything integer-looking into a int32_t, and that can easily be solved by appending ".0" to all amounts, triggering any such oversimplified implementation to use a floating-point type (which can represent all valid base unit bitcoin values without precision errors).

I have completed a working implementation of this in the Gitorious 'neutral' branch ( http://gitorious.org/bitcoin/bitcoin/commits/neutral ).

The only "bug" remaining, in my experience, is that when operating in JSON-RPC client (not server) mode, the pretty-formatting code turns the ".0" into ".00000000"; since this mode is mainly useful for testing, and even then defaults to the old RPCv0 mode, it should not be in my opinion considered a blocker issue.

Currently, to use RPCv1, methods must be prefixed by 'bitcoin.1.'; RPCv0 can be explicitly selected with 'bitcoin.0.'. MagicalTux had suggested using instead a different URI, but the changes required in bitcoind to support this would be non-trivial.

Testing and constructive criticism requested; Let's try to get this fix in for 0.3.21!

Edit: To see a diff of all the changes, run: git fetch git://gitorious.org/bitcoin/bitcoin.git neutral && git diff -r b1a657a..FETCH_HEAD
Edit: Pretty diff of everything changed: http://paste.factorcode.org/paste?id=2182 (please use git pull, don't apply patch manually)


Title: Re: 0.3.21: Base units for JSON-RPC (Please test!)
Post by: jgarzik on February 20, 2011, 02:21:51 AM

Please post a link to a diff versus mainline bitcoin, showing just your RPCv1 changes.

It increases the burden on reviewers, if we must clone and diff and do all sorts of things, just to see what changed.

Thanks.



Title: Re: 0.3.21: Base units for JSON-RPC (Please test!)
Post by: Gavin Andresen on February 20, 2011, 04:18:30 AM
Can you post examples of requests/responses with the "old" JSON-RPC and your new JSON-RPC?

I'm still firmly against this change-- it seems to me you are "solving" a non-problem.


Title: Re: 0.3.21: Base units for JSON-RPC (Please test!)
Post by: Luke-Jr on February 20, 2011, 07:33:35 PM
Can you post examples of requests/responses with the "old" JSON-RPC and your new JSON-RPC?
RPCv0: {"params":[],"method":"bitcoin.0.getbalance","id":"jsonrpc"} => {"result":351.10259079,"error":null,"id":"jsonrpc"}
RPCv1: {"params":[],"method":"bitcoin.1.getbalance","id":"jsonrpc"} => {"result":35110259079.0,"error":null,"id":"jsonrpc"}

RPCv0: {"params":["1DMNJcDoqeLcEyCD3KRZ49MT4PKFcoafKU",1.00000000],"method":"bitcoin.0.sendtoaddress","id":"jsonrpc"} => {"result":"d51e53e9a50aac1856c68ad619f6c19bb7d5791670a57c1f4e3131b10965938f","error":null,"id":"jsonrpc"}
RPCv1: {"params":["1DMNJcDoqeLcEyCD3KRZ49MT4PKFcoafKU",100000000.0],"method":"bitcoin.1.sendtoaddress","id":"jsonrpc"} => {"result":"d51e53e9a50aac1856c68ad619f6c19bb7d5791670a57c1f4e3131b10965938f","error":null,"id":"jsonrpc"}


RPCv0: (impossible to do)
RPCv1: {"params":["1DMNJcDoqeLcEyCD3KRZ49MT4PKFcoafKU",1048576.0],"method":"bitcoin.1.sendtoaddress","id":"jsonrpc"} => {"result":"d51e53e9a50aac1856c68ad619f6c19bb7d5791670a57c1f4e3131b10965938f","error":null,"id":"jsonrpc"}

I'm still firmly against this change-- it seems to me you are "solving" a non-problem.
The problems addressed have been discussed in the 3 other threads, as well as the majority expressing a desire to use base units for internals/protocols.


Title: Re: 0.3.21: Base units for JSON-RPC (Please test!)
Post by: casascius on February 20, 2011, 11:14:07 PM
Can you post examples of requests/responses with the "old" JSON-RPC and your new JSON-RPC?
["1DMNJcDoqeLcEyCD3KRZ49MT4PKFcoafKU",1048576.0],"method":"bitcoin.1.sendtoaddress","id":"jsonrpc"}[/color] => {"result":"d51e53e9a50aac1856c68ad619f6c19bb7d5791670a57c1f4e3131b10965938f","error":null,"id":"jsonrpc"}

This looks like another attempt to add "Tonal Bitcoins", given that 1048576 is 2^20.

Ordinarily I would not go out of my way to get in the way of such a harmless "feature".  But I don't think the "majority" wants a revision to a protocol that drastically changes something that's not really broken.  The side effect of that is unwarranted confusion and wasted future resources dancing around the question of "am I dealing with n, or am I dealing with n * 10^8"?

It would be like trying to navigate the hell of programming Windows in the era of Windows Me/2000, where you constantly had to ask, does this function I'm calling expect 8-bit or 16-bit characters?

I would gather that Luke, the proponent of Tonal Bitcoins, is deaf to the concern that unnecessary complication is bad.  Someone who has no problem persistently telling us we need to switch to using the character "9" to mean ten because it's "easier for humans" would have no reservations about doubling the complexity of the API to support the same agenda.


Title: Re: 0.3.21: Base units for JSON-RPC (Please test!)
Post by: Luke-Jr on February 20, 2011, 11:47:26 PM
I would gather that Luke, the proponent of Tonal Bitcoins, is deaf to the concern that unnecessary complication is bad.  Someone who has no problem persistently telling us we need to switch to using the character "9" to mean ten because it's "easier for humans" would have no reservations about doubling the complexity of the API to support the same agenda.
This is a simplification and correction, it doesn't add complexity. While proper precision is indeed a requirement of sending TBC, it is by no means TBC-specific.


Title: Re: 0.3.21: Base units for JSON-RPC (Please test!)
Post by: jgarzik on February 21, 2011, 02:25:26 AM

Thanks for posting an easy-to-review diff.

Comments:
  • We should not support multiple RPC versions in the same binary.  We are at 0.3.x, early in bitcoin's life.  If we need to change the API, we just change it.  Adding a 'version' arg to many functions pointlessly increases complexity.
  • However, I don't see a driving need to change the API just for this issue.
  • It is strange that SelectCoinsMinConf() needs any change at all.
  • In general, I understand the desire to avoid faux-floating point in JSON output, as is currently done, but "$nanocoin.0" seems like an absurd attempt to avoid weird implementation issues, when those issues should instead be confronted directly.



Title: Re: 0.3.21: Base units for JSON-RPC (Please test!)
Post by: Luke-Jr on February 21, 2011, 03:37:41 AM
Comments:
  • We should not support multiple RPC versions in the same binary.  We are at 0.3.x, early in bitcoin's life.  If we need to change the API, we just change it.  Adding a 'version' arg to many functions pointlessly increases complexity.
  • However, I don't see a driving need to change the API just for this issue.
  • It is strange that SelectCoinsMinConf() needs any change at all.
  • In general, I understand the desire to avoid faux-floating point in JSON output, as is currently done, but "$nanocoin.0" seems like an absurd attempt to avoid weird implementation issues, when those issues should instead be confronted directly.
Good catch with SelectCoinsMinConf-- that bit is part of a bugfix that should (AIUI) already be included in the 0.3.21 tree. Apparently it's not, so got grouped in. I remade the diff and updated the link in the original post.

If the ".0" workaround is not desired, it should be easy to back out just that one commit. However, it should have no negative effects, and avoids money-handling problems with buggy JSON implementations (which there seem to be plenty of).

P.S. even though I know what you mean, probably best not to use the word "nano" for what is not in any way a SI-nano division. might confuse someone.


Title: Re: 0.3.21: Base units for JSON-RPC (Please test!)
Post by: jgarzik on February 21, 2011, 06:11:37 PM
If the ".0" workaround is not desired, it should be easy to back out just that one commit. However, it should have no negative effects, and avoids money-handling problems with buggy JSON implementations (which there seem to be plenty of).

Can you be more specific?  I know of jansson (http://www.digip.org/jansson/), which has problems because the author arbitrarily declared all integers in his API to be 32-bit.  What other problematic implementation exist?

If everybody except jansson works with 64-bit integers (the bitcoin native coin size), I'd be OK with moving forward.  We can deal with a single, buggy implementation.  Perhaps I could even contribute an int64 patch to jansson's upstream.

Now, back to your proposal.

1. I agree with a proposal to change RPC to showing native int64 values for monetary amounts, in JSON output.  No special tricks like $coin.0.

2. I disagree that this issue, alone, requires a breaking change to RPC.  It's "nice to have" but not a driver for inclusion.

3. I strongly disagree with multi-version code at this early stage in bitcoin's life.  We should just pick an RPC version and stick with it.



Title: Re: 0.3.21: Base units for JSON-RPC (Please test!)
Post by: Luke-Jr on February 21, 2011, 07:35:27 PM
If the ".0" workaround is not desired, it should be easy to back out just that one commit. However, it should have no negative effects, and avoids money-handling problems with buggy JSON implementations (which there seem to be plenty of).

Can you be more specific?  I know of jansson (http://www.digip.org/jansson/), which has problems because the author arbitrarily declared all integers in his API to be 32-bit.  What other problematic implementation exist?
I'm no expert on all the JSON libraries out there, but the one embedded in BitCoin had such a bug-- if a JSON number looks float-ish, it will error if the code tries to access it as an integer. This bug is out of necessity fixed in the ".0"-adding commit.
1. I agree with a proposal to change RPC to showing native int64 values for monetary amounts, in JSON output.  No special tricks like $coin.0.

2. I disagree that this issue, alone, requires a breaking change to RPC.  It's "nice to have" but not a driver for inclusion.

3. I strongly disagree with multi-version code at this early stage in bitcoin's life.  We should just pick an RPC version and stick with it.
"This" issue? There are numerous issues being addressed by this solution...

Note: According to the JSON standard, N and N.0 are the exact same number. (not intending to argue with your points here, just making a point of fact).


Title: Re: 0.3.21: Base units for JSON-RPC (Please test!)
Post by: jgarzik on February 21, 2011, 07:51:31 PM
If the ".0" workaround is not desired, it should be easy to back out just that one commit. However, it should have no negative effects, and avoids money-handling problems with buggy JSON implementations (which there seem to be plenty of).

Can you be more specific?  I know of jansson (http://www.digip.org/jansson/), which has problems because the author arbitrarily declared all integers in his API to be 32-bit.  What other problematic implementation exist?
I'm no expert on all the JSON libraries out there, but the one embedded in BitCoin had such a bug-- if a JSON number looks float-ish, it will error if the code tries to access it as an integer. This bug is out of necessity fixed in the ".0"-adding commit.

If we simply output the raw int64, this is an unrelated bug.

So, jansson remains the only known JSON implementation that has trouble with int64 values?


Title: Re: 0.3.21: Base units for JSON-RPC (Please test!)
Post by: ribuck on February 21, 2011, 07:55:15 PM
"solving" a non-problem.
No-one has stepped forward to say that they are suffering from a problem that would be fixed by this patch, so it is "solving" a non-problem.


Title: Re: 0.3.21: Base units for JSON-RPC (Please test!)
Post by: casascius on February 21, 2011, 08:51:56 PM
"solving" a non-problem.
No-one has stepped forward to say that they are suffering from a problem that would be fixed by this patch, so it is "solving" a non-problem.

I think it would be great if we could modify Bitcoin so that we could also send irrational numbers of Bitcoins (you know, like pi, or e, or the square root of 2).  You never know - someone might want to charge for pizza by its exact surface area, rather than a flat rate per pie.  That's why I invented "radial Bitcoins".  I was going to update the Bitcoin scripting system to include all sorts of transcendental functions, as well as change the amount field so that it can accept a MATLAB-style formula.  If radial bitcoins succeeds, my next inventions will be vector bitcoins, and complex bitcoins (you know, with a real and imaginary component).

Entirely kidding, of course.


Title: Re: 0.3.21: Base units for JSON-RPC (Please test!)
Post by: ribuck on February 21, 2011, 08:54:55 PM
I think it would be great if we could modify Bitcoin so that we could also send irrational numbers of Bitcoins (you know, like pi, or e, or the square root of 2).

Send a patch, and we'll shoot it down in flames.

I'm working on a patch to associate a fragrance with each payment. Lilac to the love of your life. Campfire smoke to your hiking buddies. And lemon for your tax payments.


Title: Re: 0.3.21: Base units for JSON-RPC (Please test!)
Post by: Luke-Jr on February 23, 2011, 10:30:10 PM
In light of other major problems with the JSON-RPC protocol, I am abandoning this fix and recommending gavin's half-fix (http://bitcointalk.org/index.php?topic=3786.0) for the short term, and a new protocol (http://bitcointalk.org/index.php?topic=3757.0) for the long term.


Title: Re: 0.3.21: Base units for JSON-RPC (Please test!)
Post by: casascius on February 23, 2011, 11:02:39 PM
In light of other major problems with the JSON-RPC protocol, I am abandoning this fix and recommending gavin's half-fix (http://bitcointalk.org/index.php?topic=3786.0) for the short term, and a new protocol (http://bitcointalk.org/index.php?topic=3757.0) for the long term.

While I am sure I have imposed enough suffering on Luke and his tonal bitcoin idea, Luke's focus on the number precision is something worth ironing out.  One thing that might be worth thinking about would be a long term solution that formally overcame the hardcoded "nanocoin" now and forever, in favor of something with a decimal mantissa and exponent.

This would also give Luke his tonal bitcoins to a certain extent by accident - as if I understand correctly, base-2 fractions are all representable in decimal - as long as enough digits of precision (e.g. 64 bits) were offered.


Title: Re: 0.3.21: Base units for JSON-RPC (Please test!)
Post by: jgarzik on February 23, 2011, 11:06:58 PM
While I am sure I have imposed enough suffering on Luke and his tonal bitcoin idea, Luke's focus on the number precision is something worth ironing out.  One thing that might be worth thinking about would be a long term solution that formally overcame the hardcoded "nanocoin" now and forever, in favor of something with a decimal mantissa and exponent.

Yuck.  Exponents and money do not mix in a user friendly manner.

Nanocoins[1] are stored as 64bit integers.  That is the native representation.  Any change should move towards alignment with that representation, not away from it.



Title: Re: 0.3.21: Base units for JSON-RPC (Please test!)
Post by: casascius on February 23, 2011, 11:14:01 PM

Yuck.  Exponents and money do not mix in a user friendly manner.

Nanocoins[1] are stored as 64bit integers.  That is the native representation.  Any change should move towards alignment with that representation, not away from it.


That's certainly true for binary exponents.  Why would it be for decimal?

Native representation doesn't have to change, we would just be solving the problem now while it is easy.  One BTC is merely a nanoBTC * 10^8 (or whatever the factor is).


Title: Re: 0.3.21: Base units for JSON-RPC (Please test!)
Post by: Luke-Jr on February 24, 2011, 12:00:39 AM
Native representation doesn't have to change, we would just be solving the problem now while it is easy.  One BTC is merely a nanoBTC * 10^8 (or whatever the factor is).
This is a problem for GUIs, not wallets/internals.