Repository navigation
Conversation
Blocked InsertTx calls were admitted one at a time, each woken by the one before it, and new calls queued behind them even with room in the mempool. Once a queue formed, admission ran at goroutine hand-off speed and the queue never drained under load. Whoever frees capacity now appends the queued txs in arrival order until the mempool is full again. Co-authored-by: Cursor <cursoragent@cursor.com>
…t of the queue, drop the wakeups metric Co-authored-by: Cursor <cursoragent@cursor.com>
…rain' into yzang/producer-admission-batching * origin/yzang/autobahn-admission-batch-drain: autobahn: reset pending inserts on close, keep non-waiting inserts out of the queue, drop the wakeups metric autobahn: admit the whole insert queue when lane capacity frees
…full without taking the mempool lock Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…ew in parallel Co-authored-by: Cursor <cursoragent@cursor.com>
…batch Co-authored-by: Cursor <cursoragent@cursor.com>
…une makes them stale Co-authored-by: Cursor <cursoragent@cursor.com>
… batched admission per insert Co-authored-by: Cursor <cursoragent@cursor.com>
PR SummaryMedium Risk Overview EVM-only executor adds Reviewed by Cursor Bugbot for commit 602e9d4. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## yzang/evmonly-settled-nonce #4638 +/- ##
===============================================================
+ Coverage 59.45% 59.48% +0.02%
===============================================================
Files 2038 2038
Lines 160009 160140 +131
===============================================================
+ Hits 95139 95256 +117
- Misses 64865 64879 +14
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
This PR batches waiting InsertTx calls into a per-session admission queue. One caller drains the queue, reading each batch's untracked sender nonces outside the mempool lock (one EvmNonces call, served first from a per-commit nonce index) and then appending under a single lock; prune also reads its unsettled senders in one batch. The blocker: the caller that starts admission keeps draining the queue for as long as new inserts arrive, so under the sustained load this PR targets its own InsertTx can block indefinitely and ignore context cancellation. I reviewed against the PR's merge tree (f87cc22). Both codex findings hold and are kept: the leader one is the blocker and the queue-bound one is a suggestion. My own addition is that queued txs drained after a prune still read nonces one at a time under the lock.
Blocking
1 finding on the changed lines, as inline comments.
Non-blocking
2 findings on the changed lines, as inline comments.
- No test covers a lead caller whose context is cancelled while it is draining, or
admitBatch's retry when a prune movesfirstbetween the nonce read and the locked append. Both are new paths and easy to break silently.
seidroid review · decision request_changes · session 9c9df99d1207408a92280fbf85efdc3a · turn resp_claude_2ea0520bc88e0d876a4015f38b9bd0ac · item d9b7f16dd7f45d6d8928b3604409c2a2
Findings: 1 blocking | 3 non-blocking | 3 posted inline
…ion-batch-drain * origin/main: (25 commits) evmonly: reuse one EVM and state DB per OCC worker, claim ranges, merge in parallel (#4610) giga: add giga.execution.gc_percent to raise the Go GC target (#4609) feat(giga-state-sync): add the state sync switch and the Autobahn layout (#4560) autobahn: export lane block and proposal latency by consensus step (#4606) Remove the wasm CLI commands and the generated [wasm] config (#4634) StateWAL emits Mutation objects DO NOT MERGE INTO 6.8 (#4628) Remove the unused x/store and x/evm/artifacts root packages (#4635) Remove unused contracts from example/ (#4630) Remove unused scripts and sei-wasmd contrib leftovers (#4624) writing qc + blocks atomically (#4599) Fix broken links and stale .PHONY names left by the cleanups (#4626) Remove support for pre-cancun blocks, simplify storage logic (#4575) Remove the unused upstream Tendermint e2e harness (#4623) Record sei-cosmos mockgen commands as go:generate directives (#4622) Move cosmovisor out of sei-chain into sei-protocol/cosmovisor (#4621) Remove the Cosmos Swagger UI and sei-cosmos contrib tooling (#4619) Remove the unused sei-cosmos, sei-tendermint and sei-wasmd Makefiles (#4620) Validate snapshot __metadata against the commit being published (#4613) flatKV audit command (#4600) Remove the vendored Tendermint spec (#4617) ...
…o yzang/producer-admission-batching * origin/yzang/evmonly-settled-nonce: perf(evmonly): open the view only when the in-flight commit lacks the sender's nonce evmonly: reuse one EVM and state DB per OCC worker, claim ranges, merge in parallel (#4610) giga: add giga.execution.gc_percent to raise the Go GC target (#4609) feat(giga-state-sync): add the state sync switch and the Autobahn layout (#4560) autobahn: export lane block and proposal latency by consensus step (#4606) Remove the wasm CLI commands and the generated [wasm] config (#4634) StateWAL emits Mutation objects DO NOT MERGE INTO 6.8 (#4628) Remove the unused x/store and x/evm/artifacts root packages (#4635) Remove unused contracts from example/ (#4630) Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # giga/evmonly/giga_store.go
…rain' into yzang/producer-admission-batching * origin/yzang/autobahn-admission-batch-drain:
…, and count queued admissions in the pending limit Co-authored-by: Cursor <cursoragent@cursor.com>
|
On the tests the seidroid review asked for, in 62166aa: |
There was a problem hiding this comment.
This update (merge commit 047335f) makes the caller holding the admission turn run one batch and then hand the turn to the oldest queued insert, counts admitQueue toward MaxPendingInserts, and adds tests for a cancelled turn, concurrent inserts and a prune during the nonce read. That fixes my leader blocker and the queue-bound finding, so nothing blocks; the locked nonce read when a prune drains waiters is acknowledged as a follow-up. Of codex's two new readings, I kept the close one as a suggestion. I dropped the bound one because the batch it leaves uncounted is limited to roughly one admitQueue's worth, and anything past the limit is still rejected with errPendingFull once the batch reaches the lock. I could not run the tests here because there is no Go toolchain.
Non-blocking
2 findings on the changed lines, as inline comments.
seidroid review · decision approve · session 9c9df99d1207408a92280fbf85efdc3a · turn resp_claude_9aa981ccd5994a194c8a7f2505fa501c · item 14eef540feca570f98e1774db3fb0b99
Findings: 0 blocking | 2 non-blocking | 2 posted inline
| for admitted < len(m.waiters) && !m.closed && !m.IsFull() { | ||
| t := m.waiters[admitted] | ||
| admitted++ | ||
| t.finish(s.appendTx(m, ctrl, t.tx, t.resp, t.gasWanted, t.gasEstimated, t.appNonce, t.first)) |
There was a problem hiding this comment.
suggestion — This still holds and you have agreed to a follow-up. Waiters drained by a prune always carry a stale t.first, so appendTx reads each untracked sender's nonce under the mempool lock one at a time, which is the pattern this PR removes everywhere else. Worth tracking so it is not lost once lane capacity or load changes.
There was a problem hiding this comment.
Tracked as Linear STO-852 (batch the nonce reads of waiters a prune admits), so it isn't lost when lane capacity or load changes.
| // close ends the session and releases every blocked InsertTx call. | ||
| func (m *mempoolInner) close(ctrl *utils.WatchCtrl) { | ||
| // close ends the session and fails every blocked InsertTx call with ErrNotProducing. | ||
| func (mp *mempool) close(m *mempoolInner, ctrl *utils.WatchCtrl) { |
There was a problem hiding this comment.
suggestion — (Raised by codex.) close fails only m.waiters. Tickets still in admitQueue learn of the close only when a batch reaches the mempool lock, so they wait out any in-flight EvmNonces read. The next leader also issues a nonce read for a session that is already closed. A cheap fix is for admitNextBatch or readUntrackedNonces to check mp.closed first and finish the batch with ErrNotProducing.
There was a problem hiding this comment.
Fixed in 602e9d4: admitBatch checks mp.closed first and finishes every non-cancelled ticket in the batch with ErrNotProducing, so a closed session's queued inserts no longer wait out a nonce read and the next turn holder issues none. TestInsertTx_CloseFailsTheInsertsWaitingForABatch closes the session with two inserts queued and checks both fail without an EvmNonce call.
Superseded: the latest review found nothing blocking in this change.
…e read Co-authored-by: Cursor <cursoragent@cursor.com>
Stacked on #4633 and carries #4608 as a merge; review the commits after the merge.
Problem
InsertTxcontended for the one mempool lock and read its sender's nonce from the app while holding it; prune did the same for every unsettled sender of each pruned block, starving the producer loop that frees capacity.SettledNoncealso scanned the in-flight commit's whole change list on every read.Change
poolCanAdmitrefuses a full or closed session from atomics, without the lock. Inserts waiting for a batch count towardMaxPendingInserts.admitNextBatch) and hands the turn to the oldest insert queued meanwhile, so no call drains arrivals indefinitely; a call that gives up while queued leaves the queue.EvmNoncescall outside the lock (Executor.SettledNonces: one view, parallel reads, served first from a per-commit address index), then appends in FIFO order under one lock. A prune since the read makes the batch read again outside the lock.pruneMempoolreads a pruned block's unsettled senders in one batch. The non-waiting path is a single try that never joins the queue.Impact
Risk
Tests
producer,evmonlyapp,proxyandgiga/evmonlypass with-race.