executor: Move runtime version caching out of WasmRuntime interface. (#3993)

* 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.

* executor: Require successful querying of runtime version.

substrate/core/executor/src/lib.rs

@@ -73,7 +73,6 @@ pub fn call_in_wasm<E: Externalities>(
 	heap_pages: u64,
 ) -> error::Result<Vec<u8>> {
 	let mut instance = wasm_runtime::create_wasm_runtime_with_code(
-		ext,
 		execution_method,
 		heap_pages,
 		code,

substrate/core/executor/src/native_executor.rs

@@ -110,12 +110,13 @@ impl<D: NativeExecutionDispatch> NativeExecutor<D> {
 		ext: &mut E,
 		f: impl for<'a> FnOnce(
 			AssertUnwindSafe<&'a mut (dyn WasmRuntime + 'static)>,
+			&'a RuntimeVersion,
 			AssertUnwindSafe<&'a mut E>,
 		) -> Result<Result<R>>,
 	) -> Result<R> 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<D: NativeExecutionDispatch> NativeExecutor<D> {
 			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,8 +156,8 @@ impl<D: NativeExecutionDispatch> RuntimeInfo for NativeExecutor<D> {
 		&self,
 		ext: &mut E,
 	) -> Option<RuntimeVersion> {
-		match self.with_runtime(ext, |runtime, _ext| Ok(Ok(runtime.version()))) {
-			Ok(version) => version,
+		match self.with_runtime(ext, |_runtime, version, _ext| Ok(Ok(version.clone()))) {
+			Ok(version) => Some(version),
 			Err(e) => {
 				warn!(target: "executor", "Failed to fetch runtime: {:?}", e);
 				None
@@ -182,13 +183,10 @@ impl<D: NativeExecutionDispatch> CodeExecutor for NativeExecutor<D> {
 		native_call: Option<NC>,
 	) -> (Result<NativeOrEncoded<R>>, 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
-					.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<D: NativeExecutionDispatch> CodeExecutor for NativeExecutor<D> {
 						"Request for native execution failed (native: {}, chain: {})",
 						self.native_version.runtime_version,
 						onchain_version
-							.as_ref()
-							.map_or_else(||"<None>".into(), |v| format!("{}", v))
 					);
 
 					safe_call(
@@ -216,8 +212,6 @@ impl<D: NativeExecutionDispatch> CodeExecutor for NativeExecutor<D> {
 						"Request for native execution with native call succeeded (native: {}, chain: {}).",
 						self.native_version.runtime_version,
 						onchain_version
-							.as_ref()
-							.map_or_else(||"<None>".into(), |v| format!("{}", v))
 					);
 
 					used_native = true;
@@ -234,7 +228,7 @@ impl<D: NativeExecutionDispatch> CodeExecutor for NativeExecutor<D> {
 						target: "executor",
 						"Request for native execution succeeded (native: {}, chain: {})",
 						self.native_version.runtime_version,
-						onchain_version.as_ref().map_or_else(||"<None>".into(), |v| format!("{}", v))
+						onchain_version
 					);
 
 					used_native = true;

substrate/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<Vec<u8>, 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<RuntimeVersion>;
 }
 
 /// Specification of different methods of executing the runtime Wasm code.
@@ -58,6 +52,13 @@ pub enum WasmExecutionMethod {
 	Compiled,
 }
 
+/// A Wasm runtime object along with its cached runtime version.
+struct VersionedRuntime {
+	runtime: Box<dyn WasmRuntime>,
+	/// Runtime version according to `Core_version`.
+	version: RuntimeVersion,
+}
+
 /// Cache for the runtimes.
 ///
 /// When an instance is requested for the first time it is added to this cache. Metadata is kept
@@ -74,7 +75,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<Box<dyn WasmRuntime>, WasmError>>,
+	instances: HashMap<(WasmExecutionMethod, [u8; 32]), Result<VersionedRuntime, WasmError>>,
 }
 
 impl RuntimesCache {
@@ -97,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.
 	///
@@ -118,7 +118,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), &RuntimeVersion, 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 +132,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 +147,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 +156,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 +176,51 @@ impl RuntimesCache {
 }
 
 /// Create a wasm runtime with the given `code`.
-pub fn create_wasm_runtime_with_code<E: Externalities>(
-	ext: &mut E,
+pub fn create_wasm_runtime_with_code(
 	wasm_method: WasmExecutionMethod,
 	heap_pages: u64,
 	code: &[u8],
 ) -> Result<Box<dyn WasmRuntime>, WasmError> {
 	match wasm_method {
 		WasmExecutionMethod::Interpreted =>
-			wasmi_execution::create_instance(ext, code, heap_pages)
+			wasmi_execution::create_instance(code, heap_pages)
 				.map(|runtime| -> Box<dyn WasmRuntime> { Box::new(runtime) }),
 		#[cfg(feature = "wasmtime")]
 		WasmExecutionMethod::Compiled =>
-			wasmtime::create_instance(ext, code, heap_pages)
+			wasmtime::create_instance(code, heap_pages)
 				.map(|runtime| -> Box<dyn WasmRuntime> { Box::new(runtime) }),
 	}
 }
 
-fn create_wasm_runtime<E: Externalities>(
+fn create_versioned_wasm_runtime<E: Externalities>(
 	ext: &mut E,
 	wasm_method: WasmExecutionMethod,
 	heap_pages: u64,
-) -> Result<Box<dyn WasmRuntime>, WasmError> {
+) -> Result<VersionedRuntime, WasmError> {
 	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 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,
+		version,
+	})
 }

substrate/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<RuntimeVersion>,
 	/// 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<RuntimeVersion> {
-		self.version.clone()
-	}
 }
 
-pub fn create_instance<E: Externalities>(ext: &mut E, code: &[u8], heap_pages: u64)
-	-> Result<WasmiRuntime, WasmError>
-{
+pub fn create_instance(code: &[u8], heap_pages: u64) -> Result<WasmiRuntime, WasmError> {
 	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<E: Externalities>(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,
 	})
 }

substrate/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<u32>,
 	heap_pages: u32,
-	version: Option<RuntimeVersion>,
 }
 
 impl WasmRuntime for WasmtimeRuntime {
@@ -75,18 +72,14 @@ impl WasmRuntime for WasmtimeRuntime {
 			self.heap_pages,
 		)
 	}
-
-	fn version(&self) -> Option<RuntimeVersion> {
-		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<E: Externalities>(ext: &mut E, code: &[u8], heap_pages: u64)
+pub fn create_instance(code: &[u8], heap_pages: u64)
 	-> std::result::Result<WasmtimeRuntime, WasmError>
 {
-	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<E: Externalities>(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,
 	})
 }