feat(sequencer)!: enforce block ordering by transaction group by lobstergrindset · Pull Request #1618 · astriaorg/astria

lobstergrindset · 2024-10-03T13:20:33Z

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

lobstergrindset · 2024-10-03T13:21:54Z

Oops, hit the wrong button. This isn't ready for review.

Fraser999 · 2024-10-08T09:33:45Z

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

-    BundleableGeneral,
-    UnbundleableGeneral,
-    BundleableSudo,
+#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]


Since this is public now, might be nice to add a little rustdoc comments to the enum and its variants.

Fraser999 · 2024-10-08T09:51:44Z

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

    UnbundleableSudo,
+    BundleableSudo,
+    UnbundleableGeneral,
+    BundleableGeneral,


Seems like the ordering here is very load-bearing. It might be worth adding explicit values to each variant to make this more obvious (maybe also a non-rustdoc "// NOTE: ..." comment on the enum)?

Suggested change

UnbundleableSudo,

BundleableSudo,

UnbundleableGeneral,

BundleableGeneral,

UnbundleableSudo = 1,

BundleableSudo = 2,

UnbundleableGeneral = 3,

BundleableGeneral = 4,

Also probably worth adding a unit test to try and avoid an accidental change here, e.g.

#[test] fn should_be_in_correct_order() { assert_eq!(ActionGroup::UnbundleableSudo < ActionGroup::BundleableSudo); assert_eq!(ActionGroup::BundleableSudo < ActionGroup::UnbundleableGeneral); assert_eq!(ActionGroup::UnbundleableGeneral < ActionGroup::BundleableGeneral); }

Fraser999 · 2024-10-08T10:25:43Z

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

                );
-                excluded_tx_count = excluded_tx_count.saturating_add(1);
-                continue;
+                return Err(eyre!("max block sequenced data limit passed"));


Suggested change

return Err(eyre!("max block sequenced data limit passed"));

bail!("max block sequenced data limit passed");

Also would be nice to have a unit test for this change, asserting we return here rather than relying on the final tx count causing the proposal to fail. Could be a follow-up PR though, since there are maybe a bunch of similar tests which could be added?

Added the unit tests checking for the other error cases!

Fraser999 · 2024-10-08T10:35:52Z

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

+                        "transaction error: failed to execute transaction"
                    );
-                    excluded_tx_count = excluded_tx_count.saturating_add(1);
+                    return Err(eyre!("transaction failed to execute"));


Suggested change

return Err(eyre!("transaction failed to execute"));

bail!("transaction failed to execute");

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

Fraser999 · 2024-10-08T10:56:25Z

crates/astria-sequencer/src/mempool/transactions_container.rs

+
+        assert!(bundleable_sudo >= bundleable_sudo);
+        assert!(bundleable_sudo >= unbundleable_sudo);
+


Suggested change

assert!(unbundleable_sudo >= unbundleable_sudo);

SuperFluffy

Looks fine to me, going to follow @Fraser999's approval.

I just left some minor comments.

SuperFluffy · 2024-10-11T10:10:47Z

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

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


Suggested change

/// which should produce a valid variant of the `Action::Group` type.

/// which should produce a valid variant of the [`action::Group`] type.

SuperFluffy · 2024-10-11T10:11:15Z

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

                "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; \


Suggested change

the bundle, which should produce a valid variant of the `Action::Group` type; \

the bundle, which should produce a valid variant of the `action::Group` type; \

SuperFluffy · 2024-10-11T10:12:42Z

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

    ///
    /// # 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`.


Suggested change

/// Returns an error if the actions do not make a valid `Action::Group`.

/// Returns an error if the actions do not make a valid [`action::Group`].

SuperFluffy · 2024-10-11T10:14:30Z

crates/astria-sequencer/src/mempool/transactions_container.rs

+    group: Group,
 }

 impl PartialEq for TransactionPriority {


Isn't this the same as deriving PartialEq?

Can you say more about what you mean by this? I'm a little confused

I meant that your manual implementation looks the same as deriving PartialEq. Maybe at some point the type contained some field that you wanted to exclude from the comparison, but then you removed it at some point?

I'd derive PartialEq now instead of doing it manually.

* main: feat(sequencer)!: enforce block ordering by transaction group (#1618) fix(cli): ensure checkTx passes before waiting for inclusion (#1636) chore(sequencer)!: update storage keys locations and values (ENG-898) (#1616) chore: cargo audit warning (#1644) chore(proto, core)!: remove action suffix from all action types (#1630) feat(sequencer)!: add limit to total amount of transactions in parked (#1638)

lobstergrindset added 2 commits October 1, 2024 15:04

make empty transaction invalid

f675269

first pass of tx block ordering

39b7044

lobstergrindset marked this pull request as ready for review October 3, 2024 13:20

lobstergrindset requested a review from a team as a code owner October 3, 2024 13:20

lobstergrindset requested a review from SuperFluffy October 3, 2024 13:20

github-actions bot added the sequencer pertaining to the astria-sequencer crate label Oct 3, 2024

lobstergrindset marked this pull request as draft October 3, 2024 13:21

lobstergrindset and others added 3 commits October 3, 2024 10:25

Merge branch 'main' into lilyjjo/order_blocks_by_tx_category

2f4ae04

lint fixes

f78cc1a

update comparison test

e5cf73b

lobstergrindset marked this pull request as ready for review October 3, 2024 16:38

lobstergrindset requested review from Fraser999, joroshiba and noot October 3, 2024 16:38

lobstergrindset and others added 3 commits October 7, 2024 11:12

Merge branch 'main' into lilyjjo/order_blocks_by_tx_category

929552a

fix merge issues

b544e37

fix failing test

f7288ca

Fraser999 reviewed Oct 8, 2024

View reviewed changes

lobstergrindset and others added 5 commits October 8, 2024 15:56

respond to comments by @Fraser999

859ae26

add forgotten files that should have been in the last commit

b047a3e

Merge branch 'main' into lilyjjo/order_blocks_by_tx_category

6787a64

fix merge

2c1d395

fix test utils and move action_group to action::group

9e38a30

github-actions bot added the composer pertaining to composer label Oct 9, 2024

Merge branch 'main' into lilyjjo/order_blocks_by_tx_category

edb8963

lobstergrindset requested a review from Fraser999 October 9, 2024 15:42

Fraser999 approved these changes Oct 11, 2024

View reviewed changes

SuperFluffy reviewed Oct 11, 2024

View reviewed changes

fix doc comments from @SuperFluffy

05f4a88

use derived PartialEq and Eq

10086fa

lobstergrindset enabled auto-merge October 11, 2024 11:09

lobstergrindset added this pull request to the merge queue Oct 11, 2024

Merged via the queue into main with commit 412eebe Oct 11, 2024

lobstergrindset deleted the lilyjjo/order_blocks_by_tx_category branch October 11, 2024 11:31

lobstergrindset mentioned this pull request Oct 31, 2024

sequencer: build blocks with transaction category ordering #1417

Closed

	return Err(eyre!("max block sequenced data limit passed"));
	bail!("max block sequenced data limit passed");

	return Err(eyre!("transaction failed to execute"));
	bail!("transaction failed to execute");


		assert!(bundleable_sudo >= bundleable_sudo);
		assert!(bundleable_sudo >= unbundleable_sudo);

	/// which should produce a valid variant of the `Action::Group` type.
	/// which should produce a valid variant of the [`action::Group`] type.

	the bundle, which should produce a valid variant of the `Action::Group` type; \
	the bundle, which should produce a valid variant of the `action::Group` type; \

	/// Returns an error if the actions do not make a valid `Action::Group`.
	/// Returns an error if the actions do not make a valid [`action::Group`].

Conversation

lobstergrindset commented Oct 3, 2024

Summary

Background

Changes

Testing

Breaking Changelist

Related Issues

Uh oh!

lobstergrindset commented Oct 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuperFluffy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuperFluffy Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SuperFluffy Oct 11, 2024 •

edited

Loading