evmonlyapp: land a block's state commit behind FinalizeBlock - #4366
Conversation
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
PR SummaryHigh Risk Overview Committed-state RPC paths (
Tests cover pipelined reads, deferred commit failure, concurrent reads vs finalize/commit, and test teardown that settles before closing storage. OTel adds a Reviewed by Cursor Bugbot for commit fa79805. Bugbot is set up for automated code reviews on this repo. Configure here. |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR changes FinalizeBlock to run PrepareBlock and then ExecutePreparedBlock, so the block's state commit can land in the background. Readers of committed state wait for that commit before reading: through the atomically published executor in openSettledView, and under the state lock in currentExecutionContext. Shutdown also waits for the commit before closing Giga storage. I found no issues: the executor's AwaitCommits is safe to call from several goroutines, closeGigaStorage runs only after giga.Run has returned, and the new tests cover pipelined reads, a failed commit surfacing from the next block, and reads racing FinalizeBlock.
Findings: 0 blocking | 0 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4366 +/- ##
==========================================
- Coverage 67.70% 66.54% -1.16%
==========================================
Files 2168 2045 -123
Lines 168283 156003 -12280
==========================================
- Hits 113928 103818 -10110
+ Misses 54346 52176 -2170
Partials 9 9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
Reviewed by Cursor Bugbot for commit fa79805. Configure here.
| } | ||
| // The executor's own timer breaks execution down further. | ||
| a.finalizePhases.SetPhase(finalizePhaseExecute) | ||
| return executor.ExecutePreparedBlock(ctx, prepared) |
There was a problem hiding this comment.
Stale execution after settled commit failure
Medium Severity
FinalizeBlock no longer waits for its own store write, so a failed commit is first observed on the next block. openSettledView can settle that failure first and clear the executor overlay. The next executeBlockPipelined then runs against the last version that landed, and the executor can persist receipts for that execution before returning the latched error.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fa79805. Configure here.
… acceptance barrier (sei-protocol#4457) Backport: this ports sei-protocol#4273, the reviewable version of sei-protocol#4261, to main from the giga-1 PR stack sei-protocol#4270–sei-protocol#4273. The stack targeted stacked giga-1 branches and never reached main; only its first PR landed on main, as sei-protocol#4366. @shemnon approved sei-protocol#4273 at 1143167. Since that commit, the changes are main's renames (`baseAccount`, the `phaseOCC*` constants, `takeAccessSets`) and two review fixes: merge fragments are cleared before they return to the pool, and each parallel pass's look-ahead is bounded. On giga-testnet-2, `occ_validate` and `occ_merge` took about a third of the executor main loop, all of it on the calling goroutine. Most of that time went to transactions that the frontier accepts as they stand. A block of about 1,800 transactions holds less than one conflict. On `giga-1`, sei-protocol#4261 moved this work onto the OCC pool. It cut validate and merge from about 8.5 ms to 4.1 ms per block. That gave about 20% more blocks per second during the 95k tx/s window. sei-protocol#4269 reverted it for the reviewable stack, and that stack (sei-protocol#4273) never reached main. This PR ports sei-protocol#4261 and the sei-protocol#4273 follow-up to main (PLT-1377). Acceptance stays a serial barrier in block order. `stateAccessIndex` and `blockSTMState` split into 64 address shards. `indexResults` records the writes of every speculative result up front, and `conflictsWithin(key, sourcePrefix, txIndex)` replaces `conflictsWithAfter`. `acceptValidatedPrefix` finds the first result that the serial frontier would not accept, and `applyRange` folds the run before it into the prefix shard by shard. `validateBlockSTMFrontier` then handles only that one result, and `serialBackoff` keeps dependency chains on the calling goroutine. Each pass looks ahead at most twice as far as the previous pass accepted, and at least 2,048 results, so the work of a block's passes stays linear in its size. `changeSetIntoParallel` builds the changeset of each shard on the pool and joins the shards in address order. Its per-shard base-row cache replaces `prefetchBaseAccounts`. The port needs nothing from sei-protocol#4258 or sei-protocol#4260; the only conflicts came from the `baseAccount` and phase-constant renames on main. Non-app-hash-breaking: changesets, receipts, and tx results stay byte-identical to main. Shards are contiguous ranges of the first address byte, so shard order is canonical address order. Each address lives in one shard, so the apply order in a shard is block order. The new index can only add conflicts, from stale writes of an earlier incarnation. An extra rerun executes against the exact accepted prefix, so it gives the sequential result, and the extra reruns show only in OCC metrics. Review `stmFrontierAccepts` most closely, because it must mirror `needsSTMRerun`, and `touchedShards`, because it must cover every address that `applyOwned` and `indexResults` touch. `TestOCCRandomizedConflictingBlocksMatchSequential` checks seeded dense and sparse blocks at 3 and 8 workers against the sequential executor, down to the encoded FlatKV pairs. A seeded digest run gave identical output on main and this branch for 864 blocks, including 3,000- and 6,000-transaction blocks that cross a pass's look-ahead. --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
3 participants
The EVM-only executor on main can already pipeline commits (
PrepareBlock,ExecutePreparedBlock,AwaitCommits, from #4317), butevmOnlyApplication.FinalizeBlockstill calls the synchronousExecuteBlock. That call waits for the FlatKV commit of block N before it returns, so the node can't start N+1 until the write lands. This PR brings the application-layer half of #4270 over fromgiga-1(PLT-1312), adapted to main'sevmOnlyState/ sender-cache layout instead of the older cursor/executor split that #4270 assumes.FinalizeBlocknow runsPrepareBlockand thenExecutePreparedBlock(inexecuteBlockPipelined), and returns once the state commit has started. The next block still waits on the previous commit inside the executor before it starts its own. Committed-state readers settle the in-flight commit first.EvmNonce,EvmBalanceandEvmCodego throughopenSettledView, which callsAwaitCommitson the executor published through a lock-freesettler, so they never block onFinalizeBlock.currentExecutionContext, used byEvmCallandEvmEstimateGas, awaits while it holdsstate, so no new block can start between the settle and the snapshot, and it returns a failed commit as an error.openSettledViewlogs a failed commit once and serves the last version that landed. Its return values have no error channel, and the failure halts the node through the nextFinalizeBlock.Proxy.AwaitCommitsexposes the settle, andnodeImpl.closeGigaStoragecalls it beforemanager.Close()so shutdown never closes the store under an in-flight write.Consensus and app hash are unchanged: the app hash is still computed from the execution result, not the store write. The behaviour change is that a failed store commit now surfaces one block later, from the next
FinalizeBlock, rather than from its own. BetweenFinalizeBlockandCommit,EvmNonceand friends already see the finalized block's state; before this PR they could not, because the write had already landed by then anyway. New tests cover reads settling across four pipelined blocks and a commit failure surfacing from the next block, fromEvmCalland fromAwaitCommits, and a-racetest that drivesEvmNonce/EvmCallfrom a goroutine while blocks are finalized and committed, asserting the nonce never goes backwards. The test helpers now settle before closing storage. I validated withscripts/ramtest.sh -raceoverevmonlyapp,proxy,nodeandgiga/evmonly, plusmake fmtcheckandmake lint.Link to Devin session: https://app.devin.ai/sessions/d808db90106341278344623de10b92c0
Open in Devin Desktop: https://app.devin.ai/desktop/session/d808db90106341278344623de10b92c0?variant=devin
Requested by: @bdchatham