-
Notifications
You must be signed in to change notification settings - Fork 2.3k
refactor: parallelize invariants #5676
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
Conversation
| let Ok(InvariantFuzzTestResult { invariants, cases, reverts, last_run_inputs }) = | ||
| evm.invariant_fuzz(&invariant_contract) | ||
| else { | ||
| return vec![] |
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.
This error was just completely ignored for some reason. This gets thrown by prepare_fuzzing
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.
Yeah, I think there's a few more cases of this happening across the test runner. I have a pending refactor of another error being ignored on invariants
Evalir
left a comment
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.
I love this—there's a ton we can refactor here but this is great to reduce performance bottlenecks 😄
Let's just make sure we test this a ton before merge!
Evalir
left a comment
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.
I love this—there's a ton we can refactor here but this is great to reduce performance bottlenecks 😄
Let's just make sure we test this a ton before merge!
mattsse
left a comment
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.
amazing
| // Hacky to provide the full error to the user. | ||
| for invariant in invariant_contract.invariant_functions.iter() { | ||
| failures.failed_invariants.insert( | ||
| invariant.name.clone(), | ||
| (Some(error.clone()), invariant.to_owned().clone()), | ||
| ); | ||
| } |
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.
Why was this removed?
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.
Only one invariant is executed at a time now so there can't be multiple fails in one call to this function
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.
Can you take a look at #5726 pls? I think a bug was introduced but I can't manage to find when or where
* refactor: parallelize invariants * chore: clippy * collect before
Motivation
Most of the invariant code is written as if it handles multiple functions per call (e.g. argument is
&[Function]instead of&Functionetc.).This is fine, except we call these functions with
vec![function]in a loop over the functions...Recommended to review with "Hide whitespace" since this gets rid of a lot of indentation
Solution
Rewrite invariants to run one function at a time, and parallelize the top level loop