Repository navigation
perf(beacon): verify attestations against committee key sums - #742
MegaRedHand wants to merge 6 commits into
Conversation
An attestation aggregate usually covers most of its committee, so its verification key is cheaper to derive as the committee's key sum minus the non-attesters than as the sum of the attesters. The result is the same group element, so the pairing check is unchanged; only the number of point additions drops. Each resident shuffling in the committee cache gets a second slot for its committee key sums, so eviction and head pinning stay in one place. Block import only looks the sums up: building them costs a pass over the whole active set, which is more than they save for committees attested once. The epoch precompute worker builds them instead, after it has handed the advanced state to the actor, so the epoch's first block never waits. Sums are keyed by the shuffling key. That is sound because the registry is append-only, a pubkey is never reassigned, and a validator active at epoch E was activated at least MAX_SEED_LOOKAHEAD + 1 epochs earlier, so it is already in the history the decision root commits to. Nothing in block processing calls the new key builder yet.
Brings in the per-block signature batch and the batched key sums. The two parts each added a point-sum helper. The batched one now lives in bls.rs as a single projective sum, with sum_validated_points a thin wrapper that converts to affine once, so committee key sums (which add and subtract projective points) do not pay a field inversion per conversion. The committee-sum module uses that helper instead of its own, and AggregatePubkey::verify shares its pairing path with the new SignatureSet::from_aggregate.
Electra, fulu and gloas process_attestation now build the attestation's signature set from attesters_aggregate_pubkey, which derives the key as the warmed committee sum minus the non-attesters when most of the committee attested, and sums the attesters otherwise. The key is the same group element either way, so the verdict is unchanged; only the point additions differ. Import still never builds the sums: with them cold, the builder sums the attesters directly. The indexed attestation is still built for the participation flags, and the structural checks on it (non-empty, sorted and unique, gloas's length bound) and the push label are the ones the indexed path made. The spec runners use a fresh committee cache, which only ever runs the cold branch, so electra-and-later attestation cases and every block run now run a second time with the sums warmed. Both runs must reach the same verdict and post-state.
The replay drives import_block directly with no tick loop, so the epoch precompute worker never runs and committee key sums stay cold; the committee-sum saving is invisible in a replay although a live node gets it. The flag warms each epoch's sums before its first block, outside every measured span, so the warm-up is in no phase and no wall_seconds. It is recorded in the report params and the human header.
perf(beacon): committee key sums, warmed off the import path An attestation aggregate usually covers most of its committee, so its verification key is cheaper to derive as the committee's key sum minus the non-attesters than as the sum of the attesters. The result is the same group element, so the pairing check is unchanged; only the number of point additions drops. Each resident shuffling in the committee cache gets a second slot for its committee key sums, so eviction and head pinning stay in one place. Block import only looks the sums up: building them costs a pass over the whole active set, which is more than they save for committees attested once. The epoch precompute worker builds them instead, after it has handed the advanced state to the actor, so the epoch's first block never waits. Sums are keyed by the shuffling key. That is sound because the registry is append-only, a pubkey is never reassigned, and a validator active at epoch E was activated at least MAX_SEED_LOOKAHEAD + 1 epochs earlier, so it is already in the history the decision root commits to. Nothing in block processing calls the new key builder yet. perf(beacon): verify attestations against committee key sums Electra, fulu and gloas process_attestation now build the attestation's signature set from attesters_aggregate_pubkey, which derives the key as the warmed committee sum minus the non-attesters when most of the committee attested, and sums the attesters otherwise. The key is the same group element either way, so the verdict is unchanged; only the point additions differ. Import still never builds the sums: with them cold, the builder sums the attesters directly. The indexed attestation is still built for the participation flags, and the structural checks on it (non-empty, sorted and unique, gloas's length bound) and the push label are the ones the indexed path made. The spec runners use a fresh committee cache, which only ever runs the cold branch, so electra-and-later attestation cases and every block run now run a second time with the sums warmed. Both runs must reach the same verdict and post-state. feat(benchmark): add --warm-committee-sums to the import replay The replay drives import_block directly with no tick loop, so the epoch precompute worker never runs and committee key sums stay cold; the committee-sum saving is invisible in a replay although a live node gets it. The flag warms each epoch's sums before its first block, outside every measured span, so the warm-up is in no phase and no wall_seconds. It is recorded in the report params and the human header. Cherry-picked from perf/committee-key-sums at 508c43e.
# Conflicts: # crates/blockchain/state_transition/tests/beacon_spec/operations.rs
…arm missed epochs With the sums cold, every committee was summed on its own, and each sum of a few hundred keys paid blst's pool dispatch separately where the plain path made one sum over all attesters: a cold multi-committee aggregate got slower than before this change. Every directly summed committee now goes into one sum, so the cold path is that plain sum again. Warming only ran for the epoch a precompute worker advanced into. The worker now also warms the previous epoch (after a restart mid-epoch, or a boundary whose worker did not run), and a boundary state someone else already cached (a gloas bid for the epoch's first slot) gets a warm-only task instead of no warm-up at all. Pre-electra states skip it, since nothing reads the sums there, and the warm-up histogram now times the sums alone, only when they are built.
🤖 Claude Code ReviewReview of PR 742: warm committee key sums and use them in electra and gloas attestation verificationI read the diff but did not build it or run the tests, so everything below comes from reading the code. Overall: the design is sound and carefully argued. Warming happens off the import path. The cold path stays a single flat sum. The sum-minus-absentees result is the same group element as summing the attesters, so the pairing check can't tell them apart. The correctness argument for sharing sums on the Checked and fine
Minor points
Approve, with point 1 worth confirming before merge. Automated review by Claude (Anthropic) · sonnet · custom prompt |
🤖 Codex Code ReviewOverall this looks solid. I didn’t spot a consensus/safety bug in the changed logic, and the fallback behavior is conservative: when precomputed sums are missing or unusable, the code falls back to direct key summation rather than accepting anything it shouldn’t.
Looks good to me aside from the small blocking-pool/perf concern in Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Kimi Code ReviewI'll review this PR which introduces committee key sum caching to optimize attestation verification in Electra and later forks. Let me analyze the changes systematically. Overall AssessmentThis is a well-structured optimization PR that caches BLS public key sums per committee to speed up attestation verification. The core insight is sound: for committees with high attestation rates, subtracting absentee keys from a precomputed sum is cheaper than summing all attesters from scratch. Critical Issues1. Potential Race Condition in
|
| Priority | Item | File | Line |
|---|---|---|---|
| Critical | Wrong parameter type in warm_committee_sums calls |
replay.rs |
133, 150 |
| High | Overflow risk in majority check | electra.rs |
600 |
| Medium | Typo in comment | epoch_precompute.rs |
200 |
| Low | Metrics bucket upper bound | metrics.rs |
151 |
| Low | decide re-evaluation clarity |
epoch_precompute.rs |
198-210 |
The core cryptography and consensus logic is well-designed and correct. The main blocker is the compilation error in replay.rs.
Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt
…review fixes perf(beacon): keep cold committee sums at the plain path's cost and warm missed epochs With the sums cold, every committee was summed on its own, and each sum of a few hundred keys paid blst's pool dispatch separately where the plain path made one sum over all attesters: a cold multi-committee aggregate got slower than before this change. Every directly summed committee now goes into one sum, so the cold path is that plain sum again. Warming only ran for the epoch a precompute worker advanced into. The worker now also warms the previous epoch (after a restart mid-epoch, or a boundary whose worker did not run), and a boundary state someone else already cached (a gloas bid for the epoch's first slot) gets a warm-only task instead of no warm-up at all. Pre-electra states skip it, since nothing reads the sums there, and the warm-up histogram now times the sums alone, only when they are built. Updates nightly's #742 from perf/committee-key-sums at 508c43e to 562b369.
Block import benchmark (offline replay of mainnet blocks, steady state)
Open question: is the cold path expected to be slower than #738's direct sum? If #742 merges together with #743 the gap does not show; if #742 lands alone, it is worth a look. Method
|
Stack 3/4 of the BLS verification work: (a) batched key sums -> (b) per-block signature batch -> (c) committee key sums -> (d) index-keyed pubkey cache. Based on (b).
An attestation aggregate usually covers most of its committee, and its verification key is the sum of the attesters' keys. This PR precomputes each committee's key sum once per shuffling, off the import path, and derives an attestation's key as
committee_sum - sum(non-attesters)when more than half the committee attested, and assum(attesters)otherwise. The group element is the same either way, so the pairing check is unchanged; only the number of point additions differs (absentees instead of attesters).What it changes on top of the signature batch
ethlambda-cryptobls/committee_sums.rs:AggregatePubkey(sum,minus,plus,verify) andCommitteeKeySums. Negation uses blst's safesub_aggregate, so there is nounsafe.SignatureSet::from_aggregate(key, message, signature): builds a set from an already-summed key (Nonewhen the signature failssig_validate; one affine conversion of the key).AggregatePubkey::verifydelegates to it, so both checks share one path, and the batch's identity-key guard covers it.bls.rs(sum_validated_projective); the batched-key-sum PR'ssum_validated_pointsis now a thin wrapper that converts to affine once. Committee sums add and subtract projective points, so building them on the affine helper would cost an extra field inversion per committee, about 512 per block.ethlambda-storage: each resident shuffling gets a secondOnceLock<Arc<CommitteeKeySums>>beside its committees, so eviction and head pinning stay in one place and the sums die with their shuffling.resident_key_sumsis lookup only;get_or_init_key_sumsbuilds into a resident entry and never holds the entry-list lock while building.ethlambda-state-transitionCommitteeCacheExt::key_sums(lookup only: import never builds) andwarm_key_sums(oneiter_validators()pass, then rayon across committees). A committee with an invalid member key gets no sum and falls back to the direct path.attesters_aggregate_pubkey(helpers/electra.rs), with the majority rule, usable by gloas through the sameis_setclosure asattesting_indices_from_committee_bits. Committees derived from their sums are added one by one; the attesters of every committee summed directly go into one sum. With cold sums that is the single sum over every attester that perf(beacon): verify a block's signatures in one random-weight batch #738 makes, at its cost: one sum per committee would pay blst's pool dispatch once per committee for a few hundred keys each, which made the cold path slower than perf(beacon): verify a block's signatures in one random-weight batch #738's.process_attestationbuild theirSignatureSetfrom it (electra_attestation_signature_set,gloas_attestation_signature_set). The indexed attestation is still built, for the participation flags, and the same structural checks are made on it (non-empty, sorted and unique; gloas'sMAX_VALIDATORS_PER_SLOTbound). Same signing root, same push label. Phase0 through deneb are unchanged.ethlambda-blockchain(epoch_precompute.rs): the precomputed state is sent to the actor first, then the sums are warmed in the same blocking task, so the epoch's first block never waits for them. The task warms the epoch's sums and the previous epoch's (for the attestations its blocks carry from it), so a restart mid-epoch or a boundary whose worker did not run is covered at the next one. When the boundary state is already cached (a gloas bid for the epoch's first slot advances it), no worker runs, so a warm-only task runs from that state instead. Warming is skipped before electra, whose attestations never read the sums. New histogramlean_beacon_committee_key_sums_warm_seconds(indocs/metrics.md), recorded only when sums are actually built and covering the sums alone.Soundness of keying sums by
ShufflingKeyThe sums are filed under (epoch, decision root), so two states with one key must agree on each member's public key as well as on who the members are. Checked against this repo's spec code:
process_deposit, electra's pending-deposit path); a validator's index and pubkey are fixed at append and no code assignspubkeyafterwards.activation_epoch = FAR_FUTURE_EPOCH. Onlyprocess_registry_updates(epoch processing at epoch N) assigns one,compute_activation_exit_epoch(N) = N + 1 + MAX_SEED_LOOKAHEAD.The argument is in the
CommitteeCacheExt::key_sumsdoc comment.Measured
On a heavily loaded M3, blst 0.3.16, single-threaded, block shape of 11 signature sets and 33k signers:
The single-threaded ratio overstates the wall-time gain on a multi-core host, where #738's one large sum already runs on blst's pool. In CPU time, profiling a Hoodi block replay on a Ryzen 9700X (the whole stack, warm sums) puts key summation at 12.3 ms CPU per block before the stack and 1.5 ms after.
Behavior notes
current_sync_committee.pubkeys, which has no committee sums.Tests
New tests:
sum/minus/plusagainst blst's direct sum (including duplicate keys, andminusover 40 random masks of a 64-key committee);verifyagainstbls::fast_aggregate_verifywith real signatures; the identity aggregate rejected; the empty and invalid-key cases.key_sumsnever builds or inserts; sums are built once into a resident shuffling only; sums are evicted with their shuffling.operations/*/attestationcase and every block run (sanity,finality,random) now runs twice, cold and afterwarm_key_sums(previous, current and next epoch of the state each block applies to); both runs must reach the same verdict and post-state.Run (
--profile release-fast):cargo test -p ethlambda-crypto --lib: 35 passed, 14 ignoredcargo test -p ethlambda-storage --lib: 213 passed, 1 ignoredcargo test -p ethlambda-state-transition --lib --features beacon-spec-tests: 658 passedbeacon_spec_tests -- /attestation/: 356 passed;-- sanity/: 595 passed;-- random/: 1969 passed, 175 ignoredbeacon_spec_tests: 7493 passed, 977 ignoredcargo fmt --all; clippy-D warnings --all-targetson crypto, storage, state-transition, blockchain: cleanethlambda-blockchain --lib(before the merge): 291 passed, 43 ignoredOn this branch after its review fixes: crypto
--lib40 passed; storage 213; state-transition--lib658; blockchain 291;beacon_spec_testsoperations/2141,sanity/595,finality/41,fork_choice/206,random/1969 (175 ignored),transition/172,sync/6 passed.Rerun after the review fixes, on the stack's tip (all four PRs,
--profile release-fast):ethlambda-crypto --lib47 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; fullbeacon_spec_tests7493 passed, 977 ignored, 0 failed;make lintclean, also on a trial merge of currentmain.