From f4068ea0344e00f25d1e127caba835ed5ddb9fe9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Thu, 5 Dec 2024 16:50:45 +0100 Subject: [PATCH] pallet-revive: Adjust error handling of sub calls (#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 --- prdoc/pr_6741.prdoc | 16 ++++ .../fixtures/contracts/caller_contract.rs | 8 +- .../contracts/create_storage_and_call.rs | 8 +- .../create_storage_and_instantiate.rs | 8 +- .../contracts/delegate_call_deposit_limit.rs | 6 +- .../fixtures/contracts/set_code_hash.rs | 2 +- substrate/frame/revive/src/exec.rs | 16 ++-- substrate/frame/revive/src/tests.rs | 75 ++++++++----------- substrate/frame/revive/src/wasm/runtime.rs | 46 ++++-------- substrate/frame/revive/uapi/src/host.rs | 12 +-- .../frame/revive/uapi/src/host/riscv64.rs | 7 +- substrate/frame/revive/uapi/src/lib.rs | 18 ++--- 12 files changed, 109 insertions(+), 113 deletions(-) create mode 100644 prdoc/pr_6741.prdoc diff --git a/prdoc/pr_6741.prdoc b/prdoc/pr_6741.prdoc new file mode 100644 index 000000000000..d4b795038bcd --- /dev/null +++ b/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 diff --git a/substrate/frame/revive/fixtures/contracts/caller_contract.rs b/substrate/frame/revive/fixtures/contracts/caller_contract.rs index f9a30b87df47..edad43fae251 100644 --- a/substrate/frame/revive/fixtures/contracts/caller_contract.rs +++ b/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]; diff --git a/substrate/frame/revive/fixtures/contracts/create_storage_and_call.rs b/substrate/frame/revive/fixtures/contracts/create_storage_and_call.rs index 4fa2db0c8c1c..a12c36af856a 100644 --- a/substrate/frame/revive/fixtures/contracts/create_storage_and_call.rs +++ b/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 diff --git a/substrate/frame/revive/fixtures/contracts/create_storage_and_instantiate.rs b/substrate/frame/revive/fixtures/contracts/create_storage_and_instantiate.rs index 463706457a15..ecc0fc79e6fd 100644 --- a/substrate/frame/revive/fixtures/contracts/create_storage_and_instantiate.rs +++ b/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); diff --git a/substrate/frame/revive/fixtures/contracts/delegate_call_deposit_limit.rs b/substrate/frame/revive/fixtures/contracts/delegate_call_deposit_limit.rs index 55203d534c9b..0f157f5a18ac 100644 --- a/substrate/frame/revive/fixtures/contracts/delegate_call_deposit_limit.rs +++ b/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; diff --git a/substrate/frame/revive/fixtures/contracts/set_code_hash.rs b/substrate/frame/revive/fixtures/contracts/set_code_hash.rs index 75995d7bb8a2..7292c6fd10ae 100644 --- a/substrate/frame/revive/fixtures/contracts/set_code_hash.rs +++ b/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 diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index b23d7e4e60ef..b6f0e3ae1a81 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/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 ), - >::TransferFailed + >::TransferFailed, ); exec_success() }); diff --git a/substrate/frame/revive/src/tests.rs b/substrate/frame/revive/src/tests.rs index 1df300f031a7..a612e7760acb 100644 --- a/substrate/frame/revive/src/tests.rs +++ b/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::::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 ::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, >::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(), - >::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 _ = ::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(), - >::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,14 +3556,12 @@ 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(), - >::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!(::Currency::free_balance(&BOB), 1_000_000); @@ -3578,14 +3569,12 @@ fn deposit_limit_in_nested_instantiate() { // 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(), - >::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!(::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::::ContractTrapped); + assert_err!(call(&addr_caller, &noop_input).result, Error::::CodeNotFound); // Add the dependency back. Contracts::upload_code( diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index 7ea518081e23..8fb7e5c27470 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -745,29 +745,24 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { Ok(()) } - /// Fallible conversion of `DispatchError` to `ReturnErrorCode`. - fn err_into_return_code(from: DispatchError) -> Result { - use ReturnErrorCode::*; - - let transfer_failed = Error::::TransferFailed.into(); - let no_code = Error::::CodeNotFound.into(); - let not_found = Error::::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 { use crate::exec::ErrorOrigin::Callee; + use ReturnErrorCode::*; + let transfer_failed = Error::::TransferFailed.into(); + let out_of_gas = Error::::OutOfGas.into(); + let out_of_deposit = Error::::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 { + 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 diff --git a/substrate/frame/revive/uapi/src/host.rs b/substrate/frame/revive/uapi/src/host.rs index d3fd4ac8d03e..400a12879363 100644 --- a/substrate/frame/revive/uapi/src/host.rs +++ b/substrate/frame/revive/uapi/src/host.rs @@ -138,7 +138,7 @@ pub trait HostFn: private::Sealed { /// - [CalleeReverted][`crate::ReturnErrorCode::CalleeReverted]: Output buffer is returned. /// - [CalleeTrapped][`crate::ReturnErrorCode::CalleeTrapped] /// - [TransferFailed][`crate::ReturnErrorCode::TransferFailed] - /// - [NotCallable][`crate::ReturnErrorCode::NotCallable] + /// - [OutOfResources][`crate::ReturnErrorCode::OutOfResources] fn call( flags: CallFlags, callee: &[u8; 20], @@ -341,7 +341,7 @@ pub trait HostFn: private::Sealed { /// /// - [CalleeReverted][`crate::ReturnErrorCode::CalleeReverted]: Output buffer is returned. /// - [CalleeTrapped][`crate::ReturnErrorCode::CalleeTrapped] - /// - [CodeNotFound][`crate::ReturnErrorCode::CodeNotFound] + /// - [OutOfResources][`crate::ReturnErrorCode::OutOfResources] fn delegate_call( flags: CallFlags, address: &[u8; 20], @@ -468,7 +468,7 @@ pub trait HostFn: private::Sealed { /// - [CalleeReverted][`crate::ReturnErrorCode::CalleeReverted]: Output buffer is returned. /// - [CalleeTrapped][`crate::ReturnErrorCode::CalleeTrapped] /// - [TransferFailed][`crate::ReturnErrorCode::TransferFailed] - /// - [CodeNotFound][`crate::ReturnErrorCode::CodeNotFound] + /// - [OutOfResources][`crate::ReturnErrorCode::OutOfResources] fn instantiate( code_hash: &[u8; 32], ref_time_limit: u64, @@ -566,10 +566,10 @@ pub trait HostFn: private::Sealed { /// - `code_hash`: The hash of the new code. Should be decodable as an `T::Hash`. Traps /// otherwise. /// - /// # Errors + /// # Panics /// - /// - [CodeNotFound][`crate::ReturnErrorCode::CodeNotFound] - fn set_code_hash(code_hash: &[u8; 32]) -> Result; + /// Panics if there is no code on-chain with the specified hash. + fn set_code_hash(code_hash: &[u8; 32]); /// Set the value at the given key in the contract storage. /// diff --git a/substrate/frame/revive/uapi/src/host/riscv64.rs b/substrate/frame/revive/uapi/src/host/riscv64.rs index 3cba14db6a04..b76320718a69 100644 --- a/substrate/frame/revive/uapi/src/host/riscv64.rs +++ b/substrate/frame/revive/uapi/src/host/riscv64.rs @@ -115,7 +115,7 @@ mod sys { message_len: u32, message_ptr: *const u8, ) -> ReturnCode; - pub fn set_code_hash(code_hash_ptr: *const u8) -> ReturnCode; + pub fn set_code_hash(code_hash_ptr: *const u8); pub fn ecdsa_to_eth_address(key_ptr: *const u8, out_ptr: *mut u8) -> ReturnCode; pub fn instantiation_nonce() -> u64; pub fn lock_delegate_dependency(code_hash_ptr: *const u8); @@ -531,9 +531,8 @@ impl HostFn for HostFnImpl { ret_val.into_bool() } - fn set_code_hash(code_hash: &[u8; 32]) -> Result { - let ret_val = unsafe { sys::set_code_hash(code_hash.as_ptr()) }; - ret_val.into() + fn set_code_hash(code_hash: &[u8; 32]) { + unsafe { sys::set_code_hash(code_hash.as_ptr()) } } fn code_hash(address: &[u8; 20], output: &mut [u8; 32]) { diff --git a/substrate/frame/revive/uapi/src/lib.rs b/substrate/frame/revive/uapi/src/lib.rs index 91c2543bb719..14a5e3f28889 100644 --- a/substrate/frame/revive/uapi/src/lib.rs +++ b/substrate/frame/revive/uapi/src/lib.rs @@ -85,23 +85,21 @@ define_error_codes! { /// Transfer failed for other not further specified reason. Most probably /// reserved or locked balance of the sender that was preventing the transfer. TransferFailed = 4, - /// No code could be found at the supplied code hash. - CodeNotFound = 5, - /// The account that was called is no contract. - NotCallable = 6, /// The call to `debug_message` had no effect because debug message /// recording was disabled. - LoggingDisabled = 7, + LoggingDisabled = 5, /// The call dispatched by `call_runtime` was executed but returned an error. - CallRuntimeFailed = 8, + CallRuntimeFailed = 6, /// ECDSA public key recovery failed. Most probably wrong recovery id or signature. - EcdsaRecoveryFailed = 9, + EcdsaRecoveryFailed = 7, /// sr25519 signature verification failed. - Sr25519VerifyFailed = 10, + Sr25519VerifyFailed = 8, /// The `xcm_execute` call failed. - XcmExecutionFailed = 11, + XcmExecutionFailed = 9, /// The `xcm_send` call failed. - XcmSendFailed = 12, + XcmSendFailed = 10, + /// The subcall ran out of weight or storage deposit. + OutOfResources = 11, } /// The raw return code returned by the host side.