Reduce gas/storage limits in nested calls

cumulus/parachains/integration-tests/emulated/tests/people/people-westend/src/tests/governance.rs

@@ -396,7 +396,7 @@ fn relay_commands_add_remove_username_authority() {
 		);
 	});
 
-	// Now, remove the username authority with another priviledged XCM call.
+	// Now, remove the username authority with another privileged XCM call.
 	Westend::execute_with(|| {
 		type Runtime = <Westend as Chain>::Runtime;
 		type RuntimeCall = <Westend as Chain>::RuntimeCall;

prdoc/pr_6890.prdoc

@@ -0,0 +1,13 @@
+# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
+# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
+
+title: Bound resources for nested EVM contract calls
+
+doc:
+  - audience: [ Runtime Dev, Runtime User ]
+    description: |
+      Implementation of EIP-150 for `pallet-revive`-based EVM contracts.
+
+crates:
+  - name: pallet-revive
+    bump: minor

substrate/frame/contracts/src/storage/meter.rs

@@ -878,7 +878,7 @@ mod tests {
 			let mut meter = TestMeter::new(&test_case.origin, Some(1_000), 0).unwrap();
 			assert_eq!(meter.available(), 1_000);
 
-			let mut nested0 = meter.nested(BalanceOf::<Test>::zero());
+			let mut nested0 = meter.nested(BalanceOf::<Test>::max_value());
 			nested0.charge(&Diff {
 				bytes_added: 5,
 				bytes_removed: 1,
@@ -893,7 +893,7 @@ mod tests {
 				bytes_deposit: 100,
 				items_deposit: 20,
 			});
-			let mut nested1 = nested0.nested(BalanceOf::<Test>::zero());
+			let mut nested1 = nested0.nested(BalanceOf::<Test>::max_value());
 			nested1.charge(&Diff { items_removed: 5, ..Default::default() });
 			nested1.charge(&Diff { bytes_added: 20, ..Default::default() });
 			nested1.terminate(&nested1_info, CHARLIE);

substrate/frame/revive/src/exec.rs

@@ -53,7 +53,7 @@ use sp_core::{
 };
 use sp_io::{crypto::secp256k1_ecdsa_recover_compressed, hashing::blake2_256};
 use sp_runtime::{
-	traits::{BadOrigin, Convert, Dispatchable, Saturating, Zero},
+	traits::{BadOrigin, Bounded, Convert, Dispatchable, Saturating, Zero},
 	DispatchError, SaturatedConversion,
 };
 
@@ -885,9 +885,9 @@ where
 			args,
 			value,
 			gas_meter,
-			Weight::zero(),
+			Weight::max_value(),
 			storage_meter,
-			BalanceOf::<T>::zero(),
+			BalanceOf::<T>::max_value(),
 			false,
 			true,
 		)?
@@ -3098,8 +3098,8 @@ mod tests {
 				let (address, output) = ctx
 					.ext
 					.instantiate(
-						Weight::zero(),
-						U256::zero(),
+						Weight::MAX,
+						U256::from(u64::MAX),
 						dummy_ch,
 						<Test as Config>::Currency::minimum_balance().into(),
 						vec![],
@@ -3802,8 +3802,8 @@ mod tests {
 		let succ_fail_code = MockLoader::insert(Constructor, move |ctx, _| {
 			ctx.ext
 				.instantiate(
-					Weight::zero(),
-					U256::zero(),
+					Weight::MAX,
+					U256::from(u64::MAX),
 					fail_code,
 					ctx.ext.minimum_balance() * 100,
 					vec![],
@@ -3819,8 +3819,8 @@ mod tests {
 			let addr = ctx
 				.ext
 				.instantiate(
-					Weight::zero(),
-					U256::zero(),
+					Weight::MAX,
+					U256::from(u64::MAX),
 					success_code,
 					ctx.ext.minimum_balance() * 100,
 					vec![],
@@ -4597,7 +4597,7 @@ mod tests {
 				// Successful instantiation should set the output
 				let address = ctx
 					.ext
-					.instantiate(Weight::zero(), U256::zero(), ok_ch, value, vec![], None)
+					.instantiate(Weight::MAX, U256::from(u64::MAX), ok_ch, value, vec![], None)
 					.unwrap();
 				assert_eq!(
 					ctx.ext.last_frame_output(),
@@ -4607,8 +4607,8 @@ mod tests {
 				// Balance transfers should reset the output
 				ctx.ext
 					.call(
-						Weight::zero(),
-						U256::zero(),
+						Weight::MAX,
+						U256::from(u64::MAX),
 						&address,
 						U256::from(1),
 						vec![],
@@ -4827,7 +4827,7 @@ mod tests {
 
 				// Constructors can not access the immutable data
 				ctx.ext
-					.instantiate(Weight::zero(), U256::zero(), dummy_ch, value, vec![], None)
+					.instantiate(Weight::MAX, U256::from(u64::MAX), dummy_ch, value, vec![], None)
 					.unwrap();
 
 				exec_success()
@@ -4944,7 +4944,7 @@ mod tests {
 			move |ctx, _| {
 				let value = <Test as Config>::Currency::minimum_balance().into();
 				ctx.ext
-					.instantiate(Weight::zero(), U256::zero(), dummy_ch, value, vec![], None)
+					.instantiate(Weight::MAX, U256::from(u64::MAX), dummy_ch, value, vec![], None)
 					.unwrap();
 
 				exec_success()
@@ -4989,7 +4989,7 @@ mod tests {
 			move |ctx, _| {
 				let value = <Test as Config>::Currency::minimum_balance().into();
 				ctx.ext
-					.instantiate(Weight::zero(), U256::zero(), dummy_ch, value, vec![], None)
+					.instantiate(Weight::MAX, U256::from(u64::MAX), dummy_ch, value, vec![], None)
 					.unwrap();
 
 				exec_success()

substrate/frame/revive/src/gas.rs

@@ -22,7 +22,7 @@ use frame_support::{
 	weights::Weight,
 	DefaultNoBound,
 };
-use sp_runtime::{traits::Zero, DispatchError};
+use sp_runtime::DispatchError;
 
 #[cfg(test)]
 use std::{any::Any, fmt::Debug};
@@ -168,27 +168,13 @@ impl<T: Config> GasMeter<T> {
 		}
 	}
 
-	/// Create a new gas meter by removing gas from the current meter.
-	///
-	/// # Note
-	///
-	/// Passing `0` as amount is interpreted as "all remaining gas".
+	/// Create a new gas meter for a nested call by removing gas from the current meter.
 	pub fn nested(&mut self, amount: Weight) -> Self {
-		let amount = Weight::from_parts(
-			if amount.ref_time().is_zero() {
-				self.gas_left().ref_time()
-			} else {
-				amount.ref_time()
-			},
-			if amount.proof_size().is_zero() {
-				self.gas_left().proof_size()
-			} else {
-				amount.proof_size()
-			},
-		)
-		.min(self.gas_left);
-		self.gas_left -= amount;
-		GasMeter::new(amount)
+		// The reduction to 63/64 is to emulate EIP-150
+		// https://github.com/ethereum/EIPs/blob/master/EIPS/eip-150.md
+		let amt = amount.min(self.gas_left - self.gas_left / 64);
+		self.gas_left -= amt;
+		GasMeter::new(amt)
 	}
 
 	/// Absorb the remaining gas of a nested meter after we are done using it.
@@ -392,6 +378,51 @@ mod tests {
 		assert!(gas_meter.charge(SimpleToken(1)).is_err());
 	}
 
+	#[test]
+	/// Previously, passing a `Weight` of 0 to `nested` would consume all of the meter's current gas.
+	///
+	/// Now, a `Weight` of 0 means no gas for the nested call.
+	fn nested_zero_gas_requested() {
+		let test_weight = 50000.into();
+		let mut gas_meter = GasMeter::<Test>::new(test_weight);
+		let gas_for_nested_call = gas_meter.nested(0.into());
+
+		assert_eq!(gas_meter.gas_left(), 50000.into());
+		assert_eq!(gas_for_nested_call.gas_left(), 0.into())
+	}
+
+	#[test]
+	fn nested_some_gas_requested() {
+		let test_weight = 50000.into();
+		let mut gas_meter = GasMeter::<Test>::new(test_weight);
+		let gas_for_nested_call = gas_meter.nested(10000.into());
+
+		assert_eq!(gas_meter.gas_left(), 40000.into());
+		assert_eq!(gas_for_nested_call.gas_left(), 10000.into())
+	}
+
+	#[test]
+	fn nested_all_gas_requested() {
+		let test_weight = Weight::from_parts(50000, 50000);
+		let mut gas_meter = GasMeter::<Test>::new(test_weight);
+		let gas_for_nested_call = gas_meter.nested(test_weight);
+
+		// With EIP-150, it is not possible for subcalls to consume all available gas.
+		// They are limited to 63/64ths; 50000 / 64 ≈ 781
+		assert_eq!(gas_meter.gas_left(), Weight::from_parts(781, 781));
+		assert_eq!(gas_for_nested_call.gas_left(), 49219.into())
+	}
+
+	#[test]
+	fn nested_excess_gas_requested() {
+		let test_weight = Weight::from_parts(50000, 50000);
+		let mut gas_meter = GasMeter::<Test>::new(test_weight);
+		let gas_for_nested_call = gas_meter.nested(test_weight + 10000.into());
+
+		assert_eq!(gas_meter.gas_left(), Weight::from_parts(781, 781));
+		assert_eq!(gas_for_nested_call.gas_left(), 49219.into())
+	}
+
 	// Make sure that the gas meter does not charge in case of overcharge
 	#[test]
 	fn overcharge_does_not_charge() {

substrate/frame/revive/src/primitives.rs

@@ -72,7 +72,7 @@ pub struct ContractResult<R, Balance, EventRecord> {
 	///
 	/// # Note
 	///
-	/// This can only different from [`Self::gas_consumed`] when weight pre charging
+	/// This can only be different from [`Self::gas_consumed`] when weight pre charging
 	/// is used. Currently, only `seal_call_runtime` makes use of pre charging.
 	/// Additionally, any `seal_call` or `seal_instantiate` makes use of pre-charging
 	/// when a non-zero `gas_limit` argument is supplied.

substrate/frame/revive/src/storage/meter.rs

@@ -286,16 +286,16 @@ where
 	/// This is called whenever a new subcall is initiated in order to track the storage
 	/// usage for this sub call separately. This is necessary because we want to exchange balance
 	/// with the current contract we are interacting with.
-	pub fn nested(&self, limit: BalanceOf<T>) -> RawMeter<T, E, Nested> {
+	pub fn nested(&self, lim: BalanceOf<T>) -> RawMeter<T, E, Nested> {
 		debug_assert!(matches!(self.contract_state(), ContractState::Alive));
+
+		// Limit gas for nested call, per EIP-150.
+		let available = self.available() - self.available() / (BalanceOf::<T>::from(64u32));
+
 		// If a special limit is specified higher than it is available,
 		// we want to enforce the lesser limit to the nested meter, to fail in the sub-call.
-		let limit = self.available().min(limit);
-		if limit.is_zero() {
-			RawMeter { limit: self.available(), ..Default::default() }
-		} else {
-			RawMeter { limit, nested: Nested::OwnLimit, ..Default::default() }
-		}
+		let limit = available.min(lim);
+		RawMeter { limit, nested: Nested::OwnLimit, ..Default::default() }
 	}
 
 	/// Absorb a child that was spawned to handle a sub call.
@@ -724,6 +724,49 @@ mod tests {
 		)
 	}
 
+	#[test]
+	/// Previously, passing a limit of 0 meant unlimited storage for a nested call.
+	///
+	/// Now, a limit of 0 means the subcall will not be able to use any storage.
+	fn nested_zero_limit_requested() {
+		clear_ext();
+
+		let meter = TestMeter::new(&Origin::from_account_id(ALICE), 1_000, 0).unwrap();
+		assert_eq!(meter.available(), 1_000);
+		let nested0 = meter.nested(BalanceOf::<Test>::zero());
+		assert_eq!(nested0.available(), 0);
+	}
+
+	#[test]
+	fn nested_some_limit_requested() {
+		clear_ext();
+
+		let meter = TestMeter::new(&Origin::from_account_id(ALICE), 1_000, 0).unwrap();
+		assert_eq!(meter.available(), 1_000);
+		let nested0 = meter.nested(500);
+		assert_eq!(nested0.available(), 500);
+	}
+
+	#[test]
+	fn nested_all_limit_requested() {
+		clear_ext();
+
+		let meter = TestMeter::new(&Origin::from_account_id(ALICE), 1_000, 0).unwrap();
+		assert_eq!(meter.available(), 1_000);
+		let nested0 = meter.nested(1_000);
+		assert_eq!(nested0.available(), 985);
+	}
+
+	#[test]
+	fn nested_over_limit_requested() {
+		clear_ext();
+
+		let meter = TestMeter::new(&Origin::from_account_id(ALICE), 1_000, 0).unwrap();
+		assert_eq!(meter.available(), 1_000);
+		let nested0 = meter.nested(2_000);
+		assert_eq!(nested0.available(), 985);
+	}
+
 	#[test]
 	fn empty_charge_works() {
 		clear_ext();
@@ -879,7 +922,7 @@ mod tests {
 			let mut meter = TestMeter::new(&test_case.origin, 1_000, 0).unwrap();
 			assert_eq!(meter.available(), 1_000);
 
-			let mut nested0 = meter.nested(BalanceOf::<Test>::zero());
+			let mut nested0 = meter.nested(BalanceOf::<Test>::max_value());
 			nested0.charge(&Diff {
 				bytes_added: 5,
 				bytes_removed: 1,
@@ -895,7 +938,7 @@ mod tests {
 				items_deposit: 20,
 				immutable_data_len: 0,
 			});
-			let mut nested1 = nested0.nested(BalanceOf::<Test>::zero());
+			let mut nested1 = nested0.nested(BalanceOf::<Test>::max_value());
 			nested1.charge(&Diff { items_removed: 5, ..Default::default() });
 			nested1.charge(&Diff { bytes_added: 20, ..Default::default() });
 			nested1.terminate(&nested1_info, CHARLIE);