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

ENG-33 chore(sequencer): add cache for immutable check-tx results #1271

Closed
wants to merge 5 commits into from

Conversation

Fraser999
Copy link
Contributor

@Fraser999 Fraser999 commented Jul 15, 2024

Summary

Added an in-memory cache of check-tx results.

Background

Currently if check_tx is called repeatedly for the same tx, we recheck everything every time. There is a portion of that work which will always yield the same result, and hence would benefit from being cached, meaning rechecks only need to actually execute the changeable portion of the checks if we get a cache hit.

We also needlessly call mempool.remove for txs which fail the immutable portion of the parse/check (briefly write-locking the mempool). Such txs can never have made their way into the app mempool, hence mempool.remove doesn't need to be called for these cases.

Changes

  • split the bulk of handle_check_tx into two new functions: parse_tx_and_run_immutable_checks with a result which can be cached, and run_mutable_checks for the checks which can vary depending upon state.
  • Added a cache of 10,000 immutable check-tx results which is queried near the start of handle_check_tx. If we get a cache hit and the cached result is Ok we continue with the mutable checks. If we get a cache hit where the cached result is Err we return the cached error response. If we get a cache miss, we run the immutable checks and cache the result before proceeding with the mutable checks.
  • removed all calls to mempool.remove from the tx parsing and immutable checking stage.
  • added metrics for the cache (see below for details). If we find that we never get any cache hits, we can remove the cache in the future
  • changed the app mempool to receive Arc<SignedTransaction> to avoid expensive clones since these are now potentially also being held in the new cache.
  • changed transaction::checks::check_nonce_mempool to get_current_nonce_if_tx_nonce_valid where it now returns the nonce. This is to avoid having to fetch the nonce twice (we fetch it again when adding the tx to the mempool if all checks pass), but also to avoid potential TOCTOU issues.

Testing

Added unit tests to ensure the cache is populated. Once metrics become testable (when #1192 is merged) we can extend these tests to ensure we get cache hits.

Metrics

  • Added astria_sequencer_check_tx_cache_hit counter to record total cache hits.
  • Added astria_sequencer_check_tx_cache_miss counter to record total cache misses where the request was of type CheckTxKind::Recheck.

Related Issues

Closes #1253.

@Fraser999 Fraser999 requested a review from a team as a code owner July 15, 2024 11:17
@github-actions github-actions bot added the sequencer pertaining to the astria-sequencer crate label Jul 15, 2024
@Fraser999 Fraser999 force-pushed the fraser/cache-check-tx-results branch from 9046a25 to 3e7052a Compare July 15, 2024 11:20
@WafflesVonMaple WafflesVonMaple changed the title chore(sequencer): add cache for immutable check-tx results ENG-33 chore(sequencer): add cache for immutable check-tx results Jul 31, 2024
.get_value_or_guard_async(&tx_hash)
.await
{
Ok(Ok(cached_tx)) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason for caching the tx instead of just its hash? since you're sent the tx again in CheckTx, this seems unnecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's to avoid parsing the protobuf tx from bytes and then converting the protobuf tx to a SignedTransaction. It's mainly the second part which is worth avoiding, since we're doing signature checking in SignedTransaction::try_from_raw.

@SuperFluffy
Copy link
Member

Requesting @Lilyjjo review for this because she is owning the mempool bits of sequencer at the moment.

@Fraser999
Copy link
Contributor Author

Fraser999 commented Sep 27, 2024

Closed in favour of #1515.

@Fraser999 Fraser999 closed this Sep 27, 2024
@Fraser999 Fraser999 deleted the fraser/cache-check-tx-results branch September 27, 2024 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sequencer pertaining to the astria-sequencer crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Utilize the CheckTxKind to avoid needless execution of immutable steps of check_tx in sequencer
3 participants