feat(core, sequencer): provide checked actions and transaction #2142

Fraser999 · 2025-04-28T23:53:21Z

Summary

This introduces CheckedAction, CheckedTransaction and Checked... wrappers for each action type.

Background

We want to have stricter checks performed when CheckTx is called on a new tx in order to avoid accepting txs which subsequently fail execution.

Checks on txs and their included actions are now split into two categories: mutable and immutable. On construction of a checked object, both types of checks are run. Subsequently, only the mutable checks are re-run, and this is currently only done at the start of executing the action/transaction.

Nonce and balance checks are excluded from these checks as the Mempool only puts forward for execution txs which should have valid nonces and sufficient account balances.

In order to ensure such checks have been performed, the sequencer now deals almost exclusively in checked flavours of actions and txs.

As a result of this, many tests which previously used txs had to be refactored to ensure that constructing the equivalent checked txs would succeed.

Changes

Introduced new Checked... types for actions and Transaction. Replaced usage throughout sequencer codebase of the unchecked flavours of these.
Changed ExpandedBlockData::user_submitted_transactions to UN-parsed transactions so that all checking can be done when constructing a CheckedTransaction.
Unified several test utilities into a single, top-level test_utils module. As well as pre-existing helper functions, there is now a Fixture for intializing a new chain along with several builders/initializers to aid in constructing objects to be used in tests. These are provided with doc comments which should explain how to use them.
Disabled benchmarks for now. These are already not functioning on MacOS (albeit there is an open PR fix(sequencer): allow benchmarks to run on macOS and fix issues #1842 to address this). I would prefer to handle updating the benchmarks as part of that PR rather than adding to this one.

Testing

Existing tests were generally refactored to retain their original intent.
Individual checked actions were provided with extensive unit test coverage. Each one generally tests the construction AND execution of the specific action. To generate failures during execution, the unit tests construct the action to be tested while the global state makes construction possible (e.g signer is sudo), and then modify the global state to make execution fail (e.g. change the sudo address). All such state changes are done by executing other checked actions rather than directly manipulating global state, to ensure that such state changes remain feasible in the future.
These new action unit tests made the tests in tests_execute_transaction.rs largely redundant, hence that file has been deleted. Coverage was also provided for IbcRelay where none existed before, but the setup difficulty for testing Ics20Withdrawal is such that only a subset of the code paths are tested (as is the case currently).
CheckedTransaction also has extensive unit test coverage for construction and execution, and covers any deleted tests from tests_execute_transaction.rs not already catered for by the action unit tests.

Changelogs

Changelogs updated.

Metrics

Added metrics:
- ASTRIA_SEQUENCER_CHECK_TX_FAILED_ACTION_CHECKS
- ASTRIA_SEQUENCER_CHECK_TX_DURATION_SECONDS_CHECK_ACTIONS
- ASTRIA_SEQUENCER_CHECK_TX_DURATION_SECONDS_RECHECK
Renamed metric ASTRIA_SEQUENCER_CHECK_TX_REMOVED_TOO_LARGE to
ASTRIA_SEQUENCER_CHECK_TX_FAILED_TX_TOO_LARGE
Deleted metrics:
- ASTRIA_SEQUENCER_CHECK_TX_REMOVED_FAILED_STATELESS
- ASTRIA_SEQUENCER_CHECK_TX_DURATION_SECONDS_PARSE_TX
- ASTRIA_SEQUENCER_CHECK_TX_DURATION_SECONDS_CHECK_STATELESS
- ASTRIA_SEQUENCER_CHECK_TX_DURATION_SECONDS_CHECK_TRACKED
- ASTRIA_SEQUENCER_CHECK_TX_DURATION_SECONDS_CHECK_CHAIN_ID
- ASTRIA_SEQUENCER_CHECK_TX_DURATION_SECONDS_CHECK_REMOVED
- ASTRIA_SEQUENCER_CHECK_TX_DURATION_SECONDS_CONVERT_ADDRESS

Breaking Changelist

There should be no consensus, API or state-breaking changes other than the metrics listed above. All checks being done on actions and transactions remain the same, they are simply performed at a different stage of the flow now.

Related Issues

Closes #1958.
Closes #1620.

ethanoroshiba

Mostly just suggestions and nits, but all the logic here looks solid and quite tidy to me. Nice work! I do think we should try to followup fairly quickly to take the legacy bits out, and it's also probably a good idea to keep this synced with Mainnet until it gets pushed to validators.

crates/astria-core/CHANGELOG.md

crates/astria-core/src/sequencerblock/v1/block/mod.rs

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

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

crates/astria-sequencer/src/checked_actions/ics20_withdrawal.rs

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

…ount

SuperFluffy

astria-core changes look good to me - not yet approving because there seem to be some inaccuracies in the ABCI error codes that are mentioned in the changelog.

Wanna make sure we have a second look.

crates/astria-core/CHANGELOG.md

crates/astria-core/src/protocol/transaction/v1/action/group/mod.rs

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

SuperFluffy

Approving for astria-core. Thank you for amending the changelog. :)

ethanoroshiba

Reaffirming approval with a couple comments

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

ethanoroshiba · 2025-05-09T13:54:16Z

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

-        let tx_len = tx_bytes.len();
-        info!(transaction_hash = %tx_hash_base_64, "executing transaction");
+        let tx_len = tx.encoded_bytes().len();
+        info!(tx_id = %tx.id(), "executing transaction");


Not directly related to this PR but since we're touching this code, do you think it would be better if this was debug!? Just thinking since we're aiming to support hundreds up to a thousand TPS, the info logs could get messy very quickly.

It's a good point, but I'd probably leave as is for now since as you mentioned, this PR doesn't add a new log line here. We can look to change in the future if/when it becomes a noticeable problem.

crates/astria-sequencer/src/service/mempool/mod.rs

ethanoroshiba · 2025-05-09T14:20:10Z

crates/astria-sequencer/src/service/mempool/mod.rs

-    if let Err(outcome) = stateless_checks(tx.clone(), &state, metrics).await {
-        return outcome;
-    }
+    let checked_tx = match CheckedTransaction::new(tx_bytes, &state).await {


I'm guessing this is best put for a followup, but it would be nice if upon a recheck and the transaction still existing in mempool, we took the CheckedTransaction straight from the mempool and re-ran mutable checks, instead of constructing a new one each time.

Well, if the tx were in the mempool, we'd have returned from this function just before here, so we don't actually construct a new one in that case.

We might want to change the behaviour in the future so that check_tx does actually rerun the mutable checks, in which case we'd certainly want to do as you suggest.

ethanoroshiba · 2025-05-09T14:22:55Z

crates/astria-sequencer/src/service/mempool/mod.rs

+            return CheckTxOutcome::FailedChecks(error);
+        }
+    };
+    metrics.record_check_tx_duration_seconds_check_actions(tx_status_end.elapsed());


Nit: the metric and function names seem to be a bit at odds here, since the metric would imply the amount of time that the function takes to complete (as it's called check_tx). Maybe it would be best to rename one or the other?

As discussed offline, this name is probably ok as is - we have a few metrics all with similar prefixes.

noot · 2025-05-09T15:52:19Z

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

        let expanded_block_data =
            ExpandedBlockData::new_from_typed_data(&data, with_extended_commit_info).unwrap();

+        let rollup_data_bytes = sequence_data


is this a duplicate of rollup_data_bytes defined previously?

It more or less is. The first one (which I should probably rename for clarity) is an iterator over (&RollupId, &Bytes) while this one is a Vec of (RollupId, Bytes).

I changed this in 9bf25b6 to create the second instance using the first one so it's clear they're essentially the same.

crates/astria-sequencer/src/bridge/state_ext.rs

crates/astria-sequencer/src/checked_actions/bridge/bridge_lock.rs

noot · 2025-05-12T14:48:11Z

crates/astria-sequencer/src/checked_actions/bridge/bridge_transfer.rs

+        self.checked_bridge_lock
+            .run_mutable_checks(&state)
+            .await
+            .wrap_err("mutable checks for bridge lock failed for bridge transfer")


this is invalid, as bridge transfers are intended to only be sent from bridge accounts. change the check here to bail if tx sender is not a bridge account

I think that's incorrect. self.checked_bridge_lock is an instance of CheckedBridgeLockImpl<false>, and CheckedBridgeLockImpl<false>::run_mutable_checks skips checking for the signer being a bridge account altogether.

The line above (self.checked_bridge_unlock.run_mutable_checks) actually ensures the signer is the authorized bridge account withdrawer, which implicitly ensures the signer is also a bridge account afaik.

ah i missed that this was on CheckedBridgeLockImpl<false>, sorry! however this should still check that the receiver of the transfer is a bridge account as well.

I think that's covered when we construct the CheckedBridgeTransfer. It's calling CheckedBridgeLockImpl::<false>::new, which calls state.get_bridge_account_ibc_asset(&action.to) which ensures the receiver is a bridge account.

This is deemed an immutable check rather than part of the mutable checks since we're only interested in checks which can mutate from passing to failing. We don't care about the other way round since the CheckedAction will fail to be constructed if the checks fail.

Since there's no way to "unset" or uninitialize a bridge account, then this check that the receiver is a bridge account only needs to be run once, during construction.

crates/astria-sequencer/src/checked_actions/bridge/bridge_transfer.rs

crates/astria-sequencer/src/checked_actions/bridge/bridge_unlock.rs

crates/astria-sequencer/src/checked_actions/bridge_sudo_change.rs

crates/astria-sequencer/src/checked_actions/ics20_withdrawal.rs

crates/astria-sequencer/src/checked_actions/init_bridge_account.rs

crates/astria-sequencer/src/checked_actions/rollup_data_submission.rs

## Summary This updates the sequencer benchmarks to actually run on macOS, and fixes a couple of issues around the mempool benchmarks. ## Background The benchmarks could previously only be run on Linux. Since they're also not being run regularly, they got broken at some point. They were further disabled during the large PR #2142. Since I was working on fixing them anyway, I took the opportunity to improve the initialization of the test data. This has resulted in the total time to execute the full mempool suite dropping from ~6.5 mins to ~1.5 mins and means all tests are now able to complete 100 iterations (several could only manage one iteration previously). ## Changes - Used an item from the `astria-sequencer` crate in the benchmark's `main` function to resolve the issue of the tests not running on macOS. - Fixed an issue where the tests were reusing identical txs, causing initialization to fail with an `InsertionError::AlreadyPresent` error. - Memoized test data to reduce test initialization times. - Refactored `PendingTransactionsForAccount::find_demotables` to use an existing method, deduplicating code. - Refactored tests to use updated test utilities. ## Testing Confirmed the benchmarks ran on macOS locally. The `app` ones take a very long time to run and this should be investigated and fixed. To run only the mempool ones: ``` cargo bench --features=benchmark -qp astria-sequencer mempool ``` ## Changelogs No updates required. ## Related Issues Closes #1355.

provide checked actions and transaction

f93bf6a

Fraser999 requested review from SuperFluffy, joroshiba and noot as code owners April 28, 2025 23:53

github-actions bot added the sequencer pertaining to the astria-sequencer crate label Apr 28, 2025

Fraser999 requested a review from ethanoroshiba April 28, 2025 23:53

Fraser999 mentioned this pull request May 1, 2025

feat(sequencer): add mempool gRPC capabilities #2133

Merged

ethanoroshiba approved these changes May 2, 2025

View reviewed changes

Fraser999 added 6 commits May 5, 2025 11:40

revert changes to benchmarks temporarily

ba455ac

apply several minor cleanups to sequencer

7ba0d17

provide helper method in sequencer test fixture

586f99e

provide InsufficientFunds error type

3b6a8e4

move bridge actions to own submodule

26ecd8f

update test fixture to use checked action for initializing bridge acc…

003a1e4

…ount

Fraser999 mentioned this pull request May 6, 2025

Replace sequencer app_legacy_execute_transactions_with_every_action_snapshot test #2157

Closed

Fraser999 added 2 commits May 6, 2025 19:06

check tx nonce earlier in flow and pay fees before executing

28534ab

Merge remote-tracking branch 'upstream/main' into fraser/1958-check-tx

b705b0e

SuperFluffy reviewed May 9, 2025

View reviewed changes

added TransactionParts and updated changelog

2270e90

SuperFluffy approved these changes May 9, 2025

View reviewed changes

ethanoroshiba approved these changes May 9, 2025

View reviewed changes

typo fixes

c11746a

noot reviewed May 9, 2025

View reviewed changes

ethanoroshiba mentioned this pull request May 9, 2025

chore(sequencer): refactor address base prefix checks #2015

Closed

minor change to test helper ConfigureSequencerBlock

9bf25b6