From f7fbdf18bdeaf17a56a4ef54ab65f8594a675450 Mon Sep 17 00:00:00 2001 From: kostekIV <27210860+kostekIV@users.noreply.github.com> Date: Wed, 28 Feb 2024 13:18:12 +0100 Subject: [PATCH] contracts: Fix double charge of gas for host functions (#3361) (#4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Co-authored-by: PG Herveou --- prdoc/pr_3361.prdoc | 10 +++ .../frame/contracts/proc-macro/src/lib.rs | 37 ++++---- substrate/frame/contracts/src/gas.rs | 86 +++++++++++++------ substrate/frame/contracts/src/wasm/mod.rs | 2 +- 4 files changed, 87 insertions(+), 48 deletions(-) create mode 100644 prdoc/pr_3361.prdoc diff --git a/prdoc/pr_3361.prdoc b/prdoc/pr_3361.prdoc new file mode 100644 index 000000000000..65baa9e94a0e --- /dev/null +++ b/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 diff --git a/substrate/frame/contracts/proc-macro/src/lib.rs b/substrate/frame/contracts/proc-macro/src/lib.rs index 4ef02497b8ee..e1a570d64ee8 100644 --- a/substrate/frame/contracts/proc-macro/src/lib.rs +++ b/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::::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::::OutOfGas)) - .map_err(#into_host)?; + .consume_fuel(fuel_consumed.into()) + .map_err(|_| TrapReason::from(Error::::OutOfGas))?; } } else { quote! { } diff --git a/substrate/frame/contracts/src/gas.rs b/substrate/frame/contracts/src/gas.rs index 363ddfad975b..b85e4bb9a54b 100644 --- a/substrate/frame/contracts/src/gas.rs +++ b/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 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 { 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, #[cfg(test)] tokens: Vec, @@ -92,7 +115,7 @@ impl GasMeter { 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 GasMeter { 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 { + 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::::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 { - // 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::::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 { + 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::::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. diff --git a/substrate/frame/contracts/src/wasm/mod.rs b/substrate/frame/contracts/src/wasm/mod.rs index 448c0dd74960..388ddb89edbd 100644 --- a/substrate/frame/contracts/src/wasm/mod.rs +++ b/substrate/frame/contracts/src/wasm/mod.rs @@ -384,7 +384,7 @@ impl Executable for WasmBlob { 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) };