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

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

Open
koute opened this issue Apr 4, 2023 · 9 comments

Comments

@koute
Copy link
Contributor

koute commented Apr 4, 2023

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
Copy link
Contributor

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 🤔
Any better ideas on how to implement that?

@koute
Copy link
Contributor Author

koute commented Apr 4, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

koute commented Apr 4, 2023

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
Copy link
Contributor

Well, yes, but that sounds exactly like one more WASM parser 😉
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 🤔

@koute
Copy link
Contributor Author

koute commented Apr 4, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

koute commented Apr 4, 2023

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
Copy link
Contributor

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

@the-right-joyce the-right-joyce transferred this issue from paritytech/substrate Aug 24, 2023
helin6 pushed a commit to boolnetwork/polkadot-sdk that referenced this issue Feb 5, 2024
* Release branch polkadot-v0.9.38 (#1015)

* EIP-2539 (paritytech#15)

* Update shell.nix

* Read point from input

* Finish `BLS12377G1Add`

* Fix `BLS12377G1Add` output encode

* Finish `BLS12377G1Mul`

* Finish `BLS12377G1MultiExp`

* Finish `BLS12377G2Add`

* Finish `BLS12377G2Mul`

* Draft `eip-2539` implement

* Finish `BLS12377Pairing`

* Draft `eip-2539`

* Multiplication by the unnormalized scalar

* Rename serialize to write

* Test Cases

* Test Cases

* Rewrite read_fq

* Rename

* Doc and cleanup

* Tidy

* Tidy

* Tidy

* Only check point in subgroup for pairing

* Fmt

* Tests

* Typo

* Typo

* Fix conv

* Change err info

* Fmt

* EIP-2539 tests

* EIP-2539 tests

* Lint and test

* EIP-3026  (paritytech#16)

* Update shell.nix

* G1Add and G1Mul

* G1MultiExp

* G2Add

* G2Mul and G2MultiExp

* Bw6Pairing

* EIP-3026 tests

* EIP-3026 failure tests

* Fix lint

* Lint

* Lint and test

* Comment

* Deps order

* Fmt

* Lint

---------

Co-authored-by: Wei Tang <[email protected]>
lexnv pushed a commit that referenced this issue Apr 3, 2024
liuchengxu added a commit to subcoin-project/polkadot-sdk that referenced this issue Sep 20, 2024
* ci: dry run docs on pull request

* Fix mdbook
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: backlog
Development

No branches or pull requests

2 participants