Repository navigation
node: pass the ancient directory in OpenDatabaseWithFreezer (fixes snapshot prune-block / inspect-ancient-db) - #2501
Conversation
|
Note on the single red check: Coverage was generated fine; the uploader runs tokenless because this PR comes from a fork ( Everything that actually gates the change is green: unit-tests, integration-tests, lint, Build, e2e-tests ×2, pipeline-e2e-tests, build-bor ×3, build-heimdall-v2 ×3, CodeQL, Quality metrics, Socket Security. Happy to re-push if a maintainer prefers a fresh run. |
There was a problem hiding this comment.
🟢 Approval recommended
The focused fix matches existing database-opening behavior, with no unresolved findings.
No open issues 🎉
What changed in this PR
Restores disk-backed ancient database access for snapshot CLI operations by forwarding the resolved ancient directory.
Changes:
- Sets
DatabaseOptions.AncientsDirectoryinOpenDatabaseWithFreezer, matching the existing options-based opening path.
| File | Description |
|---|---|
node/node.go |
Passes the resolved ancient directory when opening the database. |
Review effort: Balanced
|
@claude review |
|
@dmitrybitquery - your commit is missing a verified signature. Can you please update? |
OpenDatabaseWithFreezer takes an `ancient` argument but never forwarded it
to DatabaseOptions, so openDatabase received AncientsDirectory="" and
rawdb.Open created an in-memory chain freezer (newChainFreezer("")).
Every CLI command that opens the database through this helper — snapshot
prune-block, snapshot prune-state, snapshot inspect-ancient-db and
BlockPruner.backupOldDb — therefore saw a freezer with zero items while the
on-disk tables held tens of millions, and failed in rawdb.Open with
"ancient chain segments already extracted, please set --datadir.ancient to
the correct path". The suggested flag had no effect because its value was
dropped right here.
The server path is unaffected: OpenDatabaseWithOptions sets
opt.AncientsDirectory = n.ResolveAncient(name, ...) explicitly. Mirror that.
c6856a2 to
1cdf43c
Compare
|
@pratikspatil024 done — the branch now carries a single GitHub-signed commit (07e1e9d, |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| directory: n.ResolvePath(name), | ||
| dbEngine: n.config.DBEngine, | ||
| DatabaseOptions: DatabaseOptions{ | ||
| AncientsDirectory: n.ResolveAncient(name, ancient), |
There was a problem hiding this comment.
🔴 Operators whose --datadir.ancient is its own mounted volume can now reach a prune-block step that wipes the freezer and leaves the node unable to start. Forwarding ancient at node.go:837 lets prune-block pass validation. AncientDbReplacer (pruner.go:556) then empties the volume before renaming the backup. The backup sits in a sibling ancient_back on the parent filesystem, so the rename cannot succeed. Fix: before deleting anything, check that newAncientPath and oldAncientPath are on the same filesystem and that the old path is not a mount root. Or rename first and delete afterwards.
Why this was flagged
Trigger: an operator runs bor snapshot prune-block --datadir.ancient /ancient, where /ancient is a mounted volume. On the base branch the ancient argument was dropped, so validateAgainstSnapshot (snapshot.go:347) failed with "ancient chain segments already extracted" and nothing was touched. With node.go:837, validation and backupOldDb (pruner.go:358) open the real freezer, so the delete-and-rename pipeline runs. pruneBlock sets newAncientPath = filepath.Join(path, "ancient_back") (snapshot.go:486), which is /ancient_back on the root filesystem. AncientDbReplacer calls os.RemoveAll(p.oldAncientPath) (pruner.go:556): it deletes every file on the volume, then rmdir of the mountpoint fails with EBUSY. The rename at pruner.go:561 never runs, and would fail with EXDEV anyway. On restart, rawdb.Open sees an empty freezer with offset != 0 and fails with "missing blocks from leveldb post ancientdb pruning". checkDeletePermissions (snapshot.go:449) checks only permission bits, not mount or filesystem boundaries.
Verification: With the new line AncientsDirectory: n.ResolveAncient(name, ancient) at node/node.go:837, validation passes and pruneBlock (snapshot.go:473) runs. checkDeletePermissions (snapshot.go:450-468) only checks mode bits. AncientDbReplacer (core/state/pruner/pruner.go:556) runs os.RemoveAll(p.oldAncientPath), which deletes every file on the volume and then fails on rmdir of the mount point with EBUSY.
There was a problem hiding this comment.
Agreed — this hazard is real, and it is pre-existing: it is reachable on every release up to v2.3.3, where OpenDatabaseWithFreezer still honored ancient and prune-block worked. The flow is unchanged by this PR: newAncientPath is built as a sibling of oldAncientPath in its parent dir (snapshot.go:485-490), so if --datadir.ancient is a mount root, AncientDbReplacer (pruner.go:556-561) empties the mount with RemoveAll and the following Rename fails with EXDEV/EBUSY, leaving the operator with a stranded ancient_back.
The right guard belongs in pruneBlock before anything is written, next to checkDeletePermissions: refuse when oldAncientPath is a mount point (its st_dev differs from its parent's), or when newAncientPath's parent is on a different device than oldAncientPath; and AncientDbReplacer should rename-then-delete rather than delete-then-rename. The default layout (<chaindata>/ancient, what this PR exercises) is not affected.
I've kept this PR to the one-line plumbing fix so it can land cleanly; happy to open a follow-up with the same-filesystem/mount-root guard and the rename-first ordering — or fold it in here if you'd rather have it in one change.
| directory: n.ResolvePath(name), | ||
| dbEngine: n.config.DBEngine, | ||
| DatabaseOptions: DatabaseOptions{ | ||
| AncientsDirectory: n.ResolveAncient(name, ancient), |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Operators whose bor snapshot prune-block is killed mid-backup, and who then start bor, can get a corrupted chain and a truncated freezer. On the base branch the command fails before writing anything. Now backupOldDb writes the new current offset at pruner.go:409 before the long copy loop. The old freezer stays at ancient/chain until AncientDbReplacer runs. Fix: when the block-prune lock files exist, node startup should refuse to open the database and tell the operator to rerun prune-block. A fix inside this change could instead write both offsets only after the backup loop succeeds. The design predates this change, but this change makes the path reachable.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
Trigger: bor snapshot prune-block is killed during the backup loop at pruner.go:448. By then pruner.go:409-412 has stored offsetOfCurrentAncientFreezer=startBlockNumber in the kv store, but the old freezer is still in ancient/chain. The operator then starts bor. rawdb.Open (database.go:353) reads the new offset and NewFreezer adds it to the old table count (freezer.go:162). Freezer reads map block N to table index N-newOffset (freezer.go:198), so ReadCanonicalHash returns hashes of the wrong blocks. The check at blockchain.go:856 sees head < frozen-1 and calls SetHead. That truncates the old tables to head-newOffset items (freezer.go:315), which destroys most of the ancient data. Nothing checks for PRUNEFLOCK at node startup; only the prune-block CLI reads it (snapshot.go:504). On the base branch, OpenDatabaseWithFreezer gave backupOldDb an empty MemoryFreezer. rawdb.Open then failed with "ancient chain segments already extracted" before any offset was written, so this state could not arise.
Verification: On the base, rawdb.Open failed with "ancient chain segments already extracted" (database.go:477) before any offset was written. With the PR, pruner.go:408-414 batch-writes the new current offset before the copy loop at pruner.go:448. Only the prune-block CLI checks the PRUNEFLOCK/PRUNEFLOCKBACK files (snapshot.go:504). At startup, Freezer.TruncateHead (freezer.go:315) cuts the old freezer down to about R+90k items.
There was a problem hiding this comment.
Agreed, and also pre-existing (same ordering on ≤v2.3.3): backupOldDb persists the new current offset (pruner.go:409-412) before the long copy loop, so a kill mid-backup followed by a plain bor server start opens the old tables with the new offset and SetHead truncates them. The CLI itself has RecoverInterruption (snapshot.go:516) for the resume case, but nothing at node startup checks PRUNEFLOCK/PRUNEFLOCKBACK.
Cleanest fix is to write both offsets only after the backup loop completes (and keep a startup refusal when the prune lock files are present as a belt-and-braces). I'd put that in the same follow-up as the mount-root guard above, unless you prefer it in this PR.
|
@pratikspatil024 the rewrite to a signed commit put the six workflow runs for the new head ( I've answered both inline findings from the review: both describe pre-existing behaviour of the pruner (identical on ≤v2.3.3, where this plumbing still worked) and I've proposed a follow-up PR with a same-filesystem/mount-root guard in |
|
On the This can't be caused by the change: Looks like a transient enclave bring-up abort on the fork runner path. I don't have rerun rights on this repo — @pratikspatil024 could you re-run |
Summary
node.OpenDatabaseWithFreezer(name, cache, handles, ancient, …)declares anancientargument but never puts it intoDatabaseOptions.openDatabasetherefore getsAncientsDirectory="",rawdb.OpencallsnewChainFreezer("")and gets an in-memory chain freezer with zero items. Every CLI path that opens the database through this helper then fails insiderawdb.Open's "freezer is empty" branch:Affected:
bor snapshot prune-block(validateAgainstSnapshot,internal/cli/snapshot.go),snapshot prune-state,snapshot inspect-ancient-db, andBlockPruner.backupOldDb(core/state/pruner/pruner.go).--datadir.ancienthas no effect because its value is dropped at this call site. The node itself is unaffected — the server opens the DB viaOpenDatabaseWithOptions, which setsopt.AncientsDirectory = n.ResolveAncient(name, …)explicitly. This change mirrors that one line.Tell-tale sign in the CLI output:
Opened ancient database … frozen=N offset=Mis missing —MemoryFreezernever logs it.Regression window: the line is present through v2.3.1 and v2.3.3, absent from v2.3.4 onwards (v2.4–v2.10.3,
develop,master). Issue #1274 showsprune-blockworking on 1.3.3, which matches. PR #1965 ("Add offset support to MemoryFreezer") looks like a symptom of the same bug — the CLI was silently running against aMemoryFreezer.PIP-32 ancient pruning (
bor snapshot prune-block) is effectively unusable on any release since v2.3.4 without this.Executed tests
No Go toolchain in the environment that produced this PR, so CI's unit gates are the first automated run; the change is a one-line mirror of the existing
OpenDatabaseWithOptionslogic andgofmt-clean for the hunk.Validated end-to-end on a Polygon mainnet node (
state.scheme=hash, snap sync, pebble, bor v2.10.3 + this patch), freezer 3.4 TB / 82,239,253 items in all six tables, node stopped cleanly:snapshot inspect-ancient-dbancient chain segments already extracted(with/without--datadir.ancient, any path)Opened ancient database … frozen=82,239,253 offset=0snapshot prune-block --block-amount-reserved 500000Block prune successfully, 530 s, freezer 3.4 TB → 52 GB--block-amount-reserved 400000)Block prune successfully, 406 s,offset=81,739,253 → 81,839,253Found legacy ancient chain path,Opened ancient database frozen=82,239,253 offset=81,839,253, sync resumed from the pruned head, no re-download below the offsetRollout notes
Not consensus-affecting, backwards-compatible, no config change. Operator-facing effect: the pruning CLI works again. Note that after
prune-blockthe chain freezer is rebuilt at the legacy location (<chaindata>/ancient/instead ofancient/chain/);resolveChainFreezerDiralready handles that ("Found legacy ancient chain path"), and theprune-blockhash-scheme requirement (errPbssNotSupported) is unchanged.