Do not re-check code on reinstrument

frame/contracts/Cargo.toml

@@ -26,6 +26,7 @@ smallvec = { version = "1", default-features = false, features = [
 	"const_generics",
 ] }
 wasmi = { version = "0.18", default-features = false }
+wasmparser = { package = "wasmparser-nostd", version = "0.91" }
 impl-trait-for-tuples = "0.2"
 
 # Only used in benchmarking to generate random contract code

frame/contracts/fixtures/invalid_contract.wat

@@ -0,0 +1,4 @@
+;; Valid module but missing the call function
+(module
+	(func (export "deploy"))
+)

frame/contracts/fixtures/invalid_module.wat

@@ -0,0 +1,8 @@
+;; An invalid module
+(module
+	(func (export "deploy"))
+	(func (export "call")
+		;; imbalanced stack
+		(i32.const 7)
+	)
+)

frame/contracts/src/lib.rs

@@ -102,7 +102,7 @@ use crate::{
 	exec::{AccountIdOf, ExecError, Executable, Stack as ExecStack},
 	gas::GasMeter,
 	storage::{meter::Meter as StorageMeter, ContractInfo, DeletedContract, Storage},
-	wasm::{OwnerInfo, PrefabWasmModule},
+	wasm::{OwnerInfo, PrefabWasmModule, TryInstantiate},
 	weights::WeightInfo,
 };
 use codec::{Codec, Encode, HasCompact};
@@ -997,7 +997,8 @@ where
 	) -> CodeUploadResult<CodeHash<T>, BalanceOf<T>> {
 		let schedule = T::Schedule::get();
 		let module =
-			PrefabWasmModule::from_code(code, &schedule, origin).map_err(|(err, _)| err)?;
+			PrefabWasmModule::from_code(code, &schedule, origin, TryInstantiate::Instantiate)
+				.map_err(|(err, _)| err)?;
 		let deposit = module.open_deposit();
 		if let Some(storage_deposit_limit) = storage_deposit_limit {
 			ensure!(storage_deposit_limit >= deposit, <Error<T>>::StorageDepositLimitExhausted);
@@ -1115,11 +1116,17 @@ where
 			let schedule = T::Schedule::get();
 			let (extra_deposit, executable) = match code {
 				Code::Upload(binary) => {
-					let executable = PrefabWasmModule::from_code(binary, &schedule, origin.clone())
-						.map_err(|(err, msg)| {
-							debug_message.as_mut().map(|buffer| buffer.extend(msg.as_bytes()));
-							err
-						})?;
+					let executable = PrefabWasmModule::from_code(
+						binary,
+						&schedule,
+						origin.clone(),
+						TryInstantiate::Skip,
+					)
+					.map_err(|(err, msg)| {
+						println!("found error");
+						debug_message.as_mut().map(|buffer| buffer.extend(msg.as_bytes()));
+						err
+					})?;
 					// The open deposit will be charged during execution when the
 					// uploaded module does not already exist. This deposit is not part of the
 					// storage meter because it is not transferred to the contract but

frame/contracts/src/tests.rs

@@ -3546,10 +3546,31 @@ fn contract_reverted() {
 
 #[test]
 fn code_rejected_error_works() {
-	let (wasm, _) = compile_module::<Test>("invalid_import").unwrap();
 	ExtBuilder::default().existential_deposit(200).build().execute_with(|| {
 		let _ = Balances::deposit_creating(&ALICE, 1_000_000);
 
+		let (wasm, _) = compile_module::<Test>("invalid_module").unwrap();
+		assert_noop!(
+			Contracts::upload_code(RuntimeOrigin::signed(ALICE), wasm.clone(), None),
+			<Error<Test>>::CodeRejected,
+		);
+		let result = Contracts::bare_instantiate(
+			ALICE,
+			0,
+			GAS_LIMIT,
+			None,
+			Code::Upload(wasm),
+			vec![],
+			vec![],
+			true,
+		);
+		assert_err!(result.result, <Error<Test>>::CodeRejected);
+		assert_eq!(
+			std::str::from_utf8(&result.debug_message).unwrap(),
+			"validation of new code failed"
+		);
+
+		let (wasm, _) = compile_module::<Test>("invalid_contract").unwrap();
 		assert_noop!(
 			Contracts::upload_code(RuntimeOrigin::signed(ALICE), wasm.clone(), None),
 			<Error<Test>>::CodeRejected,
@@ -3566,7 +3587,10 @@ fn code_rejected_error_works() {
 			true,
 		);
 		assert_err!(result.result, <Error<Test>>::CodeRejected);
-		assert_eq!(std::str::from_utf8(&result.debug_message).unwrap(), "validation failed");
+		assert_eq!(
+			std::str::from_utf8(&result.debug_message).unwrap(),
+			"call function isn't exported"
+		);
 	});
 }
 

frame/contracts/src/wasm/code_cache.rs

@@ -204,10 +204,8 @@ where
 	// as the contract is already deployed and every change in size would be the result
 	// of changes in the instrumentation algorithm controlled by the chain authors.
 	prefab_module.code = WeakBoundedVec::force_from(
-		prepare::check_and_instrument::<super::runtime::Env, T>(&original_code, schedule)
-			.map(|(code, _)| code)
-			.map_err(|_| <Error<T>>::CodeRejected)?,
-		Some("Contract exceeds limit after re-instrumentation."),
+		prepare::reinstrument::<super::runtime::Env, T>(&original_code, schedule)?,
+		Some("Contract exceeds size limit after re-instrumentation."),
 	);
 	prefab_module.instruction_weights_version = schedule.instruction_weights.version;
 	<CodeStorage<T>>::insert(&prefab_module.code_hash, &*prefab_module);

frame/contracts/src/wasm/mod.rs

@@ -28,6 +28,7 @@ mod runtime;
 pub use crate::wasm::code_cache::reinstrument;
 pub use crate::wasm::{
 	env_def::Environment,
+	prepare::TryInstantiate,
 	runtime::{CallFlags, ReturnCode, Runtime, RuntimeCosts},
 };
 use crate::{
@@ -126,11 +127,13 @@ where
 		original_code: Vec<u8>,
 		schedule: &Schedule<T>,
 		owner: AccountIdOf<T>,
+		try_instantiate: TryInstantiate,
 	) -> Result<Self, (DispatchError, &'static str)> {
-		let module = prepare::prepare_contract::<runtime::Env, T>(
+		let module = prepare::prepare::<runtime::Env, T>(
 			original_code.try_into().map_err(|_| (<Error<T>>::CodeTooLarge.into(), ""))?,
 			schedule,
 			owner,
+			try_instantiate,
 		)?;
 		Ok(module)
 	}
@@ -206,7 +209,7 @@ where
 		schedule: &Schedule<T>,
 		owner: T::AccountId,
 	) -> DispatchResult {
-		let executable = prepare::benchmarking::prepare_contract(original_code, schedule, owner)
+		let executable = prepare::benchmarking::prepare(original_code, schedule, owner)
 			.map_err::<DispatchError, _>(Into::into)?;
 		code_cache::store(executable, false)
 	}
@@ -259,7 +262,7 @@ where
 			(self.initial, self.maximum),
 		)
 		.map_err(|msg| {
-			log::error!(target: "runtime::contracts", "existing code rejected: {}", msg);
+			log::debug!(target: "runtime::contracts", "failed to instantiate code: {}", msg);
 			Error::<T>::CodeRejected
 		})?;
 		store.state_mut().set_memory(memory);
@@ -582,8 +585,13 @@ mod tests {
 	fn execute<E: BorrowMut<MockExt>>(wat: &str, input_data: Vec<u8>, mut ext: E) -> ExecResult {
 		let wasm = wat::parse_str(wat).unwrap();
 		let schedule = crate::Schedule::default();
-		let executable = PrefabWasmModule::<<MockExt as Ext>::T>::from_code(wasm, &schedule, ALICE)
-			.map_err(|err| err.0)?;
+		let executable = PrefabWasmModule::<<MockExt as Ext>::T>::from_code(
+			wasm,
+			&schedule,
+			ALICE,
+			TryInstantiate::Skip,
+		)
+		.map_err(|err| err.0)?;
 		executable.execute(ext.borrow_mut(), &ExportedFunction::Call, input_data)
 	}