-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Upgrade wasmtime #8714
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
Upgrade wasmtime #8714
Conversation
|
/cmd fmt |
| "gc", | ||
| "gc-null", | ||
| "parallel-compilation", | ||
| "pooling-allocator", | ||
| "threads", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The garbage collection and threads features should not be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I know, but I activated them to deactivate them in the config. Otherwise someone downstream, who maybe imports wasmtime with the default feature activated (which activates the threads and gc features) will silently activate the stuff, because they are true by default.
| // they should be introduced here as well. | ||
| config.wasm_reference_types(semantics.wasm_reference_types); | ||
| config.wasm_simd(semantics.wasm_simd); | ||
| config.wasm_relaxed_simd(semantics.wasm_simd); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the newest wasmtime has some extra new knobs besides this that are now true by default: wasm_extended_const, wasm_tail_call
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
|
/cmd prdoc --bump major --audience node_dev |
…-audience node_dev'
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
|
Now the latest version is 34.0.1, and the 33.0.0 seems yanked (replaced by 33.0.1) because of |
|
@s0me0ne-unkn0wn @koute do you have an idea how much we can trust the |
Should be reliable, as long as there are no warnings generated by the script. (Of course there's always a chance that there could be a weird bug/corner case, but if it worked so far then it probably still works? :P) |
I'm just asking because I needed to modify the expected calls :D |
s0me0ne-unkn0wn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fine (but if everything explodes, don't quote me). I'd be much more concerned about possible regressions, such as compilation time, execution time, stack usage, etc, given that we're switching versions in a non-deterministic way. I think we had plans to have tooling to at least check all the existing on-chain code for such regressions?
Yes, but not sure this manifested into anything concrete? 🙈 |
| - audience: Node Dev | ||
| description: This upgrades wasmtime to the latest version and also fixes backtraces | ||
| for `debug` builds. | ||
| crates: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have majorly bumped all crates depending on wasmtime too here? I'll give you an example. sp-wasm-interface depends on wasmtime. If no bump is introduced here, no publishing will happen for it in next release and it will still depend on older wasmtime.
However, sc-executor-wasmtime has a major bump, meaning it will be released in a new version depending on wasmtime 35.0.0.
But, sc-executor-wasmtime has a dependency on sp-wasmtime-interface, and more specifically, relies on this trait (https://github.com/paritytech/polkadot-sdk/blob/master/substrate/primitives/wasm-interface/src/lib.rs#L344) here (https://github.com/paritytech/polkadot-sdk/blob/master/substrate/client/executor/wasmtime/src/imports.rs#L95). Our parity-publish check fails since some time ago (before this PR), but I think if successful and made mandatory it would've catched this. Later merged PRs show such error in parity-publish failing worklfow.
error[E0053]: method `with_function_context` has an incompatible type for trait
--> substrate/client/executor/wasmtime/src/imports.rs:101:11
|
101 | caller: wasmtime::Caller<Self::State>,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `Caller<'_, StoreData>`, found `wasmtime::Caller<'_, StoreData>`
|
= note: expected signature `fn(sp_wasm_interface::wasmtime::Caller<'_, StoreData>, _) -> _`
found signature `fn(wasmtime::Caller<'_, StoreData>, _) -> _`
help: change the parameter type to match the trait
|
101 - caller: wasmtime::Caller<Self::State>,
101 + caller: sp_wasm_interface::wasmtime::Caller<'_, StoreData>,
|
I think we should consider making parity-publish check mandatory on master, to avoid issues with missing bumps when releasing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is more of a problem of parity-publish actually. What I observed is that whenever we bump a crate, all the crates depending on the bumped crates might get a bump too based on how they depend on the initially bumped crates (eg. usually we depend on explicit versions - by virtue of having all crates in a monorepo workspace - so any bump attracts bumps of dependents I think).
But if we bump a crate not part of the monorepo/workspace, I think parity-publish can't tell all dependents (and their dependents and so on) that need to be bumped as a result.
LE: major bumps for crates attract bumps for dependents.
# Description Adds a few crate bumps associated to PRs which missed to bump them, and updates parity-publish version across the board to 0.10.6 (to support rustc 1.88). Additionally, makes it so that parity-publish-check-compile runs first on all unreleased prdocs to bump associated crates, and only after moving those to an `unreleased` directory, runs on the current PR's prdoc. This is so that we first create a "local release" based on the unreleased prdocs, and then we follow with a "patch" release based on the previous local release, considering only the prdoc pushed with the current PR. If the workflow fails at the end it means current PR missed certain bumps. If we don't do the plan/apply twice we risk to miss bumps due to all prdocs being considered (current PR's prdoc + unreleased ones) when running parity-publish plan/apply, which might result in a set of crate bumps which are sufficient, but once some unreleased prdocs will be moved to a stable prodoc directory, because they will be part of a stable release, then the ones left will not be enough from a bump perspective (e.g. like it happened in #9320). That's why it is important to check every PR that adds a prdoc whether it is self-sufficient from a crates bumping perspective. If no prdoc is provided, the parity-publish does not need to be taken into consideration, but it should also pass nonetheless. ## Integration N/A ## Review Notes There seems to a be a corner case parity-publish can not easily catch. All bumps below are a manifestation of it. More details below: * #8714 - a major bump is necessary for `sp-wasm-interface` - context here: #8714 (comment) * `sp-keystore` was bumped during 2506 in #6010 , and the relevant prdoc got moved to stable2506 dir in #9320. This moved prdoc coexisted alongside other unreleased prdocs, and covered a needed patch bump for `sp-keystore`, that is not easily visible, and also required for crates publishing IIUC: 1. `sp-io` is major bumped because its direct dependency, `sp-state-machine`, was major bumped. 2. `sp-io` has a direct dependency on `sp-core` (minor bumped), and `sp-keystore` (not touched, not bumped by now) 3. `sp-io` fails to compile because it pulls same types from different `sp-core` versions (it implements `Keystore` trait from `sp-keystore` with methods signatures referencing types from `sp-core 38.0.0` by using the `sp-core 0.38.1` - unreleased yet - types, which confuses rustc). * `sp-rpc` needs a bump too due to pulling `sp-core 38.0.0`, like `sp-keystore`, and it is an indirect dependency of `polkadot-cli`, which has also a direct dependency on unreleased `sp-core 38.1.0`, so again, if we don't bump `sp-rpc` (historically it has been bumped only with major, but I think we can go with patch on this one), `polkadot-cli` can't compile. * `sc-storage-monitor` is in a similar situation as `sp-rpc`/`sp-keystore` - `polkadot-cli` depends on `sc-storage-monitor` (which is not bumped, and has a dependency on `sp-core 38.0.0`), but it also depends on `sp-core 38.1.0`. And yet again, something is used in `polkadot-cli` from the two different `sp-core` versions, which confuses rustc. --------- Signed-off-by: Iulian Barbu <[email protected]> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Alexander Samusev <[email protected]>
# Description Adds a few crate bumps associated to PRs which missed to bump them, and updates parity-publish version across the board to 0.10.6 (to support rustc 1.88). Additionally, makes it so that parity-publish-check-compile runs first on all unreleased prdocs to bump associated crates, and only after moving those to an `unreleased` directory, runs on the current PR's prdoc. This is so that we first create a "local release" based on the unreleased prdocs, and then we follow with a "patch" release based on the previous local release, considering only the prdoc pushed with the current PR. If the workflow fails at the end it means current PR missed certain bumps. If we don't do the plan/apply twice we risk to miss bumps due to all prdocs being considered (current PR's prdoc + unreleased ones) when running parity-publish plan/apply, which might result in a set of crate bumps which are sufficient, but once some unreleased prdocs will be moved to a stable prodoc directory, because they will be part of a stable release, then the ones left will not be enough from a bump perspective (e.g. like it happened in #9320). That's why it is important to check every PR that adds a prdoc whether it is self-sufficient from a crates bumping perspective. If no prdoc is provided, the parity-publish does not need to be taken into consideration, but it should also pass nonetheless. ## Integration N/A ## Review Notes There seems to a be a corner case parity-publish can not easily catch. All bumps below are a manifestation of it. More details below: * #8714 - a major bump is necessary for `sp-wasm-interface` - context here: #8714 (comment) * `sp-keystore` was bumped during 2506 in #6010 , and the relevant prdoc got moved to stable2506 dir in #9320. This moved prdoc coexisted alongside other unreleased prdocs, and covered a needed patch bump for `sp-keystore`, that is not easily visible, and also required for crates publishing IIUC: 1. `sp-io` is major bumped because its direct dependency, `sp-state-machine`, was major bumped. 2. `sp-io` has a direct dependency on `sp-core` (minor bumped), and `sp-keystore` (not touched, not bumped by now) 3. `sp-io` fails to compile because it pulls same types from different `sp-core` versions (it implements `Keystore` trait from `sp-keystore` with methods signatures referencing types from `sp-core 38.0.0` by using the `sp-core 0.38.1` - unreleased yet - types, which confuses rustc). * `sp-rpc` needs a bump too due to pulling `sp-core 38.0.0`, like `sp-keystore`, and it is an indirect dependency of `polkadot-cli`, which has also a direct dependency on unreleased `sp-core 38.1.0`, so again, if we don't bump `sp-rpc` (historically it has been bumped only with major, but I think we can go with patch on this one), `polkadot-cli` can't compile. * `sc-storage-monitor` is in a similar situation as `sp-rpc`/`sp-keystore` - `polkadot-cli` depends on `sc-storage-monitor` (which is not bumped, and has a dependency on `sp-core 38.0.0`), but it also depends on `sp-core 38.1.0`. And yet again, something is used in `polkadot-cli` from the two different `sp-core` versions, which confuses rustc. --------- Signed-off-by: Iulian Barbu <[email protected]> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Alexander Samusev <[email protected]>
# Goal The goal of this PR is to fix failures in `make lint-audit`. # Discussion - `slab` was yanked, so it was updated to version v0.4.11 - `tracing-subscriber` reported a RUSTSEC vulnerability warning and was updated to v0.3.20 - Additional crates were updated due to dependencies on these crates. - A new `Cargo.lock` file was generated. - Updating `tracing-subscriber` required Rust `edition2024` and therefore the rust toolchain was updated to `1.88` - The github actions container was updated to use `1.88` and that version was updated: `1.5.8` - Rust toolchain `1.88` required code edits to eliminate several warnings that have been mainlined. These are mostly around strings and some clippy warnings. - Most of these changes were grabbed from a recent polkadot SDK update, courtesy of @saraswatpuneet - `fxhash` was flagged as `unmaintained`. It is a `polkadot-sdk` dependency from `wasmtime`. This [Parity PR](paritytech/polkadot-sdk#8714) has updated `wasmtime` which removes the dependency on `fxhash`. - The RUSTSEC unmaintained warning was added to the `deny.toml` ignore list until the above PR is merged into a stable release. - `yamux` updated to 0.13.6, possible solution to some network instability Co-authored-by: Puneet Saraswat <[email protected]>
This upgrades wasmtime to the latest version and also fixes backtraces for `debug` builds. --------- Signed-off-by: Oliver Tale-Yazdi <[email protected]> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Oliver Tale-Yazdi <[email protected]>
# Description Adds a few crate bumps associated to PRs which missed to bump them, and updates parity-publish version across the board to 0.10.6 (to support rustc 1.88). Additionally, makes it so that parity-publish-check-compile runs first on all unreleased prdocs to bump associated crates, and only after moving those to an `unreleased` directory, runs on the current PR's prdoc. This is so that we first create a "local release" based on the unreleased prdocs, and then we follow with a "patch" release based on the previous local release, considering only the prdoc pushed with the current PR. If the workflow fails at the end it means current PR missed certain bumps. If we don't do the plan/apply twice we risk to miss bumps due to all prdocs being considered (current PR's prdoc + unreleased ones) when running parity-publish plan/apply, which might result in a set of crate bumps which are sufficient, but once some unreleased prdocs will be moved to a stable prodoc directory, because they will be part of a stable release, then the ones left will not be enough from a bump perspective (e.g. like it happened in #9320). That's why it is important to check every PR that adds a prdoc whether it is self-sufficient from a crates bumping perspective. If no prdoc is provided, the parity-publish does not need to be taken into consideration, but it should also pass nonetheless. ## Integration N/A ## Review Notes There seems to a be a corner case parity-publish can not easily catch. All bumps below are a manifestation of it. More details below: * #8714 - a major bump is necessary for `sp-wasm-interface` - context here: #8714 (comment) * `sp-keystore` was bumped during 2506 in #6010 , and the relevant prdoc got moved to stable2506 dir in #9320. This moved prdoc coexisted alongside other unreleased prdocs, and covered a needed patch bump for `sp-keystore`, that is not easily visible, and also required for crates publishing IIUC: 1. `sp-io` is major bumped because its direct dependency, `sp-state-machine`, was major bumped. 2. `sp-io` has a direct dependency on `sp-core` (minor bumped), and `sp-keystore` (not touched, not bumped by now) 3. `sp-io` fails to compile because it pulls same types from different `sp-core` versions (it implements `Keystore` trait from `sp-keystore` with methods signatures referencing types from `sp-core 38.0.0` by using the `sp-core 0.38.1` - unreleased yet - types, which confuses rustc). * `sp-rpc` needs a bump too due to pulling `sp-core 38.0.0`, like `sp-keystore`, and it is an indirect dependency of `polkadot-cli`, which has also a direct dependency on unreleased `sp-core 38.1.0`, so again, if we don't bump `sp-rpc` (historically it has been bumped only with major, but I think we can go with patch on this one), `polkadot-cli` can't compile. * `sc-storage-monitor` is in a similar situation as `sp-rpc`/`sp-keystore` - `polkadot-cli` depends on `sc-storage-monitor` (which is not bumped, and has a dependency on `sp-core 38.0.0`), but it also depends on `sp-core 38.1.0`. And yet again, something is used in `polkadot-cli` from the two different `sp-core` versions, which confuses rustc. --------- Signed-off-by: Iulian Barbu <[email protected]> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Alexander Samusev <[email protected]>
This upgrades wasmtime to the latest version and also fixes backtraces for `debug` builds. --------- Signed-off-by: Oliver Tale-Yazdi <[email protected]> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Oliver Tale-Yazdi <[email protected]>
This upgrades wasmtime to the latest version and also fixes backtraces for
debugbuilds.