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

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

substrate/frame/contracts/fixtures/contracts/recurse.rs

@@ -0,0 +1,53 @@
+// This file is part of Substrate.
+
+// Copyright (C) Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: Apache-2.0
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//! This fixture calls itself as many times as passed as argument.
+
+#![no_std]
+#![no_main]
+
+use common::{input, output};
+use uapi::{HostFn, HostFnImpl as api};
+
+#[no_mangle]
+#[polkavm_derive::polkavm_export]
+pub extern "C" fn deploy() {}
+
+#[no_mangle]
+#[polkavm_derive::polkavm_export]
+pub extern "C" fn call() {
+	input!(calls_left: u32, );
+
+	// own address
+	output!(addr, [0u8; 32], api::address,);
+
+	if calls_left == 0 {
+		return
+	}
+
+	api::call_v2(
+		uapi::CallFlags::ALLOW_REENTRY,
+		addr,
+		0u64,                // How much ref_time to devote for the execution. 0 = all.
+		0u64,                // How much deposit_limit to devote for the execution. 0 = all.
+		None,                // No deposit limit.
+		&0u64.to_le_bytes(), // Value transferred to the contract.
+		&(calls_left - 1).to_le_bytes(),
+		None,
+	)
+	.unwrap();
+}

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

@@ -638,37 +638,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

@@ -23,7 +23,7 @@ use frame_support::{
 	DefaultNoBound,
 };
 use sp_core::Get;
-use sp_runtime::{traits::Zero, DispatchError};
+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))]
@@ -84,8 +102,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>,
@@ -97,7 +120,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(),
@@ -172,32 +195,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/tests.rs

@@ -35,8 +35,8 @@ use crate::{
 	tests::test_utils::{get_contract, get_contract_checked},
 	wasm::{Determinism, ReturnErrorCode as RuntimeReturnCode},
 	weights::WeightInfo,
-	BalanceOf, Code, CodeHash, CodeInfoOf, CollectEvents, Config, ContractInfo, ContractInfoOf,
-	DebugInfo, DefaultAddressGenerator, DeletionQueueCounter, Error, HoldReason,
+	Array, BalanceOf, Code, CodeHash, CodeInfoOf, CollectEvents, Config, ContractInfo,
+	ContractInfoOf, DebugInfo, DefaultAddressGenerator, DeletionQueueCounter, Error, HoldReason,
 	MigrationInProgress, Origin, Pallet, PristineCode, Schedule,
 };
 use assert_matches::assert_matches;
@@ -5977,3 +5977,53 @@ fn balance_api_returns_free_balance() {
 		);
 	});
 }
+
+#[test]
+fn gas_consumed_is_linear_for_nested_calls() {
+	let (code, _code_hash) = compile_module::<Test>("recurse").unwrap();
+	ExtBuilder::default().existential_deposit(200).build().execute_with(|| {
+		let _ = <Test as Config>::Currency::set_balance(&ALICE, 1_000_000);
+
+		let addr = Contracts::bare_instantiate(
+			ALICE,
+			0,
+			GAS_LIMIT,
+			None,
+			Code::Upload(code),
+			vec![],
+			vec![],
+			DebugInfo::Skip,
+			CollectEvents::Skip,
+		)
+		.result
+		.unwrap()
+		.account_id;
+
+		let max_call_depth = <Test as Config>::CallStack::size() as u32;
+		let [gas_0, gas_1, gas_2, gas_max] = {
+			[0u32, 1u32, 2u32, max_call_depth]
+				.iter()
+				.map(|i| {
+					let result = Contracts::bare_call(
+						ALICE,
+						addr.clone(),
+						0,
+						GAS_LIMIT,
+						None,
+						i.encode(),
+						DebugInfo::Skip,
+						CollectEvents::Skip,
+						Determinism::Enforced,
+					);
+					assert_ok!(result.result);
+					result.gas_consumed
+				})
+				.collect::<Vec<_>>()
+				.try_into()
+				.unwrap()
+		};
+
+		let gas_per_recursion = gas_2.checked_sub(&gas_1).unwrap();
+		assert_eq!(gas_max, gas_0 + gas_per_recursion * max_call_depth as u64);
+	});
+}

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