feat: backfill sync v2 - #9238
Conversation
…ckfillStateRepository
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new backfill synchronization implementation (BackfillSyncV2) and updates the database schema to track backfill progress using epochs. Key changes include the addition of BackfillStateRepository and the BackfilledRange singleton, while the legacy BackfillSync and BackfilledRanges are being deprecated or disabled. Feedback highlights a breaking database schema change due to bucket repurposing, the presence of non-functional legacy code, and performance concerns regarding block-by-block network fetching and sequential database lookups in the new implementation. Additionally, starting the sync process within the constructor is noted as a testing and control issue.
| // index_lightClientInitProof = 36, // DEPRECATED on v0.32.0 | ||
|
|
||
| backfilled_ranges = 42, // Backfilled From to To, inclusive of both From, To | ||
| backfill_state = 42, // Epoch -> EpochBackfillState |
There was a problem hiding this comment.
Repurposing bucket ID 42 from backfilled_ranges (which stored Slot -> Slot) to backfill_state (which stores Epoch -> BackfillStateWrapper) is a breaking change for the database schema. Existing databases will contain data in the old format, which will cause deserialization errors or logic bugs when accessed by the new code. A database migration should be provided, or a new bucket ID should be allocated for the new backfill state.
There was a problem hiding this comment.
Confirmed — verified by diffing packages/beacon-node/src/db/buckets.ts against origin/unstable:
unstable@HEAD:backfilled_ranges = 42 // Backfilled From to To, inclusive of both From, To(Slot → Slot)- this PR:
backfill_state = 42 // Epoch -> EpochBackfillState
Same ID, different schema. Any existing node DB has entries at bucket 42 in the old Slot → Slot shape; the new repository would try to deserialize those bytes as EpochBackfillState and either fail or load garbage. Two clean paths: allocate a fresh bucket ID for backfill_state and leave 42 as a deprecated reserved slot (lowest-risk), or add a one-shot migration that drops the old bucket 42 contents before the new repository is wired in. Reserving a new ID is the cheaper option.
| const backfillRangeWrittenSlot = null as number | null; | ||
| const previousBackfilledRanges = [] as {key: number; value: number}[]; |
There was a problem hiding this comment.
The legacy BackfillSync implementation is being broken here by hardcoding backfillRangeWrittenSlot to null and previousBackfilledRanges to an empty array. If this class is intended to be deprecated and replaced by BackfillSyncV2, it should be formally deprecated or removed. Leaving it in a non-functional state with TODO comments can lead to confusion and bugs if it's still reachable in certain configurations.
There was a problem hiding this comment.
Confirmed — the legacy class is still wired into production via the barrel export export * from './backfill.js' and is what packages/beacon-node/src/node/nodejs.ts:292 instantiates via BackfillSync.init(...). With backfillRangeWrittenSlot = null and previousBackfilledRanges = [] hardcoded at lines 244-246 (plus the commented-out backfilledRanges.put(...) at line 394 that Codex flagged in discussion_r3393024831), the legacy path now silently loses its resume state — nodes with sync.backfillBatchSize > 0 will re-download from the anchor on every restart. Since the new v2 class isn't reachable from nodejs.ts either (see discussion_r3393024826), this PR effectively breaks backfill resume in production. Two scope options: keep legacy fully functional and add v2 alongside under a flag, OR replace legacy completely (wire v2 in nodejs.ts, delete backfill.ts).
| // try { | ||
| // const finalizedBlockFC = chain.forkChoice.getBlockHexDefaultStatus(finalized.rootHex); | ||
| // if (finalizedBlockFC && finalizedBlockFC.slot > chain.anchorStateLatestBlockSlot) { | ||
| // await db.backfilledRanges.put(finalizedBlockFC.slot, chain.anchorStateLatestBlockSlot); | ||
| // | ||
| // const filteredSeqs = await db.backfilledRanges.entries({ | ||
| // gt: chain.anchorStateLatestBlockSlot, | ||
| // lt: finalizedBlockFC.slot, | ||
| // }); | ||
| // logger.debug("updated backfilledRanges", { | ||
| // key: finalizedBlockFC.slot, | ||
| // value: chain.anchorStateLatestBlockSlot, | ||
| // }); | ||
| // if (filteredSeqs.length > 0) { | ||
| // await db.backfilledRanges.batchDelete(filteredSeqs.map((entry) => entry.key)); | ||
| // logger.debug( | ||
| // `Forward Sync - cleaned up backfilledRanges between ${finalizedBlockFC.slot},${chain.anchorStateLatestBlockSlot}`, | ||
| // {seqs: JSON.stringify(filteredSeqs)} | ||
| // ); | ||
| // } | ||
| // } | ||
| // } catch (e) { | ||
| // logger.error("Error updating backfilledRanges on finalization", {epoch: finalized.epoch}, e as Error); | ||
| // } |
There was a problem hiding this comment.
The logic for updating the backfill range during finalization has been commented out. This effectively disables progress tracking for backfill sync during normal node operation. If this logic is being replaced by the new backfillState and backfilledRange singleton, it should be updated to use them instead of being left as commented-out code.
There was a problem hiding this comment.
Confirmed — the entire updateBackfillRange body is now commented out (lines ~17-43 of packages/beacon-node/src/chain/archiveStore/utils/updateBackfillRange.ts). The function is still called from the finalization path but is a no-op. Same scope question as discussion_r3109761377 — either restore it pointing at the new singleton backfilledRange + per-epoch backfillState, or delete the call site and the helper entirely as part of the legacy-removal change.
| this.sync() | ||
| .then((oldestSlotSynced) => { | ||
| if (this.status !== BackfillSyncStatus.completed) { | ||
| throw new ErrorAborted(`Invalid BackfillSyncStatus at completion: status = ${this.status}`); | ||
| } | ||
| this.emit(BackfillSyncEvent.completed, oldestSlotSynced); | ||
| this.logger.info("BackfillSync completed", {oldestSlotSynced}); | ||
| this.close(); | ||
| }) | ||
| .catch((e) => { | ||
| if (!(e instanceof ErrorAborted)) { | ||
| this.logger.error("BackfillSync processor error", e); | ||
| } | ||
| this.status = BackfillSyncStatus.aborted; | ||
| this.close(); | ||
| }); |
There was a problem hiding this comment.
Starting a long-running asynchronous process like sync() directly in the constructor is generally considered a bad practice. It makes the object difficult to test, as the sync process starts immediately upon instantiation, and it doesn't allow the caller to control when the synchronization begins. Consider moving the call to this.sync() to an explicit start() method.
There was a problem hiding this comment.
Fair pattern observation, though pragmatically it follows what the legacy BackfillSync.init does (backfill.ts also kicks off this.sync() after construction). For internal services owned by BeaconNode, fire-and-forget in the ctor is the existing convention in this codebase — moving to an explicit start() would be its own ergonomics PR across multiple services. Not a blocker for this PR's scope, but worth a brief code comment near line 174 documenting that the .catch chain is the lifecycle handle and that abort flows through the constructor-bound AbortSignal.
| } | ||
| try { | ||
| // how could they fetch a block via root? | ||
| const [block] = await this.network.sendBeaconBlocksByRoot(peer, [this.anchorRoot]); |
There was a problem hiding this comment.
Fetching blocks one by one using sendBeaconBlocksByRoot is highly inefficient due to network round-trip times (RTT). Even if backfill sync is considered low priority, fetching a single block per request will make the process extremely slow for a long chain. Consider fetching blocks in batches using sendBeaconBlocksByRange or requesting multiple roots at once if the protocol supports it.
There was a problem hiding this comment.
Partially — backfillV2.ts already has a batched path at line 377 (sendBeaconBlocksByRoot(peer, batch)). The one-at-a-time fetch is at line 572 inside the parent-walk loop, which is constrained by the dependency: each request's input root is the previous response's parentRoot, so you can't pipeline more than one until the response lands. The cheap improvement is switching from sendBeaconBlocksByRoot(peer, [root]) to sendBeaconBlocksByRange(peer, {startSlot, count}) once we know we want N contiguous slots ending at the anchor — same RTT for N blocks instead of N. Worth doing but agreed it's a perf-only follow-up, not a correctness blocker.
| while (epoch > 0) { | ||
| const state = await this.db.backfillState.get(epoch); | ||
| if (!state?.hasBlock) break; | ||
| epoch--; |
There was a problem hiding this comment.
This while loop performs sequential database lookups for every epoch. If a node has already backfilled a large number of epochs, this loop could perform thousands of asynchronous DB calls, potentially blocking the event loop and causing performance issues. It would be more efficient to use a reverse stream or iterator (e.g., this.db.backfillState.keysStream({lt: startEpoch, reverse: true})) to find the first gap in the backfilled epochs.
There was a problem hiding this comment.
Confirmed — skipFilledEpochs at lines 665-672 does sequential await this.db.backfillState.get(epoch) per epoch, walking down from startEpoch until it finds a gap. For a node that has already backfilled thousands of epochs this is thousands of awaits in a tight loop. The backfillState.keysStream({lt: startEpoch, reverse: true}) suggestion is the right shape — return the first epoch whose hasBlock is false, or the lowest key if none have a gap. Single-pass stream instead of point lookups.
- Introduced comprehensive tests for the backfillV2 synchronization process, covering scenarios such as chain walking, flushing blocks at epoch boundaries, and handling skipped slots.
- Added a check for the aborted signal in the BackfillSync class to ensure proper termination of the sync process when the signal is triggered.
I found the peer score should be not that harsh because some behaviors are legitimate according to the spec.
|
@lodekeeper could you review this PR? |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a122a0622
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -1 +1,2 @@ | |||
| export * from "./backfill.js"; | |||
| export {BackfillSync as BackfillSyncV2} from "./backfillV2.js"; | |||
There was a problem hiding this comment.
The barrel still exports the new implementation only as BackfillSyncV2, while packages/beacon-node/src/node/nodejs.ts imports BackfillSync from this barrel and calls BackfillSync.init(opts.sync, ...) whenever sync.backfillBatchSize > 0. In normal beacon-node startup the new v2 code and the new sync.backfillToGenesis default are therefore never used; nodes will continue running the legacy backfill path instead of stopping at the weak-subjectivity floor by default.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and this is the headline blocker. Verified end-to-end:
// packages/beacon-node/src/sync/backfill/index.ts
export * from "./backfill.js";
export {BackfillSync as BackfillSyncV2} from "./backfillV2.js";
// packages/beacon-node/src/node/nodejs.ts:21,292
import {BackfillSync} from "../sync/backfill/index.js";
...
opts.sync.backfillBatchSize > 0
? await BackfillSync.init(opts.sync, {...})The BackfillSync identifier in nodejs.ts resolves to the legacy class via the export * re-export — the new v2 implementation is only reachable as BackfillSyncV2, which nothing imports. So nodejs.ts still constructs the legacy BackfillSync, and combined with the legacy-side changes in this PR (hardcoded null / [] per discussion_r3109761377, commented-out backfilledRanges.put per discussion_r3393024831), production gets a strictly worse legacy backfill and zero exposure to v2.
Need to either: (a) swap the export so the barrel's BackfillSync IS the v2 class and the legacy one is gone, or (b) update nodejs.ts to import BackfillSyncV2 and gate selection on a sync option. Either path also needs the bucket-42 reuse from discussion_r3109761372 resolved before merge.
| // TODO: migrate to backfillState | ||
| // await this.db.backfilledRanges.put(this.backfillStartFromSlot, this.backfillRangeWrittenSlot); |
There was a problem hiding this comment.
Because the node still instantiates the legacy BackfillSync path, commenting out this DB write means an interrupted backfill no longer persists its latest synced slot. On restart init now seeds backfillRangeWrittenSlot from null and previousBackfilledRanges from [], so any node with sync.backfillBatchSize > 0 redownloads from the checkpoint anchor instead of resuming the already verified range.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — follows directly from the v2-not-wired situation in discussion_r3393024826. Because nodejs.ts still instantiates the legacy BackfillSync, the commented-out await this.db.backfilledRanges.put(this.backfillStartFromSlot, this.backfillRangeWrittenSlot) at line 394 (plus the hardcoded previousBackfilledRanges = [] and backfillRangeWrittenSlot = null at init time per discussion_r3109761377) means nodes with sync.backfillBatchSize > 0 lose backfill resume on restart and re-download from the anchor every time.
Resolution is bundled with the wiring fix: either fully remove the legacy class once v2 is wired and the bucket-42 reuse from discussion_r3109761372 is sorted, or restore the legacy write path until v2 takes over.
lodekeeper
left a comment
lodekeeper
left a comment
There was a problem hiding this comment.
Thanks for the contribution — the v2 design (per-epoch BackfillStateRepository, parent-walk with epoch-boundary buffering, singleton backfilledRange) is a reasonable direction conceptually. There are however two blocking issues that need to be sorted before this can merge, plus a handful of secondary items the bots flagged that I've confirmed.
Blockers
-
The new v2 code isn't wired into production (
discussion_r3393024826/r3393041220).packages/beacon-node/src/sync/backfill/index.tsexportsBackfillSyncfrom the legacybackfill.jsand re-exports the new class only asBackfillSyncV2.packages/beacon-node/src/node/nodejs.ts:292importsBackfillSyncfrom that barrel and instantiates it onsync.backfillBatchSize > 0, so the v2 class is dead code in production today. Either (a) swap the barrel soBackfillSyncIS the v2 class and the legacy file is removed, or (b) gate on a sync option innodejs.tsand importBackfillSyncV2explicitly. -
Bucket ID 42 is reused for incompatible schemas (
discussion_r3109761372/r3393038740). Old:backfilled_ranges = 42(Slot → Slot). New:backfill_state = 42(Epoch → EpochBackfillState). Any node upgrading on an existing DB will hit deserialization errors or silently load garbage from bucket 42. Allocate a fresh bucket ID forbackfill_state(cheapest path; mark 42 as deprecated reserved), or land a one-shot migration that clears bucket 42 contents before the new repository is wired in.
The two interact: while v2 isn't wired (#1), the changes to the legacy path in this PR — hardcoded backfillRangeWrittenSlot = null / previousBackfilledRanges = [] (discussion_r3109761377 / r3393039227) and commented-out backfilledRanges.put (discussion_r3393024831 / r3393041523) — actively break backfill resume for production nodes with sync.backfillBatchSize > 0. They'll re-download from the anchor on every restart. So this PR as-is is a regression for the legacy path AND ships the new path as dead code.
Secondary
updateBackfillRangeis now a fully commented-out no-op (discussion_r3109761380/r3393039455). Resolve as part of the legacy-removal direction.skipFilledEpochs(backfillV2.ts:665-672) does sequentialawait db.backfillState.get(epoch)per epoch — for nodes that have already backfilled a long range this is thousands of point lookups. Switch tobackfillState.keysStream({lt: startEpoch, reverse: true})and find the first epoch missinghasBlockin one pass (discussion_r3109761400/r3393040539).sendBeaconBlocksByRoot(peer, [root])in the parent-walk (line 572) is RTT-bound by design (each request's input is the previous response'sparentRoot), so the existing batched path at line 377 is the right model — switching the parent-walk tosendBeaconBlocksByRangeonce N target slots are known would amortize the RTT (discussion_r3109761397/r3393040366). Perf-only follow-up, not a correctness blocker.sync()in the constructor (discussion_r3109761390/r3393040025) matches the existing legacy convention; not a blocker, but a brief comment near line 174 about lifecycle ownership and the.catchchain would help readers.
Suggestion
Worth pinging @nflaig for direction on scope — the v2 wiring + bucket migration is a non-trivial design decision (specifically: do we drop legacy entirely now, or run them side-by-side under a flag while v2 stabilizes?). Once that's settled the rest is mechanical.
|
Reviewed — full breakdown in
Secondary perf/cleanup items in the inline replies. Worth pinging @nflaig for direction on whether legacy gets removed entirely or runs side-by-side under a flag while v2 stabilizes — once that's decided the rest is mechanical. |
4 participants
Motivation
reconstructing backfill sync feature in lodestar.
Description
Block
DB schema related changes
BackfillStateRepository: per-epoch state with fieldshasBlockhasBlobcolumnIndicesBackfilledRangesfrom repository -> singleton since it's always a single value, never a keyed collection.backfillV2.tsparentRoot, one block per request. (This might seems not efficient but IMO backfill sync has the low priority so making the thread light is important.)slots at boundaries correctly)
anchorRoot === ZERO_HASH(i.e. we've walked past the genesisblock's parent) (TODO: changing to
MIN_EPOCHS_FOR_BLOCK_REQUESTSas default for block,MIN_EPOCHS_FOR_BLOB_SIDECARS_REQUESTSfor blobs)db.backfilledRangeDB schema refactor:
BackfillStateRepository(backfill_statebucket): per-epoch state(
hasBlock,hasBlobs,columnIndices) so future work can fill blobs/columns independently of blocks.
backfilledRangemoved fromRepository→single/(singleton pattern)since it's always a single value, never a keyed collection.
mockedBeaconDbfor the new/changed entries.Blobs
tests
Unit tests in
test/unit/sync/backfill/backfillV2.test.tscover:AI Assistance Disclosure
TODOs