pallet-revive: Adjust error handling of sub calls (paritytech#6741)

We were trapping the host context in case a sub call was exhausting the
storage deposit limit set for this sub call. This prevents the caller
from handling this error. In this PR we added a new error code that is
returned when either gas or storage deposit limit is exhausted by the
sub call.

We also remove the longer used `NotCallable` error. No longer used
because this is no longer an error: It will just be a balance transfer.

We also make `set_code_hash` infallible to be consistent with other host
functions which just trap on any error condition.

---------

Co-authored-by: GitHub Action <action@github.com>

prdoc/pr_6741.prdoc

@@ -0,0 +1,16 @@
+title: 'pallet-revive: Adjust error handling of sub calls'
+doc:
+- audience: Runtime Dev
+  description: |-
+    We were trapping the host context in case a sub call was exhausting the storage deposit limit set for this sub call. This prevents the caller from handling this error. In this PR we added a new error code that is returned when either gas or storage deposit limit is exhausted by the sub call.
+
+    We also remove the longer used `NotCallable` error. No longer used because this is no longer an error: It will just be a balance transfer.
+
+    We also make `set_code_hash` infallible to be consistent with other host functions which just trap on any error condition.
+crates:
+- name: pallet-revive
+  bump: major
+- name: pallet-revive-uapi
+  bump: major
+- name: pallet-revive-fixtures
+  bump: major

substrate/frame/revive/fixtures/contracts/caller_contract.rs

@@ -65,7 +65,7 @@ pub extern "C" fn call() {
 		None,
 		Some(&salt),
 	);
-	assert!(matches!(res, Err(ReturnErrorCode::CalleeTrapped)));
+	assert!(matches!(res, Err(ReturnErrorCode::OutOfResources)));
 
 	// Fail to deploy the contract due to insufficient proof_size weight.
 	let res = api::instantiate(
@@ -79,7 +79,7 @@ pub extern "C" fn call() {
 		None,
 		Some(&salt),
 	);
-	assert!(matches!(res, Err(ReturnErrorCode::CalleeTrapped)));
+	assert!(matches!(res, Err(ReturnErrorCode::OutOfResources)));
 
 	// Deploy the contract successfully.
 	let mut callee = [0u8; 20];
@@ -121,7 +121,7 @@ pub extern "C" fn call() {
 		&input,
 		None,
 	);
-	assert!(matches!(res, Err(ReturnErrorCode::CalleeTrapped)));
+	assert!(matches!(res, Err(ReturnErrorCode::OutOfResources)));
 
 	// Fail to call the contract due to insufficient proof_size weight.
 	let res = api::call(
@@ -134,7 +134,7 @@ pub extern "C" fn call() {
 		&input,
 		None,
 	);
-	assert!(matches!(res, Err(ReturnErrorCode::CalleeTrapped)));
+	assert!(matches!(res, Err(ReturnErrorCode::OutOfResources)));
 
 	// Call the contract successfully.
 	let mut output = [0u8; 4];

substrate/frame/revive/fixtures/contracts/create_storage_and_call.rs

@@ -40,7 +40,7 @@ pub extern "C" fn call() {
 	api::set_storage(StorageFlags::empty(), buffer, &[1u8; 4]);
 
 	// Call the callee
-	api::call(
+	let ret = api::call(
 		uapi::CallFlags::empty(),
 		callee,
 		0u64, // How much ref_time weight to devote for the execution. 0 = all.
@@ -49,8 +49,10 @@ pub extern "C" fn call() {
 		&[0u8; 32], // Value transferred to the contract.
 		input,
 		None,
-	)
-	.unwrap();
+	);
+	if let Err(code) = ret {
+		api::return_value(uapi::ReturnFlags::REVERT, &(code as u32).to_le_bytes());
+	};
 
 	// create 8 byte of storage after calling
 	// item of 12 bytes because we override 4 bytes

substrate/frame/revive/fixtures/contracts/create_storage_and_instantiate.rs

@@ -39,7 +39,7 @@ pub extern "C" fn call() {
 	let salt = [0u8; 32];
 	let mut address = [0u8; 20];
 
-	api::instantiate(
+	let ret = api::instantiate(
 		code_hash,
 		0u64, // How much ref_time weight to devote for the execution. 0 = all.
 		0u64, // How much proof_size weight to devote for the execution. 0 = all.
@@ -49,8 +49,10 @@ pub extern "C" fn call() {
 		Some(&mut address),
 		None,
 		Some(&salt),
-	)
-	.unwrap();
+	);
+	if let Err(code) = ret {
+		api::return_value(uapi::ReturnFlags::REVERT, &(code as u32).to_le_bytes());
+	};
 
 	// Return the deployed contract address.
 	api::return_value(uapi::ReturnFlags::empty(), &address);

substrate/frame/revive/fixtures/contracts/delegate_call_deposit_limit.rs

@@ -34,7 +34,11 @@ pub extern "C" fn call() {
 	);
 
 	let input = [0u8; 0];
-	api::delegate_call(uapi::CallFlags::empty(), address, 0, 0, Some(&u256_bytes(deposit_limit)), &input, None).unwrap();
+	let ret = api::delegate_call(uapi::CallFlags::empty(), address, 0, 0, Some(&u256_bytes(deposit_limit)), &input, None);
+
+	if let Err(code) = ret {
+		api::return_value(uapi::ReturnFlags::REVERT, &(code as u32).to_le_bytes());
+	};
 
 	let mut key = [0u8; 32];
 	key[0] = 1u8;

substrate/frame/revive/fixtures/contracts/set_code_hash.rs

@@ -29,7 +29,7 @@ pub extern "C" fn deploy() {}
 #[polkavm_derive::polkavm_export]
 pub extern "C" fn call() {
 	input!(addr: &[u8; 32],);
-	api::set_code_hash(addr).unwrap();
+	api::set_code_hash(addr);
 
 	// we return 1 after setting new code_hash
 	// next `call` will NOT return this value, because contract code has been changed

substrate/frame/revive/src/exec.rs

@@ -1068,7 +1068,7 @@ where
 
 		self.transient_storage.start_transaction();
 
-		let do_transaction = || {
+		let do_transaction = || -> ExecResult {
 			let caller = self.caller();
 			let frame = top_frame_mut!(self);
 
@@ -1107,11 +1107,8 @@ where
 			let call_span = T::Debug::new_call_span(&contract_address, entry_point, &input_data);
 
 			let output = T::Debug::intercept_call(&contract_address, entry_point, &input_data)
-				.unwrap_or_else(|| {
-					executable
-						.execute(self, entry_point, input_data)
-						.map_err(|e| ExecError { error: e.error, origin: ErrorOrigin::Callee })
-				})?;
+				.unwrap_or_else(|| executable.execute(self, entry_point, input_data))
+				.map_err(|e| ExecError { error: e.error, origin: ErrorOrigin::Callee })?;
 
 			call_span.after_call(&output);
 
@@ -1136,7 +1133,10 @@ where
 			// If a special limit was set for the sub-call, we enforce it here.
 			// The sub-call will be rolled back in case the limit is exhausted.
 			let contract = frame.contract_info.as_contract();
-			frame.nested_storage.enforce_subcall_limit(contract)?;
+			frame
+				.nested_storage
+				.enforce_subcall_limit(contract)
+				.map_err(|e| ExecError { error: e, origin: ErrorOrigin::Callee })?;
 
 			let account_id = T::AddressMapper::to_address(&frame.account_id);
 			match (entry_point, delegated_code_hash) {
@@ -3412,7 +3412,7 @@ mod tests {
 					true,
 					false
 				),
-				<Error<Test>>::TransferFailed
+				<Error<Test>>::TransferFailed,
 			);
 			exec_success()
 		});

substrate/frame/revive/src/tests.rs

@@ -38,8 +38,8 @@ use crate::{
 	wasm::Memory,
 	weights::WeightInfo,
 	AccountId32Mapper, BalanceOf, Code, CodeInfoOf, CollectEvents, Config, ContractInfo,
-	ContractInfoOf, DebugInfo, DeletionQueueCounter, Error, HoldReason, Origin, Pallet,
-	PristineCode, H160,
+	ContractInfoOf, DebugInfo, DeletionQueueCounter, DepositLimit, Error, HoldReason, Origin,
+	Pallet, PristineCode, H160,
 };
 
 use crate::test_utils::builder::Contract;
@@ -1211,14 +1211,11 @@ fn delegate_call_with_deposit_limit() {
 
 		// Delegate call will write 1 storage and deposit of 2 (1 item) + 32 (bytes) is required.
 		// Fails, not enough deposit
-		assert_err!(
-			builder::bare_call(caller_addr)
-				.value(1337)
-				.data((callee_addr, 33u64).encode())
-				.build()
-				.result,
-			Error::<Test>::StorageDepositLimitExhausted,
-		);
+		let ret = builder::bare_call(caller_addr)
+			.value(1337)
+			.data((callee_addr, 33u64).encode())
+			.build_and_unwrap_result();
+		assert_return_code!(ret, RuntimeReturnCode::OutOfResources);
 
 		assert_ok!(builder::call(caller_addr)
 			.value(1337)
@@ -1678,8 +1675,8 @@ fn instantiate_return_code() {
 
 		// Contract has enough balance but the passed code hash is invalid
 		<Test as Config>::Currency::set_balance(&contract.account_id, min_balance + 10_000);
-		let result = builder::bare_call(contract.addr).data(vec![0; 33]).build_and_unwrap_result();
-		assert_return_code!(result, RuntimeReturnCode::CodeNotFound);
+		let result = builder::bare_call(contract.addr).data(vec![0; 33]).build();
+		assert_err!(result.result, <Error<Test>>::CodeNotFound);
 
 		// Contract has enough balance but callee reverts because "1" is passed.
 		let result = builder::bare_call(contract.addr)
@@ -3463,13 +3460,11 @@ fn deposit_limit_in_nested_calls() {
 		// nested call. This should fail as callee adds up 2 bytes to the storage, meaning
 		// that the nested call should have a deposit limit of at least 2 Balance. The
 		// sub-call should be rolled back, which is covered by the next test case.
-		assert_err_ignore_postinfo!(
-			builder::call(addr_caller)
-				.storage_deposit_limit(16)
-				.data((102u32, &addr_callee, U256::from(1u64)).encode())
-				.build(),
-			<Error<Test>>::StorageDepositLimitExhausted,
-		);
+		let ret = builder::bare_call(addr_caller)
+			.storage_deposit_limit(DepositLimit::Balance(16))
+			.data((102u32, &addr_callee, U256::from(1u64)).encode())
+			.build_and_unwrap_result();
+		assert_return_code!(ret, RuntimeReturnCode::OutOfResources);
 
 		// Refund in the callee contract but not enough to cover the 14 Balance required by the
 		// caller. Note that if previous sub-call wouldn't roll back, this call would pass
@@ -3485,13 +3480,11 @@ fn deposit_limit_in_nested_calls() {
 		let _ = <Test as Config>::Currency::set_balance(&ALICE, 511);
 
 		// Require more than the sender's balance.
-		// We don't set a special limit for the nested call.
-		assert_err_ignore_postinfo!(
-			builder::call(addr_caller)
-				.data((512u32, &addr_callee, U256::from(1u64)).encode())
-				.build(),
-			<Error<Test>>::StorageDepositLimitExhausted,
-		);
+		// Limit the sub call to little balance so it should fail in there
+		let ret = builder::bare_call(addr_caller)
+			.data((512u32, &addr_callee, U256::from(1u64)).encode())
+			.build_and_unwrap_result();
+		assert_return_code!(ret, RuntimeReturnCode::OutOfResources);
 
 		// Same as above but allow for the additional deposit of 1 Balance in parent.
 		// We set the special deposit limit of 1 Balance for the nested call, which isn't
@@ -3563,29 +3556,25 @@ fn deposit_limit_in_nested_instantiate() {
 		// Now we set enough limit in parent call, but an insufficient limit for child
 		// instantiate. This should fail during the charging for the instantiation in
 		// `RawMeter::charge_instantiate()`
-		assert_err_ignore_postinfo!(
-			builder::call(addr_caller)
-				.origin(RuntimeOrigin::signed(BOB))
-				.storage_deposit_limit(callee_info_len + 2 + ED + 2)
-				.data((0u32, &code_hash_callee, U256::from(callee_info_len + 2 + ED + 1)).encode())
-				.build(),
-			<Error<Test>>::StorageDepositLimitExhausted,
-		);
+		let ret = builder::bare_call(addr_caller)
+			.origin(RuntimeOrigin::signed(BOB))
+			.storage_deposit_limit(DepositLimit::Balance(callee_info_len + 2 + ED + 2))
+			.data((0u32, &code_hash_callee, U256::from(callee_info_len + 2 + ED + 1)).encode())
+			.build_and_unwrap_result();
+		assert_return_code!(ret, RuntimeReturnCode::OutOfResources);
 		// The charges made on the instantiation should be rolled back.
 		assert_eq!(<Test as Config>::Currency::free_balance(&BOB), 1_000_000);
 
 		// Same as above but requires for single added storage
 		// item of 1 byte to be covered by the limit, which implies 3 more Balance.
 		// Now we set enough limit for the parent call, but insufficient limit for child
 		// instantiate. This should fail right after the constructor execution.
-		assert_err_ignore_postinfo!(
-			builder::call(addr_caller)
-				.origin(RuntimeOrigin::signed(BOB))
-				.storage_deposit_limit(callee_info_len + 2 + ED + 3) // enough parent limit
-				.data((1u32, &code_hash_callee, U256::from(callee_info_len + 2 + ED + 2)).encode())
-				.build(),
-			<Error<Test>>::StorageDepositLimitExhausted,
-		);
+		let ret = builder::bare_call(addr_caller)
+			.origin(RuntimeOrigin::signed(BOB))
+			.storage_deposit_limit(DepositLimit::Balance(callee_info_len + 2 + ED + 3)) // enough parent limit
+			.data((1u32, &code_hash_callee, U256::from(callee_info_len + 2 + ED + 2)).encode())
+			.build_and_unwrap_result();
+		assert_return_code!(ret, RuntimeReturnCode::OutOfResources);
 		// The charges made on the instantiation should be rolled back.
 		assert_eq!(<Test as Config>::Currency::free_balance(&BOB), 1_000_000);
 
@@ -3890,7 +3879,7 @@ fn locking_delegate_dependency_works() {
 		assert_ok!(Contracts::remove_code(RuntimeOrigin::signed(ALICE_FALLBACK), callee_hashes[0]));
 
 		// Calling should fail since the delegated contract is not on chain anymore.
-		assert_err!(call(&addr_caller, &noop_input).result, Error::<Test>::ContractTrapped);
+		assert_err!(call(&addr_caller, &noop_input).result, Error::<Test>::CodeNotFound);
 
 		// Add the dependency back.
 		Contracts::upload_code(

substrate/frame/revive/src/wasm/runtime.rs

@@ -745,29 +745,24 @@ impl<'a, E: Ext, M: ?Sized + Memory<E::T>> Runtime<'a, E, M> {
 		Ok(())
 	}
 
-	/// Fallible conversion of `DispatchError` to `ReturnErrorCode`.
-	fn err_into_return_code(from: DispatchError) -> Result<ReturnErrorCode, DispatchError> {
-		use ReturnErrorCode::*;
-
-		let transfer_failed = Error::<E::T>::TransferFailed.into();
-		let no_code = Error::<E::T>::CodeNotFound.into();
-		let not_found = Error::<E::T>::ContractNotFound.into();
-
-		match from {
-			x if x == transfer_failed => Ok(TransferFailed),
-			x if x == no_code => Ok(CodeNotFound),
-			x if x == not_found => Ok(NotCallable),
-			err => Err(err),
-		}
-	}
-
 	/// Fallible conversion of a `ExecError` to `ReturnErrorCode`.
+	///
+	/// This is used when converting the error returned from a subcall in order to decide
+	/// whether to trap the caller or allow handling of the error.
 	fn exec_error_into_return_code(from: ExecError) -> Result<ReturnErrorCode, DispatchError> {
 		use crate::exec::ErrorOrigin::Callee;
+		use ReturnErrorCode::*;
 
+		let transfer_failed = Error::<E::T>::TransferFailed.into();
+		let out_of_gas = Error::<E::T>::OutOfGas.into();
+		let out_of_deposit = Error::<E::T>::StorageDepositLimitExhausted.into();
+
+		// errors in the callee do not trap the caller
 		match (from.error, from.origin) {
-			(_, Callee) => Ok(ReturnErrorCode::CalleeTrapped),
-			(err, _) => Self::err_into_return_code(err),
+			(err, _) if err == transfer_failed => Ok(TransferFailed),
+			(err, Callee) if err == out_of_gas || err == out_of_deposit => Ok(OutOfResources),
+			(_, Callee) => Ok(CalleeTrapped),
+			(err, _) => Err(err),
 		}
 	}
 
@@ -1971,20 +1966,11 @@ pub mod env {
 	/// Disabled until the internal implementation takes care of collecting
 	/// the immutable data of the new code hash.
 	#[mutating]
-	fn set_code_hash(
-		&mut self,
-		memory: &mut M,
-		code_hash_ptr: u32,
-	) -> Result<ReturnErrorCode, TrapReason> {
+	fn set_code_hash(&mut self, memory: &mut M, code_hash_ptr: u32) -> Result<(), TrapReason> {
 		self.charge_gas(RuntimeCosts::SetCodeHash)?;
 		let code_hash: H256 = memory.read_h256(code_hash_ptr)?;
-		match self.ext.set_code_hash(code_hash) {
-			Err(err) => {
-				let code = Self::err_into_return_code(err)?;
-				Ok(code)
-			},
-			Ok(()) => Ok(ReturnErrorCode::Success),
-		}
+		self.ext.set_code_hash(code_hash)?;
+		Ok(())
 	}
 
 	/// Calculates Ethereum address from the ECDSA compressed public key and stores