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(forge): commit state between invariant runs #5444

Closed
wants to merge 3 commits into from

Conversation

Evalir
Copy link
Member

@Evalir Evalir commented Jul 20, 2023

Motivation

Closes #3005.

Solution

Do not drop the executor between runs, but rather use the same executor.

@Evalir Evalir requested a review from mattsse July 20, 2023 20:27
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.

looks reasonable

@Evalir
Copy link
Member Author

Evalir commented Jul 24, 2023

This is incredibly slow though @mattsse — do you think there's another way to do this? If not, we probably shouldn't do this because of performance regressions.

@Evalir
Copy link
Member Author

Evalir commented Jul 25, 2023

Hey @mds1 looping you in here too—This is a direct fix for #3005 but introduces serious performance regressions, and i'm unsure if we have another way of doing this. I'd say we should hold off of merging this

@mds1
Copy link
Collaborator

mds1 commented Jul 25, 2023

Hmm, can you help me understand why this has such a big performance regression? We already commit state for all non-test contracts, so it seems surprising that committing state for the test contract would cause this

@Evalir
Copy link
Member Author

Evalir commented Jul 25, 2023

Yep, so, it's because we persist the peripheral contract's state in a different way altogether—it's much cheaper since we're not really "growing" the internal executor changeset, but rather collecting the run state and then using this to find the next inputs.

For committing the invariant state we'd need to keep the used executor alive, and this means that for each depth of each run, the in-memory db will grow, and it'll get slower and slower as a result

@Evalir Evalir changed the title [wip] feat(forge): commit state between invariant runs feat(forge): commit state between invariant runs Jul 27, 2023
@Evalir
Copy link
Member Author

Evalir commented Jul 27, 2023

@mds1 @mattsse nevermind about performance regressions—something was affecting my environment and made builds super slow. Just moved to my new laptop and this is fine.

Just tested doing foundryup from this branch commit and the performance difference seems completely negligible. I think we can merge this in if this is truly the case! Just a sanity test, can you test this branch @mds1 ?

@mds1
Copy link
Collaborator

mds1 commented Jul 27, 2023

nevermind about performance regressions—something was affecting my environment and made builds super slow. Just moved to my new laptop and this is fine.

Oh awesome, that's great news!

Definitely can test, cc @lucas-manuel @PaulRBerg to sanity test also :)

@Evalir
Copy link
Member Author

Evalir commented Dec 7, 2023

Closing, this is not the correct solution to #3005 — i'll followup with the proper fix

@Evalir Evalir closed this Dec 7, 2023
@Evalir Evalir deleted the evalir/commit-test-state branch December 7, 2023 20:15
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.

feat: commit state changes to test contract during invariant/fuzz tests
3 participants