Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

contracts: switch from parity-wasm-based to wasmi-based module validation #14449

Merged
merged 83 commits into from
Jul 4, 2023

Conversation

agryaznov
Copy link
Contributor

We keep only the following validation checks to be enforced to a Wasm module of a contract:

  • scan_imports(),
  • scan_exports().

Once switched to wasmi builtin gas metering (#14084), we don't need to perform other checks anymore.

All the validation is now being done through wasmi querying. We thereby get rid of wasmparser dependency, and keep wasm-instrument/parity-wasm dependency only for benchmarks, where it is used to generate contract modules.

save: compiles, only gas pushing left to macro

WIP proc macro

proc macro: done
tests::run_out_of_gas_engine, need 2 more

save: 2 bugs with gas syncs: 1 of 2 tests done

gas_syncs_no_overcharge bug fixed, test passes!

cleaned out debug prints

second bug is not a bug

disabled_chain_extension test fix (err msg)

tests run_out_of_fuel_host, chain_extension pass

all tests pass
@agryaznov

This comment was marked as outdated.

@command-bot

This comment was marked as outdated.

@agryaznov agryaznov requested a review from athei July 3, 2023 16:01
@agryaznov

This comment was marked as outdated.

@command-bot

This comment was marked as outdated.

@agryaznov
Copy link
Contributor Author

bot bench $ pallet dev pallet_contracts

@command-bot

This comment was marked as outdated.

@command-bot

This comment was marked as outdated.

@agryaznov
Copy link
Contributor Author

agryaznov commented Jul 3, 2023

Some regressions on the weights have been detected:

Screenshot from 2023-07-03 22-51-44

I'm not sure if it should be a stopper for this PR though, as we still need to re-work benchmarks for all host functions (see note in this #14084 (comment) and #14084 (comment)). For now we (over)charge the engine spent part twice for every host function because of that.

@athei
Copy link
Member

athei commented Jul 3, 2023

Can you please try to explain in more detail why you think the host function benchmarks overestimate? The benchmarks only measure the time it takes to call the host function which should be unaffected by out changes.

frame/contracts/src/wasm/prepare.rs Outdated Show resolved Hide resolved
frame/contracts/src/wasm/prepare.rs Outdated Show resolved Hide resolved
frame/contracts/src/wasm/prepare.rs Outdated Show resolved Hide resolved
@agryaznov
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 60aa1d7 into master Jul 4, 2023
6 checks passed
@paritytech-processbot paritytech-processbot bot deleted the ag-wasmi-validation branch July 4, 2023 21:38
@agryaznov
Copy link
Contributor Author

Can you please try to explain in more detail why you think the host function benchmarks overestimate? The benchmarks only measure the time it takes to call the host function which should be unaffected by out changes.

@athei, I tried to give the explanation in this comment
But this "overestimation" seems to lie far below current benchmarking error anyway, so this was just my hypothesis which failed to prove the reasons of those weights change.

nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
…lidation (paritytech#14449)

* upgrade to wasmi 0.29

* prepare cleanup

* sync ref_time w engine from the stack frame

* proc_macro: sync gas in host funcs

save: compiles, only gas pushing left to macro

WIP proc macro

proc macro: done

* clean benchmarks & schedule: w_base = w_i64const

* scale gas values btw engine and gas meter

* (re)instrumentation & code_cache removed

* remove gas() host fn, continue clean-up

save

* address review comments

* move from CodeStorage&PrefabWasmModule to PristineCode&WasmBlob

* refactor: no reftime_limit&schedule passes, no CodeStorage

* bugs fixing

* fix tests: expected deposit amount

* fix prepare::tests

* update tests and fix bugs

tests::run_out_of_gas_engine, need 2 more

save: 2 bugs with gas syncs: 1 of 2 tests done

gas_syncs_no_overcharge bug fixed, test passes!

cleaned out debug prints

second bug is not a bug

disabled_chain_extension test fix (err msg)

tests run_out_of_fuel_host, chain_extension pass

all tests pass

* update docs

* bump wasmi 0.30.0

* benchmarks updated, tests pass

* refactoring

* s/OwnerInfo/CodeInfo/g;

* migration: draft, compiles

* migration: draft, runs

* migration: draft, runs (fixing)

* deposits repaid non pro rata

* deposits repaid pro rata

* better try-runtime output

* even better try-runtime output

* benchmark migration

* fix merge leftover

* add forgotten fixtures, fix docs

* address review comments

* ci fixes

* cleanup

* benchmarks::prepare to return DispatchError

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts

* store memory limits to CodeInfo

* ci: roll back weights

* ".git/.scripts/commands/bench-vm/bench-vm.sh" pallet dev pallet_contracts

* drive-by: update Readme and pallet rustdoc

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts

* use wasmi 0.29

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts

* use wasmi 0.30 again

* query memory limits from wasmi

* save: scan_exports ported, compiles

* save (wip, not compiles)

* query memory limits from wasmi

* better migration types

* ci: pull weights from master

* refactoring

* ".git/.scripts/commands/bench-vm/bench-vm.sh" pallet dev pallet_contracts

* scan_imports ported

* scan_export ported, other checks removed

* tests fixed

tests fixed

* drop wasmparser and parity-wasm dependencies

* typo fix

* addressing review comments

* refactor

* address review comments

* optimize migration

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts

* another review round comments addressed

* ci fix one

* clippy fix

* ci fix two

* allow stored modules to have no memory imports

* rollback: allow stored modules to have no memory imports

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts

* address review comments

---------

Co-authored-by: command-bot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C3-medium PR touches the given topic and has a medium impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. E2-dependencies Pull requests that update a dependency file. I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. T4-smart_contracts This PR/Issue is related to smart contracts. Z3-substantial Can be fixed by an experienced coder with a working knowledge of the codebase.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants