evmonly/evmonlyapp: revert the execution optimizations (#4254, #4258, #4260, #4261) ahead of a reviewable stack - #4269
Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
PR SummaryHigh Risk Overview Block-STM / OCC ( Commit pipeline: Pending block state is a cloned EVM-only app & node: Mempool: Drops lock-free prefetch of first-seen EVM accounts on insert (and related epoch invalidation on Giga router / proxy: Block loop is fetch → Tests and docs are trimmed to match the serial behavior; load-test bench drops the ERC20 scenario in favor of transfer-only workloads. Reviewed by Cursor Bugbot for commit 8789d48. 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 @@
## giga-1 #4269 +/- ##
===========================================
+ Coverage 65.55% 83.01% +17.45%
===========================================
Files 2081 65 -2016
Lines 157460 8725 -148735
===========================================
- Hits 103222 7243 -95979
+ Misses 54097 1480 -52617
+ Partials 141 2 -139
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.
A faithful, self-consistent revert of #4254/#4258/#4260/#4261: no dangling references to the removed shard/pipeline/prepare machinery remain, and the synchronous Executor.ExecuteBlock (which still calls AwaitCommits) is what makes the app-level settle removals safe. Two non-blocking notes: the PR description overstates what is retained, and one standalone router test assertion was dropped that did not need to be.
Findings: 0 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The description says "#4267's
evmonly_preparetimer [is] unchanged in the serial loop", but it is removed outright: #4267 only re-added that timer aroundevmOnlyApplication.PrepareBlock, which this PR deletes with #4260. There is no synchronous preparation step left in the app to wrap (executor-internal preparation is insideExecuteBlock), so nothing is preservable here — but the claim should be corrected, and operators should knowevmonly_prepare_phase_duration_seconds_totalgoes dark until #4260 is re-landed. - 1 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]
evmOnlyApplication.EvmCall(sei-tendermint/internal/evmonlyapp/app.go) releases the cursor lock beforeexecutor.Callopens its store view, so a full FinalizeBlock+Commit can land between the block context being captured and the snapshot being opened, mixing height N's NUMBER/TIMESTAMP with N+1's state. Thepending.IsPresent()guard closes the wide window (pending is staged before the state commit starts), but this narrow gap is present on the base branch too — the base's context recheck loop does not eliminate it either.
8789d48 to
c0c4c4e
Compare
giga-1 went from ~72k to ~96k tx/s through four execution changes (#4254, #4258, #4260, #4261) that were merged quickly during the testnet-2 tuning. To get them genuinely reviewed, this PR takes all four back out, and stack #4274 re-opens each one on top of it in dependency order so that the top of the stack is byte-identical to today's giga-1.
The branch is four
git revert -m 1commits and nothing else, newest first: #4261 (parallel OCC validation and merge;occ_shards.gogoes,occ.gois the serial walk again), #4260 (PrepareBlock(n+1)whilenexecutes; the router is back to fetch → execute,PrepareBlockleavesevmOnlyApplicationand the proxy, and the router test'sCheckBlockshelper goes with it), #4258 (Executor.ReadLatestAccountand the pending overlay; the mempool's first-seen lookup is back under the store lock), and #4254 (FinalizeBlockcallsexecutor.ExecuteBlocksynchronously again, sosettler/AwaitCommitsgo). Conflicts were resolved so that only those four change: #4259'sevmonly_finalizetimer stays astake_senders→execute→tx_results(thepreparephase returns with #4254), and #4263's storage-tail phases and #4265'senable_receipt_storewith its test are untouched. One thing is not preservable: #4267'sevmonly_preparetimer wrapsevmOnlyApplication.PrepareBlock, soevmonly_prepare_phase_duration_seconds_totalgoes dark until stack 3/4 re-lands #4260.No config keys or wire formats change. The image at this commit should perform like the pre-#4254 build (~72k tx/s) and is not meant to be rolled out on its own. Validated with
make lint/fmtcheck,scripts/ramtest.sh -race ./giga/evmonly/..., andgo test -raceonsei-tendermint/internal/{evmonlyapp,mempool,p2p,proxy}andsei-tendermint/node.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