app: destructure TransactionParameters during validation

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<S: StateRead + 'static>(&self, state: Arc<S>) -> 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

crates/core/app/src/action_handler/transaction/stateful.rs

@@ -6,35 +6,55 @@ 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<S: StateRead>(state: S, transaction: &Transaction) -> Result<()> {
+pub async fn tx_parameters_historical_check<S: StateRead>(
+    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<S: StateRead>(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
     );
     Ok(())
 }
 
-pub async fn expiry_height_is_valid<S: StateRead>(
-    state: S,
-    transaction: &Transaction,
-) -> Result<()> {
+pub async fn expiry_height_is_valid<S: StateRead>(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<S: StateRead>(
     Ok(())
 }
 
+pub async fn fee_greater_than_base_fee<S: StateRead>(
+    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<S: StateRead>(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<S: StateRead>(
 ) -> Result<()> {
     state.check_claimed_anchor(transaction.anchor).await
 }
-
-pub async fn fee_greater_than_base_fee<S: StateRead>(
-    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(())
-}