From f139af1eb6e72d4ae64433bc5b21a1bf58d0670f Mon Sep 17 00:00:00 2001 From: Jim Posen Date: Fri, 1 Nov 2019 14:21:08 +0100 Subject: [PATCH 1/2] executor: Move runtime caching out of WasmRuntime interface. The runtime version is now fetched and cached at a higher level, not within the WasmRuntime trait implementations. --- core/executor/src/lib.rs | 1 - core/executor/src/native_executor.rs | 10 ++--- core/executor/src/wasm_runtime.rs | 64 ++++++++++++++++++--------- core/executor/src/wasmi_execution.rs | 27 ++--------- core/executor/src/wasmtime/runtime.rs | 40 ++--------------- 5 files changed, 55 insertions(+), 87 deletions(-) diff --git a/core/executor/src/lib.rs b/core/executor/src/lib.rs index f053718b3aef5..d6d666dd28647 100644 --- a/core/executor/src/lib.rs +++ b/core/executor/src/lib.rs @@ -73,7 +73,6 @@ pub fn call_in_wasm( heap_pages: u64, ) -> error::Result> { let mut instance = wasm_runtime::create_wasm_runtime_with_code( - ext, execution_method, heap_pages, code, diff --git a/core/executor/src/native_executor.rs b/core/executor/src/native_executor.rs index 082f0ba2adc33..aa940c0c85010 100644 --- a/core/executor/src/native_executor.rs +++ b/core/executor/src/native_executor.rs @@ -110,12 +110,13 @@ impl NativeExecutor { ext: &mut E, f: impl for<'a> FnOnce( AssertUnwindSafe<&'a mut (dyn WasmRuntime + 'static)>, + &'a Option, AssertUnwindSafe<&'a mut E>, ) -> Result>, ) -> Result where E: Externalities { RUNTIMES_CACHE.with(|cache| { let mut cache = cache.borrow_mut(); - let (runtime, code_hash) = cache.fetch_runtime( + let (runtime, version, code_hash) = cache.fetch_runtime( ext, self.fallback_method, self.default_heap_pages, @@ -124,7 +125,7 @@ impl NativeExecutor { let runtime = AssertUnwindSafe(runtime); let ext = AssertUnwindSafe(ext); - match f(runtime, ext) { + match f(runtime, version, ext) { Ok(res) => res, Err(e) => { cache.invalidate_runtime(self.fallback_method, code_hash); @@ -155,7 +156,7 @@ impl RuntimeInfo for NativeExecutor { &self, ext: &mut E, ) -> Option { - match self.with_runtime(ext, |runtime, _ext| Ok(Ok(runtime.version()))) { + match self.with_runtime(ext, |_runtime, version, _ext| Ok(Ok(version.clone()))) { Ok(version) => version, Err(e) => { warn!(target: "executor", "Failed to fetch runtime: {:?}", e); @@ -182,8 +183,7 @@ impl CodeExecutor for NativeExecutor { native_call: Option, ) -> (Result>, bool){ let mut used_native = false; - let result = self.with_runtime(ext, |mut runtime, mut ext| { - let onchain_version = runtime.version(); + let result = self.with_runtime(ext, |mut runtime, onchain_version, mut ext| { match ( use_native, onchain_version diff --git a/core/executor/src/wasm_runtime.rs b/core/executor/src/wasm_runtime.rs index 447dbadde5c81..64e4ea03e52c1 100644 --- a/core/executor/src/wasm_runtime.rs +++ b/core/executor/src/wasm_runtime.rs @@ -27,7 +27,7 @@ use log::{trace, warn}; use codec::Decode; use primitives::{storage::well_known_keys, traits::Externalities, H256}; use runtime_version::RuntimeVersion; -use std::{collections::hash_map::{Entry, HashMap}}; +use std::{collections::hash_map::{Entry, HashMap}, panic::AssertUnwindSafe}; /// The Substrate Wasm runtime. pub trait WasmRuntime { @@ -40,12 +40,6 @@ pub trait WasmRuntime { /// Call a method in the Substrate runtime by name. Returns the encoded result on success. fn call(&mut self, ext: &mut dyn Externalities, method: &str, data: &[u8]) -> Result, Error>; - - /// Returns the version of this runtime. - /// - /// Returns `None` if the runtime doesn't provide the information or there was an error - /// while fetching it. - fn version(&self) -> Option; } /// Specification of different methods of executing the runtime Wasm code. @@ -58,6 +52,16 @@ pub enum WasmExecutionMethod { Compiled, } +/// A Wasm runtime object along with its cached runtime version. +struct VersionedRuntime { + runtime: Box, + /// Runtime version according to `Core_version`. + /// + /// Can be `None` if the runtime doesn't provide the information or there was an error while + /// fetching it. + version: Option, +} + /// Cache for the runtimes. /// /// When an instance is requested for the first time it is added to this cache. Metadata is kept @@ -74,7 +78,7 @@ pub struct RuntimesCache { /// A cache of runtime instances along with metadata, ready to be reused. /// /// Instances are keyed by the Wasm execution method and the hash of their code. - instances: HashMap<(WasmExecutionMethod, [u8; 32]), Result, WasmError>>, + instances: HashMap<(WasmExecutionMethod, [u8; 32]), Result>, } impl RuntimesCache { @@ -118,7 +122,7 @@ impl RuntimesCache { ext: &mut E, wasm_method: WasmExecutionMethod, default_heap_pages: u64, - ) -> Result<(&mut (dyn WasmRuntime + 'static), H256), Error> { + ) -> Result<(&mut (dyn WasmRuntime + 'static), &Option, H256), Error> { let code_hash = ext .original_storage_hash(well_known_keys::CODE) .ok_or(Error::InvalidCode("`CODE` not found in storage.".into()))?; @@ -132,12 +136,12 @@ impl RuntimesCache { Entry::Occupied(o) => { let result = o.into_mut(); if let Ok(ref mut cached_runtime) = result { - if !cached_runtime.update_heap_pages(heap_pages) { + if !cached_runtime.runtime.update_heap_pages(heap_pages) { trace!( target: "runtimes_cache", "heap_pages were changed. Reinstantiating the instance", ); - *result = create_wasm_runtime(ext, wasm_method, heap_pages); + *result = create_versioned_wasm_runtime(ext, wasm_method, heap_pages); if let Err(ref err) = result { warn!(target: "runtimes_cache", "cannot create a runtime: {:?}", err); } @@ -147,7 +151,7 @@ impl RuntimesCache { }, Entry::Vacant(v) => { trace!(target: "runtimes_cache", "no instance found in cache, creating now."); - let result = create_wasm_runtime(ext, wasm_method, heap_pages); + let result = create_versioned_wasm_runtime(ext, wasm_method, heap_pages); if let Err(ref err) = result { warn!(target: "runtimes_cache", "cannot create a runtime: {:?}", err); } @@ -156,7 +160,7 @@ impl RuntimesCache { }; result.as_mut() - .map(|runtime| (runtime.as_mut(), code_hash)) + .map(|entry| (entry.runtime.as_mut(), &entry.version, code_hash)) .map_err(|ref e| Error::InvalidCode(format!("{:?}", e))) } @@ -176,30 +180,50 @@ impl RuntimesCache { } /// Create a wasm runtime with the given `code`. -pub fn create_wasm_runtime_with_code( - ext: &mut E, +pub fn create_wasm_runtime_with_code( wasm_method: WasmExecutionMethod, heap_pages: u64, code: &[u8], ) -> Result, WasmError> { match wasm_method { WasmExecutionMethod::Interpreted => - wasmi_execution::create_instance(ext, code, heap_pages) + wasmi_execution::create_instance(code, heap_pages) .map(|runtime| -> Box { Box::new(runtime) }), #[cfg(feature = "wasmtime")] WasmExecutionMethod::Compiled => - wasmtime::create_instance(ext, code, heap_pages) + wasmtime::create_instance(code, heap_pages) .map(|runtime| -> Box { Box::new(runtime) }), } } -fn create_wasm_runtime( +fn create_versioned_wasm_runtime( ext: &mut E, wasm_method: WasmExecutionMethod, heap_pages: u64, -) -> Result, WasmError> { +) -> Result { let code = ext .original_storage(well_known_keys::CODE) .ok_or(WasmError::CodeNotFound)?; - create_wasm_runtime_with_code(ext, wasm_method, heap_pages, &code) + let mut runtime = create_wasm_runtime_with_code(wasm_method, heap_pages, &code)?; + + // Call to determine runtime version. + let version_result = { + // `ext` is already implicitly handled as unwind safe, as we store it in a global variable. + let mut ext = AssertUnwindSafe(ext); + + // The following unwind safety assertion is OK because if the method call panics, the + // runtime will be dropped. + let mut runtime = AssertUnwindSafe(runtime.as_mut()); + crate::native_executor::safe_call( + move || runtime.call(&mut **ext, "Core_version", &[]) + ).map_err(|_| WasmError::Instantiation("panic in call to get runtime version".into()))? + }; + let version = version_result + .ok() + .and_then(|v| RuntimeVersion::decode(&mut v.as_slice()).ok()); + + Ok(VersionedRuntime { + runtime, + version, + }) } diff --git a/core/executor/src/wasmi_execution.rs b/core/executor/src/wasmi_execution.rs index b6af30b7ee185..dcd6ae89094bc 100644 --- a/core/executor/src/wasmi_execution.rs +++ b/core/executor/src/wasmi_execution.rs @@ -16,7 +16,7 @@ //! Implementation of a Wasm runtime using the Wasmi interpreter. -use std::{str, mem, panic::AssertUnwindSafe}; +use std::{str, mem}; use wasmi::{ Module, ModuleInstance, MemoryInstance, MemoryRef, TableRef, ImportsBuilder, ModuleRef, memory_units::Pages, RuntimeValue::{I32, I64, self}, @@ -31,7 +31,6 @@ use crate::wasm_utils::interpret_runtime_api_result; use crate::wasm_runtime::WasmRuntime; use log::trace; use parity_wasm::elements::{deserialize_buffer, DataSegment, Instruction, Module as RawModule}; -use runtime_version::RuntimeVersion; use wasm_interface::{ FunctionContext, HostFunctions, Pointer, WordSize, Sandbox, MemoryId, Result as WResult, }; @@ -553,15 +552,11 @@ impl StateSnapshot { } } -/// A runtime along with its version and initial state snapshot. +/// A runtime along with its initial state snapshot. #[derive(Clone)] pub struct WasmiRuntime { /// A wasm module instance. instance: ModuleRef, - /// Runtime version according to `Core_version`. - /// - /// Can be `None` if the runtime doesn't expose this function. - version: Option, /// The snapshot of the instance's state taken just after the instantiation. state_snapshot: StateSnapshot, } @@ -595,15 +590,9 @@ impl WasmRuntime for WasmiRuntime { call_in_wasm_module(ext, module, method, data) }) } - - fn version(&self) -> Option { - self.version.clone() - } } -pub fn create_instance(ext: &mut E, code: &[u8], heap_pages: u64) - -> Result -{ +pub fn create_instance(code: &[u8], heap_pages: u64) -> Result { let module = Module::from_buffer(&code).map_err(|_| WasmError::InvalidModule)?; // Extract the data segments from the wasm code. @@ -625,18 +614,8 @@ pub fn create_instance(ext: &mut E, code: &[u8], heap_pages: u ", ); - let mut ext = AssertUnwindSafe(ext); - let call_instance = AssertUnwindSafe(&instance); - let version_result = crate::native_executor::safe_call( - move || call_in_wasm_module(&mut **ext, *call_instance, "Core_version", &[]) - ).map_err(|_| WasmError::Instantiation("panic in call to get runtime version".to_string()))?; - let version = version_result - .ok() - .and_then(|v| RuntimeVersion::decode(&mut v.as_slice()).ok()); - Ok(WasmiRuntime { instance, - version, state_snapshot, }) } diff --git a/core/executor/src/wasmtime/runtime.rs b/core/executor/src/wasmtime/runtime.rs index fa360773fb28c..da668e9c30999 100644 --- a/core/executor/src/wasmtime/runtime.rs +++ b/core/executor/src/wasmtime/runtime.rs @@ -23,9 +23,8 @@ use crate::wasm_utils::interpret_runtime_api_result; use crate::wasmtime::function_executor::FunctionExecutorState; use crate::wasmtime::trampoline::{EnvState, make_trampoline}; use crate::wasmtime::util::{cranelift_ir_signature, read_memory_into, write_memory_from}; -use crate::{Externalities, RuntimeVersion}; +use crate::Externalities; -use codec::Decode; use cranelift_codegen::ir; use cranelift_codegen::isa::TargetIsa; use cranelift_entity::{EntityRef, PrimaryMap}; @@ -34,7 +33,6 @@ use cranelift_wasm::DefinedFuncIndex; use std::cell::RefCell; use std::collections::HashMap; use std::convert::TryFrom; -use std::panic::AssertUnwindSafe; use std::rc::Rc; use wasm_interface::{HostFunctions, Pointer, WordSize}; use wasmtime_environ::{Module, translate_signature}; @@ -51,7 +49,6 @@ pub struct WasmtimeRuntime { context: Context, max_heap_pages: Option, heap_pages: u32, - version: Option, } impl WasmRuntime for WasmtimeRuntime { @@ -75,18 +72,14 @@ impl WasmRuntime for WasmtimeRuntime { self.heap_pages, ) } - - fn version(&self) -> Option { - self.version.clone() - } } /// Create a new `WasmtimeRuntime` given the code. This function performs translation from Wasm to /// machine code, which can be computationally heavy. -pub fn create_instance(ext: &mut E, code: &[u8], heap_pages: u64) +pub fn create_instance(code: &[u8], heap_pages: u64) -> std::result::Result { - let (mut compiled_module, mut context) = create_compiled_unit(code)?; + let (compiled_module, context) = create_compiled_unit(code)?; // Inspect the module for the min and max memory sizes. let (min_memory_size, max_memory_size) = { @@ -105,38 +98,11 @@ pub fn create_instance(ext: &mut E, code: &[u8], heap_pages: u let heap_pages = heap_pages_valid(heap_pages, max_heap_pages) .ok_or_else(|| WasmError::InvalidHeapPages)?; - // Call to determine runtime version. - let version_result = { - // `ext` is already implicitly handled as unwind safe, as we store it in a global variable. - let mut ext = AssertUnwindSafe(ext); - - // The following unwind safety assertions are OK because if the method call panics, the - // context and compiled module will be dropped. - let mut context = AssertUnwindSafe(&mut context); - let mut compiled_module = AssertUnwindSafe(&mut compiled_module); - crate::native_executor::safe_call(move || { - call_method( - &mut **context, - &mut **compiled_module, - &mut **ext, - "Core_version", - &[], - heap_pages - ) - }).map_err(|_| { - WasmError::Instantiation("panic in call to get runtime version".to_string()) - })? - }; - let version = version_result - .ok() - .and_then(|v| RuntimeVersion::decode(&mut v.as_slice()).ok()); - Ok(WasmtimeRuntime { module: compiled_module, context, max_heap_pages, heap_pages, - version, }) } From a3f49cda314dfc47ca7982d38757b33c24c8c905 Mon Sep 17 00:00:00 2001 From: Jim Posen Date: Fri, 1 Nov 2019 16:44:05 +0100 Subject: [PATCH 2/2] executor: Require successful querying of runtime version. --- core/executor/src/native_executor.rs | 14 ++++---------- core/executor/src/wasm_runtime.rs | 17 +++++++---------- 2 files changed, 11 insertions(+), 20 deletions(-) diff --git a/core/executor/src/native_executor.rs b/core/executor/src/native_executor.rs index aa940c0c85010..0fb84d9972b96 100644 --- a/core/executor/src/native_executor.rs +++ b/core/executor/src/native_executor.rs @@ -110,7 +110,7 @@ impl NativeExecutor { ext: &mut E, f: impl for<'a> FnOnce( AssertUnwindSafe<&'a mut (dyn WasmRuntime + 'static)>, - &'a Option, + &'a RuntimeVersion, AssertUnwindSafe<&'a mut E>, ) -> Result>, ) -> Result where E: Externalities { @@ -157,7 +157,7 @@ impl RuntimeInfo for NativeExecutor { ext: &mut E, ) -> Option { match self.with_runtime(ext, |_runtime, version, _ext| Ok(Ok(version.clone()))) { - Ok(version) => version, + Ok(version) => Some(version), Err(e) => { warn!(target: "executor", "Failed to fetch runtime: {:?}", e); None @@ -186,9 +186,7 @@ impl CodeExecutor for NativeExecutor { let result = self.with_runtime(ext, |mut runtime, onchain_version, mut ext| { match ( use_native, - onchain_version - .as_ref() - .map_or(false, |v| v.can_call_with(&self.native_version.runtime_version)), + onchain_version.can_call_with(&self.native_version.runtime_version), native_call, ) { (_, false, _) => { @@ -197,8 +195,6 @@ impl CodeExecutor for NativeExecutor { "Request for native execution failed (native: {}, chain: {})", self.native_version.runtime_version, onchain_version - .as_ref() - .map_or_else(||"".into(), |v| format!("{}", v)) ); safe_call( @@ -216,8 +212,6 @@ impl CodeExecutor for NativeExecutor { "Request for native execution with native call succeeded (native: {}, chain: {}).", self.native_version.runtime_version, onchain_version - .as_ref() - .map_or_else(||"".into(), |v| format!("{}", v)) ); used_native = true; @@ -234,7 +228,7 @@ impl CodeExecutor for NativeExecutor { target: "executor", "Request for native execution succeeded (native: {}, chain: {})", self.native_version.runtime_version, - onchain_version.as_ref().map_or_else(||"".into(), |v| format!("{}", v)) + onchain_version ); used_native = true; diff --git a/core/executor/src/wasm_runtime.rs b/core/executor/src/wasm_runtime.rs index 64e4ea03e52c1..caaa01c43027e 100644 --- a/core/executor/src/wasm_runtime.rs +++ b/core/executor/src/wasm_runtime.rs @@ -56,10 +56,7 @@ pub enum WasmExecutionMethod { struct VersionedRuntime { runtime: Box, /// Runtime version according to `Core_version`. - /// - /// Can be `None` if the runtime doesn't provide the information or there was an error while - /// fetching it. - version: Option, + version: RuntimeVersion, } /// Cache for the runtimes. @@ -101,8 +98,7 @@ impl RuntimesCache { /// # Parameters /// /// `ext` - Externalities to use for the runtime. This is used for setting - /// up an initial runtime instance. The parameter is only needed for calling - /// into the Wasm module to find out the `Core_version`. + /// up an initial runtime instance. /// /// `default_heap_pages` - Number of 64KB pages to allocate for Wasm execution. /// @@ -122,7 +118,7 @@ impl RuntimesCache { ext: &mut E, wasm_method: WasmExecutionMethod, default_heap_pages: u64, - ) -> Result<(&mut (dyn WasmRuntime + 'static), &Option, H256), Error> { + ) -> Result<(&mut (dyn WasmRuntime + 'static), &RuntimeVersion, H256), Error> { let code_hash = ext .original_storage_hash(well_known_keys::CODE) .ok_or(Error::InvalidCode("`CODE` not found in storage.".into()))?; @@ -218,9 +214,10 @@ fn create_versioned_wasm_runtime( move || runtime.call(&mut **ext, "Core_version", &[]) ).map_err(|_| WasmError::Instantiation("panic in call to get runtime version".into()))? }; - let version = version_result - .ok() - .and_then(|v| RuntimeVersion::decode(&mut v.as_slice()).ok()); + let encoded_version = version_result + .map_err(|e| WasmError::Instantiation(format!("failed to call \"Core_version\": {}", e)))?; + let version = RuntimeVersion::decode(&mut encoded_version.as_slice()) + .map_err(|_| WasmError::Instantiation("failed to decode \"Core_version\" result".into()))?; Ok(VersionedRuntime { runtime,