From cde4ef34782c33436f917efe38a1943d9782ea12 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 9 Aug 2024 19:07:21 +0200 Subject: [PATCH 1/4] Removes `VpsGas` and gas dividers --- crates/gas/src/lib.rs | 113 ++++++------------------ crates/node/src/protocol.rs | 21 +++-- crates/shielded_token/src/validation.rs | 3 - crates/tx/src/data/mod.rs | 5 +- 4 files changed, 43 insertions(+), 99 deletions(-) diff --git a/crates/gas/src/lib.rs b/crates/gas/src/lib.rs index d3d78e8419..3c9205af66 100644 --- a/crates/gas/src/lib.rs +++ b/crates/gas/src/lib.rs @@ -51,7 +51,7 @@ pub enum GasParseError { } // RAW GAS COSTS -// ================================================================================ +// ============================================================================= // The raw gas costs exctracted from the benchmarks. // const COMPILE_GAS_PER_BYTE_RAW: u64 = 1_664; @@ -119,7 +119,7 @@ const MASP_CONVERT_CHECK_GAS_RAW: u64 = 188_590; const MASP_OUTPUT_CHECK_GAS_RAW: u64 = 204_430; // The cost to run the final masp check in the bundle const MASP_FINAL_CHECK_GAS_RAW: u64 = 43; -// ================================================================================ +// ============================================================================= // A correction factor for non-WASM-opcodes costs. We can see that the // gas cost we get for wasm codes (txs and vps) is much greater than what we @@ -134,10 +134,10 @@ const MASP_FINAL_CHECK_GAS_RAW: u64 = 43; const GAS_COST_CORRECTION: u64 = 5; // ADJUSTED GAS COSTS -// ================================================================================ +// ============================================================================= // The gas costs adjusted for the correction factor. // -const PARALLEL_GAS_DIVIDER: u64 = 1; + // The compilation cost is reduced by a factor to compensate for the (most // likely) presence of the cache const COMPILE_GAS_PER_BYTE: u64 = @@ -207,10 +207,7 @@ pub const MASP_OUTPUT_CHECK_GAS: u64 = /// The cost to run the final masp check in the bundle pub const MASP_FINAL_CHECK_GAS: u64 = MASP_FINAL_CHECK_GAS_RAW * GAS_COST_CORRECTION; -/// Gas divider specific for the masp vp. Only allocates half of the cores to -/// the masp vp since we can expect the other half to be busy with other vps -pub const MASP_PARALLEL_GAS_DIVIDER: u64 = 1; -// ================================================================================ +// ============================================================================= /// Gas module result for functions that may fail pub type Result = std::result::Result; @@ -295,8 +292,10 @@ impl From for u64 { /// Gas represented in whole units. Used for fee payment and to display /// information to the user. +// FIXME: remove Default trait if we remove WholeGas from VpsResult #[derive( Debug, + Default, Clone, Copy, PartialEq, @@ -375,6 +374,21 @@ pub trait GasMetering { /// Get the gas limit fn get_gas_limit(&self) -> Gas; + + /// Check if the vps went out of gas. Starts with the gas consumed by the + /// transaction. + /// + fn check_vps_limit(&self, vps_gas: Gas) -> Result<()> { + let total = self + .get_tx_consumed_gas() + .checked_add(vps_gas) + .ok_or(Error::GasOverflow)?; + if total > self.get_gas_limit() { + return Err(Error::TransactionGasExceededError); + } + + Ok(()) + } } /// Gas metering in a transaction @@ -389,6 +403,7 @@ pub struct TxGasMeter { /// Gas metering in a validity predicate #[derive(Debug, Clone)] +// FIXME: can we simplify this type? pub struct VpGasMeter { /// Track gas overflow gas_overflow: bool, @@ -400,23 +415,6 @@ pub struct VpGasMeter { current_gas: Gas, } -/// Gas meter for VPs parallel runs -#[derive( - Clone, - Debug, - Default, - BorshSerialize, - BorshDeserialize, - BorshDeserializer, - BorshSchema, - Serialize, - Deserialize, -)] -pub struct VpsGas { - max: Gas, - rest: Vec, -} - impl GasMetering for TxGasMeter { fn consume(&mut self, gas: u64) -> Result<()> { if self.gas_overflow { @@ -484,11 +482,6 @@ impl TxGasMeter { ) } - /// Add the gas cost used in validity predicates to the current transaction. - pub fn add_vps_gas(&mut self, vps_gas: &VpsGas) -> Result<()> { - self.consume(vps_gas.get_current_gas()?.into()) - } - /// Get the amount of gas still available to the transaction pub fn get_available_gas(&self) -> Gas { self.tx_gas_limit @@ -563,64 +556,10 @@ impl VpGasMeter { current_gas: Gas::default(), } } -} - -impl VpsGas { - /// Set the gas cost from a VP run. It consumes the [`VpGasMeter`] - /// instance which shouldn't be accessed passed this point. - pub fn set(&mut self, vp_gas_meter: VpGasMeter) -> Result<()> { - if vp_gas_meter.current_gas > self.max { - self.rest.push(self.max); - self.max = vp_gas_meter.current_gas; - } else { - self.rest.push(vp_gas_meter.current_gas); - } - - self.check_limit(&vp_gas_meter) - } - - /// Merge validity predicates gas meters from parallelized runs. Consumes - /// the other `VpsGas` instance which shouldn't be used passed this point. - pub fn merge( - &mut self, - mut other: VpsGas, - tx_gas_meter: &TxGasMeter, - ) -> Result<()> { - if self.max < other.max { - self.rest.push(self.max); - self.max = other.max; - } else { - self.rest.push(other.max); - } - self.rest.append(&mut other.rest); - - self.check_limit(tx_gas_meter) - } - - /// Check if the vp went out of gas. Starts from the gas consumed by the - /// transaction. - fn check_limit(&self, gas_meter: &impl GasMetering) -> Result<()> { - let total = gas_meter - .get_tx_consumed_gas() - .checked_add(self.get_current_gas()?) - .ok_or(Error::GasOverflow)?; - if total > gas_meter.get_gas_limit() { - return Err(Error::TransactionGasExceededError); - } - Ok(()) - } - /// Get the gas consumed by the parallelized VPs - fn get_current_gas(&self) -> Result { - let parallel_gas = self - .rest - .iter() - .try_fold(Gas::default(), |acc, gas| { - acc.checked_add(*gas).ok_or(Error::GasOverflow) - })? - .checked_div(PARALLEL_GAS_DIVIDER) - .expect("Div by non-zero int cannot fail"); - self.max.checked_add(parallel_gas).ok_or(Error::GasOverflow) + /// Get the gas consumed by the VP alone + pub fn get_vp_consumed_gas(&self) -> Gas { + self.current_gas } } diff --git a/crates/node/src/protocol.rs b/crates/node/src/protocol.rs index 5e961af1c5..90ada1859a 100644 --- a/crates/node/src/protocol.rs +++ b/crates/node/src/protocol.rs @@ -11,7 +11,7 @@ use namada_sdk::events::extend::{ ComposeEvent, Height as HeightAttr, TxHash as TxHashAttr, UserAccount, }; use namada_sdk::events::EventLevel; -use namada_sdk::gas::{Gas, GasMetering, TxGasMeter, VpGasMeter}; +use namada_sdk::gas::{self, Gas, GasMetering, TxGasMeter, VpGasMeter}; use namada_sdk::hash::Hash; use namada_sdk::ibc::{IbcTxDataHash, IbcTxDataRefs}; use namada_sdk::masp::{MaspTxRefs, TxId}; @@ -1104,7 +1104,7 @@ where tracing::debug!("Total VPs gas cost {:?}", vps_result.gas_used); tx_gas_meter - .add_vps_gas(&vps_result.gas_used) + .consume(vps_result.gas_used.into()) .map_err(|err| Error::GasError(err.to_string()))?; Ok(vps_result) @@ -1324,9 +1324,13 @@ where // all the other errors we keep evaluating the vps. This // allows to display a consistent VpsResult across all // nodes and find any invalid signatures - result + result.gas_used = result .gas_used - .set(gas_meter.into_inner()) + .checked_add(gas_meter.borrow().get_vp_consumed_gas()) + .ok_or(Error::GasError(gas::Error::GasOverflow.to_string()))?; + gas_meter + .borrow() + .check_vps_limit(result.gas_used) .map_err(|err| Error::GasError(err.to_string()))?; Ok(result) @@ -1351,10 +1355,13 @@ fn merge_vp_results( let mut errors = a.errors; errors.append(&mut b.errors); let status_flags = a.status_flags | b.status_flags; - let mut gas_used = a.gas_used; - gas_used - .merge(b.gas_used, tx_gas_meter) + let gas_used = a + .gas_used + .checked_add(b.gas_used) + .ok_or(Error::GasError(gas::Error::GasOverflow.to_string()))?; + tx_gas_meter + .check_vps_limit(gas_used) .map_err(|err| Error::GasError(err.to_string()))?; Ok(VpsResult { diff --git a/crates/shielded_token/src/validation.rs b/crates/shielded_token/src/validation.rs index c52300b4d1..2fe6786b51 100644 --- a/crates/shielded_token/src/validation.rs +++ b/crates/shielded_token/src/validation.rs @@ -236,7 +236,6 @@ where consume_verify_gas(namada_gas::MASP_FIXED_SPEND_GAS)?; consume_verify_gas(checked!( namada_gas::MASP_VARIABLE_SPEND_GAS * remaining_notes as u64 - / namada_gas::MASP_PARALLEL_GAS_DIVIDER )?)?; } @@ -246,7 +245,6 @@ where consume_verify_gas(namada_gas::MASP_FIXED_CONVERT_GAS)?; consume_verify_gas(checked!( namada_gas::MASP_VARIABLE_CONVERT_GAS * remaining_notes as u64 - / namada_gas::MASP_PARALLEL_GAS_DIVIDER )?)?; } @@ -256,7 +254,6 @@ where consume_verify_gas(namada_gas::MASP_FIXED_OUTPUT_GAS)?; consume_verify_gas(checked!( namada_gas::MASP_VARIABLE_OUTPUT_GAS * remaining_notes as u64 - / namada_gas::MASP_PARALLEL_GAS_DIVIDER )?)?; } diff --git a/crates/tx/src/data/mod.rs b/crates/tx/src/data/mod.rs index 11386c0d02..4b0b0121a3 100644 --- a/crates/tx/src/data/mod.rs +++ b/crates/tx/src/data/mod.rs @@ -26,7 +26,7 @@ use namada_core::ibc::IbcTxDataRefs; use namada_core::masp::MaspTxRefs; use namada_core::storage; use namada_events::Event; -use namada_gas::{VpsGas, WholeGas}; +use namada_gas::{Gas, WholeGas}; use namada_macros::BorshDeserializer; #[cfg(feature = "migrations")] use namada_migrations::*; @@ -482,7 +482,8 @@ pub struct VpsResult { /// The addresses whose VPs rejected the transaction pub rejected_vps: BTreeSet
, /// The total gas used by all the VPs - pub gas_used: VpsGas, + // FIXME: can't we just remove it? + pub gas_used: Gas, /// Errors occurred in any of the VPs, if any pub errors: Vec<(Address, String)>, /// Validity predicate status flags, containing info From 5c88f4f6245f8876f347bbd5104c0bca9ddb7f1b Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 12 Aug 2024 15:57:09 +0200 Subject: [PATCH 2/4] Removes `gas_used` from `VpsResult` --- crates/gas/src/lib.rs | 4 - crates/node/src/protocol.rs | 206 +++++++++++++++++++----------------- crates/tx/src/data/mod.rs | 5 +- 3 files changed, 111 insertions(+), 104 deletions(-) diff --git a/crates/gas/src/lib.rs b/crates/gas/src/lib.rs index 3c9205af66..2847c9c0ac 100644 --- a/crates/gas/src/lib.rs +++ b/crates/gas/src/lib.rs @@ -292,10 +292,8 @@ impl From for u64 { /// Gas represented in whole units. Used for fee payment and to display /// information to the user. -// FIXME: remove Default trait if we remove WholeGas from VpsResult #[derive( Debug, - Default, Clone, Copy, PartialEq, @@ -377,7 +375,6 @@ pub trait GasMetering { /// Check if the vps went out of gas. Starts with the gas consumed by the /// transaction. - /// fn check_vps_limit(&self, vps_gas: Gas) -> Result<()> { let total = self .get_tx_consumed_gas() @@ -403,7 +400,6 @@ pub struct TxGasMeter { /// Gas metering in a validity predicate #[derive(Debug, Clone)] -// FIXME: can we simplify this type? pub struct VpGasMeter { /// Track gas overflow gas_overflow: bool, diff --git a/crates/node/src/protocol.rs b/crates/node/src/protocol.rs index 90ada1859a..ba34eebb51 100644 --- a/crates/node/src/protocol.rs +++ b/crates/node/src/protocol.rs @@ -1092,7 +1092,7 @@ where .write_log() .verifiers_and_changed_keys(verifiers_from_tx); - let vps_result = execute_vps( + let (vps_result, vps_gas) = execute_vps( verifiers, keys_changed, tx, @@ -1101,10 +1101,10 @@ where tx_gas_meter, vp_wasm_cache, )?; - tracing::debug!("Total VPs gas cost {:?}", vps_result.gas_used); + tracing::debug!("Total VPs gas cost {:?}", vps_gas); tx_gas_meter - .consume(vps_result.gas_used.into()) + .consume(vps_gas.into()) .map_err(|err| Error::GasError(err.to_string()))?; Ok(vps_result) @@ -1120,62 +1120,72 @@ fn execute_vps( state: &S, tx_gas_meter: &TxGasMeter, vp_wasm_cache: &mut VpCache, -) -> Result +) -> Result<(VpsResult, namada_sdk::gas::Gas)> where S: 'static + State + Sync, CA: 'static + WasmCacheAccess + Sync, { - let vps_result = verifiers - .par_iter() - .try_fold(VpsResult::default, |mut result, addr| { - let gas_meter = - RefCell::new(VpGasMeter::new_from_tx_meter(tx_gas_meter)); - let tx_accepted = match &addr { - Address::Implicit(_) | Address::Established(_) => { - let (vp_hash, gas) = state + let vps_result = + verifiers + .par_iter() + .try_fold( + || (VpsResult::default(), Gas::from(0)), + |(mut result, mut vps_gas), addr| { + let gas_meter = RefCell::new( + VpGasMeter::new_from_tx_meter(tx_gas_meter), + ); + let tx_accepted = + match &addr { + Address::Implicit(_) | Address::Established(_) => { + let (vp_hash, gas) = state .validity_predicate::>(addr) .map_err(Error::StateError)?; - gas_meter - .borrow_mut() - .consume(gas) - .map_err(|err| Error::GasError(err.to_string()))?; - let Some(vp_code_hash) = vp_hash else { - return Err(Error::MissingAddress(addr.clone())); - }; - - wasm::run::vp( - vp_code_hash, - batched_tx, - tx_index, - addr, - state, - &gas_meter, - &keys_changed, - &verifiers, - vp_wasm_cache.clone(), - ) - .map_err(|err| match err { + gas_meter.borrow_mut().consume(gas).map_err( + |err| Error::GasError(err.to_string()), + )?; + let Some(vp_code_hash) = vp_hash else { + return Err(Error::MissingAddress( + addr.clone(), + )); + }; + + wasm::run::vp( + vp_code_hash, + batched_tx, + tx_index, + addr, + state, + &gas_meter, + &keys_changed, + &verifiers, + vp_wasm_cache.clone(), + ) + .map_err( + |err| { + match err { wasm::run::Error::GasError(msg) => Error::GasError(msg), wasm::run::Error::InvalidSectionSignature(msg) => { Error::InvalidSectionSignature(msg) } _ => Error::VpRunnerError(err), - }) - } - Address::Internal(internal_addr) => { - let ctx = NativeVpCtx::new( - addr, - state, - batched_tx.tx, - batched_tx.cmt, - tx_index, - &gas_meter, - &keys_changed, - &verifiers, - vp_wasm_cache.clone(), - ); + } + }, + ) + } + Address::Internal(internal_addr) => { + let ctx = NativeVpCtx::new( + addr, + state, + batched_tx.tx, + batched_tx.cmt, + tx_index, + &gas_meter, + &keys_changed, + &verifiers, + vp_wasm_cache.clone(), + ); - match internal_addr { + match internal_addr { InternalAddress::PoS => { let pos = PosVp::new(ctx); pos.validate_tx( @@ -1301,53 +1311,56 @@ where Error::AccessForbidden((*internal_addr).clone()), ), } - } - }; + } + }; + + tx_accepted.map_or_else( + |err| { + result + .status_flags + .insert(err.invalid_section_signature_flag()); + result.rejected_vps.insert(addr.clone()); + result.errors.push((addr.clone(), err.to_string())); + }, + |()| { + result.accepted_vps.insert(addr.clone()); + }, + ); - tx_accepted.map_or_else( - |err| { - result - .status_flags - .insert(err.invalid_section_signature_flag()); - result.rejected_vps.insert(addr.clone()); - result.errors.push((addr.clone(), err.to_string())); - }, - |()| { - result.accepted_vps.insert(addr.clone()); - }, - ); + // Execution of VPs can (and must) be short-circuited + // only in case of a gas overflow to prevent the + // transaction from consuming resources that have not + // been acquired in the corresponding wrapper tx. For + // all the other errors we keep evaluating the vps. This + // allows to display a consistent VpsResult across all + // nodes and find any invalid signatures + vps_gas = vps_gas + .checked_add(gas_meter.borrow().get_vp_consumed_gas()) + .ok_or(Error::GasError( + gas::Error::GasOverflow.to_string(), + ))?; + gas_meter + .borrow() + .check_vps_limit(vps_gas) + .map_err(|err| Error::GasError(err.to_string()))?; - // Execution of VPs can (and must) be short-circuited - // only in case of a gas overflow to prevent the - // transaction from consuming resources that have not - // been acquired in the corresponding wrapper tx. For - // all the other errors we keep evaluating the vps. This - // allows to display a consistent VpsResult across all - // nodes and find any invalid signatures - result.gas_used = result - .gas_used - .checked_add(gas_meter.borrow().get_vp_consumed_gas()) - .ok_or(Error::GasError(gas::Error::GasOverflow.to_string()))?; - gas_meter - .borrow() - .check_vps_limit(result.gas_used) - .map_err(|err| Error::GasError(err.to_string()))?; - - Ok(result) - }) - .try_reduce(VpsResult::default, |a, b| { - merge_vp_results(a, b, tx_gas_meter) - })?; + Ok((result, vps_gas)) + }, + ) + .try_reduce( + || (VpsResult::default(), Gas::from(0)), + |a, b| merge_vp_results(a, b, tx_gas_meter), + )?; Ok(vps_result) } /// Merge VP results from parallel runs fn merge_vp_results( - a: VpsResult, - mut b: VpsResult, + (a, a_gas): (VpsResult, Gas), + (mut b, b_gas): (VpsResult, Gas), tx_gas_meter: &TxGasMeter, -) -> Result { +) -> Result<(VpsResult, Gas)> { let mut accepted_vps = a.accepted_vps; let mut rejected_vps = a.rejected_vps; accepted_vps.extend(b.accepted_vps); @@ -1356,21 +1369,22 @@ fn merge_vp_results( errors.append(&mut b.errors); let status_flags = a.status_flags | b.status_flags; - let gas_used = a - .gas_used - .checked_add(b.gas_used) + let vps_gas = a_gas + .checked_add(b_gas) .ok_or(Error::GasError(gas::Error::GasOverflow.to_string()))?; tx_gas_meter - .check_vps_limit(gas_used) + .check_vps_limit(vps_gas) .map_err(|err| Error::GasError(err.to_string()))?; - Ok(VpsResult { - accepted_vps, - rejected_vps, - gas_used, - errors, - status_flags, - }) + Ok(( + VpsResult { + accepted_vps, + rejected_vps, + errors, + status_flags, + }, + vps_gas, + )) } #[cfg(test)] diff --git a/crates/tx/src/data/mod.rs b/crates/tx/src/data/mod.rs index 4b0b0121a3..ea73b8ed70 100644 --- a/crates/tx/src/data/mod.rs +++ b/crates/tx/src/data/mod.rs @@ -26,7 +26,7 @@ use namada_core::ibc::IbcTxDataRefs; use namada_core::masp::MaspTxRefs; use namada_core::storage; use namada_events::Event; -use namada_gas::{Gas, WholeGas}; +use namada_gas::WholeGas; use namada_macros::BorshDeserializer; #[cfg(feature = "migrations")] use namada_migrations::*; @@ -481,9 +481,6 @@ pub struct VpsResult { pub accepted_vps: BTreeSet
, /// The addresses whose VPs rejected the transaction pub rejected_vps: BTreeSet
, - /// The total gas used by all the VPs - // FIXME: can't we just remove it? - pub gas_used: Gas, /// Errors occurred in any of the VPs, if any pub errors: Vec<(Address, String)>, /// Validity predicate status flags, containing info From 92feb38d601d69c44f0bb5b45e26ce0aa5773cb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Tue, 13 Aug 2024 11:39:21 +0100 Subject: [PATCH 3/4] update hermes --- .github/workflows/scripts/hermes.txt | 2 +- scripts/get_hermes.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/scripts/hermes.txt b/.github/workflows/scripts/hermes.txt index dcaa2f05c3..ce746dcc90 100644 --- a/.github/workflows/scripts/hermes.txt +++ b/.github/workflows/scripts/hermes.txt @@ -1 +1 @@ -1.10.0-namada-beta15-rc +1.10.0-namada-beta15-rc2 diff --git a/scripts/get_hermes.sh b/scripts/get_hermes.sh index eba0924b02..0923f26101 100755 --- a/scripts/get_hermes.sh +++ b/scripts/get_hermes.sh @@ -4,7 +4,7 @@ set -Eo pipefail HERMES_MAJORMINOR="1.10" HERMES_PATCH="0" -HERMES_SUFFIX="-namada-beta15-rc" +HERMES_SUFFIX="-namada-beta15-rc2" HERMES_REPO="https://github.com/heliaxdev/hermes" From 0c07a1ba76116bd41b66a2e8a5cbc1b377cdc0f3 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 12 Aug 2024 16:31:18 +0200 Subject: [PATCH 4/4] Changelog #3615 --- .changelog/unreleased/improvements/3615-no-parallel-gas.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/improvements/3615-no-parallel-gas.md diff --git a/.changelog/unreleased/improvements/3615-no-parallel-gas.md b/.changelog/unreleased/improvements/3615-no-parallel-gas.md new file mode 100644 index 0000000000..2c4a768129 --- /dev/null +++ b/.changelog/unreleased/improvements/3615-no-parallel-gas.md @@ -0,0 +1,2 @@ +- Removed parallel gas accounting. + ([\#3615](https://github.com/anoma/namada/pull/3615)) \ No newline at end of file