paritytech / substrate

Substrate: The platform for blockchain innovators
Apache License 2.0
8.39k stars 2.65k forks source link

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

Closed agryaznov closed 1 year ago

agryaznov commented 1 year ago

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

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.

agryaznov commented 1 year ago

bot bench $ pallet dev pallet_contracts

command-bot[bot] commented 1 year ago

@agryaznov "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_contracts (https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3113351) was cancelled in https://github.com/paritytech/substrate/pull/14449#issuecomment-1618920794

agryaznov commented 1 year ago

bot cancel

command-bot[bot] commented 1 year ago

@agryaznov Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_contracts has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3113351 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3113351/artifacts/download.

agryaznov commented 1 year ago

bot bench $ pallet dev pallet_contracts

command-bot[bot] commented 1 year ago

@agryaznov https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3114006 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_contracts. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 16-4176a09d-692b-4725-b24d-d17472fed7ad to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot[bot] commented 1 year ago

@agryaznov Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_contracts has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3114006 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3114006/artifacts/download.

agryaznov commented 1 year ago

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 https://github.com/paritytech/substrate/pull/14084#issuecomment-1596850814 and https://github.com/paritytech/substrate/pull/14084#issuecomment-1619112784). For now we (over)charge the engine spent part twice for every host function because of that.

athei commented 1 year ago

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.

agryaznov commented 1 year ago

bot merge

agryaznov commented 1 year ago

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.