From 1f0841852c9468518df766721d62f367d2963a14 Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Tue, 5 Mar 2024 21:22:40 -0800 Subject: [PATCH] component: eliminate TOCTOU bug factory Closes #3690 This renames the previous `check_stateful` method to `check_historical`, renames `execute` to `check_and_execute`, and by default moves all of the content of `check_stateful` into `check_and_execute`, unless there's an obvious and LOCAL reason why it's safe to do so that can be written into the body of the method. IMO it's still worth it to have the method in the trait. We may wish to take advantage of it later, when we can spend more time carefully reasoning about performance. Note: we've left behind two versions of the trait, which will be reconciled in a later commit. --- .../cnidarium-component/src/action_handler.rs | 38 ++++++---- crates/core/app/src/action_handler.rs | 4 +- crates/core/app/src/action_handler/actions.rs | 76 +++++++++---------- crates/core/app/tests/spend.rs | 12 +-- crates/core/app/tests/swap_and_swap_claim.rs | 22 +++--- .../action_handler/community_pool_deposit.rs | 11 +-- .../action_handler/community_pool_output.rs | 11 +-- .../action_handler/community_pool_spend.rs | 12 +-- .../action_handler/position/close.rs | 10 +-- .../component/action_handler/position/open.rs | 10 +-- .../action_handler/position/withdraw.rs | 12 +-- .../dex/src/component/action_handler/swap.rs | 10 +-- .../component/action_handler/swap_claim.rs | 15 ++-- .../src/action_handler/delegator_vote.rs | 26 +------ .../src/action_handler/deposit_claim.rs | 9 +-- .../src/action_handler/validator_vote.rs | 28 ++----- .../governance/src/action_handler/withdraw.rs | 9 +-- .../action_handler/ics20_withdrawal.rs | 11 +-- .../src/component/action_handler/output.rs | 10 +-- .../src/component/action_handler/spend.rs | 10 +-- .../src/component/action_handler/delegate.rs | 14 ++-- .../component/action_handler/undelegate.rs | 14 ++-- .../action_handler/undelegate_claim.rs | 17 +++-- .../action_handler/validator_definition.rs | 14 ++-- 24 files changed, 157 insertions(+), 248 deletions(-) diff --git a/crates/cnidarium-component/src/action_handler.rs b/crates/cnidarium-component/src/action_handler.rs index a5b198f2c0..abb64eae57 100644 --- a/crates/cnidarium-component/src/action_handler.rs +++ b/crates/cnidarium-component/src/action_handler.rs @@ -44,33 +44,45 @@ pub trait ActionHandler { /// be run in parallel across all transactions in a block. async fn check_stateless(&self, context: Self::CheckStatelessContext) -> Result<()>; - /// Performs all of this action's stateful validity checks. + /// Performs those stateful validity checks that can be performed against a + /// historical state snapshot. /// /// This method provides read access to a snapshot of the `State` prior to - /// transaction execution. Because it does not give write access, it's safe - /// to run the `ActionHandler` implementations for all actions in a given - /// transaction in parallel. + /// transaction execution. It is intended to be run in parallel across all + /// actions within a transaction. /// - /// As much work as possible should be done in `check_stateful`, as it can - /// be run in parallel across all actions within a transaction. + /// # Warning + /// + /// Misuse of this method creates TOCTOU vulnerabilities. Checks performed + /// in this method must be valid if they are performed against a _prior_ + /// state, as another action in the same transaction may execute first and + /// change the state. + /// + /// Checks performed in this phase should have a justification for why they + /// are safe to run in parallel with other actions in the same transaction, + /// and the default behavior should be to perform checks in + /// [`ActionHandler::check_and_execute`]. /// /// # Invariants /// /// This method should only be called on data that has been checked /// with [`ActionHandler::check_stateless`]. This method can be called /// before [`Component::begin_block`](crate::Component::begin_block). - async fn check_stateful(&self, state: Arc) -> Result<()>; + async fn check_historical(&self, _state: Arc) -> Result<()> { + // Default behavior: no-op + Ok(()) + } /// Attempts to execute this action against the provided `state`. /// /// This method provides read and write access to the `state`. It is - /// fallible, so it's possible to perform checks within the `execute` + /// fallible, so it's possible to perform checks within the `check_and_execute` /// implementation and abort execution on error; the [`StateTransaction`] /// mechanism ensures that all writes are correctly discarded. /// /// Because `execute` must run sequentially, whenever possible, checks - /// should be performed in [`ActionHandler::check_stateful`] or - /// [`ActionHandler::check_stateless`]. One example of where this is not + /// should be performed in [`ActionHandler::check_stateless`], or (more carefully) in + /// [`ActionHandler::check_historical`]. One example of where this is not /// possible (in fact, the motivating example) is for IBC, where a /// transaction may (1) submit a client update and then (2) relay messages /// valid relative to the newly updated state. In this case, the checks for @@ -81,8 +93,8 @@ pub trait ActionHandler { /// /// # Invariants /// - /// This method should only be called immediately after an invocation of - /// [`ActionHandler::check_stateful`] on the same transaction. This method + /// This method should only be called after an invocation of + /// [`ActionHandler::check_historical`] on the same transaction. This method /// can be called before [`Component::begin_block`](crate::Component::begin_block). - async fn execute(&self, state: S) -> Result<()>; + async fn check_and_execute(&self, state: S) -> Result<()>; } diff --git a/crates/core/app/src/action_handler.rs b/crates/core/app/src/action_handler.rs index f4d5785ba5..0ce95417cd 100644 --- a/crates/core/app/src/action_handler.rs +++ b/crates/core/app/src/action_handler.rs @@ -33,9 +33,9 @@ impl<'a, T: ComponentActionHandler + Sync> ActionHandler for crate::Compat<'a, T ComponentActionHandler::check_stateless(self.0, context).await } async fn check_stateful(&self, state: Arc) -> Result<()> { - ComponentActionHandler::check_stateful(self.0, state).await + ComponentActionHandler::check_historical(self.0, state).await } async fn execute(&self, state: S) -> Result<()> { - ComponentActionHandler::execute(self.0, state).await + ComponentActionHandler::check_and_execute(self.0, state).await } } diff --git a/crates/core/app/src/action_handler/actions.rs b/crates/core/app/src/action_handler/actions.rs index 2dd8828a39..3806d6c6f4 100644 --- a/crates/core/app/src/action_handler/actions.rs +++ b/crates/core/app/src/action_handler/actions.rs @@ -55,22 +55,22 @@ impl ActionHandler for Action { async fn check_stateful(&self, state: Arc) -> Result<()> { match self { - Action::Delegate(action) => action.check_stateful(state).await, - Action::Undelegate(action) => action.check_stateful(state).await, - Action::UndelegateClaim(action) => action.check_stateful(state).await, - Action::ValidatorDefinition(action) => action.check_stateful(state).await, - Action::DelegatorVote(action) => action.check_stateful(state).await, - Action::ValidatorVote(action) => action.check_stateful(state).await, - Action::PositionClose(action) => action.check_stateful(state).await, - Action::PositionOpen(action) => action.check_stateful(state).await, - Action::PositionWithdraw(action) => action.check_stateful(state).await, + Action::Delegate(action) => action.check_historical(state).await, + Action::Undelegate(action) => action.check_historical(state).await, + Action::UndelegateClaim(action) => action.check_historical(state).await, + Action::ValidatorDefinition(action) => action.check_historical(state).await, + Action::DelegatorVote(action) => action.check_historical(state).await, + Action::ValidatorVote(action) => action.check_historical(state).await, + Action::PositionClose(action) => action.check_historical(state).await, + Action::PositionOpen(action) => action.check_historical(state).await, + Action::PositionWithdraw(action) => action.check_historical(state).await, Action::ProposalSubmit(action) => action.check_stateful(state).await, - Action::ProposalWithdraw(action) => action.check_stateful(state).await, - Action::ProposalDepositClaim(action) => action.check_stateful(state).await, - Action::Swap(action) => action.check_stateful(state).await, - Action::SwapClaim(action) => action.check_stateful(state).await, - Action::Spend(action) => action.check_stateful(state).await, - Action::Output(action) => action.check_stateful(state).await, + Action::ProposalWithdraw(action) => action.check_historical(state).await, + Action::ProposalDepositClaim(action) => action.check_historical(state).await, + Action::Swap(action) => action.check_historical(state).await, + Action::SwapClaim(action) => action.check_historical(state).await, + Action::Spend(action) => action.check_historical(state).await, + Action::Output(action) => action.check_historical(state).await, Action::IbcRelay(action) => { if !state.get_ibc_params().await?.ibc_enabled { anyhow::bail!("transaction contains IBC actions, but IBC is not enabled"); @@ -82,31 +82,31 @@ impl ActionHandler for Action { .check_stateful(state) .await } - Action::Ics20Withdrawal(action) => action.check_stateful(state).await, - Action::CommunityPoolSpend(action) => action.check_stateful(state).await, - Action::CommunityPoolOutput(action) => action.check_stateful(state).await, - Action::CommunityPoolDeposit(action) => action.check_stateful(state).await, + Action::Ics20Withdrawal(action) => action.check_historical(state).await, + Action::CommunityPoolSpend(action) => action.check_historical(state).await, + Action::CommunityPoolOutput(action) => action.check_historical(state).await, + Action::CommunityPoolDeposit(action) => action.check_historical(state).await, } } async fn execute(&self, state: S) -> Result<()> { match self { - Action::Delegate(action) => action.execute(state).await, - Action::Undelegate(action) => action.execute(state).await, - Action::UndelegateClaim(action) => action.execute(state).await, - Action::ValidatorDefinition(action) => action.execute(state).await, - Action::DelegatorVote(action) => action.execute(state).await, - Action::ValidatorVote(action) => action.execute(state).await, - Action::PositionClose(action) => action.execute(state).await, - Action::PositionOpen(action) => action.execute(state).await, - Action::PositionWithdraw(action) => action.execute(state).await, + Action::Delegate(action) => action.check_and_execute(state).await, + Action::Undelegate(action) => action.check_and_execute(state).await, + Action::UndelegateClaim(action) => action.check_and_execute(state).await, + Action::ValidatorDefinition(action) => action.check_and_execute(state).await, + Action::DelegatorVote(action) => action.check_and_execute(state).await, + Action::ValidatorVote(action) => action.check_and_execute(state).await, + Action::PositionClose(action) => action.check_and_execute(state).await, + Action::PositionOpen(action) => action.check_and_execute(state).await, + Action::PositionWithdraw(action) => action.check_and_execute(state).await, Action::ProposalSubmit(action) => action.execute(state).await, - Action::ProposalWithdraw(action) => action.execute(state).await, - Action::ProposalDepositClaim(action) => action.execute(state).await, - Action::Swap(action) => action.execute(state).await, - Action::SwapClaim(action) => action.execute(state).await, - Action::Spend(action) => action.execute(state).await, - Action::Output(action) => action.execute(state).await, + Action::ProposalWithdraw(action) => action.check_and_execute(state).await, + Action::ProposalDepositClaim(action) => action.check_and_execute(state).await, + Action::Swap(action) => action.check_and_execute(state).await, + Action::SwapClaim(action) => action.check_and_execute(state).await, + Action::Spend(action) => action.check_and_execute(state).await, + Action::Output(action) => action.check_and_execute(state).await, Action::IbcRelay(action) => { action .clone() @@ -114,10 +114,10 @@ impl ActionHandler for Action { .execute(state) .await } - Action::Ics20Withdrawal(action) => action.execute(state).await, - Action::CommunityPoolSpend(action) => action.execute(state).await, - Action::CommunityPoolOutput(action) => action.execute(state).await, - Action::CommunityPoolDeposit(action) => action.execute(state).await, + Action::Ics20Withdrawal(action) => action.check_and_execute(state).await, + Action::CommunityPoolSpend(action) => action.check_and_execute(state).await, + Action::CommunityPoolOutput(action) => action.check_and_execute(state).await, + Action::CommunityPoolDeposit(action) => action.check_and_execute(state).await, } } } diff --git a/crates/core/app/tests/spend.rs b/crates/core/app/tests/spend.rs index caf114d52d..91eed71431 100644 --- a/crates/core/app/tests/spend.rs +++ b/crates/core/app/tests/spend.rs @@ -69,10 +69,10 @@ async fn spend_happy_path() -> anyhow::Result<()> { // 3. Simulate execution of the Spend action spend.check_stateless(transaction_context).await?; - spend.check_stateful(state.clone()).await?; + spend.check_historical(state.clone()).await?; let mut state_tx = state.try_begin_transaction().unwrap(); state_tx.put_mock_source(1u8); - spend.execute(&mut state_tx).await?; + spend.check_and_execute(&mut state_tx).await?; state_tx.apply(); // 4. Execute EndBlock @@ -253,13 +253,13 @@ async fn spend_duplicate_nullifier_previous_transaction() { .await .expect("can apply first spend"); spend - .check_stateful(state.clone()) + .check_historical(state.clone()) .await .expect("can apply first spend"); let mut state_tx = state.try_begin_transaction().unwrap(); state_tx.put_mock_source(1u8); spend - .execute(&mut state_tx) + .check_and_execute(&mut state_tx) .await .expect("should be able to apply first spend"); state_tx.apply(); @@ -280,10 +280,10 @@ async fn spend_duplicate_nullifier_previous_transaction() { .check_stateless(transaction_context) .await .expect("check stateless should succeed"); - spend.check_stateful(state.clone()).await.unwrap(); + spend.check_historical(state.clone()).await.unwrap(); let mut state_tx = state.try_begin_transaction().unwrap(); state_tx.put_mock_source(2u8); - spend.execute(&mut state_tx).await.unwrap(); + spend.check_and_execute(&mut state_tx).await.unwrap(); state_tx.apply(); } diff --git a/crates/core/app/tests/swap_and_swap_claim.rs b/crates/core/app/tests/swap_and_swap_claim.rs index 8698f69513..9062d8459b 100644 --- a/crates/core/app/tests/swap_and_swap_claim.rs +++ b/crates/core/app/tests/swap_and_swap_claim.rs @@ -69,10 +69,10 @@ async fn swap_and_swap_claim() -> anyhow::Result<()> { // 3. Simulate execution of the Swap action swap.check_stateless(()).await?; - swap.check_stateful(state.clone()).await?; + swap.check_historical(state.clone()).await?; let mut state_tx = state.try_begin_transaction().unwrap(); state_tx.put_mock_source(1u8); - swap.execute(&mut state_tx).await?; + swap.check_and_execute(&mut state_tx).await?; state_tx.apply(); // 4. Execute EndBlock (where the swap is actually executed) @@ -127,10 +127,10 @@ async fn swap_and_swap_claim() -> anyhow::Result<()> { .context(); claim.check_stateless(context.clone()).await?; - claim.check_stateful(state.clone()).await?; + claim.check_historical(state.clone()).await?; let mut state_tx = state.try_begin_transaction().unwrap(); state_tx.put_mock_source(2u8); - claim.execute(&mut state_tx).await?; + claim.check_and_execute(&mut state_tx).await?; state_tx.apply(); Ok(()) @@ -184,10 +184,10 @@ async fn swap_claim_duplicate_nullifier_previous_transaction() { // 3. Simulate execution of the Swap action swap.check_stateless(()).await.unwrap(); - swap.check_stateful(state.clone()).await.unwrap(); + swap.check_historical(state.clone()).await.unwrap(); let mut state_tx = state.try_begin_transaction().unwrap(); state_tx.put_mock_source(1u8); - swap.execute(&mut state_tx).await.unwrap(); + swap.check_and_execute(&mut state_tx).await.unwrap(); state_tx.apply(); // 4. Execute EndBlock (where the swap is actually executed) @@ -241,10 +241,10 @@ async fn swap_claim_duplicate_nullifier_previous_transaction() { .context(); claim.check_stateless(context.clone()).await.unwrap(); - claim.check_stateful(state.clone()).await.unwrap(); + claim.check_historical(state.clone()).await.unwrap(); let mut state_tx = state.try_begin_transaction().unwrap(); state_tx.put_mock_source(2u8); - claim.execute(&mut state_tx).await.unwrap(); + claim.check_and_execute(&mut state_tx).await.unwrap(); state_tx.apply(); // 8. Now form a second SwapClaim action attempting to claim the outputs again. @@ -259,7 +259,7 @@ async fn swap_claim_duplicate_nullifier_previous_transaction() { let claim = claim_plan.swap_claim(&test_keys::FULL_VIEWING_KEY, &swap_auth_path); // 9. Execute the second SwapClaim action - the test should panic here - claim.check_stateful(state.clone()).await.unwrap(); + claim.check_historical(state.clone()).await.unwrap(); } #[tokio::test] @@ -304,10 +304,10 @@ async fn swap_with_nonzero_fee() -> anyhow::Result<()> { // 3. Simulate execution of the Swap action swap.check_stateless(()).await?; - swap.check_stateful(state.clone()).await?; + swap.check_historical(state.clone()).await?; let mut state_tx = state.try_begin_transaction().unwrap(); state_tx.put_mock_source(1u8); - swap.execute(&mut state_tx).await?; + swap.check_and_execute(&mut state_tx).await?; state_tx.apply(); // 4. Execute EndBlock (where the swap is actually executed) diff --git a/crates/core/component/community-pool/src/component/action_handler/community_pool_deposit.rs b/crates/core/component/community-pool/src/component/action_handler/community_pool_deposit.rs index 6922085815..bfc80b1a2c 100644 --- a/crates/core/component/community-pool/src/component/action_handler/community_pool_deposit.rs +++ b/crates/core/component/community-pool/src/component/action_handler/community_pool_deposit.rs @@ -1,8 +1,6 @@ -use std::sync::Arc; - use anyhow::Result; use async_trait::async_trait; -use cnidarium::{StateRead, StateWrite}; +use cnidarium::StateWrite; use cnidarium_component::ActionHandler; use crate::{component::StateWriteExt as _, CommunityPoolDeposit}; @@ -15,12 +13,7 @@ impl ActionHandler for CommunityPoolDeposit { Ok(()) } - async fn check_stateful(&self, _state: Arc) -> Result<()> { - // Any deposit into the Community Pool is valid. - Ok(()) - } - - async fn execute(&self, mut state: S) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { state.community_pool_deposit(self.value).await } } diff --git a/crates/core/component/community-pool/src/component/action_handler/community_pool_output.rs b/crates/core/component/community-pool/src/component/action_handler/community_pool_output.rs index ee923341f2..e0259bab4b 100644 --- a/crates/core/component/community-pool/src/component/action_handler/community_pool_output.rs +++ b/crates/core/component/community-pool/src/component/action_handler/community_pool_output.rs @@ -1,8 +1,6 @@ -use std::sync::Arc; - use anyhow::Result; use async_trait::async_trait; -use cnidarium::{StateRead, StateWrite}; +use cnidarium::StateWrite; use cnidarium_component::ActionHandler; use penumbra_sct::CommitmentSource; use penumbra_shielded_pool::component::NoteManager; @@ -17,12 +15,7 @@ impl ActionHandler for CommunityPoolOutput { Ok(()) } - async fn check_stateful(&self, _state: Arc) -> Result<()> { - // Any output from the Community Pool is valid (it's just a transparent output). - Ok(()) - } - - async fn execute(&self, mut state: S) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { // Executing a Community Pool output is just minting a note to the recipient of the output. state .mint_note( diff --git a/crates/core/component/community-pool/src/component/action_handler/community_pool_spend.rs b/crates/core/component/community-pool/src/component/action_handler/community_pool_spend.rs index b68baffde1..ca87d5243c 100644 --- a/crates/core/component/community-pool/src/component/action_handler/community_pool_spend.rs +++ b/crates/core/component/community-pool/src/component/action_handler/community_pool_spend.rs @@ -1,8 +1,6 @@ -use std::sync::Arc; - use anyhow::Result; use async_trait::async_trait; -use cnidarium::{StateRead, StateWrite}; +use cnidarium::StateWrite; use cnidarium_component::ActionHandler; use crate::{component::StateWriteExt as _, CommunityPoolSpend}; @@ -16,13 +14,7 @@ impl ActionHandler for CommunityPoolSpend { Ok(()) } - async fn check_stateful(&self, _state: Arc) -> Result<()> { - // Instead of checking here, we just check during execution, which will fail if we try to - // overdraw the Community Pool. - Ok(()) - } - - async fn execute(&self, mut state: S) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { // This will fail if we try to overdraw the Community Pool, so we can never spend more than we have. state.community_pool_withdraw(self.value).await } diff --git a/crates/core/component/dex/src/component/action_handler/position/close.rs b/crates/core/component/dex/src/component/action_handler/position/close.rs index 1bbe66ea88..f6ce7db1fa 100644 --- a/crates/core/component/dex/src/component/action_handler/position/close.rs +++ b/crates/core/component/dex/src/component/action_handler/position/close.rs @@ -1,8 +1,6 @@ -use std::sync::Arc; - use anyhow::Result; use async_trait::async_trait; -use cnidarium::{StateRead, StateWrite}; +use cnidarium::StateWrite; use cnidarium_component::ActionHandler; use penumbra_proto::StateWriteProto as _; @@ -18,11 +16,7 @@ impl ActionHandler for PositionClose { Ok(()) } - async fn check_stateful(&self, _state: Arc) -> Result<()> { - Ok(()) - } - - async fn execute(&self, mut state: S) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { // We don't want to actually close the position here, because otherwise // the economic effects could depend on intra-block ordering, and we'd // lose the ability to do block-scoped JIT liquidity, where a single diff --git a/crates/core/component/dex/src/component/action_handler/position/open.rs b/crates/core/component/dex/src/component/action_handler/position/open.rs index 4f77d88a1e..2474d50290 100644 --- a/crates/core/component/dex/src/component/action_handler/position/open.rs +++ b/crates/core/component/dex/src/component/action_handler/position/open.rs @@ -1,8 +1,6 @@ -use std::sync::Arc; - use anyhow::Result; use async_trait::async_trait; -use cnidarium::{StateRead, StateWrite}; +use cnidarium::StateWrite; use cnidarium_component::ActionHandler; use penumbra_proto::StateWriteProto as _; @@ -32,11 +30,7 @@ impl ActionHandler for PositionOpen { Ok(()) } - async fn check_stateful(&self, _state: Arc) -> Result<()> { - Ok(()) - } - - async fn execute(&self, mut state: S) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { // Validate that the position ID doesn't collide state.check_position_id_unused(&self.position.id()).await?; state.put_position(self.position.clone()).await?; diff --git a/crates/core/component/dex/src/component/action_handler/position/withdraw.rs b/crates/core/component/dex/src/component/action_handler/position/withdraw.rs index 590497e9c7..aed55aa0cd 100644 --- a/crates/core/component/dex/src/component/action_handler/position/withdraw.rs +++ b/crates/core/component/dex/src/component/action_handler/position/withdraw.rs @@ -1,9 +1,7 @@ -use std::sync::Arc; - use anyhow::{anyhow, Result}; use ark_ff::Zero; use async_trait::async_trait; -use cnidarium::{StateRead, StateWrite}; +use cnidarium::StateWrite; use cnidarium_component::ActionHandler; use decaf377::Fr; use penumbra_proto::StateWriteProto; @@ -24,13 +22,7 @@ impl ActionHandler for PositionWithdraw { Ok(()) } - async fn check_stateful(&self, _state: Arc) -> Result<()> { - // Nothing to do here: we defer consistency checks on the reserves to - // execution, to avoid having to reason about parallellism in checks. - Ok(()) - } - - async fn execute(&self, mut state: S) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { // See comment in check_stateful for why we check the position state here: // we need to ensure that we're checking the reserves at the moment we execute // the withdrawal, to prevent any possibility of TOCTOU attacks. diff --git a/crates/core/component/dex/src/component/action_handler/swap.rs b/crates/core/component/dex/src/component/action_handler/swap.rs index 87823df77a..b1dce652dc 100644 --- a/crates/core/component/dex/src/component/action_handler/swap.rs +++ b/crates/core/component/dex/src/component/action_handler/swap.rs @@ -1,8 +1,6 @@ -use std::sync::Arc; - use anyhow::Result; use async_trait::async_trait; -use cnidarium::{StateRead, StateWrite}; +use cnidarium::StateWrite; use cnidarium_component::ActionHandler; use penumbra_proof_params::SWAP_PROOF_VERIFICATION_KEY; use penumbra_proto::StateWriteProto; @@ -35,11 +33,7 @@ impl ActionHandler for Swap { Ok(()) } - async fn check_stateful(&self, _state: Arc) -> Result<()> { - Ok(()) - } - - async fn execute(&self, mut state: S) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { let swap_start = std::time::Instant::now(); let swap = self; diff --git a/crates/core/component/dex/src/component/action_handler/swap_claim.rs b/crates/core/component/dex/src/component/action_handler/swap_claim.rs index 18918240f6..48b35d228c 100644 --- a/crates/core/component/dex/src/component/action_handler/swap_claim.rs +++ b/crates/core/component/dex/src/component/action_handler/swap_claim.rs @@ -42,11 +42,13 @@ impl ActionHandler for SwapClaim { Ok(()) } - async fn check_stateful(&self, state: Arc) -> Result<()> { + async fn check_historical(&self, state: Arc) -> Result<()> { let swap_claim = self; // 1. Validate the epoch duration passed in the swap claim matches // what we know. + // + // SAFETY: this is safe to check here because the epoch duration cannot change during transaction processing. let epoch_duration = state.get_epoch_duration_parameter().await?; let provided_epoch_duration = swap_claim.epoch_duration; if epoch_duration != provided_epoch_duration { @@ -55,6 +57,9 @@ impl ActionHandler for SwapClaim { // 2. The stateful check *must* validate that the clearing // prices used in the proof are valid. + // + // SAFETY: this is safe to check here because the historical batch swap + // output data will not change. let provided_output_height = swap_claim.body.output_data.height; let provided_trading_pair = swap_claim.body.output_data.trading_pair; let output_data = state @@ -68,14 +73,14 @@ impl ActionHandler for SwapClaim { anyhow::bail!("provided output data does not match chain output data"); } + Ok(()) + } + + async fn check_and_execute(&self, mut state: S) -> Result<()> { // 3. Check that the nullifier hasn't been spent before. let spent_nullifier = self.body.nullifier; state.check_nullifier_unspent(spent_nullifier).await?; - Ok(()) - } - - async fn execute(&self, mut state: S) -> Result<()> { // Record the output notes in the state. let source = state .get_current_source() diff --git a/crates/core/component/governance/src/action_handler/delegator_vote.rs b/crates/core/component/governance/src/action_handler/delegator_vote.rs index 61b6e28e97..8f284153ba 100644 --- a/crates/core/component/governance/src/action_handler/delegator_vote.rs +++ b/crates/core/component/governance/src/action_handler/delegator_vote.rs @@ -1,9 +1,7 @@ -use std::sync::Arc; - use anyhow::{Context, Result}; use ark_ff::Zero; use async_trait::async_trait; -use cnidarium::{StateRead, StateWrite}; +use cnidarium::StateWrite; use decaf377::Fr; use penumbra_proof_params::DELEGATOR_VOTE_PROOF_VERIFICATION_KEY; use penumbra_proto::StateWriteProto as _; @@ -55,12 +53,12 @@ impl ActionHandler for DelegatorVote { Ok(()) } - async fn check_stateful(&self, state: Arc) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { let DelegatorVote { body: DelegatorVoteBody { proposal, - vote: _, // All votes are valid, so we don't need to do anything with this + vote, start_position, value, unbonded_amount, @@ -85,24 +83,6 @@ impl ActionHandler for DelegatorVote { .check_unbonded_amount_correct_exchange_for_proposal(*proposal, value, unbonded_amount) .await?; - Ok(()) - } - - async fn execute(&self, mut state: S) -> Result<()> { - let DelegatorVote { - body: - DelegatorVoteBody { - proposal, - vote, - nullifier, - unbonded_amount, - value, - start_position: _, // Not needed to execute: used to check validity of vote - rk: _, // Not needed to execute: used to check auth sig - }, - .. - } = self; - state .mark_nullifier_voted_on_proposal(*proposal, nullifier) .await; diff --git a/crates/core/component/governance/src/action_handler/deposit_claim.rs b/crates/core/component/governance/src/action_handler/deposit_claim.rs index 2fc3cd5887..711de7a8b1 100644 --- a/crates/core/component/governance/src/action_handler/deposit_claim.rs +++ b/crates/core/component/governance/src/action_handler/deposit_claim.rs @@ -1,8 +1,6 @@ -use std::sync::Arc; - use anyhow::Result; use async_trait::async_trait; -use cnidarium::{StateRead, StateWrite}; +use cnidarium::StateWrite; use penumbra_proto::StateWriteProto as _; use penumbra_shielded_pool::component::SupplyWrite; @@ -22,17 +20,14 @@ impl ActionHandler for ProposalDepositClaim { Ok(()) } - async fn check_stateful(&self, state: Arc) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { // Any finished proposal can have its deposit claimed state.check_proposal_claimable(self.proposal).await?; // Check that the deposit amount matches the proposal being claimed state .check_proposal_claim_valid_deposit(self.proposal, self.deposit_amount) .await?; - Ok(()) - } - async fn execute(&self, mut state: S) -> Result<()> { let ProposalDepositClaim { proposal, deposit_amount: _, // not needed to transition state; deposit is self-minted in tx diff --git a/crates/core/component/governance/src/action_handler/validator_vote.rs b/crates/core/component/governance/src/action_handler/validator_vote.rs index 66cca4e9d4..4afe31748d 100644 --- a/crates/core/component/governance/src/action_handler/validator_vote.rs +++ b/crates/core/component/governance/src/action_handler/validator_vote.rs @@ -1,8 +1,6 @@ -use std::sync::Arc; - use anyhow::{Context, Result}; use async_trait::async_trait; -use cnidarium::{StateRead, StateWrite}; +use cnidarium::StateWrite; use penumbra_proto::{DomainType, StateWriteProto as _}; use crate::component::StateWriteExt; @@ -38,17 +36,17 @@ impl ActionHandler for ValidatorVote { Ok(()) } - async fn check_stateful(&self, state: Arc) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { let ValidatorVote { + auth_sig: _, body: ValidatorVoteBody { proposal, - vote: _, // All votes are valid, so we don't need to do anything with this + vote, identity_key, governance_key, - reason: _, // Checked the length in the stateless verification + reason, }, - auth_sig: _, // We already checked this in stateless verification } = self; state.check_proposal_votable(*proposal).await?; @@ -62,22 +60,6 @@ impl ActionHandler for ValidatorVote { .check_governance_key_matches_validator(identity_key, governance_key) .await?; - Ok(()) - } - - async fn execute(&self, mut state: S) -> Result<()> { - let ValidatorVote { - auth_sig: _, - body: - ValidatorVoteBody { - proposal, - vote, - identity_key, - governance_key: _, // This is only used for checks so that stateless verification can be done on the signature - reason, - }, - } = self; - let proposal_state = state .proposal_state(*proposal) .await? diff --git a/crates/core/component/governance/src/action_handler/withdraw.rs b/crates/core/component/governance/src/action_handler/withdraw.rs index 47848d60aa..7ab876ba6e 100644 --- a/crates/core/component/governance/src/action_handler/withdraw.rs +++ b/crates/core/component/governance/src/action_handler/withdraw.rs @@ -1,8 +1,6 @@ -use std::sync::Arc; - use anyhow::Result; use async_trait::async_trait; -use cnidarium::{StateRead, StateWrite}; +use cnidarium::StateWrite; use penumbra_proto::StateWriteProto as _; use penumbra_shielded_pool::component::SupplyWrite; @@ -30,13 +28,10 @@ impl ActionHandler for ProposalWithdraw { Ok(()) } - async fn check_stateful(&self, state: Arc) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { // Any votable proposal can be withdrawn state.check_proposal_votable(self.proposal).await?; - Ok(()) - } - async fn execute(&self, mut state: S) -> Result<()> { let ProposalWithdraw { proposal, reason } = self; // Update the proposal state to withdrawn diff --git a/crates/core/component/shielded-pool/src/component/action_handler/ics20_withdrawal.rs b/crates/core/component/shielded-pool/src/component/action_handler/ics20_withdrawal.rs index 2a1bedfba4..3b83c05084 100644 --- a/crates/core/component/shielded-pool/src/component/action_handler/ics20_withdrawal.rs +++ b/crates/core/component/shielded-pool/src/component/action_handler/ics20_withdrawal.rs @@ -1,8 +1,6 @@ -use std::sync::Arc; - use anyhow::Result; use async_trait::async_trait; -use cnidarium::{StateRead, StateWrite}; +use cnidarium::StateWrite; use cnidarium_component::ActionHandler; use crate::{ @@ -17,11 +15,8 @@ impl ActionHandler for Ics20Withdrawal { self.validate() } - async fn check_stateful(&self, state: Arc) -> Result<()> { - state.withdrawal_check(self).await - } - - async fn execute(&self, mut state: S) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { + state.withdrawal_check(self).await?; state.withdrawal_execute(self).await } } diff --git a/crates/core/component/shielded-pool/src/component/action_handler/output.rs b/crates/core/component/shielded-pool/src/component/action_handler/output.rs index 81559b20e2..330a39c0e0 100644 --- a/crates/core/component/shielded-pool/src/component/action_handler/output.rs +++ b/crates/core/component/shielded-pool/src/component/action_handler/output.rs @@ -1,8 +1,6 @@ -use std::sync::Arc; - use anyhow::Result; use async_trait::async_trait; -use cnidarium::{StateRead, StateWrite}; +use cnidarium::StateWrite; use cnidarium_component::ActionHandler; use penumbra_proof_params::OUTPUT_PROOF_VERIFICATION_KEY; use penumbra_proto::StateWriteProto as _; @@ -27,11 +25,7 @@ impl ActionHandler for Output { Ok(()) } - async fn check_stateful(&self, _state: Arc) -> Result<()> { - Ok(()) - } - - async fn execute(&self, mut state: S) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { let source = state .get_current_source() .expect("source should be set during execution"); diff --git a/crates/core/component/shielded-pool/src/component/action_handler/spend.rs b/crates/core/component/shielded-pool/src/component/action_handler/spend.rs index dd0419f487..ee4c1ace85 100644 --- a/crates/core/component/shielded-pool/src/component/action_handler/spend.rs +++ b/crates/core/component/shielded-pool/src/component/action_handler/spend.rs @@ -1,8 +1,6 @@ -use std::sync::Arc; - use anyhow::{Context, Result}; use async_trait::async_trait; -use cnidarium::{StateRead, StateWrite}; +use cnidarium::StateWrite; use cnidarium_component::ActionHandler; use penumbra_proof_params::SPEND_PROOF_VERIFICATION_KEY; use penumbra_proto::StateWriteProto as _; @@ -41,13 +39,11 @@ impl ActionHandler for Spend { Ok(()) } - async fn check_stateful(&self, state: Arc) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { // Check that the `Nullifier` has not been spent before. let spent_nullifier = self.body.nullifier; - state.check_nullifier_unspent(spent_nullifier).await - } + state.check_nullifier_unspent(spent_nullifier).await?; - async fn execute(&self, mut state: S) -> Result<()> { let source = state.get_current_source().expect("source should be set"); state.nullify(self.body.nullifier, source).await; diff --git a/crates/core/component/stake/src/component/action_handler/delegate.rs b/crates/core/component/stake/src/component/action_handler/delegate.rs index c7a6f44d74..dfae403c18 100644 --- a/crates/core/component/stake/src/component/action_handler/delegate.rs +++ b/crates/core/component/stake/src/component/action_handler/delegate.rs @@ -1,8 +1,6 @@ -use std::sync::Arc; - use anyhow::{ensure, Result}; use async_trait::async_trait; -use cnidarium::{StateRead, StateWrite}; +use cnidarium::StateWrite; use cnidarium_component::ActionHandler; use penumbra_num::Amount; @@ -21,7 +19,11 @@ impl ActionHandler for Delegate { Ok(()) } - async fn check_stateful(&self, state: Arc) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { + // These checks all formerly happened in the `check_historical` method, + // if profiling shows that they cause a bottleneck we could (CAREFULLY) + // move some of them back. + let d = self; let next_rate_data = state .get_validator_rate(&d.validator_identity) @@ -91,10 +93,8 @@ impl ActionHandler for Delegate { ); } - Ok(()) - } + // (end of former check_historical checks) - async fn execute(&self, mut state: S) -> Result<()> { let validator = self.validator_identity; let unbonded_delegation = self.unbonded_amount; // This action is executed in two phases: diff --git a/crates/core/component/stake/src/component/action_handler/undelegate.rs b/crates/core/component/stake/src/component/action_handler/undelegate.rs index aaeed910ec..a885ae6633 100644 --- a/crates/core/component/stake/src/component/action_handler/undelegate.rs +++ b/crates/core/component/stake/src/component/action_handler/undelegate.rs @@ -1,8 +1,6 @@ -use std::sync::Arc; - use anyhow::{ensure, Result}; use async_trait::async_trait; -use cnidarium::{StateRead, StateWrite}; +use cnidarium::StateWrite; use penumbra_shielded_pool::component::SupplyWrite; use crate::{ @@ -18,7 +16,11 @@ impl ActionHandler for Undelegate { Ok(()) } - async fn check_stateful(&self, state: Arc) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { + // These checks all formerly happened in the `check_historical` method, + // if profiling shows that they cause a bottleneck we could (CAREFULLY) + // move some of them back. + let u = self; let rate_data = state .get_validator_rate(&u.validator_identity) @@ -60,10 +62,8 @@ impl ActionHandler for Undelegate { expected_unbonded_amount, ); - Ok(()) - } + // (end of former check_historical impl) - async fn execute(&self, mut state: S) -> Result<()> { tracing::debug!(?self, "queuing undelegation for next epoch"); state.push_undelegation(self.clone()); // Register the undelegation's denom, so clients can look it up later. diff --git a/crates/core/component/stake/src/component/action_handler/undelegate_claim.rs b/crates/core/component/stake/src/component/action_handler/undelegate_claim.rs index 37ea98389c..0a903191d4 100644 --- a/crates/core/component/stake/src/component/action_handler/undelegate_claim.rs +++ b/crates/core/component/stake/src/component/action_handler/undelegate_claim.rs @@ -1,8 +1,6 @@ -use std::sync::Arc; - use anyhow::{ensure, Result}; use async_trait::async_trait; -use cnidarium::{StateRead, StateWrite}; +use cnidarium::StateWrite; use penumbra_proof_params::CONVERT_PROOF_VERIFICATION_KEY; use penumbra_sct::component::clock::EpochRead; @@ -31,7 +29,11 @@ impl ActionHandler for UndelegateClaim { Ok(()) } - async fn check_stateful(&self, state: Arc) -> Result<()> { + async fn check_and_execute(&self, state: S) -> Result<()> { + // These checks all formerly happened in the `check_historical` method, + // if profiling shows that they cause a bottleneck we could (CAREFULLY) + // move some of them back. + // If the validator delegation pool is bonded, or unbonding, check that enough epochs // have elapsed to claim the unbonding tokens: let current_epoch = state.get_current_epoch().await?; @@ -60,10 +62,11 @@ impl ActionHandler for UndelegateClaim { self.body.penalty == expected_penalty, "penalty does not match expected penalty" ); - Ok(()) - } - async fn execute(&self, _state: S) -> Result<()> { + // (end of former check_historical impl) + + // No state changes here - this action just converts one token to another + // TODO: where should we be tracking token supply changes? Ok(()) } diff --git a/crates/core/component/stake/src/component/action_handler/validator_definition.rs b/crates/core/component/stake/src/component/action_handler/validator_definition.rs index b8573217b6..716c0a389b 100644 --- a/crates/core/component/stake/src/component/action_handler/validator_definition.rs +++ b/crates/core/component/stake/src/component/action_handler/validator_definition.rs @@ -1,10 +1,8 @@ use anyhow::{Context, Result}; use async_trait::async_trait; -use cnidarium::{StateRead, StateWrite}; +use cnidarium::StateWrite; use penumbra_sct::component::clock::EpochRead; -use std::sync::Arc; - use penumbra_proto::DomainType; use crate::{ @@ -60,7 +58,11 @@ impl ActionHandler for validator::Definition { Ok(()) } - async fn check_stateful(&self, state: Arc) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { + // These checks all formerly happened in the `check_historical` method, + // if profiling shows that they cause a bottleneck we could (CAREFULLY) + // move some of them back. + let v = self; // Check that the sequence numbers of the updated validators is correct... @@ -103,10 +105,8 @@ impl ActionHandler for validator::Definition { } } - Ok(()) - } + // (end of former check_historical impl) - async fn execute(&self, mut state: S) -> Result<()> { let v = self; let current_epoch = state