contracts: Fix double charge of gas for host functions (paritytech#3361…

…) (#4)

This PR is fixing a bug in the sync mechanism between wasmi and
pallet-contracts. This bug leads to essentially double charging all the
gas that was used during the execution of the host function. When the
`call` host function is used for recursion this will lead to a quadratic
amount of gas consumption with regard to the nesting depth.We also took
the chance to refactor the code in question and improve the rust docs.

The bug was caused by not updating `GasMeter::executor_consumed`
(previously `engine_consumed`) when leaving the host function. This lead
to the value being stale (too low) when entering another host function.

---------

Co-authored-by: Alexander Theißen <alex.theissen@me.com>
Co-authored-by: PG Herveou <pgherveou@gmail.com>

prdoc/pr_3361.prdoc

@@ -0,0 +1,10 @@
+title: Fix double charge of host function weight
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      Fixed a double charge which can lead to quadratic gas consumption of
+      the `call` and `instantiate` host functions.
+
+crates:
+  - name: pallet-contracts

substrate/frame/contracts/proc-macro/src/lib.rs

@@ -675,37 +675,34 @@ fn expand_functions(def: &EnvDef, expand_blocks: bool, host_state: TokenStream2)
 		};
 		let sync_gas_before = if expand_blocks {
 			quote! {
-				// Gas left in the gas meter right before switching to engine execution.
-				let __gas_before__ = {
-					let engine_consumed_total =
+				// Write gas from wasmi into pallet-contracts before entering the host function.
+				let __gas_left_before__ = {
+					let executor_total =
 						__caller__.fuel_consumed().expect("Fuel metering is enabled; qed");
-					let gas_meter = __caller__.data_mut().ext().gas_meter_mut();
-					gas_meter
-						.charge_fuel(engine_consumed_total)
+					__caller__
+						.data_mut()
+						.ext()
+						.gas_meter_mut()
+						.sync_from_executor(executor_total)
 						.map_err(TrapReason::from)
 						.map_err(#into_host)?
-						.ref_time()
 				};
 			}
 		} else {
 			quote! { }
 		};
-		// Gas left in the gas meter right after returning from engine execution.
+		// Write gas from pallet-contracts into wasmi after leaving the host function.
 		let sync_gas_after = if expand_blocks {
 			quote! {
-				let mut gas_after = __caller__.data_mut().ext().gas_meter().gas_left().ref_time();
-				let mut host_consumed = __gas_before__.saturating_sub(gas_after);
-				// Possible undercharge of at max 1 fuel here, if host consumed less than `instruction_weights.base`
-				// Not a problem though, as soon as host accounts its spent gas properly.
-				let fuel_consumed = host_consumed
-					.checked_div(__caller__.data_mut().ext().schedule().instruction_weights.base as u64)
-					.ok_or(Error::<E::T>::InvalidSchedule)
-					.map_err(TrapReason::from)
-					.map_err(#into_host)?;
+				let fuel_consumed = __caller__
+					.data_mut()
+					.ext()
+					.gas_meter_mut()
+					.sync_to_executor(__gas_left_before__)
+					.map_err(TrapReason::from)?;
 				 __caller__
-					 .consume_fuel(fuel_consumed)
-					 .map_err(|_| TrapReason::from(Error::<E::T>::OutOfGas))
-					 .map_err(#into_host)?;
+					 .consume_fuel(fuel_consumed.into())
+					 .map_err(|_| TrapReason::from(Error::<E::T>::OutOfGas))?;
 			}
 		} else {
 			quote! { }

substrate/frame/contracts/src/gas.rs

@@ -22,8 +22,8 @@ use frame_support::{
 	DefaultNoBound,
 };
 use sp_core::Get;
-use sp_runtime::{traits::Zero, DispatchError};
 use sp_std::marker::PhantomData;
+use sp_runtime::{traits::Zero, DispatchError, Saturating};
 
 #[cfg(test)]
 use std::{any::Any, fmt::Debug};
@@ -37,6 +37,24 @@ impl ChargedAmount {
 	}
 }
 
+/// Used to capture the gas left before entering a host function.
+///
+/// Has to be consumed in order to sync back the gas after leaving the host function.
+#[must_use]
+pub struct RefTimeLeft(u64);
+
+/// Resource that needs to be synced to the executor.
+///
+/// Wrapped to make sure that the resource will be synced back the the executor.
+#[must_use]
+pub struct Syncable(u64);
+
+impl From<Syncable> for u64 {
+	fn from(from: Syncable) -> u64 {
+		from.0
+	}
+}
+
 #[cfg(not(test))]
 pub trait TestAuxiliaries {}
 #[cfg(not(test))]
@@ -79,8 +97,13 @@ pub struct GasMeter<T: Config> {
 	gas_left: Weight,
 	/// Due to `adjust_gas` and `nested` the `gas_left` can temporarily dip below its final value.
 	gas_left_lowest: Weight,
-	/// Amount of fuel consumed by the engine from the last host function call.
-	engine_consumed: u64,
+	/// The amount of resources that was consumed by the execution engine.
+	///
+	/// This should be equivalent to `self.gas_consumed().ref_time()` but expressed in whatever
+	/// unit the execution engine uses to track resource consumption. We have to track it
+	/// separately in order to avoid the loss of precision that happens when converting from
+	/// ref_time to the execution engine unit.
+	executor_consumed: u64,
 	_phantom: PhantomData<T>,
 	#[cfg(test)]
 	tokens: Vec<ErasedToken>,
@@ -92,7 +115,7 @@ impl<T: Config> GasMeter<T> {
 			gas_limit,
 			gas_left: gas_limit,
 			gas_left_lowest: gas_limit,
-			engine_consumed: Default::default(),
+			executor_consumed: 0,
 			_phantom: PhantomData,
 			#[cfg(test)]
 			tokens: Vec::new(),
@@ -183,32 +206,41 @@ impl<T: Config> GasMeter<T> {
 		self.gas_left = self.gas_left.saturating_add(adjustment).min(self.gas_limit);
 	}
 
-	/// This method is used for gas syncs with the engine.
+	/// Hand over the gas metering responsibility from the executor to this meter.
 	///
-	/// Updates internal `engine_comsumed` tracker of engine fuel consumption.
+	/// Needs to be called when entering a host function to update this meter with the
+	/// gas that was tracked by the executor. It tracks the latest seen total value
+	/// in order to compute the delta that needs to be charged.
+	pub fn sync_from_executor(
+		&mut self,
+		executor_total: u64,
+	) -> Result<RefTimeLeft, DispatchError> {
+		let chargable_reftime = executor_total
+			.saturating_sub(self.executor_consumed)
+			.saturating_mul(u64::from(T::Schedule::get().instruction_weights.base));
+		self.executor_consumed = executor_total;
+		self.gas_left
+			.checked_reduce(Weight::from_parts(chargable_reftime, 0))
+			.ok_or_else(|| Error::<T>::OutOfGas)?;
+		Ok(RefTimeLeft(self.gas_left.ref_time()))
+	}
+
+	/// Hand over the gas metering responsibility from this meter to the executor.
 	///
-	/// Charges self with the `ref_time` Weight corresponding to wasmi fuel consumed on the engine
-	/// side since last sync. Passed value is scaled by multiplying it by the weight of a basic
-	/// operation, as such an operation in wasmi engine costs 1.
+	/// Needs to be called when leaving a host function in order to calculate how much
+	/// gas needs to be charged from the **executor**. It updates the last seen executor
+	/// total value so that it is correct when `sync_from_executor` is called the next time.
 	///
-	/// Returns the updated `gas_left` `Weight` value from the meter.
-	/// Normally this would never fail, as engine should fail first when out of gas.
-	pub fn charge_fuel(&mut self, wasmi_fuel_total: u64) -> Result<Weight, DispatchError> {
-		// Take the part consumed since the last update.
-		let wasmi_fuel = wasmi_fuel_total.saturating_sub(self.engine_consumed);
-		if !wasmi_fuel.is_zero() {
-			self.engine_consumed = wasmi_fuel_total;
-			let reftime_consumed =
-				wasmi_fuel.saturating_mul(T::Schedule::get().instruction_weights.base as u64);
-			let ref_time_left = self
-				.gas_left
-				.ref_time()
-				.checked_sub(reftime_consumed)
-				.ok_or_else(|| Error::<T>::OutOfGas)?;
-
-			*(self.gas_left.ref_time_mut()) = ref_time_left;
-		}
-		Ok(self.gas_left)
+	/// It is important that this does **not** actually sync with the executor. That has
+	/// to be done by the caller.
+	pub fn sync_to_executor(&mut self, before: RefTimeLeft) -> Result<Syncable, DispatchError> {
+		let chargable_executor_resource = before
+			.0
+			.saturating_sub(self.gas_left().ref_time())
+			.checked_div(u64::from(T::Schedule::get().instruction_weights.base))
+			.ok_or(Error::<T>::InvalidSchedule)?;
+		self.executor_consumed.saturating_accrue(chargable_executor_resource);
+		Ok(Syncable(chargable_executor_resource))
 	}
 
 	/// Returns the amount of gas that is required to run the same call.

substrate/frame/contracts/src/wasm/mod.rs

@@ -386,7 +386,7 @@ impl<T: Config> Executable<T> for WasmBlob<T> {
 			let engine_consumed_total =
 				store.fuel_consumed().expect("Fuel metering is enabled; qed");
 			let gas_meter = store.data_mut().ext().gas_meter_mut();
-			gas_meter.charge_fuel(engine_consumed_total)?;
+			let _ = gas_meter.sync_from_executor(engine_consumed_total)?;
 			store.into_data().to_execution_result(result)
 		};