feat(blst): replace blst and pubkeys with lodestar-z#8900
feat(blst): replace blst and pubkeys with lodestar-z#8900spiral-ladder wants to merge 15 commits intounstablefrom
blst and pubkeys with lodestar-z#8900Conversation
blst with blst-z where possible
Summary of ChangesHello @spiral-ladder, 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 updates the beacon node's cryptographic backend by replacing the existing 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. Changelog
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
|
blst with blst-z where possibleblst with blst-z where possible
There was a problem hiding this comment.
Code Review
This pull request replaces the @chainsafe/blst dependency with @chainsafe/blst-z in most of the codebase. The changes are mostly import replacements, but also include an update to the aggregateSignatures function calls, adding a false argument to likely disable signature verification for performance. In two spec test files, aggregateVerify is still imported from the old @chainsafe/blst library, which seems intentional. My review includes suggestions to add comments explaining this mixed dependency for better maintainability.
5604001 to
59688b7
Compare
|
You should be able to either |
ec6bb4d to
6c95d69
Compare
blst with blst-z where possibleblst with lodestarz/blst
blst with lodestarz/blstblst with lodestar-z/blst
Performance Report✔️ no performance regression detected Full benchmark results
|
## Description During finalized sync, blocks are imported in a tight loop without yielding to the event loop. When BLS verification is async (not awaiting the execution engine's `newPayload`), this prevents the checkpoint state cache's `processState()` cleanup from running, causing unbounded state accumulation and OOM on memory-constrained hosts. This adds `nextEventLoop()` after each `importBlock` to allow pending async cleanup (state serialization, cache eviction) to execute between block imports. ### Root Cause In `processBlocks()`, the import loop previously had a natural yield point when `importBlock` awaited the execution engine. With async BLS via `@chainsafe/blst` (PR #8900), `importBlock` no longer blocks on the EL call, so blocks blast through without yielding. The checkpoint state cache's `processState()` — which runs as fire-and-forget async — never gets a chance to serialize and evict old states, causing memory to climb until OOM. ### Fix ```typescript for (const fullyVerifiedBlock of fullyVerifiedBlocks) { await importBlock.call(this, fullyVerifiedBlock, opts); await nextEventLoop(); // Allow processState() cleanup to run } ``` `nextEventLoop()` is an existing utility (`sleep(0)`) that yields to the event loop's timers phase, giving pending microtasks and macrotasks a chance to execute. ### Testing Deployed as commit 983d923 on feat3 infrastructure nodes — resolved OOM crash loops on nodes with sufficient memory. Memory-constrained hosts (~16GB) may need additional tuning (reduced `maxBlockStates`, queue depth limits). Co-authored-by: Cayman <caymannava@gmail.com> --- > [!NOTE] > This PR was authored with AI assistance (Claude). All code has been reviewed and validated. Co-authored-by: lodekeeper <lodekeeper@users.noreply.github.com> Co-authored-by: Cayman <caymannava@gmail.com>
549bae6 to
9152e82
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9152e822a4
ℹ️ 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".
.github/workflows/test-bun.yml
Outdated
| version: ${{ env.ZIG_VERSION }} | ||
| cache-key: ${{ env.ZIG_VERSION }} |
There was a problem hiding this comment.
Define ZIG_VERSION before invoking setup-zig
This workflow now calls mlugg/setup-zig@v2 with version: ${{ env.ZIG_VERSION }} and cache-key: ${{ env.ZIG_VERSION }}, but test-bun.yml does not define ZIG_VERSION at workflow/job scope (unlike the other updated workflows). On Bun CI runs this yields an empty version input, which can make the Zig setup step fail or become non-deterministic by falling back to an implicit default, blocking or destabilizing the Bun test job.
Useful? React with 👍 / 👎.
b50b073 to
80473b8
Compare
blst with lodestar-z/blstblst and pubkeys with lodestar-z
f74941a to
a701111
Compare
3258fac to
2018db5
Compare
**Motivation** as previously discussed, remove bun tests entirely. Extracted from #8900 cc @nazarhussain
f8fd7c1 to
1961757
Compare
1961757 to
ecb2087
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request replaces the @chainsafe/blst dependency with @chainsafe/lodestar-z across the entire codebase. This is a significant refactoring that also introduces a new native pubkey cache for better performance, replacing createPubkeyCache with a singleton getPubkeyCache. The changes are extensive but appear to be applied consistently and correctly. The CI configuration is also updated to install Zig, which is a new build dependency. Overall, this is a great improvement. I have one minor suggestion regarding a pre-existing issue in pnpm-workspace.yaml.
| overrides: | ||
| dns-over-http-resolver": ^2.1.1 | ||
| elliptic: '>=6.6.1' | ||
| loupe": ^2.3.6 | ||
| nan": ^2.19.0 |
There was a problem hiding this comment.
It seems there's a pre-existing formatting issue in the overrides section. Several keys have a trailing double quote, which makes them invalid YAML keys. While this PR only moves this block, it would be a good opportunity to fix it. The keys should be properly quoted.
overrides:
"dns-over-http-resolver": ^2.1.1
elliptic: '>=6.6.1'
"loupe": ^2.3.6
"nan": ^2.19.0
Motivation
Replacing usages of
blstwith alodestar-zbindings toblst-zfor usage in the beacon node.