paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.network/
1.71k stars 615 forks source link

Add explicit feature flags for non-MVP WASM features implicitly supported by `wasmtime` #15

Open koute opened 1 year ago

koute commented 1 year ago

Currently wasmtime supports extra features which were not part of the WASM MVP. These features cannot be disabled in wasmtime but (at least for the signed ops extension) we reject the runtimes which make use of them since parity-wasm (which we currently use to preprocess WASM blobs) will choke on their use at load time.

We don't want to suddenly enable any new extra features without explicit versioning, so it's good that we don't load those runtimes, but it's bad that this is not explicitly controlled and should be fixed.

For example, for the signed ops feature we should:

1) enable the sign_ext feature in parity-wasm, 2) explicitly detect runtimes which use these instructions reject them, (instead of relying on the feature flag in parity-wasm not being enabled and only implicitly rejecting those runtimes, which brings in the risk of it being accidentally enabled through a transitive dependency without us noticing) 3) (optionally, in a later step) consider enabling the sign_ext, which would entail adding a new feature flag like in this PR which would disable the check that the runtime doesn't use the feature.

s0me0ne-unkn0wn commented 1 year ago

explicitly detect runtimes which use these instructions

This one sounds like a new feature of parity-wasm, but we want to get rid of parity-wasm :thinking: Any better ideas on how to implement that?

koute commented 1 year ago

explicitly detect runtimes which use these instructions

This one sounds like a new feature of parity-wasm, but we want to get rid of parity-wasm thinking Any better ideas on how to implement that?

For e.g. sign_ext extension parity-wasm already has the support with the appropriate feature flag, so we can enable that; for others where parity-wasm has no support we can punt until we switch to wasmparser since parity-wasm will always fail to parse those anyway (but we should add an explicit test in sc-executor where it'll try to load a runtime using those unsupported features and make sure they fail to load)

s0me0ne-unkn0wn commented 1 year ago

I mean the detection itself... It doesn't make sense to add one more WASM parser to sc-executor just to detect if the feature is used, so the existing parser should detect it. We could incorporate such functionality to parity-wasm, but it would make trading it for wasmparser harder, as the latter lacks that functionality I believe.

koute commented 1 year ago

I mean the detection itself...

Just iterate over the code, match the instructions and if an instruction matches one from the extension reject it? Neither parity-wasm nor wasmparser need to have explicit support for this as long as they can parse them.

s0me0ne-unkn0wn commented 1 year ago

Well, yes, but that sounds exactly like one more WASM parser :wink: We already parse WASM bytecode twice on its way to compilation -- in parity-wasm and in wasmtime. And there will be one more for feature detection. I mean, it's worth thinking about incorporating the detection somewhere where WASM bytecode already gets parsed :thinking:

koute commented 1 year ago

We already parse WASM bytecode twice on its way to compilation -- in parity-wasm and in wasmtime. And there will be one more for feature detection.

One more? But it's already parsed by parity-wasm? (:

s0me0ne-unkn0wn commented 1 year ago

Okay, I'll try to make my concern more clear :)

Where exactly will the feature detection step take place?

If it is a new function in sc-executor code that parses the bytecode and detects features based on the instruction set used -- it adds one more parsing layer (and we already have two)

If it is a new function in parity-wasm like detect_wasm_extensions() -> HashSet<WasmExtension>, we're reusing one of the parser layers (which is good) but making it harder to switch from parity-wasm to wasmparser lacking that new function (which is bad).

koute commented 1 year ago

Where exactly will the feature detection step take place?

If it is a new function in sc-executor code that parses the bytecode and detects features based on the instruction set used -- it adds one more parsing layer (and we already have two)

An extra function in sc-executor-common in runtime_blob.rs which would iterate over the already parsed parity-wasm::Module and see whether any disallowed instructions are used. There's no extra parsing involved because parity-wasm has already parsed the module.

s0me0ne-unkn0wn commented 1 year ago

Ah yes, sorry, I'm not very clever. You're getting an already parsed sequence from parity-wasm. It's okay then, nevermind :)