Bump wasmtime to 0.31.0
#10149
Bump wasmtime to 0.31.0
#10149
Conversation
koute
added
A0-please_review
koute
added
C1-low
client/executor/wasmtime/src/host.rs
Outdated
| @@ -333,6 +333,7 @@ impl<'a, 'b, 'c, 'd> sandbox::SandboxContext for SandboxContext<'a, 'b, 'c, 'd> | |||
| state: u32, | |||
| func_idx: SupervisorFuncIndex, | |||
| ) -> Result<i64> { | |||
| let mut ret_vals = Vec::new(); | |||
There was a problem hiding this comment.
Actually, I think we do know the signature.
extern "C" fn dispatch_thunk<T>(
serialized_args_ptr: *const u8,
serialized_args_len: usize,
state: usize,
f: ffi::HostFuncIndex,
) -> u64 { ... }so we can actually get away with an array of one item here.
There was a problem hiding this comment.
Actually, I think I screwed up here, because Func::call takes a &mut [Val] instead of a &mut Vec<Val>, so it can't work, and yet... no tests actually fail? I'll check this.
There was a problem hiding this comment.
Okay, I had to dig through many unfamiliar layers of code, but I can confirm this is indeed not being tested at all.
In the sp-sandbox we have two alternative codepaths which are picked during compile time: embedded_executor and host_executor. Only the host_executor goes through the Sandbox runtime interface and actually triggers this code. However it is not(feature = "std")-only, and the std feature is always enabled on that crate, so in consequence the embedded_executor codepath is always compiled in.
The sp-sandbox itself is (in the tests which are supposed to test this code, according to what @pepyakin told me) invoked from a #[cfg(not(feature = "std"))] function in sc-runtime-test, so I'm guessing the current situation is totally unintended and (for these tests) the host_executor should always be used; maybe someone somewhere accidentally transitively enabled the std feature on sp-sandbox and we just never noticed since nothing actually failed?
I'll fix it so that this actually gets tested.
There was a problem hiding this comment.
and the std feature is always enabled on that crate
Are you talking about this logic?
mod imp {
#[cfg(all(feature = "wasmer-sandbox", not(feature = "std")))]
include!("../host_executor.rs");
#[cfg(not(all(feature = "wasmer-sandbox", not(feature = "std"))))]
include!("../embedded_executor.rs");
}I was under the impression that std is not activated when it is compiled as part of the wasm runtime.
There was a problem hiding this comment.
That is correct. I think the problem is more that currently host_executor is not being used because wasmer is deactivated.
There was a problem hiding this comment.
Yes this is on purpose. Generally, we want the embedded executor for contracts (which is wasmi) because polkadot validators are missing the sandboxing host functions.
However, the experimental wasmer-sandbox feature can be enabled on the command line (it is done by one CI test) in order to to testing and benchmarking with wasmer. The intention is to keep it around until we can fully switch to wasmer. wasmer cannot be embedded because it is a compiler which is inherently platform dependent.
There was a problem hiding this comment.
@athei Yes; for the sandbox integration tests the embedded_executor is always selected. I might have not been entirely correct when I said the std feature is always enabled, but there is something weird going on with the feature flags.
Anyway, I've put up a separate PR to try and get both codepaths tested:
|
Now that this is properly tested (test now fail when |
|
bot merge |
|
Error: It's not possible to meet the minimal approval count of 1 in paritytech/polkadot#4210 |
|
bot merge |
|
Error: It's not possible to meet the minimal approval count of 1 in paritytech/polkadot#4210 |
|
bot merge |
* Bump `wasmtime` to 0.31.0 * Bump `itoa` to 0.4.8 * sc-executor-wasmtime: fix `SandboxContext::invoke` which I've broke * sc-executor-wasmtime: cargo fmt
* Bump `wasmtime` to 0.31.0 * Bump `itoa` to 0.4.8 * sc-executor-wasmtime: fix `SandboxContext::invoke` which I've broke * sc-executor-wasmtime: cargo fmt
polkadot companion: paritytech/polkadot#4210