ENG-33 chore(sequencer): add cache for immutable check-tx results#1271
Closed
ENG-33 chore(sequencer): add cache for immutable check-tx results#1271
Conversation
9046a25 to
3e7052a
Compare
noot
reviewed
Aug 28, 2024
| .get_value_or_guard_async(&tx_hash) | ||
| .await | ||
| { | ||
| Ok(Ok(cached_tx)) => { |
Contributor
There was a problem hiding this comment.
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
Contributor
Author
There was a problem hiding this comment.
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.
Contributor
|
Requesting @Lilyjjo review for this because she is owning the mempool bits of sequencer at the moment. |
Contributor
Author
|
Closed in favour of #1515. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Added an in-memory cache of check-tx results.
Background
Currently if
check_txis 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.removefor 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, hencemempool.removedoesn't need to be called for these cases.Changes
handle_check_txinto two new functions:parse_tx_and_run_immutable_checkswith a result which can be cached, andrun_mutable_checksfor the checks which can vary depending upon state.handle_check_tx. If we get a cache hit and the cached result isOkwe continue with the mutable checks. If we get a cache hit where the cached result isErrwe 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.mempool.removefrom the tx parsing and immutable checking stage.Arc<SignedTransaction>to avoid expensive clones since these are now potentially also being held in the new cache.transaction::checks::check_nonce_mempooltoget_current_nonce_if_tx_nonce_validwhere 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
astria_sequencer_check_tx_cache_hitcounter to record total cache hits.astria_sequencer_check_tx_cache_misscounter to record total cache misses where the request was of typeCheckTxKind::Recheck.Related Issues
Closes #1253.