diff --git a/Cargo.lock b/Cargo.lock index 0270043a9d..9be9b3954f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -791,7 +791,6 @@ dependencies = [ "borsh", "bytes", "cnidarium", - "cnidarium-component", "divan", "futures", "hex", diff --git a/crates/astria-sequencer/Cargo.toml b/crates/astria-sequencer/Cargo.toml index cef3ced64a..d487930dcb 100644 --- a/crates/astria-sequencer/Cargo.toml +++ b/crates/astria-sequencer/Cargo.toml @@ -28,7 +28,6 @@ borsh = { version = "1", features = ["derive"] } cnidarium = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.78.0", features = [ "metrics", ] } -cnidarium-component = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.78.0" } ibc-proto = { version = "0.41.0", features = ["server"] } matchit = "0.7.2" priority-queue = "2.0.2" diff --git a/crates/astria-sequencer/src/accounts/action.rs b/crates/astria-sequencer/src/accounts/action.rs index 8d4853ea68..842283897c 100644 --- a/crates/astria-sequencer/src/accounts/action.rs +++ b/crates/astria-sequencer/src/accounts/action.rs @@ -4,31 +4,125 @@ use anyhow::{ Result, }; use astria_core::{ - primitive::v1::Address, + primitive::v1::ADDRESS_LEN, protocol::transaction::v1alpha1::action::TransferAction, Protobuf, }; -use tracing::instrument; +use cnidarium::{ + StateRead, + StateWrite, +}; +use super::AddressBytes; use crate::{ accounts::{ - self, StateReadExt as _, + StateWriteExt as _, + }, + address::StateReadExt as _, + app::ActionHandler, + assets::{ + StateReadExt as _, + StateWriteExt as _, }, - address, - assets, bridge::StateReadExt as _, - transaction::action_handler::ActionHandler, + transaction::StateReadExt as _, }; -pub(crate) async fn transfer_check_stateful( +#[async_trait::async_trait] +impl ActionHandler for TransferAction { + type CheckStatelessContext = (); + + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { + Ok(()) + } + + async fn check_and_execute(&self, state: S) -> Result<()> { + let from = state + .get_current_source() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); + + ensure!( + state + .get_bridge_account_rollup_id(from) + .await + .context("failed to get bridge account rollup id")? + .is_none(), + "cannot transfer out of bridge account; BridgeUnlock must be used", + ); + + check_transfer(self, from, &state).await?; + execute_transfer(self, from, state).await?; + + Ok(()) + } +} + +pub(crate) async fn execute_transfer( action: &TransferAction, + from: [u8; ADDRESS_LEN], + mut state: S, +) -> anyhow::Result<()> { + let fee = state + .get_transfer_base_fee() + .await + .context("failed to get transfer base fee")?; + state + .get_and_increase_block_fees(&action.fee_asset, fee, TransferAction::full_name()) + .await + .context("failed to add to block fees")?; + + // if fee payment asset is same asset as transfer asset, deduct fee + // from same balance as asset transferred + if action.asset.to_ibc_prefixed() == action.fee_asset.to_ibc_prefixed() { + // check_stateful should have already checked this arithmetic + let payment_amount = action + .amount + .checked_add(fee) + .expect("transfer amount plus fee should not overflow"); + + state + .decrease_balance(from, &action.asset, payment_amount) + .await + .context("failed decreasing `from` account balance")?; + state + .increase_balance(action.to, &action.asset, action.amount) + .await + .context("failed increasing `to` account balance")?; + } else { + // otherwise, just transfer the transfer asset and deduct fee from fee asset balance + // later + state + .decrease_balance(from, &action.asset, action.amount) + .await + .context("failed decreasing `from` account balance")?; + state + .increase_balance(action.to, &action.asset, action.amount) + .await + .context("failed increasing `to` account balance")?; + + // deduct fee from fee asset balance + state + .decrease_balance(from, &action.fee_asset, fee) + .await + .context("failed decreasing `from` account balance for fee payment")?; + } + Ok(()) +} + +pub(crate) async fn check_transfer( + action: &TransferAction, + from: TAddress, state: &S, - from: Address, ) -> Result<()> where - S: accounts::StateReadExt + assets::StateReadExt + 'static, + S: StateRead, + TAddress: AddressBytes, { + state.ensure_base_prefix(&action.to).await.context( + "failed ensuring that the destination address matches the permitted base prefix", + )?; ensure!( state .is_allowed_fee_asset(&action.fee_asset) @@ -44,7 +138,7 @@ where let transfer_asset = action.asset.clone(); let from_fee_balance = state - .get_account_balance(from, &action.fee_asset) + .get_account_balance(&from, &action.fee_asset) .await .context("failed getting `from` account balance for fee payment")?; @@ -80,84 +174,3 @@ where Ok(()) } - -#[async_trait::async_trait] -impl ActionHandler for TransferAction { - async fn check_stateless(&self) -> Result<()> { - Ok(()) - } - - async fn check_stateful(&self, state: &S, from: Address) -> Result<()> - where - S: accounts::StateReadExt + address::StateReadExt + 'static, - { - state.ensure_base_prefix(&self.to).await.context( - "failed ensuring that the destination address matches the permitted base prefix", - )?; - ensure!( - state - .get_bridge_account_rollup_id(&from) - .await - .context("failed to get bridge account rollup id")? - .is_none(), - "cannot transfer out of bridge account; BridgeUnlock must be used", - ); - - transfer_check_stateful(self, state, from) - .await - .context("stateful transfer check failed") - } - - #[instrument(skip_all)] - async fn execute(&self, state: &mut S, from: Address) -> Result<()> - where - S: accounts::StateWriteExt + assets::StateWriteExt, - { - let fee = state - .get_transfer_base_fee() - .await - .context("failed to get transfer base fee")?; - state - .get_and_increase_block_fees(&self.fee_asset, fee, Self::full_name()) - .await - .context("failed to add to block fees")?; - - // if fee payment asset is same asset as transfer asset, deduct fee - // from same balance as asset transferred - if self.asset.to_ibc_prefixed() == self.fee_asset.to_ibc_prefixed() { - // check_stateful should have already checked this arithmetic - let payment_amount = self - .amount - .checked_add(fee) - .expect("transfer amount plus fee should not overflow"); - - state - .decrease_balance(from, &self.asset, payment_amount) - .await - .context("failed decreasing `from` account balance")?; - state - .increase_balance(self.to, &self.asset, self.amount) - .await - .context("failed increasing `to` account balance")?; - } else { - // otherwise, just transfer the transfer asset and deduct fee from fee asset balance - // later - state - .decrease_balance(from, &self.asset, self.amount) - .await - .context("failed decreasing `from` account balance")?; - state - .increase_balance(self.to, &self.asset, self.amount) - .await - .context("failed increasing `to` account balance")?; - - // deduct fee from fee asset balance - state - .decrease_balance(from, &self.fee_asset, fee) - .await - .context("failed decreasing `from` account balance for fee payment")?; - } - - Ok(()) - } -} diff --git a/crates/astria-sequencer/src/accounts/mod.rs b/crates/astria-sequencer/src/accounts/mod.rs index ae0b420808..ddb9fe68a9 100644 --- a/crates/astria-sequencer/src/accounts/mod.rs +++ b/crates/astria-sequencer/src/accounts/mod.rs @@ -3,7 +3,61 @@ pub(crate) mod component; pub(crate) mod query; mod state_ext; +use astria_core::{ + crypto::{ + SigningKey, + VerificationKey, + }, + primitive::v1::{ + Address, + ADDRESS_LEN, + }, + protocol::transaction::v1alpha1::SignedTransaction, +}; pub(crate) use state_ext::{ StateReadExt, StateWriteExt, }; + +pub(crate) trait AddressBytes: Send + Sync { + fn address_bytes(&self) -> [u8; ADDRESS_LEN]; +} + +impl AddressBytes for Address { + fn address_bytes(&self) -> [u8; ADDRESS_LEN] { + self.bytes() + } +} + +impl AddressBytes for [u8; ADDRESS_LEN] { + fn address_bytes(&self) -> [u8; ADDRESS_LEN] { + *self + } +} + +impl AddressBytes for SignedTransaction { + fn address_bytes(&self) -> [u8; ADDRESS_LEN] { + self.address_bytes() + } +} + +impl AddressBytes for SigningKey { + fn address_bytes(&self) -> [u8; ADDRESS_LEN] { + self.address_bytes() + } +} + +impl AddressBytes for VerificationKey { + fn address_bytes(&self) -> [u8; ADDRESS_LEN] { + self.address_bytes() + } +} + +impl<'a, T> AddressBytes for &'a T +where + T: AddressBytes, +{ + fn address_bytes(&self) -> [u8; ADDRESS_LEN] { + (*self).address_bytes() + } +} diff --git a/crates/astria-sequencer/src/accounts/state_ext.rs b/crates/astria-sequencer/src/accounts/state_ext.rs index fa0fe00a6c..e8a60f3d03 100644 --- a/crates/astria-sequencer/src/accounts/state_ext.rs +++ b/crates/astria-sequencer/src/accounts/state_ext.rs @@ -3,14 +3,9 @@ use anyhow::{ Result, }; use astria_core::{ - crypto::{ - SigningKey, - VerificationKey, - }, primitive::v1::{ asset, Address, - ADDRESS_LEN, }, protocol::account::v1alpha1::AssetBalance, }; @@ -26,6 +21,8 @@ use cnidarium::{ use futures::StreamExt; use tracing::instrument; +use super::AddressBytes; + /// Newtype wrapper to read and write a u32 from rocksdb. #[derive(BorshSerialize, BorshDeserialize, Debug)] struct Nonce(u32); @@ -41,57 +38,20 @@ struct Fee(u128); const ACCOUNTS_PREFIX: &str = "accounts"; const TRANSFER_BASE_FEE_STORAGE_KEY: &str = "transferfee"; -trait GetAddressBytes: Send + Sync { - fn get_address_bytes(&self) -> [u8; ADDRESS_LEN]; -} - -impl GetAddressBytes for Address { - fn get_address_bytes(&self) -> [u8; ADDRESS_LEN] { - self.bytes() - } -} - -impl GetAddressBytes for [u8; ADDRESS_LEN] { - fn get_address_bytes(&self) -> [u8; ADDRESS_LEN] { - *self - } -} - -impl GetAddressBytes for SigningKey { - fn get_address_bytes(&self) -> [u8; ADDRESS_LEN] { - self.verification_key().get_address_bytes() - } -} - -impl GetAddressBytes for VerificationKey { - fn get_address_bytes(&self) -> [u8; ADDRESS_LEN] { - self.address_bytes() - } -} - -impl<'a, T> GetAddressBytes for &'a T -where - T: GetAddressBytes, -{ - fn get_address_bytes(&self) -> [u8; ADDRESS_LEN] { - (*self).get_address_bytes() - } -} - struct StorageKey<'a, T>(&'a T); -impl<'a, T: GetAddressBytes> std::fmt::Display for StorageKey<'a, T> { +impl<'a, T: AddressBytes> std::fmt::Display for StorageKey<'a, T> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.write_str(ACCOUNTS_PREFIX)?; f.write_str("/")?; - for byte in self.0.get_address_bytes() { + for byte in self.0.address_bytes() { f.write_fmt(format_args!("{byte:02x}"))?; } Ok(()) } } -fn balance_storage_key>( - address: Address, +fn balance_storage_key>( + address: TAddress, asset: TAsset, ) -> String { format!( @@ -101,7 +61,7 @@ fn balance_storage_key>( ) } -fn nonce_storage_key(address: T) -> String { +fn nonce_storage_key(address: T) -> String { format!("{}/nonce", StorageKey(&address)) } @@ -161,8 +121,13 @@ pub(crate) trait StateReadExt: StateRead + crate::assets::StateReadExt { } #[instrument(skip_all)] - async fn get_account_balance<'a, TAsset>(&self, address: Address, asset: TAsset) -> Result + async fn get_account_balance<'a, TAddress, TAsset>( + &self, + address: TAddress, + asset: TAsset, + ) -> Result where + TAddress: AddressBytes, TAsset: Into + std::fmt::Display + Send, { let Some(bytes) = self @@ -177,7 +142,7 @@ pub(crate) trait StateReadExt: StateRead + crate::assets::StateReadExt { } #[instrument(skip_all)] - async fn get_account_nonce(&self, address: T) -> Result { + async fn get_account_nonce(&self, address: T) -> Result { let bytes = self .get_raw(&nonce_storage_key(address)) .await @@ -211,13 +176,14 @@ impl StateReadExt for T {} #[async_trait] pub(crate) trait StateWriteExt: StateWrite { #[instrument(skip_all)] - fn put_account_balance( + fn put_account_balance( &mut self, - address: Address, + address: TAddress, asset: TAsset, balance: u128, ) -> Result<()> where + TAddress: AddressBytes, TAsset: Into + std::fmt::Display + Send, { let bytes = borsh::to_vec(&Balance(balance)).context("failed to serialize balance")?; @@ -226,29 +192,30 @@ pub(crate) trait StateWriteExt: StateWrite { } #[instrument(skip_all)] - fn put_account_nonce(&mut self, address: Address, nonce: u32) -> Result<()> { + fn put_account_nonce(&mut self, address: T, nonce: u32) -> Result<()> { let bytes = borsh::to_vec(&Nonce(nonce)).context("failed to serialize nonce")?; self.put_raw(nonce_storage_key(address), bytes); Ok(()) } #[instrument(skip_all)] - async fn increase_balance( + async fn increase_balance( &mut self, - address: Address, + address: TAddress, asset: TAsset, amount: u128, ) -> Result<()> where + TAddress: AddressBytes, TAsset: Into + std::fmt::Display + Send, { let asset = asset.into(); let balance = self - .get_account_balance(address, asset) + .get_account_balance(&address, asset) .await .context("failed to get account balance")?; self.put_account_balance( - address, + &address, asset, balance .checked_add(amount) @@ -259,22 +226,23 @@ pub(crate) trait StateWriteExt: StateWrite { } #[instrument(skip_all)] - async fn decrease_balance( + async fn decrease_balance( &mut self, - address: Address, + address: TAddress, asset: TAsset, amount: u128, ) -> Result<()> where + TAddress: AddressBytes, TAsset: Into + std::fmt::Display + Send, { let asset = asset.into(); let balance = self - .get_account_balance(address, asset) + .get_account_balance(&address, asset) .await .context("failed to get account balance")?; self.put_account_balance( - address, + &address, asset, balance .checked_sub(amount) diff --git a/crates/astria-sequencer/src/app/action_handler.rs b/crates/astria-sequencer/src/app/action_handler.rs new file mode 100644 index 0000000000..73af0cefee --- /dev/null +++ b/crates/astria-sequencer/src/app/action_handler.rs @@ -0,0 +1,22 @@ +use cnidarium::StateWrite; + +/// This trait is a verbatim copy of [`cnidarium_component::ActionHandler`]. +/// +/// It's duplicated here because all actions are foreign types, forbidding +/// the the implementation of [`cnidarium_component::ActionHandler`] for these +/// types due to Rust orphan rules. +#[async_trait::async_trait] +pub(crate) trait ActionHandler { + type CheckStatelessContext: Clone + Send + Sync + 'static; + async fn check_stateless(&self, context: Self::CheckStatelessContext) -> anyhow::Result<()>; + + // Commenting out for the time being as CI flags this as not being used. Leaving this in + // for reference as this is copied from cnidarium_component. + // ``` + // async fn check_historical(&self, _state: Arc) -> anyhow::Result<()> { + // Ok(()) + // } + // ``` + + async fn check_and_execute(&self, mut state: S) -> anyhow::Result<()>; +} diff --git a/crates/astria-sequencer/src/app/mod.rs b/crates/astria-sequencer/src/app/mod.rs index cc59071f13..525667a14b 100644 --- a/crates/astria-sequencer/src/app/mod.rs +++ b/crates/astria-sequencer/src/app/mod.rs @@ -7,11 +7,13 @@ mod tests_breaking_changes; #[cfg(test)] mod tests_execute_transaction; +mod action_handler; use std::{ collections::VecDeque, sync::Arc, }; +pub(crate) use action_handler::ActionHandler; use anyhow::{ anyhow, ensure, @@ -19,7 +21,6 @@ use anyhow::{ }; use astria_core::{ generated::protocol::transaction::v1alpha1 as raw, - primitive::v1::Address, protocol::{ abci::AbciErrorCode, transaction::v1alpha1::{ @@ -37,7 +38,10 @@ use cnidarium::{ StateDelta, Storage, }; -use prost::Message as _; +use prost::{ + Message as _, + Name as _, +}; use sha2::{ Digest as _, Sha256, @@ -105,15 +109,21 @@ use crate::{ StateReadExt as _, StateWriteExt as _, }, - transaction::{ - self, - InvalidNonce, - }, + transaction::InvalidNonce, }; /// The inter-block state being written to by the application. type InterBlockState = Arc>; +/// The maximum permitted size of an encoded [`raw::SignedTransaction`]. +const MAX_TX_SIZE: usize = 256_000; // 256 KB + +#[derive(Debug, thiserror::Error)] +#[error("transaction size too large; allowed {MAX_TX_SIZE} bytes, got {actual}")] +pub(crate) struct TransactionTooLarge { + actual: usize, +} + /// The Sequencer application, written as a bundle of [`Component`]s. /// /// Note: this is called `App` because this is a Tendermint ABCI application, @@ -535,7 +545,7 @@ impl App { } // execute tx and store in `execution_results` list on success - match self.execute_transaction(tx.clone()).await { + match self.deliver_tx(tx.clone()).await { Ok(events) => { execution_results.push(ExecTxResult { events, @@ -550,16 +560,16 @@ impl App { validated_txs.push(bytes.into()); included_signed_txs.push((*tx).clone()); } - Err(e) => { + Err(err) => { self.metrics .increment_prepare_proposal_excluded_transactions_failed_execution(); debug!( transaction_hash = %tx_hash_base64, - error = AsRef::::as_ref(&e), + error = AsRef::::as_ref(&err), "failed to execute transaction, not including in block" ); - if e.downcast_ref::().is_some() { + if err.downcast_ref::().is_some() { // we re-insert the tx into the mempool if it failed to execute // due to an invalid nonce, as it may be valid in the future. // if it's invalid due to the nonce being too low, it'll be @@ -572,7 +582,9 @@ impl App { self.mempool .track_removal_comet_bft( enqueued_tx.tx_hash(), - RemovalReason::FailedPrepareProposal(e.to_string()), + RemovalReason::FailedPrepareProposal { + source: err, + }, ) .await; } @@ -651,7 +663,7 @@ impl App { } // execute tx and store in `execution_results` list on success - match self.execute_transaction(Arc::new(tx.clone())).await { + match self.deliver_tx(Arc::new(tx.clone())).await { Ok(events) => { execution_results.push(ExecTxResult { events, @@ -806,16 +818,13 @@ impl App { .context("failed to execute block")?; // skip the first two transactions, as they are the rollup data commitments - for tx in finalize_block.txs.iter().skip(2) { + for bytes in finalize_block.txs.iter().skip(2) { // remove any included txs from the mempool - let tx_hash = Sha256::digest(tx).into(); + let tx_hash = Sha256::digest(bytes).into(); self.mempool.remove(tx_hash).await; - let signed_tx = signed_transaction_from_bytes(tx) - .context("protocol error; only valid txs should be finalized")?; - - match self.execute_transaction(Arc::new(signed_tx)).await { - Ok(events) => tx_results.push(ExecTxResult { + match self.deliver_tx_bytes(bytes).await { + Ok((_, events)) => tx_results.push(ExecTxResult { events, ..Default::default() }), @@ -975,31 +984,50 @@ impl App { Ok(self.apply(state_tx)) } + /// Wrapper around [`Self::execute_transaction`] to deserialize from bytes. + #[instrument(name = "App::deliver_tx", skip_all)] + pub(crate) async fn deliver_tx_bytes( + &mut self, + bytes: &[u8], + ) -> anyhow::Result<(Arc, Vec)> { + ensure!( + bytes.len() <= MAX_TX_SIZE, + TransactionTooLarge { + actual: bytes.len(), + } + ); + + let tx = raw::SignedTransaction::decode(bytes) + .with_context(|| { + format!( + "failed decoding bytes as `{}`", + raw::SignedTransaction::full_name() + ) + }) + .and_then(|proto| { + SignedTransaction::try_from_raw(proto).context("transaction contains invalid data") + })?; + let tx = Arc::new(tx); + // Not providing context because this is inside a wrapper and the errors should be returned + // transparently. + let events = self.deliver_tx(tx.clone()).await?; + Ok((tx, events)) + } + /// Executes a signed transaction. - #[instrument(name = "App::execute_transaction", skip_all)] - pub(crate) async fn execute_transaction( + async fn deliver_tx( &mut self, signed_tx: Arc, ) -> anyhow::Result> { let signed_tx_2 = signed_tx.clone(); - let stateless = tokio::spawn( - async move { transaction::check_stateless(&signed_tx_2).await }.in_current_span(), - ); - let signed_tx_2 = signed_tx.clone(); - let state2 = self.state.clone(); - let stateful = tokio::spawn( - async move { transaction::check_stateful(&signed_tx_2, &state2).await } - .in_current_span(), - ); + let stateless = + tokio::spawn(async move { signed_tx_2.check_stateless(()).await }.in_current_span()); stateless .await .context("stateless check task aborted while executing")? .context("stateless check failed")?; - stateful - .await - .context("stateful check task aborted while executing")? - .context("stateful check failed")?; + // At this point, the stateful checks should have completed, // leaving us with exclusive access to the Arc. let mut state_tx = self @@ -1007,20 +1035,19 @@ impl App { .try_begin_transaction() .expect("state Arc should be present and unique"); - transaction::execute(&signed_tx, &mut state_tx) + signed_tx + .check_and_execute(&mut state_tx) .await .context("failed executing transaction")?; - let (_, events) = state_tx.apply(); - info!(event_count = events.len(), "executed transaction"); - Ok(events) + Ok(state_tx.apply().1) } #[instrument(name = "App::end_block", skip_all)] pub(crate) async fn end_block( &mut self, height: u64, - fee_recipient: Address, + fee_recipient: [u8; 20], ) -> anyhow::Result { let state_tx = StateDelta::new(self.state.clone()); let mut arc_state_tx = Arc::new(state_tx); diff --git a/crates/astria-sequencer/src/app/test_utils.rs b/crates/astria-sequencer/src/app/test_utils.rs index 6ef60fac9c..0ba5e71a26 100644 --- a/crates/astria-sequencer/src/app/test_utils.rs +++ b/crates/astria-sequencer/src/app/test_utils.rs @@ -25,7 +25,10 @@ use crate::{ app::App, mempool::Mempool, metrics::Metrics, - test_utils::astria_address_from_hex_string, + test_utils::{ + astria_address_from_hex_string, + nria, + }, }; pub(crate) const ALICE_ADDRESS: &str = "1c0c490f1b5528d8173c5de46d131160e4b2c0c3"; @@ -147,7 +150,7 @@ pub(crate) fn get_mock_tx(nonce: u32) -> SignedTransaction { SequenceAction { rollup_id: RollupId::from_unhashed_bytes([0; 32]), data: vec![0x99], - fee_asset: "astria".parse().unwrap(), + fee_asset: nria().into(), } .into(), ], diff --git a/crates/astria-sequencer/src/app/tests_app.rs b/crates/astria-sequencer/src/app/tests_app.rs index 762a27ba1c..10e92faf4f 100644 --- a/crates/astria-sequencer/src/app/tests_app.rs +++ b/crates/astria-sequencer/src/app/tests_app.rs @@ -292,9 +292,9 @@ async fn app_create_sequencer_block_with_sequenced_data_and_deposits() { let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); let mut state_tx = StateDelta::new(app.state.clone()); - state_tx.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); state_tx - .put_bridge_account_ibc_asset(&bridge_address, nria()) + .put_bridge_account_ibc_asset(bridge_address, nria()) .unwrap(); app.apply(state_tx); app.prepare_commit(storage.clone()).await.unwrap(); @@ -382,9 +382,9 @@ async fn app_execution_results_match_proposal_vs_after_proposal() { let asset = nria().clone(); let mut state_tx = StateDelta::new(app.state.clone()); - state_tx.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); state_tx - .put_bridge_account_ibc_asset(&bridge_address, &asset) + .put_bridge_account_ibc_asset(bridge_address, &asset) .unwrap(); app.apply(state_tx); app.prepare_commit(storage.clone()).await.unwrap(); @@ -411,7 +411,7 @@ async fn app_execution_results_match_proposal_vs_after_proposal() { actions: vec![lock_action.into(), sequence_action.into()], }; - let signed_tx = tx.into_signed(&alice); + let signed_tx = Arc::new(tx.into_signed(&alice)); let expected_deposit = Deposit::new( bridge_address, @@ -421,7 +421,7 @@ async fn app_execution_results_match_proposal_vs_after_proposal() { "nootwashere".to_string(), ); let deposits = HashMap::from_iter(vec![(rollup_id, vec![expected_deposit.clone()])]); - let commitments = generate_rollup_datas_commitment(&[signed_tx.clone()], deposits.clone()); + let commitments = generate_rollup_datas_commitment(&[(*signed_tx).clone()], deposits.clone()); let timestamp = Time::now(); let block_hash = Hash::try_from([99u8; 32].to_vec()).unwrap(); @@ -558,8 +558,8 @@ async fn app_prepare_proposal_cometbft_max_bytes_overflow_ok() { } .into_signed(&alice); - app.mempool.insert(tx_pass, 0).await.unwrap(); - app.mempool.insert(tx_overflow, 0).await.unwrap(); + app.mempool.insert(Arc::new(tx_pass), 0).await.unwrap(); + app.mempool.insert(Arc::new(tx_overflow), 0).await.unwrap(); // send to prepare_proposal let prepare_args = abci::request::PrepareProposal { @@ -631,8 +631,8 @@ async fn app_prepare_proposal_sequencer_max_bytes_overflow_ok() { } .into_signed(&alice); - app.mempool.insert(tx_pass, 0).await.unwrap(); - app.mempool.insert(tx_overflow, 0).await.unwrap(); + app.mempool.insert(Arc::new(tx_pass), 0).await.unwrap(); + app.mempool.insert(Arc::new(tx_overflow), 0).await.unwrap(); // send to prepare_proposal let prepare_args = abci::request::PrepareProposal { @@ -679,7 +679,7 @@ async fn app_end_block_validator_updates() { ]; let mut app = initialize_app(None, initial_validator_set).await; - let proposer_address = astria_address(&[0u8; 20]); + let proposer_address = [0u8; 20]; let validator_updates = vec![ ValidatorUpdate { diff --git a/crates/astria-sequencer/src/app/tests_breaking_changes.rs b/crates/astria-sequencer/src/app/tests_breaking_changes.rs index b7c11a8bf1..0523ee43b0 100644 --- a/crates/astria-sequencer/src/app/tests_breaking_changes.rs +++ b/crates/astria-sequencer/src/app/tests_breaking_changes.rs @@ -104,9 +104,9 @@ async fn app_finalize_block_snapshot() { let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); let mut state_tx = StateDelta::new(app.state.clone()); - state_tx.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); state_tx - .put_bridge_account_ibc_asset(&bridge_address, nria()) + .put_bridge_account_ibc_asset(bridge_address, nria()) .unwrap(); app.apply(state_tx); @@ -246,7 +246,7 @@ async fn app_execute_transaction_with_every_action_snapshot() { }; let signed_tx = Arc::new(tx.into_signed(&alice)); - app.execute_transaction(signed_tx).await.unwrap(); + app.deliver_tx(signed_tx).await.unwrap(); let tx = UnsignedTransaction { params: TransactionParams::builder() @@ -265,7 +265,7 @@ async fn app_execute_transaction_with_every_action_snapshot() { ], }; let signed_tx = Arc::new(tx.into_signed(&bridge)); - app.execute_transaction(signed_tx).await.unwrap(); + app.deliver_tx(signed_tx).await.unwrap(); let tx = UnsignedTransaction { params: TransactionParams::builder() @@ -300,7 +300,7 @@ async fn app_execute_transaction_with_every_action_snapshot() { }; let signed_tx = Arc::new(tx.into_signed(&bridge)); - app.execute_transaction(signed_tx).await.unwrap(); + app.deliver_tx(signed_tx).await.unwrap(); app.prepare_commit(storage.clone()).await.unwrap(); app.commit(storage.clone()).await; diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index f0c83ddfa6..28a508a4d5 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -33,7 +33,10 @@ use tendermint::abci::EventAttributeIndexExt as _; use crate::{ accounts::StateReadExt as _, - app::test_utils::*, + app::{ + test_utils::*, + ActionHandler as _, + }, assets::StateReadExt as _, authority::StateReadExt as _, bridge::{ @@ -106,7 +109,7 @@ async fn app_execute_transaction_transfer() { }; let signed_tx = Arc::new(tx.into_signed(&alice)); - app.execute_transaction(signed_tx).await.unwrap(); + app.deliver_tx(signed_tx).await.unwrap(); assert_eq!( app.state @@ -163,7 +166,7 @@ async fn app_execute_transaction_transfer_not_native_token() { }; let signed_tx = Arc::new(tx.into_signed(&alice)); - app.execute_transaction(signed_tx).await.unwrap(); + app.deliver_tx(signed_tx).await.unwrap(); assert_eq!( app.state @@ -229,7 +232,7 @@ async fn app_execute_transaction_transfer_balance_too_low_for_fee() { let signed_tx = Arc::new(tx.into_signed(&keypair)); let res = app - .execute_transaction(signed_tx) + .deliver_tx(signed_tx) .await .unwrap_err() .root_cause() @@ -268,7 +271,7 @@ async fn app_execute_transaction_sequence() { }; let signed_tx = Arc::new(tx.into_signed(&alice)); - app.execute_transaction(signed_tx).await.unwrap(); + app.deliver_tx(signed_tx).await.unwrap(); assert_eq!(app.state.get_account_nonce(alice_address).await.unwrap(), 1); assert_eq!( @@ -303,7 +306,7 @@ async fn app_execute_transaction_invalid_fee_asset() { }; let signed_tx = Arc::new(tx.into_signed(&alice)); - assert!(app.execute_transaction(signed_tx).await.is_err()); + assert!(app.deliver_tx(signed_tx).await.is_err()); } #[tokio::test] @@ -327,7 +330,7 @@ async fn app_execute_transaction_validator_update() { }; let signed_tx = Arc::new(tx.into_signed(&alice)); - app.execute_transaction(signed_tx).await.unwrap(); + app.deliver_tx(signed_tx).await.unwrap(); assert_eq!(app.state.get_account_nonce(alice_address).await.unwrap(), 1); let validator_updates = app.state.get_validator_updates().await.unwrap(); @@ -354,9 +357,9 @@ async fn app_execute_transaction_ibc_relayer_change_addition() { }; let signed_tx = Arc::new(tx.into_signed(&alice)); - app.execute_transaction(signed_tx).await.unwrap(); + app.deliver_tx(signed_tx).await.unwrap(); assert_eq!(app.state.get_account_nonce(alice_address).await.unwrap(), 1); - assert!(app.state.is_ibc_relayer(&alice_address).await.unwrap()); + assert!(app.state.is_ibc_relayer(alice_address).await.unwrap()); } #[tokio::test] @@ -381,9 +384,9 @@ async fn app_execute_transaction_ibc_relayer_change_deletion() { }; let signed_tx = Arc::new(tx.into_signed(&alice)); - app.execute_transaction(signed_tx).await.unwrap(); + app.deliver_tx(signed_tx).await.unwrap(); assert_eq!(app.state.get_account_nonce(alice_address).await.unwrap(), 1); - assert!(!app.state.is_ibc_relayer(&alice_address).await.unwrap()); + assert!(!app.state.is_ibc_relayer(alice_address).await.unwrap()); } #[tokio::test] @@ -408,7 +411,7 @@ async fn app_execute_transaction_ibc_relayer_change_invalid() { }; let signed_tx = Arc::new(tx.into_signed(&alice)); - assert!(app.execute_transaction(signed_tx).await.is_err()); + assert!(app.deliver_tx(signed_tx).await.is_err()); } #[tokio::test] @@ -431,11 +434,11 @@ async fn app_execute_transaction_sudo_address_change() { }; let signed_tx = Arc::new(tx.into_signed(&alice)); - app.execute_transaction(signed_tx).await.unwrap(); + app.deliver_tx(signed_tx).await.unwrap(); assert_eq!(app.state.get_account_nonce(alice_address).await.unwrap(), 1); let sudo_address = app.state.get_sudo_address().await.unwrap(); - assert_eq!(sudo_address, new_address); + assert_eq!(sudo_address, new_address.bytes()); } #[tokio::test] @@ -465,7 +468,7 @@ async fn app_execute_transaction_sudo_address_change_error() { let signed_tx = Arc::new(tx.into_signed(&alice)); let res = app - .execute_transaction(signed_tx) + .deliver_tx(signed_tx) .await .unwrap_err() .root_cause() @@ -493,7 +496,7 @@ async fn app_execute_transaction_fee_asset_change_addition() { }; let signed_tx = Arc::new(tx.into_signed(&alice)); - app.execute_transaction(signed_tx).await.unwrap(); + app.deliver_tx(signed_tx).await.unwrap(); assert_eq!(app.state.get_account_nonce(alice_address).await.unwrap(), 1); assert!(app.state.is_allowed_fee_asset(&test_asset()).await.unwrap()); @@ -525,7 +528,7 @@ async fn app_execute_transaction_fee_asset_change_removal() { }; let signed_tx = Arc::new(tx.into_signed(&alice)); - app.execute_transaction(signed_tx).await.unwrap(); + app.deliver_tx(signed_tx).await.unwrap(); assert_eq!(app.state.get_account_nonce(alice_address).await.unwrap(), 1); assert!(!app.state.is_allowed_fee_asset(test_asset()).await.unwrap()); @@ -551,7 +554,7 @@ async fn app_execute_transaction_fee_asset_change_invalid() { let signed_tx = Arc::new(tx.into_signed(&alice)); let res = app - .execute_transaction(signed_tx) + .deliver_tx(signed_tx) .await .unwrap_err() .root_cause() @@ -595,11 +598,11 @@ async fn app_execute_transaction_init_bridge_account_ok() { .get_account_balance(alice_address, nria()) .await .unwrap(); - app.execute_transaction(signed_tx).await.unwrap(); + app.deliver_tx(signed_tx).await.unwrap(); assert_eq!(app.state.get_account_nonce(alice_address).await.unwrap(), 1); assert_eq!( app.state - .get_bridge_account_rollup_id(&alice_address) + .get_bridge_account_rollup_id(alice_address) .await .unwrap() .unwrap(), @@ -607,7 +610,7 @@ async fn app_execute_transaction_init_bridge_account_ok() { ); assert_eq!( app.state - .get_bridge_account_ibc_asset(&alice_address) + .get_bridge_account_ibc_asset(alice_address) .await .unwrap(), nria().to_ibc_prefixed(), @@ -646,7 +649,7 @@ async fn app_execute_transaction_init_bridge_account_account_already_registered( }; let signed_tx = Arc::new(tx.into_signed(&alice)); - app.execute_transaction(signed_tx).await.unwrap(); + app.deliver_tx(signed_tx).await.unwrap(); let action = InitBridgeAccountAction { rollup_id, @@ -664,7 +667,7 @@ async fn app_execute_transaction_init_bridge_account_account_already_registered( }; let signed_tx = Arc::new(tx.into_signed(&alice)); - assert!(app.execute_transaction(signed_tx).await.is_err()); + assert!(app.deliver_tx(signed_tx).await.is_err()); } #[tokio::test] @@ -677,9 +680,9 @@ async fn app_execute_transaction_bridge_lock_action_ok() { let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); let mut state_tx = StateDelta::new(app.state.clone()); - state_tx.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); state_tx - .put_bridge_account_ibc_asset(&bridge_address, nria()) + .put_bridge_account_ibc_asset(bridge_address, nria()) .unwrap(); app.apply(state_tx); @@ -712,7 +715,7 @@ async fn app_execute_transaction_bridge_lock_action_ok() { .await .unwrap(); - app.execute_transaction(signed_tx).await.unwrap(); + app.deliver_tx(signed_tx).await.unwrap(); assert_eq!(app.state.get_account_nonce(alice_address).await.unwrap(), 1); let transfer_fee = app.state.get_transfer_base_fee().await.unwrap(); let expected_deposit = Deposit::new( @@ -777,7 +780,7 @@ async fn app_execute_transaction_bridge_lock_action_invalid_for_eoa() { }; let signed_tx = Arc::new(tx.into_signed(&alice)); - assert!(app.execute_transaction(signed_tx).await.is_err()); + assert!(app.deliver_tx(signed_tx).await.is_err()); } #[tokio::test] @@ -805,7 +808,7 @@ async fn app_execute_transaction_invalid_nonce() { }; let signed_tx = Arc::new(tx.into_signed(&alice)); - let response = app.execute_transaction(signed_tx).await; + let response = app.deliver_tx(signed_tx).await; // check that tx was not executed by checking nonce and balance are unchanged assert_eq!(app.state.get_account_nonce(alice_address).await.unwrap(), 0); @@ -821,9 +824,11 @@ async fn app_execute_transaction_invalid_nonce() { response .unwrap_err() .downcast_ref::() - .map(|nonce_err| nonce_err.0) .unwrap(), - 1 + &InvalidNonce { + current: 0, + in_transaction: 1 + }, ); } @@ -852,7 +857,7 @@ async fn app_execute_transaction_invalid_chain_id() { }; let signed_tx = Arc::new(tx.into_signed(&alice)); - let response = app.execute_transaction(signed_tx).await; + let response = app.deliver_tx(signed_tx).await; // check that tx was not executed by checking nonce and balance are unchanged assert_eq!(app.state.get_account_nonce(alice_address).await.unwrap(), 0); @@ -878,8 +883,6 @@ async fn app_execute_transaction_invalid_chain_id() { async fn app_stateful_check_fails_insufficient_total_balance() { use rand::rngs::OsRng; - use crate::transaction; - let mut app = initialize_app(None, vec![]).await; let alice = get_alice_signing_key(); @@ -913,7 +916,7 @@ async fn app_stateful_check_fails_insufficient_total_balance() { .into_signed(&alice); // make transfer - app.execute_transaction(Arc::new(signed_tx)).await.unwrap(); + app.deliver_tx(Arc::new(signed_tx)).await.unwrap(); // build double transfer exceeding balance let signed_tx_fail = UnsignedTransaction { @@ -939,7 +942,8 @@ async fn app_stateful_check_fails_insufficient_total_balance() { .into_signed(&keypair); // try double, see fails stateful check - let res = transaction::check_stateful(&signed_tx_fail, &app.state) + let res = signed_tx_fail + .check_and_execute(Arc::get_mut(&mut app.state).unwrap()) .await .unwrap_err() .root_cause() @@ -963,7 +967,8 @@ async fn app_stateful_check_fails_insufficient_total_balance() { } .into_signed(&keypair); - transaction::check_stateful(&signed_tx_pass, &app.state) + signed_tx_pass + .check_and_execute(Arc::get_mut(&mut app.state).unwrap()) .await .expect("stateful check should pass since we transferred enough to cover fee"); } @@ -990,11 +995,11 @@ async fn app_execute_transaction_bridge_lock_unlock_action_ok() { .unwrap(); // create bridge account - state_tx.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); state_tx - .put_bridge_account_ibc_asset(&bridge_address, nria()) + .put_bridge_account_ibc_asset(bridge_address, nria()) .unwrap(); - state_tx.put_bridge_account_withdrawer_address(&bridge_address, &bridge_address); + state_tx.put_bridge_account_withdrawer_address(bridge_address, bridge_address); app.apply(state_tx); let amount = 100; @@ -1015,7 +1020,7 @@ async fn app_execute_transaction_bridge_lock_unlock_action_ok() { let signed_tx = Arc::new(tx.into_signed(&alice)); - app.execute_transaction(signed_tx).await.unwrap(); + app.deliver_tx(signed_tx).await.unwrap(); assert_eq!(app.state.get_account_nonce(alice_address).await.unwrap(), 1); // see can unlock through bridge unlock @@ -1036,7 +1041,7 @@ async fn app_execute_transaction_bridge_lock_unlock_action_ok() { }; let signed_tx = Arc::new(tx.into_signed(&bridge)); - app.execute_transaction(signed_tx) + app.deliver_tx(signed_tx) .await .expect("executing bridge unlock action should succeed"); assert_eq!( @@ -1075,7 +1080,7 @@ async fn transaction_execution_records_fee_event() { let signed_tx = Arc::new(tx.into_signed(&alice)); - let events = app.execute_transaction(signed_tx).await.unwrap(); + let events = app.deliver_tx(signed_tx).await.unwrap(); let transfer_fee = app.state.get_transfer_base_fee().await.unwrap(); let event = events.first().unwrap(); assert_eq!(event.kind, "tx.fees"); diff --git a/crates/astria-sequencer/src/authority/action.rs b/crates/astria-sequencer/src/authority/action.rs index baf6220db9..d8431570d1 100644 --- a/crates/astria-sequencer/src/authority/action.rs +++ b/crates/astria-sequencer/src/authority/action.rs @@ -4,30 +4,41 @@ use anyhow::{ Context as _, Result, }; -use astria_core::{ - primitive::v1::Address, - protocol::transaction::v1alpha1::action::{ - FeeChange, - FeeChangeAction, - SudoAddressChangeAction, - ValidatorUpdate, - }, +use astria_core::protocol::transaction::v1alpha1::action::{ + FeeChange, + FeeChangeAction, + SudoAddressChangeAction, + ValidatorUpdate, }; -use tracing::instrument; +use cnidarium::StateWrite; use crate::{ - address, - authority, - transaction::action_handler::ActionHandler, + accounts::StateWriteExt as _, + address::StateReadExt as _, + app::ActionHandler, + authority::{ + StateReadExt as _, + StateWriteExt as _, + }, + bridge::StateWriteExt as _, + ibc::StateWriteExt as _, + sequence::StateWriteExt as _, + transaction::StateReadExt as _, }; #[async_trait::async_trait] impl ActionHandler for ValidatorUpdate { - async fn check_stateful( - &self, - state: &S, - from: Address, - ) -> Result<()> { + type CheckStatelessContext = (); + + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { + Ok(()) + } + + async fn check_and_execute(&self, mut state: S) -> Result<()> { + let from = state + .get_current_source() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); // ensure signer is the valid `sudo` key in state let sudo_address = state .get_sudo_address() @@ -52,15 +63,7 @@ impl ActionHandler for ValidatorUpdate { // check that this is not the only validator, cannot remove the last one ensure!(validator_set.len() != 1, "cannot remove the last validator"); } - Ok(()) - } - #[instrument(skip_all)] - async fn execute( - &self, - state: &mut S, - _: Address, - ) -> Result<()> { // add validator update in non-consensus state to be used in end_block let mut validator_updates = state .get_validator_updates() @@ -76,17 +79,19 @@ impl ActionHandler for ValidatorUpdate { #[async_trait::async_trait] impl ActionHandler for SudoAddressChangeAction { - async fn check_stateless(&self) -> Result<()> { + type CheckStatelessContext = (); + + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { Ok(()) } /// check that the signer of the transaction is the current sudo address, /// as only that address can change the sudo address - async fn check_stateful( - &self, - state: &S, - from: Address, - ) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { + let from = state + .get_current_source() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); state .ensure_base_prefix(&self.new_address) .await @@ -97,11 +102,6 @@ impl ActionHandler for SudoAddressChangeAction { .await .context("failed to get sudo address from state")?; ensure!(sudo_address == from, "signer is not the sudo key"); - Ok(()) - } - - #[instrument(skip_all)] - async fn execute(&self, state: &mut S, _: Address) -> Result<()> { state .put_sudo_address(self.new_address) .context("failed to put sudo address in state")?; @@ -111,30 +111,25 @@ impl ActionHandler for SudoAddressChangeAction { #[async_trait::async_trait] impl ActionHandler for FeeChangeAction { + type CheckStatelessContext = (); + + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { + Ok(()) + } + /// check that the signer of the transaction is the current sudo address, /// as only that address can change the fee - async fn check_stateful( - &self, - state: &S, - from: Address, - ) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { + let from = state + .get_current_source() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); // ensure signer is the valid `sudo` key in state let sudo_address = state .get_sudo_address() .await .context("failed to get sudo address from state")?; ensure!(sudo_address == from, "signer is not the sudo key"); - Ok(()) - } - - #[instrument(skip_all)] - async fn execute(&self, state: &mut S, _: Address) -> Result<()> { - use crate::{ - accounts::StateWriteExt as _, - bridge::StateWriteExt as _, - ibc::StateWriteExt as _, - sequence::StateWriteExt as _, - }; match self.fee_change { FeeChange::TransferBaseFee => { @@ -172,31 +167,28 @@ mod test { use super::*; use crate::{ - accounts::{ - StateReadExt as _, + accounts::StateReadExt as _, + bridge::StateReadExt as _, + ibc::StateReadExt as _, + sequence::StateReadExt as _, + transaction::{ StateWriteExt as _, + TransactionContext, }, - bridge::{ - StateReadExt as _, - StateWriteExt as _, - }, - ibc::{ - StateReadExt as _, - StateWriteExt as _, - }, - sequence::{ - StateReadExt as _, - StateWriteExt as _, - }, - test_utils::astria_address, }; #[tokio::test] - async fn fee_change_action_execute() { + async fn fee_change_action_executes() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); let transfer_fee = 12; + + state.put_current_source(TransactionContext { + address_bytes: [1; 20], + }); + state.put_sudo_address([1; 20]).unwrap(); + state.put_transfer_base_fee(transfer_fee).unwrap(); let fee_change = FeeChangeAction { @@ -204,10 +196,7 @@ mod test { new_value: 10, }; - fee_change - .execute(&mut state, astria_address(&[1; 20])) - .await - .unwrap(); + fee_change.check_and_execute(&mut state).await.unwrap(); assert_eq!(state.get_transfer_base_fee().await.unwrap(), 10); let sequence_base_fee = 5; @@ -218,10 +207,7 @@ mod test { new_value: 3, }; - fee_change - .execute(&mut state, astria_address(&[1; 20])) - .await - .unwrap(); + fee_change.check_and_execute(&mut state).await.unwrap(); assert_eq!(state.get_sequence_action_base_fee().await.unwrap(), 3); let sequence_byte_cost_multiplier = 2; @@ -232,10 +218,7 @@ mod test { new_value: 4, }; - fee_change - .execute(&mut state, astria_address(&[1; 20])) - .await - .unwrap(); + fee_change.check_and_execute(&mut state).await.unwrap(); assert_eq!( state .get_sequence_action_byte_cost_multiplier() @@ -252,10 +235,7 @@ mod test { new_value: 2, }; - fee_change - .execute(&mut state, astria_address(&[1; 20])) - .await - .unwrap(); + fee_change.check_and_execute(&mut state).await.unwrap(); assert_eq!(state.get_init_bridge_account_base_fee().await.unwrap(), 2); let bridge_lock_byte_cost_multiplier = 1; @@ -266,10 +246,7 @@ mod test { new_value: 2, }; - fee_change - .execute(&mut state, astria_address(&[1; 20])) - .await - .unwrap(); + fee_change.check_and_execute(&mut state).await.unwrap(); assert_eq!( state.get_bridge_lock_byte_cost_multiplier().await.unwrap(), 2 @@ -285,10 +262,7 @@ mod test { new_value: 2, }; - fee_change - .execute(&mut state, astria_address(&[1; 20])) - .await - .unwrap(); + fee_change.check_and_execute(&mut state).await.unwrap(); assert_eq!(state.get_ics20_withdrawal_base_fee().await.unwrap(), 2); } } diff --git a/crates/astria-sequencer/src/authority/state_ext.rs b/crates/astria-sequencer/src/authority/state_ext.rs index afdb4856a8..f98d18a78f 100644 --- a/crates/astria-sequencer/src/authority/state_ext.rs +++ b/crates/astria-sequencer/src/authority/state_ext.rs @@ -5,10 +5,7 @@ use anyhow::{ Context, Result, }; -use astria_core::primitive::v1::{ - Address, - ADDRESS_LEN, -}; +use astria_core::primitive::v1::ADDRESS_LEN; use async_trait::async_trait; use borsh::{ BorshDeserialize, @@ -21,7 +18,7 @@ use cnidarium::{ use tracing::instrument; use super::ValidatorSet; -use crate::address; +use crate::accounts::AddressBytes; /// Newtype wrapper to read and write an address from rocksdb. #[derive(BorshSerialize, BorshDeserialize, Debug)] @@ -32,9 +29,9 @@ const VALIDATOR_SET_STORAGE_KEY: &str = "valset"; const VALIDATOR_UPDATES_KEY: &[u8] = b"valupdates"; #[async_trait] -pub(crate) trait StateReadExt: StateRead + address::StateReadExt { +pub(crate) trait StateReadExt: StateRead { #[instrument(skip_all)] - async fn get_sudo_address(&self) -> Result
{ + async fn get_sudo_address(&self) -> Result<[u8; ADDRESS_LEN]> { let Some(bytes) = self .get_raw(SUDO_STORAGE_KEY) .await @@ -45,9 +42,7 @@ pub(crate) trait StateReadExt: StateRead + address::StateReadExt { }; let SudoAddress(address_bytes) = SudoAddress::try_from_slice(&bytes).context("invalid sudo key bytes")?; - self.try_base_prefixed(&address_bytes) - .await - .context("failed constructing address from prefixed stored in state") + Ok(address_bytes) } #[instrument(skip_all)] @@ -88,10 +83,10 @@ impl StateReadExt for T {} #[async_trait] pub(crate) trait StateWriteExt: StateWrite { #[instrument(skip_all)] - fn put_sudo_address(&mut self, address: Address) -> Result<()> { + fn put_sudo_address(&mut self, address: T) -> Result<()> { self.put_raw( SUDO_STORAGE_KEY.to_string(), - borsh::to_vec(&SudoAddress(address.bytes())) + borsh::to_vec(&SudoAddress(address.address_bytes())) .context("failed to convert sudo address to vec")?, ); Ok(()) @@ -126,7 +121,10 @@ impl StateWriteExt for T {} #[cfg(test)] mod tests { - use astria_core::protocol::transaction::v1alpha1::action::ValidatorUpdate; + use astria_core::{ + primitive::v1::ADDRESS_LEN, + protocol::transaction::v1alpha1::action::ValidatorUpdate, + }; use cnidarium::StateDelta; use super::{ @@ -137,7 +135,6 @@ mod tests { address::StateWriteExt as _, authority::ValidatorSet, test_utils::{ - astria_address, verification_key, ASTRIA_PREFIX, }, @@ -162,7 +159,7 @@ mod tests { .expect_err("no sudo address should exist at first"); // can write new - let mut address_expected = astria_address(&[42u8; 20]); + let mut address_expected = [42u8; ADDRESS_LEN]; state .put_sudo_address(address_expected) .expect("writing sudo address should not fail"); @@ -176,7 +173,7 @@ mod tests { ); // can rewrite with new value - address_expected = astria_address(&[41u8; 20]); + address_expected = [41u8; ADDRESS_LEN]; state .put_sudo_address(address_expected) .expect("writing sudo address should not fail"); diff --git a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs index 2326593150..2bc87e1300 100644 --- a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs @@ -4,64 +4,58 @@ use anyhow::{ Result, }; use astria_core::{ - primitive::v1::Address, protocol::transaction::v1alpha1::action::{ BridgeLockAction, TransferAction, }, sequencerblock::v1alpha1::block::Deposit, }; -use tracing::instrument; +use cnidarium::StateWrite; use crate::{ accounts::{ - action::transfer_check_stateful, + action::{ + check_transfer, + execute_transfer, + }, StateReadExt as _, StateWriteExt as _, }, - address, + address::StateReadExt as _, + app::ActionHandler, bridge::{ StateReadExt as _, StateWriteExt as _, }, - state_ext::{ - StateReadExt, - StateWriteExt, - }, - transaction::action_handler::ActionHandler, + transaction::StateReadExt as _, }; #[async_trait::async_trait] impl ActionHandler for BridgeLockAction { - async fn check_stateless(&self) -> Result<()> { + type CheckStatelessContext = (); + + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { Ok(()) } - async fn check_stateful( - &self, - state: &S, - from: Address, - ) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { + let from = state + .get_current_source() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); state .ensure_base_prefix(&self.to) .await .context("failed check for base prefix of destination address")?; - let transfer_action = TransferAction { - to: self.to, - asset: self.asset.clone(), - amount: self.amount, - fee_asset: self.fee_asset.clone(), - }; - // ensure the recipient is a bridge account. let rollup_id = state - .get_bridge_account_rollup_id(&self.to) + .get_bridge_account_rollup_id(self.to) .await .context("failed to get bridge account rollup id")? .ok_or_else(|| anyhow::anyhow!("bridge lock must be sent to a bridge account"))?; let allowed_asset = state - .get_bridge_account_ibc_asset(&self.to) + .get_bridge_account_ibc_asset(self.to) .await .context("failed to get bridge account asset ID")?; ensure!( @@ -95,12 +89,6 @@ impl ActionHandler for BridgeLockAction { .saturating_add(transfer_fee); ensure!(from_balance >= fee, "insufficient funds for fee payment"); - // this performs the same checks as a normal `TransferAction` - transfer_check_stateful(&transfer_action, state, from).await - } - - #[instrument(skip_all)] - async fn execute(&self, state: &mut S, from: Address) -> Result<()> { let transfer_action = TransferAction { to: self.to, asset: self.asset.clone(), @@ -108,13 +96,11 @@ impl ActionHandler for BridgeLockAction { fee_asset: self.fee_asset.clone(), }; - transfer_action - .execute(state, from) - .await - .context("failed to execute bridge lock action as transfer action")?; + check_transfer(&transfer_action, from, &state).await?; + execute_transfer(&transfer_action, from, &mut state).await?; let rollup_id = state - .get_bridge_account_rollup_id(&self.to) + .get_bridge_account_rollup_id(self.to) .await .context("failed to get bridge account rollup id")? .expect("recipient must be a bridge account; this is a bug in check_stateful"); @@ -157,7 +143,6 @@ pub(crate) fn get_deposit_byte_len(deposit: &Deposit) -> u128 { #[cfg(test)] mod tests { - use address::StateWriteExt; use astria_core::primitive::v1::{ asset, RollupId, @@ -166,11 +151,17 @@ mod tests { use super::*; use crate::{ + address::StateWriteExt, assets::StateWriteExt as _, test_utils::{ + assert_anyhow_error, astria_address, ASTRIA_PREFIX, }, + transaction::{ + StateWriteExt as _, + TransactionContext, + }, }; fn test_asset() -> asset::Denom { @@ -178,73 +169,18 @@ mod tests { } #[tokio::test] - async fn bridge_lock_check_stateful_fee_calc() { + async fn execute_fee_calc() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); let transfer_fee = 12; - state.put_base_prefix(ASTRIA_PREFIX).unwrap(); - state.put_transfer_base_fee(transfer_fee).unwrap(); - state.put_bridge_lock_byte_cost_multiplier(2); - - let bridge_address = astria_address(&[1; 20]); - let asset = test_asset(); - let bridge_lock = BridgeLockAction { - to: bridge_address, - asset: asset.clone(), - amount: 100, - fee_asset: asset.clone(), - destination_chain_address: "someaddress".to_string(), - }; - - let rollup_id = RollupId::from_unhashed_bytes(b"test_rollup_id"); - state.put_bridge_account_rollup_id(&bridge_address, &rollup_id); - state - .put_bridge_account_ibc_asset(&bridge_address, &asset) - .unwrap(); - state.put_allowed_fee_asset(&asset); - let from_address = astria_address(&[2; 20]); + state.put_current_source(TransactionContext { + address_bytes: from_address.bytes(), + }); + state.put_base_prefix(ASTRIA_PREFIX).unwrap(); - // not enough balance; should fail - state - .put_account_balance(from_address, &asset, 100) - .unwrap(); - - assert!( - bridge_lock - .check_stateful(&state, from_address) - .await - .unwrap_err() - .to_string() - .contains("insufficient funds for fee payment") - ); - - // enough balance; should pass - let expected_deposit_fee = transfer_fee - + get_deposit_byte_len(&Deposit::new( - bridge_address, - rollup_id, - 100, - asset.clone(), - "someaddress".to_string(), - )) * 2; - state - .put_account_balance(from_address, &asset, 100 + expected_deposit_fee) - .unwrap(); - bridge_lock - .check_stateful(&state, from_address) - .await - .unwrap(); - } - - #[tokio::test] - async fn bridge_lock_execute_fee_calc() { - let storage = cnidarium::TempStorage::new().await.unwrap(); - let snapshot = storage.latest_snapshot(); - let mut state = StateDelta::new(snapshot); - let transfer_fee = 12; state.put_transfer_base_fee(transfer_fee).unwrap(); state.put_bridge_lock_byte_cost_multiplier(2); @@ -259,25 +195,19 @@ mod tests { }; let rollup_id = RollupId::from_unhashed_bytes(b"test_rollup_id"); - state.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state.put_bridge_account_rollup_id(bridge_address, &rollup_id); state - .put_bridge_account_ibc_asset(&bridge_address, &asset) + .put_bridge_account_ibc_asset(bridge_address, &asset) .unwrap(); state.put_allowed_fee_asset(&asset); - let from_address = astria_address(&[2; 20]); - // not enough balance; should fail state .put_account_balance(from_address, &asset, 100 + transfer_fee) .unwrap(); - assert!( - bridge_lock - .execute(&mut state, from_address) - .await - .unwrap_err() - .to_string() - .eq("failed to deduct fee from account balance") + assert_anyhow_error( + &bridge_lock.check_and_execute(&mut state).await.unwrap_err(), + "insufficient funds for fee payment", ); // enough balance; should pass @@ -292,6 +222,6 @@ mod tests { state .put_account_balance(from_address, &asset, 100 + expected_deposit_fee) .unwrap(); - bridge_lock.execute(&mut state, from_address).await.unwrap(); + bridge_lock.check_and_execute(&mut state).await.unwrap(); } } diff --git a/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs b/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs index 518a9417e9..cd835877c7 100644 --- a/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs @@ -3,38 +3,33 @@ use anyhow::{ Context as _, Result, }; -use astria_core::{ - primitive::v1::Address, - protocol::transaction::v1alpha1::action::BridgeSudoChangeAction, -}; -use tracing::instrument; +use astria_core::protocol::transaction::v1alpha1::action::BridgeSudoChangeAction; +use cnidarium::StateWrite; use crate::{ accounts::StateWriteExt as _, - address, + address::StateReadExt as _, + app::ActionHandler, assets::StateReadExt as _, bridge::state_ext::{ StateReadExt as _, StateWriteExt as _, }, - state_ext::{ - StateReadExt, - StateWriteExt, - }, - transaction::action_handler::ActionHandler, + transaction::StateReadExt as _, }; - #[async_trait::async_trait] impl ActionHandler for BridgeSudoChangeAction { - async fn check_stateless(&self) -> Result<()> { + type CheckStatelessContext = (); + + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { Ok(()) } - async fn check_stateful( - &self, - state: &S, - from: Address, - ) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { + let from = state + .get_current_source() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); state .ensure_base_prefix(&self.bridge_address) .await @@ -62,7 +57,7 @@ impl ActionHandler for BridgeSudoChangeAction { // check that the sender of this tx is the authorized sudo address for the bridge account let Some(sudo_address) = state - .get_bridge_account_sudo_address(&self.bridge_address) + .get_bridge_account_sudo_address(self.bridge_address) .await .context("failed to get bridge account sudo address")? else { @@ -76,11 +71,6 @@ impl ActionHandler for BridgeSudoChangeAction { "unauthorized for bridge sudo change action", ); - Ok(()) - } - - #[instrument(skip_all)] - async fn execute(&self, state: &mut S, _: Address) -> Result<()> { let fee = state .get_bridge_sudo_change_base_fee() .await @@ -91,11 +81,11 @@ impl ActionHandler for BridgeSudoChangeAction { .context("failed to decrease balance for bridge sudo change fee")?; if let Some(sudo_address) = self.new_sudo_address { - state.put_bridge_account_sudo_address(&self.bridge_address, &sudo_address); + state.put_bridge_account_sudo_address(self.bridge_address, sudo_address); } if let Some(withdrawer_address) = self.new_withdrawer_address { - state.put_bridge_account_withdrawer_address(&self.bridge_address, &withdrawer_address); + state.put_bridge_account_withdrawer_address(self.bridge_address, withdrawer_address); } Ok(()) @@ -104,17 +94,21 @@ impl ActionHandler for BridgeSudoChangeAction { #[cfg(test)] mod tests { - use address::StateWriteExt; use astria_core::primitive::v1::asset; use cnidarium::StateDelta; use super::*; use crate::{ + address::StateWriteExt as _, assets::StateWriteExt as _, test_utils::{ astria_address, ASTRIA_PREFIX, }, + transaction::{ + StateWriteExt as _, + TransactionContext, + }, }; fn test_asset() -> asset::Denom { @@ -122,36 +116,14 @@ mod tests { } #[tokio::test] - async fn check_stateless_ok() { - let storage = cnidarium::TempStorage::new().await.unwrap(); - let snapshot = storage.latest_snapshot(); - let mut state = StateDelta::new(snapshot); - - state.put_base_prefix(ASTRIA_PREFIX).unwrap(); - - let asset = test_asset(); - state.put_allowed_fee_asset(&asset); - - let bridge_address = astria_address(&[99; 20]); - let sudo_address = astria_address(&[98; 20]); - state.put_bridge_account_sudo_address(&bridge_address, &sudo_address); - - let action = BridgeSudoChangeAction { - bridge_address, - new_sudo_address: None, - new_withdrawer_address: None, - fee_asset: asset.clone(), - }; - - action.check_stateful(&state, sudo_address).await.unwrap(); - } - - #[tokio::test] - async fn check_stateless_unauthorized() { + async fn fails_with_unauthorized_if_signer_is_not_sudo_address() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); + state.put_current_source(TransactionContext { + address_bytes: [1; 20], + }); state.put_base_prefix(ASTRIA_PREFIX).unwrap(); let asset = test_asset(); @@ -159,7 +131,7 @@ mod tests { let bridge_address = astria_address(&[99; 20]); let sudo_address = astria_address(&[98; 20]); - state.put_bridge_account_sudo_address(&bridge_address, &sudo_address); + state.put_bridge_account_sudo_address(bridge_address, sudo_address); let action = BridgeSudoChangeAction { bridge_address, @@ -170,7 +142,7 @@ mod tests { assert!( action - .check_stateful(&state, bridge_address) + .check_and_execute(state) .await .unwrap_err() .to_string() @@ -179,16 +151,25 @@ mod tests { } #[tokio::test] - async fn execute_ok() { + async fn executes() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); + let sudo_address = astria_address(&[98; 20]); + state.put_current_source(TransactionContext { + address_bytes: sudo_address.bytes(), + }); state.put_base_prefix(ASTRIA_PREFIX).unwrap(); state.put_bridge_sudo_change_base_fee(10); let fee_asset = test_asset(); + state.put_allowed_fee_asset(&fee_asset); + let bridge_address = astria_address(&[99; 20]); + + state.put_bridge_account_sudo_address(bridge_address, sudo_address); + let new_sudo_address = astria_address(&[98; 20]); let new_withdrawer_address = astria_address(&[97; 20]); state @@ -202,21 +183,21 @@ mod tests { fee_asset, }; - action.execute(&mut state, bridge_address).await.unwrap(); + action.check_and_execute(&mut state).await.unwrap(); assert_eq!( state - .get_bridge_account_sudo_address(&bridge_address) + .get_bridge_account_sudo_address(bridge_address) .await .unwrap(), - Some(new_sudo_address), + Some(new_sudo_address.bytes()), ); assert_eq!( state - .get_bridge_account_withdrawer_address(&bridge_address) + .get_bridge_account_withdrawer_address(bridge_address) .await .unwrap(), - Some(new_withdrawer_address), + Some(new_withdrawer_address.bytes()), ); } } diff --git a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs index 550cf5dce4..e5d957cba1 100644 --- a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs @@ -11,30 +11,32 @@ use astria_core::{ TransferAction, }, }; -use tracing::instrument; +use cnidarium::StateWrite; use crate::{ - accounts::action::transfer_check_stateful, - address, - bridge::StateReadExt as _, - state_ext::{ - StateReadExt, - StateWriteExt, + accounts::action::{ + check_transfer, + execute_transfer, }, - transaction::action_handler::ActionHandler, + address::StateReadExt as _, + app::ActionHandler, + bridge::StateReadExt as _, + transaction::StateReadExt as _, }; #[async_trait::async_trait] impl ActionHandler for BridgeUnlockAction { - async fn check_stateless(&self) -> Result<()> { + type CheckStatelessContext = (); + + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { Ok(()) } - async fn check_stateful( - &self, - state: &S, - from: Address, - ) -> Result<()> { + async fn check_and_execute(&self, state: S) -> Result<()> { + let from = state + .get_current_source() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); state .ensure_base_prefix(&self.to) .await @@ -48,17 +50,16 @@ impl ActionHandler for BridgeUnlockAction { // the bridge address to withdraw funds from // if unset, use the tx sender's address - let bridge_address = self.bridge_address.unwrap_or(from); + let bridge_address = self.bridge_address.map_or(from, Address::bytes); - // grab the bridge account's asset let asset = state - .get_bridge_account_ibc_asset(&bridge_address) + .get_bridge_account_ibc_asset(bridge_address) .await .context("failed to get bridge's asset id, must be a bridge account")?; // check that the sender of this tx is the authorized withdrawer for the bridge account let Some(withdrawer_address) = state - .get_bridge_account_withdrawer_address(&bridge_address) + .get_bridge_account_withdrawer_address(bridge_address) .await .context("failed to get bridge account withdrawer address")? else { @@ -77,31 +78,8 @@ impl ActionHandler for BridgeUnlockAction { fee_asset: self.fee_asset.clone(), }; - // this performs the same checks as a normal `TransferAction` - transfer_check_stateful(&transfer_action, state, bridge_address).await - } - - #[instrument(skip_all)] - async fn execute(&self, state: &mut S, from: Address) -> Result<()> { - // the bridge address to withdraw funds from - let bridge_address = self.bridge_address.unwrap_or(from); - - let asset = state - .get_bridge_account_ibc_asset(&bridge_address) - .await - .context("failed to get bridge's asset id, must be a bridge account")?; - - let transfer_action = TransferAction { - to: self.to, - asset: asset.into(), - amount: self.amount, - fee_asset: self.fee_asset.clone(), - }; - - transfer_action - .execute(state, bridge_address) - .await - .context("failed to execute bridge unlock action as transfer action")?; + check_transfer(&transfer_action, bridge_address, &state).await?; + execute_transfer(&transfer_action, bridge_address, state).await?; Ok(()) } @@ -109,22 +87,30 @@ impl ActionHandler for BridgeUnlockAction { #[cfg(test)] mod test { - use astria_core::primitive::v1::{ - asset, - RollupId, + use astria_core::{ + primitive::v1::{ + asset, + RollupId, + }, + protocol::transaction::v1alpha1::action::BridgeUnlockAction, }; use cnidarium::StateDelta; - use super::*; use crate::{ accounts::StateWriteExt as _, address::StateWriteExt as _, + app::ActionHandler as _, assets::StateWriteExt as _, bridge::StateWriteExt as _, test_utils::{ + assert_anyhow_error, astria_address, ASTRIA_PREFIX, }, + transaction::{ + StateWriteExt as _, + TransactionContext, + }, }; fn test_asset() -> asset::Denom { @@ -132,17 +118,19 @@ mod test { } #[tokio::test] - async fn fail_non_bridge_accounts() { + async fn fails_if_bridge_address_is_not_set_and_signer_is_not_bridge() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); + state.put_current_source(TransactionContext { + address_bytes: [1; 20], + }); state.put_base_prefix(ASTRIA_PREFIX).unwrap(); let asset = test_asset(); let transfer_amount = 100; - let address = astria_address(&[1; 20]); let to_address = astria_address(&[2; 20]); let bridge_unlock = BridgeUnlockAction { @@ -156,7 +144,7 @@ mod test { // not a bridge account, should fail assert!( bridge_unlock - .check_stateful(&state, address) + .check_and_execute(state) .await .unwrap_err() .to_string() @@ -165,24 +153,25 @@ mod test { } #[tokio::test] - async fn fail_withdrawer_unset_invalid_withdrawer() { + async fn fails_if_bridge_account_has_no_withdrawer_address() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); + state.put_current_source(TransactionContext { + address_bytes: [1; 20], + }); state.put_base_prefix(ASTRIA_PREFIX).unwrap(); let asset = test_asset(); let transfer_amount = 100; - let sender_address = astria_address(&[1; 20]); let to_address = astria_address(&[2; 20]); - let bridge_address = astria_address(&[3; 20]); + // state.put_bridge_account_withdrawer_address(bridge_address, bridge_address); state - .put_bridge_account_ibc_asset(&bridge_address, &asset) + .put_bridge_account_ibc_asset(bridge_address, &asset) .unwrap(); - state.put_bridge_account_withdrawer_address(&bridge_address, &bridge_address); let bridge_unlock = BridgeUnlockAction { to: to_address, @@ -193,35 +182,32 @@ mod test { }; // invalid sender, doesn't match action's `from`, should fail - assert!( - bridge_unlock - .check_stateful(&state, sender_address) - .await - .unwrap_err() - .to_string() - .contains("unauthorized to unlock bridge account") + assert_anyhow_error( + &bridge_unlock.check_and_execute(state).await.unwrap_err(), + "bridge account does not have an associated withdrawer address", ); } #[tokio::test] - async fn fail_withdrawer_set_invalid_withdrawer() { + async fn fails_if_withdrawer_is_not_signer() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); + state.put_current_source(TransactionContext { + address_bytes: [1; 20], + }); state.put_base_prefix(ASTRIA_PREFIX).unwrap(); let asset = test_asset(); let transfer_amount = 100; - let sender_address = astria_address(&[1; 20]); let to_address = astria_address(&[2; 20]); - let bridge_address = astria_address(&[3; 20]); let withdrawer_address = astria_address(&[4; 20]); - state.put_bridge_account_withdrawer_address(&bridge_address, &withdrawer_address); + state.put_bridge_account_withdrawer_address(bridge_address, withdrawer_address); state - .put_bridge_account_ibc_asset(&bridge_address, &asset) + .put_bridge_account_ibc_asset(bridge_address, &asset) .unwrap(); let bridge_unlock = BridgeUnlockAction { @@ -233,77 +219,22 @@ mod test { }; // invalid sender, doesn't match action's bridge account's withdrawer, should fail - assert!( - bridge_unlock - .check_stateful(&state, sender_address) - .await - .unwrap_err() - .to_string() - .contains("unauthorized to unlock bridge account") + assert_anyhow_error( + &bridge_unlock.check_and_execute(state).await.unwrap_err(), + "unauthorized to unlock bridge account", ); } #[tokio::test] - async fn fee_check_stateful_from_none() { + async fn execute_with_bridge_address_unset() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); - state.put_base_prefix(ASTRIA_PREFIX).unwrap(); - - let asset = test_asset(); - let transfer_fee = 10; - let transfer_amount = 100; - state.put_transfer_base_fee(transfer_fee).unwrap(); - let bridge_address = astria_address(&[1; 20]); - let to_address = astria_address(&[2; 20]); - let rollup_id = RollupId::from_unhashed_bytes(b"test_rollup_id"); - - state.put_bridge_account_rollup_id(&bridge_address, &rollup_id); - state - .put_bridge_account_ibc_asset(&bridge_address, &asset) - .unwrap(); - state.put_allowed_fee_asset(&asset); - state.put_bridge_account_withdrawer_address(&bridge_address, &bridge_address); - - let bridge_unlock = BridgeUnlockAction { - to: to_address, - amount: transfer_amount, - fee_asset: asset.clone(), - memo: "{}".into(), - bridge_address: None, - }; - - // not enough balance to transfer asset; should fail - state - .put_account_balance(bridge_address, &asset, transfer_amount) - .unwrap(); - assert!( - bridge_unlock - .check_stateful(&state, bridge_address) - .await - .unwrap_err() - .to_string() - .contains("insufficient funds for transfer and fee payment") - ); - - // enough balance; should pass - state - .put_account_balance(bridge_address, &asset, transfer_amount + transfer_fee) - .unwrap(); - bridge_unlock - .check_stateful(&state, bridge_address) - .await - .unwrap(); - } - - #[tokio::test] - async fn fee_check_stateful_from_some() { - let storage = cnidarium::TempStorage::new().await.unwrap(); - let snapshot = storage.latest_snapshot(); - let mut state = StateDelta::new(snapshot); - + state.put_current_source(TransactionContext { + address_bytes: bridge_address.bytes(), + }); state.put_base_prefix(ASTRIA_PREFIX).unwrap(); let asset = test_asset(); @@ -311,69 +242,14 @@ mod test { let transfer_amount = 100; state.put_transfer_base_fee(transfer_fee).unwrap(); - let bridge_address = astria_address(&[1; 20]); - let to_address = astria_address(&[2; 20]); - let rollup_id = RollupId::from_unhashed_bytes(b"test_rollup_id"); - - state.put_bridge_account_rollup_id(&bridge_address, &rollup_id); - state - .put_bridge_account_ibc_asset(&bridge_address, &asset) - .unwrap(); - state.put_allowed_fee_asset(&asset); - - let withdrawer_address = astria_address(&[3; 20]); - state.put_bridge_account_withdrawer_address(&bridge_address, &withdrawer_address); - - let bridge_unlock = BridgeUnlockAction { - to: to_address, - amount: transfer_amount, - fee_asset: asset.clone(), - memo: "{}".into(), - bridge_address: Some(bridge_address), - }; - - // not enough balance to transfer asset; should fail - state - .put_account_balance(bridge_address, &asset, transfer_amount) - .unwrap(); - assert!( - bridge_unlock - .check_stateful(&state, withdrawer_address) - .await - .unwrap_err() - .to_string() - .contains("insufficient funds for transfer and fee payment") - ); - - // enough balance; should pass - state - .put_account_balance(bridge_address, &asset, transfer_amount + transfer_fee) - .unwrap(); - bridge_unlock - .check_stateful(&state, withdrawer_address) - .await - .unwrap(); - } - - #[tokio::test] - async fn execute_from_none() { - let storage = cnidarium::TempStorage::new().await.unwrap(); - let snapshot = storage.latest_snapshot(); - let mut state = StateDelta::new(snapshot); - - let asset = test_asset(); - let transfer_fee = 10; - let transfer_amount = 100; - state.put_transfer_base_fee(transfer_fee).unwrap(); - - let bridge_address = astria_address(&[1; 20]); let to_address = astria_address(&[2; 20]); let rollup_id = RollupId::from_unhashed_bytes(b"test_rollup_id"); - state.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state.put_bridge_account_rollup_id(bridge_address, &rollup_id); state - .put_bridge_account_ibc_asset(&bridge_address, &asset) + .put_bridge_account_ibc_asset(bridge_address, &asset) .unwrap(); + state.put_bridge_account_withdrawer_address(bridge_address, bridge_address); state.put_allowed_fee_asset(&asset); let bridge_unlock = BridgeUnlockAction { @@ -388,44 +264,46 @@ mod test { state .put_account_balance(bridge_address, &asset, transfer_amount) .unwrap(); - assert!( - bridge_unlock - .execute(&mut state, bridge_address) + assert_anyhow_error( + &bridge_unlock + .check_and_execute(&mut state) .await - .unwrap_err() - .to_string() - .eq("failed to execute bridge unlock action as transfer action") + .unwrap_err(), + "insufficient funds for transfer and fee payment", ); // enough balance; should pass state .put_account_balance(bridge_address, &asset, transfer_amount + transfer_fee) .unwrap(); - bridge_unlock - .execute(&mut state, bridge_address) - .await - .unwrap(); + bridge_unlock.check_and_execute(&mut state).await.unwrap(); } #[tokio::test] - async fn execute_from_some() { + async fn execute_with_bridge_address_set() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); + let bridge_address = astria_address(&[1; 20]); + state.put_current_source(TransactionContext { + address_bytes: bridge_address.bytes(), + }); + state.put_base_prefix(ASTRIA_PREFIX).unwrap(); + let asset = test_asset(); let transfer_fee = 10; let transfer_amount = 100; state.put_transfer_base_fee(transfer_fee).unwrap(); - let bridge_address = astria_address(&[1; 20]); let to_address = astria_address(&[2; 20]); let rollup_id = RollupId::from_unhashed_bytes(b"test_rollup_id"); - state.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state.put_bridge_account_rollup_id(bridge_address, &rollup_id); state - .put_bridge_account_ibc_asset(&bridge_address, &asset) + .put_bridge_account_ibc_asset(bridge_address, &asset) .unwrap(); + state.put_bridge_account_withdrawer_address(bridge_address, bridge_address); state.put_allowed_fee_asset(&asset); let bridge_unlock = BridgeUnlockAction { @@ -440,22 +318,18 @@ mod test { state .put_account_balance(bridge_address, &asset, transfer_amount) .unwrap(); - assert!( - bridge_unlock - .execute(&mut state, bridge_address) + assert_anyhow_error( + &bridge_unlock + .check_and_execute(&mut state) .await - .unwrap_err() - .to_string() - .eq("failed to execute bridge unlock action as transfer action") + .unwrap_err(), + "insufficient funds for transfer and fee payment", ); // enough balance; should pass state .put_account_balance(bridge_address, &asset, transfer_amount + transfer_fee) .unwrap(); - bridge_unlock - .execute(&mut state, bridge_address) - .await - .unwrap(); + bridge_unlock.check_and_execute(&mut state).await.unwrap(); } } diff --git a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs index 986a314bfe..54b7122444 100644 --- a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs +++ b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs @@ -8,37 +8,36 @@ use astria_core::{ primitive::v1::Address, protocol::transaction::v1alpha1::action::InitBridgeAccountAction, }; -use tracing::instrument; +use cnidarium::StateWrite; use crate::{ accounts::{ StateReadExt as _, StateWriteExt as _, }, - address, + address::StateReadExt as _, + app::ActionHandler, assets::StateReadExt as _, bridge::state_ext::{ StateReadExt as _, StateWriteExt as _, }, - state_ext::{ - StateReadExt, - StateWriteExt, - }, - transaction::action_handler::ActionHandler, + transaction::StateReadExt as _, }; #[async_trait::async_trait] impl ActionHandler for InitBridgeAccountAction { - async fn check_stateless(&self) -> Result<()> { + type CheckStatelessContext = (); + + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { Ok(()) } - async fn check_stateful( - &self, - state: &S, - from: Address, - ) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { + let from = state + .get_current_source() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); if let Some(withdrawer_address) = &self.withdrawer_address { state .ensure_base_prefix(withdrawer_address) @@ -73,7 +72,12 @@ impl ActionHandler for InitBridgeAccountAction { // // after the account becomes a bridge account, it can no longer receive funds // via `TransferAction`, only via `BridgeLockAction`. - if state.get_bridge_account_rollup_id(&from).await?.is_some() { + if state + .get_bridge_account_rollup_id(from) + .await + .context("failed getting rollup ID of bridge account")? + .is_some() + { bail!("bridge account already exists"); } @@ -87,23 +91,15 @@ impl ActionHandler for InitBridgeAccountAction { "insufficient funds for bridge account initialization", ); - Ok(()) - } - - #[instrument(skip_all)] - async fn execute(&self, state: &mut S, from: Address) -> Result<()> { - let fee = state - .get_init_bridge_account_base_fee() - .await - .context("failed to get base fee for initializing bridge account")?; - - state.put_bridge_account_rollup_id(&from, &self.rollup_id); + state.put_bridge_account_rollup_id(from, &self.rollup_id); state - .put_bridge_account_ibc_asset(&from, &self.asset) + .put_bridge_account_ibc_asset(from, &self.asset) .context("failed to put asset ID")?; - state.put_bridge_account_sudo_address(&from, &self.sudo_address.unwrap_or(from)); - state - .put_bridge_account_withdrawer_address(&from, &self.withdrawer_address.unwrap_or(from)); + state.put_bridge_account_sudo_address(from, self.sudo_address.map_or(from, Address::bytes)); + state.put_bridge_account_withdrawer_address( + from, + self.withdrawer_address.map_or(from, Address::bytes), + ); state .decrease_balance(from, &self.fee_asset, fee) diff --git a/crates/astria-sequencer/src/bridge/query.rs b/crates/astria-sequencer/src/bridge/query.rs index 3ea29d867d..e0eb0e247b 100644 --- a/crates/astria-sequencer/src/bridge/query.rs +++ b/crates/astria-sequencer/src/bridge/query.rs @@ -14,6 +14,7 @@ use tendermint::abci::{ }; use crate::{ + address::StateReadExt, assets::StateReadExt as _, bridge::StateReadExt as _, state_ext::StateReadExt as _, @@ -36,11 +37,14 @@ fn error_query_response( } } +// allow / FIXME: there is a lot of code duplication due to `error_query_response`. +// this could be significantly shortened. +#[allow(clippy::too_many_lines)] async fn get_bridge_account_info( snapshot: cnidarium::Snapshot, address: Address, ) -> anyhow::Result, response::Query> { - let rollup_id = match snapshot.get_bridge_account_rollup_id(&address).await { + let rollup_id = match snapshot.get_bridge_account_rollup_id(address).await { Ok(Some(rollup_id)) => rollup_id, Ok(None) => { return Ok(None); @@ -54,7 +58,7 @@ async fn get_bridge_account_info( } }; - let ibc_asset = match snapshot.get_bridge_account_ibc_asset(&address).await { + let ibc_asset = match snapshot.get_bridge_account_ibc_asset(address).await { Ok(asset) => asset, Err(err) => { return Err(error_query_response( @@ -83,8 +87,8 @@ async fn get_bridge_account_info( } }; - let sudo_address = match snapshot.get_bridge_account_sudo_address(&address).await { - Ok(Some(sudo_address)) => sudo_address, + let sudo_address_bytes = match snapshot.get_bridge_account_sudo_address(address).await { + Ok(Some(bytes)) => bytes, Ok(None) => { return Err(error_query_response( None, @@ -101,8 +105,20 @@ async fn get_bridge_account_info( } }; - let withdrawer_address = match snapshot - .get_bridge_account_withdrawer_address(&address) + let sudo_address = match snapshot.try_base_prefixed(&sudo_address_bytes).await { + Err(err) => { + return Err(error_query_response( + Some(err), + AbciErrorCode::INTERNAL_ERROR, + "failed to construct bech32m address from address prefix and account bytes read \ + from state", + )); + } + Ok(address) => address, + }; + + let withdrawer_address_bytes = match snapshot + .get_bridge_account_withdrawer_address(address) .await { Ok(Some(withdrawer_address)) => withdrawer_address, @@ -122,6 +138,18 @@ async fn get_bridge_account_info( } }; + let withdrawer_address = match snapshot.try_base_prefixed(&withdrawer_address_bytes).await { + Err(err) => { + return Err(error_query_response( + Some(err), + AbciErrorCode::INTERNAL_ERROR, + "failed to construct bech32m address from address prefix and account bytes read \ + from state", + )); + } + Ok(address) => address, + }; + Ok(Some(BridgeAccountInfo { rollup_id, asset: trace_asset.into(), @@ -293,15 +321,15 @@ mod test { let sudo_address = astria_address(&[1u8; 20]); let withdrawer_address = astria_address(&[2u8; 20]); state.put_block_height(1); - state.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state.put_bridge_account_rollup_id(bridge_address, &rollup_id); state .put_ibc_asset(asset.as_trace_prefixed().unwrap()) .unwrap(); state - .put_bridge_account_ibc_asset(&bridge_address, &asset) + .put_bridge_account_ibc_asset(bridge_address, &asset) .unwrap(); - state.put_bridge_account_sudo_address(&bridge_address, &sudo_address); - state.put_bridge_account_withdrawer_address(&bridge_address, &withdrawer_address); + state.put_bridge_account_sudo_address(bridge_address, sudo_address); + state.put_bridge_account_withdrawer_address(bridge_address, withdrawer_address); storage.commit(state).await.unwrap(); let query = request::Query { diff --git a/crates/astria-sequencer/src/bridge/state_ext.rs b/crates/astria-sequencer/src/bridge/state_ext.rs index 3b8ba0b5f6..0316603c09 100644 --- a/crates/astria-sequencer/src/bridge/state_ext.rs +++ b/crates/astria-sequencer/src/bridge/state_ext.rs @@ -14,6 +14,7 @@ use astria_core::{ asset, Address, RollupId, + ADDRESS_LEN, }, sequencerblock::v1alpha1::block::Deposit, }; @@ -34,7 +35,10 @@ use tracing::{ instrument, }; -use crate::address; +use crate::{ + accounts::AddressBytes, + address, +}; /// Newtype wrapper to read and write a u128 from rocksdb. #[derive(BorshSerialize, BorshDeserialize, Debug)] @@ -60,23 +64,26 @@ const INIT_BRIDGE_ACCOUNT_BASE_FEE_STORAGE_KEY: &str = "initbridgeaccfee"; const BRIDGE_LOCK_BYTE_COST_MULTIPLIER_STORAGE_KEY: &str = "bridgelockmultiplier"; const BRIDGE_SUDO_CHANGE_FEE_STORAGE_KEY: &str = "bridgesudofee"; -struct BridgeAccountKey<'a> { +struct BridgeAccountKey<'a, T> { prefix: &'static str, - address: &'a Address, + address: &'a T, } -impl<'a> std::fmt::Display for BridgeAccountKey<'a> { +impl<'a, T> std::fmt::Display for BridgeAccountKey<'a, T> +where + T: AddressBytes, +{ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.write_str(self.prefix)?; f.write_str("/")?; - for byte in self.address.bytes() { + for byte in self.address.address_bytes() { f.write_fmt(format_args!("{byte:02x}"))?; } Ok(()) } } -fn rollup_id_storage_key(address: &Address) -> String { +fn rollup_id_storage_key(address: &T) -> String { format!( "{}/rollupid", BridgeAccountKey { @@ -86,7 +93,7 @@ fn rollup_id_storage_key(address: &Address) -> String { ) } -fn asset_id_storage_key(address: &Address) -> String { +fn asset_id_storage_key(address: &T) -> String { format!( "{}/assetid", BridgeAccountKey { @@ -108,7 +115,7 @@ fn deposit_nonce_storage_key(rollup_id: &RollupId) -> Vec { format!("depositnonce/{}", rollup_id.encode_hex::()).into() } -fn bridge_account_sudo_address_storage_key(address: &Address) -> String { +fn bridge_account_sudo_address_storage_key(address: &T) -> String { format!( "{}", BridgeAccountKey { @@ -118,7 +125,7 @@ fn bridge_account_sudo_address_storage_key(address: &Address) -> String { ) } -fn bridge_account_withdrawer_address_storage_key(address: &Address) -> String { +fn bridge_account_withdrawer_address_storage_key(address: &T) -> String { format!( "{}", BridgeAccountKey { @@ -128,7 +135,7 @@ fn bridge_account_withdrawer_address_storage_key(address: &Address) -> String { ) } -fn last_transaction_hash_for_bridge_account_storage_key(address: &Address) -> Vec { +fn last_transaction_hash_for_bridge_account_storage_key(address: &T) -> Vec { format!( "{}/lasttx", BridgeAccountKey { @@ -143,9 +150,12 @@ fn last_transaction_hash_for_bridge_account_storage_key(address: &Address) -> Ve #[async_trait] pub(crate) trait StateReadExt: StateRead + address::StateReadExt { #[instrument(skip_all)] - async fn get_bridge_account_rollup_id(&self, address: &Address) -> Result> { + async fn get_bridge_account_rollup_id( + &self, + address: T, + ) -> Result> { let Some(rollup_id_bytes) = self - .get_raw(&rollup_id_storage_key(address)) + .get_raw(&rollup_id_storage_key(&address)) .await .context("failed reading raw account rollup ID from state")? else { @@ -159,9 +169,12 @@ pub(crate) trait StateReadExt: StateRead + address::StateReadExt { } #[instrument(skip_all)] - async fn get_bridge_account_ibc_asset(&self, address: &Address) -> Result { + async fn get_bridge_account_ibc_asset( + &self, + address: T, + ) -> Result { let bytes = self - .get_raw(&asset_id_storage_key(address)) + .get_raw(&asset_id_storage_key(&address)) .await .context("failed reading raw asset ID from state")? .ok_or_else(|| anyhow!("asset ID not found"))?; @@ -171,34 +184,35 @@ pub(crate) trait StateReadExt: StateRead + address::StateReadExt { } #[instrument(skip_all)] - async fn get_bridge_account_sudo_address( + async fn get_bridge_account_sudo_address( &self, - bridge_address: &Address, - ) -> Result> { + bridge_address: T, + ) -> Result> { let Some(sudo_address_bytes) = self - .get_raw(&bridge_account_sudo_address_storage_key(bridge_address)) + .get_raw(&bridge_account_sudo_address_storage_key(&bridge_address)) .await .context("failed reading raw bridge account sudo address from state")? else { debug!("bridge account sudo address not found, returning None"); return Ok(None); }; - - let sudo_address = self.try_base_prefixed(&sudo_address_bytes).await.context( - "failed check for constructing sudo address from address bytes and prefix stored \ - retrieved from state", - )?; + let sudo_address = sudo_address_bytes.try_into().map_err(|bytes: Vec<_>| { + anyhow::format_err!( + "failed to convert address `{}` bytes read from state to fixed length address", + bytes.len() + ) + })?; Ok(Some(sudo_address)) } #[instrument(skip_all)] - async fn get_bridge_account_withdrawer_address( + async fn get_bridge_account_withdrawer_address( &self, - bridge_address: &Address, - ) -> Result> { + bridge_address: T, + ) -> Result> { let Some(withdrawer_address_bytes) = self .get_raw(&bridge_account_withdrawer_address_storage_key( - bridge_address, + &bridge_address, )) .await .context("failed reading raw bridge account withdrawer address from state")? @@ -206,15 +220,15 @@ pub(crate) trait StateReadExt: StateRead + address::StateReadExt { debug!("bridge account withdrawer address not found, returning None"); return Ok(None); }; - - let withdrawer_address = self - .try_base_prefixed(&withdrawer_address_bytes) - .await - .context( - "failed check for constructing withdrawer address from address bytes and prefix \ - stored retrieved from state", - )?; - Ok(Some(withdrawer_address)) + let addr = withdrawer_address_bytes + .try_into() + .map_err(|bytes: Vec<_>| { + anyhow::Error::msg(format!( + "failed converting `{}` bytes retrieved from storage to fixed address length", + bytes.len() + )) + })?; + Ok(Some(addr)) } #[instrument(skip_all)] @@ -347,48 +361,55 @@ impl StateReadExt for T {} #[async_trait] pub(crate) trait StateWriteExt: StateWrite { #[instrument(skip_all)] - fn put_bridge_account_rollup_id(&mut self, address: &Address, rollup_id: &RollupId) { - self.put_raw(rollup_id_storage_key(address), rollup_id.to_vec()); + fn put_bridge_account_rollup_id(&mut self, address: T, rollup_id: &RollupId) { + self.put_raw(rollup_id_storage_key(&address), rollup_id.to_vec()); } #[instrument(skip_all)] - fn put_bridge_account_ibc_asset( + fn put_bridge_account_ibc_asset( &mut self, - address: &Address, + address: TAddress, asset: TAsset, ) -> Result<()> where + TAddress: AddressBytes, TAsset: Into + std::fmt::Display, { let ibc = asset.into(); self.put_raw( - asset_id_storage_key(address), + asset_id_storage_key(&address), borsh::to_vec(&AssetId(ibc.get())).context("failed to serialize asset IDs")?, ); Ok(()) } #[instrument(skip_all)] - fn put_bridge_account_sudo_address( + fn put_bridge_account_sudo_address( &mut self, - bridge_address: &Address, - sudo_address: &Address, - ) { + bridge_address: TBridgeAddress, + sudo_address: TSudoAddress, + ) where + TBridgeAddress: AddressBytes, + TSudoAddress: AddressBytes, + { self.put_raw( - bridge_account_sudo_address_storage_key(bridge_address), - sudo_address.bytes().to_vec(), + bridge_account_sudo_address_storage_key(&bridge_address), + sudo_address.address_bytes().to_vec(), ); } #[instrument(skip_all)] - fn put_bridge_account_withdrawer_address( + fn put_bridge_account_withdrawer_address( &mut self, - bridge_address: &Address, - withdrawer_address: &Address, - ) { + bridge_address: TBridgeAddress, + withdrawer_address: TWithdrawerAddress, + ) where + TBridgeAddress: AddressBytes, + TWithdrawerAddress: AddressBytes, + { self.put_raw( - bridge_account_withdrawer_address_storage_key(bridge_address), - withdrawer_address.bytes().to_vec(), + bridge_account_withdrawer_address_storage_key(&bridge_address), + withdrawer_address.address_bytes().to_vec(), ); } @@ -465,13 +486,13 @@ pub(crate) trait StateWriteExt: StateWrite { } #[instrument(skip_all)] - fn put_last_transaction_hash_for_bridge_account( + fn put_last_transaction_hash_for_bridge_account( &mut self, - address: &Address, + address: T, tx_hash: &[u8; 32], ) { self.nonverifiable_put_raw( - last_transaction_hash_for_bridge_account_storage_key(address), + last_transaction_hash_for_bridge_account_storage_key(&address), tx_hash.to_vec(), ); } @@ -520,7 +541,7 @@ mod test { // uninitialized ok assert_eq!( - state.get_bridge_account_rollup_id(&address).await.expect( + state.get_bridge_account_rollup_id(address).await.expect( "call to get bridge account rollup id should not fail for uninitialized addresses" ), Option::None, @@ -538,10 +559,10 @@ mod test { let address = astria_address(&[42u8; 20]); // can write new - state.put_bridge_account_rollup_id(&address, &rollup_id); + state.put_bridge_account_rollup_id(address, &rollup_id); assert_eq!( state - .get_bridge_account_rollup_id(&address) + .get_bridge_account_rollup_id(address) .await .expect("a rollup ID was written and must exist inside the database") .expect("expecting return value"), @@ -551,10 +572,10 @@ mod test { // can rewrite with new value rollup_id = RollupId::new([2u8; 32]); - state.put_bridge_account_rollup_id(&address, &rollup_id); + state.put_bridge_account_rollup_id(address, &rollup_id); assert_eq!( state - .get_bridge_account_rollup_id(&address) + .get_bridge_account_rollup_id(address) .await .expect("a rollup ID was written and must exist inside the database") .expect("expecting return value"), @@ -565,10 +586,10 @@ mod test { // can write additional account and both valid let rollup_id_1 = RollupId::new([2u8; 32]); let address_1 = astria_address(&[41u8; 20]); - state.put_bridge_account_rollup_id(&address_1, &rollup_id_1); + state.put_bridge_account_rollup_id(address_1, &rollup_id_1); assert_eq!( state - .get_bridge_account_rollup_id(&address_1) + .get_bridge_account_rollup_id(address_1) .await .expect("a rollup ID was written and must exist inside the database") .expect("expecting return value"), @@ -578,7 +599,7 @@ mod test { assert_eq!( state - .get_bridge_account_rollup_id(&address) + .get_bridge_account_rollup_id(address) .await .expect("a rollup ID was written and must exist inside the database") .expect("expecting return value"), @@ -595,7 +616,7 @@ mod test { let address = astria_address(&[42u8; 20]); state - .get_bridge_account_ibc_asset(&address) + .get_bridge_account_ibc_asset(address) .await .expect_err("call to get bridge account asset ids should fail if no assets"); } @@ -611,10 +632,10 @@ mod test { // can write state - .put_bridge_account_ibc_asset(&address, &asset) + .put_bridge_account_ibc_asset(address, &asset) .expect("storing bridge account asset should not fail"); let mut result = state - .get_bridge_account_ibc_asset(&address) + .get_bridge_account_ibc_asset(address) .await .expect("bridge asset id was written and must exist inside the database"); assert_eq!( @@ -626,10 +647,10 @@ mod test { // can update asset = "asset_2".parse::().unwrap(); state - .put_bridge_account_ibc_asset(&address, &asset) + .put_bridge_account_ibc_asset(address, &asset) .expect("storing bridge account assets should not fail"); result = state - .get_bridge_account_ibc_asset(&address) + .get_bridge_account_ibc_asset(address) .await .expect("bridge asset id was written and must exist inside the database"); assert_eq!( @@ -642,18 +663,18 @@ mod test { let address_1 = astria_address(&[41u8; 20]); let asset_1 = asset_1(); state - .put_bridge_account_ibc_asset(&address_1, &asset_1) + .put_bridge_account_ibc_asset(address_1, &asset_1) .expect("storing bridge account assets should not fail"); assert_eq!( state - .get_bridge_account_ibc_asset(&address_1) + .get_bridge_account_ibc_asset(address_1) .await .expect("bridge asset id was written and must exist inside the database"), asset_1.into(), "second bridge account asset not what was expected" ); result = state - .get_bridge_account_ibc_asset(&address) + .get_bridge_account_ibc_asset(address) .await .expect("original bridge asset id was written and must exist inside the database"); assert_eq!( diff --git a/crates/astria-sequencer/src/fee_asset_change.rs b/crates/astria-sequencer/src/fee_asset_change.rs index 7ccdfdf804..8779e5dbb9 100644 --- a/crates/astria-sequencer/src/fee_asset_change.rs +++ b/crates/astria-sequencer/src/fee_asset_change.rs @@ -4,26 +4,33 @@ use anyhow::{ Context as _, Result, }; -use astria_core::{ - primitive::v1::Address, - protocol::transaction::v1alpha1::action::FeeAssetChangeAction, -}; +use astria_core::protocol::transaction::v1alpha1::action::FeeAssetChangeAction; use async_trait::async_trait; +use cnidarium::StateWrite; use crate::{ - assets, - assets::StateReadExt as _, - authority, - transaction::action_handler::ActionHandler, + app::ActionHandler, + assets::{ + StateReadExt as _, + StateWriteExt as _, + }, + authority::StateReadExt as _, + transaction::StateReadExt as _, }; #[async_trait] impl ActionHandler for FeeAssetChangeAction { - async fn check_stateful( - &self, - state: &S, - from: Address, - ) -> Result<()> { + type CheckStatelessContext = (); + + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { + Ok(()) + } + + async fn check_and_execute(&self, mut state: S) -> Result<()> { + let from = state + .get_current_source() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); let authority_sudo_address = state .get_sudo_address() .await @@ -32,10 +39,6 @@ impl ActionHandler for FeeAssetChangeAction { authority_sudo_address == from, "unauthorized address for fee asset change" ); - Ok(()) - } - - async fn execute(&self, state: &mut S, _from: Address) -> Result<()> { match self { FeeAssetChangeAction::Addition(asset) => { state.put_allowed_fee_asset(asset); diff --git a/crates/astria-sequencer/src/grpc/sequencer.rs b/crates/astria-sequencer/src/grpc/sequencer.rs index c17f899587..408186ee4d 100644 --- a/crates/astria-sequencer/src/grpc/sequencer.rs +++ b/crates/astria-sequencer/src/grpc/sequencer.rs @@ -261,12 +261,12 @@ mod test { let alice = get_alice_signing_key(); let alice_address = astria_address(&alice.address_bytes()); let nonce = 99; - let tx = crate::app::test_utils::get_mock_tx(nonce); + let tx = Arc::new(crate::app::test_utils::get_mock_tx(nonce)); mempool.insert(tx, 0).await.unwrap(); // insert a tx with lower nonce also, but we should get the highest nonce let lower_nonce = 98; - let tx = crate::app::test_utils::get_mock_tx(lower_nonce); + let tx = Arc::new(crate::app::test_utils::get_mock_tx(lower_nonce)); mempool.insert(tx, 0).await.unwrap(); let server = Arc::new(SequencerServer::new(storage.clone(), mempool)); diff --git a/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs b/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs index cb37a1ac30..fbf8ff58f8 100644 --- a/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs +++ b/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs @@ -3,29 +3,33 @@ use anyhow::{ Context as _, Result, }; -use astria_core::{ - primitive::v1::Address, - protocol::transaction::v1alpha1::action::IbcRelayerChangeAction, -}; +use astria_core::protocol::transaction::v1alpha1::action::IbcRelayerChangeAction; use async_trait::async_trait; +use cnidarium::StateWrite; use crate::{ - address, - ibc, - transaction::action_handler::ActionHandler, + address::StateReadExt as _, + app::ActionHandler, + ibc::{ + StateReadExt as _, + StateWriteExt as _, + }, + transaction::StateReadExt as _, }; #[async_trait] impl ActionHandler for IbcRelayerChangeAction { - async fn check_stateless(&self) -> Result<()> { + type CheckStatelessContext = (); + + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { Ok(()) } - async fn check_stateful( - &self, - state: &S, - from: Address, - ) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { + let from = state + .get_current_source() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); match self { IbcRelayerChangeAction::Addition(addr) | IbcRelayerChangeAction::Removal(addr) => { state.ensure_base_prefix(addr).await.context( @@ -42,10 +46,7 @@ impl ActionHandler for IbcRelayerChangeAction { ibc_sudo_address == from, "unauthorized address for IBC relayer change" ); - Ok(()) - } - async fn execute(&self, state: &mut S, _from: Address) -> Result<()> { match self { IbcRelayerChangeAction::Addition(address) => { state.put_ibc_relayer_address(address); diff --git a/crates/astria-sequencer/src/ibc/ics20_transfer.rs b/crates/astria-sequencer/src/ibc/ics20_transfer.rs index 1b2dac46f8..3ae8eacfd5 100644 --- a/crates/astria-sequencer/src/ibc/ics20_transfer.rs +++ b/crates/astria-sequencer/src/ibc/ics20_transfer.rs @@ -556,7 +556,7 @@ async fn execute_ics20_transfer_bridge_lock( // check if the recipient is a bridge account; if so, // ensure that the packet memo field (`destination_address`) is set. let is_bridge_lock = state - .get_bridge_account_rollup_id(&recipient) + .get_bridge_account_rollup_id(recipient) .await .context("failed to get bridge account rollup ID from state")? .is_some(); @@ -612,7 +612,7 @@ async fn execute_deposit( // ensure that the asset ID being transferred // to it is allowed. let Some(rollup_id) = state - .get_bridge_account_rollup_id(&bridge_address) + .get_bridge_account_rollup_id(bridge_address) .await .context("failed to get bridge account rollup ID from state")? else { @@ -620,7 +620,7 @@ async fn execute_deposit( }; let allowed_asset = state - .get_bridge_account_ibc_asset(&bridge_address) + .get_bridge_account_ibc_asset(bridge_address) .await .context("failed to get bridge account asset ID")?; ensure!( @@ -765,9 +765,9 @@ mod test { let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); let denom = "dest_port/dest_channel/nootasset".parse::().unwrap(); - state_tx.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); state_tx - .put_bridge_account_ibc_asset(&bridge_address, &denom) + .put_bridge_account_ibc_asset(bridge_address, &denom) .unwrap(); let memo = memos::v1alpha1::Ics20TransferDeposit { @@ -822,9 +822,9 @@ mod test { let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); let denom = "dest_port/dest_channel/nootasset".parse::().unwrap(); - state_tx.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); state_tx - .put_bridge_account_ibc_asset(&bridge_address, &denom) + .put_bridge_account_ibc_asset(bridge_address, &denom) .unwrap(); // use invalid memo, which should fail @@ -860,9 +860,9 @@ mod test { let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); let denom = "dest_port/dest_channel/nootasset".parse::().unwrap(); - state_tx.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); state_tx - .put_bridge_account_ibc_asset(&bridge_address, &denom) + .put_bridge_account_ibc_asset(bridge_address, &denom) .unwrap(); // use invalid asset, which should fail @@ -1000,9 +1000,9 @@ mod test { .parse::() .unwrap(); - state_tx.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); state_tx - .put_bridge_account_ibc_asset(&bridge_address, &denom) + .put_bridge_account_ibc_asset(bridge_address, &denom) .unwrap(); let amount = 100; @@ -1041,9 +1041,9 @@ mod test { let denom = "nootasset".parse::().unwrap(); let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); - state_tx.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); state_tx - .put_bridge_account_ibc_asset(&bridge_address, &denom) + .put_bridge_account_ibc_asset(bridge_address, &denom) .unwrap(); let packet = FungibleTokenPacketData { diff --git a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs index 859ca1d920..34ca3fee80 100644 --- a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs +++ b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs @@ -12,6 +12,10 @@ use astria_core::{ }, protocol::transaction::v1alpha1::action, }; +use cnidarium::{ + StateRead, + StateWrite, +}; use ibc_types::core::channel::{ ChannelId, PortId, @@ -22,17 +26,21 @@ use penumbra_ibc::component::packet::{ SendPacketWrite as _, Unchecked, }; -use tracing::instrument; use crate::{ - accounts, - address, + accounts::{ + AddressBytes, + StateReadExt as _, + StateWriteExt as _, + }, + address::StateReadExt as _, + app::ActionHandler, bridge::StateReadExt as _, ibc::{ StateReadExt as _, StateWriteExt as _, }, - transaction::action_handler::ActionHandler, + transaction::StateReadExt as _, }; fn withdrawal_to_unchecked_ibc_packet( @@ -51,10 +59,10 @@ fn withdrawal_to_unchecked_ibc_packet( ) } -async fn ics20_withdrawal_check_stateful_bridge_account( +async fn ics20_withdrawal_check_stateful_bridge_account( action: &action::Ics20Withdrawal, state: &S, - from: Address, + from: [u8; 20], ) -> Result<()> { // bridge address checks: // - if the sender of this transaction is not a bridge account, and the tx `bridge_address` @@ -65,7 +73,7 @@ async fn ics20_withdrawal_check_stateful_bridge_account Result<()> { + type CheckStatelessContext = (); + + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { ensure!(self.timeout_time() != 0, "timeout time must be non-zero",); // NOTE (from penumbra): we could validate the destination chain address as bech32 to @@ -106,12 +115,12 @@ impl ActionHandler for action::Ics20Withdrawal { Ok(()) } - #[instrument(skip_all)] - async fn check_stateful( - &self, - state: &S, - from: Address, - ) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { + let from = state + .get_current_source() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); + state .ensure_base_prefix(&self.return_address) .await @@ -123,18 +132,20 @@ impl ActionHandler for action::Ics20Withdrawal { )?; } - ics20_withdrawal_check_stateful_bridge_account(self, state, from).await?; + ics20_withdrawal_check_stateful_bridge_account(self, &state, from).await?; let fee = state .get_ics20_withdrawal_base_fee() .await .context("failed to get ics20 withdrawal base fee")?; - let packet: IBCPacket = withdrawal_to_unchecked_ibc_packet(self); - state - .send_packet_check(packet) - .await - .context("packet failed send check")?; + let packet = { + let packet = withdrawal_to_unchecked_ibc_packet(self); + state + .send_packet_check(packet) + .await + .context("packet failed send check")? + }; let transfer_asset = self.denom(); @@ -173,21 +184,6 @@ impl ActionHandler for action::Ics20Withdrawal { ); } - Ok(()) - } - - #[instrument(skip_all)] - async fn execute( - &self, - state: &mut S, - from: Address, - ) -> Result<()> { - let fee = state - .get_ics20_withdrawal_base_fee() - .await - .context("failed to get ics20 withdrawal base fee")?; - let checked_packet = withdrawal_to_unchecked_ibc_packet(self).assume_checked(); - state .decrease_balance(from, self.denom(), self.amount()) .await @@ -200,11 +196,7 @@ impl ActionHandler for action::Ics20Withdrawal { // if we're the source, move tokens to the escrow account, // otherwise the tokens are just burned - if is_source( - checked_packet.source_port(), - checked_packet.source_channel(), - self.denom(), - ) { + if is_source(packet.source_port(), packet.source_channel(), self.denom()) { let channel_balance = state .get_ibc_channel_balance(self.source_channel(), self.denom()) .await @@ -221,7 +213,7 @@ impl ActionHandler for action::Ics20Withdrawal { .context("failed to update channel balance")?; } - state.send_packet_execute(checked_packet).await; + state.send_packet_execute(packet).await; Ok(()) } } @@ -236,13 +228,13 @@ fn is_source(source_port: &PortId, source_channel: &ChannelId, asset: &Denom) -> #[cfg(test)] mod tests { - use address::StateWriteExt; use astria_core::primitive::v1::RollupId; use cnidarium::StateDelta; use ibc_types::core::client::Height; use super::*; use crate::{ + address::StateWriteExt as _, bridge::StateWriteExt as _, test_utils::{ astria_address, @@ -257,13 +249,13 @@ mod tests { let state = StateDelta::new(snapshot); let denom = "test".parse::().unwrap(); - let from = astria_address(&[1u8; 20]); + let from = [1u8; 20]; let action = action::Ics20Withdrawal { amount: 1, denom: denom.clone(), bridge_address: None, destination_chain_address: "test".to_string(), - return_address: from, + return_address: astria_address(&from), timeout_height: Height::new(1, 1).unwrap(), timeout_time: 1, source_channel: "channel-0".to_string().parse().unwrap(), @@ -285,12 +277,12 @@ mod tests { state.put_base_prefix(ASTRIA_PREFIX).unwrap(); // sender is a bridge address, which is also the withdrawer, so it's ok - let bridge_address = astria_address(&[1u8; 20]); + let bridge_address = [1u8; 20]; state.put_bridge_account_rollup_id( - &bridge_address, + bridge_address, &RollupId::from_unhashed_bytes("testrollupid"), ); - state.put_bridge_account_withdrawer_address(&bridge_address, &bridge_address); + state.put_bridge_account_withdrawer_address(bridge_address, bridge_address); let denom = "test".parse::().unwrap(); let action = action::Ics20Withdrawal { @@ -298,7 +290,7 @@ mod tests { denom: denom.clone(), bridge_address: None, destination_chain_address: "test".to_string(), - return_address: bridge_address, + return_address: astria_address(&bridge_address), timeout_height: Height::new(1, 1).unwrap(), timeout_time: 1, source_channel: "channel-0".to_string().parse().unwrap(), @@ -320,12 +312,12 @@ mod tests { state.put_base_prefix(ASTRIA_PREFIX).unwrap(); // withdraw is *not* the bridge address, Ics20Withdrawal must be sent by the withdrawer - let bridge_address = astria_address(&[1u8; 20]); + let bridge_address = [1u8; 20]; state.put_bridge_account_rollup_id( - &bridge_address, + bridge_address, &RollupId::from_unhashed_bytes("testrollupid"), ); - state.put_bridge_account_withdrawer_address(&bridge_address, &astria_address(&[2u8; 20])); + state.put_bridge_account_withdrawer_address(bridge_address, astria_address(&[2u8; 20])); let denom = "test".parse::().unwrap(); let action = action::Ics20Withdrawal { @@ -333,7 +325,7 @@ mod tests { denom: denom.clone(), bridge_address: None, destination_chain_address: "test".to_string(), - return_address: bridge_address, + return_address: astria_address(&bridge_address), timeout_height: Height::new(1, 1).unwrap(), timeout_time: 1, source_channel: "channel-0".to_string().parse().unwrap(), @@ -359,21 +351,21 @@ mod tests { state.put_base_prefix(ASTRIA_PREFIX).unwrap(); // sender the withdrawer address, so it's ok - let bridge_address = astria_address(&[1u8; 20]); - let withdrawer_address = astria_address(&[2u8; 20]); + let bridge_address = [1u8; 20]; + let withdrawer_address = [2u8; 20]; state.put_bridge_account_rollup_id( - &bridge_address, + bridge_address, &RollupId::from_unhashed_bytes("testrollupid"), ); - state.put_bridge_account_withdrawer_address(&bridge_address, &withdrawer_address); + state.put_bridge_account_withdrawer_address(bridge_address, withdrawer_address); let denom = "test".parse::().unwrap(); let action = action::Ics20Withdrawal { amount: 1, denom: denom.clone(), - bridge_address: Some(bridge_address), + bridge_address: Some(astria_address(&bridge_address)), destination_chain_address: "test".to_string(), - return_address: bridge_address, + return_address: astria_address(&bridge_address), timeout_height: Height::new(1, 1).unwrap(), timeout_time: 1, source_channel: "channel-0".to_string().parse().unwrap(), @@ -395,21 +387,21 @@ mod tests { state.put_base_prefix(ASTRIA_PREFIX).unwrap(); // sender is not the withdrawer address, so must fail - let bridge_address = astria_address(&[1u8; 20]); + let bridge_address = [1u8; 20]; let withdrawer_address = astria_address(&[2u8; 20]); state.put_bridge_account_rollup_id( - &bridge_address, + bridge_address, &RollupId::from_unhashed_bytes("testrollupid"), ); - state.put_bridge_account_withdrawer_address(&bridge_address, &withdrawer_address); + state.put_bridge_account_withdrawer_address(bridge_address, withdrawer_address); let denom = "test".parse::().unwrap(); let action = action::Ics20Withdrawal { amount: 1, denom: denom.clone(), - bridge_address: Some(bridge_address), + bridge_address: Some(astria_address(&bridge_address)), destination_chain_address: "test".to_string(), - return_address: bridge_address, + return_address: astria_address(&bridge_address), timeout_height: Height::new(1, 1).unwrap(), timeout_time: 1, source_channel: "channel-0".to_string().parse().unwrap(), @@ -434,15 +426,15 @@ mod tests { let state = StateDelta::new(snapshot); // sender is not the withdrawer address, so must fail - let not_bridge_address = astria_address(&[1u8; 20]); + let not_bridge_address = [1u8; 20]; let denom = "test".parse::().unwrap(); let action = action::Ics20Withdrawal { amount: 1, denom: denom.clone(), - bridge_address: Some(not_bridge_address), + bridge_address: Some(astria_address(¬_bridge_address)), destination_chain_address: "test".to_string(), - return_address: not_bridge_address, + return_address: astria_address(¬_bridge_address), timeout_height: Height::new(1, 1).unwrap(), timeout_time: 1, source_channel: "channel-0".to_string().parse().unwrap(), diff --git a/crates/astria-sequencer/src/ibc/state_ext.rs b/crates/astria-sequencer/src/ibc/state_ext.rs index 3abde83481..880f86f6ba 100644 --- a/crates/astria-sequencer/src/ibc/state_ext.rs +++ b/crates/astria-sequencer/src/ibc/state_ext.rs @@ -5,7 +5,6 @@ use anyhow::{ }; use astria_core::primitive::v1::{ asset, - Address, ADDRESS_LEN, }; use async_trait::async_trait; @@ -23,7 +22,7 @@ use tracing::{ instrument, }; -use crate::address; +use crate::accounts::AddressBytes; /// Newtype wrapper to read and write a u128 from rocksdb. #[derive(BorshSerialize, BorshDeserialize, Debug)] @@ -40,13 +39,13 @@ struct Fee(u128); const IBC_SUDO_STORAGE_KEY: &str = "ibcsudo"; const ICS20_WITHDRAWAL_BASE_FEE_STORAGE_KEY: &str = "ics20withdrawalfee"; -struct IbcRelayerKey<'a>(&'a Address); +struct IbcRelayerKey<'a, T>(&'a T); -impl<'a> std::fmt::Display for IbcRelayerKey<'a> { +impl<'a, T: AddressBytes> std::fmt::Display for IbcRelayerKey<'a, T> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.write_str("ibc-relayer")?; f.write_str("/")?; - for byte in self.0.bytes() { + for byte in self.0.address_bytes() { f.write_fmt(format_args!("{byte:02x}"))?; } Ok(()) @@ -63,12 +62,12 @@ fn channel_balance_storage_key>( ) } -fn ibc_relayer_key(address: &Address) -> String { +fn ibc_relayer_key(address: &T) -> String { IbcRelayerKey(address).to_string() } #[async_trait] -pub(crate) trait StateReadExt: StateRead + address::StateReadExt { +pub(crate) trait StateReadExt: StateRead { #[instrument(skip_all)] async fn get_ibc_channel_balance( &self, @@ -91,7 +90,7 @@ pub(crate) trait StateReadExt: StateRead + address::StateReadExt { } #[instrument(skip_all)] - async fn get_ibc_sudo_address(&self) -> Result
{ + async fn get_ibc_sudo_address(&self) -> Result<[u8; ADDRESS_LEN]> { let Some(bytes) = self .get_raw(IBC_SUDO_STORAGE_KEY) .await @@ -102,16 +101,13 @@ pub(crate) trait StateReadExt: StateRead + address::StateReadExt { }; let SudoAddress(address_bytes) = SudoAddress::try_from_slice(&bytes).context("invalid ibc sudo key bytes")?; - Ok(self.try_base_prefixed(&address_bytes).await.context( - "failed constructing ibc sudo address from address bytes and address prefix stored in \ - state", - )?) + Ok(address_bytes) } #[instrument(skip_all)] - async fn is_ibc_relayer(&self, address: &Address) -> Result { + async fn is_ibc_relayer(&self, address: T) -> Result { Ok(self - .get_raw(&ibc_relayer_key(address)) + .get_raw(&ibc_relayer_key(&address)) .await .context("failed to read ibc relayer key from state")? .is_some()) @@ -151,23 +147,23 @@ pub(crate) trait StateWriteExt: StateWrite { } #[instrument(skip_all)] - fn put_ibc_sudo_address(&mut self, address: Address) -> Result<()> { + fn put_ibc_sudo_address(&mut self, address: T) -> Result<()> { self.put_raw( IBC_SUDO_STORAGE_KEY.to_string(), - borsh::to_vec(&SudoAddress(address.bytes())) + borsh::to_vec(&SudoAddress(address.address_bytes())) .context("failed to convert sudo address to vec")?, ); Ok(()) } #[instrument(skip_all)] - fn put_ibc_relayer_address(&mut self, address: &Address) { - self.put_raw(ibc_relayer_key(address), vec![]); + fn put_ibc_relayer_address(&mut self, address: T) { + self.put_raw(ibc_relayer_key(&address), vec![]); } #[instrument(skip_all)] - fn delete_ibc_relayer_address(&mut self, address: &Address) { - self.delete(ibc_relayer_key(address)); + fn delete_ibc_relayer_address(&mut self, address: T) { + self.delete(ibc_relayer_key(&address)); } #[instrument(skip_all)] @@ -234,7 +230,7 @@ mod tests { state.put_base_prefix(ASTRIA_PREFIX).unwrap(); // can write new - let mut address = astria_address(&[42u8; 20]); + let mut address = [42u8; 20]; state .put_ibc_sudo_address(address) .expect("writing sudo address should not fail"); @@ -248,7 +244,7 @@ mod tests { ); // can rewrite with new value - address = astria_address(&[41u8; 20]); + address = [41u8; 20]; state .put_ibc_sudo_address(address) .expect("writing sudo address should not fail"); @@ -274,7 +270,7 @@ mod tests { let address = astria_address(&[42u8; 20]); assert!( !state - .is_ibc_relayer(&address) + .is_ibc_relayer(address) .await .expect("calls to properly formatted addresses should not fail"), "inputted address should've returned false" @@ -291,20 +287,20 @@ mod tests { // can write let address = astria_address(&[42u8; 20]); - state.put_ibc_relayer_address(&address); + state.put_ibc_relayer_address(address); assert!( state - .is_ibc_relayer(&address) + .is_ibc_relayer(address) .await .expect("a relayer address was written and must exist inside the database"), "stored relayer address could not be verified" ); // can delete - state.delete_ibc_relayer_address(&address); + state.delete_ibc_relayer_address(address); assert!( !state - .is_ibc_relayer(&address) + .is_ibc_relayer(address) .await .expect("calls on unset addresses should not fail"), "relayer address was not deleted as was intended" @@ -321,10 +317,10 @@ mod tests { // can write let address = astria_address(&[42u8; 20]); - state.put_ibc_relayer_address(&address); + state.put_ibc_relayer_address(address); assert!( state - .is_ibc_relayer(&address) + .is_ibc_relayer(address) .await .expect("a relayer address was written and must exist inside the database"), "stored relayer address could not be verified" @@ -332,17 +328,17 @@ mod tests { // can write multiple let address_1 = astria_address(&[41u8; 20]); - state.put_ibc_relayer_address(&address_1); + state.put_ibc_relayer_address(address_1); assert!( state - .is_ibc_relayer(&address_1) + .is_ibc_relayer(address_1) .await .expect("a relayer address was written and must exist inside the database"), "additional stored relayer address could not be verified" ); assert!( state - .is_ibc_relayer(&address) + .is_ibc_relayer(address) .await .expect("a relayer address was written and must exist inside the database"), "original stored relayer address could not be verified" diff --git a/crates/astria-sequencer/src/mempool/benchmarks.rs b/crates/astria-sequencer/src/mempool/benchmarks.rs index 7c97d278c1..33ec33d282 100644 --- a/crates/astria-sequencer/src/mempool/benchmarks.rs +++ b/crates/astria-sequencer/src/mempool/benchmarks.rs @@ -2,7 +2,10 @@ use std::{ collections::HashMap, - sync::OnceLock, + sync::{ + Arc, + OnceLock, + }, time::Duration, }; @@ -155,7 +158,7 @@ fn init_mempool() -> Mempool { let mempool = Mempool::new(); runtime.block_on(async { for tx in transactions().iter().take(T::checked_size()) { - mempool.insert(tx.clone(), 0).await.unwrap(); + mempool.insert(Arc::new(tx.clone()), 0).await.unwrap(); } for i in 0..super::REMOVAL_CACHE_SIZE { let hash = Sha256::digest(i.to_le_bytes()).into(); @@ -193,7 +196,7 @@ fn insert(bencher: divan::Bencher) { .with_inputs(|| (init_mempool::(), get_unused_tx::())) .bench_values(move |(mempool, tx)| { runtime.block_on(async { - mempool.insert(tx, 0).await.unwrap(); + mempool.insert(Arc::new(tx), 0).await.unwrap(); }); }); } diff --git a/crates/astria-sequencer/src/mempool/mod.rs b/crates/astria-sequencer/src/mempool/mod.rs index 6ccf5f9e22..11f3321422 100644 --- a/crates/astria-sequencer/src/mempool/mod.rs +++ b/crates/astria-sequencer/src/mempool/mod.rs @@ -78,16 +78,13 @@ impl PartialOrd for TransactionPriority { pub(crate) struct EnqueuedTransaction { tx_hash: [u8; 32], signed_tx: Arc, - address_bytes: [u8; ADDRESS_LEN], } impl EnqueuedTransaction { - fn new(signed_tx: SignedTransaction) -> Self { - let address_bytes = signed_tx.verification_key().address_bytes(); + fn new(signed_tx: Arc) -> Self { Self { tx_hash: signed_tx.sha256_of_proto_encoding(), - signed_tx: Arc::new(signed_tx), - address_bytes, + signed_tx, } } @@ -118,7 +115,7 @@ impl EnqueuedTransaction { } pub(crate) fn address_bytes(&self) -> [u8; 20] { - self.address_bytes + self.signed_tx.address_bytes() } } @@ -138,10 +135,12 @@ impl std::hash::Hash for EnqueuedTransaction { } } -#[derive(Debug, Clone)] +#[derive(Debug, thiserror::Error)] pub(crate) enum RemovalReason { + #[error("transaction expired in the app's mempool")] Expired, - FailedPrepareProposal(String), + #[error("transaction failed execution because")] + FailedPrepareProposal { source: anyhow::Error }, } const TX_TTL: Duration = Duration::from_secs(600); // 10 minutes @@ -152,7 +151,6 @@ const REMOVAL_CACHE_SIZE: usize = 4096; /// /// This is useful for when a transaction fails execution or when a transaction /// has expired in the app's mempool. -#[derive(Clone)] pub(crate) struct RemovalCache { cache: HashMap<[u8; 32], RemovalReason>, remove_queue: VecDeque<[u8; 32]>, @@ -237,7 +235,7 @@ impl Mempool { #[instrument(skip_all)] pub(crate) async fn insert( &self, - tx: SignedTransaction, + tx: Arc, current_account_nonce: u32, ) -> anyhow::Result<()> { let enqueued_tx = EnqueuedTransaction::new(tx); @@ -301,11 +299,10 @@ impl Mempool { /// removes a transaction from the mempool #[instrument(skip_all)] pub(crate) async fn remove(&self, tx_hash: [u8; 32]) { - let (signed_tx, address_bytes) = dummy_signed_tx(); + let signed_tx = dummy_signed_tx(); let enqueued_tx = EnqueuedTransaction { tx_hash, signed_tx, - address_bytes, }; self.queue.write().await.remove(&enqueued_tx); } @@ -411,26 +408,22 @@ impl Mempool { /// this `signed_tx` field is ignored in the `PartialEq` and `Hash` impls of `EnqueuedTransaction` - /// only the tx hash is considered. So we create an `EnqueuedTransaction` on the fly with the /// correct tx hash and this dummy signed tx when removing from the queue. -fn dummy_signed_tx() -> (Arc, [u8; ADDRESS_LEN]) { - static TX: OnceLock<(Arc, [u8; ADDRESS_LEN])> = OnceLock::new(); - let (signed_tx, address_bytes) = TX.get_or_init(|| { +fn dummy_signed_tx() -> Arc { + static TX: OnceLock> = OnceLock::new(); + let signed_tx = TX.get_or_init(|| { let actions = vec![]; let params = TransactionParams::builder() .nonce(0) .chain_id("dummy") .build(); let signing_key = SigningKey::from([0; 32]); - let address_bytes = signing_key.verification_key().address_bytes(); let unsigned_tx = UnsignedTransaction { actions, params, }; - ( - Arc::new(unsigned_tx.into_signed(&signing_key)), - address_bytes, - ) + Arc::new(unsigned_tx.into_signed(&signing_key)) }); - (signed_tx.clone(), *address_bytes) + signed_tx.clone() } #[cfg(test)] @@ -448,7 +441,7 @@ mod test { #[test] fn transaction_priority_should_error_if_invalid() { - let enqueued_tx = EnqueuedTransaction::new(get_mock_tx(0)); + let enqueued_tx = EnqueuedTransaction::new(Arc::new(get_mock_tx(0))); let priority = enqueued_tx.priority(1, None); assert!( priority @@ -514,17 +507,14 @@ mod test { let tx0 = EnqueuedTransaction { tx_hash: [0; 32], signed_tx: Arc::new(get_mock_tx(0)), - address_bytes: get_mock_tx(0).address_bytes(), }; let other_tx0 = EnqueuedTransaction { tx_hash: [0; 32], signed_tx: Arc::new(get_mock_tx(1)), - address_bytes: get_mock_tx(1).address_bytes(), }; let tx1 = EnqueuedTransaction { tx_hash: [1; 32], signed_tx: Arc::new(get_mock_tx(0)), - address_bytes: get_mock_tx(0).address_bytes(), }; assert!(tx0 == other_tx0); assert!(tx0 != tx1); @@ -544,11 +534,11 @@ mod test { let mempool = Mempool::new(); // Priority 0 (highest priority). - let tx0 = get_mock_tx(0); + let tx0 = Arc::new(get_mock_tx(0)); mempool.insert(tx0.clone(), 0).await.unwrap(); // Priority 1. - let tx1 = get_mock_tx(1); + let tx1 = Arc::new(get_mock_tx(1)); mempool.insert(tx1.clone(), 0).await.unwrap(); assert_eq!(mempool.len().await, 2); @@ -581,7 +571,7 @@ mod test { let txs: Vec<_> = (0..tx_count) .map(|index| { let enqueued_tx = - EnqueuedTransaction::new(get_mock_tx(u32::try_from(index).unwrap())); + EnqueuedTransaction::new(Arc::new(get_mock_tx(u32::try_from(index).unwrap()))); let priority = enqueued_tx.priority(current_account_nonce, None).unwrap(); (enqueued_tx, priority) }) @@ -615,8 +605,8 @@ mod test { let mempool = Mempool::new(); // Insert txs signed by alice with nonces 0 and 1. - mempool.insert(get_mock_tx(0), 0).await.unwrap(); - mempool.insert(get_mock_tx(1), 0).await.unwrap(); + mempool.insert(Arc::new(get_mock_tx(0)), 0).await.unwrap(); + mempool.insert(Arc::new(get_mock_tx(1)), 0).await.unwrap(); // Insert txs from a different signer with nonces 100 and 102. let other = SigningKey::from([1; 32]); @@ -631,8 +621,14 @@ mod test { } .into_signed(&other) }; - mempool.insert(other_mock_tx(100), 0).await.unwrap(); - mempool.insert(other_mock_tx(102), 0).await.unwrap(); + mempool + .insert(Arc::new(other_mock_tx(100)), 0) + .await + .unwrap(); + mempool + .insert(Arc::new(other_mock_tx(102)), 0) + .await + .unwrap(); assert_eq!(mempool.len().await, 4); @@ -681,7 +677,7 @@ mod test { let mempool = Mempool::new(); let insert_time = Instant::now(); - let tx = get_mock_tx(0); + let tx = Arc::new(get_mock_tx(0)); mempool.insert(tx.clone(), 0).await.unwrap(); // pass time @@ -712,7 +708,7 @@ mod test { let mempool = Mempool::new(); let insert_time = Instant::now(); - let tx = get_mock_tx(0); + let tx = Arc::new(get_mock_tx(0)); mempool.insert(tx.clone(), 0).await.unwrap(); // pass time @@ -748,8 +744,8 @@ mod test { let mempool = Mempool::new(); // Insert txs signed by alice with nonces 0 and 1. - mempool.insert(get_mock_tx(0), 0).await.unwrap(); - mempool.insert(get_mock_tx(1), 0).await.unwrap(); + mempool.insert(Arc::new(get_mock_tx(0)), 0).await.unwrap(); + mempool.insert(Arc::new(get_mock_tx(1)), 0).await.unwrap(); // Insert txs from a different signer with nonces 100 and 101. let other = SigningKey::from([1; 32]); @@ -764,8 +760,14 @@ mod test { } .into_signed(&other) }; - mempool.insert(other_mock_tx(100), 0).await.unwrap(); - mempool.insert(other_mock_tx(101), 0).await.unwrap(); + mempool + .insert(Arc::new(other_mock_tx(100)), 0) + .await + .unwrap(); + mempool + .insert(Arc::new(other_mock_tx(101)), 0) + .await + .unwrap(); assert_eq!(mempool.len().await, 4); @@ -828,7 +830,7 @@ mod test { #[test] fn enqueued_transaction_can_be_instantiated() { // This just tests that the constructor does not fail. - let signed_tx = crate::app::test_utils::get_mock_tx(0); + let signed_tx = Arc::new(crate::app::test_utils::get_mock_tx(0)); let _ = EnqueuedTransaction::new(signed_tx); } } diff --git a/crates/astria-sequencer/src/metrics.rs b/crates/astria-sequencer/src/metrics.rs index eadd2365f8..740c6008e1 100644 --- a/crates/astria-sequencer/src/metrics.rs +++ b/crates/astria-sequencer/src/metrics.rs @@ -24,17 +24,11 @@ pub(crate) struct Metrics { proposal_deposits: Histogram, proposal_transactions: Histogram, process_proposal_skipped_proposal: Counter, + check_tx_removed_failed_speculative_deliver_tx: Counter, check_tx_removed_too_large: Counter, check_tx_removed_expired: Counter, - check_tx_removed_failed_execution: Counter, - check_tx_removed_failed_stateless: Counter, - check_tx_removed_stale_nonce: Counter, - check_tx_removed_account_balance: Counter, - check_tx_duration_seconds_parse_tx: Histogram, - check_tx_duration_seconds_check_stateless: Histogram, - check_tx_duration_seconds_check_nonce: Histogram, - check_tx_duration_seconds_check_chain_id: Histogram, - check_tx_duration_seconds_check_balance: Histogram, + check_tx_removed_failed_deliver_tx: Counter, + check_tx_duration_seconds_speculative_deliver_tx: Histogram, check_tx_duration_seconds_check_removed: Histogram, check_tx_duration_seconds_insert_to_app_mempool: Histogram, actions_per_transaction_in_mempool: Histogram, @@ -110,36 +104,21 @@ impl Metrics { let check_tx_removed_too_large = counter!(CHECK_TX_REMOVED_TOO_LARGE); describe_counter!( - CHECK_TX_REMOVED_FAILED_STATELESS, - Unit::Count, - "The number of transactions that have been removed from the mempool due to failing \ - the stateless check" - ); - let check_tx_removed_failed_stateless = counter!(CHECK_TX_REMOVED_FAILED_STATELESS); - - describe_counter!( - CHECK_TX_REMOVED_STALE_NONCE, - Unit::Count, - "The number of transactions that have been removed from the mempool due to having a \ - stale nonce" - ); - let check_tx_removed_stale_nonce = counter!(CHECK_TX_REMOVED_STALE_NONCE); - - describe_counter!( - CHECK_TX_REMOVED_ACCOUNT_BALANCE, + CHECK_TX_REMOVED_FAILED_SPECULATIVE_DELIVER_TX, Unit::Count, - "The number of transactions that have been removed from the mempool due to having not \ - enough account balance" + "The number of transactions that have been removed from the mempool due to failing + a speculative deliver_tx" ); - let check_tx_removed_account_balance = counter!(CHECK_TX_REMOVED_ACCOUNT_BALANCE); + let check_tx_removed_failed_speculative_deliver_tx = + counter!(CHECK_TX_REMOVED_FAILED_SPECULATIVE_DELIVER_TX); describe_counter!( - CHECK_TX_REMOVED_FAILED_EXECUTION, + CHECK_TX_REMOVED_FAILED_DELIVER_TX, Unit::Count, "The number of transactions that have been removed from the mempool due to failing \ - execution in prepare_proposal()" + deliver_tx in prepare_proposal()" ); - let check_tx_removed_failed_execution = counter!(CHECK_TX_REMOVED_FAILED_EXECUTION); + let check_tx_removed_failed_execution = counter!(CHECK_TX_REMOVED_FAILED_DELIVER_TX); describe_counter!( CHECK_TX_REMOVED_EXPIRED, @@ -155,25 +134,9 @@ impl Metrics { "The amount of time taken in seconds to successfully complete the various stages of \ check_tx" ); - let check_tx_duration_seconds_parse_tx = histogram!( + let check_tx_duration_seconds_speculative_deliver_tx = histogram!( CHECK_TX_DURATION_SECONDS, - CHECK_TX_STAGE => "length check and parse raw tx" - ); - let check_tx_duration_seconds_check_stateless = histogram!( - CHECK_TX_DURATION_SECONDS, - CHECK_TX_STAGE => "stateless check" - ); - let check_tx_duration_seconds_check_nonce = histogram!( - CHECK_TX_DURATION_SECONDS, - CHECK_TX_STAGE => "nonce check" - ); - let check_tx_duration_seconds_check_chain_id = histogram!( - CHECK_TX_DURATION_SECONDS, - CHECK_TX_STAGE => "chain id check" - ); - let check_tx_duration_seconds_check_balance = histogram!( - CHECK_TX_DURATION_SECONDS, - CHECK_TX_STAGE => "balance check" + CHECK_TX_STAGE => "speculative deliver tx to the app" ); let check_tx_duration_seconds_check_removed = histogram!( CHECK_TX_DURATION_SECONDS, @@ -213,17 +176,11 @@ impl Metrics { proposal_deposits, proposal_transactions, process_proposal_skipped_proposal, + check_tx_removed_failed_speculative_deliver_tx, check_tx_removed_too_large, check_tx_removed_expired, - check_tx_removed_failed_execution, - check_tx_removed_failed_stateless, - check_tx_removed_stale_nonce, - check_tx_removed_account_balance, - check_tx_duration_seconds_parse_tx, - check_tx_duration_seconds_check_stateless, - check_tx_duration_seconds_check_nonce, - check_tx_duration_seconds_check_chain_id, - check_tx_duration_seconds_check_balance, + check_tx_removed_failed_deliver_tx: check_tx_removed_failed_execution, + check_tx_duration_seconds_speculative_deliver_tx, check_tx_duration_seconds_check_removed, check_tx_duration_seconds_insert_to_app_mempool, actions_per_transaction_in_mempool, @@ -269,6 +226,11 @@ impl Metrics { self.process_proposal_skipped_proposal.increment(1); } + pub(crate) fn increment_check_tx_removed_failed_speculative_deliver_tx(&self) { + self.check_tx_removed_failed_speculative_deliver_tx + .increment(1); + } + pub(crate) fn increment_check_tx_removed_too_large(&self) { self.check_tx_removed_too_large.increment(1); } @@ -278,41 +240,14 @@ impl Metrics { } pub(crate) fn increment_check_tx_removed_failed_execution(&self) { - self.check_tx_removed_failed_execution.increment(1); + self.check_tx_removed_failed_deliver_tx.increment(1); } - pub(crate) fn increment_check_tx_removed_failed_stateless(&self) { - self.check_tx_removed_failed_stateless.increment(1); - } - - pub(crate) fn increment_check_tx_removed_stale_nonce(&self) { - self.check_tx_removed_stale_nonce.increment(1); - } - - pub(crate) fn increment_check_tx_removed_account_balance(&self) { - self.check_tx_removed_account_balance.increment(1); - } - - pub(crate) fn record_check_tx_duration_seconds_parse_tx(&self, duration: Duration) { - self.check_tx_duration_seconds_parse_tx.record(duration); - } - - pub(crate) fn record_check_tx_duration_seconds_check_stateless(&self, duration: Duration) { - self.check_tx_duration_seconds_check_stateless - .record(duration); - } - - pub(crate) fn record_check_tx_duration_seconds_check_nonce(&self, duration: Duration) { - self.check_tx_duration_seconds_check_nonce.record(duration); - } - - pub(crate) fn record_check_tx_duration_seconds_check_chain_id(&self, duration: Duration) { - self.check_tx_duration_seconds_check_chain_id - .record(duration); - } - - pub(crate) fn record_check_tx_duration_seconds_check_balance(&self, duration: Duration) { - self.check_tx_duration_seconds_check_balance + pub(crate) fn record_check_tx_duration_seconds_speculative_deliver_tx( + &self, + duration: Duration, + ) { + self.check_tx_duration_seconds_speculative_deliver_tx .record(duration); } @@ -357,7 +292,8 @@ metric_names!(pub const METRICS_NAMES: PROCESS_PROPOSAL_SKIPPED_PROPOSAL, CHECK_TX_REMOVED_TOO_LARGE, CHECK_TX_REMOVED_EXPIRED, - CHECK_TX_REMOVED_FAILED_EXECUTION, + CHECK_TX_REMOVED_FAILED_SPECULATIVE_DELIVER_TX, + CHECK_TX_REMOVED_FAILED_DELIVER_TX, CHECK_TX_REMOVED_FAILED_STATELESS, CHECK_TX_REMOVED_STALE_NONCE, CHECK_TX_REMOVED_ACCOUNT_BALANCE, @@ -374,7 +310,8 @@ mod tests { CHECK_TX_DURATION_SECONDS, CHECK_TX_REMOVED_ACCOUNT_BALANCE, CHECK_TX_REMOVED_EXPIRED, - CHECK_TX_REMOVED_FAILED_EXECUTION, + CHECK_TX_REMOVED_FAILED_DELIVER_TX, + CHECK_TX_REMOVED_FAILED_SPECULATIVE_DELIVER_TX, CHECK_TX_REMOVED_FAILED_STATELESS, CHECK_TX_REMOVED_STALE_NONCE, CHECK_TX_REMOVED_TOO_LARGE, @@ -421,11 +358,15 @@ mod tests { PROCESS_PROPOSAL_SKIPPED_PROPOSAL, "process_proposal_skipped_proposal", ); + assert_const( + CHECK_TX_REMOVED_FAILED_SPECULATIVE_DELIVER_TX, + "check_tx_removed_failed_speculative_deliver_tx", + ); assert_const(CHECK_TX_REMOVED_TOO_LARGE, "check_tx_removed_too_large"); assert_const(CHECK_TX_REMOVED_EXPIRED, "check_tx_removed_expired"); assert_const( - CHECK_TX_REMOVED_FAILED_EXECUTION, - "check_tx_removed_failed_execution", + CHECK_TX_REMOVED_FAILED_DELIVER_TX, + "check_tx_removed_failed_deliver_tx", ); assert_const( CHECK_TX_REMOVED_FAILED_STATELESS, diff --git a/crates/astria-sequencer/src/sequence/action.rs b/crates/astria-sequencer/src/sequence/action.rs index 5b52bfe760..e81a0f5387 100644 --- a/crates/astria-sequencer/src/sequence/action.rs +++ b/crates/astria-sequencer/src/sequence/action.rs @@ -4,50 +4,30 @@ use anyhow::{ Result, }; use astria_core::{ - primitive::v1::Address, protocol::transaction::v1alpha1::action::SequenceAction, - Protobuf, + Protobuf as _, }; -use tracing::instrument; +use cnidarium::StateWrite; use crate::{ - accounts, accounts::{ + StateReadExt as _, + StateWriteExt as _, + }, + app::ActionHandler, + assets::{ StateReadExt, StateWriteExt, }, - assets, sequence, - transaction::action_handler::ActionHandler, + transaction::StateReadExt as _, }; #[async_trait::async_trait] impl ActionHandler for SequenceAction { - async fn check_stateful( - &self, - state: &S, - from: Address, - ) -> Result<()> - where - S: accounts::StateReadExt + assets::StateReadExt + 'static, - { - ensure!( - state.is_allowed_fee_asset(&self.fee_asset).await?, - "invalid fee asset", - ); + type CheckStatelessContext = (); - let curr_balance = state - .get_account_balance(from, &self.fee_asset) - .await - .context("failed getting `from` account balance for fee payment")?; - let fee = calculate_fee_from_state(&self.data, state) - .await - .context("calculated fee overflows u128")?; - ensure!(curr_balance >= fee, "insufficient funds"); - Ok(()) - } - - async fn check_stateless(&self) -> Result<()> { + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { // TODO: do we want to place a maximum on the size of the data? // https://github.com/astriaorg/astria/issues/222 ensure!( @@ -57,14 +37,28 @@ impl ActionHandler for SequenceAction { Ok(()) } - #[instrument(skip_all)] - async fn execute(&self, state: &mut S, from: Address) -> Result<()> - where - S: assets::StateWriteExt, - { - let fee = calculate_fee_from_state(&self.data, state) + async fn check_and_execute(&self, mut state: S) -> Result<()> { + let from = state + .get_current_source() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); + + ensure!( + state + .is_allowed_fee_asset(&self.fee_asset) + .await + .context("failed accessing state to check if fee is allowed")?, + "invalid fee asset", + ); + + let curr_balance = state + .get_account_balance(from, &self.fee_asset) + .await + .context("failed getting `from` account balance for fee payment")?; + let fee = calculate_fee_from_state(&self.data, &state) .await - .context("failed to calculate fee")?; + .context("calculated fee overflows u128")?; + ensure!(curr_balance >= fee, "insufficient funds"); state .get_and_increase_block_fees(&self.fee_asset, fee, Self::full_name()) diff --git a/crates/astria-sequencer/src/sequencer.rs b/crates/astria-sequencer/src/sequencer.rs index 1e7ef52bc4..3468832bba 100644 --- a/crates/astria-sequencer/src/sequencer.rs +++ b/crates/astria-sequencer/src/sequencer.rs @@ -31,9 +31,7 @@ use tracing::{ }; use crate::{ - address::StateReadExt as _, app::App, - assets::StateReadExt as _, config::Config, grpc::sequencer::SequencerServer, ibc::host_interface::AstriaHost, @@ -84,21 +82,6 @@ impl Sequencer { .context("failed to load storage backing chain state")?; let snapshot = storage.latest_snapshot(); - // the native asset should be configurable only at genesis. - // the genesis state must include the native asset's base - // denomination, and it is set in storage during init_chain. - // on subsequent startups, we load the native asset from storage. - if storage.latest_version() != u64::MAX { - let _ = snapshot - .get_native_asset() - .await - .context("failed to query state for native asset")?; - let _ = snapshot - .get_base_prefix() - .await - .context("failed to query state for base prefix")?; - } - let mempool = Mempool::new(); let app = App::new(snapshot, mempool.clone(), metrics) .await diff --git a/crates/astria-sequencer/src/service/consensus.rs b/crates/astria-sequencer/src/service/consensus.rs index b9da05020e..c617ce04e2 100644 --- a/crates/astria-sequencer/src/service/consensus.rs +++ b/crates/astria-sequencer/src/service/consensus.rs @@ -202,6 +202,7 @@ mod test { use std::{ collections::HashMap, str::FromStr, + sync::Arc, }; use astria_core::{ @@ -287,12 +288,12 @@ mod test { let (mut consensus_service, mempool) = new_consensus_service(Some(signing_key.verification_key())).await; let tx = make_unsigned_tx(); - let signed_tx = tx.into_signed(&signing_key); - let tx_bytes = signed_tx.clone().into_raw().encode_to_vec(); + let signed_tx = Arc::new(tx.into_signed(&signing_key)); + let tx_bytes = signed_tx.clone().to_raw().encode_to_vec(); let txs = vec![tx_bytes.into()]; mempool.insert(signed_tx.clone(), 0).await.unwrap(); - let res = generate_rollup_datas_commitment(&vec![signed_tx], HashMap::new()); + let res = generate_rollup_datas_commitment(&vec![(*signed_tx).clone()], HashMap::new()); let prepare_proposal = new_prepare_proposal_request(); let prepare_proposal_response = consensus_service @@ -492,10 +493,10 @@ mod test { new_consensus_service(Some(signing_key.verification_key())).await; let tx = make_unsigned_tx(); - let signed_tx = tx.into_signed(&signing_key); - let tx_bytes = signed_tx.clone().into_raw().encode_to_vec(); + let signed_tx = Arc::new(tx.into_signed(&signing_key)); + let tx_bytes = signed_tx.to_raw().encode_to_vec(); let txs = vec![tx_bytes.clone().into()]; - let res = generate_rollup_datas_commitment(&vec![signed_tx.clone()], HashMap::new()); + let res = generate_rollup_datas_commitment(&vec![(*signed_tx).clone()], HashMap::new()); let block_data = res.into_transactions(txs.clone()); let data_hash = diff --git a/crates/astria-sequencer/src/service/mempool.rs b/crates/astria-sequencer/src/service/mempool.rs deleted file mode 100644 index 8ce3d114d3..0000000000 --- a/crates/astria-sequencer/src/service/mempool.rs +++ /dev/null @@ -1,300 +0,0 @@ -use std::{ - pin::Pin, - task::{ - Context, - Poll, - }, - time::Instant, -}; - -use anyhow::Context as _; -use astria_core::{ - generated::protocol::transaction::v1alpha1 as raw, - protocol::{ - abci::AbciErrorCode, - transaction::v1alpha1::SignedTransaction, - }, -}; -use cnidarium::Storage; -use futures::{ - Future, - FutureExt, - TryFutureExt as _, -}; -use prost::Message as _; -use tendermint::v0_38::abci::{ - request, - response, - MempoolRequest, - MempoolResponse, -}; -use tower::Service; -use tower_abci::BoxError; -use tracing::{ - instrument, - Instrument as _, -}; - -use crate::{ - accounts, - address, - mempool::{ - Mempool as AppMempool, - RemovalReason, - }, - metrics::Metrics, - transaction, -}; - -const MAX_TX_SIZE: usize = 256_000; // 256 KB - -/// Mempool handles [`request::CheckTx`] abci requests. -// -/// It performs a stateless check of the given transaction, -/// returning a [`tendermint::v0_38::abci::response::CheckTx`]. -#[derive(Clone)] -pub(crate) struct Mempool { - storage: Storage, - inner: AppMempool, - metrics: &'static Metrics, -} - -impl Mempool { - pub(crate) fn new(storage: Storage, mempool: AppMempool, metrics: &'static Metrics) -> Self { - Self { - storage, - inner: mempool, - metrics, - } - } -} - -impl Service for Mempool { - type Error = BoxError; - type Future = Pin> + Send + 'static>>; - type Response = MempoolResponse; - - fn poll_ready(&mut self, _cx: &mut Context<'_>) -> Poll> { - Poll::Ready(Ok(())) - } - - fn call(&mut self, req: MempoolRequest) -> Self::Future { - use penumbra_tower_trace::v038::RequestExt as _; - let span = req.create_span(); - let storage = self.storage.clone(); - let mut mempool = self.inner.clone(); - let metrics = self.metrics; - async move { - let rsp = match req { - MempoolRequest::CheckTx(req) => MempoolResponse::CheckTx( - handle_check_tx(req, storage.latest_snapshot(), &mut mempool, metrics).await, - ), - }; - Ok(rsp) - } - .instrument(span) - .boxed() - } -} - -/// Handles a [`request::CheckTx`] request. -/// -/// Performs stateless checks (decoding and signature check), -/// as well as stateful checks (nonce and balance checks). -/// -/// If the tx passes all checks, status code 0 is returned. -#[allow(clippy::too_many_lines)] -#[instrument(skip_all)] -async fn handle_check_tx( - req: request::CheckTx, - state: S, - mempool: &mut AppMempool, - metrics: &'static Metrics, -) -> response::CheckTx { - use sha2::Digest as _; - - let start_parsing = Instant::now(); - - let request::CheckTx { - tx, .. - } = req; - - let tx_hash = sha2::Sha256::digest(&tx).into(); - let tx_len = tx.len(); - - if tx_len > MAX_TX_SIZE { - mempool.remove(tx_hash).await; - metrics.increment_check_tx_removed_too_large(); - return response::CheckTx { - code: AbciErrorCode::TRANSACTION_TOO_LARGE.into(), - log: format!( - "transaction size too large; allowed: {MAX_TX_SIZE} bytes, got {}", - tx.len() - ), - info: AbciErrorCode::TRANSACTION_TOO_LARGE.to_string(), - ..response::CheckTx::default() - }; - } - - let raw_signed_tx = match raw::SignedTransaction::decode(tx) { - Ok(tx) => tx, - Err(e) => { - mempool.remove(tx_hash).await; - return response::CheckTx { - code: AbciErrorCode::INVALID_PARAMETER.into(), - log: e.to_string(), - info: "failed decoding bytes as a protobuf SignedTransaction".into(), - ..response::CheckTx::default() - }; - } - }; - let signed_tx = match SignedTransaction::try_from_raw(raw_signed_tx) { - Ok(tx) => tx, - Err(e) => { - mempool.remove(tx_hash).await; - return response::CheckTx { - code: AbciErrorCode::INVALID_PARAMETER.into(), - info: "the provided bytes was not a valid protobuf-encoded SignedTransaction, or \ - the signature was invalid" - .into(), - log: e.to_string(), - ..response::CheckTx::default() - }; - } - }; - - let finished_parsing = Instant::now(); - metrics.record_check_tx_duration_seconds_parse_tx( - finished_parsing.saturating_duration_since(start_parsing), - ); - - if let Err(e) = transaction::check_stateless(&signed_tx).await { - mempool.remove(tx_hash).await; - metrics.increment_check_tx_removed_failed_stateless(); - return response::CheckTx { - code: AbciErrorCode::INVALID_PARAMETER.into(), - info: "transaction failed stateless check".into(), - log: e.to_string(), - ..response::CheckTx::default() - }; - }; - - let finished_check_stateless = Instant::now(); - metrics.record_check_tx_duration_seconds_check_stateless( - finished_check_stateless.saturating_duration_since(finished_parsing), - ); - - if let Err(e) = transaction::check_nonce_mempool(&signed_tx, &state).await { - mempool.remove(tx_hash).await; - metrics.increment_check_tx_removed_stale_nonce(); - return response::CheckTx { - code: AbciErrorCode::INVALID_NONCE.into(), - info: "failed verifying transaction nonce".into(), - log: e.to_string(), - ..response::CheckTx::default() - }; - }; - - let finished_check_nonce = Instant::now(); - metrics.record_check_tx_duration_seconds_check_nonce( - finished_check_nonce.saturating_duration_since(finished_check_stateless), - ); - - if let Err(e) = transaction::check_chain_id_mempool(&signed_tx, &state).await { - mempool.remove(tx_hash).await; - return response::CheckTx { - code: AbciErrorCode::INVALID_CHAIN_ID.into(), - info: "failed verifying chain id".into(), - log: e.to_string(), - ..response::CheckTx::default() - }; - } - - let finished_check_chain_id = Instant::now(); - metrics.record_check_tx_duration_seconds_check_chain_id( - finished_check_chain_id.saturating_duration_since(finished_check_nonce), - ); - - if let Err(e) = transaction::check_balance_mempool(&signed_tx, &state).await { - mempool.remove(tx_hash).await; - metrics.increment_check_tx_removed_account_balance(); - return response::CheckTx { - code: AbciErrorCode::INSUFFICIENT_FUNDS.into(), - info: "failed verifying account balance".into(), - log: e.to_string(), - ..response::CheckTx::default() - }; - }; - - let finished_check_balance = Instant::now(); - metrics.record_check_tx_duration_seconds_check_balance( - finished_check_balance.saturating_duration_since(finished_check_chain_id), - ); - - if let Some(removal_reason) = mempool.check_removed_comet_bft(tx_hash).await { - mempool.remove(tx_hash).await; - - match removal_reason { - RemovalReason::Expired => { - metrics.increment_check_tx_removed_expired(); - return response::CheckTx { - code: AbciErrorCode::TRANSACTION_EXPIRED.into(), - info: "transaction expired in app's mempool".into(), - log: "Transaction expired in the app's mempool".into(), - ..response::CheckTx::default() - }; - } - RemovalReason::FailedPrepareProposal(err) => { - metrics.increment_check_tx_removed_failed_execution(); - return response::CheckTx { - code: AbciErrorCode::TRANSACTION_FAILED.into(), - info: "transaction failed execution in prepare_proposal()".into(), - log: format!("transaction failed execution because: {err}"), - ..response::CheckTx::default() - }; - } - } - }; - - let finished_check_removed = Instant::now(); - metrics.record_check_tx_duration_seconds_check_removed( - finished_check_removed.saturating_duration_since(finished_check_balance), - ); - - // tx is valid, push to mempool - let current_account_nonce = match state - .try_base_prefixed(&signed_tx.verification_key().address_bytes()) - .and_then(|address| state.get_account_nonce(address)) - .await - .context("failed fetching nonce for account") - { - Err(err) => { - return response::CheckTx { - code: AbciErrorCode::INTERNAL_ERROR.into(), - info: AbciErrorCode::INTERNAL_ERROR.to_string(), - log: format!("transaction failed execution because: {err:#?}"), - ..response::CheckTx::default() - }; - } - Ok(nonce) => nonce, - }; - - let actions_count = signed_tx.actions().len(); - - mempool - .insert(signed_tx, current_account_nonce) - .await - .expect( - "tx nonce is greater than or equal to current account nonce; this was checked in \ - check_nonce_mempool", - ); - let mempool_len = mempool.len().await; - - metrics - .record_check_tx_duration_seconds_insert_to_app_mempool(finished_check_removed.elapsed()); - metrics.record_actions_per_transaction_in_mempool(actions_count); - metrics.record_transaction_in_mempool_size_bytes(tx_len); - metrics.set_transactions_in_mempool_total(mempool_len); - - response::CheckTx::default() -} diff --git a/crates/astria-sequencer/src/service/mempool/mod.rs b/crates/astria-sequencer/src/service/mempool/mod.rs new file mode 100644 index 0000000000..56107a9a11 --- /dev/null +++ b/crates/astria-sequencer/src/service/mempool/mod.rs @@ -0,0 +1,283 @@ +use std::{ + pin::Pin, + sync::Arc, + task::{ + Context, + Poll, + }, + time::Instant, +}; + +use anyhow::Context as _; +use astria_core::{ + generated::protocol::transaction::v1alpha1 as raw, + protocol::{ + abci::AbciErrorCode, + transaction::v1alpha1::SignedTransaction, + }, +}; +use cnidarium::Storage; +use futures::{ + Future, + FutureExt as _, +}; +use prost::Message as _; +use tendermint::v0_38::abci::{ + request, + response, + MempoolRequest, + MempoolResponse, +}; +use tower::Service; +use tower_abci::BoxError; +use tracing::{ + instrument, + Instrument as _, +}; + +use crate::{ + accounts::StateReadExt as _, + app::App, + mempool::RemovalReason, + metrics::Metrics, + transaction::InvalidNonce, +}; + +#[cfg(test)] +mod tests; + +impl<'a> From<&'a crate::app::TransactionTooLarge> for response::CheckTx { + fn from(value: &'a crate::app::TransactionTooLarge) -> Self { + response::CheckTx { + code: AbciErrorCode::TRANSACTION_TOO_LARGE.into(), + info: AbciErrorCode::TRANSACTION_TOO_LARGE.to_string(), + log: format!("transaction failed execution because: {value:#?}"), + ..response::CheckTx::default() + } + } +} + +impl<'a> From<&'a RemovalReason> for response::CheckTx { + fn from(value: &'a RemovalReason) -> Self { + let code = match value { + RemovalReason::Expired => AbciErrorCode::TRANSACTION_EXPIRED, + RemovalReason::FailedPrepareProposal { + .. + } => AbciErrorCode::TRANSACTION_FAILED, + }; + response::CheckTx { + code: code.into(), + info: code.to_string(), + log: format!("transaction failed execution because: {value:#?}"), + ..response::CheckTx::default() + } + } +} + +fn dynamic_error_to_abci_response( + err: &anyhow::Error, + metrics: &'static Metrics, +) -> response::CheckTx { + if let Some(err) = err.downcast_ref::() { + metrics.increment_check_tx_removed_too_large(); + return err.into(); + } + if let Some(err) = err.downcast_ref::() { + match &err { + RemovalReason::Expired => metrics.increment_check_tx_removed_expired(), + RemovalReason::FailedPrepareProposal { + .. + } => metrics.increment_check_tx_removed_failed_execution(), + } + return err.into(); + } + // FIXME: this is used as a catch-all right now, even though "internal error" + // might be misleading or wrong. Need to figure out how to map the + // currently opaque tx.check_and_execute to specific abci error codes. + metrics.increment_check_tx_removed_failed_speculative_deliver_tx(); + response::CheckTx { + code: AbciErrorCode::INTERNAL_ERROR.into(), + info: AbciErrorCode::INTERNAL_ERROR.to_string(), + log: format!("transaction failed execution because: {err:#?}"), + ..response::CheckTx::default() + } +} + +/// Mempool handles [`request::CheckTx`] abci requests. +// +/// It performs a stateless check of the given transaction, +/// returning a [`tendermint::v0_38::abci::response::CheckTx`]. +#[derive(Clone)] +pub(crate) struct Mempool { + storage: Storage, + inner: crate::mempool::Mempool, + metrics: &'static Metrics, +} + +impl Mempool { + pub(crate) fn new( + storage: Storage, + mempool: crate::mempool::Mempool, + metrics: &'static Metrics, + ) -> Self { + Self { + storage, + inner: mempool, + metrics, + } + } +} + +impl Service for Mempool { + type Error = BoxError; + type Future = Pin> + Send + 'static>>; + type Response = MempoolResponse; + + fn poll_ready(&mut self, _cx: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) + } + + fn call(&mut self, req: MempoolRequest) -> Self::Future { + use penumbra_tower_trace::v038::RequestExt as _; + let span = req.create_span(); + let storage = self.storage.clone(); + let app_mempool = self.inner.clone(); + let metrics = self.metrics; + async move { + let rsp = match req { + MempoolRequest::CheckTx(req) => MempoolResponse::CheckTx( + handle_check_tx(req, storage, app_mempool, metrics).await, + ), + }; + Ok(rsp) + } + .instrument(span) + .boxed() + } +} + +/// Handles a [`request::CheckTx`] request. +/// +/// Performs stateless checks (decoding and signature check), +/// as well as stateful checks (nonce and balance checks). +/// +/// If the tx passes all checks, status code 0 is returned. +#[allow(clippy::too_many_lines)] +#[instrument(skip_all)] +async fn handle_check_tx( + req: request::CheckTx, + storage: Storage, + mempool: crate::mempool::Mempool, + metrics: &'static Metrics, +) -> response::CheckTx { + use sha2::Digest as _; + + let request::CheckTx { + tx: bytes, .. + } = req; + + let tx_hash = sha2::Sha256::digest(&bytes).into(); + + let finished_speculative_deliver_tx = Instant::now(); + let snapshot = storage.latest_snapshot(); + let mut app = App::new(snapshot.clone(), mempool.clone(), metrics) + .await + .unwrap(); + + let (the_tx, _) = match app.deliver_tx_bytes(&bytes).await { + Err(mut err) => { + if let Some(current_nonce) = find_invalid_nonce_error(&err) + .and_then(|invalid_nonce| invalid_nonce.is_ahead().then_some(invalid_nonce.current)) + { + let signed_tx = transaction_from_bytes_unchecked(&bytes); + if let Err(mempool_error) = mempool + .insert(Arc::new(signed_tx), current_nonce) + .await + .context("mempool rejected transaction with future nonce") + { + // override the outer arror and fall down to the general handler + err = mempool_error; + } else { + return response::CheckTx::default(); + } + } + return dynamic_error_to_abci_response(&err, metrics); + } + Ok(ret) => ret, + }; + + metrics.record_check_tx_duration_seconds_speculative_deliver_tx( + finished_speculative_deliver_tx.elapsed(), + ); + + if let Some(removal_reason) = mempool.check_removed_comet_bft(tx_hash).await { + mempool.remove(tx_hash).await; + return dynamic_error_to_abci_response(&anyhow::Error::new(removal_reason), metrics); + }; + + let finished_check_removed = Instant::now(); + metrics.record_check_tx_duration_seconds_check_removed( + finished_check_removed.saturating_duration_since(finished_speculative_deliver_tx), + ); + + // tx is valid, push to mempool + let current_account_nonce = match snapshot + .get_account_nonce(the_tx.address_bytes()) + .await + .context("failed fetching nonce for transaction signer") + { + Err(err) => { + return response::CheckTx { + code: AbciErrorCode::INTERNAL_ERROR.into(), + info: AbciErrorCode::INTERNAL_ERROR.to_string(), + log: format!("transaction failed execution because: {err:#?}"), + ..response::CheckTx::default() + }; + } + Ok(nonce) => nonce, + }; + + if let Err(err) = mempool + .insert(the_tx.clone(), current_account_nonce) + .await + .context("mempool rejected validated transaction") + { + return response::CheckTx { + code: AbciErrorCode::INTERNAL_ERROR.into(), + info: AbciErrorCode::INTERNAL_ERROR.to_string(), + log: format!("transaction failed execution because: {err:#?}"), + ..response::CheckTx::default() + }; + } + let mempool_len = mempool.len().await; + + metrics + .record_check_tx_duration_seconds_insert_to_app_mempool(finished_check_removed.elapsed()); + metrics.record_actions_per_transaction_in_mempool(the_tx.actions().len()); + metrics.record_transaction_in_mempool_size_bytes(bytes.len()); + metrics.set_transactions_in_mempool_total(mempool_len); + + response::CheckTx::default() +} + +fn find_invalid_nonce_error(error: &anyhow::Error) -> Option<&InvalidNonce> { + for cause in error.chain() { + if let Some(invalid_nonce) = cause.downcast_ref::() { + return Some(invalid_nonce); + } + } + None +} + +/// Constructs a signed transaction from bytes, panicking if decoding +/// the protobuf bytes failed or if the transaction was malformed. +fn transaction_from_bytes_unchecked(bytes: &[u8]) -> SignedTransaction { + let proto = raw::SignedTransaction::decode(bytes).expect( + "an invalid nonce was established which only makes sense if the transaction was \ + successfully decoded", + ); + SignedTransaction::try_from_raw(proto).expect( + "an invalid nonce was established which only makes sense if the transaction was well \ + formed", + ) +} diff --git a/crates/astria-sequencer/src/service/mempool/tests.rs b/crates/astria-sequencer/src/service/mempool/tests.rs new file mode 100644 index 0000000000..3ec00db6be --- /dev/null +++ b/crates/astria-sequencer/src/service/mempool/tests.rs @@ -0,0 +1,93 @@ +use prost::Message as _; +use tendermint::{ + abci::{ + request::CheckTxKind, + Code, + }, + v0_38::abci::request::CheckTx, +}; + +use crate::{ + app::{ + test_utils::get_mock_tx, + App, + }, + mempool::Mempool, + metrics::Metrics, +}; + +#[tokio::test] +async fn transaction_with_future_nonce_enters_mempool() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + + let mempool = Mempool::new(); + let metrics = Box::leak(Box::new(Metrics::new())); + let mut app = App::new(snapshot, mempool, metrics).await.unwrap(); + + let genesis_state = crate::app::test_utils::genesis_state(); + + app.init_chain(storage.clone(), genesis_state, vec![], "test".to_string()) + .await + .unwrap(); + app.commit(storage.clone()).await; + + let mempool = Mempool::new(); + let metrics = Box::leak(Box::new(Metrics::new())); + + let the_future_nonce = 10; + let tx = get_mock_tx(the_future_nonce); + let req = CheckTx { + tx: tx.to_raw().encode_to_vec().into(), + kind: CheckTxKind::New, + }; + let rsp = super::handle_check_tx(req, storage.clone(), mempool.clone(), metrics).await; + assert_eq!(rsp.code, Code::Ok, "{rsp:#?}"); + // with the current mempool implementation we can't directly observe the transaction; + // but we can check the pending nonce of the signer. + assert_eq!( + mempool.pending_nonce(tx.address_bytes()).await.expect( + "signer should have a pending nonce because the transaction should have made it into \ + the pool" + ), + the_future_nonce, + ); +} + +#[tokio::test] +async fn valid_transaction_enters_mempool() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + + let mempool = Mempool::new(); + let metrics = Box::leak(Box::new(Metrics::new())); + let mut app = App::new(snapshot, mempool, metrics).await.unwrap(); + + let genesis_state = crate::app::test_utils::genesis_state(); + + app.init_chain(storage.clone(), genesis_state, vec![], "test".to_string()) + .await + .unwrap(); + app.commit(storage.clone()).await; + + let mempool = Mempool::new(); + let metrics = Box::leak(Box::new(Metrics::new())); + + let nonce = 0; + let tx = get_mock_tx(nonce); + let req = CheckTx { + tx: tx.to_raw().encode_to_vec().into(), + kind: CheckTxKind::New, + }; + let rsp = super::handle_check_tx(req, storage.clone(), mempool.clone(), metrics).await; + assert_eq!(rsp.code, Code::Ok, "{rsp:#?}"); + // with the current mempool implementation we can't directly observe the transaction; + // but we can check the pending nonce of the signer. + assert_eq!( + mempool.pending_nonce(tx.address_bytes()).await.expect( + "signer should have a pending nonce because the transaction should have made it into \ + the pool" + ), + nonce, + ); +} diff --git a/crates/astria-sequencer/src/test_utils.rs b/crates/astria-sequencer/src/test_utils.rs index 24a078e30f..c25099ad52 100644 --- a/crates/astria-sequencer/src/test_utils.rs +++ b/crates/astria-sequencer/src/test_utils.rs @@ -38,3 +38,12 @@ pub(crate) fn verification_key(seed: u64) -> VerificationKey { let signing_key = SigningKey::new(rng); signing_key.verification_key() } + +#[track_caller] +pub(crate) fn assert_anyhow_error(error: &anyhow::Error, expected: &'static str) { + let msg = error.to_string(); + assert!( + msg.contains(expected), + "error contained different message\n\texpected: {expected}\n\tfull_error: {msg}", + ); +} diff --git a/crates/astria-sequencer/src/transaction/action_handler.rs b/crates/astria-sequencer/src/transaction/action_handler.rs deleted file mode 100644 index 17cfb7345b..0000000000 --- a/crates/astria-sequencer/src/transaction/action_handler.rs +++ /dev/null @@ -1,24 +0,0 @@ -use anyhow::Result; -use astria_core::primitive::v1::Address; -use async_trait::async_trait; -use cnidarium::{ - StateRead, - StateWrite, -}; - -#[async_trait] -pub(crate) trait ActionHandler { - async fn check_stateless(&self) -> Result<()> { - Ok(()) - } - async fn check_stateful( - &self, - _state: &S, - _from: Address, - ) -> Result<()> { - Ok(()) - } - async fn execute(&self, _state: &mut S, _from: Address) -> Result<()> { - Ok(()) - } -} diff --git a/crates/astria-sequencer/src/transaction/checks.rs b/crates/astria-sequencer/src/transaction/checks.rs index f96f26ebd3..83e26e6fd2 100644 --- a/crates/astria-sequencer/src/transaction/checks.rs +++ b/crates/astria-sequencer/src/transaction/checks.rs @@ -7,7 +7,6 @@ use anyhow::{ use astria_core::{ primitive::v1::{ asset, - Address, RollupId, }, protocol::transaction::v1alpha1::{ @@ -19,75 +18,17 @@ use astria_core::{ UnsignedTransaction, }, }; +use cnidarium::StateRead; use tracing::instrument; use crate::{ - accounts, - address, + accounts::StateReadExt as _, bridge::StateReadExt as _, ibc::StateReadExt as _, - state_ext::StateReadExt as _, }; #[instrument(skip_all)] -pub(crate) async fn check_nonce_mempool(tx: &SignedTransaction, state: &S) -> anyhow::Result<()> -where - S: accounts::StateReadExt + address::StateReadExt + 'static, -{ - let signer_address = state - .try_base_prefixed(&tx.verification_key().address_bytes()) - .await - .context( - "failed constructing the signer address from signed transaction verification and \ - prefix provided by app state", - )?; - let curr_nonce = state - .get_account_nonce(signer_address) - .await - .context("failed to get account nonce")?; - ensure!(tx.nonce() >= curr_nonce, "nonce already used by account"); - Ok(()) -} - -#[instrument(skip_all)] -pub(crate) async fn check_chain_id_mempool( - tx: &SignedTransaction, - state: &S, -) -> anyhow::Result<()> -where - S: accounts::StateReadExt + 'static, -{ - let chain_id = state - .get_chain_id() - .await - .context("failed to get chain id")?; - ensure!(tx.chain_id() == chain_id.as_str(), "chain id mismatch"); - Ok(()) -} - -#[instrument(skip_all)] -pub(crate) async fn check_balance_mempool( - tx: &SignedTransaction, - state: &S, -) -> anyhow::Result<()> -where - S: accounts::StateReadExt + address::StateReadExt + 'static, -{ - let signer_address = state - .try_base_prefixed(&tx.verification_key().address_bytes()) - .await - .context( - "failed constructing the signer address from signed transaction verification and \ - prefix provided by app state", - )?; - check_balance_for_total_fees_and_transfers(tx.unsigned_transaction(), signer_address, state) - .await - .context("failed to check balance for total fees and transfers")?; - Ok(()) -} - -#[instrument(skip_all)] -pub(crate) async fn get_fees_for_transaction( +pub(crate) async fn get_fees_for_transaction( tx: &UnsignedTransaction, state: &S, ) -> anyhow::Result> { @@ -163,20 +104,16 @@ pub(crate) async fn get_fees_for_transaction( - tx: &UnsignedTransaction, - from: Address, +pub(crate) async fn check_balance_for_total_fees_and_transfers( + tx: &SignedTransaction, state: &S, -) -> anyhow::Result<()> -where - S: accounts::StateReadExt + 'static, -{ - let mut cost_by_asset = get_fees_for_transaction(tx, state) +) -> anyhow::Result<()> { + let mut cost_by_asset = get_fees_for_transaction(tx.unsigned_transaction(), state) .await .context("failed to get fees for transaction")?; // add values transferred within the tx to the cost - for action in &tx.actions { + for action in tx.actions() { match action { Action::Transfer(act) => { cost_by_asset @@ -198,7 +135,7 @@ where } Action::BridgeUnlock(act) => { let asset = state - .get_bridge_account_ibc_asset(&from) + .get_bridge_account_ibc_asset(tx) .await .context("failed to get bridge account asset id")?; cost_by_asset @@ -222,7 +159,7 @@ where for (asset, total_fee) in cost_by_asset { let balance = state - .get_account_balance(from, asset) + .get_account_balance(tx, asset) .await .context("failed to get account balance")?; ensure!( @@ -246,15 +183,12 @@ fn transfer_update_fees( .or_insert(transfer_fee); } -async fn sequence_update_fees( +async fn sequence_update_fees( state: &S, fee_asset: &asset::Denom, fees_by_asset: &mut HashMap, data: &[u8], -) -> anyhow::Result<()> -where - S: accounts::StateReadExt, -{ +) -> anyhow::Result<()> { let fee = crate::sequence::calculate_fee_from_state(data, state) .await .context("fee for sequence action overflowed; data too large")?; @@ -312,188 +246,3 @@ fn bridge_unlock_update_fees( .and_modify(|amt| *amt = amt.saturating_add(transfer_fee)) .or_insert(transfer_fee); } - -#[cfg(test)] -mod tests { - use address::{ - StateReadExt, - StateWriteExt, - }; - use astria_core::{ - primitive::v1::{ - asset::Denom, - RollupId, - ADDRESS_LEN, - }, - protocol::transaction::v1alpha1::{ - action::{ - SequenceAction, - TransferAction, - }, - TransactionParams, - }, - }; - use cnidarium::StateDelta; - - use super::*; - use crate::{ - accounts::{ - StateReadExt as _, - StateWriteExt as _, - }, - app::test_utils::*, - assets::StateWriteExt as _, - bridge::StateWriteExt as _, - ibc::StateWriteExt as _, - sequence::StateWriteExt as _, - }; - - #[tokio::test] - async fn check_balance_mempool_ok() { - let storage = cnidarium::TempStorage::new().await.unwrap(); - let snapshot = storage.latest_snapshot(); - let mut state_tx = StateDelta::new(snapshot); - - state_tx.put_base_prefix("astria").unwrap(); - state_tx.put_native_asset(&crate::test_utils::nria()); - state_tx.put_transfer_base_fee(12).unwrap(); - state_tx.put_sequence_action_base_fee(0); - state_tx.put_sequence_action_byte_cost_multiplier(1); - state_tx.put_ics20_withdrawal_base_fee(1).unwrap(); - state_tx.put_init_bridge_account_base_fee(12); - state_tx.put_bridge_lock_byte_cost_multiplier(1); - state_tx.put_bridge_sudo_change_base_fee(24); - - let other_asset = "other".parse::().unwrap(); - - let alice = get_alice_signing_key(); - let amount = 100; - let data = [0; 32].to_vec(); - let transfer_fee = state_tx.get_transfer_base_fee().await.unwrap(); - state_tx - .increase_balance( - state_tx - .try_base_prefixed(&alice.address_bytes()) - .await - .unwrap(), - crate::test_utils::nria(), - transfer_fee - + crate::sequence::calculate_fee_from_state(&data, &state_tx) - .await - .unwrap(), - ) - .await - .unwrap(); - state_tx - .increase_balance( - state_tx - .try_base_prefixed(&alice.address_bytes()) - .await - .unwrap(), - &other_asset, - amount, - ) - .await - .unwrap(); - - let actions = vec![ - Action::Transfer(TransferAction { - asset: other_asset.clone(), - amount, - fee_asset: crate::test_utils::nria().into(), - to: state_tx.try_base_prefixed(&[0; ADDRESS_LEN]).await.unwrap(), - }), - Action::Sequence(SequenceAction { - rollup_id: RollupId::from_unhashed_bytes([0; 32]), - data, - fee_asset: crate::test_utils::nria().into(), - }), - ]; - - let params = TransactionParams::builder() - .nonce(0) - .chain_id("test-chain-id") - .build(); - let tx = UnsignedTransaction { - actions, - params, - }; - - let signed_tx = tx.into_signed(&alice); - check_balance_mempool(&signed_tx, &state_tx) - .await - .expect("sufficient balance for all actions"); - } - - #[tokio::test] - async fn check_balance_mempool_insufficient_other_asset_balance() { - let storage = cnidarium::TempStorage::new().await.unwrap(); - let snapshot = storage.latest_snapshot(); - let mut state_tx = StateDelta::new(snapshot); - - state_tx.put_base_prefix("nria").unwrap(); - state_tx.put_native_asset(&crate::test_utils::nria()); - state_tx.put_transfer_base_fee(12).unwrap(); - state_tx.put_sequence_action_base_fee(0); - state_tx.put_sequence_action_byte_cost_multiplier(1); - state_tx.put_ics20_withdrawal_base_fee(1).unwrap(); - state_tx.put_init_bridge_account_base_fee(12); - state_tx.put_bridge_lock_byte_cost_multiplier(1); - state_tx.put_bridge_sudo_change_base_fee(24); - - let other_asset = "other".parse::().unwrap(); - - let alice = get_alice_signing_key(); - let amount = 100; - let data = [0; 32].to_vec(); - let transfer_fee = state_tx.get_transfer_base_fee().await.unwrap(); - state_tx - .increase_balance( - state_tx - .try_base_prefixed(&alice.address_bytes()) - .await - .unwrap(), - crate::test_utils::nria(), - transfer_fee - + crate::sequence::calculate_fee_from_state(&data, &state_tx) - .await - .unwrap(), - ) - .await - .unwrap(); - - let actions = vec![ - Action::Transfer(TransferAction { - asset: other_asset.clone(), - amount, - fee_asset: crate::test_utils::nria().into(), - to: state_tx.try_base_prefixed(&[0; ADDRESS_LEN]).await.unwrap(), - }), - Action::Sequence(SequenceAction { - rollup_id: RollupId::from_unhashed_bytes([0; 32]), - data, - fee_asset: crate::test_utils::nria().into(), - }), - ]; - - let params = TransactionParams::builder() - .nonce(0) - .chain_id("test-chain-id") - .build(); - let tx = UnsignedTransaction { - actions, - params, - }; - - let signed_tx = tx.into_signed(&alice); - let err = check_balance_mempool(&signed_tx, &state_tx) - .await - .err() - .unwrap(); - assert!( - err.root_cause() - .to_string() - .contains(&other_asset.to_ibc_prefixed().to_string()) - ); - } -} diff --git a/crates/astria-sequencer/src/transaction/mod.rs b/crates/astria-sequencer/src/transaction/mod.rs index 1426a91804..9132e9bbbb 100644 --- a/crates/astria-sequencer/src/transaction/mod.rs +++ b/crates/astria-sequencer/src/transaction/mod.rs @@ -1,34 +1,42 @@ -pub(crate) mod action_handler; mod checks; pub(crate) mod query; +mod state_ext; use std::fmt; -pub(crate) use action_handler::ActionHandler; use anyhow::{ ensure, Context as _, }; -use astria_core::{ - primitive::v1::Address, - protocol::transaction::v1alpha1::{ - action::Action, - SignedTransaction, - UnsignedTransaction, - }, +use astria_core::protocol::transaction::v1alpha1::{ + action::Action, + SignedTransaction, }; -pub(crate) use checks::{ - check_balance_for_total_fees_and_transfers, - check_balance_mempool, - check_chain_id_mempool, - check_nonce_mempool, +pub(crate) use checks::check_balance_for_total_fees_and_transfers; +use cnidarium::StateWrite; + +#[cfg(test)] +mod tests; + +// Conditional to quiet warnings. This object is used throughout the codebase, +// but is never explicitly named - hence Rust warns about it being unused. +#[cfg(test)] +pub(crate) use state_ext::TransactionContext; +pub(crate) use state_ext::{ + StateReadExt, + StateWriteExt, }; -use tracing::instrument; use crate::{ - accounts, - address, - bridge, + accounts::{ + StateReadExt as _, + StateWriteExt as _, + }, + app::ActionHandler, + bridge::{ + StateReadExt as _, + StateWriteExt as _, + }, ibc::{ host_interface::AstriaHost, StateReadExt as _, @@ -36,64 +44,6 @@ use crate::{ state_ext::StateReadExt as _, }; -#[instrument(skip_all)] -pub(crate) async fn check_stateless(tx: &SignedTransaction) -> anyhow::Result<()> { - tx.unsigned_transaction() - .check_stateless() - .await - .context("stateless check failed") -} - -#[instrument(skip_all)] -pub(crate) async fn check_stateful(tx: &SignedTransaction, state: &S) -> anyhow::Result<()> -where - S: accounts::StateReadExt + address::StateReadExt + 'static, -{ - let signer_address = state - .try_base_prefixed(&tx.verification_key().address_bytes()) - .await - .context( - "failed constructing signed address from state and verification key contained in \ - signed transaction", - )?; - tx.unsigned_transaction() - .check_stateful(state, signer_address) - .await -} - -pub(crate) async fn execute(tx: &SignedTransaction, state: &mut S) -> anyhow::Result<()> -where - S: accounts::StateReadExt - + accounts::StateWriteExt - + address::StateReadExt - + bridge::StateReadExt - + bridge::StateWriteExt, -{ - let signer_address = state - .try_base_prefixed(&tx.verification_key().address_bytes()) - .await - .context( - "failed constructing signed address from state and verification key contained in \ - signed transaction", - )?; - - if state - .get_bridge_account_rollup_id(&signer_address) - .await - .context("failed to check account rollup id")? - .is_some() - { - state.put_last_transaction_hash_for_bridge_account( - &signer_address, - &tx.sha256_of_proto_encoding(), - ); - } - - tx.unsigned_transaction() - .execute(state, signer_address) - .await -} - #[derive(Debug)] pub(crate) struct InvalidChainId(pub(crate) String); @@ -109,46 +59,46 @@ impl fmt::Display for InvalidChainId { impl std::error::Error for InvalidChainId {} -#[derive(Debug)] -pub(crate) struct InvalidNonce(pub(crate) u32); +#[derive(Debug, thiserror::Error, PartialEq)] +#[error("expected current nonce `{current}`, but transaction contained `{in_transaction}`")] +pub(crate) struct InvalidNonce { + pub(crate) current: u32, + pub(crate) in_transaction: u32, +} -impl fmt::Display for InvalidNonce { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!( - f, - "provided nonce {} does not match expected next nonce", - self.0, - ) +impl InvalidNonce { + pub(crate) fn is_ahead(&self) -> bool { + self.in_transaction > self.current } } -impl std::error::Error for InvalidNonce {} - #[async_trait::async_trait] -impl ActionHandler for UnsignedTransaction { - async fn check_stateless(&self) -> anyhow::Result<()> { - ensure!(!self.actions.is_empty(), "must have at least one action"); +impl ActionHandler for SignedTransaction { + type CheckStatelessContext = (); + + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> anyhow::Result<()> { + ensure!(!self.actions().is_empty(), "must have at least one action"); - for action in &self.actions { + for action in self.actions() { match action { Action::Transfer(act) => act - .check_stateless() + .check_stateless(()) .await .context("stateless check failed for TransferAction")?, Action::Sequence(act) => act - .check_stateless() + .check_stateless(()) .await .context("stateless check failed for SequenceAction")?, Action::ValidatorUpdate(act) => act - .check_stateless() + .check_stateless(()) .await .context("stateless check failed for ValidatorUpdateAction")?, Action::SudoAddressChange(act) => act - .check_stateless() + .check_stateless(()) .await .context("stateless check failed for SudoAddressChangeAction")?, Action::FeeChange(act) => act - .check_stateless() + .check_stateless(()) .await .context("stateless check failed for FeeChangeAction")?, Action::Ibc(act) => { @@ -161,31 +111,31 @@ impl ActionHandler for UnsignedTransaction { .context("stateless check failed for IbcAction")?; } Action::Ics20Withdrawal(act) => act - .check_stateless() + .check_stateless(()) .await .context("stateless check failed for Ics20WithdrawalAction")?, Action::IbcRelayerChange(act) => act - .check_stateless() + .check_stateless(()) .await .context("stateless check failed for IbcRelayerChangeAction")?, Action::FeeAssetChange(act) => act - .check_stateless() + .check_stateless(()) .await .context("stateless check failed for FeeAssetChangeAction")?, Action::InitBridgeAccount(act) => act - .check_stateless() + .check_stateless(()) .await .context("stateless check failed for InitBridgeAccountAction")?, Action::BridgeLock(act) => act - .check_stateless() + .check_stateless(()) .await .context("stateless check failed for BridgeLockAction")?, Action::BridgeUnlock(act) => act - .check_stateless() + .check_stateless(()) .await .context("stateless check failed for BridgeLockAction")?, Action::BridgeSudoChange(act) => act - .check_stateless() + .check_stateless(()) .await .context("stateless check failed for BridgeSudoChangeAction")?, } @@ -193,10 +143,16 @@ impl ActionHandler for UnsignedTransaction { Ok(()) } - async fn check_stateful(&self, state: &S, from: Address) -> anyhow::Result<()> - where - S: accounts::StateReadExt + 'static, - { + // allowed / FIXME: because most lines come from delegating (and error wrapping) to the + // individual actions. This could be tidied up by implementing `ActionHandler for Action` + // and letting it delegate. + #[allow(clippy::too_many_lines)] + async fn check_and_execute(&self, mut state: S) -> anyhow::Result<()> { + // Add the current signed transaction into the ephemeral state in case + // downstream actions require access to it. + // XXX: This must be deleted at the end of `check_stateful`. + state.put_current_source(self); + // Transactions must match the chain id of the node. let chain_id = state.get_chain_id().await?; ensure!( @@ -204,171 +160,122 @@ impl ActionHandler for UnsignedTransaction { InvalidChainId(self.chain_id().to_string()) ); - // Nonce should be equal to the number of executed transactions before this tx. - // First tx has nonce 0. - let curr_nonce = state.get_account_nonce(from).await?; - ensure!(curr_nonce == self.nonce(), InvalidNonce(self.nonce())); + let current_account_nonce = state + .get_account_nonce(self.address_bytes()) + .await + .context("failed to get nonce for transaction signer")?; + ensure!( + current_account_nonce == self.nonce(), + InvalidNonce { + current: current_account_nonce, + in_transaction: self.nonce(), + } + ); // Should have enough balance to cover all actions. - check_balance_for_total_fees_and_transfers(self, from, state) + check_balance_for_total_fees_and_transfers(self, &state) .await .context("failed to check balance for total fees and transfers")?; - for action in &self.actions { + if state + .get_bridge_account_rollup_id(self) + .await + .context("failed to check account rollup id")? + .is_some() + { + state.put_last_transaction_hash_for_bridge_account( + self, + &self.sha256_of_proto_encoding(), + ); + } + + let from_nonce = state + .get_account_nonce(self) + .await + .context("failed getting nonce of transaction signer")?; + let next_nonce = from_nonce + .checked_add(1) + .context("overflow occurred incrementing stored nonce")?; + state + .put_account_nonce(self, next_nonce) + .context("failed updating `from` nonce")?; + + // FIXME: this should create one span per `check_and_execute` + for action in self.actions() { match action { Action::Transfer(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await - .context("stateful check failed for TransferAction")?, + .context("executing transfer action failed")?, Action::Sequence(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await - .context("stateful check failed for SequenceAction")?, + .context("executing sequence action failed")?, Action::ValidatorUpdate(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await - .context("stateful check failed for ValidatorUpdateAction")?, + .context("executing validor update")?, Action::SudoAddressChange(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await - .context("stateful check failed for SudoAddressChangeAction")?, + .context("executing sudo address change failed")?, Action::FeeChange(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await - .context("stateful check failed for FeeChangeAction")?, - Action::Ibc(_) => { + .context("executing fee change failed")?, + Action::Ibc(act) => { + // FIXME: this check should be moved to check_and_execute, as it now has + // access to the the signer through state. However, what's the correct + // ibc AppHandler call to do it? Can we just update one of the trait methods + // of crate::ibc::ics20_transfer::Ics20Transfer? ensure!( state - .is_ibc_relayer(&from) + .is_ibc_relayer(self) .await .context("failed to check if address is IBC relayer")?, "only IBC sudo address can execute IBC actions" ); + let action = act + .clone() + .with_handler::(); + action + .check_and_execute(&mut state) + .await + .context("failed executing ibc action")?; } Action::Ics20Withdrawal(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await - .context("stateful check failed for Ics20WithdrawalAction")?, + .context("failed executing ics20 withdrawal")?, Action::IbcRelayerChange(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await - .context("stateful check failed for IbcRelayerChangeAction")?, + .context("failed executing ibc relayer change")?, Action::FeeAssetChange(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await - .context("stateful check failed for FeeAssetChangeAction")?, + .context("failed executing fee asseet change")?, Action::InitBridgeAccount(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await - .context("stateful check failed for InitBridgeAccountAction")?, + .context("failed executing init bridge account")?, Action::BridgeLock(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await - .context("stateful check failed for BridgeLockAction")?, + .context("failed executing bridge lock")?, Action::BridgeUnlock(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await - .context("stateful check failed for BridgeUnlockAction")?, + .context("failed executing bridge unlock")?, Action::BridgeSudoChange(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await - .context("stateful check failed for BridgeSudoChangeAction")?, - } - } - - Ok(()) - } - - #[instrument(skip_all)] - async fn execute(&self, state: &mut S, from: Address) -> anyhow::Result<()> - where - S: accounts::StateReadExt + accounts::StateWriteExt, - { - let from_nonce = state - .get_account_nonce(from) - .await - .context("failed getting `from` nonce")?; - let next_nonce = from_nonce - .checked_add(1) - .context("overflow occurred incrementing stored nonce")?; - state - .put_account_nonce(from, next_nonce) - .context("failed updating `from` nonce")?; - - for action in &self.actions { - match action { - Action::Transfer(act) => { - act.execute(state, from) - .await - .context("execution failed for TransferAction")?; - } - Action::Sequence(act) => { - act.execute(state, from) - .await - .context("execution failed for SequenceAction")?; - } - Action::ValidatorUpdate(act) => { - act.execute(state, from) - .await - .context("execution failed for ValidatorUpdateAction")?; - } - Action::SudoAddressChange(act) => { - act.execute(state, from) - .await - .context("execution failed for SudoAddressChangeAction")?; - } - Action::FeeChange(act) => { - act.execute(state, from) - .await - .context("execution failed for FeeChangeAction")?; - } - Action::Ibc(act) => { - let action = act - .clone() - .with_handler::(); - action - .check_and_execute(&mut *state) - .await - .context("execution failed for IbcAction")?; - } - Action::Ics20Withdrawal(act) => { - act.execute(state, from) - .await - .context("execution failed for Ics20WithdrawalAction")?; - } - Action::IbcRelayerChange(act) => { - act.execute(state, from) - .await - .context("execution failed for IbcRelayerChangeAction")?; - } - Action::FeeAssetChange(act) => { - act.execute(state, from) - .await - .context("execution failed for FeeAssetChangeAction")?; - } - Action::InitBridgeAccount(act) => { - act.execute(state, from) - .await - .context("execution failed for InitBridgeAccountAction")?; - } - Action::BridgeLock(act) => { - act.execute(state, from) - .await - .context("execution failed for BridgeLockAction")?; - } - Action::BridgeUnlock(act) => { - act.execute(state, from) - .await - .context("execution failed for BridgeUnlockAction")?; - } - Action::BridgeSudoChange(act) => { - act.execute(state, from) - .await - .context("execution failed for BridgeSudoChangeAction")?; - } + .context("failed executing bridge sudo change")?, } } + // XXX: Delete the current transaction data from the ephemeral state. + state.delete_current_source(); Ok(()) } } diff --git a/crates/astria-sequencer/src/transaction/state_ext.rs b/crates/astria-sequencer/src/transaction/state_ext.rs new file mode 100644 index 0000000000..4304505b76 --- /dev/null +++ b/crates/astria-sequencer/src/transaction/state_ext.rs @@ -0,0 +1,51 @@ +use astria_core::{ + primitive::v1::ADDRESS_LEN, + protocol::transaction::v1alpha1::SignedTransaction, +}; +use cnidarium::{ + StateRead, + StateWrite, +}; + +fn current_source() -> &'static str { + "transaction/current_source" +} + +#[derive(Clone)] +pub(crate) struct TransactionContext { + pub(crate) address_bytes: [u8; ADDRESS_LEN], +} + +impl TransactionContext { + pub(crate) fn address_bytes(&self) -> [u8; ADDRESS_LEN] { + self.address_bytes + } +} + +impl From<&SignedTransaction> for TransactionContext { + fn from(value: &SignedTransaction) -> Self { + Self { + address_bytes: value.address_bytes(), + } + } +} + +pub(crate) trait StateWriteExt: StateWrite { + fn put_current_source(&mut self, transaction: impl Into) { + let context: TransactionContext = transaction.into(); + self.object_put(current_source(), context); + } + + fn delete_current_source(&mut self) { + self.object_delete(current_source()); + } +} + +pub(crate) trait StateReadExt: StateRead { + fn get_current_source(&self) -> Option { + self.object_get(current_source()) + } +} + +impl StateReadExt for T {} +impl StateWriteExt for T {} diff --git a/crates/astria-sequencer/src/transaction/tests.rs b/crates/astria-sequencer/src/transaction/tests.rs new file mode 100644 index 0000000000..ae62c93574 --- /dev/null +++ b/crates/astria-sequencer/src/transaction/tests.rs @@ -0,0 +1,26 @@ +use super::InvalidNonce; + +#[test] +fn invalid_nonce() { + assert!( + InvalidNonce { + current: 24, + in_transaction: 42 + } + .is_ahead() + ); + assert!( + !InvalidNonce { + current: 42, + in_transaction: 24 + } + .is_ahead() + ); + assert!( + !InvalidNonce { + current: 42, + in_transaction: 42 + } + .is_ahead() + ); +}