Skip to content

Read the EVM account nonce before taking the producer mempool lock - #4181

Merged
masih merged 6 commits into
giga-1from
masih/1789479455-nonce-outside-lock
Sep 16, 2026
Merged

masih merged 6 commits into
giga-1from
masih/1789479455-nonce-outside-lock

Conversation

@masih

@masih masih commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

On autobahn-e2e load tests the producer mempool's insertTx held the mempool Watch lock while calling app.EvmNonce for 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 in runMempool, 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 evmNonces and the lane's current first block; if the sender is untracked, EvmNonce is 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 and m.first has 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 by pruneMempool advancing m.first. Error semantics are unchanged; pruneMempool keeps its in-lock EvmNonce call 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.

@devin-ai-integration

Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@github-actions

github-actions Bot commented Sep 15, 2026

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedSep 16, 2026, 12:09 PM

@masih
masih requested review from wen-coding and a lite review from Copilot and removed request for Copilot September 16, 2026 10:08
@masih
masih marked this pull request as ready for review September 16, 2026 10:09
@codecov

codecov Bot commented Sep 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.54%. Comparing base (7e3ca24) to head (4a98fe7).
⚠️ Report is 10 commits behind head on giga-1.

Files with missing lines Patch % Lines
...i-tendermint/internal/autobahn/producer/mempool.go 88.88% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            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              
Flag Coverage Δ
sei-chain-pr 92.21% <88.88%> (?)
sei-db ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...i-tendermint/internal/autobahn/producer/mempool.go 94.93% <88.88%> (+5.08%) ⬆️

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@cursor

cursor Bot commented Sep 16, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes EVM nonce validation under concurrency and pruning; correctness relies on the m.first snapshot guard, though behavior and errors are intended to stay the same.

Overview
Moves app.EvmNonce off the hot path for EVM InsertTx when the sender is not yet in evmNonces, so admission no longer holds the mempool watch lock during expensive app state reads.

A new preReadEvmNonce takes a short lock only to see whether the address is already tracked and to snapshot m.first, then reads the app nonce without the lock. appendTx still treats in-memory evmNonces as authoritative; it uses the pre-read value only when there is no entry and m.first matches the snapshot, otherwise it re-reads under the lock to handle prunes that raced the pre-read. The pre-read happens before the capacity-wait loop, so it also applies while callers block for mempool space.

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.

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@masih
masih enabled auto-merge September 16, 2026 12:08
@masih
masih requested a review from shemnon September 16, 2026 12:14
@masih
masih added this pull request to the merge queue Sep 16, 2026
Merged via the queue into giga-1 with commit 61d6710 Sep 16, 2026
62 checks passed
@masih
masih deleted the masih/1789479455-nonce-outside-lock branch September 16, 2026 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant