feat: add support for backfilling blocks #8677
Conversation
Summary of ChangesHello @vedant-asati, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the beacon node's ability to perform historical block synchronization, making it a more robust and useful participant in the network. It introduces a new, more resilient Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 implementation for backfill sync, a significant feature for historical block synchronization. The changes are extensive, including a new BackfillSync class, updates to the database schema, and new peer management logic. My review has identified several areas for improvement to ensure the feature is robust and maintainable. Key issues include a critical typo in a property name, a potential bug in handling multi-epoch finalization, incomplete error handling, and several instances of throwing generic errors which can make debugging difficult. Additionally, there are opportunities to improve code quality by removing debug code, refactoring duplicated logic, and replacing magic numbers with constants. Addressing these points will greatly enhance the quality and reliability of the new backfill sync implementation.
| // Todo: verify if this function runs every epoch, else intermediate epoch backfill states will be empty. | ||
| // Below could be a possible solution to this issue. | ||
|
|
||
| // // In case of long unfinality, this needs to be done to save multiple epochs | ||
| // // First, find all *unique* epochs from the list of finalized blocks | ||
| // const uniqueEpochs = Array.from(new Set(finalizedCanonicalBlocks.map((block) => block.finalizedEpoch))); | ||
| // const backfillStates: KeyValue<number, EpochBackfillState>[] = uniqueEpochs.map((epoch) => { | ||
| // return { | ||
| // key: epoch, | ||
| // value: { | ||
| // hasBlock: true, | ||
| // // check if blobs & columns are filled in live chain | ||
| // hasBlobs: finalizedPostDeneb ? true : null, | ||
| // columnIndices: finalizedPostFulu ? [] : null, | ||
| // }, | ||
| // }; | ||
| // }); | ||
| // await db.backfillState.batchPut(backfillStates); |
There was a problem hiding this comment.
The Todo comment on line 75 highlights a potential issue: if finalization spans multiple epochs, this function might not be called for every epoch, which could lead to gaps in the backfillState. The current implementation only updates the state for finalized.epoch. The commented-out code block below it provides a good solution by iterating over all finalized blocks to update the state for each unique epoch. This logic should be implemented to ensure correctness and prevent gaps in the backfill state.
| continue; | ||
| } | ||
| if (!validationRes.nextAnchor) { | ||
| throw Error; |
There was a problem hiding this comment.
Throwing a generic Error object without a descriptive message makes debugging difficult. This occurs here and in other places in this file (e.g., lines 595, 604, 809, 910). A more specific error with a helpful message should be thrown.
| throw Error; | |
| throw new Error("Validation failed to produce a next anchor"); |
| // Todo | ||
| // if (error instanceof BackfillSyncError) { | ||
| // switch (error.type.code) { | ||
| // // case BackfillSyncErrorCode.INTERNAL_ERROR: | ||
| // // // Break it out of the loop and throw error | ||
| // // this.status = BackfillSyncStatus.aborted; | ||
| // // break; | ||
| // // case BackfillSyncErrorCode.NOT_ANCHORED: | ||
| // // // biome-ignore lint/suspicious/noFallthroughSwitchClause: We need fall-through behavior here | ||
| // // case BackfillSyncErrorCode.NOT_LINEAR: | ||
| // // // Lets try to jump directly to the parent of this anchorBlock as previous | ||
| // // // (segment) of blocks could be orphaned/missed | ||
| // // if (this.syncAnchor.anchorBlock) { | ||
| // // this.syncAnchor = { | ||
| // // anchorBlock: null, | ||
| // // anchorBlockRoot: this.syncAnchor.anchorBlock.message.parentRoot, | ||
| // // anchorSlot: null, | ||
| // // lastBackSyncedBlock: this.syncAnchor.lastBackSyncedBlock, | ||
| // // }; | ||
| // // } | ||
|
|
||
| // // // falls through | ||
| // case BackfillSyncErrorCode.INVALID_SIGNATURE: | ||
| // this.network.reportPeer("goodPeer", PeerAction.LowToleranceError, "BadSyncBlocks"); | ||
| // } | ||
| // } |
There was a problem hiding this comment.
The catch block for the main sync logic contains a large commented-out section for handling specific BackfillSyncError types. The current implementation only logs the error. This commented-out logic, which includes actions like reporting a misbehaving peer, should be implemented to make the backfill sync more robust.
| // DEBUG_CODE | ||
| logger.info("Updated backfillRange while migrating from hot to cold db", { | ||
| beginningEpoch: computeEpochAtSlot(finalizedBlockFC.slot), | ||
| endingEpoch: previousBackfillRange?.endingEpoch || computeEpochAtSlot(chain.anchorStateLatestBlockSlot), | ||
| previousBackfillRangeBeginningEpoch: previousBackfillRange?.beginningEpoch, | ||
| previousBackfillRangeEndingEpoch: previousBackfillRange?.endingEpoch, | ||
| chainAnchorStateLatestBlockSlotEpoch: computeEpochAtSlot(chain.anchorStateLatestBlockSlot), | ||
| }); | ||
| // DEBUG_CODE |
There was a problem hiding this comment.
This DEBUG_CODE block, along with the one at lines 66-73, appears to be for debugging. This verbose logging should be removed before merging to keep the codebase clean and avoid excessive noise in production logs. Similar debug logging is present in other new files in this PR and should also be removed.
| modules.logger.error("Invalid prevBackfillRange in db. Reinitializing backfill states using anchorState."); | ||
| // use anchor from modules | ||
| const {checkpoint: anchorCp} = computeAnchorCheckpoint(config, anchorState); | ||
| const anchorBlockParentRoot = anchorState.latestBlockHeader.toValue().parentRoot; | ||
| const anchorSlot = anchorState.latestBlockHeader.slot; | ||
| syncAnchor = { | ||
| anchorBlockParentRoot, | ||
| anchorBlock: null, | ||
| anchorBlockRoot: anchorCp.root, // this may help | ||
| anchorSlot, | ||
| lastBackSyncedBlock: null, | ||
| }; | ||
| // Initialize backfill states to maintain point of reference for future | ||
| await modules.db.backfillRange.put({beginningEpoch: anchorCp.epoch, endingEpoch: anchorCp.epoch}); | ||
| await modules.db.backfillState.put(anchorCp.epoch, {hasBlock: true, hasBlobs: true, columnIndices: []}); | ||
| } | ||
| } else { | ||
| // Todo: Remove this duplicate code. | ||
| if (isForcedCheckpointSync) | ||
| modules.logger.warn("ForcedCheckpointSync. Initializing backfill states using anchorState(checkpointState)."); | ||
| else modules.logger.warn("prevBackfillRange absent in db. Initializing backfill states using anchorState."); | ||
| // use anchor from modules | ||
| const {checkpoint: anchorCp} = computeAnchorCheckpoint(config, anchorState); | ||
| const anchorBlockParentRoot = anchorState.latestBlockHeader.toValue().parentRoot; | ||
| const anchorSlot = anchorState.latestBlockHeader.slot; | ||
| syncAnchor = { | ||
| anchorBlockParentRoot, | ||
| anchorBlock: null, | ||
| anchorBlockRoot: anchorCp.root, // this may help | ||
| anchorSlot, | ||
| lastBackSyncedBlock: null, | ||
| }; | ||
| // Initialize backfill states to maintain point of reference for future | ||
| await modules.db.backfillRange.put({beginningEpoch: anchorCp.epoch, endingEpoch: anchorCp.epoch}); | ||
| await modules.db.backfillState.put(anchorCp.epoch, {hasBlock: true, hasBlobs: true, columnIndices: []}); | ||
| } |
There was a problem hiding this comment.
There is significant code duplication between the else block starting at line 236 and the one at line 254. Both blocks handle the initialization of syncAnchor and backfill states when starting from the anchorState. This logic should be extracted into a helper function to reduce duplication and improve maintainability.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…..f instead of 0x0..01
1 participant
Motivation
This PR introduces support for backfilling blocks, enabling historical block sync, following the spec, and to be a more useful peer to the network.
Tracks: #7753
Description
This PR continues the work from PR #8353, which got little cluttereddue to an incorrect base merge, to provide a clean continuation.
It builds on the db changes already merged in PR #8085 (BackfillState & BackfillRange repos).
This adds:
Future Work(separate PRs):
AI Assistance Disclosure
AI (Gemini/ChatGPT) was used to get reviews and trace bugs in the implementation.