Read the EVM account nonce before taking the producer mempool lock - #4181
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
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 @@
## giga-1 #4181 +/- ##
==========================================
- Coverage 65.55% 65.54% -0.01%
==========================================
Files 2081 2076 -5
Lines 157460 157084 -376
==========================================
- Hits 103222 102964 -258
+ Misses 54097 53979 -118
Partials 141 141
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryMedium Risk Overview A new Tests add coverage for first-seen senders, concurrent sequential inserts across accounts, and bad-nonce rejection including replay after tracking exists. Reviewed by Cursor Bugbot for commit 4a98fe7. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Moving the app-nonce read outside the mempool lock is sound for the local lane: every m.evmNonces deletion in pruneMempool advances m.first, so the m.first == first guard correctly detects prunes, and the tracked entry still wins. Three non-blocking notes: the pre-read now runs ahead of the mempool-full short-circuit on TryInsertTx, the guard doesn't cover app-nonce advances from other lanes, and the new re-read branch is untested.
Findings: 0 blocking | 3 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 3 suggestion(s)/nit(s) flagged inline on specific lines.
| appNonce := utils.None[uint64]() | ||
| var first types.BlockNumber | ||
| if resp.IsEVM { | ||
| appNonce, first, err = s.preReadEvmNonce(mp, resp.EVMSenderAddress) |
There was a problem hiding this comment.
[suggestion] The pre-read now runs before the mempool-full check, so TryInsertTx (waitIfFull == false) pays for an app state view on every tx it is about to reject with errMempoolFull. Previously if m.IsFull() && !waitIfFull (line 352) short-circuited before any EvmNonce call, so a saturated mempool rejected cheaply — which is exactly the state the node is in under the load this PR targets.
preReadEvmNonce already holds the lock and can see m.IsFull(), so it could return errMempoolFull for the non-blocking caller and skip the read entirely. That stays correct because IsFull is re-checked under the main lock anyway, so the early answer is only a fast path for a condition that is already best-effort.
| // there is no entry and no block was pruned since it was taken (m.first | ||
| // unchanged), since pruning may delete this sender's entry and advance the | ||
| // app nonce. | ||
| if pre, ok := appNonce.Get(); ok && m.first == first { |
There was a problem hiding this comment.
[suggestion] The m.first == first guard catches prunes of the local lane, but a sender's app nonce can also advance from execution of blocks in another validator's lane — that never moves m.first and never touches m.evmNonces, so a stale pre-read survives the guard.
Concretely: account A is untracked locally, pre-read returns nonce N; before this goroutine acquires the main lock, a remote-lane block containing A's tx N executes and the app nonce becomes N+1; a concurrently submitted tx with nonce N+1 is then rejected with errBadNonce: got N+1, want N. Before this change the in-lock read returned N+1 and admitted it. The reverse ordering admits an already-executed nonce, which is wasted block space rather than a correctness problem.
This is narrow (it needs one account submitting through two nodes, and the tracked-entry path is already stale in that scenario), so I don't think it blocks. But the comment currently reads as if pruning were the only hazard; worth recording that the guard assumes the sender's app nonce only advances via a local-lane prune.
| require.Equal(t, nonce+2, env.state.EvmNextPendingNonce(addr)) | ||
| } | ||
|
|
||
| func TestInsertTx_ConcurrentSequentialNonces(t *testing.T) { |
There was a problem hiding this comment.
[suggestion] The new tests cover the two branches that were already reachable (fresh sender uses the pre-read, tracked entry wins) but not the branch this change actually adds: the m.first != first re-read. With alignLocalMempool and no State.Run, nothing seals or prunes blocks here, and the five senders are disjoint, so the guard is never exercised and a regression that dropped the m.first == first condition would still pass.
Two additions would pin the new behavior: force a prune between the pre-read and the lock (a test app whose EvmNonce blocks until pruneMempool has advanced m.first and bumped the app nonce) and assert the refreshed value is used; and assert the actual goal of the PR by holding EvmNonce open and checking another InsertTx/EvmNextPendingNonce still completes.
Also note TestInsertTx_BadNonceRejected duplicates TestMempool_BadNonce (line 348) except for the replay case — folding the replay assertions into the existing test would avoid two tests to keep in sync.
On autobahn-e2e load tests the producer mempool's
insertTxheld the mempool Watch lock while callingapp.EvmNoncefor every sender it had not seen yet. That read opens a state view in the app, so each first-seen sender stalled every other inserter and the block-sealing task inrunMempool, and under load-generator traffic the receiving validator spent most of its time serialised on that lock.The account nonce is now read before the main lock is taken. A brief lock first snapshots whether the sender is already tracked in
evmNoncesand the lane's current first block; if the sender is untracked,EvmNonceis read with no lock held. Under the main lock the in-memory entry, when present, always wins because it covers txs already sequenced but not yet executed. The pre-read value is used only when there is no entry andm.firsthas not moved since the snapshot; otherwise the nonce is re-read under the lock. This guards the race where a prune between the pre-read and the lock deletes the sender's entry and advances the app nonce, and it also covers the wait-for-capacity path for free, since capacity is only freed bypruneMempooladvancingm.first. Error semantics are unchanged;pruneMempoolkeeps its in-lockEvmNoncecall since it only runs on the path where some of a sender's txs failed to execute.New tests cover a first tx from an untracked sender, concurrent sequential nonces from several senders, and bad-nonce rejection including a replay after the in-memory entry exists; the existing mempool tests pass unchanged.