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

feat: snapshot fuzz tests using deterministic seed #2591

Merged
merged 1 commit into from
Aug 3, 2022

Conversation

onbjerg
Copy link
Member

@onbjerg onbjerg commented Aug 3, 2022

Motivation

We introduced --include-fuzz-tests to make gas snapshots less prone to change from fuzz RNG because we had no way to make the fuzzer deterministic. Now that the fuzzer is deterministic we can remove the flag and include fuzz tests always

Solution

Removes --include-fuzz-tests and uses a deterministic seed for forge snapshot runs. This also cleans up the code a bit, since we introduced a variable we had to carry into many files to disable fuzz tests.

Closes #2553

@onbjerg onbjerg added the T-feature Type: feature label Aug 3, 2022
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@onbjerg
Copy link
Member Author

onbjerg commented Aug 3, 2022

Hmm, have to think about this a bit more. It slowed down coverage quite a lot (26s on solmate). I guess that's how slow coverage is going to be with 100% fuzz test support... surely there's a way to optimize

@gakonst
Copy link
Member

gakonst commented Aug 3, 2022

It slowed down coverage quite a lot (26s on solmate). I guess that's how slow coverage is going to be with 100% fuzz test support... surely there's a way to optimize

Feels unrelated though right? i.e. in order to get good coverage metrics we'd still want our fuzz tests included (using the det. seed)

Have you tried profiling coverage? Suspect a lot would be in the IC/PC/stack/memory instrumentation?

@onbjerg
Copy link
Member Author

onbjerg commented Aug 3, 2022

Yeah I'm sure there are a lot of performance gains to be had in optimizing our IC/PC thing, both for the debugger and for coverage. FWIW it was really fast before fuzz tests were enabled. I have some ideas for how to optimize it I'll look at tomorrow, since I am going to be looking at full fuzz support for coverage anyway

I haven't tried profiling it yet, though

@gakonst
Copy link
Member

gakonst commented Aug 3, 2022

do you feel OK with merging this, so we get snapshots "finished", and we can investigate coverage separately given we're not 100% there yet anyway?

@onbjerg
Copy link
Member Author

onbjerg commented Aug 3, 2022

Yep, sg

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-feature Type: feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: forge snapshot should remove --include-fuzz and use a hardcoded fuzz_seed
3 participants