Repository navigation
perf(beacon): run epoch steps 1-3 over one registry scan - #632
MegaRedHand wants to merge 6 commits into
Conversation
Justification, inactivity updates and rewards read the tree-backed registry through ~19 full scans and ~21 validator(i) descents per active validator, and the per-block pulled-up tip repeated step 1 on every imported block. helpers::participation walks validators().iter() once, zipped with the flat participation slices, into per-validator flags, effective balances and the balance totals (EpochSummary). Steps 1-3 then run over flat data: scores are updated in place, and only balances that changed are written back, per component in spec order. Justification (and so the pulled-up tip) needs only ParticipationTotals, one scan with no allocation. The public step functions and the flag/inactivity delta helpers keep their signatures for the fixtures and become thin wrappers. Effective balance updates zip validators with balances, since balances()[i] is now a tree descent for most validators. The specification-shaped code stays as a reference oracle for tests and debug builds: the driver runs it on a clone (registries up to 4096) and asserts balances, scores, justification bits and checkpoints agree; randomized tests compare every value and error across forks.
The participation equivalence generator could finalize the current epoch, which no reachable state does, so get_finality_delay underflowed. The suite runs under release-fast, where overflow checks are off and both the fast path and the reference wrapped identically; the minimal-preset CI job runs a dev build and panicked.
get_finality_delay subtracted the finalized epoch from the previous epoch unchecked, so a finalized checkpoint past the previous epoch panicked in debug builds and wrapped into a permanent inactivity leak in release. The specification treats a uint64 underflow as an invalid state transition, and Lighthouse and Grandine reject it the same way, so the delay and the leak check now return ArithmeticOverflow, as the inactivity penalty's own checked product already does. No chain reaches that state, since justification only finalizes an epoch behind the current one.
…iation Now that an underflowing finality delay is an error rather than a wrap, the point where the leak flag is read decides which states fail. The altair fast path reads it once per step where the specification reads it per validator, so a crafted state with the finalized epoch past the previous one fails here and can pass there.
…erf/beacon-epoch-participation-b
🤖 Claude Code ReviewReview: perf(beacon): run epoch steps 1-3 over one registry scan (PR 632)I read the diff through Overall: The change looks correct and I found no consensus-affecting bugs. It reproduces the spec arithmetic and its order of operations, and the oracle-on-clone assertion in debug builds is a strong safety net. Correctness checks that hold
Notes and nits (non-blocking)
The perf numbers (about 20% less Automated review by Claude (Anthropic) · sonnet · custom prompt |
🤖 Codex Code ReviewLooks good overall: the refactor is performance-motivated, keeps a spec-shaped reference path for debug/test comparison, and I did not spot an obvious consensus-breaking bug in fork choice / FFG / attestation accounting from the touched code. Findings
Nits / Follow-ups
Consensus / Security assessment
Validation note
Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Kimi Code ReviewI'll review this PR which introduces a one-pass participation accounting optimization for Altair-and-later epoch processing. This is a significant performance improvement with consensus-critical code. High-Level AssessmentThis is a well-structured optimization that replaces O(n²) epoch processing with O(n) by scanning the validator registry once. The approach of keeping a reference implementation for testing is excellent. However, I've identified several issues ranging from minor correctness concerns to potential consensus bugs. Critical Issues1. Missing
|
One of three alternative implementations of the same follow-up to lambdaclass/ethlambda_private#37 (epoch participation without per-index registry reads): (a) #630, (b) this PR, (c) #633. They are meant to be compared, and at most one of (a) and (b) should merge; (c) is stacked on this one.
Motivation
Since lambdaclass/ethlambda_private#37 the registry is a persistent Merkle tree, so
state.validator(i)is a tree descent rather than an array index, whilevalidators().iter()walks leaf slices.Epoch steps 1-3 (justification, inactivity updates, rewards) read the registry through about 19 full scans and about 21 descents per active validator. The pulled-up-tip check in fork choice repeats justification on every imported block.
Change: option (b), one summary pass, then flat passes
helpers::participation:EpochSummary: one walk ofvalidators().iter(), zipped with the flat participation lists, into per-validator flags, effective balances and the balance totals.ParticipationTotals: the same scan without allocation. This is all justification (and so the per-block pulled-up tip) needs.RewardContextandValidatorDeltas: today's reward arithmetic per validator, in today's operation order.process_participation_stepsreplaces the three step calls in the altair, capella and electra drivers. It builds the summary once, then:get_flag_index_deltas/get_inactivity_penalty_deltaskeep their signatures, since the fixtures call them one at a time. They become thin wrappers that build what they need.balances().iter(). Once only changed balances are written,balances()[i]is a tree descent for most validators.get_finality_delayandis_in_inactivity_leaknow returnResult: a finalized epoch past the previous epoch is anArithmeticOverflowrather than a debug panic or a release wrap into a permanent leak. The spec calls thatuint64underflow an invalid transition, and Lighthouse and Grandine reject it the same way (Teku saturates to zero; Prysm and Nimbus wrap).RewardContext::newpasses the error up.docs/beacon_stf.md: a short note in "Registry and balances".docs/spec_deviations.md: from altair on, the fast path reads the leak flag once per step, where the spec reads it per validator. So a crafted state with the finalized epoch past the previous one fails here even where the spec never reaches the check. No chain reaches that state, since justification only finalizes an epoch behind the current one.Testing
cfg(any(test, debug_assertions)).process_participation_stepsruns the reference on a clone for registries up to 4096. It asserts equal balances, scores, justification bits and checkpoints, and that both paths error or neither does.release-fasthas debug assertions on, so this runs on every fixture.Test minimal presetjob, which uses the dev profile with overflow checks on.release-fastturns them off, which is how the underflow above went unnoticed under mainnet.-D warningsand theethlambda-state-transitionlib tests pass.epoch_processing,rewards,sanity,finality,random,fork_choiceandtransitionpass on both presets. That run predates the finality-delay change; CI's beacon spec jobs cover it.Benchmark (all three options)
ethlambda benchmark import replayon ethlambda-5:cargo build --release, mainnet corpus of 128 blocks (slots 15279073..=15279200, 4 epoch-start blocks), legs interleavedbase a b cfor two rounds. Base against base: 0.996. Every leg imported all 128 blocks.stf, epoch-start block (mean of 4)stfp50, other blocksstfvs base (ratio of totals)get_headp50get_headrises asstffalls, although no option touches the head computation. This is probably contention with the background state writer, as seen in earlier import benchmarks; it is not verified. Block wall time still falls.Moved
beacon-chain-integrationlives on this repo.beacon-chain-integration@c79fabd5, merged into the branch.