From 74cba198feb971722c95e9f1c807e688c3c939d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Mon, 5 Feb 2024 16:30:18 +0100 Subject: [PATCH 1/4] Don't fail if the max weight for a sub call is larger than the remaining weigh --- .../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 +++++++++++------- 4 files changed, 140 insertions(+), 103 deletions(-) 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 363ddfad975b..8bddc5aeda2c 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..ad1b44b6f8be 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 litte 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.clone(), + ); + + // Check that it fails with too litte 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!( From da9765e0721c5e8ae67f26aa84312c3f07bce93b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Thu, 8 Feb 2024 13:56:00 +0100 Subject: [PATCH 2/4] Apply suggestions from code review Co-authored-by: PG Herveou --- substrate/frame/contracts/src/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/contracts/src/tests.rs b/substrate/frame/contracts/src/tests.rs index ad1b44b6f8be..25eff193bec9 100644 --- a/substrate/frame/contracts/src/tests.rs +++ b/substrate/frame/contracts/src/tests.rs @@ -3063,10 +3063,10 @@ fn gas_estimation_for_subcalls() { Determinism::Enforced, ) .result, - error.clone(), + error, ); - // Check that it fails with too litte proof_size + // Check that it fails with too little proof_size assert_err!( Contracts::bare_call( ALICE, From d1edb5b53edd4a4c6fdad2fba9c870419ab81fc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Thu, 8 Feb 2024 13:56:28 +0100 Subject: [PATCH 3/4] Update substrate/frame/contracts/src/tests.rs Co-authored-by: PG Herveou --- substrate/frame/contracts/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/contracts/src/tests.rs b/substrate/frame/contracts/src/tests.rs index 25eff193bec9..eba959e8d465 100644 --- a/substrate/frame/contracts/src/tests.rs +++ b/substrate/frame/contracts/src/tests.rs @@ -3049,7 +3049,7 @@ fn gas_estimation_for_subcalls() { .result ); - // Check that it fails with too litte ref_time + // Check that it fails with too little ref_time assert_err!( Contracts::bare_call( ALICE, From 468bcb2a759268178722e00a650801d5e3ee64bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Thu, 8 Feb 2024 14:24:05 +0100 Subject: [PATCH 4/4] Add prdoc --- prdoc/pr_3243.prdoc | 13 +++++++++++++ 1 file changed, 13 insertions(+) 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