Only validate when uploading

fix _unchecked naming convention

Only load module once when validating

Fixes

simplify

substrate/frame/contracts/src/benchmarking/sandbox.rs

@@ -20,7 +20,8 @@
 /// ! environment that provides the seal interface as imported functions.
 use super::{code::WasmModule, Config};
 use crate::wasm::{
-	AllowDeprecatedInterface, AllowUnstableInterface, Determinism, Environment, WasmBlob,
+	AllowDeprecatedInterface, AllowUnstableInterface, Determinism, Environment, LoadedModule,
+	WasmBlob,
 };
 use sp_core::Get;
 use wasmi::{errors::LinkerError, CompilationMode, Func, Linker, StackLimits, Store};
@@ -42,12 +43,18 @@ impl<T: Config> From<&WasmModule<T>> for Sandbox {
 	/// Creates an instance from the supplied module.
 	/// Sets the execution engine fuel level to `u64::MAX`.
 	fn from(module: &WasmModule<T>) -> Self {
-		let (mut store, _memory, instance) = WasmBlob::<T>::instantiate::<EmptyEnv, _>(
+		let contract = LoadedModule::new::<T>(
 			&module.code,
+			Determinism::Relaxed,
+			Some(StackLimits::default()),
+			CompilationMode::Eager,
+		)
+		.expect("Failed to load Wasm module");
+
+		let (mut store, _memory, instance) = WasmBlob::<T>::instantiate::<EmptyEnv, _>(
+			contract,
 			(),
 			&<T>::Schedule::get(),
-			Determinism::Relaxed,
-			StackLimits::default(),
 			// We are testing with an empty environment anyways
 			AllowDeprecatedInterface::No,
 			CompilationMode::Eager,

substrate/frame/contracts/src/tests.rs

@@ -1299,6 +1299,38 @@ fn delegate_call() {
 	});
 }
 
+#[test]
+fn track_check_uncheck_module_call() {
+	let (wasm, code_hash) = compile_module::<Test>("dummy").unwrap();
+	ExtBuilder::default().build().execute_with(|| {
+		let _ = <Test as Config>::Currency::set_balance(&ALICE, 1_000_000);
+
+		Contracts::bare_upload_code(ALICE, wasm, None, Determinism::Enforced).unwrap();
+
+		Contracts::bare_instantiate(
+			ALICE,
+			1_000,
+			GAS_LIMIT,
+			None,
+			Code::Existing(code_hash),
+			vec![],
+			vec![],
+			DebugInfo::Skip,
+			CollectEvents::Skip,
+		)
+		.result
+		.unwrap()
+		.account_id;
+	});
+
+	assert_eq!(
+		crate::wasm::LOADED_MODULE_TRACKER.with(|stack| stack.borrow().clone()),
+		// Module is loaded once checked (true) for upload, then once unchecked (false) when we
+		// instantiate it.
+		vec![true, false]
+	);
+}
+
 #[test]
 fn transfer_expendable_cannot_kill_account() {
 	let (wasm, _code_hash) = compile_module::<Test>("dummy").unwrap();

substrate/frame/contracts/src/wasm/mod.rs

@@ -27,20 +27,25 @@ pub use runtime::STABLE_API_COUNT;
 #[cfg(doc)]
 pub use crate::wasm::runtime::api_doc;
 
-pub use crate::wasm::runtime::{
-	AllowDeprecatedInterface, AllowUnstableInterface, Environment, Runtime, RuntimeCosts,
+pub use crate::wasm::{
+	prepare::LoadedModule,
+	runtime::{
+		AllowDeprecatedInterface, AllowUnstableInterface, Environment, Runtime, RuntimeCosts,
+	},
 };
 
 #[cfg(test)]
 pub use tests::MockExt;
 
+#[cfg(test)]
+pub use crate::wasm::prepare::LOADED_MODULE_TRACKER;
+
 #[cfg(test)]
 pub use crate::wasm::runtime::ReturnErrorCode;
 
 use crate::{
 	exec::{ExecResult, Executable, ExportedFunction, Ext},
 	gas::{GasMeter, Token},
-	wasm::prepare::LoadedModule,
 	weights::WeightInfo,
 	AccountIdOf, BadOrigin, BalanceOf, CodeHash, CodeInfoOf, CodeVec, Config, Error, Event,
 	HoldReason, Pallet, PristineCode, Schedule, Weight, LOG_TARGET,
@@ -201,19 +206,15 @@ impl<T: Config> WasmBlob<T> {
 	/// When validating we pass `()` as `host_state`. Please note that such a dummy instance must
 	/// **never** be called/executed, since it will panic the executor.
 	pub fn instantiate<E, H>(
-		code: &[u8],
+		contract: LoadedModule,
 		host_state: H,
 		schedule: &Schedule<T>,
-		determinism: Determinism,
-		stack_limits: StackLimits,
 		allow_deprecated: AllowDeprecatedInterface,
 		compilation_mode: CompilationMode,
 	) -> Result<(Store<H>, Memory, InstancePre), &'static str>
 	where
 		E: Environment<H>,
 	{
-		let contract =
-			LoadedModule::new::<T>(&code, determinism, Some(stack_limits), compilation_mode)?;
 		let mut store = Store::new(&contract.engine, host_state);
 		let mut linker = Linker::new(&contract.engine);
 		E::define(
@@ -360,12 +361,22 @@ impl<T: Config> Executable<T> for WasmBlob<T> {
 		// Instantiate the Wasm module to the engine.
 		let runtime = Runtime::new(ext, input_data);
 		let schedule = <T>::Schedule::get();
+
+		let contract = LoadedModule::new::<T>(
+			&code,
+			self.code_info.determinism,
+			Some(StackLimits::default()),
+			false,
+		)
+		.map_err(|err| {
+			log::debug!(target: LOG_TARGET, "failed to create wasmi module: {err:?}");
+			Error::<T>::CodeRejected
+		})?;
+
 		let (mut store, memory, instance) = Self::instantiate::<crate::wasm::runtime::Env, _>(
-			code,
+			contract,
 			runtime,
 			&schedule,
-			self.code_info.determinism,
-			StackLimits::default(),
 			match function {
 				ExportedFunction::Call => AllowDeprecatedInterface::Yes,
 				ExportedFunction::Constructor => AllowDeprecatedInterface::No,

substrate/frame/contracts/src/wasm/prepare.rs

@@ -37,6 +37,9 @@ use wasmi::{
 	Module, StackLimits,
 };
 
+#[cfg(test)]
+use core::cell::RefCell;
+
 /// Imported memory must be located inside this module. The reason for hardcoding is that current
 /// compiler toolchains might not support specifying other modules than "env" for memory imports.
 pub const IMPORT_MODULE_MEMORY: &str = "env";
@@ -48,6 +51,12 @@ pub struct LoadedModule {
 	pub engine: Engine,
 }
 
+#[cfg(test)]
+thread_local! {
+	/// Tack check(true) and uncheck(false) for each `LoadedModule` instance created in tests.
+	pub static LOADED_MODULE_TRACKER: RefCell<Vec<bool>> = RefCell::new(Vec::new());
+}
+
 impl LoadedModule {
 	/// Creates a new instance of `LoadedModule`.
 	///
@@ -58,6 +67,7 @@ impl LoadedModule {
 		determinism: Determinism,
 		stack_limits: Option<StackLimits>,
 		compilation_mode: CompilationMode,
+		validate_module: bool,
 	) -> Result<Self, &'static str> {
 		// NOTE: wasmi does not support unstable WebAssembly features. The module is implicitly
 		// checked for not having those ones when creating `wasmi::Module` below.
@@ -80,11 +90,20 @@ impl LoadedModule {
 		}
 
 		let engine = Engine::new(&config);
-		let module = Module::new(&engine, code).map_err(|err| {
+
+		let module = if validate_module {
+			Module::new(&engine, code)
+		} else {
+			unsafe { Module::new_unsafe(&engine, code) }
+		}
+		.map_err(|err| {
 			log::debug!(target: LOG_TARGET, "Module creation failed: {:?}", err);
 			"Can't load the module into wasmi!"
 		})?;
 
+		#[cfg(test)]
+		LOADED_MODULE_TRACKER.with(|t| t.borrow_mut().push(_validate_module));
+
 		// Return a `LoadedModule` instance with
 		// __valid__ module.
 		Ok(LoadedModule { module, engine })
@@ -230,16 +249,21 @@ where
 	E: Environment<()>,
 	T: Config,
 {
-	(|| {
+	let module = (|| {
+		// We don't actually ever execute this instance so we can get away with a minimal stack
+		// which reduces the amount of memory that needs to be zeroed.
+		let stack_limits = Some(StackLimits::new(1, 1, 0).expect("initial <= max; qed"));
+
 		// We check that the module is generally valid,
 		// and does not have restricted WebAssembly features, here.
 		let contract_module = match *determinism {
 			Determinism::Relaxed =>
 				if let Ok(module) = LoadedModule::new::<T>(
 					code,
 					Determinism::Enforced,
-					None,
+					stack_limits,
 					CompilationMode::Eager,
+					true,
 				) {
 					*determinism = Determinism::Enforced;
 					module
@@ -249,16 +273,22 @@ where
 						Determinism::Relaxed,
 						None,
 						CompilationMode::Eager,
+						true,
 					)?
 				},
-			Determinism::Enforced =>
-				LoadedModule::new::<T>(code, Determinism::Enforced, None, CompilationMode::Eager)?,
+			Determinism::Enforced => LoadedModule::new::<T>(
+				code,
+				Determinism::Enforced,
+				stack_limits,
+				CompilationMode::Eager,
+				true,
+			)?,
 		};
 
 		// The we check that module satisfies constraints the pallet puts on contracts.
 		contract_module.scan_exports()?;
 		contract_module.scan_imports::<T>(schedule)?;
-		Ok(())
+		Ok(contract_module)
 	})()
 	.map_err(|msg: &str| {
 		log::debug!(target: LOG_TARGET, "New code rejected on validation: {}", msg);
@@ -269,23 +299,11 @@ where
 	//
 	// - It doesn't use any unknown imports.
 	// - It doesn't explode the wasmi bytecode generation.
-	//
-	// We don't actually ever execute this instance so we can get away with a minimal stack which
-	// reduces the amount of memory that needs to be zeroed.
-	let stack_limits = StackLimits::new(1, 1, 0).expect("initial <= max; qed");
-	WasmBlob::<T>::instantiate::<E, _>(
-		&code,
-		(),
-		schedule,
-		*determinism,
-		stack_limits,
-		AllowDeprecatedInterface::No,
-		CompilationMode::Eager,
-	)
-	.map_err(|err| {
-		log::debug!(target: LOG_TARGET, "{}", err);
-		(Error::<T>::CodeRejected.into(), "New code rejected on wasmi instantiation!")
-	})?;
+	WasmBlob::<T>::instantiate::<E, _>(module, (), schedule, AllowDeprecatedInterface::No)
+		.map_err(|err| {
+			log::debug!(target: LOG_TARGET, "{err}");
+			(Error::<T>::CodeRejected.into(), "New code rejected on wasmi instantiation!")
+		})?;
 
 	Ok(())
 }
@@ -339,7 +357,7 @@ pub mod benchmarking {
 	) -> Result<WasmBlob<T>, DispatchError> {
 		let determinism = Determinism::Enforced;
 		let contract_module =
-			LoadedModule::new::<T>(&code, determinism, None, CompilationMode::Eager)?;
+			LoadedModule::new::<T>(&code, determinism, None, CompilationMode::Eager, true)?;
 		let _ = contract_module.scan_imports::<T>(schedule)?;
 		let code: CodeVec<T> = code.try_into().map_err(|_| <Error<T>>::CodeTooLarge)?;
 		let code_info = CodeInfo {