fix(sequencer): rewrite check_tx to be more efficient and fix regression #1515
Conversation
Lilyjjo
changed the title
Lilyjjo
changed the title
Lilyjjo
changed the title
Lilyjjo
changed the title
Lilyjjo
force-pushed
the
lilyjjo/check-tx-rewrite
branch
3 times, most recently
from
577d0c4
to
0de80ec
Compare
Lilyjjo
force-pushed
the
lilyjjo/check-tx-rewrite
branch
from
0a4f435
to
56ff638
Compare
| parked.add( | ||
| timemarked_tx, | ||
| match parked.add( | ||
| timemarked_tx.clone(), |
There was a problem hiding this comment.
i think you can do let id = timemarked_tx.id() before this, then use that later on in the function and remove the clone()
| if let Some(rsp) = insert_into_mempool(mempool, &state, signed_tx, metrics).await { | ||
| return rsp; | ||
| } |
There was a problem hiding this comment.
this function returning an Option that's only set if there's an error response is somewhat confusing, would prefer for this function to return a Result<(), response::CheckTx> so the error variant is the error response
same for check_removed_comet_bft
There was a problem hiding this comment.
Changed it, feels much better that way
| /// Returns a [`response::CheckTx`] if the transaction fails any of the checks. | ||
| /// Otherwise, it returns the [`SignedTransaction`] to be inserted into the mempool. |
There was a problem hiding this comment.
you can use a Result<SignedTransaction, response::CheckTx> as this function's return
|
I think this PR looks good, just a few smaller comments!
how does this work? with cometbft recheck? |
Hmmm. I think I misheard why we wanted to allow rechecks to add to the appside mempool. I checked out the CometBFT code and didn't see it persisting the transactions anywhere that was accessible, (I wonder if I missed it). In the scenario where CometBFT doesn't persist, then the allowing recheck would only be useful if CometBFT doesn't crash if the sequencer app crashses. I'll do some more research |
| @@ -220,6 +223,10 @@ impl Mempool { | |||
| ); | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Just above here and in run_maintenance, we do parked.find_promotables which potentially removes multiple txs from parked. We then try to do pending.add for each of these, but if that add call returns an error, we should remove that tx from self.contained_txs since it's lost from the mempool I think.
There was a problem hiding this comment.
good catch. added similar logic to the error cases in maintenance too. i'll add a metric for these synchronization errors and ask Ido to setup an alert for it
Lilyjjo
force-pushed
the
lilyjjo/check-tx-rewrite
branch
from
345fcab
to
af24538
Compare
Summary
Previously
handle_check_tx()'s processing ofCheckTx::Recheckwould 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 in the scenario where the sequencer restarts-- CometBFT persists transactions but our app mempool does not. The cache is used to allow transactions to re-enter the mempool on restarts.
This PR also restores the original behavior of
CheckTXto not kick out transaction needlessly on rechecks, this regression was added when upgrading the mempool in #1323.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 between rechecks 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
check_tx_removed_stale_noncecheck_tx_duration_seconds_check_tracked,mempool_tx_logic_errorcheck_tx_duration_seconds_check_noncetocheck_tx_duration_seconds_fetch_nonceRelated Issues
closes #1481