Bitcoin Forum
December 04, 2016, 02:06:31 AM *
News: To be able to use the next phase of the beta forum software, please ensure that your email address is correct/functional.
 
   Home   Help Search Donate Login Register  
Pages: [1]
  Print  
Author Topic: 0.3.21: Base units for JSON-RPC (Please test!)  (Read 3349 times)
Luke-Jr
Legendary
*
expert
Offline Offline

Activity: 2086



View Profile
February 20, 2011, 01:36:21 AM
 #1

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)

1480817191
Hero Member
*
Offline Offline

Posts: 1480817191

View Profile Personal Message (Offline)

Ignore
1480817191
Reply with quote  #2

1480817191
Report to moderator
Advertised sites are not endorsed by the Bitcoin Forum. They may be unsafe, untrustworthy, or illegal in your jurisdiction. Advertise here.
1480817191
Hero Member
*
Offline Offline

Posts: 1480817191

View Profile Personal Message (Offline)

Ignore
1480817191
Reply with quote  #2

1480817191
Report to moderator
jgarzik
Legendary
*
qt
Offline Offline

Activity: 1470


View Profile
February 20, 2011, 02:21:51 AM
 #2


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.


Jeff Garzik, bitcoin core dev team and BitPay engineer; opinions are my own, not my employer.
Donations / tip jar: 1BrufViLKnSWtuWGkryPsKsxonV2NQ7Tcj
Gavin Andresen
Legendary
*
qt
Offline Offline

Activity: 1652


Chief Scientist


View Profile WWW
February 20, 2011, 04:18:30 AM
 #3

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.

How often do you get the chance to work on a potentially world-changing project?
Luke-Jr
Legendary
*
expert
Offline Offline

Activity: 2086



View Profile
February 20, 2011, 07:33:35 PM
 #4

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.

casascius
Mike Caldwell
VIP
Legendary
*
Offline Offline

Activity: 1344


The Casascius 1oz 10BTC Silver Round (w/ Gold B)


View Profile WWW
February 20, 2011, 11:14:07 PM
 #5

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.

Companies claiming they got hacked and lost your coins sounds like fraud so perfect it could be called fashionable.  I never believe them.  If I ever experience the misfortune of a real intrusion, I declare I have been honest about the way I have managed the keys in Casascius Coins.  I maintain no ability to recover or reproduce the keys, not even under limitless duress or total intrusion.  Remember that trusting strangers with your coins without any recourse is, as a matter of principle, not a best practice.  Don't keep coins online. Use paper wallets instead.
Luke-Jr
Legendary
*
expert
Offline Offline

Activity: 2086



View Profile
February 20, 2011, 11:47:26 PM
 #6

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.

jgarzik
Legendary
*
qt
Offline Offline

Activity: 1470


View Profile
February 21, 2011, 02:25:26 AM
 #7


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.


Jeff Garzik, bitcoin core dev team and BitPay engineer; opinions are my own, not my employer.
Donations / tip jar: 1BrufViLKnSWtuWGkryPsKsxonV2NQ7Tcj
Luke-Jr
Legendary
*
expert
Offline Offline

Activity: 2086



View Profile
February 21, 2011, 03:37:41 AM
 #8

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.

jgarzik
Legendary
*
qt
Offline Offline

Activity: 1470


View Profile
February 21, 2011, 06:11:37 PM
 #9

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, 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.


Jeff Garzik, bitcoin core dev team and BitPay engineer; opinions are my own, not my employer.
Donations / tip jar: 1BrufViLKnSWtuWGkryPsKsxonV2NQ7Tcj
Luke-Jr
Legendary
*
expert
Offline Offline

Activity: 2086



View Profile
February 21, 2011, 07:35:27 PM
 #10

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, 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).

jgarzik
Legendary
*
qt
Offline Offline

Activity: 1470


View Profile
February 21, 2011, 07:51:31 PM
 #11

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, 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?

Jeff Garzik, bitcoin core dev team and BitPay engineer; opinions are my own, not my employer.
Donations / tip jar: 1BrufViLKnSWtuWGkryPsKsxonV2NQ7Tcj
ribuck
Donator
Legendary
*
Offline Offline

Activity: 826


View Profile
February 21, 2011, 07:55:15 PM
 #12

"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.
casascius
Mike Caldwell
VIP
Legendary
*
Offline Offline

Activity: 1344


The Casascius 1oz 10BTC Silver Round (w/ Gold B)


View Profile WWW
February 21, 2011, 08:51:56 PM
 #13

"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.

Companies claiming they got hacked and lost your coins sounds like fraud so perfect it could be called fashionable.  I never believe them.  If I ever experience the misfortune of a real intrusion, I declare I have been honest about the way I have managed the keys in Casascius Coins.  I maintain no ability to recover or reproduce the keys, not even under limitless duress or total intrusion.  Remember that trusting strangers with your coins without any recourse is, as a matter of principle, not a best practice.  Don't keep coins online. Use paper wallets instead.
ribuck
Donator
Legendary
*
Offline Offline

Activity: 826


View Profile
February 21, 2011, 08:54:55 PM
 #14

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.
Luke-Jr
Legendary
*
expert
Offline Offline

Activity: 2086



View Profile
February 23, 2011, 10:30:10 PM
 #15

In light of other major problems with the JSON-RPC protocol, I am abandoning this fix and recommending gavin's half-fix for the short term, and a new protocol for the long term.

casascius
Mike Caldwell
VIP
Legendary
*
Offline Offline

Activity: 1344


The Casascius 1oz 10BTC Silver Round (w/ Gold B)


View Profile WWW
February 23, 2011, 11:02:39 PM
 #16

In light of other major problems with the JSON-RPC protocol, I am abandoning this fix and recommending gavin's half-fix for the short term, and a new protocol 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.

Companies claiming they got hacked and lost your coins sounds like fraud so perfect it could be called fashionable.  I never believe them.  If I ever experience the misfortune of a real intrusion, I declare I have been honest about the way I have managed the keys in Casascius Coins.  I maintain no ability to recover or reproduce the keys, not even under limitless duress or total intrusion.  Remember that trusting strangers with your coins without any recourse is, as a matter of principle, not a best practice.  Don't keep coins online. Use paper wallets instead.
jgarzik
Legendary
*
qt
Offline Offline

Activity: 1470


View Profile
February 23, 2011, 11:06:58 PM
 #17

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.


Jeff Garzik, bitcoin core dev team and BitPay engineer; opinions are my own, not my employer.
Donations / tip jar: 1BrufViLKnSWtuWGkryPsKsxonV2NQ7Tcj
casascius
Mike Caldwell
VIP
Legendary
*
Offline Offline

Activity: 1344


The Casascius 1oz 10BTC Silver Round (w/ Gold B)


View Profile WWW
February 23, 2011, 11:14:01 PM
 #18


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).

Companies claiming they got hacked and lost your coins sounds like fraud so perfect it could be called fashionable.  I never believe them.  If I ever experience the misfortune of a real intrusion, I declare I have been honest about the way I have managed the keys in Casascius Coins.  I maintain no ability to recover or reproduce the keys, not even under limitless duress or total intrusion.  Remember that trusting strangers with your coins without any recourse is, as a matter of principle, not a best practice.  Don't keep coins online. Use paper wallets instead.
Luke-Jr
Legendary
*
expert
Offline Offline

Activity: 2086



View Profile
February 24, 2011, 12:00:39 AM
 #19

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.

Pages: [1]
  Print  
 
Jump to:  

Sponsored by , a Bitcoin-accepting VPN.
Powered by MySQL Powered by PHP Powered by SMF 1.1.19 | SMF © 2006-2009, Simple Machines Valid XHTML 1.0! Valid CSS!