stacks-network / clarity-wasm

`clar2wasm` is a compiler for generating WebAssembly from Clarity.
GNU General Public License v3.0
14 stars 15 forks source link

wasm binary validation #435

Closed csgui closed 4 months ago

csgui commented 4 months ago

Fix #433.

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 86.15%. Comparing base (09fe754) to head (a512c14).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #435 +/- ## ======================================= Coverage 86.15% 86.15% ======================================= Files 44 44 Lines 19673 19673 Branches 19673 19673 ======================================= Hits 16950 16950 Misses 1229 1229 Partials 1494 1494 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

csgui commented 4 months ago

So it would be a mandatory check on any kind of build except debug? These seems fair.

Yes. Development mode ignores the binary validation.

Maybe we could also avoid it for benchmarks?

IMO, benchmarks using a validated binary is the correct approach since the validated binary is the one to be used in production mode. Nevertheless, we can change that, if need, adding a compile-time feature flag. But let's take care of that when benchmark effort has started and after evaluating that need.

Acaccia commented 4 months ago

Still, I'm not super fond of the fact that we rely on future developers will know that the generated binary is not checked only in debug mode. Future people might believe that a successful compilation == a correct binary. This could lead to people adding CI tasks where they just compile Clarity files and believe it is checked and correct (what happened in #433 already).

I think a flag would be a better idea.

csgui commented 4 months ago

@Acaccia changed to make the binary validation a default feature.

obycode commented 4 months ago

I think instead of adding this inside the code, it could be added to the tests, as a separate process. Outside of the tests, the module will get loaded after being compiled to execute the top-level code, and any problem with the module would be reported then. It's only in calls to clar2wasm where this check would not happen. It might be useful during development to generate invalid wasm. I think I'd prefer if your tests add a call to validate-wasm after calling clar2wasm.

obycode commented 4 months ago

Another better option would be to write some tests that actually call functions from the boot contracts instead of just checking if it compiles. We should be able to find some existing tests for this that we can replicate.

csgui commented 4 months ago

@obycode Thanks for pointing that the module will get loaded to execute the top-level code. 🙇

I've added the binary validation as a step inside the boot-contracts-compile.sh script.

csgui commented 4 months ago

Yes. Was really handy to find wasm-tools for GitHub Actions.