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 <pgherveou@gmail.com>

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,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<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);
-
-		// We have a subcall with a fixed gas limit. This constitutes precharging.
-		assert!(result.gas_required.all_gt(result.gas_consumed));
-
-		// 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
-		);
-
-		// 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);
+		.result
+		.unwrap()
+		.account_id;
+
+		// 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),
+		];
+
+		// 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();
+
+		// 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!(