From 51e45712e90a3aed554dd1757ef55a9d7a7fcaec Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 12 Aug 2024 15:57:09 +0200 Subject: [PATCH] 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 3c9205af669..2847c9c0acb 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 90ada1859a0..ba34eebb51b 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 4b0b0121a39..ea73b8ed70d 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