Rollback tooling for LittDB / recieptDB - #4080
Conversation
PR SummaryHigh Risk Overview Disk table iterators no longer depend on a live The former
Reviewed by Cursor Bugbot for commit 483e238. 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).
|
![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 483e238. Configure here.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4080 +/- ##
==========================================
- Coverage 61.23% 60.17% -1.06%
==========================================
Files 2177 2072 -105
Lines 190632 178426 -12206
==========================================
- Hits 116729 107368 -9361
+ Misses 62892 61041 -1851
+ Partials 11011 10017 -994
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The offline iterator extraction and the litt package rename are clean and well tested, but the new receipt.PruneAfter/GetRange entry points can silently report success while leaving the store inconsistent: they ignore cfg.Backend (which defaults to pebbledb), and they rewrite the store's head metadata even when the litt rollback found no pivot and changed nothing.
Findings: 2 blocking | 5 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion]
GetRangereports the lowest block physically present in litt, which can be below the store's retention floor (m:earliest): after pruning, bodies survive until litt's TTL/GC reclaims them, so an operator usingGetRangeto choose a rollback target can be shown a lowest block the store will not serve and thatPruneAfterwill then refuse. Consider readingm:earliestand reporting the served floor (or documenting the distinction onGetRange). - [suggestion] Test coverage gaps for the two riskiest
PruneAfterpaths: a target below the lowest block present in litt (the no-pivot case), and a config whose backend is notlittidx. Both currently return success having done nothing meaningful. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| // checked before any mutation, so a refusal leaves the store untouched. Otherwise it rolls back the | ||
| // litt-backed receipt bodies, then deletes the pebble tag-index entries above highestBlockToKeep and moves | ||
| // the store's latest-block metadata back to match. | ||
| func PruneAfter(cfg dbconfig.ReceiptStoreConfig, highestBlockToKeep uint64) error { |
There was a problem hiding this comment.
[blocker] GetRange and PruneAfter both assume the littidx on-disk layout (<DBDirectory>/littdb, <DBDirectory>/log-index) but never consult cfg.Backend, which defaults to pebbledb (dbconfig.DefaultReceiptStoreConfig). For a pebbledb-backed store the receipts live directly under DBDirectory (mvcc.OpenDB(ssConfig.DBDirectory, ...) in receipt_store.go), so:
GetRangereturnsok=false("no receipts") for a store full of receipts, andPruneAfteropens/creates an emptylog-indexpebble DB, readslatest == 0, and returnsnil— reporting success while pruning nothing.
Both also create the missing littdb/log-index subdirectories inside the live store's directory as a side effect, and because neither touches the pebbledb backend's own lock, this can happen while the node is running (the littidx path is protected only incidentally, by pebble's lock on log-index).
Guard at the entry point, e.g. reject normalizeReceiptBackend(cfg.Backend) != receiptBackendLittIdx before doing anything.
| } | ||
| return height <= highestBlockToKeep, nil | ||
| } | ||
| if err := offline.RollbackLittDB(littConfig, filter); err != nil { |
There was a problem hiding this comment.
[blocker] When the filter never returns true, RollbackLittDB logs "no rollback point found, leaving table unchanged" and returns nil (offline/rollback.go:137-140), but PruneAfter carries on to delete the tag-index entries above highestBlockToKeep and move m:latest down.
That happens whenever highestBlockToKeep is below the lowest block present in litt — e.g. PruneAfter(cfg, 0), or a store whose receipts start at a state-sync height being rolled back below it. The refusal above doesn't catch it: m:earliest is 0 on a never-pruned store.
Result: the store reports head highestBlockToKeep, FilterLogs sees nothing above it, but every receipt body above it is still in litt and still served by GetReceiptFromStore / eth_getTransactionReceipt, because the read-time floor keys off m:earliest, which is untouched.
RollbackLittDB should report whether it found a pivot (per table) so PruneAfter can fail loudly — or discard the table outright — instead of rewriting metadata that the data no longer matches.
| // Returns 0 if the key is absent, malformed, or unreadable, matching littReceiptStore.readMeta's behavior. | ||
| func readMetaOffline(index dbtypes.KeyValueDB, key []byte) (uint64, error) { | ||
| val, err := index.Get(key) | ||
| if err != nil || len(val) != blockNumLen { |
There was a problem hiding this comment.
[suggestion] readMetaOffline collapses every failure into (0, nil), though pebble distinguishes a missing key (errorutils.ErrNotFound) from a real read error (pebbledb/db.go:105-116). On this path that turns two distinct corruptions into silent wrong behaviour:
m:earliestunreadable or malformed ⇒earliest == 0⇒ the retention-floor refusal this function's own godoc promises ("checked before any mutation") is skipped and the rollback proceeds;m:latestunreadable ⇒latest == 0⇒PruneAfterreturns nil having done nothing, reporting success.
The function already returns an error it never populates — map only ErrNotFound to 0 and propagate the rest. (littReceiptStore.readMeta's laxity is fine by comparison: it defaults an in-memory hint rather than gating a destructive operation, so the "matching readMeta's behavior" rationale in the comment doesn't carry over.)
| if err != nil { | ||
| return fmt.Errorf("failed to open receipt log index: %w", err) | ||
| } | ||
| defer func() { _ = index.Close() }() |
There was a problem hiding this comment.
[suggestion] On this mutating path the index Close error is discarded, so a failure to flush the range-delete tombstone and the new m:latest still lets PruneAfter return nil. Capture it into a named return (err = errors.Join(err, index.Close())) so a failed close surfaces as a failed prune.
|
|
||
| lowestSegmentIndex, highestSegmentIndex, segments, err := segment.GatherSegmentFiles( | ||
| logger, errorMonitor, segmentPaths, false /* snapshottingEnabled */, time.Now(), | ||
| true /* cleanOrphans */, fsync) |
There was a problem hiding this comment.
[suggestion] cleanOrphans: true makes the read-only iterator delete orphaned segment files (and GatherSegmentFiles unconditionally removes garbage files), while NewIterator also creates the data directories at line 34. So GetRange, documented as reporting heights "without opening the store", mutates the directory it inspects and destroys exactly the leftovers an operator would want for a post-mortem. Passing false here loads the same segments without the cleanup — the rollback path is the one that legitimately wants true.
2 participants
Describe your changes and provide context
Tooling that allows the caller to get the blocks stored in the DB and to prune the DB before first starting it.