contracts: Fix double charge of gas for host functions

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