perf(bls): add cache-aware signature verifier - #562
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f53b807f4
ℹ️ 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".
| if (!pubkeys.state.initialized) return error.PubkeyIndexNotInitialized; | ||
| const indices = try uint32Slice(try set.getNamedProperty("indices")); | ||
| if (indices.len == 0) return error.EmptyIndices; | ||
| if (indices.len > max_indices_per_set) return error.TooManyIndices; |
There was a problem hiding this comment.
Cap aggregate indices across the whole batch
When a caller supplies 256 aggregate sets, each set may reuse a 131,072-entry Uint32Array, so this per-set check still permits over 33 million sequential elliptic-curve additions in PubkeyCache.aggregateIndices() before verification. Because the exported NAPI function is synchronous, malformed or adversarial input can stall the calling Node.js thread for a prolonged period. Track and reject a bounded cumulative index count for the batch, rather than limiting each set independently.
AGENTS.md reference: AGENTS.md:L8-L9
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is good example of why something like #557 may be helpful
There was a problem hiding this comment.
Correction: the measurements are valid for the direct in-process API, but my merge-blocking classification was not. Per the threat model in #557, I needed to trace a least-privileged attacker through a supported Lodestar path, account for protocol bounds and rate/peer controls, and compare against the existing cryptographic workload. This probe instead constructed the Cartesian maximum through arbitrary same-process input. I withdraw this as a blocker.
There was a problem hiding this comment.
why do we have bls_verifier and blst_verifier?
There was a problem hiding this comment.
Its a poor name, but one is essentially shared utilities between the cache-aware verifier and low-level blst bindings, and the other is the cache-aware verifier.
lodekeeper-z
left a comment
There was a problem hiding this comment.
Reviewed the native verifier and companion contract. The core design is coherent and the targeted build/tests pass. I found one additional error-classification issue below. I also confirmed the existing cumulative aggregate-index concern in the open thread: the maximum 256 x 131,072-index call took about 16.8 seconds synchronously in a local probe, so I consider a cumulative bound blocking before merge.
| public_keys[i] = (try resolvePublicKey(set, set_type)) orelse | ||
| return js.Boolean.from(false); | ||
|
|
||
| signatures[i] = (try parseSignature(set)) orelse |
There was a problem hiding this comment.
The early false return makes cache-error behavior order-dependent. A batch whose first set has a malformed signature returns false without resolving later sets, so a later missing validator index is hidden instead of throwing as the API contract says. I reproduced this at this head: [malformed(index 0), valid(index 99)] returned false, while reversing the same two sets threw PubkeyIndexNotFound. That can classify a local cache desynchronization as an invalid remote signature. Please resolve/validate every set's interface and cache state before returning the cryptographic result, and add an ordering regression test.
lodekeeper-z
left a comment
There was a problem hiding this comment.
Approved. Correction to my earlier review: after applying the repository threat model in #557, I withdraw the cumulative aggregate-work bound as a blocking finding. My probe exercised an arbitrary same-process Cartesian maximum; it did not establish a least-privileged hostile production path, account for protocol and Lodestar rate bounds, or show amplification beyond expected cryptographic work. The measured timing remains useful performance data, not a demonstrated merge blocker. The remaining cache-error ordering note is non-blocking API semantics. Native implementation, companion contract, and targeted verification look good.
Summary
Uint32Arrayindices directly from the native public-key cache.Why
Lodestar currently materializes or aggregates public keys in TypeScript before crossing the native boundary. Failed same-message aggregate checks also require another worker-queue pass to identify invalid signatures. This interface keeps validator indices through the native boundary, reuses the native public-key cache, and returns final per-signature results.
Impact
This reduces JavaScript/native serialization and repeated public-key work while providing bounded validation for every new input shape.
Paired with ChainSafe/lodestar#9820.
Written with codex assistance