feat(sequencer)!: enforce block ordering by transaction group (#1618)

## Summary
We want to order blocks by the transaction categories introduced in
#1512 to finish the goals of #1412.

## Background
Certain transactions have the ability to cause other transactions to
become invalid. For example, `FeeChangeActions` can cause transactions
that were valid to become invalid. For the sake of UX we want to order
the potentially-invalidating actions after others so we don't needlessly
execute transactions that were just invalidated. More background can be
found in #1412.

## Changes
- The mempool now orders the transactions it feeds to `prepare_proposal`
by `ActionGroup`.
- `prepare_proposal` will now skip any transactions in it's block
construction process that are out of group order.
- `process_proposal` will now reject any blocks that contain
out-of-order transactions.

Note: the mempool does not check for out of order actions inside of the
pending queue. This was decided because the amount of times that actions
besides `BundleableGeneral` are expected to be ran is low. Instead we
let `prepare_proposal` gracefully ignore out-of-order actions in a way
that allows them to be included in future blocks.

For example, let's say an account sends these two transactions into the
mempool:
- `tx_0: {nonce: 0, action_group: UnbundleableGeneral}`
- `tx_1: {nonce: 1, action_group: BundleableGeneral}`

The mempool will feed these transaction in the order of `{tx_1, tx_0}`
to `prepare_proposal`, which is correct on a group ordering level but
incorrect on a nonce ordering level. `prepare_proposal` will handle this
by skipping `tx_1` and only including `tx_0` in the block. The mempool
will re-feed `tx_1` on the next round to `prepare_proposal` to be
included.

## Testing
Unit tests and ran locally. 

## Breaking Changelist
Block that would've passed before with incorrect transaction ordering
will now fail.

## Related Issues
closes #1412 #1417

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",
             )
     }
 

crates/astria-core/src/protocol/transaction/v1alpha1/action_group/mod.rs → 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,19 +118,15 @@ 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,
             action,
         })
     }
 
-    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<Action>,
 }
 
 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<Action> {
+    pub(crate) fn into_actions(self) -> Vec<Action> {
         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<Action>) -> Result<Self, Error> {
+    pub(crate) fn try_from_list_of_actions(actions: Vec<Action>) -> Result<Self, Error> {
         let mut actions_iter = actions.iter();
         let group = match actions_iter.next() {
             Some(action) => action.group(),

crates/astria-core/src/protocol/transaction/v1alpha1/action_group/tests.rs → 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);
+}

crates/astria-core/src/protocol/transaction/v1alpha1/action.rs → 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",