From 28463a12f08e81fb7ffd455d59d66c2b274b07e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Thu, 8 Feb 2024 15:05:00 +0100 Subject: [PATCH] contracts: Don't fail fast if the `Weight` limit of a cross contract call is too big (#3243) When doing a cross contract call you can supply an optional Weight limit for that call. If one doesn't specify the limit (setting it to 0) the sub call will have all the remaining gas available. If one does specify the limit we subtract that amount eagerly from the Weight meter and fail fast if not enough `Weight` is available. This is quite annoying because setting a fixed limit will set the `gas_required` in the gas estimation according to the specified limit. Even if in that dry-run the actual call didn't consume that whole amount. It effectively discards the more precise measurement it should have from the dry-run. This PR changes the behaviour so that the supplied limit is an actual limit: We do the cross contract call even if the limit is higher than the remaining `Weight`. We then fail and roll back in the cub call in case there is not enough weight. This makes the weight estimation in the dry-run no longer dependent on the weight limit supplied when doing a cross contract call. --------- Co-authored-by: PG Herveou --- prdoc/pr_3243.prdoc | 13 ++ .../fixtures/contracts/call_with_limit.rs | 4 +- substrate/frame/contracts/src/exec.rs | 2 +- substrate/frame/contracts/src/gas.rs | 34 +-- substrate/frame/contracts/src/tests.rs | 203 +++++++++++------- 5 files changed, 153 insertions(+), 103 deletions(-) create mode 100644 prdoc/pr_3243.prdoc diff --git a/prdoc/pr_3243.prdoc b/prdoc/pr_3243.prdoc new file mode 100644 index 000000000000..5bad985a2672 --- /dev/null +++ b/prdoc/pr_3243.prdoc @@ -0,0 +1,13 @@ +title: Don't fail fast if the weight limit of a cross contract call is too big + +doc: + - audience: Runtime Dev + description: | + Cross contracts calls will now be executed even if the supplied weight + limit is bigger than the reamining weight. If the **actual** weight is too low + they will fail in the cross contract call and roll back. This is different from the + old behaviour where the limit for the cross contract call must be smaller than + the remaining weight. + +crates: + - name: pallet-contracts diff --git a/substrate/frame/contracts/fixtures/contracts/call_with_limit.rs b/substrate/frame/contracts/fixtures/contracts/call_with_limit.rs index 5e98aa614e95..40cde234b44a 100644 --- a/substrate/frame/contracts/fixtures/contracts/call_with_limit.rs +++ b/substrate/frame/contracts/fixtures/contracts/call_with_limit.rs @@ -31,9 +31,11 @@ pub extern "C" fn deploy() {} #[polkavm_derive::polkavm_export] pub extern "C" fn call() { input!( + 256, callee_addr: [u8; 32], ref_time: u64, proof_size: u64, + forwarded_input: [u8], ); #[allow(deprecated)] @@ -44,7 +46,7 @@ pub extern "C" fn call() { proof_size, None, // No deposit limit. &0u64.to_le_bytes(), // value transferred to the contract. - &[0u8; 0], // input data. + forwarded_input, None, ) .unwrap(); diff --git a/substrate/frame/contracts/src/exec.rs b/substrate/frame/contracts/src/exec.rs index 2183d6b96cc5..6f7131f3ea41 100644 --- a/substrate/frame/contracts/src/exec.rs +++ b/substrate/frame/contracts/src/exec.rs @@ -834,7 +834,7 @@ where contract_info: CachedContract::Cached(contract_info), account_id, entry_point, - nested_gas: gas_meter.nested(gas_limit)?, + nested_gas: gas_meter.nested(gas_limit), nested_storage: storage_meter.nested(deposit_limit), allows_reentry: true, }; diff --git a/substrate/frame/contracts/src/gas.rs b/substrate/frame/contracts/src/gas.rs index f78fb03a401d..11292dc03f03 100644 --- a/substrate/frame/contracts/src/gas.rs +++ b/substrate/frame/contracts/src/gas.rs @@ -104,9 +104,7 @@ impl GasMeter { /// # Note /// /// Passing `0` as amount is interpreted as "all remaining gas". - pub fn nested(&mut self, amount: Weight) -> Result { - // NOTE that it is ok to allocate all available gas since it still ensured - // by `charge` that it doesn't reach zero. + pub fn nested(&mut self, amount: Weight) -> Self { let amount = Weight::from_parts( if amount.ref_time().is_zero() { self.gas_left().ref_time() @@ -118,33 +116,17 @@ impl GasMeter { } else { amount.proof_size() }, - ); - self.gas_left = self.gas_left.checked_sub(&amount).ok_or_else(|| >::OutOfGas)?; - Ok(GasMeter::new(amount)) + ) + .min(self.gas_left); + self.gas_left -= amount; + GasMeter::new(amount) } /// Absorb the remaining gas of a nested meter after we are done using it. pub fn absorb_nested(&mut self, nested: Self) { - if self.gas_left.ref_time().is_zero() { - // All of the remaining gas was inherited by the nested gas meter. When absorbing - // we can therefore safely inherit the lowest gas that the nested gas meter experienced - // as long as it is lower than the lowest gas that was experienced by the parent. - // We cannot call `self.gas_left_lowest()` here because in the state that this - // code is run the parent gas meter has `0` gas left. - *self.gas_left_lowest.ref_time_mut() = - nested.gas_left_lowest().ref_time().min(self.gas_left_lowest.ref_time()); - } else { - // The nested gas meter was created with a fixed amount that did not consume all of the - // parents (self) gas. The lowest gas that self will experience is when the nested - // gas was pre charged with the fixed amount. - *self.gas_left_lowest.ref_time_mut() = self.gas_left_lowest().ref_time(); - } - if self.gas_left.proof_size().is_zero() { - *self.gas_left_lowest.proof_size_mut() = - nested.gas_left_lowest().proof_size().min(self.gas_left_lowest.proof_size()); - } else { - *self.gas_left_lowest.proof_size_mut() = self.gas_left_lowest().proof_size(); - } + self.gas_left_lowest = (self.gas_left + nested.gas_limit) + .saturating_sub(nested.gas_required()) + .min(self.gas_left_lowest); self.gas_left += nested.gas_left; } diff --git a/substrate/frame/contracts/src/tests.rs b/substrate/frame/contracts/src/tests.rs index 5711d3ccc83a..eba959e8d465 100644 --- a/substrate/frame/contracts/src/tests.rs +++ b/substrate/frame/contracts/src/tests.rs @@ -40,7 +40,7 @@ use crate::{ MigrationInProgress, Origin, Pallet, PristineCode, Schedule, }; use assert_matches::assert_matches; -use codec::Encode; +use codec::{Decode, Encode}; use frame_support::{ assert_err, assert_err_ignore_postinfo, assert_err_with_weight, assert_noop, assert_ok, derive_impl, @@ -217,8 +217,6 @@ impl ChainExtension for TestExtension { where E: Ext, { - use codec::Decode; - let func_id = env.func_id(); let id = env.ext_id() as u32 | func_id as u32; match func_id { @@ -2924,12 +2922,13 @@ fn debug_message_invalid_utf8() { } #[test] -fn gas_estimation_nested_call_fixed_limit() { +fn gas_estimation_for_subcalls() { let (caller_code, _caller_hash) = compile_module::("call_with_limit").unwrap(); - let (callee_code, _callee_hash) = compile_module::("dummy").unwrap(); + let (call_runtime_code, _caller_hash) = compile_module::("call_runtime").unwrap(); + let (dummy_code, _callee_hash) = compile_module::("dummy").unwrap(); ExtBuilder::default().existential_deposit(50).build().execute_with(|| { let min_balance = Contracts::min_balance(); - let _ = ::Currency::set_balance(&ALICE, 1000 * min_balance); + let _ = ::Currency::set_balance(&ALICE, 2_000 * min_balance); let addr_caller = Contracts::bare_instantiate( ALICE, @@ -2938,7 +2937,7 @@ fn gas_estimation_nested_call_fixed_limit() { None, Code::Upload(caller_code), vec![], - vec![0], + vec![], DebugInfo::Skip, CollectEvents::Skip, ) @@ -2946,14 +2945,14 @@ fn gas_estimation_nested_call_fixed_limit() { .unwrap() .account_id; - let addr_callee = Contracts::bare_instantiate( + let addr_dummy = Contracts::bare_instantiate( ALICE, min_balance * 100, GAS_LIMIT, None, - Code::Upload(callee_code), + Code::Upload(dummy_code), + vec![], vec![], - vec![1], DebugInfo::Skip, CollectEvents::Skip, ) @@ -2961,68 +2960,136 @@ fn gas_estimation_nested_call_fixed_limit() { .unwrap() .account_id; - let input: Vec = AsRef::<[u8]>::as_ref(&addr_callee) - .iter() - .cloned() - .chain((GAS_LIMIT / 5).ref_time().to_le_bytes()) - .chain((GAS_LIMIT / 5).proof_size().to_le_bytes()) - .collect(); - - // Call in order to determine the gas that is required for this call - let result = Contracts::bare_call( + let addr_call_runtime = Contracts::bare_instantiate( ALICE, - addr_caller.clone(), - 0, + min_balance * 100, GAS_LIMIT, None, - input.clone(), + Code::Upload(call_runtime_code), + vec![], + vec![], DebugInfo::Skip, CollectEvents::Skip, - Determinism::Enforced, - ); - assert_ok!(&result.result); + ) + .result + .unwrap() + .account_id; - // We have a subcall with a fixed gas limit. This constitutes precharging. - assert!(result.gas_required.all_gt(result.gas_consumed)); + // Run the test for all of those weight limits for the subcall + let weights = [ + Weight::zero(), + GAS_LIMIT, + GAS_LIMIT * 2, + GAS_LIMIT / 5, + Weight::from_parts(0, GAS_LIMIT.proof_size()), + Weight::from_parts(GAS_LIMIT.ref_time(), 0), + ]; - // Make the same call using the estimated gas. Should succeed. - assert_ok!( - Contracts::bare_call( - ALICE, - addr_caller.clone(), - 0, - result.gas_required, - Some(result.storage_deposit.charge_or_zero()), - input.clone(), - DebugInfo::Skip, - CollectEvents::Skip, - Determinism::Enforced, - ) - .result - ); + // This call is passed to the sub call in order to create a large `required_weight` + let runtime_call = RuntimeCall::Dummy(pallet_dummy::Call::overestimate_pre_charge { + pre_charge: Weight::from_parts(10_000_000_000, 512 * 1024), + actual_weight: Weight::from_parts(1, 1), + }) + .encode(); - // Make the same call using proof_size but less than estimated. Should fail with OutOfGas. - let result = Contracts::bare_call( - ALICE, - addr_caller, - 0, - result.gas_required.sub_proof_size(1), - Some(result.storage_deposit.charge_or_zero()), - input, - DebugInfo::Skip, - CollectEvents::Skip, - Determinism::Enforced, - ) - .result; - assert_err!(result, >::OutOfGas); + // Encodes which contract should be sub called with which input + let sub_calls: [(&[u8], Vec<_>, bool); 2] = [ + (addr_dummy.as_ref(), vec![], false), + (addr_call_runtime.as_ref(), runtime_call, true), + ]; + + for weight in weights { + for (sub_addr, sub_input, out_of_gas_in_subcall) in &sub_calls { + let input: Vec = sub_addr + .iter() + .cloned() + .chain(weight.ref_time().to_le_bytes()) + .chain(weight.proof_size().to_le_bytes()) + .chain(sub_input.clone()) + .collect(); + + // Call in order to determine the gas that is required for this call + let result = Contracts::bare_call( + ALICE, + addr_caller.clone(), + 0, + GAS_LIMIT, + None, + input.clone(), + DebugInfo::Skip, + CollectEvents::Skip, + Determinism::Enforced, + ); + assert_ok!(&result.result); + + // If the out of gas happens in the subcall the caller contract + // will just trap. Otherwise we would need to forward an error + // code to signal that the sub contract ran out of gas. + let error: DispatchError = if *out_of_gas_in_subcall { + assert!(result.gas_required.all_gt(result.gas_consumed)); + >::ContractTrapped.into() + } else { + assert_eq!(result.gas_required, result.gas_consumed); + >::OutOfGas.into() + }; + + // Make the same call using the estimated gas. Should succeed. + assert_ok!( + Contracts::bare_call( + ALICE, + addr_caller.clone(), + 0, + result.gas_required, + Some(result.storage_deposit.charge_or_zero()), + input.clone(), + DebugInfo::Skip, + CollectEvents::Skip, + Determinism::Enforced, + ) + .result + ); + + // Check that it fails with too little ref_time + assert_err!( + Contracts::bare_call( + ALICE, + addr_caller.clone(), + 0, + result.gas_required.sub_ref_time(1), + Some(result.storage_deposit.charge_or_zero()), + input.clone(), + DebugInfo::Skip, + CollectEvents::Skip, + Determinism::Enforced, + ) + .result, + error, + ); + + // Check that it fails with too little proof_size + assert_err!( + Contracts::bare_call( + ALICE, + addr_caller.clone(), + 0, + result.gas_required.sub_proof_size(1), + Some(result.storage_deposit.charge_or_zero()), + input, + DebugInfo::Skip, + CollectEvents::Skip, + Determinism::Enforced, + ) + .result, + error, + ); + } + } }); } #[test] fn gas_estimation_call_runtime() { - use codec::Decode; let (caller_code, _caller_hash) = compile_module::("call_runtime").unwrap(); - let (callee_code, _callee_hash) = compile_module::("dummy").unwrap(); ExtBuilder::default().existential_deposit(50).build().execute_with(|| { let min_balance = Contracts::min_balance(); let _ = ::Currency::set_balance(&ALICE, 1000 * min_balance); @@ -3043,25 +3110,11 @@ fn gas_estimation_call_runtime() { .unwrap() .account_id; - Contracts::bare_instantiate( - ALICE, - min_balance * 100, - GAS_LIMIT, - None, - Code::Upload(callee_code), - vec![], - vec![1], - DebugInfo::Skip, - CollectEvents::Skip, - ) - .result - .unwrap(); - // Call something trivial with a huge gas limit so that we can observe the effects // of pre-charging. This should create a difference between consumed and required. let call = RuntimeCall::Dummy(pallet_dummy::Call::overestimate_pre_charge { - pre_charge: Weight::from_parts(10_000_000, 0), - actual_weight: Weight::from_parts(100, 0), + pre_charge: Weight::from_parts(10_000_000, 1_000), + actual_weight: Weight::from_parts(100, 100), }); let result = Contracts::bare_call( ALICE, @@ -3077,7 +3130,7 @@ fn gas_estimation_call_runtime() { // contract encodes the result of the dispatch runtime let outcome = u32::decode(&mut result.result.unwrap().data.as_ref()).unwrap(); assert_eq!(outcome, 0); - assert!(result.gas_required.ref_time() > result.gas_consumed.ref_time()); + assert!(result.gas_required.all_gt(result.gas_consumed)); // Make the same call using the required gas. Should succeed. assert_ok!(