revert giga snapshot journal rollback changes#3536
Conversation
|
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 @@
## main #3536 +/- ##
==========================================
- Coverage 59.18% 58.29% -0.89%
==========================================
Files 2225 2151 -74
Lines 183625 174955 -8670
==========================================
- Hits 108677 101998 -6679
+ Misses 65154 63871 -1283
+ Partials 9794 9086 -708
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 Snapshot / revert: Removed: Per-mutation journal types and revert paths for storage, code, nonce, balances, Finalize / reads: Tracing: Giga and main Reviewed by Cursor Bugbot for commit 15c64f6. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit df9ff92. Configure here.
| } | ||
| s.journal = s.journal[:snapshot.journalIndex] | ||
| s.ctx = s.snapshottedCtxs[rev] | ||
| s.snapshottedCtxs = s.snapshottedCtxs[:rev] |
There was a problem hiding this comment.
Empty base after revert zero
High Severity
RevertToSnapshot(0) sets snapshottedCtxs to an empty slice while GetCommittedState and flushEvents still read snapshottedCtxs[0]. After a revert to the initial revision, those paths can panic even though s.ctx already holds the base context that replaced the removed committedCtx field.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit df9ff92. Configure here.
df9ff92 to
747638c
Compare
There was a problem hiding this comment.
This is a large, app-hash-breaking revert of journal-based snapshot/rollback logic in the EVM StateDB — worth a human look given the scope and consensus implications, even though no bugs were found by the bug hunter.
Extended reasoning...
Overview
This PR reverts two prior PRs (#3414 and #3295) that introduced journal-based Giga StateDB snapshot/rollback, returning the implementation to the CacheMultiStore stack model. It also touches sei-cosmos/x/auth/keeper/keeper.go (removes GetGlobalAccountNumberBytes/SetGlobalAccountNumberBytes), modifies the expected_keepers interface (removes SetAddressMapping), deletes large snapshot integration test suites, and simplifies docker/localnode/Dockerfile (ubuntu:24.04 → ubuntu:latest, retry logic loosened).
Security risks
The PR is labeled app-hash-breaking, meaning it changes consensus behavior. The semantics of snapshot/revert during EVM execution directly affect state root computation; any divergence from the prior implementation could fork the network. The Dockerfile change to ubuntu:latest reduces reproducibility but is dev-environment scoped.
Level of scrutiny
High. This touches consensus-critical EVM state handling and is explicitly app-hash-breaking. The Cursor bot raised two concerns (panic risk on RevertToSnapshot(0) reading snapshottedCtxs[0], and a nonce hook guard checking V1 while calling V2) which the author acknowledged in the PR description as intentionally matching x/evm/state. A human should validate that intent and confirm the rebased Giga semantics are correct.
Other factors
Coverage dropped on the touched files (patch coverage 55.55%, statedb.go at 40%, nonce.go at 0%). Significant test suites were deleted (snapshot_test.go, statedb_internal_test.go) as they tested the journal model being removed. The author validated with go test ./giga/deps/xevm/... but the loss of integration coverage on the new (restored) snapshot model is worth weighing.
6317aa4 to
df6bfea
Compare


Summary
09d0d60e813f9e619c68d769b7a57bf0d1bfe4fc)c7e729a6f658a4a1ed452057f0cec0c3bfe870d3)Why
The desired fix is to undo the journal-based Giga snapshot/rollback changes and return to the pre-#3295 / pre-#3414 behavior. This restores Giga StateDB snapshot handling to the CacheMultiStore stack model instead of the journal rollback approach introduced and adjusted by those PRs.
Cursor review notes
RevertToSnapshot(0)/snapshottedCtxs[0]: left unchanged because the rebased Giga implementation matchesx/evm/state.x/evm/state(OnNonceChangeguard withOnNonceChangeV2callback).Validation
origin/mainatb8473e713go test ./giga/deps/xevm/state -count=1go test ./giga/deps/xevm/... -count=1go test ./sei-cosmos/x/auth/keeper -count=1