feat(sequencer)!: transaction categories on UnsignedTransaction #1512

Lilyjjo · 2024-09-17T16:44:41Z

Note for Reviewers

Most of the logic changes are in the files crates/astria-core/src/protocol/transaction/v1alpha1/action_group.rs and crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs. The rest of the changes are updating the construction of UnsignedTransactions and/or changing the access to the inner fields.

Summary

Adds restrictions on what type of Actions can be included in the same UnsignedTransaction. Implements the categories as described in #1412 but with slightly different names:

General:
- TransferAction
- SequenceAction
- BridgeLockAction
- BridgeUnlockAction
- IbcRelay
- Ics20Withdrawal
- ValidatorUpdate
UnbundeableGeneral (Instead of Bridge Control):
- InitBridgeAccountAction
- BridgeSudoChangeAction
Sudo:
- FeeAssetChangeAction
- FeeChangeAction
- IbcRelayerChangeAction
UnbundleableSudo (Instead of Sudo Control):
- SudoAddressChangeAction
- IbcSudoChangeAction

The check is applied at the time of constructing the UnsignedTransaction. The UnsignedTransaction additionally had its struct fields changed to private and now uses a new constructor to prevent the contained actions from being modified.

Background

We want transactions that can affect the validity of other transactions to be ordered last in blocks to reduce the amount of failed transactions we process. These logic changes are the first code changes being made to realize this goal.

Changes

Introduced the Actions struct to hold valid groupings of Actions.
Introduced ActionGroup enum to represent which Actions can be included together in a transaction and if more than one action can be included in a transaction.
Changed the UnsignedTransaction struct to have private fields and to use a new constructor.
Changed the UnsignedTransaction's action to be a Actions type instead of just a vector of Actions.

Testing

Unit testing and ran locally.

Breaking Changelist

Transactions that contain invalid ActionGroup combos (due to mixed groups or multiple actions in non-bundleable group types) are now 'invalid' transactions. I had to update one of the snapshot tests due to having to unbundle some of the transactions, creating a new state.

Related Issues

Initial steps for #1412

closes #1414, #1416

crates/astria-composer/src/executor/mod.rs

Lilyjjo · 2024-09-18T14:35:44Z

crates/astria-composer/src/executor/mod.rs

+                            .actions(this.bundle.clone().into_actions())
+                            .params(params)
+                            .build()
+                            .expect("failed to build transaction from actions")


SuperFluffy

Looks like an elegant solution to me, and I am always a fan of locking down types.

I think this is ready to be merged, but I would like to see the errors for the wire-type -> domain-type conversion to contain more information.

crates/astria-core/src/protocol/transaction/v1alpha1/action_groups.rs

crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs

crates/astria-sequencer/src/app/mod.rs

crates/astria-cli/src/commands/bridge/submit.rs

crates/astria-cli/src/commands/sequencer.rs

crates/astria-sequencer/src/proposal/commitment.rs

Fraser999

Initial (incomplete) review.

crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs

crates/astria-core/src/protocol/transaction/v1alpha1/action_groups.rs

Lilyjjo · 2024-09-23T21:50:18Z

Since last set of reviews:

Rewrote action_group.rs trait setup in suggested way
Changed UnsignedTransaction() from using a Builder to use a new() constructor
Made error handling more verbose

crates/astria-composer/src/executor/bundle_factory/mod.rs

crates/astria-core/src/protocol/test_utils.rs

crates/astria-core/src/protocol/transaction/v1alpha1/action_group.rs

crates/astria-core/src/protocol/transaction/v1alpha1/action_group/mod.rs

crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs

crates/astria-sequencer/src/app/tests_breaking_changes.rs

crates/astria-composer/src/executor/bundle_factory/mod.rs

crates/astria-composer/src/executor/bundle_factory/tests.rs

SuperFluffy · 2024-09-30T14:24:46Z

crates/astria-core/src/protocol/transaction/v1alpha1/action.rs

@@ -169,6 +169,14 @@ impl Action {
        };
        Some(transfer_action)
    }
+
+    pub fn is_fee_asset_change(&self) -> bool {


Let's make a tracking issue to add unit tests for these.

created and commented in code: #1593

SuperFluffy

Thank you for going going through the many review comments, online and offline!

SuperFluffy · 2024-09-30T16:09:04Z

crates/astria-composer/src/executor/bundle_factory/mod.rs

+    /// Constructs an [`UnsignedTransaction`] from the actions contained in the bundle and `params`.
+    // Method is expected to never panic because only `SequenceActions` are added to the bundle,
+    // which should produce a valid variant of the `ActionGroup` type.
+    #[allow(clippy::panic)]


Heads-up: with #1561 being merged we switch everything to expect and require a reason:

Suggested change

#[allow(clippy::panic)]

#[expect(clippy::panic, reason = "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")]

SuperFluffy · 2024-09-30T16:13:18Z

crates/astria-core/src/protocol/transaction/v1alpha1/action_group/mod.rs

+    General,
+    UnbundleableGeneral,
+    Sudo,
+    UnbundleableSudo,


Bikeshed:

@noot left some comments on these where Bundleable* was removed and Unbundleable* introduced.

My preference would have been to go all out and use General -> BundleableGeneral and Sudo -> BundleableSudo (and keep Unbundleable*) so that this is always spelled out explicitly.

But I am going to leave the final call on this one to you.

Changed to BundleableXXX. Agree it's easier to understand

SuperFluffy · 2024-09-30T16:13:48Z

crates/astria-core/src/protocol/transaction/v1alpha1/action_group/mod.rs

+impl fmt::Display for ActionGroup {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            ActionGroup::General => write!(f, "general"),


Just leaving a note that should you go with my bikeshedding to rename prefix with Bundleable*, to also change the display name.

* main: (34 commits) feat(proto): add bundle and optimistic block apis (#1519) feat(sequencer)!: make empty transactions invalid (#1609) chore(sequencer): simplify boolean expressions in `transaction container` (#1595) refactor(cli): merge argument parsing and command execution (#1568) feat(charts): astrotrek chart (#1513) chore(charts): genesis template to support latest changes (#1594) fix(ci): code freeze action fix (#1610) chore(sequencer)!: exclusively use Borsh encoding for stored data (ENG-768) (#1492) ci: code freeze through github actions (#1588) refactor(sequencer): use builder pattern for transaction container tests (#1592) feat(conductor)!: implement chain ID checks (#1482) chore(ci): upgrade audit-check (#1577) feat(sequencer)!: transaction categories on UnsignedTransaction (#1512) fix(charts): sequencer prometheus rules (#1598) chore(all): Migrate all instances of `#[allow]` to `#[expect]` (#1561) chore(charts,sequencer-faucet): asset precision (#1517) chore(docs): endpoints (#1543) fix(docker): use target binary build param as name of image entrypoint (#1573) fix(ci): ibc bridge test timeout (#1587) Feature: Add `graph-node` to charts. (#1489) ...

## 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

Lilyjjo force-pushed the lilyjjo/transaction_categories_no_types branch from c12d6e6 to bdc9301 Compare September 17, 2024 19:11

github-actions bot added sequencer pertaining to the astria-sequencer crate composer pertaining to composer labels Sep 17, 2024

Lilyjjo force-pushed the lilyjjo/transaction_categories_no_types branch 4 times, most recently from 2bccf24 to 5898ffd Compare September 18, 2024 14:13

Lilyjjo commented Sep 18, 2024

View reviewed changes

crates/astria-composer/src/executor/mod.rs Outdated Show resolved Hide resolved

Lilyjjo commented Sep 18, 2024

View reviewed changes

Lilyjjo force-pushed the lilyjjo/transaction_categories_no_types branch from 6dbf4f5 to becf939 Compare September 18, 2024 14:40

Lilyjjo marked this pull request as ready for review September 18, 2024 14:50

Lilyjjo requested review from a team, joroshiba, SuperFluffy and noot as code owners September 18, 2024 14:50

SuperFluffy reviewed Sep 20, 2024

View reviewed changes

Fraser999 reviewed Sep 20, 2024

View reviewed changes

Lilyjjo added 2 commits September 23, 2024 07:46

ready for review

3cd390a

fix comment

ed325e8

Lilyjjo force-pushed the lilyjjo/transaction_categories_no_types branch from e226fb3 to ed325e8 Compare September 23, 2024 11:46

Lilyjjo added 2 commits September 23, 2024 17:35

respond to review

5fbd0b7

fix clippy errors

de9b744

Lilyjjo requested review from Fraser999 and SuperFluffy September 23, 2024 21:48

noot reviewed Sep 24, 2024

View reviewed changes

crates/astria-composer/src/executor/bundle_factory/mod.rs Outdated Show resolved Hide resolved

noot reviewed Sep 24, 2024

View reviewed changes

crates/astria-core/src/protocol/test_utils.rs Outdated Show resolved Hide resolved

noot reviewed Sep 24, 2024

View reviewed changes

crates/astria-core/src/protocol/transaction/v1alpha1/action_group.rs Outdated Show resolved Hide resolved

noot reviewed Sep 24, 2024

View reviewed changes

crates/astria-core/src/protocol/transaction/v1alpha1/action_group.rs Outdated Show resolved Hide resolved

noot reviewed Sep 24, 2024

View reviewed changes

crates/astria-core/src/protocol/transaction/v1alpha1/action_group.rs Outdated Show resolved Hide resolved

Lilyjjo requested a review from Fraser999 September 29, 2024 18:16