Repository navigation
Revert persisting the EVM-only execution cursor with FlatKV state - #4212
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
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 #4212 +/- ##
==========================================
+ Coverage 65.55% 65.56% +0.01%
==========================================
Files 2081 2078 -3
Lines 157460 157297 -163
==========================================
- Hits 103222 103137 -85
+ Misses 54097 54019 -78
Partials 141 141
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryHigh Risk Overview The Autobahn EVM-only app keeps height, app hash, parent hash, and pending finalize/commit state in a single in-memory Docs drop guidance that apps must derive Reviewed by Cursor Bugbot for commit c693c12. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
A clean, complete revert of #4190: no dangling references to the removed cursor remain, and the three commits that landed on top (checkedSenders, EvmChainConfig, EvmBaseFee, EvmCall) are faithfully re-applied onto the pre-#4190 struct. The only finding is the restart regression the description itself acknowledges (#4169 reopened), which is deliberate but leaves no test recording the restored behaviour.
Findings: 0 blocking | 2 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The three restart tests and
cursor_test.goare removed along with the feature, which is correct, but nothing now pins the restored behaviour.reopenEVMOnlyTestAppsurvives and is only used for its storage side effect (_, storage = reopenEVMOnlyTestApp(...)at app_test.go:135, so its*evmOnlyApplicationreturn and therequire.True(t, ok)assertion are dead). Reusing it for a short characterization test — reopen after committing a block, assertInfo().LastBlockHeight == 0— would record the reopened #4169 behaviour in the tree, so re-landing cursor persistence flips a visible assertion rather than adding one from scratch. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| storage *bootstrap.GigaStorageManager, | ||
| changeSetEncoder evmonly.NamedChangeSetEncoder, | ||
| ) (abci.Application, error) { | ||
| ) abci.Application { |
There was a problem hiding this comment.
[suggestion] Dropping the constructor's error return also drops the only path that reloaded a durable position, so a restarted node starts at committedHeight == 0 with a state store that still holds every prior block. The consequence is worse than Info() reporting 0: gigaRouterCommon.runExecute branches on last == 0 and calls InitChain, seedInitialStateVersion returns early for InitialHeight == 1 without inspecting the store, and block 1 is then re-executed against state whose nonces and balances have already advanced — the executor refuses it (or CommitStateChanges(1, ...) fails against a store at a higher version), and runExecute returns, so the node does not come up at all.
This is the #4169 regression the description acknowledges reopening, and the scope is the Autobahn load-test application rather than production (TODO(evmonly-production) still gates real traffic), so I read it as an accepted cost of the revert rather than something to fix here. Worth confirming that no load-test environment currently relies on restarting a node over existing EVM-only storage, since those will now fail to start rather than degrade.
…cursor' into masih/1789584107-revert-evmonly-cursor
…vert-evmonly-cursor
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 e5c63d0. Configure here.
…vert-evmonly-cursor # Conflicts: # giga/evmonly/executor.go # giga/evmonly/giga_store.go

This reverts #4190, which persisted the EVM-only execution cursor (height, app hash, parent hash, gas limit) as an
evmonly/cursorchangeset alongside each block's FlatKV state and taughtNewEVMOnlyApplicationto reload it soInfo()reported the durable height after a restart. The app returns to keeping its position in the in-memoryevmOnlyState, advanced byCommit, and theBlockChangeSetEncoderhook on the executor is removed. Consequently a restarted EVM-only node again reports height 0 fromInfo(), so #4169 is reopened by this change.Three commits landed on top of #4190 in
evmonlyapp(#4183, #4194, #4205) and conflicted with the automatic revert. Their behaviour is preserved:checkedSenders,EvmChainConfig,EvmBaseFeeandEvmCallare re-applied onto the pre-#4190 struct, withEvmCallreadingcommittedHeight,gasLimit,parentHashand the newlastBlockTimefield instead of the removed cursor. The chained SHA-256 app hash is unchanged, and the existingevmonlyapp,proxy,giga/evmonly,nodeandp2ptests pass.