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 9dc34d5223b2..092d55277ad4 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 4650a9bc79a3..ac0ca188b479 100644 --- a/substrate/frame/contracts/src/wasm/mod.rs +++ b/substrate/frame/contracts/src/wasm/mod.rs @@ -386,7 +386,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) };