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: use non atomic tracker for snapshot reverts #6451

Merged

Conversation

mattsse
Copy link
Member

@mattsse mattsse commented Nov 28, 2023

closes probably #6355
possibly related #6450

Some background:

due to ds-test legacy behaviour failures are stored in a state variable.

Prior to #4974

Fuzzing was having soundness issues with snapshot failures. This is because snapshot failures were not being persisted throughout fuzz runs, and when the Fuzz Backend Wrapper we use was dropped, this information was lost, leading to inconsistent test PASS states.

because we were using a Cow of the Backend so regular fuzz runs don't need to clone it per fuzz call, however as #4974 highlighted this is unsound for snapshots because we lose state that contains the the state variable that tracks errors

#4974 tried to fix this by persisting snapshot failures in an AtomicBool. But this is also unsound because this is susceptible to race conditions because this bool is now shared by all tests, even if we clone the Backend (Arc).

The fix is to use a simple boolean again and instead add the has_snapshot_failure to the RawCallResult that we then can also check when checking for failures.

That being said, the Executor + ContractRunner, especially the Fuzz variants desperately need a cleanup.

Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

man what a find. I should've noticed the problem with the atomic back then, which I probably would've caught now...

@mattsse
Copy link
Member Author

mattsse commented Nov 28, 2023

I didn't think about it either, and the ds-test failed var limitations are easy to miss.
The executor suffers massively from feature creep.
I should have cleaned that up a long time ago...

}

// always fails
function test_shouldFaillWithRevertTo() public {
Copy link
Member

Choose a reason for hiding this comment

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

typo

@mattsse mattsse merged commit 7c122b0 into foundry-rs:master Nov 29, 2023
19 checks passed
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.

3 participants