crypto-org-chain / crypto-org-chain/cronos
app-side mempool follow-ups (#2091)
- Dominant language
- Go
- Stars
- 336
- Forks
- 299
- Avg merge
- 2d 17h
- Merged PRs (30d)
- 4
Description
## 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:
- **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.
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()` duration
`app/app.go`. Peer `InsertTx` and RPC `CheckTx` block for the entire store commit / disk IO
+ streaming-listener window (100–500ms under load).
- Direction: narrow the mutex to the `checkState` reset; requires memiavl reads safe
during commit (SDK/store change).
- Ref: https://github.com/crypto-org-chain/cronos/pull/2091#discussion_r3412734527
### 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`.
- Ref: https://github.com/crypto-org-chain/cronos/pull/2091#discussion_r3413420463
## 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`.
- 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.
- Refs: https://github.com/crypto-org-chain/cronos/pull/2091#discussion_r3413717119
### 4. Nonce-gap-after-eviction cleanup
`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:
1. Alice has nonce `4`, `5`, `6`; not in recent blocks → not staged for recheck.
2. nonce `5` is evicted (timeout/TTL/other); nonce `6` stays with a gap.
3. Recheck never runs for Alice; proposal ante is skipped.
4. 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.
- Refs: https://github.com/crypto-org-chain/cronos/pull/2091#discussion_r3413441164
### 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.
- Refs: https://github.com/crypto-org-chain/cronos/pull/2091#discussion_r3413493268
## Proposed sequencing
1. Group A (items 1, 2) — SDK/store prerequisites; independent, can proceed in parallel.
2. Item 3 — async recheck (the enabler).
3. Item 4 — nonce-gap cleanup (independent, low-risk; can land any time).
4. 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.
Cronos could be redesigned as:
AdmissionManager
- InsertTxHandler
- CheckTxHandler
- BaseApp checkState serialization
Rechecker
- owns recheck lifecycle
- owns recheck context or holds batch lock
- StageCommittedBlock
- StageSkippedSenders
- RunAfterCommit
- timeout/TTL eviction
- deferred batch
like part 5
## References
- PR: #2091; discussion threads linked per item.
Contributor guide
Research direction
Start by reading app/app.go and app/mempool/admitter.go, then trace the recheck path and the dependencies between items 3–5. Compare the current RunTx recheck behavior with the proposed lifecycle changes and review the SDK/store concerns in app/mempool/helpers.go. Done includes differential tests covering stale nonce, baseFee drift, timeout/TTL, and recheck backlog validity semantics.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend-api-design, blockchain, performance
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 28/100