Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
PR SummaryHigh Risk Overview A Tests add Reviewed by Cursor Bugbot for commit 4fefcc7. 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).
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit aec5c3b. Configure here.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## giga-1 #4270 +/- ##
===========================================
+ Coverage 65.55% 75.42% +9.86%
===========================================
Files 2081 10 -2071
Lines 157460 1778 -155682
===========================================
- Hits 103222 1341 -101881
+ Misses 54097 436 -53661
+ Partials 141 1 -140
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.
Moving the block state commit behind FinalizeBlock is implemented carefully: the settler covers every committed-state reader, changeset encoding and the pipeline overlay are alias-free, lock order is preserved, and shutdown settles before closing the stores. Two non-blocking notes concern the new EvmCall settle-and-recheck loop and the swallowed commit failure in openSettledView.
Findings: 0 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The new coverage is valuable but entirely sequential.
TestEVMOnlyApplicationReadsSettleBehindFinalizeBlockfinalizes, then reads, on one goroutine, so it never exercises the concurrency the PR introduces — a reader callingAwaitCommits/openSettledViewwhileFinalizeBlockis mid-execution and retiring the previous commit. A-racetest that drivesEvmNonce/EvmCallfrom a goroutine while blocks are finalized would pin the interaction betweenawaitPipelineCommitretiringpipelineChangesandexecutePreparedBlockWithStorereading it before opening its view. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
EvmCall(sei-tendermint/internal/evmonlyapp/app.go) releases the cursor lock beforeexecutor.Callopens its own store view, so a block can commit in between and pair block N's block context with block N+1's state. This predates the PR; the new settle-and-recheck loop narrows the window but does not close it.
| if err := executor.AwaitCommits(); err != nil { | ||
| return nil, err | ||
| } | ||
| settled, err := a.callBlockContext() |
There was a problem hiding this comment.
[suggestion] Two things about the recheck.
First, the comment above claims that after the recheck "the store holds the advertised block and no later one has been committed to the cursor." That only holds at the instant of this call: executor.Call opens its own view (giga/evmonly/call.go:33) after this returns, so a block committed between here and that OpenView is still visible to the call while blockCtx advertises the older NUMBER/TIMESTAMP/BLOCKHASH. The loop genuinely guarantees the store is at least at blockCtx.Number, which is the useful part — worth stating that instead, so a later reader does not take the stronger guarantee as given.
Second, this second callBlockContext() can return the "EVM-only call attempted before committing the finalized block" error for a state that is transient by construction. Concretely: an eth_call arrives with cursor at N and nothing pending, so the first callBlockContext succeeds; AwaitCommits blocks while commit(N) lands; during that wait FinalizeBlock(N+1) returns and stages its cursor; the recheck now sees pending and fails the call. Before this change the pending check ran once at an instant, so the same call would have succeeded. Since this is already a retry loop, a staged-but-uncommitted cursor is arguably a retry condition rather than a failure — though a plain continue would busy-spin here (AwaitCommits returns immediately once settled), so it needs a backoff or a bound rather than just falling through.
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.
| // consistent version, and the failure halts the node through the next | ||
| // FinalizeBlock. | ||
| func (a *evmOnlyApplication) openSettledView() gigatypes.StateView { | ||
| if err := a.AwaitCommits(); err != nil && !a.settleFailureLogged.Swap(true) { |
There was a problem hiding this comment.
[suggestion] Swallowing the settle error means committed-state readers silently serve state from before the failed block. TestEVMOnlyApplicationSurfacesAFailedCommitFromTheNextBlock codifies the reachable state: commit(1) fails, Commit still advances the cursor, so LastBlockHeight() reports 1 while GetLatestVersion() is 0. In that window eth_getBalance/eth_getTransactionCount at "latest" answer from genesis state with no error to the caller and one log line for the process lifetime, while eth_blockNumber reports 1 — and EvmCall on the same app does return the error, so the RPC surface is internally inconsistent. Pre-change a failed commit failed FinalizeBlock, so the cursor could never advance past it.
The halt-on-next-FinalizeBlock argument bounds this for a producing node, but the doc comment's "the failure halts the node through the next FinalizeBlock" is doing a lot of work for a node that is already halting and still serving RPC. Consider surfacing it to the reader (as EvmCall does) rather than only logging.
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.
46d3fc2 to
aec5c3b
Compare
aec5c3b to
4fefcc7
Compare

Describe your changes and provide context
Re-opens #4254 on top of the revert in #4269 so it can be reviewed on its own. Bottom of a four-PR stack (#4254 → #4258 → #4260 → #4261); the top of the stack is byte-identical to today's
giga-1.FinalizeBlockstops waiting for the block's FlatKV commit. It callsexecutor.PrepareBlock+ExecutePreparedBlock(timed as theprepareandexecutephases ofevmonly_finalize) and returns once the commit has been started; the executor lands block N's commit while N+1 executes, reading N's changes through an overlay in the meantime.Anything that must observe committed state settles first through the new
settler(AwaitCommits):EvmCall,Query, RPC views (openSettledView), andnode.closeGigaStoragebefore the stores are closed. A failed background commit is latched by the executor and surfaces on the nextFinalizeBlock(halting the node) and to committed-state readers;abandonPendingtakes the executor so it can settle before dropping the cursor.Measured on giga-testnet-2 as the first step of ~72k → ~96k tx/s.
Testing performed to validate your change
go test -race ./sei-tendermint/internal/evmonlyapp/... ./sei-tendermint/node/...(includes the failed-background-commit surfacing and commit-lands-behind-Finalize tests).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