You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
PR #2091 introduced the opt-in app-side mempool (mempool.type=app). Several concurrency
/ behavior concerns were raised in review and deliberately deferred so they wouldn't
block the feature。
The items fall into two groups:
Group A (1–2) — admission-path lock contention rooted in the SDK/store layer: the
AdmissionMutex serializes admission against Commit() because memiavl is not
read-concurrent with commit (SetTree swaps the tree pointer unguarded; rs.db.Commit()
mutates the tree) and cometBFT's AppMempool.Lock() is now a no-op. These need
SDK/store changes and are independent prerequisites.
Group B (3–5) — a recheck redesign. Recheck routes through BaseApp.RunTx (mutates
shared checkState), so it's synchronous and lock-bound. Making it async (3) and adding
nonce-gap cleanup (4) both add weight/independence to the recheck path, and the Admitter/RecheckScheduler split (5) is the capstone that consolidates them. The
split should not be done standalone — without 3, both RunTx paths still share the checkState lock, so it'd just spread one mutex across two structs.
2. PoolSnapshot O(N) scan under the mempool write lock
app/mempool/helpers.go. SelectBy holds mp.mtx for the full pool iteration (twice per
block: recheck + reap), blocking Insert/Remove. Bounded today by MaxTx.
Direction: SDK mempool implementation allowing a lock-light snapshot.
Telemetry for PoolSnapshot latency added in f88d025.
3. Make post-commit recheck async (off the commit path)
app/app.go. App.Commit() doesn't return until the full recheck batch completes,
adding to commit latency before the next PrepareProposal.
Direction: investigate moving recheck to a cancellable background worker that runs the
ante on a branched/cloned context (so it would no longer touch checkState / need the
mutex). This must prove semantic equivalence with the current RunTx-based recheck —
in particular the cross-tx state accumulation, where sequential nonces in one batch see
each other's writes via checkState.Write. Only if equivalence holds is this the enabler
for item 5.
Note: a naive async attempt did not improve testground scores — the win (if any)
requires the checkState-decoupling above, not just a goroutine.
app/mempool/admitter.go. Recheck is selective (only senders in recent blocks) and
proposal-time ante is skipped (CacheProposalTxVerifier encodes only), so an orphaned
higher-nonce tx can survive into a proposal:
Alice has nonce 4, 5, 6; not in recent blocks → not staged for recheck.
nonce 5 is evicted (timeout/TTL/other); nonce 6 stays with a gap.
Recheck never runs for Alice; proposal ante is skipped.
nonce 6 enters a proposal and fails ante at FinalizeBlock.
Impact (bounded, not a safety issue — deterministic fail, block stays valid):
invalid higher-nonce txs stay proposal-eligible; wasted block space; valid txs displaced
when the block is near-full. Bounded: orphans eventually age out via their own TTL when ttlNumBlocks > 0 (with TTL disabled, EVM txs carry TimeoutHeight=0 and have no eviction
path).
Fix: when an eviction creates a gap, add that sender to the current recheck set
(pending) so its remaining txs are re-validated and the orphans purged — instead of
relying on the sender re-appearing in a committed block or on TTL aging. Adds logic to
the recheck path → reinforces the case for item 5.
Maintainer view: acceptable as a known limitation.
5. Split Admitter into Admitter + RecheckScheduler (capstone)
app/mempool/admitter.go. Two lifecycles with different concurrency contracts:
latency-sensitive admission vs. throughput-oriented recheck/TTL eviction.
Do this after 3 (and alongside/after 4). Once recheck is async on a branched context
it no longer shares the checkState lock, so the split maps to a real concurrency
boundary and delivers "recheck can't stall admission" + isolated testability. The
nonce-gap logic (4) further justifies a dedicated recheck component.
Standalone (before 3) it's not worth it — just one mutex shared across two structs.
Group A (items 1, 2) — SDK/store prerequisites; independent, can proceed in parallel.
Item 3 — async recheck (the enabler).
Item 4 — nonce-gap cleanup (independent, low-risk; can land any time).
Item 5 — split, once 3 (and ideally 4) are in.
Notes
Items 1–3, 5 are performance/latency, not correctness — current behavior is safe, just
contended/synchronous. Item 4 is a bounded efficiency/behavior gap, also not a safety issue.
differential tests
Please add differential tests showing the app-mempool fast path produces proposal blocks with the same validity semantics as the default proposal path, especially for stale nonce, baseFee drift, timeout/TTL, and recheck backlog cases.
Issue:
RunTx(ExecModeReCheck) can write successful ante changes into BaseApp checkState. Since runRecheck unlocks between candidates, InsertTx / RPC CheckTx can interleave and mutate the same checkState mid-batch. Later recheck candidates then observe timing-dependent admission
simple fix: hold Manager.mu across the whole RunTx(ReCheck) batch, or
bigger redesign: introduce a Cosmos EVM-style dedicated rechecker context and avoid using shared BaseApp checkState for batch recheck.
Background
PR #2091 introduced the opt-in app-side mempool (
mempool.type=app). Several concurrency/ behavior concerns were raised in review and deliberately deferred so they wouldn't
block the feature。
The items fall into two groups:
AdmissionMutex serializes admission against
Commit()because memiavl is notread-concurrent with commit (
SetTreeswaps the tree pointer unguarded;rs.db.Commit()mutates the tree) and cometBFT's
AppMempool.Lock()is now a no-op. These needSDK/store changes and are independent prerequisites.
BaseApp.RunTx(mutatesshared
checkState), so it's synchronous and lock-bound. Making it async (3) and addingnonce-gap cleanup (4) both add weight/independence to the recheck path, and the
Admitter/RecheckSchedulersplit (5) is the capstone that consolidates them. Thesplit should not be done standalone — without 3, both RunTx paths still share the
checkStatelock, so it'd just spread one mutex across two structs.Dependency:
3 (async) + 4 (nonce-gap) → 5 (split).Group A — admission-path lock contention (SDK/store layer)
1. AdmissionMutex held for the full
BaseApp.Commit()durationapp/app.go. PeerInsertTxand RPCCheckTxblock for the entire store commit / disk IOcheckStatereset; requires memiavl reads safeduring commit (SDK/store change).
2.
PoolSnapshotO(N) scan under the mempool write lockapp/mempool/helpers.go.SelectByholdsmp.mtxfor the full pool iteration (twice perblock: recheck + reap), blocking
Insert/Remove. Bounded today byMaxTx.PoolSnapshotlatency added inf88d025.Group B — recheck redesign (capstone = the split)
3. Make post-commit recheck async (off the commit path)
app/app.go.App.Commit()doesn't return until the full recheck batch completes,adding to commit latency before the next
PrepareProposal.ante on a branched/cloned context (so it would no longer touch
checkState/ need themutex). This must prove semantic equivalence with the current
RunTx-based recheck —in particular the cross-tx state accumulation, where sequential nonces in one batch see
each other's writes via
checkState.Write. Only if equivalence holds is this the enablerfor item 5.
requires the
checkState-decoupling above, not just a goroutine.4. Nonce-gap-after-eviction cleanup
app/mempool/admitter.go. Recheck is selective (only senders in recent blocks) andproposal-time ante is skipped (
CacheProposalTxVerifierencodes only), so an orphanedhigher-nonce tx can survive into a proposal:
4,5,6; not in recent blocks → not staged for recheck.5is evicted (timeout/TTL/other); nonce6stays with a gap.6enters a proposal and fails ante atFinalizeBlock.Impact (bounded, not a safety issue — deterministic fail, block stays valid):
invalid higher-nonce txs stay proposal-eligible; wasted block space; valid txs displaced
when the block is near-full. Bounded: orphans eventually age out via their own TTL when
ttlNumBlocks > 0(with TTL disabled, EVM txs carryTimeoutHeight=0and have no evictionpath).
(
pending) so its remaining txs are re-validated and the orphans purged — instead ofrelying on the sender re-appearing in a committed block or on TTL aging. Adds logic to
the recheck path → reinforces the case for item 5.
5. Split
AdmitterintoAdmitter+RecheckScheduler(capstone)app/mempool/admitter.go. Two lifecycles with different concurrency contracts:latency-sensitive admission vs. throughput-oriented recheck/TTL eviction.
it no longer shares the
checkStatelock, so the split maps to a real concurrencyboundary and delivers "recheck can't stall admission" + isolated testability. The
nonce-gap logic (4) further justifies a dedicated recheck component.
Proposed sequencing
Notes
contended/synchronous. Item 4 is a bounded efficiency/behavior gap, also not a safety issue.
differential tests
Please add differential tests showing the app-mempool fast path produces proposal blocks with the same validity semantics as the default proposal path, especially for stale nonce, baseFee drift, timeout/TTL, and recheck backlog cases.
Issue:
RunTx(ExecModeReCheck) can write successful ante changes into BaseApp checkState. Since runRecheck unlocks between candidates, InsertTx / RPC CheckTx can interleave and mutate the same checkState mid-batch. Later recheck candidates then observe timing-dependent admission
simple fix: hold Manager.mu across the whole RunTx(ReCheck) batch, or
bigger redesign: introduce a Cosmos EVM-style dedicated rechecker context and avoid using shared BaseApp checkState for batch recheck.
Cronos could be redesigned as:
AdmissionManager
Rechecker
like part 5
References