feat: make lodestar-z BLS opt-in#9652
Draft
wemeetagain wants to merge 128 commits into
Draft
Conversation
## Motivation The lodestar-z pubkey cache introduced in #8900 is a process-global singleton shared across threads without locking. The historical state regen worker reads it (`getIndex` during sync committee cache computation) and writes it (`addPubkey` when replaying deposit blocks) while the main thread does the same. A main-thread `set()` that outgrows the reserved capacity reallocates both native structures and frees the old buffers, so a concurrent read from the worker thread is a use-after-free. The previous `ensureCapacity(validators.length)` left zero headroom, meaning the first deposit after startup triggered exactly this realloc. ## Description - Reserve headroom when populating the cache at startup so it does not realloc during a realistic process lifetime. Sized as 3 months of worst-case registry growth (`MAX_PENDING_DEPOSITS_PER_EPOCH` new validators per epoch), which is over a year at organic rates and beyond the update cadence of any Lodestar node observed on mainnet. On mainnet this is ~324k slots, ~31MB of virtual memory and ~0 resident. ChainSafe/lodestar-z#481 aligns the native growth policy to the same step, so if the reservation is ever exceeded the cache extends by another 3-month window instead of doubling. - Skip no-op rewrites in `EpochCache.addPubkey` when the identical pubkey is already cached at that index. Replayed deposits during historical state regen always hit this path, so the worker thread no longer writes to the shared cache at all. A different pubkey at an existing index still overwrites, preserving existing behavior on forks with conflicting deposits. This is a mitigation, not a fix. The remaining race (a genuinely new main-thread insert concurrent with a worker hashmap probe) can at worst fail a single historical state query. The proper fix is locking around the pubkeys binding in lodestar-z, at which point the headroom becomes a realloc-avoidance nicety. --------- Co-authored-by: matthewkeil <me@matthewkeil.com>
This reverts commit 8c3a0a9.
![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 support for a new Zig-based BLS implementation (lodestar-z) alongside the existing blst library. It abstracts BLS types and operations under a unified interface in @lodestar/state-transition/bls and updates imports across the codebase. Review feedback suggests guarding against undefined process.env in non-Node environments to prevent runtime errors, and avoiding the mutation of imported bindings with Object.assign by using class inheritance instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const BLS_IMPLEMENTATION_ENV = "LODESTAR_BLS_IMPLEMENTATION"; | ||
|
|
||
| export function getBlsImplementation(): BlsImplementation { | ||
| const implementation = typeof process === "undefined" ? undefined : process.env[BLS_IMPLEMENTATION_ENV]; |
Contributor
There was a problem hiding this comment.
In environments where process is polyfilled or defined but process.env is not present (such as certain edge runtimes or bundler configurations), accessing process.env directly will throw a TypeError. Guarding the access to process.env ensures better compatibility and robustness across different environments.
Suggested change
| const implementation = typeof process === "undefined" ? undefined : process.env[BLS_IMPLEMENTATION_ENV]; | |
| const implementation = typeof process === "undefined" || !process.env ? undefined : process.env[BLS_IMPLEMENTATION_ENV]; |
Comment on lines
+121
to
+131
| bls = { | ||
| ...(blsModule as unknown as BlsModule), | ||
| PublicKey: Object.assign(blsModule.PublicKey, { | ||
| COMPRESS_SIZE: blsModule.PUBLIC_KEY_LENGTH_COMPRESSED, | ||
| SERIALIZE_SIZE: blsModule.PUBLIC_KEY_LENGTH_UNCOMPRESSED, | ||
| }), | ||
| Signature: Object.assign(blsModule.Signature, { | ||
| COMPRESS_SIZE: blsModule.SIGNATURE_LENGTH_COMPRESSED, | ||
| SERIALIZE_SIZE: blsModule.SIGNATURE_LENGTH_UNCOMPRESSED, | ||
| }), | ||
| }; |
Contributor
There was a problem hiding this comment.
Using Object.assign directly on the imported blsModule.PublicKey and blsModule.Signature classes mutates the shared module's exports. In strict ESM environments, or when bundled/minified, mutating imported bindings can throw errors or lead to unexpected side effects.\n\nUsing standard class inheritance avoids mutating the imported classes while cleanly inheriting all static and prototype methods.
class PublicKey extends blsModule.PublicKey {\n static COMPRESS_SIZE = blsModule.PUBLIC_KEY_LENGTH_COMPRESSED;\n static SERIALIZE_SIZE = blsModule.PUBLIC_KEY_LENGTH_UNCOMPRESSED;\n }\n class Signature extends blsModule.Signature {\n static COMPRESS_SIZE = blsModule.SIGNATURE_LENGTH_COMPRESSED;\n static SERIALIZE_SIZE = blsModule.SIGNATURE_LENGTH_UNCOMPRESSED;\n }\n bls = {\n ...(blsModule as unknown as BlsModule),\n PublicKey,\n Signature,\n };
Contributor
Performance Report✔️ no performance regression detected Full benchmark results
|
Member
There was a problem hiding this comment.
hm first impression is this is not as ugly as i thought it might be but i still prefer that it seems more straightforward to introduce zig bls directly rather than do this
I guess doing this like u said mainly kicks the can down the road. even doing this we still probably should run zig bls X amount of time to be comfortable with permanently removing blst-ts, the question seems to be what this X is
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
5 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.
Summary
@lodestar/state-transition/blssubpath that selects the BLS implementation at module load time@chainsafe/blstand the original JavaScript pubkey cache as the default--zig-blsCLI flag, applied inapplyPreset.tsbefore the rest of the source tree loadsImpact
Existing users keep the current BLS implementation and cache behavior by default. The lodestar-z implementation can be evaluated explicitly with
--zig-blswithout mixing implementations within one process.Validation
pnpm lintpnpm check-typespnpm test:unit— 3,239 passed, 9 skipped, 1 todo