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

refactor: switch to pure-rust wasm parser for test #4488

Merged
merged 5 commits into from
Jul 14, 2021

Conversation

matklad
Copy link
Contributor

@matklad matklad commented Jul 9, 2021

This substitutes wat (pure Rust impl from bytecode alliance) for wabt
(bindings to C++ lib) to parse wasm s-expresisons to binary form.

wat doesn't do validation, only syntax checking, that's why
.validate(false) calls are just removed.

wat preserves the debug names section, that's why we chage burnt_gas
once

https://webassembly.github.io/spec/core/appendix/custom.html#name-section

See #4471 for motivation

closes #4471

@matklad matklad force-pushed the wabt2wat branch 3 times, most recently from 55366c8 to a95086f Compare July 9, 2021 18:42
This substitutes wat (pure Rust impl from bytecode alliance) for wabt
(bindings to C++ lib) to parse wasm s-expresisons to binary form.

`wat` doesn't do validation, only syntax checking, that's why
`.validate(false)` calls are just removed.

`wat` preserves the debug names section, that's why we chage burnt_gas
once

https://webassembly.github.io/spec/core/appendix/custom.html#name-section

See near#4471 for motivation

closes near#4471
Copy link
Collaborator

@bowenwang1996 bowenwang1996 left a comment

Choose a reason for hiding this comment

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

How does this affect gas costs?

Copy link
Member

@ailisp ailisp left a comment

Choose a reason for hiding this comment

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

Looks good. My concern is since wat doesn't validate, should we test more syntax correct, wabt not validated contracts to wasmer? Can we find a few from fuzzing?

@matklad
Copy link
Contributor Author

matklad commented Jul 13, 2021

@bowenwang1996 It doesn't affect gas costs: it's in the dev-deps for the vm-runner, and in the estimator it is used (and should be used) outised of the measured segments.

@ailisp yeah, I also was surprised at the difference in validation. Thinking more about this, wat behavior (not validating) seems more reasonable and more useful for us.

It's more reasonable because semantic validation ("type checking" of the stack discipline) is distinct from parsing, and belongs to the separate layer. It' doesn't make much sense to run validation to return just Vec<u8>, which erases the info that validation has been applied.

It is more useful for us, because, during testing we specifically care about behavior of the runtime on invalid contracts. Note that we only use wat/wabt during tests (this is statically enforced by them being a dev dep) -- in production, it's the VM (wasmer) which enforces validation.

@near-bulldozer near-bulldozer bot merged commit 00bfb71 into near:master Jul 14, 2021
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.

Switch from wabt to wat
4 participants