-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Better number generation strategy #735
Better number generation strategy #735
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ship it - nice work. @brockelmore said he'd do any follow-up refactors on this.
re: Fixtures, one nice follow-up could be to find the constants in the code and populate the fixtures with it, e.g. how it's done in Echidna by tapping on the output of solc --ast-compact-json
https://github.com/crytic/echidna/blob/06cd34b2e8527028e64f0afdd353644976a5b97c/lib/Echidna/Solidity.hs#L154-L185
Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>
We seem to have hit a Shrinking issue?
Maybe this code? |
Yeah it's bacause I made dict and edge cases values not shrinkable. I reverted that for this time. Probably it makes sense to try shrink these values since iterations are run pretty fast anyway... |
@alexeuler this is working great inc ombination with the Assume PR! |
Otherwise they can be invalid curve points since Secp256k1's Curve Order is `0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141`. See [k256](https://docs.rs/k256/latest/src/k256/lib.rs.html#54)'s implemenation. This was detected in Forge after we made the fuzzer smarter foundry-rs/foundry#735
Motivation
Current uint generation strategy doesn't give good number coverage and doesn't catch even simple edge cases. This is discussed in #387
Solution
Create a new strategy for uint. The strategy combines 3 different strategies, each assigned a specific weight:
bits
param). Then generate a value for this bit size.Results
The new strategy was tested on the following contract:
The results for test cases that should fail:
Also for testPositive the random test cases seems to have much better coverage:
New strategy
Vs for the old strategy: