[MegaRedHand]

Stack 1/4 of the BLS verification work: (a) batched key sums -> (b) per-block signature batch -> (c) committee key sums -> (d) index-keyed pubkey cache.

What changed

crates/common/crypto/src/bls.rs only. A private helper, sum_validated_points, sums already-validated public keys through blst's safe MultiPoint::add for [PublicKey], which calls blst_p1s_add: a batched affine addition with one shared field inversion. fast_aggregate_verify (now ending in Signature::fast_aggregate_verify_pre_aggregated) and eth_aggregate_pubkeys use it instead of AggregatePublicKey::aggregate. No unsafe, no signature changes. aggregate_verify (distinct messages) never sums keys, so it is untouched.

Notes on the helper:

• MultiPoint::add indexes the first element, so the empty slice is guarded (None). Both callers already reject empty input before.
• From 384 points up, on a host with two or more CPUs, blst splits the work over its own thread pool (separate from rayon; the same pool its pairing already ran on); below that, or on one CPU, it runs on the calling thread. The result is the same either way.
• Equal points (sync committees are drawn with replacement, so P + P occurs) give the same element as the sequential sum.

Why

Measured on blst 0.3.16, block shape of 11 signature sets and 33k signers, single-threaded, on a heavily loaded M3 (ratios only; absolutes to be taken on a quiet x86 host):

Phase per block	Ratio
key aggregation, sequential (before)	1.4-2.5x the pairing
key aggregation, blst_p1s_add (this PR)	1.6-2.3x faster than sequential

Behavior

No change: every input accepted before is accepted, every input rejected before is rejected, including the identity aggregate: P and -P are both valid keys (flip the sort flag in the compressed encoding; whoever holds sk can sign -P's deposit with -sk), so a registry can hold both and an aggregate can name both. eth_aggregate_pubkeys([P, -P]) returns the identity's bytes on both paths, and fast_aggregate_verify stays false because blst refuses an infinite key at pairing time.

Tests

New unit tests compare sum_validated_points with AggregatePublicKey::aggregate: empty input, sizes 1, 2, 3, 17, 383, 384, 385, 767 and 2000, P + P, and runs with scattered repeats at 10, 400 and 1000 keys; fast_aggregate_verify with a repeated signer verifies (and fails with the key listed once); eth_aggregate_pubkeys equals the sequential sum with repeats; keys that cancel (adjacent P, -P pairs and two halves that cancel each other, at 8, 200, 400 and 1100 pairs) match the sequential sum, and [P, -P] aggregates to the identity and never verifies, not even against the identity signature.

Run:

• cargo test -p ethlambda-crypto --profile release-fast --lib: 17 passed, 14 ignored
• cargo test -p ethlambda-state-transition --profile release-fast --lib --features beacon-spec-tests (BLS fixture vectors): 645 passed
• ... --test beacon_spec_tests --features beacon-spec-tests -- /sync_aggregate/: 182 passed
• ... -- /attestation/: 356 passed
• ... -- sanity/: 595 passed
• cargo fmt --all, cargo clippy -p ethlambda-crypto --all-targets -- -D warnings: clean

Rerun after the review fixes, on the stack's tip (all four PRs, --profile release-fast): ethlambda-crypto --lib 47 passed, 14 ignored; ethlambda-state-transition --lib --features beacon-spec-tests (with the BLS fixture vectors) 658 passed; storage 213, blockchain 291, p2p 362, rpc 335 passed; full beacon_spec_tests 7493 passed, 977 ignored, 0 failed; make lint clean, also on a trial merge of current main.

[github-actions]

🤖 Claude Code Review

Review of PR 737: batched public-key sum in crates/common/crypto/src/bls.rs

I read the diff only. I did not build it or run the tests.

The change looks correct and is well tested. I found no blocking issues.

• Correctness:
  • sum_validated_points guards the empty slice, which MultiPoint::add would otherwise panic on, and returns None for it.
  • eth_aggregate_pubkeys keeps its len(pubkeys) > 0 SpecAssert through that None.
  • fast_aggregate_verify can only reach the None branch with empty input, and validated_pubkeys already rejects that case.
• Safety of skipping the group check: every point comes from validated_pubkeys, which runs key_validate, so passing false for the group check is sound. The signature is validated earlier in the function.
• Edge cases:
  • Repeated keys: the doubling case is covered, and sync committees drawn with replacement hit it.
  • Cancelling keys (P and -P): the sum is the identity, and verification fails. blst's pre-aggregated verify rejects an infinity public key.
  • eth_aggregate_pubkeys returns the identity encoding for that cancelling case, matching the spec pseudocode.
  • These cases are covered by fast_aggregate_verify_with_a_repeated_signer_still_verifies, a_key_and_its_negation_sum_to_the_identity_and_never_verify and the_batched_sum_matches_the_sequential_one_when_keys_cancel.
• Tests: the batched sum is compared with the sequential sum at sizes on both sides of blst's 384-point threading threshold, which is the right boundary to test.
• Performance: one shared field inversion across the whole slice replaces a projective addition per key. The doc comment also notes that blst's own thread pool is separate from rayon's, so no new threads are added.

Nits, optional

• The comment "pubkeys is non-empty, so there is always a sum" in fast_aggregate_verify could say the non-empty guarantee comes from validated_pubkeys. As written, a reader has to check that.
• The the_batched_sum_* tests build up to 2000 keys, which is fine for the speed of blst. If fresh_pubkey is slow, the 2000 and 1100 cases could be trimmed.

Approve.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt

[github-actions]

🤖 Codex Code Review

Looks good overall. The change is narrowly scoped to BLS pubkey aggregation, improves the hot path for attestation verification, and the added tests cover the main edge cases that matter for consensus safety: repeated keys, cancellation, and identity results.

• No correctness issue jumped out in fast_aggregate_verify at crates/common/crypto/src/bls.rs:399. Replacing per-call AggregatePublicKey::aggregate with MultiPoint::add preserves the same group sum for validated keys, and the empty-slice guard in crates/common/crypto/src/bls.rs:289 avoids the only obvious panic hazard.
• Security-wise, this does not appear to weaken subgroup or non-identity checks. Inputs still flow through Signature::sig_validate and cached PublicKey::key_validate before verification at crates/common/crypto/src/bls.rs:407 and crates/common/crypto/src/bls.rs:410.
• Performance impact should be positive on the attestation hot path. Avoiding Vec<&PublicKey> allocation plus using batched addition in crates/common/crypto/src/bls.rs:418 is a sensible optimization.
• The new tests in crates/common/crypto/src/bls.rs:653, crates/common/crypto/src/bls.rs:682, and crates/common/crypto/src/bls.rs:702 are strong and directly target consensus-relevant corner cases.

One minor maintainability note:

• sum_validated_points at crates/common/crypto/src/bls.rs:289 returns Option<PublicKey>, but all call sites already enforce non-empty input. Returning PublicKey and keeping the emptiness check at the boundary callers might make the invariant clearer; not a blocker, just a readability tradeoff.

I don’t see fork-choice, justification/finalization, state transition, XMSS, or SSZ concerns in this PR since the diff is isolated to BLS aggregation logic.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

[github-actions]

🤖 Kimi Code Review

I'll review this PR which optimizes BLS public key aggregation using blst's batched MultiPoint::add instead of sequential AggregatePublicKey::aggregate.

Overall Assessment

This is a well-crafted performance optimization with excellent test coverage. The core mathematical reasoning is sound, and the safety considerations around point validation and edge cases are properly handled.

Issues Found

1. Critical: fast_aggregate_verify spec compliance for empty pubkey list

File: crates/common/crypto/src/bls.rs, Lines 408-415

let Some(points) = validated_pubkeys(pubkeys) else {
    return false;
};
// `pubkeys` is non-empty, so there is always a sum.
let Some(aggregated) = sum_validated_points(&points) else {
    return false;
};

The comment claims pubkeys is non-empty, but this isn't guaranteed. validated_pubkeys returns Some(empty_vec) for an empty input, not None:

fn validated_pubkeys(pubkeys: &[BlsPubkey]) -> Option<Vec<PublicKey>> {
    // ...
    let mut points = Vec::with_capacity(pubkeys.len());
    // ...
    points.into_iter().collect()  // Returns Some([]) for empty input
}

Impact: fast_aggregate_verify(&[], message, &signature) will hit the sum_validated_points empty guard and return false, which is correct per spec (should fail). However, the code path is misleading—the comment is wrong, and we're relying on the downstream guard rather than explicit validation.

Fix: Either add an explicit is_empty() check with a clear comment, or fix the comment to accurately describe the behavior. The current code happens to be correct but for the wrong reason.

2. Minor: sum_validated_points naming inconsistency with behavior

File: crates/common/crypto/src/bls.rs, Lines 287-288

The function returns Option<PublicKey> where None means "empty input." But in eth_aggregate_pubkeys (line 437), the error message says "len(pubkeys) > 0"—this conflates "empty input" with "validation failed."

Suggestion: Consider making sum_validated_points return Result<PublicKey, &'static str> or similar to distinguish:

• Empty input (caller decides if this is an error)
• Other potential future failures

For now, this is acceptable since the only failure mode is empty input, but it's slightly fragile.

3. Performance: Unnecessary allocation in fast_aggregate_verify

File: crates/common/crypto/src/bls.rs, Lines 408-415

The old code collected point_refs: Vec<&PublicKey>; the new code still allocates points: Vec<PublicKey> in validated_pubkeys. The optimization is in the aggregation step, but we're still allocating the full vector of public keys.

Note: This is pre-existing and not introduced by this PR. The PR actually removes the point_refs allocation, which is good. No action needed, but worth noting the validated_pubkeys allocation remains.

4. Test coverage gap: fast_aggregate_verify with empty pubkeys

File: crates/common/crypto/src/bls.rs, Tests

No test verifies fast_aggregate_verify(&[], message, &signature) returns false. Given the subtlety in Point 1, add:

#[test]
fn fast_aggregate_verify_empty_pubkeys_fails() {
    let message = H256([1u8; 32]);
    let signature = BlsSignature(G2_POINT_AT_INFINITY);
    assert!(!fast_aggregate_verify(&[], message, &signature));
}

5. Documentation: MultiPoint trait import could be more specific

File: crates/common/crypto/src/bls.rs, Line 100

use blst::{BLST_ERROR, MultiPoint};

The MultiPoint trait is used only for .add() on &[PublicKey]. This is fine, but consider importing it closer to usage or using a more explicit call like <[PublicKey] as MultiPoint>::add(&points) if there's any ambiguity risk. Currently acceptable.

Positive Observations

1. Excellent test coverage: The tests for doubling, cancellation, threshold boundaries (383/384/385), and repeated signers are thorough and mathematically meaningful.

2. Correct handling of identity element: The negated test confirms that P + (-P) = identity is handled consistently, which is critical for consensus safety.

3. Proper validation gating: The sum_validated_points function correctly requires pre-validated points and skips re-checking, with clear documentation of this safety invariant.

4. Thread pool awareness: The documentation accurately describes blst's internal threading behavior and its interaction with the existing thread pool.

Summary

Severity	Count	Description
Critical	1	Misleading comment in fast_aggregate_verify masks empty-input handling; needs explicit check or corrected comment
Minor	1	sum_validated_points error semantics slightly conflate empty input with other failures
Test gap	1	Missing empty-pubkeys test for fast_aggregate_verify

The core optimization is correct and safe. Address Point 1 before merge—it's a latent maintainability issue that could lead to bugs if validated_pubkeys behavior changes.

---

Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt

[MegaRedHand]

Block import benchmark (offline replay of mainnet blocks, steady state)

leg	STF per block	difference
main 012dc4f	157.7 ms
main + #737	150.0 ms	-7.7 ms (-4.9%), 95% CI [-8.3, -7.2]

• Same in both rounds: -4.8% and -5.0%.
• Wall time per import: 179.3 to 171.6 ms (-4.3%).
• The rest of the BLS stack on top of this: perf(beacon): verify a block's signatures in one random-weight batch #738 -4.1 ms, perf(beacon): verify attestations against committee key sums #742 -1.8 ms, perf(crypto): resolve registry public keys through a validator-index table #743 -2.3 ms. Nightly 0abc099 with everything: 140.0 ms (-11.2% vs main).

Method

• Offline ethlambda benchmark import replay on ethlambda-5 (Ryzen 7 9700X), with the node stopped.
• 3 consecutive mainnet windows fetched from that node: 173 measured blocks (slots 15567200 to 15567385), each window after one unmeasured warm-up epoch. --block-delay 1000.
• One leg per PR: the PR head merged onto main 012dc4f. The BLS PRs (perf(crypto): sum validated public keys through blst's batched affine addition #737, perf(beacon): verify a block's signatures in one random-weight batch #738, perf(beacon): verify attestations against committee key sums #742, perf(crypto): resolve registry public keys through a validator-index table #743) are stacked, so each of those legs includes its parents. --warm-committee-sums on the legs that have the flag (perf(beacon): verify attestations against committee key sums #742, perf(crypto): resolve registry public keys through a validator-index table #743, nightly).
• 2 rounds over all legs, leg order rotated per window. Every run of a window imported the same block roots.
• Numbers are per-block STF means, paired against the reference leg on the same blocks, with a bootstrap 95% CI of the mean difference.
• Steady state: 9 of the 173 blocks are dropped from every leg, because their cost exists only in a fresh process with no gossip. They are the first measured block of each window (about 31k attesters whose keys the process has never validated) and, per epoch, the first block carrying that epoch's attestations (it builds the committee shuffling inside the timed span; a live node's gossip validation builds it first). Including them makes the legs with --warm-committee-sums look about 3 points better than they are, since the flag does both before the timer starts.
• Noise floor: legs whose code the replay cannot exercise move about ±1.5% between rounds.