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 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/crates/gas/src/lib.rs b/crates/gas/src/lib.rs index d3d78e8419..2847c9c0ac 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; @@ -375,6 +372,20 @@ 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 @@ -400,23 +411,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 +478,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 +552,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..ba34eebb51 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}; @@ -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 - .add_vps_gas(&vps_result.gas_used) + .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,49 +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 - .set(gas_meter.into_inner()) - .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); @@ -1351,19 +1368,23 @@ 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 vps_gas = a_gas + .checked_add(b_gas) + .ok_or(Error::GasError(gas::Error::GasOverflow.to_string()))?; + tx_gas_meter + .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/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..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::{VpsGas, WholeGas}; +use namada_gas::WholeGas; use namada_macros::BorshDeserializer; #[cfg(feature = "migrations")] use namada_migrations::*; @@ -481,8 +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 - pub gas_used: VpsGas, /// Errors occurred in any of the VPs, if any pub errors: Vec<(Address, String)>, /// Validity predicate status flags, containing info 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"