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

Float formatting differs from bitcoin core #421

Open
arnuschky opened this issue May 5, 2015 · 8 comments
Open

Float formatting differs from bitcoin core #421

arnuschky opened this issue May 5, 2015 · 8 comments

Comments

@arnuschky
Copy link

Bitcoin core formats all floats as %.8f in JSON, while btcd omits trailing zeros.

This also leads to btcd returning an integer (1) when bitcoin core returns a float (1.0). Differing results based on this could be considered a bug in the serializer, but if your striving for compatibility, this should maybe be fixed. For example, I am using python-bitcoinrpc. It converts all floats automatically to Decimal objects, but misses the ones btcd returns as integers.

Note: @davecgh mentioned that there's only a single number type in JSON, so technically there's no difference

@davecgh
Copy link
Member

davecgh commented May 5, 2015

Thanks for opening this for discussion.

@jrick
Copy link
Member

jrick commented May 5, 2015

Might need something similar to this to control how the numbers are formatted.

http://play.golang.org/p/1blGf1w_Rc

@luke-jr
Copy link

luke-jr commented May 5, 2015

IMO it shouldn't be btcd's job to workaround bugs in other software. bitcoind doesn't guarantee any particular formatting either.

@jrick
Copy link
Member

jrick commented May 5, 2015

I generally agree. Modifying all of our structures to use json.Number (a string type) is a huge amount of effort for very little gain, and any small gain could be broken a day later with a change from core. It would be better to modify the client code to hint as to what kind of types to unmarshal json values as (I don't know if this is possible with the python client).

The real issue is that json has so many different valid encodings of the same thing, and there's no standard specification of how to go from marshalled json -> language types. A simple byte comparison can't be used to test equivalence of marshalled json, and the python code is not wrong per se to decode the json number to different concrete types.

@davecgh
Copy link
Member

davecgh commented May 6, 2015

I'm leaning towards closing this. I'm certainly not against changing things for the sake of compatibility when the risk is low, but as the others have noted, changing this would require changing every instance of a float to a string type which entails its own level of risk since invalid values could now unmarshal and would have to be manually checked at every call site versus allowing the parser to do it. Also, changing this would cause a trickle effect into not only btcd, but also btcwallet, the RPC client, btcsim, and everything else using the structs.

All of this would introduce a considerable amount of risk in regards to introducing an error. Given this and the fact that it's really the caller's responsibility to handle JSON numeric conversions properly (some languages have typed numbers, some don't), I don't feel like this is something we should attempt to change.

@jrick
Copy link
Member

jrick commented May 6, 2015

This is also an option: http://play.golang.org/p/lqde4Ub-aZ

Would need explicit casting everywhere it's used.

@davecgh
Copy link
Member

davecgh commented May 6, 2015

That's certainly a better approach than using json.Number which would have the problem I discussed above where the errors would have to be checked at every call site versus allowing the parser to do it.

At least with your proposed approach, only casts would be needed and the compiler would catch those.

@jrick
Copy link
Member

jrick commented May 6, 2015

The following bogus cast wouldn't be caught:

amt := btcutil.Amount(123.456e8)
famt := btcjson.FloatAmount(amt)

but the existing code has the same potential issue converting from btcutil.Amount to float64.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants