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

Implement FromStr for UDecimal/Decimal. #142

Merged
merged 7 commits into from
Aug 22, 2018

Conversation

jeandudey
Copy link
Contributor

@jeandudey jeandudey commented Aug 20, 2018

Solves #140

@apoelstra
Copy link
Member

Can you add a fuzz test that tries to parse a decimal, and when it succeeds tries to reserialize it (and check that it roundtrips correctly)?

@apoelstra apoelstra added this to the 0.14 milestone Aug 20, 2018
@jeandudey
Copy link
Contributor Author

Done

@apoelstra
Copy link
Member

apoelstra commented Aug 21, 2018

Nice, thanks! Unfortunately it looks like it failed trying to roundtrip the number '6' (which I kinda expected to happen ... that should obviously parse as a 6, but then I'd expect it to serialize as 6.00000000, so I guess the fuzz test needs to be more accepting... perhaps it should parse, then serialize, then parse again, and check that both parses gave the same Decimal / UDecimal?)

Also, not a big deal, but in future I'd appreciate if fuzz-tests would appear in their own commit, because they're generally independent of other changes and can be reviewed independently.

@jeandudey
Copy link
Contributor Author

Done 👍, fuzz tests are now in their own commit.

@jeandudey
Copy link
Contributor Author

Build is failing because rust-bitcoinconsensus doesn't build on 1.14.0, the cc crate was updated and doesn't build on 1.14.0 anymore.

@jeandudey
Copy link
Contributor Author

Tracking issue: rust-lang/cc-rs#336

@apoelstra
Copy link
Member

Looks like there is also a failing fuzz test case (hex 2d3735363539383732343037353635393837323434 which you should be able to copy into your fuzz binary's unit test, then run cargo test in the fuzz/ directory to reproduce)

@jeandudey
Copy link
Contributor Author

jeandudey commented Aug 22, 2018

That hex string is the number -75659872407565987244 (as a string), it's too large to fit in the mantissa field. The Decimal/UDecimal parsing functions now return Error::TooBig when an overflow occurs.

@jeandudey
Copy link
Contributor Author

Ok, don't know why tests are failing I'm feeling stupid right now :P, I'm going to sleep and check that tomorrow.

@jeandudey jeandudey force-pushed the 2018-08-decimal-fromstr branch 2 times, most recently from 8fe36fd to a915bc1 Compare August 22, 2018 15:09
The negative symbol wasn't there when `int_part` was equal to zero.

Signed-off-by: Jean Pierre Dudey <[email protected]>
@apoelstra
Copy link
Member

I don't mind the churn, but you may find life easier if you run ./travis-fuzz.sh locally before pushing (and maybe edit the file to do more iterations).

I'm glad we're finding this stuff though ... I expect all of these issues existed in the old code, we just never sussed them out.

@jeandudey
Copy link
Contributor Author

@apoelstra I can't do fuzzing right now on my PC (some reasons, not related to this), that's why I'm pushing and fixing.

@jeandudey
Copy link
Contributor Author

Also the tests are failing with the bytes 302e333434343034333334343333303333303333302039333330203b3333 (a very large fp number), but crashes when dec.to_string is called, I found out that this line in the Display implementation is causing this error (an overflow panic):

        let ten = 10i64.pow(self.exponent as u32);

Not sure how to handle this though.

@apoelstra
Copy link
Member

I think we should just forbid exponents larger than 18. I guess this could cause issues for Dogecoin or something but we don't support anything like that now.

Signed-off-by: Jean Pierre Dudey <[email protected]>
@jeandudey
Copy link
Contributor Author

lol, the build is finally green

@apoelstra
Copy link
Member

Thanks so much! Seems like most of the bugs were latent and had been hanging around since 2014 so it's awesome to finally clear them out.

@apoelstra apoelstra merged commit dbefaef into rust-bitcoin:master Aug 22, 2018
@jeandudey jeandudey deleted the 2018-08-decimal-fromstr branch August 22, 2018 19:10
yancyribbens pushed a commit to yancyribbens/rust-bitcoin that referenced this pull request Mar 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants