-
Notifications
You must be signed in to change notification settings - Fork 605
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: differentially fuzz wasmer2 against wasmtime #7025
Conversation
let code = ContractCode::new(module.0.module.to_bytes(), None); | ||
let wasmer2 = run_fuzz(&code, VMKind::Wasmer2); | ||
let wasmtime = run_fuzz(&code, VMKind::Wasmtime); | ||
assert_eq!(wasmer2, wasmtime); |
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.
Hm, this I think shouldn't work? wasmer2 and wasmtime differ at least in the texts of some error messages.
If this actually works, perhaps that means that our fuzzing is not deep at all? Makes sense to look at .opaque_err
tests too see which cases should trip this assert.
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.
Well, I ran it only a few minutes on my laptop, so there's quite a chance that given a bit more time it'd have found error messages that differ! That said, looking for .opaque_error() looks to me like something that'd at least take a few hours for our fuzzing infra to find (sandbox-specific imports we probably can't generate, or too many imports), so it makes sense to me that my laptop wasn't able to find them :)
I just pushed a commit censoring the error contents, so this should hopefully be handled now :)
(I also censored the logs, but did not censor the profile data as it seems to be purely gas-related and we do want to check that gas profile is the same across runtimes)
…impacting production
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.
LGTM, but needs a reformat!
@pompon0 could you look at the test failure here? Seems spurious proto compat failure? |
Yes, that's because the proto presubmit check compares against master head, rather than against the lowest common ancestor of master and this branch. I'll try to fix that. In the meantime please just rebase to master head. |
fix in flight: #7046 |
Fixes #6184