contracts: Don't fail fast if the Weight limit of a cross contract call is too big

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

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();

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,
 		};

substrate/frame/contracts/src/gas.rs

@@ -104,9 +104,7 @@ impl<T: Config> GasMeter<T> {
 	/// # Note
 	///
 	/// Passing `0` as amount is interpreted as "all remaining gas".
-	pub fn nested(&mut self, amount: Weight) -> Result<Self, DispatchError> {
-		// 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<T: Config> GasMeter<T> {
 			} else {
 				amount.proof_size()
 			},
-		);
-		self.gas_left = self.gas_left.checked_sub(&amount).ok_or_else(|| <Error<T>>::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;
 	}
 

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<Test> for TestExtension {
 	where
 		E: Ext<T = Test>,
 	{
-		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::<Test>("call_with_limit").unwrap();
-	let (callee_code, _callee_hash) = compile_module::<Test>("dummy").unwrap();
+	let (call_runtime_code, _caller_hash) = compile_module::<Test>("call_runtime").unwrap();
+	let (dummy_code, _callee_hash) = compile_module::<Test>("dummy").unwrap();
 	ExtBuilder::default().existential_deposit(50).build().execute_with(|| {
 		let min_balance = Contracts::min_balance();
-		let _ = <Test as Config>::Currency::set_balance(&ALICE, 1000 * min_balance);
+		let _ = <Test as Config>::Currency::set_balance(&ALICE, 2_000 * min_balance);
 
 		let addr_caller = Contracts::bare_instantiate(
 			ALICE,
@@ -2938,91 +2937,159 @@ fn gas_estimation_nested_call_fixed_limit() {
 			None,
 			Code::Upload(caller_code),
 			vec![],
-			vec![0],
+			vec![],
 			DebugInfo::Skip,
 			CollectEvents::Skip,
 		)
 		.result
 		.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,
 		)
 		.result
 		.unwrap()
 		.account_id;
 
-		let input: Vec<u8> = 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, <Error<Test>>::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<u8> = 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));
+					<Error<Test>>::ContractTrapped.into()
+				} else {
+					assert_eq!(result.gas_required, result.gas_consumed);
+					<Error<Test>>::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::<Test>("call_runtime").unwrap();
-	let (callee_code, _callee_hash) = compile_module::<Test>("dummy").unwrap();
 	ExtBuilder::default().existential_deposit(50).build().execute_with(|| {
 		let min_balance = Contracts::min_balance();
 		let _ = <Test as Config>::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!(