fix(mempool): mempool-owned branched context for admission + recheck - #2159
Draft
JayT106 wants to merge 13 commits into
Draft
fix(mempool): mempool-owned branched context for admission + recheck#2159JayT106 wants to merge 13 commits into
JayT106 wants to merge 13 commits into
Conversation
…context Admission and recheck shared baseapp's checkState as the pending-nonce store. Give the app mempool its own CacheMultiStore, branched off the committed store and refreshed inside the existing admission-mutex span at Commit, and pass it as RunTx's txMultiStore at all three call sites (admit, CheckTxHandler, runRecheck). All three must move together: the branch is the sole nonce authority, so a split would leave one path reading state reset at every Commit. A generation counter lets an in-flight recheck pass abandon candidates validated against a superseded branch; the unreached candidates' senders are re-merged into staging so the next pass re-covers them.
…gap evictions runRecheck took stateMu once per candidate, so an admission could land between two txs of the same sender and make evictions timing-dependent. Bucket candidates by the signer the mempool orders by and take stateMu once per group: a sender's nonce chain now advances atomically against other senders' admissions, while the hold time stays bounded by that sender's queue depth instead of the whole batch. Encoding moves out of the lock, and the generation check now runs under stateMu before each group, so a group is never split mid-flight. On a nonce failure, evict the remaining higher-nonce siblings without a RunTx each. Only when the gap is provable: an earlier tx in the group passed this pass, so lastOK+1 is the expected nonce, and the failing nonce is strictly above it. A wrong-sequence failure can also mean a stale (already committed) nonce, whose successor may be valid — cascading there would evict good txs. Disabled for any group that isn't the signer's contiguous ascending view.
Manager grew into one struct holding admission, recheck staging, selection, and the shared execution state. Split it along the boundary the branched context made explicit: - exec.go: txExec owns the admission mutex, mempoolState, the generation counter, and the codecs. Both halves run txs through it, so this state belongs to neither alone. - admitter.go: admit, InsertTx/CheckTx handlers, cacheTx. - scheduler.go: sender staging, candidate selection, TTL/timeout eviction, recheck grouping, and the async worker. Manager is now a facade over the three, so app.go and the proposal handler call sites are unchanged. Lock order is unchanged: recheckMu > txExec.mu > stagingMu, with mempoolState.mu innermost.
The fast path (mempool.type=app with the encoder cache) trusts admission and recheck, so it only encodes each pooled tx instead of re-running the ante like the default handler does. Nothing captured what that buys or costs. Run both handlers over identically seeded pools and assert the boundary: - all-valid pool and a same-sender nonce gap: identical selections and pools, since the gap guard lives in the shared DefaultProposalHandler sequence tracking. - stale nonce, recheck backlog, timeout height: the fast path proposes txs the ante rejects and leaves them pooled for recheck instead of evicting them mid-proposal. - baseFee drift: selections match because the proposal gate replaces the ante's fee check; only the pool differs, as a gated tx stays pooled. Each divergent case also runs the real ProcessProposal over the fast path's proposal with a non-empty blocklist and asserts ACCEPT: cronos ProcessProposal is blocklist-only, so an ante-invalid tx cannot make peers reject the block. Pooled txs are a local diffTx carrying its own signer, nonce, fee, gas, and timeout, so no account keeper or real codec is needed.
Sort recheck groups ascending by seq and disable cascade for co-signers of a multi-signer tx, whose nonces the keyed group cannot see.
The flat batch cap could hand a sender's higher-nonce txs to a later cycle without their prefix, so they failed wrong-sequence against a freshly rebranched base and were evicted while valid. Cap whole groups instead, run each group in bounded chunks so a deep queue can't stall Commit, and read the generation counter after the pool scan rather than before it.
The deferred carry is keyed on tx identity, so a fee bump replacing a carried tx at the same nonce dropped it from the next cycle's group and took the live tail down as a false wrong-sequence failure; carry the senders too. Also keep cascade eviction inside the chunked mutex hold, and tighten the recheck test runner to reject stale nonces like the real ante does.
cascadeChunkLocked now spends one RunTx on a chunk's head before blind- evicting the rest, since the lock releases between chunks and a same- sender admission can fill a gap proven in an earlier chunk. Also guard unordered txs out of cascadable grouping, and carry unreached senders into deferred on a gen-abort so a low-priority tail can't be starved by sustained aborts.
Both eviction paths remove a tx from the pool without spending a RunTx, so the EVM ante's per-(sender, nonce) admission cache never learns the slot is free and skips nonce verification on a resubmit at that nonce. Add an eviction hook the scheduler fires with (sender, nonce) on every eviction; wire it to the ante cache's Delete in app.go. Also: cascadeChunkLocked no longer assumes any head failure proves the gap survived — only a nonce error does; other failures (e.g. funds) fall through to per-candidate rechecking for the rest of the chunk.
…ames evict fired the ante nonce-cache hook only for the group's key signer. A multi-MsgEthereumTx tx stages one ante-cache entry per msg, so a second-and-later signer's entry leaked on cascade/TTL eviction the same way round 5 fixed for the key signer. evict now enumerates all signers via GetSigners when the evicted tx is multi-signer.
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: JayT106 <JayT106@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+98
to
+100
| for sg := range senders { | ||
| s.recheckSenders[sg] = struct{}{} | ||
| } |
| // Pass 1: evictions. Collect senders of evicted txs so their remaining pool txs | ||
| // (e.g. higher-nonce siblings) are rechecked — they become invalid after the gap. | ||
| var evictedSet map[sdk.Tx]struct{} // nil until first eviction; nil-map read is safe | ||
| now := time.Now() |
| // nonce state, so run the rest of the chunk one RunTx at a time instead | ||
| // of assuming the gap held. | ||
| evicted, cascaded, next, gapFound = s.runCandidatesLocked(g, start+1, end, nonceCursor{}) | ||
| return evicted + 1, cascaded, next, gapFound, true |
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
2 participants
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.
What
Gives the app-side mempool (
mempool.type=app) its own working state instead of sharing baseapp'scheckStatebetween admission and recheck. Touchesapp/mempool/(newmempoolState,txExec/admitter/recheckSchedulersplit) andapp/app.go(refresh-at-Commit wiring, ante nonce-cache eviction hook).Issue
Recheck used
checkStateand released the lock between candidates, so concurrentInsertTx/CheckTxinterleaved with recheck's own writes into that state, making evictions timing-dependent (#2109). All threeRunTxcall sites (admit,CheckTxHandler,runRecheck) now run against aCacheMultiStorebranched off the committed store and owned by the mempool, refreshed under the admission mutex right after eachCommit.Determinism during recheck is handled separately: candidates are grouped per signer and run in bounded chunks, so a signer's nonce chain stays contiguous and the mutex hold per chunk stays short regardless of queue depth.
Solution
mempoolState: mempool-ownedCacheMultiStore, refreshed atCommit, nil-safe fallback tocheckState.Managersplit intotxExec(mutex + state + codecs),admitter(admission),recheckScheduler(staging/grouping/recheck).maxRecheckBatch) — never splits a sender's nonce chain across a cycle.recheckChunkSize) so one oversized sender's queue can't stallCommit.RunTxbefore blind-evicting, since the lock releases between chunks.RunTx) also drops the matching entry in ethermint's per-(sender, nonce)ante cache — otherwise a stale entry lets a resubmit skip nonce verification. Fires once per signer named by the evicted tx, not just the pool's key signer.app/proposal_diff_test.godifferentially tests the fastPrepareProposalpath against the default full-ante path.Test
go test -tags objstore -mod=mod ./app/... -race -count=1(bothappandapp/mempool),golangci-lint runclean,go build -tags objstore -mod=mod ./app/...clean. New/updated tests cover: nonce continuity across the branched store, generation-based cancellation on a superseded pass, group-boundary capping without splitting a sender, chunked cascade eviction and its cross-chunk re-verification, multi-signer/unordered cascade guards, and eviction-hook firing (including the multi-signer case).Design notes:
docs/architecture/mempool-branched-recheck-context.md.