Skip to content

Commit

Permalink
Removes gas_used from VpsResult
Browse files Browse the repository at this point in the history
  • Loading branch information
@grarco
grarco committed Aug 12, 2024
1 parent c47ca1b commit 5a9fa18
Show file tree
Hide file tree
Showing 3 changed files with 111 additions and 104 deletions.
4 changes: 0 additions & 4 deletions crates/gas/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -292,10 +292,8 @@ impl From<Gas> 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,
Expand Down Expand Up @@ -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()
Expand All @@ -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,
Expand Down
206 changes: 110 additions & 96 deletions crates/node/src/protocol.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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)
Expand All @@ -1120,62 +1120,72 @@ fn execute_vps<S, CA>(
state: &S,
tx_gas_meter: &TxGasMeter,
vp_wasm_cache: &mut VpCache<CA>,
) -> Result<VpsResult>
) -> 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::<parameters::Store<()>>(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(
Expand Down Expand Up @@ -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<VpsResult> {
) -> Result<(VpsResult, Gas)> {
let mut accepted_vps = a.accepted_vps;
let mut rejected_vps = a.rejected_vps;
accepted_vps.extend(b.accepted_vps);
Expand All @@ -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)]
Expand Down
5 changes: 1 addition & 4 deletions crates/tx/src/data/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;
Expand Down Expand Up @@ -481,9 +481,6 @@ pub struct VpsResult {
pub accepted_vps: BTreeSet<Address>,
/// The addresses whose VPs rejected the transaction
pub rejected_vps: BTreeSet<Address>,
/// 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
Expand Down

0 comments on commit 5a9fa18

Please sign in to comment.