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: CheckTX fails all rechecks #1481

Closed
Lilyjjo opened this issue Sep 10, 2024 · 0 comments · Fixed by #1515
Closed

sequencer/mempool: CheckTX fails all rechecks #1481

Lilyjjo opened this issue Sep 10, 2024 · 0 comments · Fixed by #1515
Assignees
Labels
mempool observability observability, tracing, metrics sequencer pertaining to the astria-sequencer crate

Comments

@Lilyjjo
Copy link
Contributor

Lilyjjo commented Sep 10, 2024

During the mempool rewrite CheckTX's functionality was accidentally changed to fail all CheckTxKind::Recheck calls. Previously attempting to re-insert a transaction into the mempool would not cause an error.

This is a UX regression as CometBFT is used to display to users the possible statuses of their transactions. We should allow the CheckTxKind::Recheck calls to succeed until we have better ways to report to users the status of their transactions. Nice to have also is basic unit testing for CheckTX.

┆Issue Number: ENG-811

@Lilyjjo Lilyjjo added sequencer pertaining to the astria-sequencer crate observability observability, tracing, metrics mempool labels Sep 10, 2024
@Lilyjjo Lilyjjo self-assigned this Sep 10, 2024
github-merge-queue bot pushed a commit that referenced this issue Oct 4, 2024
…sion (#1515)

## Summary
Previously `handle_check_tx()`'s would always run all stateless checks
and attempt insertion. This PR modified the function to only run the
checks on transactions that have not been removed or are not contained
inside of the app side mempool.

This PR additionally adds a HashSet of transaction hashes that are
contained in the app's Mempool to the state of the app Mempool. This is
used to enable seeing if a transaction is contained in the Mempool
without having to decode it into a `SignedTransaction`. This will be
kept in sync with unit tests which check if the length of the mempool
(which is now calculated with the HashSet) matches the expected state of
mutating the underlying containers.

We considered using just a toggle on `CheckTX::New` and
`CheckTX::Recheck` instead of the HashSet, but there is the possibility
of CometBFT persisting transactions on restart which could make use of
toggling on the app mempool knowing the transaction instead. TBD, but
it's also useful to be able to query the mempool if it contains a
transaction.

This PR also restores the original behavior of `CheckTx` to not kick out
transaction needlessly on rechecks, this regression was added when
upgrading the mempool in #1323. Pre #1323, re-inserting the same
transaction into the mempool would not cause an error. Post #1323, the
re-insertion would cause an error. We now check to make sure that the
transaction is not inside of the mempool before attempting to re-insert
it.

## Background
A lot of the functionality from the original `handle_check_tx()` has
moved into the app side mempool itself, including the balance and nonce
checks. All of the remaining checks will not change on
`CheckTx::Recheck` and only need to be ran when considering a
transaction for insertion into the appside mempool.

## Testing
Ran locally and added unit tests to ensure future nonces can be added
and rechecks do not remove transactions needlessly.

## Metrics
- Deleted `check_tx_removed_stale_nonce`
- Added `check_tx_duration_seconds_check_tracked`,
`mempool_tx_logic_error`
- Changed `check_tx_duration_seconds_check_nonce` to
`check_tx_duration_seconds_fetch_nonce`

## Breaking Changes
Added new ABCI error codes for transaction insertion errors that would
be useful to surface to the end user: `ALREADY_PRESENT`, `NONCE_TAKEN`,
`ACCOUNT_SIZE_LIMIT`.

## Related Issues
closes #1481

---------

Co-authored-by: Fraser Hutchison <[email protected]>
github-merge-queue bot pushed a commit that referenced this issue Oct 4, 2024
…sion (#1515)

## Summary
Previously `handle_check_tx()`'s would always run all stateless checks
and attempt insertion. This PR modified the function to only run the
checks on transactions that have not been removed or are not contained
inside of the app side mempool.

This PR additionally adds a HashSet of transaction hashes that are
contained in the app's Mempool to the state of the app Mempool. This is
used to enable seeing if a transaction is contained in the Mempool
without having to decode it into a `SignedTransaction`. This will be
kept in sync with unit tests which check if the length of the mempool
(which is now calculated with the HashSet) matches the expected state of
mutating the underlying containers.

We considered using just a toggle on `CheckTX::New` and
`CheckTX::Recheck` instead of the HashSet, but there is the possibility
of CometBFT persisting transactions on restart which could make use of
toggling on the app mempool knowing the transaction instead. TBD, but
it's also useful to be able to query the mempool if it contains a
transaction.

This PR also restores the original behavior of `CheckTx` to not kick out
transaction needlessly on rechecks, this regression was added when
upgrading the mempool in #1323. Pre #1323, re-inserting the same
transaction into the mempool would not cause an error. Post #1323, the
re-insertion would cause an error. We now check to make sure that the
transaction is not inside of the mempool before attempting to re-insert
it.

## Background
A lot of the functionality from the original `handle_check_tx()` has
moved into the app side mempool itself, including the balance and nonce
checks. All of the remaining checks will not change on
`CheckTx::Recheck` and only need to be ran when considering a
transaction for insertion into the appside mempool.

## Testing
Ran locally and added unit tests to ensure future nonces can be added
and rechecks do not remove transactions needlessly.

## Metrics
- Deleted `check_tx_removed_stale_nonce`
- Added `check_tx_duration_seconds_check_tracked`,
`mempool_tx_logic_error`
- Changed `check_tx_duration_seconds_check_nonce` to
`check_tx_duration_seconds_fetch_nonce`

## Breaking Changes
Added new ABCI error codes for transaction insertion errors that would
be useful to surface to the end user: `ALREADY_PRESENT`, `NONCE_TAKEN`,
`ACCOUNT_SIZE_LIMIT`.

## Related Issues
closes #1481

---------

Co-authored-by: Fraser Hutchison <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mempool observability observability, tracing, metrics sequencer pertaining to the astria-sequencer crate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant