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

u64 field should be serialized as string in json format #2045

Open
HarukaMa opened this issue Nov 10, 2022 · 8 comments
Open

u64 field should be serialized as string in json format #2045

HarukaMa opened this issue Nov 10, 2022 · 8 comments
Labels
question Further information is requested RPC RPC-related issues testnet3

Comments

@HarukaMa
Copy link
Contributor

🐛 Bug Report

The json formatter is storing u64 fields as a number, and it's silently losing some precision for the data. I'm not sure why it happened during the serialization though - I'd expect rust libraries use u64 instead of double for that.

Currently the only place affected is the nonce of the PartialSolution.

Steps to Reproduce

REST endpoint testnet3/latest/block reports inaccurate nonce for coinbase solutions.

Expected Behavior

The data should either be accurate numbers or strings.

Your Environment

Assume latest

@HarukaMa HarukaMa added the bug Incorrect or unexpected behavior label Nov 10, 2022
@niklaslong
Copy link
Collaborator

niklaslong commented Nov 10, 2022

Thank you for reporting this issue @HarukaMa. This is puzzling, the Serializer implementation work fine with serde's JSON, as tested in snarkVM for PartialSolution; and this trait is simply what warp uses under the hood in order to handle the data. What are you using to query the rest API? 🤔

@niklaslong niklaslong added RPC RPC-related issues testnet3 labels Nov 10, 2022
@HarukaMa
Copy link
Contributor Author

HarukaMa commented Nov 10, 2022

wow, this might be an invalid issue then. It looks like jq is actually the culprit here. The snarkOS code is actually producing good results. That's a little embarrassing.

That said, I've seen another consumer of the rest api incorrectly dealing with those large numbers. Maybe using strings could raise the awareness about that?


To clarify, there is another block explorer outta there and it's also losing precision on the nonce field. The data in the current form is not safe in the javascript environment, as js uses double for all numbers.

@niklaslong
Copy link
Collaborator

I see, good to know and agreed; we'll definitely consider the switch and if we don't end up making the change we'll document this better. Thanks again!

@niklaslong niklaslong added question Further information is requested and removed bug Incorrect or unexpected behavior labels Nov 10, 2022
@HarukaMa HarukaMa changed the title [Bug] u64 field should be serialized as string in json format u64 field should be serialized as string in json format Nov 10, 2022
@zvolin
Copy link
Contributor

zvolin commented Nov 10, 2022

yeah, jq handles numbers only to about 2^53 which is mantissa size in double precision floats. You could try using jql instead of jq, it will handle u64 and probably also even u128

@zvolin
Copy link
Contributor

zvolin commented Nov 10, 2022

please note that jql will probably be a lot slower than jq when reading from stdin (at least that was true a month ago when I was checking). It prefers files provided as an argument

@howardwu
Copy link
Contributor

howardwu commented Oct 9, 2023

Check in here, is this issue still relevant?

@HarukaMa
Copy link
Contributor Author

Yes, the nonce is still returned in json as a number.

@raychu86
Copy link
Contributor

Is this issue still relevant after AleoNet/snarkVM#2559?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested RPC RPC-related issues testnet3
Projects
None yet
Development

No branches or pull requests

5 participants