feat: async block import persistence - #8784
Merged
Merged
Conversation
Contributor
Summary of ChangesHello @wemeetagain, 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 refactors the block persistence mechanism by introducing an asynchronous job queue for writing unfinalized block data to the database. This change aims to improve performance during block import by offloading I/O operations and ensuring a more controlled and sequential persistence process. It also optimizes block retrieval by prioritizing cached data and centralizes cache management within the persistence logic. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
wemeetagain
commented
Jan 23, 2026
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces asynchronous persistence for block inputs, moving the "write block input to db" step to an asynchronous job queue. This change aims to improve performance by decoupling block import from immediate database writes. Key modifications include the introduction of a JobItemQueue for unfinalized block writes, updates to importBlock and verifyBlock to utilize this new queue, and a refactoring of writeBlockInputToDb to integrate with the async mechanism and ensure proper cache pruning. Additionally, the BeaconChain now checks the seenBlockInputCache for unfinalized blocks before querying the hot database, and a filterInPlace method was added to LinkedList to support job queue filtering. However, there is a critical bug in the writeBlockInputToDb function regarding how promises are collected and awaited.
Comment on lines
+94
to
+97
| await Promise.all(fnPromises); | ||
| this.logger.debug("Persisted blocksInput to db", { | ||
| blocksInput: blocksInputs.length, | ||
| slots: prettyPrintIndices(slots), |
Contributor
There was a problem hiding this comment.
The fnPromises array is declared once outside the for loop (line 16), and promises for each blockInput are pushed into it. However, await Promise.all(fnPromises); is called inside the loop (lines 94-97). This means that for each blockInput in blocksInputs, Promise.all will be called on the entire fnPromises array, which contains promises from all previous blockInputs processed so far. This leads to redundant awaiting and incorrect behavior, as promises from earlier blockInputs will be awaited multiple times. To fix this, these lines should be moved outside the for loop, after line 98, to ensure all promises are collected before being awaited once.
Member
Author
There was a problem hiding this comment.
I suppose this is correct, but not really the point of this PR
Member
There was a problem hiding this comment.
Depends on if we wanna write block inputs sequentially or in parallel so either should move fnPromises inside the for-loop or move await Promise.all(fnPromises) outside. From the logs it seems like sequential write is intended, maybe @matthewkeil remembers what the intend was. I don't think it matters much since we usually just write one block input per slot when following head
![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: 8744f60b03
ℹ️ 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".
wemeetagain
commented
Jan 23, 2026
wemeetagain
commented
Jan 23, 2026
nflaig
reviewed
Jan 23, 2026
nflaig
left a comment
nflaig
left a comment
Member
There was a problem hiding this comment.
part of this needs to be that we need to be able to serve by root requests which is now uncertain since we might update head before db write is done
I think we can use block input for this, so either serve the data from in-memory, or fallback to db query
Contributor
Performance Report✔️ no performance regression detected Full benchmark results
|
Member
Author
|
Added blob sidecar serving. Maybe some of logic chould be added into a chain method, like, |
wemeetagain
commented
Jan 23, 2026
nflaig
reviewed
Jan 24, 2026
nflaig
reviewed
Jan 24, 2026
nflaig
reviewed
Jan 24, 2026
nflaig
reviewed
Jan 24, 2026
nflaig
reviewed
Jan 24, 2026
nflaig
reviewed
Jan 25, 2026
twoeths
reviewed
Jan 26, 2026
twoeths
reviewed
Jan 28, 2026
| throw new Error(`Expected block input to have columns: ${blockSlot} ${blockRootHex}`); | ||
| } | ||
| if (indices === undefined) { | ||
| return blockInput.getAllColumns(); |
Member
There was a problem hiding this comment.
do we have to have this fallback?
instead of that just throw error to avoid unnecessary overhead
Member
Author
There was a problem hiding this comment.
yes this is used in the beacon API. The intention of nullable indices is to provide a way to say "give me all stored columns for that block"
twoeths
reviewed
Jan 28, 2026
wemeetagain
commented
Jan 28, 2026
wemeetagain
commented
Jan 28, 2026
Member
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a significant performance improvement by making block persistence asynchronous. The use of a job queue for database writes decouples block processing from I/O operations, which should improve chain head advancement times. The refactoring of data access methods to use a unified interface on the BeaconChain object is a great step towards better code organization and abstraction. My review includes a suggestion to update a vulnerable dependency and a fix for a method signature mismatch.
nflaig
reviewed
Jan 29, 2026
nflaig
approved these changes
Jan 29, 2026
twoeths
approved these changes
Jan 30, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #8784 +/- ##
============================================
+ Coverage 52.04% 52.33% +0.28%
============================================
Files 848 848
Lines 64584 63784 -800
Branches 4762 4718 -44
============================================
- Hits 33612 33379 -233
+ Misses 30903 30336 -567
Partials 69 69 🚀 New features to boost your workflow:
|
nflaig
added a commit
that referenced
this pull request
Feb 2, 2026
…blocks (#8823) Follow up on #8784 to consistently check block input cache before checking hot db for blocks. This avoids edge cases where block is not yet persistent in db, also querying db if we can serve it from in-memory cache seems unnecessary. This issue was caught by e2e tests ``` Error: Head block 30 null is not available in database ❯ BeaconChain.onForkChoiceFinalized src/chain/chain.ts:1351:13 ```
lodekeeper
added a commit
to lodekeeper/lodestar
that referenced
this pull request
Feb 9, 2026
Add waitForSpace() to JobItemQueue that resolves immediately when space is available, or awaits until a running job completes and frees a slot. Use waitForSpace() in importBlock before pushing to the unfinalized block write queue. This applies backpressure during sync, preventing supernodes (128 custody groups) from accumulating 30+ blocks of column data in memory when persistence can't keep up, which causes OOM. Ref: ChainSafe#8784 (comment) Co-authored-by: lodekeeper <lodekeeper@users.noreply.github.com>
1 task
matthewkeil
added a commit
that referenced
this pull request
Feb 11, 2026
## Motivation During finalized sync, supernodes with 128 custody groups can accumulate 30+ blocks of column data in memory when the async write queue can't persist fast enough. Each block holds 128 column sidecars, and with a queue depth of 32, this can consume ~10GB+ of external memory causing OOM kills and crash loops. This was observed on `beta-mainnet-super` during v1.40.0 RC testing — the node repeatedly crashed with no error logs (OOM kill signature), with external memory spiking from ~4.5GB to ~10GB before each crash. Ref: #8784 (comment) ## Description - Add `waitForSpace()` method to `JobItemQueue` that resolves immediately when the queue has capacity, or awaits until a running job completes and frees a slot - Call `waitForSpace()` in `importBlock` before pushing to the unfinalized block write queue, applying backpressure during sync - When the queue is not full (normal operation at head), `waitForSpace()` is a noop — no performance impact - During sync when the queue fills up, block import pauses until persistence catches up, preventing unbounded memory growth ## Changes - `packages/beacon-node/src/util/queue/itemQueue.ts` — Add `waitForSpace()` method and `notifySpaceWaiters()` hook after job completion - `packages/beacon-node/src/chain/blocks/importBlock.ts` — Await queue space before pushing block input - `packages/beacon-node/test/unit/util/queue.test.ts` — Add 3 tests: immediate resolve, blocking until space, abort handling ## AI Assistance Disclosure - [x] External Contributors: I have read the [contributor guidelines](https://github.com/ChainSafe/lodestar/blob/unstable/CONTRIBUTING.md#ai-assistance-notice) and disclosed my usage of AI below. This PR was authored by Lodekeeper (AI assistant) with review from GPT-5.2 and Gemini sub-agents. Co-authored-by: lodekeeper <lodekeeper@users.noreply.github.com> --------- Co-authored-by: lodekeeper <lodekeeper@users.noreply.github.com> Co-authored-by: Nico Flaig <nflaig@protonmail.com> Co-authored-by: Matthew Keil <github@mail.matthewkeil.com>
Member
Author
|
🎉 This PR is included in v1.40.0 🎉 |
twoeths
added a commit
to twoeths/lodestar
that referenced
this pull request
Feb 14, 2026
…letes `serializedCache.clear()` was called immediately after queuing the block for async DB persistence in `importBlock()`. Since `writeBlockInputToDb` runs asynchronously, the cache was always empty by the time the write tried to read from it — silently defeating the serialization optimization on every block import. The regression was introduced in ChainSafe#8784 when the DB write was converted from `await` to fire-and-forget, without moving the cache clear accordingly. Move the clear to the `finally` block of `persistBlockInputs` so it runs after the DB write completes, and add a JSDoc on `SerializedCache.clear()` documenting the constraint. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
nflaig
added a commit
that referenced
this pull request
Feb 16, 2026
…letes (#8907) **Motivation** `serializedCache.clear()` was called immediately after queuing the block for async DB persistence in `importBlock()`. Since the DB write (`writeBlockInputToDb`) runs asynchronously, the cache was always empty by the time the write tried to read from it — silently defeating the serialization optimization on every block import. The regression was introduced in #8784 when the DB write was converted from `await` to fire-and-forget, without moving the cache clear accordingly. **Description** - Remove `serializedCache.clear()` from `importBlock.ts` where it ran too early (before the async DB write) - Move it to the `finally` block of `persistBlockInputs` in `writeBlockInputToDb.ts`, so it runs after the DB write completes regardless of success or failure - Add a JSDoc on `SerializedCache.clear()` documenting that it must only be called after all DB writes that may read from the cache have completed **AI Assistance Disclosure** This fix was identified and authored with Claude Code (Claude Sonnet 4.5). Co-authored-by: Tuyen Nguyen <twoeths@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Co-authored-by: Nico Flaig <nflaig@protonmail.com>
rnfairchild
pushed a commit
that referenced
this pull request
Feb 17, 2026
…letes (#8907) **Motivation** `serializedCache.clear()` was called immediately after queuing the block for async DB persistence in `importBlock()`. Since the DB write (`writeBlockInputToDb`) runs asynchronously, the cache was always empty by the time the write tried to read from it — silently defeating the serialization optimization on every block import. The regression was introduced in #8784 when the DB write was converted from `await` to fire-and-forget, without moving the cache clear accordingly. **Description** - Remove `serializedCache.clear()` from `importBlock.ts` where it ran too early (before the async DB write) - Move it to the `finally` block of `persistBlockInputs` in `writeBlockInputToDb.ts`, so it runs after the DB write completes regardless of success or failure - Add a JSDoc on `SerializedCache.clear()` documenting that it must only be called after all DB writes that may read from the cache have completed **AI Assistance Disclosure** This fix was identified and authored with Claude Code (Claude Sonnet 4.5). Co-authored-by: Tuyen Nguyen <twoeths@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Co-authored-by: Nico Flaig <nflaig@protonmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Description
writeBlockInputToDbasync with block import/head updatechain.getBlobSidecarsandchain.getSerializedBlobSidecars-- note: only serves all or nonechain.getDataColumnSidecarsandchain.getSerializedDataColumnSidecarswriteBlockInputToDbprocess prune the block input cache after its runeagerPersistBlockoption, since it's now irrelevant