fix: prevent cosmos mempool proposal starvation under backlog - #1229
fix: prevent cosmos mempool proposal starvation under backlog#1229mmsqe wants to merge 23 commits into
Conversation
* carry rechecked cosmos snapshot across heights instead of reset in each block * guard re-adds with a committed-nonce watermark pruned synchronously at FinalizeBlock * serve last completed snapshot when recheck loop falls behind a proposal
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1229 +/- ##
==========================================
+ Coverage 67.41% 67.59% +0.17%
==========================================
Files 320 320
Lines 23451 23577 +126
==========================================
+ Hits 15810 15937 +127
+ Misses 6459 6457 -2
- Partials 1182 1183 +1
🚀 New features to boost your workflow:
|
|
@greptile review |
Greptile SummaryThis PR fixes cosmos mempool proposal starvation by carrying the validated tx snapshot across block heights instead of resetting it to empty on each new block. A recheck pass cancelled mid-iteration no longer discards all previously validated state, so proposals served while the recheck loop is behind always see a non-empty snapshot.
Confidence Score: 5/5Safe to merge; the carry-forward clone is always taken under the HeightSync write lock preventing any concurrent PruneCommitted from racing the clone, and AgeWatermarks is only called after a fully-completed uncancelled pass. The watermark aging, signer-index scans, stale-fallback path, and carry-forward clone all check out under concurrent access. The locking hierarchy is consistently respected and test coverage is comprehensive including synctest-guarded race conditions. mempool/tx_store.go — the filterSignerBucketsLocked double-visit invariant for multi-signer buckets is correct but relies on predicates being idempotent; worth watching if new predicates are added. Important Files Changed
Reviews (2): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
* retire watermarks once a completed recheck pass revalidates pool: bounds maps and heals marks from optimistically-executed blocks that never committed * drop committed unordered txs by identity instead of watermarking (ChooseNonce value is timestamp instead of sequence) * prune cosmos snapshot for committed EVM txs, which consume same account sequence
* drop failed-recheck txs at detection time since end-of-pass removal loop is skipped on cancellation * invalidate a replaced tx whose signer set differs from its replacement via replacement hook, where InvalidateFrom cannot see it * stop carrying unkeyed txs across heights: they are unremovable and would duplicate every pass
index buckets by signer so PruneCommitted and InvalidateFrom visit only matching buckets instead of probing whole store
|
cc @mattac21 |
|
@greptile review again |
mattac21
left a comment
mattac21
left a comment
There was a problem hiding this comment.
I like the approach, I've done a quick pass over the pr but a few questions before I do a deeper review. This would be changing some of the guarantees of the evm (cosmos + evm) memepool, right? This would now allow the mempool to return a set of txs that are actually invalid on the latest state according to ante handlers? However it would not ever return a tx that was previously committed.
| // validated txs. Safe to serve as long as producers keep it free | ||
| // of state a since-committed block invalidated (the cosmos pool | ||
| // does this via its committed-nonce watermark). |
There was a problem hiding this comment.
// Safe to serve as long as producers keep it free
// of state a since-committed block invalidated (the cosmos pool
// does this via its committed-nonce watermark).
If this is safe to serve would really depend on the chains PrepareProposal and ProcessProposal implementations, correct? I'm assuming you mean this is safe because PrepareProposal/ProcessProposal would catch a block where the proposer serves stale txs that are actually invalid on the latest state, but they were proposed anyway via this?
There was a problem hiding this comment.
Yes, per-tx re-verification is the backstop, PrepareProposalVerifyTx drops failing candidates, ProcessProposal reruns ante, and committed txs can't reexecute past sequence/nonce checks at FinalizeBlock. safe here is narrower, not left to handlers: producers keep carried store free of just-committed txs, or proposer burns its window rejecting them. Enforced at source: PruneCommitted runs during FinalizeBlock before next PrepareProposal reads it, and AddTx's watermark blocks racing re-adds.
There was a problem hiding this comment.
i see yeah, my concern is similar to #1229 (comment) where we dont use any ante verification during prepare proposal in evmd because of the current guarantees that this changes. that was done for perf reasons, so im curious how that change + this would modify perf.
There was a problem hiding this comment.
Just measured on a 200-tx block: steady-state PrepareProposal stays at main's cost (0.93–1.10ms vs 0.94–1.06ms) since txs the mempool proves validated at head skip ante, only stale carried txs pay full ante (bounded by block gas), and recheck passes end ~1.9x faster (~79 -> ~42us/tx)
|
Also have you tested this under some load and has it properly solved #1227? I'm assuming you all have experienced the same issue? |
Yes, snapshot can now serve txs that would fail ante on latest state ( |
I see, we would want to change evmd to not use the no verify implementation of |
sorry for the reviewing delay @mmsqe , ill give this a more thorough review next week and hopefully get it merged |
There was a problem hiding this comment.
I think this generally looks good, another question I have is what actually was the bug that you/the issue reporter is experiencing. I agree that this seems like it would fix it, but why is every block empty past a certain threshold? The way this works pre this pr is that the pool will recheck as many txs as possible until a request for a proposal is received, and at that point it will return back whatever txs it has rechecked against the latest state. This may not be a lot of txs if we were late to finalize the prev block, but it should be something sometimes, rather than always nothing which is surprising. Do you know why that is?
| // fields of the Store itself | ||
| mu sync.RWMutex | ||
|
|
||
| // staleFallback makes GetStore return the current carried-forward Store |
There was a problem hiding this comment.
we should modify evmd to not use the custom no verify process proposal handler, since this now breaks the assumption it makes that all batches reaching it are valid
There was a problem hiding this comment.
Yes, evmd now runs BaseApp ante over any proposal tx the mempool can't prove was validated at head (SnapshotVerifiedTxVerifier), at-head entries just encode, keep old no-verify cost in steady state.
| // before it validates anything does not present an empty snapshot to proposals. | ||
| // Before the fix, StartNewHeight reset the store to empty each height, so a | ||
| // pass that had not yet re-added txs exposed a zero-length snapshot. | ||
| func TestRecheckMempool_CarryForwardSurvivesCancellation(t *testing.T) { |
There was a problem hiding this comment.
i would love to see this test in a more real world condition that reproduces the empty block issue, without manually blocking the ante execution which would trivially produce all empty blocks
* a carried-forward snapshot is validated at a height <= the one being proposed, so evmd can no longer assume every tx the mempool selects is still valid. * drop NoCheckProposalTxVerifier and let BaseApp's default verifier run ante over each selected tx, cost is bounded by proposal's gas budget, not pool size.
* stamp snapshot entries with the height their ante validation ran against: a pass stamps what it re-adds, carried entries keep the stamp of the pass that validated them. * byTx indexes stored pointer-typed txs straight to their stamp, so ValidatedAt is one map hit and a pass re-adding a known tx only refreshes its stamp — no signer extraction (cgo pubkey decompression) or fmt-built keys per candidate. Removals and slot overwrites unindex, so a replaced or pruned tx never keeps vouching. * evmd's SnapshotVerifiedTxVerifier encodes entries stamped at head and runs full ante only for carried ones — exactly the txs that may have become invalid. Head comes from an atomic mirror of the pinned context's height. * 200-tx proposal: 1.0ms encode-only (main), 16ms unconditional ante, 0.93–1.10ms with this verifier in the steady state.
* recheck passes now run with IsReCheckTx, the contract CometBFT's own recheck uses. * audited every IsReCheckTx site in the assembled chain; IBC's RedundantRelayDecorator now runs its recheck-mode eviction as upstream intended. * 200-tx recheck pass: 80us/tx -> 54us/tx.
* bech32 parsing reaches it several times per tx * pin scope to app name to reduce lookup to a map hit * win applies to every GetConfig caller in the node, not just mempool * 200-tx recheck pass: 54us/tx -> 44.5us/tx
CurrentBlock rebuilds header from a fresh query context plus keeper reads on every call, paid by proposal path via the iterator's base-fee lookup and by every EVM insert's fee-cap check. 200-tx recheck pass: ~42us/tx (main: ~79us/tx), proposal steady state 0.93-1.10ms, parity with main.
GetStore's height-behind branch has no partial-results path, so one height of recheck lag returns nil instead of smaller set, and with the cap-1 event-bus subscription one missed beat drops subscriber permanently — past the backlog threshold the failure is a cliff instead of gradient: every later proposal is empty. This PR fixes what gets served while loop lags, but not the permanence: with a dead subscription the carried snapshot drains as its txs commit and nothing refills it. stalepin_main is the other half — driving |
* a stale mark (optimistically executed block that never committed): ante succeeds, write() bumps the cached sequence, AddTx silently rejects — later same-signer txs enter the snapshot as a nonce gap * ante disambiguates: real marks still evict via ErrWrongSequence; under a stale mark, skip the tx and its dependents without evicting until the mark ages out
* the skip predicate trusted the notify-driven pin, which can lag the last commit (or die with the event bus), letting stale entries skip ante * record req.Height-1 on the verifier per proposal; stamps behind base fail closed into re-verification
* base-height re-verification already rejects committed txs (wrong sequence), and entries stamped at base cannot be committed ones — the watermark only pre-filtered what the verifier rejects anyway, at the cost of the branch's trickiest invariants: two-generation aging, optimistic-execution false marks, and silent AddTx rejection with its nonce-gap cascade * stale-fallback contract becomes single-sided: consumers must re-verify entries not validated at the proposal base, which evmd's proposal handler does by default * store's signer extractor reverts to the SDK default — the Eth wrapper existed only so PruneCommitted could read EVM tx signers * 200-tx bench unchanged: recheck ~42us/tx, proposal 0.82-0.95ms steady state
logs per-pass and per-proposal timings
4 participants
Description
200-tx bench:
Closes: #1227
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
mainbranch