From 40335b1b89e3ea5a54e795804b6695ac973205e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Thu, 15 Feb 2024 15:05:24 +0100 Subject: [PATCH 1/7] Fix double charge of gas for host functions --- .../contracts/fixtures/contracts/recurse.rs | 51 ++++++++++ .../frame/contracts/proc-macro/src/lib.rs | 37 ++++--- substrate/frame/contracts/src/gas.rs | 86 +++++++++++------ substrate/frame/contracts/src/tests.rs | 96 ++++++++++++++++++- substrate/frame/contracts/src/wasm/mod.rs | 2 +- 5 files changed, 222 insertions(+), 50 deletions(-) create mode 100644 substrate/frame/contracts/fixtures/contracts/recurse.rs diff --git a/substrate/frame/contracts/fixtures/contracts/recurse.rs b/substrate/frame/contracts/fixtures/contracts/recurse.rs new file mode 100644 index 000000000000..b42e69f36c67 --- /dev/null +++ b/substrate/frame/contracts/fixtures/contracts/recurse.rs @@ -0,0 +1,51 @@ +// 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_v1( + uapi::CallFlags::ALLOW_REENTRY, + addr, + 0u64, // How much gas to devote for the execution. 0 = all. + &0u64.to_le_bytes(), // value transferred to the contract. + &(calls_left - 1).to_le_bytes(), + None, + ) + .unwrap(); +} diff --git a/substrate/frame/contracts/proc-macro/src/lib.rs b/substrate/frame/contracts/proc-macro/src/lib.rs index 403db15ac2cb..de961776c322 100644 --- a/substrate/frame/contracts/proc-macro/src/lib.rs +++ b/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::::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 11292dc03f03..dec32c4415f6 100644 --- a/substrate/frame/contracts/src/gas.rs +++ b/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 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 seperatly + /// 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(), @@ -165,32 +188,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/tests.rs b/substrate/frame/contracts/src/tests.rs index eba959e8d465..958a68eab5b4 100644 --- a/substrate/frame/contracts/src/tests.rs +++ b/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; @@ -5996,3 +5996,95 @@ fn balance_api_returns_free_balance() { ); }); } + +#[test] +fn gas_consumed_is_linear_for_nested_calls() { + let (code, _code_hash) = compile_module::("recurse").unwrap(); + ExtBuilder::default().existential_deposit(200).build().execute_with(|| { + let _ = ::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 gas_0 = { + let result = Contracts::bare_call( + ALICE, + addr.clone(), + 0, + GAS_LIMIT, + None, + 0u32.encode(), + DebugInfo::Skip, + CollectEvents::Skip, + Determinism::Enforced, + ); + assert_ok!(result.result); + result.gas_consumed + }; + + let gas_1 = { + let result = Contracts::bare_call( + ALICE, + addr.clone(), + 0, + GAS_LIMIT, + None, + 1u32.encode(), + DebugInfo::Skip, + CollectEvents::Skip, + Determinism::Enforced, + ); + assert_ok!(result.result); + result.gas_consumed + }; + + let gas_2 = { + let result = Contracts::bare_call( + ALICE, + addr.clone(), + 0, + GAS_LIMIT, + None, + 2u32.encode(), + DebugInfo::Skip, + CollectEvents::Skip, + Determinism::Enforced, + ); + assert_ok!(result.result); + result.gas_consumed + }; + + let max_call_depth = ::CallStack::size(); + + let gas_max = { + let result = Contracts::bare_call( + ALICE, + addr.clone(), + 0, + GAS_LIMIT, + None, + (max_call_depth as u32).encode(), + DebugInfo::Skip, + CollectEvents::Skip, + Determinism::Enforced, + ); + assert_ok!(result.result); + result.gas_consumed + }; + + 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); + }); +} diff --git a/substrate/frame/contracts/src/wasm/mod.rs b/substrate/frame/contracts/src/wasm/mod.rs index d00ec2e47521..17246cc0c257 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) }; From 25ff2022eff29377c7f58dc95565f302321c4532 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Mon, 19 Feb 2024 22:43:54 +0800 Subject: [PATCH 2/7] Update substrate/frame/contracts/src/tests.rs Co-authored-by: PG Herveou --- substrate/frame/contracts/src/tests.rs | 86 +++++++------------------- 1 file changed, 22 insertions(+), 64 deletions(-) diff --git a/substrate/frame/contracts/src/tests.rs b/substrate/frame/contracts/src/tests.rs index 958a68eab5b4..daaacf784b66 100644 --- a/substrate/frame/contracts/src/tests.rs +++ b/substrate/frame/contracts/src/tests.rs @@ -6018,70 +6018,28 @@ fn gas_consumed_is_linear_for_nested_calls() { .unwrap() .account_id; - let gas_0 = { - let result = Contracts::bare_call( - ALICE, - addr.clone(), - 0, - GAS_LIMIT, - None, - 0u32.encode(), - DebugInfo::Skip, - CollectEvents::Skip, - Determinism::Enforced, - ); - assert_ok!(result.result); - result.gas_consumed - }; - - let gas_1 = { - let result = Contracts::bare_call( - ALICE, - addr.clone(), - 0, - GAS_LIMIT, - None, - 1u32.encode(), - DebugInfo::Skip, - CollectEvents::Skip, - Determinism::Enforced, - ); - assert_ok!(result.result); - result.gas_consumed - }; - - let gas_2 = { - let result = Contracts::bare_call( - ALICE, - addr.clone(), - 0, - GAS_LIMIT, - None, - 2u32.encode(), - DebugInfo::Skip, - CollectEvents::Skip, - Determinism::Enforced, - ); - assert_ok!(result.result); - result.gas_consumed - }; - - let max_call_depth = ::CallStack::size(); - - let gas_max = { - let result = Contracts::bare_call( - ALICE, - addr.clone(), - 0, - GAS_LIMIT, - None, - (max_call_depth as u32).encode(), - DebugInfo::Skip, - CollectEvents::Skip, - Determinism::Enforced, - ); - assert_ok!(result.result); - result.gas_consumed + let max_call_depth = ::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::>() + .try_into() + .unwrap() }; let gas_per_recursion = gas_2.checked_sub(&gas_1).unwrap(); From d9d8db34e27f0bbfbd498edb2ef46f28749d4b47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Mon, 19 Feb 2024 22:44:01 +0800 Subject: [PATCH 3/7] Update substrate/frame/contracts/src/gas.rs Co-authored-by: PG Herveou --- substrate/frame/contracts/src/gas.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/contracts/src/gas.rs b/substrate/frame/contracts/src/gas.rs index dec32c4415f6..c2a98f5a8c79 100644 --- a/substrate/frame/contracts/src/gas.rs +++ b/substrate/frame/contracts/src/gas.rs @@ -100,7 +100,7 @@ pub struct GasMeter { /// 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 seperatly + /// 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, From c0fcafe9a3c65399fcd7f8414f811395b93984d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Wed, 21 Feb 2024 17:07:49 +0800 Subject: [PATCH 4/7] Add prdoc --- prdoc/pr_3361.prdoc | 10 ++++++++++ 1 file changed, 10 insertions(+) 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..cc1522d4a9ad --- /dev/null +++ b/prdoc/pr_3361.prdoc @@ -0,0 +1,10 @@ +title: contracts: 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 From 854a2a5f2b869ae869ff0d701faec3c36454cfa3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Wed, 21 Feb 2024 17:08:25 +0800 Subject: [PATCH 5/7] fmt --- substrate/frame/contracts/src/gas.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/substrate/frame/contracts/src/gas.rs b/substrate/frame/contracts/src/gas.rs index 656281851272..b9d91f38f16f 100644 --- a/substrate/frame/contracts/src/gas.rs +++ b/substrate/frame/contracts/src/gas.rs @@ -105,9 +105,9 @@ pub struct GasMeter { /// 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. + /// 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)] From a050d0821d0b9359182a0348ae704ad004e5f855 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Wed, 21 Feb 2024 17:14:01 +0800 Subject: [PATCH 6/7] Fix prdoc --- prdoc/pr_3361.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_3361.prdoc b/prdoc/pr_3361.prdoc index cc1522d4a9ad..65baa9e94a0e 100644 --- a/prdoc/pr_3361.prdoc +++ b/prdoc/pr_3361.prdoc @@ -1,4 +1,4 @@ -title: contracts: Fix double charge of host function weight +title: Fix double charge of host function weight doc: - audience: Runtime Dev From 16e53cf55150cec345154c4acb065cc6c8a8b234 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Wed, 21 Feb 2024 17:52:13 +0800 Subject: [PATCH 7/7] Use callv2 --- substrate/frame/contracts/fixtures/contracts/recurse.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/substrate/frame/contracts/fixtures/contracts/recurse.rs b/substrate/frame/contracts/fixtures/contracts/recurse.rs index b42e69f36c67..b1ded608c2fc 100644 --- a/substrate/frame/contracts/fixtures/contracts/recurse.rs +++ b/substrate/frame/contracts/fixtures/contracts/recurse.rs @@ -39,11 +39,13 @@ pub extern "C" fn call() { return } - api::call_v1( + api::call_v2( uapi::CallFlags::ALLOW_REENTRY, addr, - 0u64, // How much gas to devote for the execution. 0 = all. - &0u64.to_le_bytes(), // value transferred to the contract. + 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, )