diff --git a/crates/astria-composer/src/executor/bundle_factory/mod.rs b/crates/astria-composer/src/executor/bundle_factory/mod.rs index b32938f6bf..8c1c87fd1f 100644 --- a/crates/astria-composer/src/executor/bundle_factory/mod.rs +++ b/crates/astria-composer/src/executor/bundle_factory/mod.rs @@ -78,7 +78,7 @@ impl SizedBundle { /// Constructs an [`UnsignedTransaction`] from the actions contained in the bundle and `params`. /// # Panics /// Method is expected to never panic because only `SequenceActions` are added to the bundle, - /// which should produce a valid variant of the `ActionGroup` type. + /// which should produce a valid variant of the [`action::Group`] type. pub(super) fn to_unsigned_transaction( &self, nonce: u32, @@ -91,8 +91,8 @@ impl SizedBundle { .try_build() .expect( "method is expected to never panic because only `SequenceActions` are added to \ - the bundle, which should produce a valid variant of the `ActionGroup` type; this \ - is checked by `tests::transaction_construction_should_not_panic", + the bundle, which should produce a valid variant of the `action::Group` type; \ + this is checked by `tests::transaction_construction_should_not_panic", ) } diff --git a/crates/astria-core/src/protocol/transaction/v1alpha1/action_group/mod.rs b/crates/astria-core/src/protocol/transaction/v1alpha1/action/group/mod.rs similarity index 50% rename from crates/astria-core/src/protocol/transaction/v1alpha1/action_group/mod.rs rename to crates/astria-core/src/protocol/transaction/v1alpha1/action/group/mod.rs index 0de397d874..7c79c29306 100644 --- a/crates/astria-core/src/protocol/transaction/v1alpha1/action_group/mod.rs +++ b/crates/astria-core/src/protocol/transaction/v1alpha1/action/group/mod.rs @@ -9,58 +9,56 @@ use std::fmt::{ use penumbra_ibc::IbcRelay; use super::{ - action::{ - ActionName, - BridgeLock, - BridgeSudoChange, - BridgeUnlock, - FeeAssetChange, - FeeChangeKind, - IbcRelayerChange, - IbcSudoChange, - Ics20Withdrawal, - InitBridgeAccount, - Sequence, - SudoAddressChange, - Transfer, - ValidatorUpdate, - }, Action, + ActionName, + BridgeLock, + BridgeSudoChange, + BridgeUnlock, + FeeAssetChange, + FeeChangeKind, + IbcRelayerChange, + IbcSudoChange, + Ics20Withdrawal, + InitBridgeAccount, + Sequence, + SudoAddressChange, + Transfer, + ValidatorUpdate, }; trait BelongsToGroup { - const GROUP: ActionGroup; + const GROUP: Group; } macro_rules! impl_belong_to_group { ($(($act:ty, $group:expr)),*$(,)?) => { $( impl BelongsToGroup for $act { - const GROUP: ActionGroup = $group; + const GROUP: Group = $group; } )* } } impl_belong_to_group!( - (Sequence, ActionGroup::BundleableGeneral), - (Transfer, ActionGroup::BundleableGeneral), - (ValidatorUpdate, ActionGroup::BundleableGeneral), - (SudoAddressChange, ActionGroup::UnbundleableSudo), - (IbcRelayerChange, ActionGroup::BundleableSudo), - (Ics20Withdrawal, ActionGroup::BundleableGeneral), - (InitBridgeAccount, ActionGroup::UnbundleableGeneral), - (BridgeLock, ActionGroup::BundleableGeneral), - (BridgeUnlock, ActionGroup::BundleableGeneral), - (BridgeSudoChange, ActionGroup::UnbundleableGeneral), - (FeeChangeKind, ActionGroup::BundleableSudo), - (FeeAssetChange, ActionGroup::BundleableSudo), - (IbcRelay, ActionGroup::BundleableGeneral), - (IbcSudoChange, ActionGroup::UnbundleableSudo), + (Sequence, Group::BundleableGeneral), + (Transfer, Group::BundleableGeneral), + (ValidatorUpdate, Group::BundleableGeneral), + (SudoAddressChange, Group::UnbundleableSudo), + (IbcRelayerChange, Group::BundleableSudo), + (Ics20Withdrawal, Group::BundleableGeneral), + (InitBridgeAccount, Group::UnbundleableGeneral), + (BridgeLock, Group::BundleableGeneral), + (BridgeUnlock, Group::BundleableGeneral), + (BridgeSudoChange, Group::UnbundleableGeneral), + (FeeChangeKind, Group::BundleableSudo), + (FeeAssetChange, Group::BundleableSudo), + (IbcRelay, Group::BundleableGeneral), + (IbcSudoChange, Group::UnbundleableSudo), ); impl Action { - const fn group(&self) -> ActionGroup { + pub const fn group(&self) -> Group { match self { Action::Sequence(_) => Sequence::GROUP, Action::Transfer(_) => Transfer::GROUP, @@ -80,34 +78,37 @@ impl Action { } } -#[derive(Copy, Clone, Debug, PartialEq, Eq)] -pub(super) enum ActionGroup { - BundleableGeneral, - UnbundleableGeneral, - BundleableSudo, - UnbundleableSudo, +/// `action::Group` +/// +/// Used to constrain the types of actions that can be included in a single +/// transaction and the order which transactions are ran in a block. +/// +/// NOTE: The ordering is important and must be maintained. +#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] +pub enum Group { + UnbundleableSudo = 1, + BundleableSudo = 2, + UnbundleableGeneral = 3, + BundleableGeneral = 4, } -impl ActionGroup { - pub(super) fn is_bundleable(self) -> bool { - matches!( - self, - ActionGroup::BundleableGeneral | ActionGroup::BundleableSudo - ) +impl Group { + pub(crate) fn is_bundleable(self) -> bool { + matches!(self, Group::BundleableGeneral | Group::BundleableSudo) } - pub(super) fn is_bundleable_sudo(self) -> bool { - matches!(self, ActionGroup::BundleableSudo) + pub(crate) fn is_bundleable_sudo(self) -> bool { + matches!(self, Group::BundleableSudo) } } -impl fmt::Display for ActionGroup { +impl fmt::Display for Group { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - ActionGroup::BundleableGeneral => write!(f, "bundleable general"), - ActionGroup::UnbundleableGeneral => write!(f, "unbundleable general"), - ActionGroup::BundleableSudo => write!(f, "bundleable sudo"), - ActionGroup::UnbundleableSudo => write!(f, "unbundleable sudo"), + Group::BundleableGeneral => write!(f, "bundleable general"), + Group::UnbundleableGeneral => write!(f, "unbundleable general"), + Group::BundleableSudo => write!(f, "bundleable sudo"), + Group::UnbundleableSudo => write!(f, "unbundleable sudo"), } } } @@ -117,11 +118,7 @@ impl fmt::Display for ActionGroup { pub struct Error(ErrorKind); impl Error { - fn mixed( - original_group: ActionGroup, - additional_group: ActionGroup, - action: &'static str, - ) -> Self { + fn mixed(original_group: Group, additional_group: Group, action: &'static str) -> Self { Self(ErrorKind::Mixed { original_group, additional_group, @@ -129,7 +126,7 @@ impl Error { }) } - fn not_bundleable(group: ActionGroup) -> Self { + fn not_bundleable(group: Group) -> Self { Self(ErrorKind::NotBundleable { group, }) @@ -143,41 +140,41 @@ impl Error { #[derive(Debug, thiserror::Error)] enum ErrorKind { #[error( - "input contains mixed `ActionGroup` types. original group: {original_group}, additional \ - group: {additional_group}, triggering action: {action}" + "input contains mixed `Group` types. original group: {original_group}, additional group: \ + {additional_group}, triggering action: {action}" )] Mixed { - original_group: ActionGroup, - additional_group: ActionGroup, + original_group: Group, + additional_group: Group, action: &'static str, }, - #[error("attempted to create bundle with non bundleable `ActionGroup` type: {group}")] - NotBundleable { group: ActionGroup }, + #[error("attempted to create bundle with non bundleable `Group` type: {group}")] + NotBundleable { group: Group }, #[error("actions cannot be empty")] Empty, } #[derive(Clone, Debug)] -pub(super) struct Actions { - group: ActionGroup, +pub(crate) struct Actions { + group: Group, inner: Vec, } impl Actions { - pub(super) fn actions(&self) -> &[Action] { + pub(crate) fn actions(&self) -> &[Action] { &self.inner } #[must_use] - pub(super) fn into_actions(self) -> Vec { + pub(crate) fn into_actions(self) -> Vec { self.inner } - pub(super) fn group(&self) -> ActionGroup { + pub(crate) fn group(&self) -> Group { self.group } - pub(super) fn try_from_list_of_actions(actions: Vec) -> Result { + pub(crate) fn try_from_list_of_actions(actions: Vec) -> Result { let mut actions_iter = actions.iter(); let group = match actions_iter.next() { Some(action) => action.group(), diff --git a/crates/astria-core/src/protocol/transaction/v1alpha1/action_group/tests.rs b/crates/astria-core/src/protocol/transaction/v1alpha1/action/group/tests.rs similarity index 88% rename from crates/astria-core/src/protocol/transaction/v1alpha1/action_group/tests.rs rename to crates/astria-core/src/protocol/transaction/v1alpha1/action/group/tests.rs index d4679f381a..c5cd6212e0 100644 --- a/crates/astria-core/src/protocol/transaction/v1alpha1/action_group/tests.rs +++ b/crates/astria-core/src/protocol/transaction/v1alpha1/action/group/tests.rs @@ -7,29 +7,27 @@ use crate::{ Address, RollupId, }, - protocol::transaction::v1alpha1::{ - action::{ - Action, - BridgeLock, - BridgeSudoChange, - BridgeUnlock, - FeeAssetChange, - FeeChange, - FeeChangeKind, - IbcRelayerChange, - IbcSudoChange, - Ics20Withdrawal, - InitBridgeAccount, - Sequence, - SudoAddressChange, - Transfer, - ValidatorUpdate, - }, - action_group::{ - ActionGroup, + protocol::transaction::v1alpha1::action::{ + group::{ Actions, ErrorKind, + Group, }, + Action, + BridgeLock, + BridgeSudoChange, + BridgeUnlock, + FeeAssetChange, + FeeChange, + FeeChangeKind, + IbcRelayerChange, + IbcSudoChange, + Ics20Withdrawal, + InitBridgeAccount, + Sequence, + SudoAddressChange, + Transfer, + ValidatorUpdate, }, }; const ASTRIA_ADDRESS_PREFIX: &str = "astria"; @@ -92,7 +90,7 @@ fn try_from_list_of_actions_bundleable_general() { assert!(matches!( Actions::try_from_list_of_actions(actions).unwrap().group(), - ActionGroup::BundleableGeneral + Group::BundleableGeneral )); } @@ -116,7 +114,7 @@ fn from_list_of_actions_bundleable_sudo() { assert!(matches!( Actions::try_from_list_of_actions(actions).unwrap().group(), - ActionGroup::BundleableSudo + Group::BundleableSudo )); } @@ -134,7 +132,7 @@ fn from_list_of_actions_unbundleable_sudo() { assert!(matches!( Actions::try_from_list_of_actions(actions).unwrap().group(), - ActionGroup::UnbundleableSudo + Group::UnbundleableSudo )); let actions = vec![Action::IbcSudoChange(IbcSudoChange { @@ -143,7 +141,7 @@ fn from_list_of_actions_unbundleable_sudo() { assert!(matches!( Actions::try_from_list_of_actions(actions).unwrap().group(), - ActionGroup::UnbundleableSudo + Group::UnbundleableSudo )); let actions = vec![ @@ -191,14 +189,14 @@ fn from_list_of_actions_unbundleable_general() { assert!(matches!( Actions::try_from_list_of_actions(actions).unwrap().group(), - ActionGroup::UnbundleableGeneral + Group::UnbundleableGeneral )); let actions = vec![sudo_bridge_address_change_action.clone().into()]; assert!(matches!( Actions::try_from_list_of_actions(actions).unwrap().group(), - ActionGroup::UnbundleableGeneral + Group::UnbundleableGeneral )); let actions = vec![ @@ -248,3 +246,10 @@ fn from_list_of_actions_empty() { "expected ErrorKind::Empty, got {error_kind:?}" ); } + +#[test] +fn should_be_in_expected_order() { + assert!(Group::UnbundleableSudo < Group::BundleableSudo); + assert!(Group::BundleableSudo < Group::UnbundleableGeneral); + assert!(Group::UnbundleableGeneral < Group::BundleableGeneral); +} diff --git a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs b/crates/astria-core/src/protocol/transaction/v1alpha1/action/mod.rs similarity index 99% rename from crates/astria-core/src/protocol/transaction/v1alpha1/action.rs rename to crates/astria-core/src/protocol/transaction/v1alpha1/action/mod.rs index cb7af7de75..fb34593bd1 100644 --- a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs +++ b/crates/astria-core/src/protocol/transaction/v1alpha1/action/mod.rs @@ -23,6 +23,8 @@ use crate::{ Protobuf, }; +pub mod group; + #[derive(Clone, Debug)] #[cfg_attr( feature = "serde", diff --git a/crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs b/crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs index 1f80adc2ba..f91de71b22 100644 --- a/crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs +++ b/crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs @@ -1,4 +1,3 @@ -use action_group::Actions; use bytes::Bytes; use prost::{ Message as _, @@ -22,8 +21,14 @@ use crate::{ }; pub mod action; -pub mod action_group; -pub use action::Action; +use action::group::Actions; +pub use action::{ + group::{ + Error, + Group, + }, + Action, +}; #[derive(Debug, thiserror::Error)] #[error(transparent)] @@ -178,6 +183,11 @@ impl SignedTransaction { self.transaction.actions.actions() } + #[must_use] + pub fn group(&self) -> Group { + self.transaction.actions.group() + } + #[must_use] pub fn is_bundleable_sudo_action_group(&self) -> bool { self.transaction.actions.group().is_bundleable_sudo() @@ -322,7 +332,7 @@ impl UnsignedTransaction { .chain_id(params.chain_id) .nonce(params.nonce) .try_build() - .map_err(UnsignedTransactionError::action_group) + .map_err(UnsignedTransactionError::group) } /// Attempt to convert from a protobuf [`pbjson_types::Any`]. @@ -365,8 +375,8 @@ impl UnsignedTransactionError { Self(UnsignedTransactionErrorKind::DecodeAny(inner)) } - fn action_group(inner: action_group::Error) -> Self { - Self(UnsignedTransactionErrorKind::ActionGroup(inner)) + fn group(inner: action::group::Error) -> Self { + Self(UnsignedTransactionErrorKind::Group(inner)) } } @@ -388,7 +398,7 @@ enum UnsignedTransactionErrorKind { )] DecodeAny(#[source] prost::DecodeError), #[error("`actions` field does not form a valid group of actions")] - ActionGroup(#[source] action_group::Error), + Group(#[source] action::group::Error), } #[derive(Default)] @@ -432,11 +442,11 @@ impl UnsignedTransactionBuilder { /// Constructs a [`UnsignedTransaction`] from the configured builder. /// /// # Errors - /// Returns an error if the actions do not make a valid `ActionGroup`. + /// Returns an error if the actions do not make a valid [`action::Group`]. /// /// Returns an error if the set chain ID does not contain a chain name that can be turned into /// a bech32 human readable prefix (everything before the first dash i.e. `-`). - pub fn try_build(self) -> Result { + pub fn try_build(self) -> Result { let Self { nonce, chain_id, diff --git a/crates/astria-sequencer/src/app/mod.rs b/crates/astria-sequencer/src/app/mod.rs index 688c6c1ece..935b11fabd 100644 --- a/crates/astria-sequencer/src/app/mod.rs +++ b/crates/astria-sequencer/src/app/mod.rs @@ -10,6 +10,8 @@ mod tests_app; #[cfg(test)] mod tests_block_fees; #[cfg(test)] +mod tests_block_ordering; +#[cfg(test)] mod tests_breaking_changes; #[cfg(test)] mod tests_execute_transaction; @@ -25,7 +27,10 @@ use astria_core::{ abci::AbciErrorCode, genesis::v1alpha1::GenesisAppState, transaction::v1alpha1::{ - action::ValidatorUpdate, + action::{ + group::Group, + ValidatorUpdate, + }, Action, SignedTransaction, }, @@ -543,6 +548,7 @@ impl App { let mut failed_tx_count: usize = 0; let mut execution_results = Vec::new(); let mut excluded_txs: usize = 0; + let mut current_tx_group = Group::BundleableGeneral; // get copy of transactions to execute from mempool let pending_txs = self @@ -598,6 +604,20 @@ impl App { continue; } + // ensure transaction's group is less than or equal to current action group + let tx_group = tx.group(); + if tx_group > current_tx_group { + debug!( + transaction_hash = %tx_hash_base64, + block_size_constraints = %json(&block_size_constraints), + "excluding transaction: group is higher priority than previously included transactions" + ); + excluded_txs = excluded_txs.saturating_add(1); + + // note: we don't remove the tx from mempool as it may be valid in the future + continue; + } + // execute tx and store in `execution_results` list on success match self.execute_transaction(tx.clone()).await { Ok(events) => { @@ -628,6 +648,12 @@ impl App { // 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 // removed from the mempool in `update_mempool_after_finalization`. + // + // this is important for possible out-of-order transaction + // groups fed into prepare_proposal. a transaction with a higher + // nonce might be in a higher priority group than a transaction + // from the same account wiht a lower nonce. this higher nonce + // could execute in the next block fine. } else { failed_tx_count = failed_tx_count.saturating_add(1); @@ -645,6 +671,9 @@ impl App { } } } + + // update current action group to tx's action group + current_tx_group = tx_group; } if failed_tx_count > 0 { @@ -692,8 +721,8 @@ impl App { txs: Vec, block_size_constraints: &mut BlockSizeConstraints, ) -> Result> { - let mut excluded_tx_count = 0u32; let mut execution_results = Vec::new(); + let mut current_tx_group = Group::BundleableGeneral; for tx in txs { let bytes = tx.to_raw().encode_to_vec(); @@ -713,10 +742,19 @@ impl App { transaction_hash = %telemetry::display::base64(&tx_hash), block_size_constraints = %json(&block_size_constraints), tx_data_bytes = tx_sequence_data_bytes, - "excluding transaction: max block sequenced data limit reached" + "transaction error: max block sequenced data limit passed" ); - excluded_tx_count = excluded_tx_count.saturating_add(1); - continue; + bail!("max block sequenced data limit passed"); + } + + // ensure transaction's group is less than or equal to current action group + let tx_group = tx.group(); + if tx_group > current_tx_group { + debug!( + transaction_hash = %telemetry::display::base64(&tx_hash), + "transaction error: block has incorrect transaction group ordering" + ); + bail!("transactions have incorrect transaction group ordering"); } // execute tx and store in `execution_results` list on success @@ -737,19 +775,14 @@ impl App { debug!( transaction_hash = %telemetry::display::base64(&tx_hash), error = AsRef::::as_ref(&e), - "failed to execute transaction, not including in block" + "transaction error: failed to execute transaction" ); - excluded_tx_count = excluded_tx_count.saturating_add(1); + bail!("transaction failed to execute"); } } - } - if excluded_tx_count > 0 { - info!( - excluded_tx_count = excluded_tx_count, - included_tx_count = execution_results.len(), - "excluded transactions from block" - ); + // update current action group to tx's action group + current_tx_group = tx_group; } Ok(execution_results) diff --git a/crates/astria-sequencer/src/app/test_utils.rs b/crates/astria-sequencer/src/app/test_utils.rs index 915b977914..f988423d4a 100644 --- a/crates/astria-sequencer/src/app/test_utils.rs +++ b/crates/astria-sequencer/src/app/test_utils.rs @@ -20,9 +20,14 @@ use astria_core::{ }, transaction::v1alpha1::{ action::{ + group::Group, + FeeAssetChange, + InitBridgeAccount, Sequence, + SudoAddressChange, ValidatorUpdate, }, + Action, SignedTransaction, UnsignedTransaction, }, @@ -46,7 +51,10 @@ use crate::{ mempool::Mempool, metrics::Metrics, sequence::StateWriteExt as SequenceStateWriteExt, - test_utils::astria_address_from_hex_string, + test_utils::{ + astria_address_from_hex_string, + nria, + }, }; pub(crate) const ALICE_ADDRESS: &str = "1c0c490f1b5528d8173c5de46d131160e4b2c0c3"; @@ -261,6 +269,7 @@ pub(crate) struct MockTxBuilder { nonce: u32, signer: SigningKey, chain_id: String, + group: Group, } #[expect( @@ -275,6 +284,7 @@ impl MockTxBuilder { chain_id: "test".to_string(), nonce: 0, signer: get_alice_signing_key(), + group: Group::BundleableGeneral, } } @@ -299,16 +309,45 @@ impl MockTxBuilder { } } + pub(crate) fn group(self, group: Group) -> Self { + Self { + group, + ..self + } + } + pub(crate) fn build(self) -> Arc { + let action: Action = match self.group { + Group::BundleableGeneral => Sequence { + rollup_id: RollupId::from_unhashed_bytes("rollup-id"), + data: Bytes::from_static(&[0x99]), + fee_asset: denom_0(), + } + .into(), + Group::UnbundleableGeneral => InitBridgeAccount { + rollup_id: RollupId::from_unhashed_bytes("rollup-id"), + asset: denom_0(), + fee_asset: denom_0(), + sudo_address: None, + withdrawer_address: None, + } + .into(), + Group::BundleableSudo => FeeAssetChange::Addition(denom_0()).into(), + Group::UnbundleableSudo => SudoAddressChange { + new_address: astria_address_from_hex_string(JUDY_ADDRESS), + } + .into(), + }; + + assert!( + action.group() == self.group, + "action group mismatch: wanted {:?}, got {:?}", + self.group, + action.group() + ); + let tx = UnsignedTransaction::builder() - .actions(vec![ - Sequence { - rollup_id: RollupId::from_unhashed_bytes("rollup-id"), - data: Bytes::from_static(&[0x99]), - fee_asset: denom_0(), - } - .into(), - ]) + .actions(vec![action]) .chain_id(self.chain_id) .nonce(self.nonce) .try_build() @@ -320,7 +359,7 @@ impl MockTxBuilder { pub(crate) const MOCK_SEQUENCE_FEE: u128 = 10; pub(crate) fn denom_0() -> Denom { - "denom_0".parse().unwrap() + nria().into() } pub(crate) fn denom_1() -> Denom { diff --git a/crates/astria-sequencer/src/app/tests_app/mod.rs b/crates/astria-sequencer/src/app/tests_app/mod.rs index a7bba14858..e215d1cee1 100644 --- a/crates/astria-sequencer/src/app/tests_app/mod.rs +++ b/crates/astria-sequencer/src/app/tests_app/mod.rs @@ -14,6 +14,7 @@ use astria_core::{ action::{ BridgeLock, Sequence, + SudoAddressChange, Transfer, }, UnsignedTransaction, @@ -29,7 +30,10 @@ use prost::{ use tendermint::{ abci::{ self, - request::PrepareProposal, + request::{ + PrepareProposal, + ProcessProposal, + }, types::CommitInfo, }, account, @@ -736,6 +740,122 @@ async fn app_prepare_proposal_sequencer_max_bytes_overflow_ok() { ); } +#[tokio::test] +async fn app_process_proposal_sequencer_max_bytes_overflow_fail() { + let (mut app, storage) = initialize_app_with_storage(None, vec![]).await; + app.prepare_commit(storage.clone()).await.unwrap(); + app.commit(storage.clone()).await; + + // create txs which will cause sequencer overflow (max is currently 256_000 bytes) + let alice = get_alice_signing_key(); + let tx_pass = UnsignedTransaction::builder() + .actions(vec![ + Sequence { + rollup_id: RollupId::from([1u8; 32]), + data: Bytes::copy_from_slice(&[1u8; 200_000]), + fee_asset: nria().into(), + } + .into(), + ]) + .chain_id("test") + .try_build() + .unwrap() + .into_signed(&alice); + let tx_overflow = UnsignedTransaction::builder() + .actions(vec![ + Sequence { + rollup_id: RollupId::from([1u8; 32]), + data: Bytes::copy_from_slice(&[1u8; 100_000]), + fee_asset: nria().into(), + } + .into(), + ]) + .nonce(1) + .chain_id("test") + .try_build() + .unwrap() + .into_signed(&alice); + + let txs: Vec = vec![tx_pass, tx_overflow]; + let generated_commitment = generate_rollup_datas_commitment(&txs, HashMap::new()); + let txs = generated_commitment.into_transactions( + txs.into_iter() + .map(|tx| tx.to_raw().encode_to_vec().into()) + .collect(), + ); + + let process_proposal = ProcessProposal { + hash: Hash::default(), + height: 1u32.into(), + time: Time::now(), + next_validators_hash: Hash::default(), + proposer_address: [0u8; 20].to_vec().try_into().unwrap(), + txs, + proposed_last_commit: None, + misbehavior: vec![], + }; + + let result = app + .process_proposal(process_proposal.clone(), storage.clone()) + .await + .expect_err("expected max sequenced data limit error"); + + assert!( + format!("{result:?}").contains("max block sequenced data limit passed"), + "process proposal should fail due to max sequenced data limit" + ); +} + +#[tokio::test] +async fn app_process_proposal_transaction_fails_to_execute_fails() { + let (mut app, storage) = initialize_app_with_storage(None, vec![]).await; + app.prepare_commit(storage.clone()).await.unwrap(); + app.commit(storage.clone()).await; + + // create txs which will cause transaction execution failure + let alice = get_alice_signing_key(); + let tx_fail = UnsignedTransaction::builder() + .actions(vec![ + SudoAddressChange { + new_address: astria_address_from_hex_string(BOB_ADDRESS), + } + .into(), + ]) + .chain_id("test") + .try_build() + .unwrap() + .into_signed(&alice); + + let txs: Vec = vec![tx_fail]; + let generated_commitment = generate_rollup_datas_commitment(&txs, HashMap::new()); + let txs = generated_commitment.into_transactions( + txs.into_iter() + .map(|tx| tx.to_raw().encode_to_vec().into()) + .collect(), + ); + + let process_proposal = ProcessProposal { + hash: Hash::default(), + height: 1u32.into(), + time: Time::now(), + next_validators_hash: Hash::default(), + proposer_address: [0u8; 20].to_vec().try_into().unwrap(), + txs, + proposed_last_commit: None, + misbehavior: vec![], + }; + + let result = app + .process_proposal(process_proposal.clone(), storage.clone()) + .await + .expect_err("expected transaction execution failure"); + + assert!( + format!("{result:?}").contains("transaction failed to execute"), + "process proposal should fail due transaction execution failure" + ); +} + #[tokio::test] async fn app_end_block_validator_updates() { let initial_validator_set = vec![ diff --git a/crates/astria-sequencer/src/app/tests_block_ordering.rs b/crates/astria-sequencer/src/app/tests_block_ordering.rs new file mode 100644 index 0000000000..c1ca02eb54 --- /dev/null +++ b/crates/astria-sequencer/src/app/tests_block_ordering.rs @@ -0,0 +1,239 @@ +use std::{ + collections::HashMap, + ops::Deref, +}; + +use astria_core::protocol::transaction::v1alpha1::{ + action::group::Group, + SignedTransaction, +}; +use bytes::Bytes; +use prost::Message; +use tendermint::{ + abci::request::{ + PrepareProposal, + ProcessProposal, + }, + block::Height, + Hash, + Time, +}; + +use super::test_utils::get_alice_signing_key; +use crate::{ + app::test_utils::{ + get_bob_signing_key, + get_judy_signing_key, + initialize_app_with_storage, + mock_balances, + mock_tx_cost, + MockTxBuilder, + }, + proposal::commitment::generate_rollup_datas_commitment, +}; + +#[tokio::test] +async fn app_process_proposal_ordering_ok() { + let (mut app, storage) = initialize_app_with_storage(None, vec![]).await; + + // create transactions that should pass with expected ordering + let txs: Vec = vec![ + MockTxBuilder::new() + .group(Group::BundleableGeneral) + .signer(get_alice_signing_key()) + .build() + .deref() + .clone(), + MockTxBuilder::new() + .group(Group::UnbundleableGeneral) + .signer(get_bob_signing_key()) + .build() + .deref() + .clone(), + MockTxBuilder::new() + .group(Group::BundleableSudo) + .signer(get_judy_signing_key()) + .build() + .deref() + .clone(), + MockTxBuilder::new() + .group(Group::UnbundleableSudo) + .nonce(1) + .signer(get_judy_signing_key()) + .build() + .deref() + .clone(), + ]; + + let generated_commitment = generate_rollup_datas_commitment(&txs, HashMap::new()); + let txs = generated_commitment.into_transactions( + txs.into_iter() + .map(|tx| tx.to_raw().encode_to_vec().into()) + .collect(), + ); + + let process_proposal = ProcessProposal { + hash: Hash::Sha256([1; 32]), + height: 1u32.into(), + time: Time::now(), + next_validators_hash: Hash::default(), + proposer_address: [0u8; 20].to_vec().try_into().unwrap(), + txs, + proposed_last_commit: None, + misbehavior: vec![], + }; + + assert!( + app.process_proposal(process_proposal.clone(), storage.clone()) + .await + .is_ok(), + "process proposal should succeed with expected ordering" + ); +} + +#[tokio::test] +async fn app_process_proposal_ordering_fail() { + // Tests that process proposal will reject blocks that contain transactions that are out of + // order. + let (mut app, storage) = initialize_app_with_storage(None, vec![]).await; + + // create transactions that should fail due to incorrect ordering + let txs: Vec = vec![ + MockTxBuilder::new() + .group(Group::UnbundleableGeneral) + .signer(get_bob_signing_key()) + .build() + .deref() + .clone(), + MockTxBuilder::new() + .group(Group::BundleableGeneral) + .signer(get_alice_signing_key()) + .build() + .deref() + .clone(), + ]; + + let generated_commitment = generate_rollup_datas_commitment(&txs, HashMap::new()); + let txs = generated_commitment.into_transactions( + txs.into_iter() + .map(|tx| tx.to_raw().encode_to_vec().into()) + .collect(), + ); + + let process_proposal = ProcessProposal { + hash: Hash::default(), + height: 1u32.into(), + time: Time::now(), + next_validators_hash: Hash::default(), + proposer_address: [0u8; 20].to_vec().try_into().unwrap(), + txs, + proposed_last_commit: None, + misbehavior: vec![], + }; + + let result = app + .process_proposal(process_proposal.clone(), storage.clone()) + .await + .expect_err("expected ordering error"); + + assert!( + format!("{result:?}").contains("transactions have incorrect transaction group ordering"), + "process proposal should fail due to incorrect ordering" + ); +} + +#[tokio::test] +async fn app_prepare_proposal_account_block_misordering_ok() { + // This test ensures that if an account has transactions that are valid eventually but are + // invalid in the same block that they aren't rejected but instead are included in multiple + // blocks. + // + // For example, if an account sends transactions: + // tx_1: {nonce:0, action_group_type:UnbundleableGeneral} + // tx_2: {nonce:1, action_group_type:BundleableGeneral} + // If these were included in the same block tx_2 would be placed before tx_1 because its group + // has a higher priority even though it will fail execution due to having the wrong nonce. + // + // The block building process should handle this in a way that allows the transactions to + // both eventually be included. + let (mut app, storage) = initialize_app_with_storage(None, vec![]).await; + + // create transactions that should fail due to incorrect ordering if both are included in the + // same block + let tx_0 = MockTxBuilder::new() + .group(Group::UnbundleableGeneral) + .signer(get_alice_signing_key()) + .build(); + let tx_1 = MockTxBuilder::new() + .group(Group::BundleableGeneral) + .nonce(1) + .signer(get_alice_signing_key()) + .build(); + + app.mempool + .insert(tx_0.clone(), 0, mock_balances(0, 0), mock_tx_cost(0, 0, 0)) + .await + .unwrap(); + + app.mempool + .insert(tx_1.clone(), 0, mock_balances(0, 0), mock_tx_cost(0, 0, 0)) + .await + .unwrap(); + + let prepare_args = PrepareProposal { + max_tx_bytes: 600_000, + txs: vec![], + local_last_commit: None, + misbehavior: vec![], + height: Height::default(), + time: Time::now(), + next_validators_hash: Hash::default(), + proposer_address: [88u8; 20].to_vec().try_into().unwrap(), + }; + + let prepare_proposal_result = app + .prepare_proposal(prepare_args, storage.clone()) + .await + .expect("incorrect account ordering shouldn't cause blocks to fail"); + + assert_eq!( + prepare_proposal_result.txs[2], + Into::::into(tx_0.to_raw().encode_to_vec()), + "expected to contain first transaction" + ); + + app.mempool.run_maintenance(&app.state, false).await; + assert_eq!( + app.mempool.len().await, + 1, + "mempool should contain 2nd transaction still" + ); + + // commit state for next prepare proposal + app.prepare_commit(storage.clone()).await.unwrap(); + app.commit(storage.clone()).await; + + let prepare_args = PrepareProposal { + max_tx_bytes: 600_000, + txs: vec![], + local_last_commit: None, + misbehavior: vec![], + height: 1u32.into(), + time: Time::now(), + next_validators_hash: Hash::default(), + proposer_address: [88u8; 20].to_vec().try_into().unwrap(), + }; + let prepare_proposal_result = app + .prepare_proposal(prepare_args, storage.clone()) + .await + .expect("incorrect account ordering shouldn't cause blocks to fail"); + + assert_eq!( + prepare_proposal_result.txs[2], + Into::::into(tx_1.to_raw().encode_to_vec()), + "expected to contain second transaction" + ); + + app.mempool.run_maintenance(&app.state, false).await; + assert_eq!(app.mempool.len().await, 0, "mempool should be empty"); +} diff --git a/crates/astria-sequencer/src/mempool/transactions_container.rs b/crates/astria-sequencer/src/mempool/transactions_container.rs index c055fda88c..44d84d28a3 100644 --- a/crates/astria-sequencer/src/mempool/transactions_container.rs +++ b/crates/astria-sequencer/src/mempool/transactions_container.rs @@ -12,7 +12,10 @@ use std::{ use astria_core::{ primitive::v1::asset::IbcPrefixed, - protocol::transaction::v1alpha1::SignedTransaction, + protocol::transaction::v1alpha1::{ + action::group::Group, + SignedTransaction, + }, }; use astria_eyre::eyre::{ eyre, @@ -64,6 +67,7 @@ impl TimemarkedTransaction { Ok(TransactionPriority { nonce_diff, time_first_seen: self.time_first_seen, + group: self.signed_tx.group(), }) } @@ -115,42 +119,40 @@ impl fmt::Display for TimemarkedTransaction { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!( f, - "tx_hash: {}, address: {}, signer: {}, nonce: {}, chain ID: {}", + "tx_hash: {}, address: {}, signer: {}, nonce: {}, chain ID: {}, group: {}", telemetry::display::base64(&self.tx_hash), telemetry::display::base64(&self.address), self.signed_tx.verification_key(), self.signed_tx.nonce(), self.signed_tx.chain_id(), + self.signed_tx.group(), ) } } -#[derive(Clone, Copy, Debug)] +#[derive(Clone, Copy, Debug, PartialEq, Eq)] struct TransactionPriority { nonce_diff: u32, time_first_seen: Instant, + group: Group, } -impl PartialEq for TransactionPriority { - fn eq(&self, other: &Self) -> bool { - self.nonce_diff == other.nonce_diff && self.time_first_seen == other.time_first_seen - } -} - -impl Eq for TransactionPriority {} - impl Ord for TransactionPriority { fn cmp(&self, other: &Self) -> Ordering { - // we want to first order by nonce difference - // lower nonce diff means higher priority - let nonce_diff = self.nonce_diff.cmp(&other.nonce_diff).reverse(); + // first ordered by group + let group = self.group.cmp(&other.group); + if group != Ordering::Equal { + return group; + } - // then by timestamp if equal - if nonce_diff == Ordering::Equal { - // lower timestamp means higher priority - return self.time_first_seen.cmp(&other.time_first_seen).reverse(); + // then by nonce difference where lower nonce diff means higher priority + let nonce_diff = self.nonce_diff.cmp(&other.nonce_diff).reverse(); + if nonce_diff != Ordering::Equal { + return nonce_diff; } - nonce_diff + + // then by timestamp if nonce and group are equal + self.time_first_seen.cmp(&other.time_first_seen).reverse() } } @@ -867,6 +869,7 @@ mod tests { get_alice_signing_key, get_bob_signing_key, get_carol_signing_key, + get_judy_signing_key, mock_balances, mock_state_getter, mock_state_put_account_nonce, @@ -888,6 +891,7 @@ mod tests { signer: SigningKey, chain_id: String, cost_map: HashMap, + group: Group, } impl MockTTXBuilder { @@ -896,6 +900,7 @@ mod tests { .nonce(self.nonce) .signer(self.signer) .chain_id(&self.chain_id) + .group(self.group) .build(); TimemarkedTransaction::new(tx, self.cost_map) @@ -913,6 +918,7 @@ mod tests { nonce: 0, signer: get_alice_signing_key(), chain_id: "test".to_string(), + group: Group::BundleableGeneral, cost_map: mock_tx_cost(0, 0, 0), } } @@ -931,6 +937,13 @@ mod tests { } } + fn group(self, group: Group) -> Self { + Self { + group, + ..self + } + } + fn cost_map(self, cost: HashMap) -> Self { Self { cost_map: cost, @@ -953,15 +966,128 @@ mod tests { } // From https://doc.rust-lang.org/std/cmp/trait.PartialOrd.html + #[test] + fn transaction_priority_comparisons_should_be_consistent_action_group() { + let instant = Instant::now(); + + let bundleable_general = TransactionPriority { + group: Group::BundleableGeneral, + nonce_diff: 0, + time_first_seen: instant, + }; + let unbundleable_general = TransactionPriority { + group: Group::UnbundleableGeneral, + nonce_diff: 0, + time_first_seen: instant, + }; + let bundleable_sudo = TransactionPriority { + group: Group::BundleableSudo, + nonce_diff: 0, + time_first_seen: instant, + }; + let unbundleable_sudo = TransactionPriority { + group: Group::UnbundleableSudo, + nonce_diff: 0, + time_first_seen: instant, + }; + + // partial_cmp + assert!(bundleable_general.partial_cmp(&bundleable_general) == Some(Ordering::Equal)); + assert!(bundleable_general.partial_cmp(&unbundleable_general) == Some(Ordering::Greater)); + assert!(bundleable_general.partial_cmp(&bundleable_sudo) == Some(Ordering::Greater)); + assert!(bundleable_general.partial_cmp(&unbundleable_sudo) == Some(Ordering::Greater)); + + assert!(unbundleable_general.partial_cmp(&bundleable_general) == Some(Ordering::Less)); + assert!(unbundleable_general.partial_cmp(&unbundleable_general) == Some(Ordering::Equal)); + assert!(unbundleable_general.partial_cmp(&bundleable_sudo) == Some(Ordering::Greater)); + assert!(unbundleable_general.partial_cmp(&unbundleable_sudo) == Some(Ordering::Greater)); + + assert!(bundleable_sudo.partial_cmp(&bundleable_general) == Some(Ordering::Less)); + assert!(bundleable_sudo.partial_cmp(&unbundleable_general) == Some(Ordering::Less)); + assert!(bundleable_sudo.partial_cmp(&bundleable_sudo) == Some(Ordering::Equal)); + assert!(bundleable_sudo.partial_cmp(&unbundleable_sudo) == Some(Ordering::Greater)); + + assert!(unbundleable_sudo.partial_cmp(&bundleable_general) == Some(Ordering::Less)); + assert!(unbundleable_sudo.partial_cmp(&unbundleable_general) == Some(Ordering::Less)); + assert!(unbundleable_sudo.partial_cmp(&bundleable_sudo) == Some(Ordering::Less)); + assert!(unbundleable_sudo.partial_cmp(&unbundleable_sudo) == Some(Ordering::Equal)); + + // equal + assert!(bundleable_general == bundleable_general); + assert!(unbundleable_general == unbundleable_general); + assert!(bundleable_sudo == bundleable_sudo); + assert!(unbundleable_sudo == unbundleable_sudo); + + // greater than + assert!(bundleable_general > unbundleable_general); + assert!(bundleable_general > bundleable_sudo); + assert!(bundleable_general > unbundleable_sudo); + + assert!(unbundleable_general > bundleable_sudo); + assert!(unbundleable_general > unbundleable_sudo); + + assert!(bundleable_sudo > unbundleable_sudo); + + // greater than or equal to + assert!(bundleable_general >= bundleable_general); + assert!(bundleable_general >= unbundleable_general); + assert!(bundleable_general >= bundleable_sudo); + assert!(bundleable_general >= unbundleable_sudo); + + assert!(unbundleable_general >= unbundleable_general); + assert!(unbundleable_general >= bundleable_sudo); + assert!(unbundleable_general >= unbundleable_sudo); + + assert!(bundleable_sudo >= bundleable_sudo); + assert!(bundleable_sudo >= unbundleable_sudo); + + assert!(unbundleable_sudo >= unbundleable_sudo); + + // less than + assert!(unbundleable_sudo < bundleable_sudo); + assert!(unbundleable_sudo < unbundleable_general); + assert!(unbundleable_sudo < bundleable_general); + + assert!(bundleable_sudo < bundleable_general); + assert!(bundleable_sudo < unbundleable_general); + + assert!(unbundleable_general < bundleable_general); + + // less than or equal to + assert!(unbundleable_sudo <= unbundleable_sudo); + assert!(unbundleable_sudo <= bundleable_sudo); + assert!(unbundleable_sudo <= unbundleable_general); + assert!(unbundleable_general <= bundleable_general); + + assert!(bundleable_sudo <= bundleable_sudo); + assert!(bundleable_sudo <= bundleable_general); + assert!(bundleable_sudo <= unbundleable_general); + + assert!(unbundleable_general <= unbundleable_general); + assert!(unbundleable_general <= bundleable_general); + + assert!(bundleable_general <= bundleable_general); + + // not equal + assert!(bundleable_general != unbundleable_general); + assert!(bundleable_general != unbundleable_sudo); + assert!(bundleable_general != bundleable_sudo); + assert!(unbundleable_general != bundleable_sudo); + assert!(unbundleable_general != unbundleable_sudo); + assert!(bundleable_sudo != unbundleable_sudo); + } + #[test] fn transaction_priority_comparisons_should_be_consistent_nonce_diff() { let instant = Instant::now(); let high = TransactionPriority { + group: Group::BundleableGeneral, nonce_diff: 0, time_first_seen: instant, }; let low = TransactionPriority { + group: Group::BundleableGeneral, nonce_diff: 1, time_first_seen: instant, }; @@ -1005,10 +1131,12 @@ mod tests { #[test] fn transaction_priority_comparisons_should_be_consistent_time_gap() { let high = TransactionPriority { + group: Group::BundleableGeneral, nonce_diff: 0, time_first_seen: Instant::now(), }; let low = TransactionPriority { + group: Group::BundleableGeneral, nonce_diff: 0, time_first_seen: Instant::now() + Duration::from_micros(10), }; @@ -2119,6 +2247,80 @@ mod tests { ); } + #[tokio::test] + async fn builder_queue_should_be_sorted_by_action_group_type() { + let mut pending_txs = PendingTransactions::new(TX_TTL); + let mock_state = mock_state_getter().await; + + // create transactions in reverse order + let ttx_unbundleable_sudo = MockTTXBuilder::new() + .nonce(0) + .signer(get_judy_signing_key()) + .group(Group::UnbundleableSudo) + .build(); + let ttx_bundleable_sudo = MockTTXBuilder::new() + .nonce(0) + .signer(get_carol_signing_key()) + .group(Group::BundleableSudo) + .build(); + let ttx_unbundleable_general = MockTTXBuilder::new() + .nonce(0) + .signer(get_bob_signing_key()) + .group(Group::UnbundleableGeneral) + .build(); + let ttx_bundleable_general = MockTTXBuilder::new() + .nonce(0) + .group(Group::BundleableGeneral) + .build(); + let account_balances_full = mock_balances(100, 100); + + // add all transactions to the container + pending_txs + .add(ttx_bundleable_general.clone(), 0, &account_balances_full) + .unwrap(); + pending_txs + .add(ttx_unbundleable_general.clone(), 0, &account_balances_full) + .unwrap(); + pending_txs + .add(ttx_bundleable_sudo.clone(), 0, &account_balances_full) + .unwrap(); + pending_txs + .add(ttx_unbundleable_sudo.clone(), 0, &account_balances_full) + .unwrap(); + + // get the builder queue + // note: the account nonces are set to zero when not initialized in the mock state + let builder_queue = pending_txs + .builder_queue(&mock_state) + .await + .expect("building builders queue should work"); + + // check that the transactions are in the expected order + let (first_tx_hash, _) = builder_queue[0]; + assert_eq!( + first_tx_hash, ttx_bundleable_general.tx_hash, + "expected bundleable general transaction to be first" + ); + + let (second_tx_hash, _) = builder_queue[1]; + assert_eq!( + second_tx_hash, ttx_unbundleable_general.tx_hash, + "expected unbundleable general transaction to be second" + ); + + let (third_tx_hash, _) = builder_queue[2]; + assert_eq!( + third_tx_hash, ttx_bundleable_sudo.tx_hash, + "expected bundleable sudo transaction to be third" + ); + + let (fourth_tx_hash, _) = builder_queue[3]; + assert_eq!( + fourth_tx_hash, ttx_unbundleable_sudo.tx_hash, + "expected unbundleable sudo transaction to be last" + ); + } + #[tokio::test] async fn parked_transactions_size_limit_works() { let mut parked_txs = ParkedTransactions::::new(TX_TTL, 1);