From 9a944a76f3e3c69391dd3b2ffd0a5e91f3a82c91 Mon Sep 17 00:00:00 2001 From: Erwan Or Date: Sat, 4 May 2024 10:55:34 -0400 Subject: [PATCH] app: destructure `TransactionParameters` during validation --- .../app/src/action_handler/transaction.rs | 14 +-- .../action_handler/transaction/stateful.rs | 106 +++++++++++------- 2 files changed, 67 insertions(+), 53 deletions(-) diff --git a/crates/core/app/src/action_handler/transaction.rs b/crates/core/app/src/action_handler/transaction.rs index 5e5db3fcf0..b289c0938a 100644 --- a/crates/core/app/src/action_handler/transaction.rs +++ b/crates/core/app/src/action_handler/transaction.rs @@ -14,8 +14,7 @@ mod stateful; mod stateless; use self::stateful::{ - chain_id_is_correct, claimed_anchor_is_valid, expiry_height_is_valid, - fee_greater_than_base_fee, fmd_parameters_valid, + claimed_anchor_is_valid, fmd_parameters_valid, tx_parameters_historical_check, }; use stateless::{ check_memo_exists_if_outputs_absent_if_not, num_clues_equal_to_num_outputs, @@ -59,18 +58,13 @@ impl AppActionHandler for Transaction { async fn check_historical(&self, state: Arc) -> Result<()> { let mut action_checks = JoinSet::new(); - // TODO: these could be pushed into the action checks and run concurrently if needed - - // SAFETY: chain ID never changes during a transaction execution. - chain_id_is_correct(state.clone(), self).await?; - // SAFETY: height never changes during a transaction execution. - expiry_height_is_valid(state.clone(), self).await?; + // SAFETY: Transaction parameters (chain id, expiry height, fee) against chain state + // that cannot change during transaction execution. + tx_parameters_historical_check(state.clone(), self).await?; // SAFETY: anchors are historical data and cannot change during transaction execution. claimed_anchor_is_valid(state.clone(), self).await?; // SAFETY: FMD parameters cannot change during transaction execution. fmd_parameters_valid(state.clone(), self).await?; - // SAFETY: gas prices cannot change during transaction execution. - fee_greater_than_base_fee(state.clone(), self).await?; // Currently, we need to clone the component actions so that the spawned // futures can have 'static lifetimes. In the future, we could try to diff --git a/crates/core/app/src/action_handler/transaction/stateful.rs b/crates/core/app/src/action_handler/transaction/stateful.rs index ea5a490b86..6a5a5c4fdb 100644 --- a/crates/core/app/src/action_handler/transaction/stateful.rs +++ b/crates/core/app/src/action_handler/transaction/stateful.rs @@ -6,19 +6,46 @@ use penumbra_sct::component::tree::VerificationExt; use penumbra_shielded_pool::component::StateReadExt as _; use penumbra_shielded_pool::fmd; use penumbra_transaction::gas::GasCost; -use penumbra_transaction::Transaction; +use penumbra_transaction::{Transaction, TransactionParameters}; use crate::app::StateReadExt; const FMD_GRACE_PERIOD_BLOCKS: u64 = 10; -pub async fn chain_id_is_correct(state: S, transaction: &Transaction) -> Result<()> { +pub async fn tx_parameters_historical_check( + state: S, + transaction: &Transaction, +) -> Result<()> { + let TransactionParameters { + chain_id, + expiry_height, + // This is checked in `fee_greater_than_base_fee` against the whole + // transaction, for convenience. + fee: _, + // IMPORTANT: Adding a transaction parameter? Then you **must** add a SAFETY + // argument here to justify why it is safe to validate against a historical + // state. + } = transaction.transaction_parameters(); + + // SAFETY: This is safe to do in a **historical** check because the chain's actual + // id cannot change during transaction processing. + chain_id_is_correct(&state, chain_id).await?; + // SAFETY: This is safe to do in a **historical** check because the chain's current + // block height cannot change during transaction processing. + expiry_height_is_valid(&state, expiry_height).await?; + // SAFETY: This is safe to do in a **historical** check as long as the current gas prices + // are static, or set in the previous block. + fee_greater_than_base_fee(&state, transaction).await?; + + Ok(()) +} + +pub async fn chain_id_is_correct(state: S, tx_chain_id: String) -> Result<()> { let chain_id = state.get_chain_id().await?; - let tx_chain_id = &transaction.transaction_body.transaction_parameters.chain_id; // The chain ID in the transaction must exactly match the current chain ID. ensure!( - *tx_chain_id == chain_id, + tx_chain_id == chain_id, "transaction chain ID '{}' must match the current chain ID '{}'", tx_chain_id, chain_id @@ -26,15 +53,8 @@ pub async fn chain_id_is_correct(state: S, transaction: &Transacti Ok(()) } -pub async fn expiry_height_is_valid( - state: S, - transaction: &Transaction, -) -> Result<()> { +pub async fn expiry_height_is_valid(state: S, expiry_height: u64) -> Result<()> { let current_height = state.get_block_height().await?; - let expiry_height = transaction - .transaction_body - .transaction_parameters - .expiry_height; // A zero expiry height means that the transaction is valid indefinitely. if expiry_height == 0 { @@ -52,6 +72,37 @@ pub async fn expiry_height_is_valid( Ok(()) } +pub async fn fee_greater_than_base_fee( + state: S, + transaction: &Transaction, +) -> Result<()> { + let current_gas_prices = state + .get_gas_prices() + .await + .expect("gas prices must be present in state"); + + let transaction_base_price = current_gas_prices.fee(&transaction.gas_cost()); + let user_supplied_fee = transaction.transaction_body().transaction_parameters.fee; + let user_supplied_fee_amount = user_supplied_fee.amount(); + let user_supplied_fee_asset_id = user_supplied_fee.asset_id(); + + ensure!( + user_supplied_fee_amount >= transaction_base_price, + "fee must be greater than or equal to the transaction base price (supplied: {}, base: {})", + user_supplied_fee_amount, + transaction_base_price + ); + + // We split the check to provide granular error messages. + ensure!( + user_supplied_fee_asset_id == *penumbra_asset::STAKING_TOKEN_ASSET_ID, + "fee must be paid in staking tokens (found: {})", + user_supplied_fee_asset_id + ); + + Ok(()) +} + pub async fn fmd_parameters_valid(state: S, transaction: &Transaction) -> Result<()> { let previous_fmd_parameters = state .get_previous_fmd_parameters() @@ -120,34 +171,3 @@ pub async fn claimed_anchor_is_valid( ) -> Result<()> { state.check_claimed_anchor(transaction.anchor).await } - -pub async fn fee_greater_than_base_fee( - state: S, - transaction: &Transaction, -) -> Result<()> { - let current_gas_prices = state - .get_gas_prices() - .await - .expect("gas prices must be present in state"); - - let transaction_base_price = current_gas_prices.fee(&transaction.gas_cost()); - let user_supplied_fee = transaction.transaction_body().transaction_parameters.fee; - let user_supplied_fee_amount = user_supplied_fee.amount(); - let user_supplied_fee_asset_id = user_supplied_fee.asset_id(); - - ensure!( - user_supplied_fee_amount >= transaction_base_price, - "fee must be greater than or equal to the transaction base price (supplied: {}, base: {})", - user_supplied_fee_amount, - transaction_base_price - ); - - // We split the check to provide granular error messages. - ensure!( - user_supplied_fee_asset_id == *penumbra_asset::STAKING_TOKEN_ASSET_ID, - "fee must be paid in staking tokens (found: {})", - user_supplied_fee_asset_id - ); - - Ok(()) -}