Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sequencer, mempool: make transaction execution and CheckTx share code #1775

Open
Lilyjjo opened this issue Nov 1, 2024 · 0 comments
Open
Assignees
Labels
code-quality mempool refactor code refactoring or maintainence sequencer pertaining to the astria-sequencer crate

Comments

@Lilyjjo
Copy link
Contributor

Lilyjjo commented Nov 1, 2024

We want the mempool's CheckTx and the app's transaction execution logic to mostly perform the same checks. Right now the mempool duplicates a lot of the checks that execution performs, which can lead to issues if devs forget to update the CheckTx code when changing the execution code. Instead of trying to remember to update code in a certain way, we should have the code be shared between the two locations.

For example, CheckTx relies on the function get_total_transaction_cost(), which has a manual rewrite of fee logic that is trying to duplicate code in check_and_execute() which does the action's non-fee balance reductions inside of the action execution. This getting out of sync could cause issues in the mempool's pending vs parked distinction. (Note: the check_and_execute() code also calls into the manual function which is a redundant check that should be removed).

This logic was initially separated due to the performance concerns of running the full transaction execution in CheckTx. Since then, we've changed the mempool to only run CheckTx on the first time a transaction is seen instead of on every block. For performance, it would be nice if we could have the CheckTx implementation run the actions checks in parallel.

Other implementation notes:

  • CheckTx has slightly different requirements than actual transaction execution. CheckTx doesn't want to reject transactions that have too-high nonces or lack balance, and, CheckTx wants to know the total cost of the transaction. The shared implementation should be written in a way where all shared failure checks run first and be able to return the total cost of the transaction even when the account has a too low nonce or not enough balance.

┆Issue Number: ENG-976

@Lilyjjo Lilyjjo self-assigned this Nov 1, 2024
@Lilyjjo Lilyjjo added sequencer pertaining to the astria-sequencer crate mempool code-quality refactor code refactoring or maintainence labels Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-quality mempool refactor code refactoring or maintainence sequencer pertaining to the astria-sequencer crate
Projects
None yet
Development

No branches or pull requests

1 participant