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