Conversation
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 revamps Lodestar's BLS signature verification logic by transitioning from a JavaScript worker-thread based system to a native NAPI BLS verifier. The motivation behind this change is to overcome the overhead associated with JS worker-thread management, such as data serialization and job coordination, and to leverage native code for improved performance and more predictable characteristics. The new architecture delegates verification tasks directly to native code, utilizing the libuv thread pool for asynchronous concurrency, which is expected to reduce per-verification overhead and enhance overall efficiency, particularly under high-load conditions like gossip processing. 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
|
|
@nazarhussain can you re-review :) I moved this from #8900 to here |
![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
The pull request introduces a native BLS verifier using NAPI and libuv, replacing the previous JavaScript worker-thread pool. This is a significant architectural improvement that should reduce serialization overhead and improve verification throughput. The new BlsVerifier class handles synchronous, asynchronous, and batched verification modes with built-in backpressure. However, there are some issues regarding error handling during shutdown, missing state checks in the batching logic, and potential compatibility issues with older Node.js 18 versions in the thread pool sizing script.
| } catch { | ||
| // A signature could be malformed, causing a deserialization error | ||
| return false; | ||
| } finally { |
There was a problem hiding this comment.
The catch block in verifyAsync catches all errors, including the Error("BlsVerifier closing") thrown by trackJob when the verifier is shutting down. This causes the verifier to return false (indicating an invalid signature) instead of propagating the closure error. This can lead to incorrect behavior in callers, such as blacklisting valid blocks or attestations during node shutdown. It should distinguish between verification failures and system errors.
|
|
||
| /** Run a native async job, waiting for a slot if at capacity */ | ||
| private async trackJob<T>(fn: () => Promise<T>): Promise<T> { | ||
| if (this.inflightJobs >= this.maxInflightJobs) { |
There was a problem hiding this comment.
The trackJob method should check if the verifier is closed at the very beginning. Currently, it only checks this.closed after waiting for a slot in the queue. If there are available slots, it proceeds to execute the job even if close() has been called.
private async trackJob<T>(fn: () => Promise<T>): Promise<T> {
if (this.closed) {
throw Error("BlsVerifier closing");
}
if (this.inflightJobs >= this.maxInflightJobs) {| } | ||
|
|
||
| /** Enqueue a batchable job into the buffer */ | ||
| private enqueueBatchable(job: PendingJob, priority: boolean): void { |
There was a problem hiding this comment.
The enqueueBatchable method does not check if the verifier is closed. This allows new batchable jobs to be accepted and buffered even after close() has been called, potentially leading to leaked promises or delayed rejections.
private enqueueBatchable(job: PendingJob, priority: boolean): void {
if (this.closed) {
job.reject(Error("BlsVerifier closing"));
return;
}
if (!this.buffer) {| // Try aggregate verification first (1 native job) | ||
| const isAllValid = await this.trackJob(() => | ||
| blsBatch.asyncVerifySameMessage( | ||
| sets.map((s) => ({index: s.index, signature: s.signature})), |
There was a problem hiding this comment.
The mapping here is redundant as the sets array already contains objects with the required {index, signature} shape. Removing this mapping avoids unnecessary allocations, especially for large batches of attestations.
sets,| // read once by libuv before the first async I/O, so it must be set at the earliest | ||
| // entry point. Respect any explicit user override. | ||
|
|
||
| import {availableParallelism} from "node:os"; |
There was a problem hiding this comment.
availableParallelism was introduced in Node.js v18.14.0. Since Lodestar supports Node.js 18.0.0+, this import will cause a crash on older Node.js 18 versions. A fallback to os.cpus().length or a check for the existence of the function should be added.
| * - Backpressure via `canAcceptWork()` using `inflightJobs` counter against `maxInflightJobs`. | ||
| */ | ||
| export class BlsVerifier implements IBlsVerifier { | ||
| private maxInflightJobs = 40_000; |
There was a problem hiding this comment.
The maxInflightJobs limit is hardcoded to 40,000. While this might be a safe upper bound for memory, it is significantly higher than the previous limit (512) and is not configurable via CLI or constructor options, contrary to what the PR description suggests. Consider making this configurable or explaining the choice of 40,000.
spiral-ladder
force-pushed
the
bing/blst-z
branch
2 times, most recently
from
52d9206 to
740c395
Compare
spiral-ladder
force-pushed
the
cayman/bls-verifier
branch
from
ae921c1 to
ec98851
Compare
spiral-ladder
force-pushed
the
cayman/bls-verifier
branch
from
459fcf2 to
b0204dc
Compare
spiral-ladder
force-pushed
the
bing/blst-z
branch
from
1f63139 to
df75779
Compare
2 participants
Motivation
blstandpubkeyswithlodestar-z#8900 , we can take advantage of this to revamp our bls batch verifier logicDescription
This PR replaces Lodestar's previous JavaScript-based multi-threaded BLS signature verification system with a new native NAPI BLS verifier powered by
@chainsafe/lodestar-z/bls-batch. The old architecture used a custom JS worker-thread pool to parallelize BLS verification across multiple threads. The new architecture eliminates the JS worker pool entirely and instead delegates verification work directly to native (Rust/C++) code via NAPI, using the libuv thread pool for async concurrency.Key Changes
New:
BlsVerifierclass (packages/beacon-node/src/chain/bls/blsVerifier.ts)A single unified verifier that handles all BLS verification modes:
asyncVerifyIndexed,asyncVerifyAggregate,asyncVerifySingle).ISignatureSetarrays into three native-friendly buckets (indexed,aggregate,single) for optimized native codepaths. Indexed sets resolve public keys natively from a shared pubkey cache by validator index.New: libuv thread pool sizing (
packages/cli/src/setUvThreadPool.ts)Automatically sizes
UV_THREADPOOL_SIZEto match available CPU cores (clamped to 4–32), since the native verifier relies on libuv threads instead of JS workers. Set at the earliest CLI entry point before any async I/O.TODO this doesn't seem to work, we need a better setup here
New: Grafana dashboard (
dashboards/lodestar_bls_thread_pool.json)A monitoring dashboard for the new BLS verifier with metrics for inflight jobs, batch sizes, flush durations, buffer wait times, retry rates, and per-sig-set timing.
Removed
It's unfortunate, but this design requires breaking the old interface, and it doesn't seem worth it to maintain both anymore.
multithread/worker pool subsystem (6 files:index.ts,jobItem.ts,poolSize.ts,types.ts,utils.ts,worker.ts).singleThread.tsfallback verifier.maybeBatch.tsandutils.tshelpers that supported the old architecture.--chain.blsVerifyAllMultiThreadCLI option (no longer needed — all async verification goes through native calls).Updated
IBlsVerifierinterface — streamlined; addedverifySignatureSetsSameMessagefor efficient same-message batch verification (used in attestation validation).chain.ts— constructs the newBlsVerifierdirectly instead of the old multi-thread pool.This is currently deployed on feat3