Repository navigation
Integrate undo log - #4498
Integrate undo log#4498
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 #4498 +/- ##
==========================================
+ Coverage 57.30% 57.39% +0.08%
==========================================
Files 2144 2145 +1
Lines 167447 167582 +135
==========================================
+ Hits 95950 96178 +228
+ Misses 71492 71399 -93
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
227dd5a to
2214180
Compare
PR SummaryHigh Risk Overview Undo-log integration: When the undo backend is selected, Execution path: The EVM-only executor clones prior balance/code in pipeline overlays and, for undo-capable stores, passes the block Reviewed by Cursor Bugbot for commit 0d28886. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR wires the undo-log history store into the giga StateDB. Execution now hands each block's prior values to it, and OpenViewAt serves past heights from either the undo log or the pebbledb state store. The blocker is that the undo log stores non-empty bytecode made only of zero bytes as absent code. Of the three codex findings I verified and kept all three: the zero-byte code bug (blocker), nil priors through CommitStateChanges (suggestion), and the unpinned pebbledb view (suggestion); no reading was dropped.
Blocking
1 finding on the changed lines, as inline comments.
Non-blocking
3 findings on the changed lines, as inline comments.
seidroid review · decision request_changes · session 0040bb59d27c4a1393c5004e6b92d309 · turn resp_claude_c58bdc026bcd54df681568a3f3452f17 · item 127bd4f807c056f7b3ed28e04ae4fca1
Findings: 1 blocking | 3 non-blocking | 4 posted inline
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| for _, change := range changes.Storage { | ||
| w.add(StateKeyPrefix[0], change.Address[:], change.Key[:], nonZero(change.PriorValue[:])) | ||
| } | ||
| return w.finish() |
There was a problem hiding this comment.
Undo log drops storage clears
High Severity
writeRecords never records StorageClears. It only writes per-slot Storage entries, on the assumption that a clear only covers storage created in the same transaction. SelfDestruct also clears pre-existing slots that never appear in Storage, including slots that were only read. After that block, historical reads of those slots fall through to the cleared head value, so earlier heights look empty.
Reviewed by Cursor Bugbot for commit 2214180. Configure here.
|
@seidroid you failed, review again |
|
Claude finished @alrevuelta's task in 1m 53s —— View job Re-review at
|
| Finding | Status |
|---|---|
| Blocker: zero-byte bytecode was recorded as absent | ✅ Fixed. writeRecords now treats code as absent only when len(PriorCode) == 0, the same rule the code hash uses. TestZeroByteCodeIsPresent covers it. |
CommitStateChanges with no priors silently advanced the undo log |
✅ Fixed. CommitStateChangesWithPrior now rejects a non-empty changeset with changes == nil when s.undo != nil (state_db.go), and TestUndoStateDBRefusesABlockWithoutItsChangeSet covers it. Side effect: gigasim, the loadtest genesis commit, and any other caller of the plain gigatypes.StateDB method now fail loudly under undolog instead of corrupting history. That's the right failure mode. |
Pebbledb OpenViewAt reported pruned keys as absent |
✅ Fixed. Each read checks GetEarliestVersion() again after the Get. I checked the ordering: mvcc.pruneAscending → beginPrunePass → SetEarliestVersion raises the marker before anything is deleted, so a read that races a prune either sees the data or fails. It can't come back empty. TestSSViewFailsReadsOnceItsHeightIsPruned covers it. |
| A storage clear assumes the cleared storage was already empty |
Non-blocking
1. A storage clear records no priors (sei-db/db_engine/pebbledb/undo/db.go, writeRecords). Cursor flagged the same thing.
The doc comment says a clear "only clears accounts created in the same transaction, whose storage was empty". Nothing enforces that:
nativeStateDB.Finalise(giga/evmonly/state_db.go:648-657) clears storage for any account whereSelfDestructedis set. LegacySelfDestruct(state_db.go:496) sets it with noCreatedcheck. OnlySelfDestruct6780restricts it. So any block that runs under pre-Cancun rules can wipe storage that already existed.- Even under 6780, an account created at an address that already has storage would wipe that storage.
GetStorageRootalways returns zero (state_db.go:465), so geth's EIP-7610 collision check never fires. encodeFlatKVChangeSetturns each clear into deletes of every slot SC holds (appendFlatKVStorageClearPairs). MeanwhilechangeSetIntoemitsStorageChangeonly for slots with a non-zero new value. The deleted slots never reach the undo log, so history below that block silently reads them as empty.
On a chain that is Cancun from genesis this is narrow, but it fails silently. Cheapest fix: the encoder already iterates the cleared slots, so either emit a StorageChange{PriorValue: …} for each one, or return an error when a clear finds stored slots, so the assumption becomes an enforced invariant.
2. Any unclean shutdown drops the whole history window (state_db.go resumeUndoLog → undo.resume).
ApplyBlock is queued asynchronously and records are written pebble.NoSync, but SC catches up to the WAL head on restart. After a crash the log is almost always behind SC, and then resume calls raiseEarliest(height): the node keeps running with no historical state, and the only signal is a Warn log line. The resumeUndoLog comment acknowledges this. Even so, it's a significant operational difference from the pebbledb backend that deserves at least one of these:
- a line in the
state_store_backendconfig template, - a metric or a louder log,
- or
Syncon the latest-height marker every N blocks, to limit how much is lost.
Minor
historicalView.Getpanics for non-EVM modules and on read errors. That's consistent with the "views have no recoverable errors" contract, but RPC callers ofOpenViewAtneed to know that a pruned pebbledb height now panics mid-request rather than returning an error.gigatypes.StateChangeSetduplicates the evmonly types, andgigaChangeSetcopies every block. This is already marked TODO; fine for now.
Not run
I didn't run the tests in this job. Codecov reports patch coverage of 82%; the gaps are mostly in giga_store.go (commitState/gigaChangeSet, 29%).
|
@seidroid im aware of this "A storage clear records no priors". The undo log assumes that self destruct behaviour is post cancun fork. review and approve if you are ok. |
|
Claude finished @alrevuelta's task in 41s —— View job Storage-clear assumption: I'm OK with thisI can't submit a formal GitHub approval, because the app doesn't have that permission (FAQ). A maintainer will need to click approve. On the merits, I have no remaining objections, and I'm withdrawing my earlier change request. The "storage clear records no priors" note is accepted as a documented design constraint. Why the post-Cancun assumption holds in production
Remaining edge (non-blocking, practically unreachable)
Optional hardening (not required for merge)Today the assumption lives in a doc comment on
Either fits a follow-up PR. Status of the other findings at
|
There was a problem hiding this comment.
This PR wires the undo-log EVM state store into giga as an alternative SS backend: it adds a config switch, feeds each block's change set through CommitStateChangesWithPrior, and adds historical OpenViewAt views over either backend. Two of my four earlier findings are fixed and the other two are now non-blocking, so this approves; I kept codex's pruning-panic point as a suggestion, and the tree at the PR merge commit was available but no Go toolchain was, so tests were not run.
Non-blocking
2 findings on the changed lines, as inline comments.
- With the undo backend, any unclean exit while blocks are still queued in the undo writer leaves the log behind SC, because SC commits synchronously before
ApplyBlockenqueues.resumeUndoLogthen callsResume, which raises the earliest height to SC's height and discards all retained history. That includes the writer's ownpanic(err)path. The comment says this is deliberate, but it turns a crash or a single write failure into losing the whole history. Consider bounding that loss, for example by having the commit wait on the undo writer, or at least alerting on the dropped range beyond the warn-level log.
seidroid review · decision approve · session 36ae51512c614f9fb25d9571f8159d68 · turn resp_claude_a38e6684aeab06079e7b01e6f3c71cf8 · item 54ca185086505c2692819d2592dfd85d
Findings: 0 blocking | 3 non-blocking | 2 posted inline
| } | ||
|
|
||
| // writeRecords writes a record of the value before the block at height of every key changes writes, | ||
| // absent for a key that held a zero value or none. A storage clear writes none: it only clears accounts |
There was a problem hiding this comment.
suggestion — This comment still assumes a storage clear only empties storage that was already empty, and nothing enforces it. encodeFlatKVChangeSet turns each clear into deletes of every slot SC currently holds, while the change set carries priors only for slots the block then writes non-zero. If a clear ever drops an existing slot, history below that block silently loses it. Either record a prior for each cleared slot SC held, or fail the block when a clear finds slots.
| } | ||
| return value, err | ||
| } | ||
| return &historicalView{height: blockNum, read: read, release: func() {}}, true |
There was a problem hiding this comment.
suggestion — (from codex) The pebbledb view now detects pruning correctly, but it still pins nothing and its release does nothing. A normal prune cycle can therefore invalidate a view that opened successfully, and the next read errors, which historicalView.Get turns into a panic (TestSSViewFailsReadsOnceItsHeightIsPruned asserts this). The undo branch keeps its view valid until Close, and the OpenViewAt contract implies the same. Hold the retained range until Close here too, before an RPC caller exists to crash on it.
Superseded: the latest review found nothing blocking in this change.
cody-littley
left a comment
cody-littley
left a comment
There was a problem hiding this comment.
LGTM, but addressing the view pin vs. pruning race might be nice (i.e. the comment from the LLM). Also ok if you think this should be handled as a follow up PR.
|
|
||
| import "math/big" | ||
|
|
||
| // TODO: Temporal copy of giga/evmonly/types.go |
There was a problem hiding this comment.
nit, "temporary" is probably a better term. I see "temporal" used in a few places other than here as well. I understand what you mean by this, but temporal is usually interpreted as meaning "related to time", not "for a limited amount of time".
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0d28886. Configure here.
| if s.undo != nil && changes == nil && len(changeset) != 0 { | ||
| // The undo log would advance past the block with no record of what it replaced. | ||
| return fmt.Errorf("commit block %d: the undo-log EVM state store needs the block's change set", blockNum) | ||
| } |
There was a problem hiding this comment.
Empty EVM blocks fail undo commit
High Severity
The undo-log commit guard treats any non-empty changeset as requiring a non-nil EVM changes pointer. The executor clones an empty EVM change set to nil while still appending extra non-EVM changesets (the production cursor/app-hash encoder runs on every block), so blocks that did not touch EVM state fail CommitStateChangesWithPrior instead of recording nothing and advancing.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 0d28886. Configure here.
| LookbackWindow int64 `mapstructure:"lookback_window"` | ||
| // StateStoreBackend is how the state store keeps its history: "pebbledb" stores every version of | ||
| // every key, "undolog" the value each block replaced, over the current state. | ||
| StateStoreBackend string `mapstructure:"state_store_backend"` |
There was a problem hiding this comment.
Do we plan to support multiple backend in giga world? If undo log is the only SS we want to support moving forwards, I think we don't need to add this config at all
There was a problem hiding this comment.
i plan to remove the "old ss" at some point, and just support undo log for giga.
|
StateDB should only contains SC, SS and StateWAL, undo log ss should be folded into ss package as one of the backend, there should be no giga. We can do this as a separate refactor though |
| if err := s.catchUpToWAL(ctx); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := s.resumeUndoLog(); err != nil { |
There was a problem hiding this comment.
This might be an issue: fresh undolog node deadlocks in InitChain
resumeUndoLogpins an SC view at version 0 here.seedInitialStateVersion(evmonlyapp/app.go:289) then callsSC().SetInitialVersion, which hangs influshLatestVersion(flatkv/store_meta.go:420) while that view is held. Reproduced with initial height 1 and 5; pebbledb backend returns fine.- Even without the hang, initial height > 1 leaves the undo log at 0 while SC is at H-1, so the first block fails
does not follow the latest applied blockand the writer panics. - Suggest a
StateDB.SetInitialVersionthat releases the undo head, seeds SC, then re-Resumes, and route InitChain through it.
| return fmt.Errorf("commit block %d: block number must not be negative", blockNum) | ||
| } | ||
|
|
||
| if s.undo != nil && changes == nil && len(changeset) != 0 { |
There was a problem hiding this comment.
Another issue: any block with no EVM changes halts the pipeline
- Empty EVM change set ->
clone()returns nil ->gigaChangeSet(nil)is nil. - The cursor changeset is still appended every block, so
len(changeset) != 0and this guard errors. - The error becomes
pipelineFailure, so every later block fails too. Reproduceble locally. - Suggest checking for a non-empty EVM-keyed changeset instead of
len(changeset), or passing an empty&gigatypes.StateChangeSet{}instead of nil.
| } | ||
| // Before SC: the undo log holds a view of SC's current block until it closes. | ||
| if s.undo != nil { | ||
| if err := timer.Close("ss", s.undo.Close); err != nil { |
There was a problem hiding this comment.
Shutdown with a historical view still open
- If a caller still holds a view from
OpenViewAt,undo.Closerefuses and returns an error, so the undo log stays open. - We then close SC anyway, while the undo log is still holding an SC view.
- No production caller of
OpenViewAtyet, but once RPC uses it, an in-flight query during shutdown hits this. - Suggest waiting for (or force-releasing) open views before closing, or skipping SC close when the undo log failed to close.
|
@yzang2019 @cody-littley will address your comments in followup PRs. tracked in linear. |
4 participants
Uh oh!
There was an error while loading. Please reload this page.