-
Notifications
You must be signed in to change notification settings - Fork 75
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
Conversation
9046a25
to
3e7052a
Compare
.get_value_or_guard_async(&tx_hash) | ||
.await | ||
{ | ||
Ok(Ok(cached_tx)) => { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.
Requesting @Lilyjjo review for this because she is owning the mempool bits of sequencer at the moment. |
Closed in favour of #1515. |
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, hencemempool.remove
doesn't need to be called for these cases.Changes
handle_check_tx
into two new functions:parse_tx_and_run_immutable_checks
with a result which can be cached, andrun_mutable_checks
for the checks which can vary depending upon state.handle_check_tx
. If we get a cache hit and the cached result isOk
we continue with the mutable checks. If we get a cache hit where the cached result isErr
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.mempool.remove
from 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_mempool
toget_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
astria_sequencer_check_tx_cache_hit
counter to record total cache hits.astria_sequencer_check_tx_cache_miss
counter to record total cache misses where the request was of typeCheckTxKind::Recheck
.Related Issues
Closes #1253.