contracts: Get rid of the dreaded Other error (#10595)

* Print more detailed error when instrumentation fails

* Apply suggestions from code review

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

* Check contents of debug buffer

* Fix test

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

substrate/frame/contracts/fixtures/invalid_import.wat

@@ -0,0 +1,6 @@
+;; A valid contract which does nothing at all but imports an invalid function
+(module
+	(import "invalid" "invalid_88_99" (func (param i32 i32 i32)))
+	(func (export "deploy"))
+	(func (export "call"))
+)

substrate/frame/contracts/src/lib.rs

@@ -632,6 +632,10 @@ pub mod pallet {
 		/// or via RPC an `Ok` will be returned. In this case the caller needs to inspect the flags
 		/// to determine whether a reversion has taken place.
 		ContractReverted,
+		/// The contract's code was found to be invalid during validation or instrumentation.
+		/// A more detailed error can be found on the node console if debug messages are enabled
+		/// or in the debug buffer which is returned to RPC clients.
+		CodeRejected,
 	}
 
 	/// A mapping from an original code hash to the original code, untouched by instrumentation.
@@ -781,7 +785,8 @@ where
 		storage_deposit_limit: Option<BalanceOf<T>>,
 	) -> CodeUploadResult<CodeHash<T>, BalanceOf<T>> {
 		let schedule = T::Schedule::get();
-		let module = PrefabWasmModule::from_code(code, &schedule, origin)?;
+		let module = PrefabWasmModule::from_code(code, &schedule, origin)
+			.map_err(|_| <Error<T>>::CodeRejected)?;
 		let deposit = module.open_deposit();
 		if let Some(storage_deposit_limit) = storage_deposit_limit {
 			ensure!(storage_deposit_limit >= deposit, <Error<T>>::StorageDepositLimitExhausted);
@@ -879,7 +884,7 @@ where
 		code: Code<CodeHash<T>>,
 		data: Vec<u8>,
 		salt: Vec<u8>,
-		debug_message: Option<&mut Vec<u8>>,
+		mut debug_message: Option<&mut Vec<u8>>,
 	) -> InternalInstantiateOutput<T> {
 		let mut storage_deposit = Default::default();
 		let mut gas_meter = GasMeter::new(gas_limit);
@@ -891,8 +896,11 @@ where
 						binary.len() as u32 <= schedule.limits.code_len,
 						<Error<T>>::CodeTooLarge
 					);
-					let executable =
-						PrefabWasmModule::from_code(binary, &schedule, origin.clone())?;
+					let executable = PrefabWasmModule::from_code(binary, &schedule, origin.clone())
+						.map_err(|msg| {
+							debug_message.as_mut().map(|buffer| buffer.extend(msg.as_bytes()));
+							<Error<T>>::CodeRejected
+						})?;
 					ensure!(
 						executable.code_len() <= schedule.limits.code_len,
 						<Error<T>>::CodeTooLarge

substrate/frame/contracts/src/tests.rs

@@ -1386,7 +1386,7 @@ fn disabled_chain_extension_wont_deploy() {
 				vec![],
 				vec![],
 			),
-			"module uses chain extensions but chain extensions are disabled",
+			<Error<Test>>::CodeRejected,
 		);
 	});
 }
@@ -2903,3 +2903,32 @@ fn contract_reverted() {
 		assert_eq!(result.data.0, buffer);
 	});
 }
+
+#[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);
+
+		assert_noop!(
+			Contracts::upload_code(Origin::signed(ALICE), wasm.clone(), None),
+			<Error<Test>>::CodeRejected,
+		);
+
+		let result = Contracts::bare_instantiate(
+			ALICE,
+			0,
+			GAS_LIMIT,
+			None,
+			Code::Upload(Bytes(wasm)),
+			vec![],
+			vec![],
+			true,
+		);
+		assert_err!(result.result, <Error<Test>>::CodeRejected);
+		assert_eq!(
+			std::str::from_utf8(&result.debug_message).unwrap(),
+			"module imports a non-existent function"
+		);
+	});
+}

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

@@ -122,8 +122,8 @@ where
 		original_code: Vec<u8>,
 		schedule: &Schedule<T>,
 		owner: AccountIdOf<T>,
-	) -> Result<Self, DispatchError> {
-		prepare::prepare_contract(original_code, schedule, owner).map_err(Into::into)
+	) -> Result<Self, &'static str> {
+		prepare::prepare_contract(original_code, schedule, owner)
 	}
 
 	/// Store the code without instantiating it.

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

@@ -372,26 +372,34 @@ fn check_and_instrument<C: ImportSatisfyCheck, T: Config>(
 	original_code: &[u8],
 	schedule: &Schedule<T>,
 ) -> Result<(Vec<u8>, (u32, u32)), &'static str> {
-	let contract_module = ContractModule::new(&original_code, schedule)?;
-	contract_module.scan_exports()?;
-	contract_module.ensure_no_internal_memory()?;
-	contract_module.ensure_table_size_limit(schedule.limits.table_size)?;
-	contract_module.ensure_global_variable_limit(schedule.limits.globals)?;
-	contract_module.ensure_no_floating_types()?;
-	contract_module.ensure_parameter_limit(schedule.limits.parameters)?;
-	contract_module.ensure_br_table_size_limit(schedule.limits.br_table_size)?;
+	let result = (|| {
+		let contract_module = ContractModule::new(&original_code, schedule)?;
+		contract_module.scan_exports()?;
+		contract_module.ensure_no_internal_memory()?;
+		contract_module.ensure_table_size_limit(schedule.limits.table_size)?;
+		contract_module.ensure_global_variable_limit(schedule.limits.globals)?;
+		contract_module.ensure_no_floating_types()?;
+		contract_module.ensure_parameter_limit(schedule.limits.parameters)?;
+		contract_module.ensure_br_table_size_limit(schedule.limits.br_table_size)?;
 
-	// We disallow importing `gas` function here since it is treated as implementation detail.
-	let disallowed_imports = [b"gas".as_ref()];
-	let memory_limits =
-		get_memory_limits(contract_module.scan_imports::<C>(&disallowed_imports)?, schedule)?;
+		// We disallow importing `gas` function here since it is treated as implementation detail.
+		let disallowed_imports = [b"gas".as_ref()];
+		let memory_limits =
+			get_memory_limits(contract_module.scan_imports::<C>(&disallowed_imports)?, schedule)?;
 
-	let code = contract_module
-		.inject_gas_metering()?
-		.inject_stack_height_metering()?
-		.into_wasm_code()?;
+		let code = contract_module
+			.inject_gas_metering()?
+			.inject_stack_height_metering()?
+			.into_wasm_code()?;
 
-	Ok((code, memory_limits))
+		Ok((code, memory_limits))
+	})();
+
+	if let Err(msg) = &result {
+		log::debug!(target: "runtime::contracts", "CodeRejected: {}", msg);
+	}
+
+	result
 }
 
 fn do_preparation<C: ImportSatisfyCheck, T: Config>(