mempool/evmonlyapp: read nonce/balance through the pending overlay; fetch first-seen accounts outside the store lock - #4271
Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
PR SummaryHigh Risk Overview
Mempool prefetches first-seen sender balance/nonce outside the store mutex; Reviewed by Cursor Bugbot for commit 313568b. 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 @@
## devin/1789943071-stack-1-commit-behind-finalize #4271 +/- ##
====================================================================================
+ Coverage 75.42% 89.41% +13.99%
====================================================================================
Files 10 33 +23
Lines 1778 4499 +2721
====================================================================================
+ Hits 1341 4023 +2682
- Misses 436 476 +40
+ Partials 1 0 -1
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.
The pipelined latest-account read and the out-of-lock mempool prefetch both look correct: the pipelineGeneration guard is checked after the view is opened (so an unchanged generation proves the view holds nothing newer than the overlay), accountsEpoch is bumped at every accounts reset site, and LatestAccount.Balance can never be nil. Only non-blocking efficiency/observability suggestions.
Findings: 0 blocking | 3 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
EvmNonceandEvmBalanceeach run a fulllatestAccount(view open + generation check + overlay), sotxStore.prefetchAccountandtxStore.inserteach do two independent pipelined reads per first-seen account, and the balance/nonce pair they fold into oneevmAccountcan come from two different blocks. The newevmonly.LatestAccountalready returns both together — a single combined method on the ABCIApplicationsurface would halve the work and make the pair consistent. (The two-call shape is pre-existing; this PR adds the API that would let it collapse.) - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
| reader := pending.overlay(gigaSnapshotStateReader{snapshot: snapshot, missingState: e.missingState}) | ||
| if rowReader, ok := reader.(accountSnapshotReader); ok { | ||
| if row, ok := rowReader.ReadAccount(addr); ok { |
There was a problem hiding this comment.
[suggestion] ReadAccount is the full-row read: gigaSnapshotStateReader.ReadAccount additionally does r.snapshot.GetCode(addr) (plus a cloneBytes copy) whenever row.CodeHash != EmptyCodeHash, and LatestAccount throws that code away.
For EOAs — the mempool hot path this PR targets — that costs nothing. But EvmBalance/EvmNonce are also the backing for eth_getBalance and eth_getTransactionCount (rpc/core/mempool.go → Environment.EvmBalance/EvmTransactionCount → latestAccount), which accept arbitrary addresses. Against a contract that is a new per-call read and copy of up to 24 KiB of bytecode; the previous openSettledView path (AccountExists + GetBalance) never touched the code store.
Since only balance and nonce are wanted here, reading reader.GetBalance(addr) / reader.GetNonce(addr) directly (or adding a code-free row read) would avoid it.
There was a problem hiding this comment.
Deliberately not changed here: this stack re-opens the already-merged code byte-for-byte so the team can review what is actually running on giga-1 (the top of the stack equals current giga-1). Agreed this is a real improvement; tracking it as a follow-up to land on top once the stack has been reviewed, unless the reviewers prefer it folded in.
| // has failed, it reads the settled store instead. | ||
| func (a *evmOnlyApplication) latestAccount(address common.Address) evmonly.LatestAccount { | ||
| if executor, ok := a.settler.Load().Get(); ok { | ||
| if account, err := executor.ReadLatestAccount(address); err == nil { |
There was a problem hiding this comment.
[suggestion] The error from ReadLatestAccount is discarded entirely. The two documented cases are fine — before InitChain the settler is empty, and a failed commit is logged once by openSettledView — but the remaining error (errors.New("giga store returned a nil snapshot")) falls through with no signal at all.
The fallback is openSettledView, which calls AwaitCommits() and therefore blocks. So if the store ever starts handing back nil views, every CheckTx admission silently reverts to settling the pipeline on each call — precisely the regression this PR removes — and nothing in the logs says so. A one-shot log on the non-nil, non-failure error (mirroring settleFailureLogged) would make that visible.
There was a problem hiding this comment.
Deliberately not changed here: this stack re-opens the already-merged code byte-for-byte so the team can review what is actually running on giga-1 (the top of the stack equals current giga-1). Agreed this is a real improvement; tracking it as a follow-up to land on top once the stack has been reviewed, unless the reviewers prefer it folded in.
43c1a5c to
c40a502
Compare
c40a502 to
313568b
Compare
| if moved { | ||
| return LatestAccount{}, false | ||
| } | ||
| reader := pending.overlay(gigaSnapshotStateReader{snapshot: snapshot, missingState: e.missingState}) |
There was a problem hiding this comment.
is it possible that N's changes haven't finished committing when N+2 has begun execution? In that case missingState would carry changes from N+1 but not from N unless missingState itself is stacked
There was a problem hiding this comment.
No — at most one block is ever uncommitted. executePreparedBlockWithStore calls awaitPipelineCommit() (lands N) right before startPipelineCommit for N+1, and it holds storeMu for the whole block, so N+2 cannot begin executing until N+1 has returned, i.e. until N is in the store. That is the invariant the pendingChanges overlay relies on: the view holds ≤ N, pending is exactly N+1, nothing in between can be missing. The pipelineGeneration recheck in readLatestAccount covers the one race that remains — a commit for N+2 starting between the read of pending and the view being opened — by retrying rather than replaying N+1 over a view that already contains N+2.
missingState is a different thing: it is not a per-block layer but the fallback StateReader for accounts the store has never seen (WithMissingAccountState, used for genesis-less funding in tests/loadtest), consulted only when snapshot.AccountExists(addr) is false. It never carries block changes, so there is nothing to stack — the overlay sits above it and above the snapshot alike.
Describe your changes and provide context
Re-opens #4258 on top of #4270 (stack 2/4). Depends on #4270's background commit: with the commit landing behind
FinalizeBlock,CheckTx's nonce/balance reads would otherwise settle (wait for the commit) on every admission.Executor.ReadLatestAccount(addr)returns the account as of the last block the executor ran, laying the in-flight block'spendingChangesover a store view without waiting for its commit. ApipelineGenerationcounter detects a commit that started between opening the view and reading, in which case the read restarts (the view may hold a later block, and replaying the older overlay over it would go backwards). It reports the first failed commit rather than state that lacks that block.evmonlyapp.EvmNonce/EvmBalanceuse it instead ofopenSettledView, so admission no longer settles the pipeline.mempool/tx.go: the first-seen account lookup (accountReader.Account) moves out of the inner mempool mutex — lookup, lock, recheck that nobody inserted the account meanwhile — so the store read no longer serializes allCheckTxcallers.Testing performed to validate your change
scripts/ramtest.sh -race ./giga/evmonly/...(overlay/pipelined-read tests: sees in-flight block, reports failed commit, restarts when a block lands or retires underneath).go test -race ./sei-tendermint/internal/evmonlyapp/... ./sei-tendermint/internal/mempool/....make fmtcheck,make lint.Link to Devin session: https://app.devin.ai/sessions/ff612badcded4aa5914ea408dbb41888
Open in Devin Desktop: https://app.devin.ai/desktop/session/ff612badcded4aa5914ea408dbb41888?variant=devin
Requested by: @bdchatham