[MegaRedHand]

Motivation

Since the registry moved onto a persistent tree, state.validator(i) is a tree descent instead of an array read. Two places in get_head pay one per validator:

• compute_weights does a descent per latest vote, then a hash-order probe into the equivocator set and a SipHash map.
• With a proposer boost set, get_proposer_score calls get_total_active_balance: an active-index Vec, then one descent per active index.

At mainnet scale (estimate: about 1M voters and 1M active validators in a 2.4M registry) that is of the order of two million descents per get_head. The get_head regression after the tree PR was measured at 126 ms to 198 ms p50 in the import-replay benchmark; how much of it is these two loops was inferred from the code, not profiled. The import-replay benchmark was not run as part of this work, so this PR carries no timing claims.

Changes

Three commits (plus merges from main), so the PR can be split along the commit boundaries if preferred.

Commit	What
perf(beacon): weigh fork-choice votes from a justified-balances snapshot	JustifiedBalances (types crate): per-validator vote weight (0 if inactive or slashed) plus their total, built in one in-order pass over checkpoint_state(justified) and cached in BeaconScratch, keyed by its own checkpoint. compute_weights and get_proposer_score read it. calculate_committee_fraction is split into a pure committee_fraction(total, percent). Metrics lean_beacon_justified_balances_lookups_total{result} and lean_beacon_justified_balances_build_seconds; docs in docs/metrics.md and docs/beacon_stf.md.
perf(beacon): keep latest messages dense, indexed by validator	BeaconScratch.latest_messages becomes Vec<Option<LatestMessage>>, so the vote loop walks votes and snapshot balances in index order and skips the equivocator probe while the set is empty. No API change.
fix(beacon): leave slashed validators out of the committee-fraction total	Review fix. The committee-fraction total (proposer boost and both reorg thresholds) counts active, unslashed validators only, per consensus-specs #5679. The vote weights and the total share one predicate (vote_balance), so the snapshot's total is the sum of its balances; JustifiedBalances::total_active_balance is renamed total_balance. calculate_committee_fraction walks the registry once with iter_validators() and no longer returns Result.

Tests:

• get_weight stays spec-literal as the oracle. The weights-vs-spec unit test now includes slashed, not-yet-active, exited and past-the-registry voters while a boost is set.
• New unit tests: build_justified_balances edge cases (activation and exit at the epoch, slashed-but-active, all-inactive floor, index past the end); a new justified checkpoint rebuilds the snapshot; get_proposer_score leaves out a slashed-but-active validator (and agrees with calculate_committee_fraction on the state); dense storage table (index order, gaps, overwrite, equivocators).
• The beacon fork_choice fixture runner now asserts, at every checks step, that compute_weights equals get_weight for every root of the filtered block tree. No fixture checks weights directly, so this puts the snapshot against the spec path on every case. It skips a root only where get_weight itself errors because invalidation removed the voted block (the sync/*/optimistic/from_syncing_to_invalid cases), since compute_weights drops those votes by design. It is the only edit to the runner.

Design decisions

Question	Decision
Boost total	Active, unslashed validators only, floored at the increment, computed in the same pass as the snapshot (saturating_add). This is consensus-specs #5679 (v1.7.0-beta.4), ahead of the v1.7.0-beta.2 fixture pin, whose calculate_committee_fraction still uses get_total_active_balance; Lighthouse already excluded slashed stake (Lighthouse issue 10057). The state transition's get_total_active_balance is unchanged.
Storage	Box<[Gwei]>, not u16 increments (avoids an invariant to assert).
Where it lives	A BeaconScratch field, not a new module.
Snapshot source	Only checkpoint_state(justified). A later post-state of the same epoch is not exact (slashed can change mid-epoch). Equivocations stay out of the snapshot and are filtered per vote.
Cache key	The checkpoint itself, checked on every read, so no hook on the code that moves the justified checkpoint.
is_head_weak / is_parent_strong	Not switched: their thresholds still come from calculate_committee_fraction(checkpoint_state(justified)). That is the same state the snapshot is built from, and both totals use vote_balance, so they cannot disagree.
Dense vote bound	debug_assert! that an index is below a plausible bound before the table grows; the table has one slot per index up to the highest seen, so only validated attestation indices may reach it. Release builds do not check.
Lock	Only the chain actor reaches the BeaconScratch lock on this path: no crates/net code calls the snapshot or vote accessors.

Deviation from the original sketch: the builder does not debug_assert that the state's epoch equals the checkpoint's epoch. Two existing unit-test stores pair an epoch-0 checkpoint with a state one epoch in, and activity is evaluated at the state's own epoch exactly as get_weight does, so correctness does not depend on it.

Not in this PR

• Prefetching the snapshot for the unrealized justified checkpoint (so the epoch-start pull-up finds it ready): wait for get_head max at epoch-start blocks after this PR.
• Per-root running vote totals (delta weights): only worth it if a profile after this PR still shows the weight loop.
• A proto-array style tree: a separate fork-choice project.
• Optional single lock per attestation in update_latest_messages and a root-prefix hasher for the weights map.

Open question: the replay harness never ticks, so the proposer boost never resets there and every replayed get_head pays the boost path. Should it tick? Out of scope here.

Testing

Run with --profile release-fast; spec fixtures from the existing local copy.

Command	Result
cargo clippy --locked --workspace --all-targets -- -D warnings	clean
cargo clippy -p ethlambda-state-transition --all-targets --features beacon-spec-tests -- -D warnings	clean
cargo test -p ethlambda-types --lib	198 passed
cargo test -p ethlambda-storage --lib	139 passed, 1 ignored
cargo test -p ethlambda-state-transition --lib	327 passed, 16 ignored
beacon spec suite, mainnet preset, full	5734 passed, 152 ignored; 4 gossip/*/matched_fixture_cases fail only because the gossip fixture tree is empty in this environment
beacon spec suite, minimal preset, full	40038 passed, 3692 ignored; same 4 gossip trials
cargo check -p ethlambda	ok

After the review fix (ec477bc1):

Command	Result
make lint	clean
cargo test -p ethlambda-types -p ethlambda-state-transition -p ethlambda-storage --lib	607 + 189 + 227 passed
beacon spec suite, minimal preset, fork_choice (v1.7.0-beta.2)	416 passed
v1.7.0-beta.4's get_head/proposer_boost_excludes_slashed_validators, copied into the local tree (altair to gloas)	7 passed; 7 failed before the fix
beacon spec suite, mainnet preset	not rerun locally; CI's test-beacon-mainnet covers it

[MegaRedHand]

Import-replay benchmark

Method. ethlambda benchmark import replay, release build, 128 consecutive mainnet blocks (slots 15279073..=15279200, 4 of them epoch-start blocks) on a 16-core host. This PR's leg and a base leg (beacon-chain-integration) were interleaved, 2 rounds each; the table shows the mean of the two. Both legs carry #651, so stf excludes the state-writer hand-off wait. Block roots were identical to the base in every leg. Base vs base noise floor: 1.004.

	base	this PR	change
get_head p50	200 ms	15 ms	−92%
block_atts p50	5.4 ms	2.2 ms	−59%
stf ordinary block p50	577 ms	618 ms	+7%
stf epoch-start block (mean of 4)	1505 ms	1583 ms	+5%
block wall p50	777 ms	654 ms	−16%
user CPU (whole run)	214.7 s	197.9 s	−8%
peak RSS	3.90 GiB	4.39 GiB	+0.49
paired block wall, ratio of totals (round 1 / round 2)		0.829 / 0.829

get_head lands well under the estimated ≤ 126 ms.

This PR does not touch block processing, yet stf rises 7%. The writer-wait phase shows the state-writer queue never fills here, so the cause is not that wait. A likely cause is contention with the writer thread's state encode: with get_head this much shorter, that encode overlaps stf instead of get_head. Supporting this, a 1 s delay between blocks lowers even the base's stf by ~50 ms. Not verified on this PR's leg.

All five performance PRs together (#647, #648, #649, #650, #633), measured with a 1 s delay between blocks so the state writer drains (--block-delay, #651):

• ordinary stf 527 → 163 ms; epoch-start 1440 → 420 ms;
• get_head 200 → 16 ms;
• block wall p50 729 → 185 ms (3.9x); user CPU −30%.

[github-actions]

🤖 Codex Code Review

Overall this looks solid. The main fork-choice optimization is well targeted, and I don’t see a consensus-logic regression in the new JustifiedBalances snapshot or dense latest-message table.

• crates/blockchain/state_transition/src/beacon/fork_choice.rs:1172 preserves the spec-important distinction correctly: slashed-but-active validators still contribute to total_active_balance, but not to vote weight.
• crates/blockchain/state_transition/src/beacon/fork_choice.rs:1297 drops votes for pruned roots instead of aborting head computation, which is a good resilience fix for live nodes and does not appear to change LMD-GHOST outcomes above the retained finalized window.
• crates/blockchain/state_transition/src/beacon/fork_choice.rs:1320 applies proposer boost only along the boosted block’s ancestor chain up to the justified checkpoint, which matches the intended fork-choice scope.

One thing I’d flag:

• crates/storage/src/store.rs:3393 calls the user-supplied closure while holding the beacon mutex. That is safe for the current caller set, but it is a maintainability/concurrency footgun: any future callback that touches store beacon state will deadlock. I’d strongly prefer snapshotting the needed (index, message) pairs under the lock, releasing it, then iterating the snapshot. This is not a consensus bug today, but it is the main review risk I see.

No other correctness, security, memory-safety, or SSZ concerns stood out from this diff.

---

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

[github-actions]

🤖 Claude Code Review

Review of PR 649: justified-balances snapshot and dense vote table (beacon fork choice)

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

The change is sound overall and the semantics look preserved. The tests are thorough.

Why the semantics look preserved

• build_justified_balances mirrors the old compute_weights filter. It uses the state's own epoch, treats a slashed or inactive validator as zero, and treats an index past the registry as a skip.
• The total follows get_total_active_balance. It counts slashed-but-active validators, saturates, and is floored at one increment. building_the_snapshot_… asserts this against the spec function.
• Keying the snapshot by the justified checkpoint avoids hooks on every writer. A stale snapshot just misses.
• Equivocators are still filtered per vote, because they can change at any time.
• The oracle in check_weights_against_the_spec runs compute_weights against get_weight at every checks step of every fixture. That is a strong guard.

Points to consider

1. Debug-only bound on the dense vote table (crates/storage/src/store.rs, set_latest_message). In release builds nothing bounds the table, so one bad index would allocate a large Vec<Option<LatestMessage>>. LatestMessage is about 40 bytes. An index near 2^32 would take roughly 160 GB, and the resize would abort the process.

   • The callers I found (update_latest_messages in fork_choice.rs, reached from on_attestation, on_aggregate and apply_verified_aggregate) use attesting indices that were already verified, so this should be fine today.
   • Consider a cheap release-mode guard anyway, such as ignoring or warn!-ing on an index above a bound, or checking against the justified registry length. A consensus client should not be able to hit an OOM through a future caller mistake. This is the only item I would call a robustness risk.

2. Memory per resize. resize(slot + 1, None) grows amortized, so the doubling is fine. The cost is that Vec<Option<LatestMessage>> is about 2 million entries times 40+ bytes, roughly 80+ MB on mainnet. The old HashMap was sparse. This is probably acceptable for the speed gain, but it is worth noting in the PR description or docs/beacon_stf.md.

3. Lock and cache behavior.

   • On a miss, justified_balances calls checkpoint_state and then set_justified_balances without holding the lock across the build. Two concurrent readers could both build the snapshot. That is only duplicated work and is harmless, but the metrics doc says "or the snapshot is being replaced between two readers", so it is already acknowledged.
   • compute_weights now takes the snapshot once, so the boost and the votes always use the same checkpoint. That is a slight improvement over the old code, which re-read the checkpoint through get_proposer_score.

4. Minor nit. check_weights_against_the_spec skips errors by matching format!("{err:?}").contains("root in store.blocks"). That breaks silently if the error text changes. A typed match would be sturdier, but it is acceptable in test-only code.

5. Docs. docs/metrics.md and docs/beacon_stf.md are updated, and the metric names follow the lean_ convention. Good.

I found no consensus-correctness issues. Item 1 is the one I'd address before merging, or at least confirm that every caller passes a validated index.

---

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

[github-actions]

🤖 Kimi Code Review

I'll review this PR which optimizes fork-choice weight computation by introducing a JustifiedBalances snapshot and converting latest_messages from a HashMap to a dense Vec. This is performance-critical consensus code, so I'll be thorough.

Security & Correctness

1. get method on JustifiedBalances — potential truncation on 32-bit systems

File: crates/common/types/src/beacon/fork_choice.rs, lines 147-153

pub fn get(&self, index: ValidatorIndex) -> Gwei {
    usize::try_from(index)
        .ok()
        .and_then(|index| self.balances.get(index))
        .copied()
        .unwrap_or_default()
}

On 32-bit systems, usize::try_from(index) will fail for validator indices > u32::MAX, silently returning 0. While mainnet won't hit this soon, this is consensus-critical: a validator with index > 2^32-1 would have their vote weight treated as zero.

Suggestion: Add a comment documenting this limitation, or use usize::try_from(index).expect("validator index fits in usize") if the code already assumes 64-bit elsewhere. Actually, looking at Store::set_latest_message, it uses usize::try_from(index).expect("validator index fits in usize") — so there's already an implicit 64-bit assumption. Consider being consistent.

2. build_justified_balances — epoch semantics vs. checkpoint epoch

File: crates/blockchain/state_transition/src/beacon/fork_choice.rs, lines 1172-1175

// Activity at the state's own epoch, as `get_weight` reads it; not asserted
// equal to `checkpoint.epoch`, which the unit-test stores do not keep.
let epoch = get_current_epoch(state);

The comment acknowledges this but it's subtle: the snapshot uses get_current_epoch(state), not checkpoint.epoch. This is correct per spec (activity is always evaluated at the state's current epoch), but the divergence between checkpoint.epoch and state's epoch could be a source of bugs if states are ever cached with mismatched epochs.

Suggestion: The comment is good, but consider a debug assertion in non-test builds:

debug_assert_eq!(epoch, checkpoint.epoch, "snapshot epoch mismatch");

3. MAX_PLAUSIBLE_VALIDATOR_INDEX — debug-only bound insufficient for production

File: crates/storage/src/store.rs, lines 646-650

const MAX_PLAUSIBLE_VALIDATOR_INDEX: u64 = 1 << 26;

This is only checked via debug_assert!. In release builds, a malicious or buggy attestation with a huge validator index could cause massive memory allocation via resize. The latest_messages vector grows to index + 1 elements.

Critical issue: While the comment says indices "must come from a validated attestation," the defense is only by convention. If any code path calls set_latest_message without prior validation, this is an unbounded growth vector.

Suggestion: Make this a hard check, not debug-only:

assert!(
    index < MAX_PLAUSIBLE_VALIDATOR_INDEX,
    "validator index {index} exceeds maximum plausible index"
);

Or at minimum, use checked_add and return Result or Option to fail gracefully.

Performance

4. for_each_non_equivocating_latest_message — branch prediction optimization

File: crates/storage/src/store.rs, lines 3394-3407

let any_equivocators = !beacon.equivocating_indices.is_empty();
for (index, message) in beacon.latest_messages.iter().enumerate() {
    // ...
    if any_equivocators && beacon.equivocating_indices.contains(&index) {
        continue;
    }

Good optimization for the common case. However, HashSet::contains on equivocating_indices is still called per-message when there are any equivocators. Since equivocations are rare and typically involve few validators, consider:

Suggestion: If equivocating_indices is typically small, iterate the equivocating set to clear entries in latest_messages instead, or use a BitVec for equivocating_indices when the validator set is dense. This is probably overkill unless profiling shows it's hot.

5. compute_weights — double lookup for proposer score

File: crates/blockchain/state_transition/src/beacon/fork_choice.rs, lines 1335-1338

let proposer_score =
    committee_fraction(balances.total_active_balance(), config.proposer_score_boost);

This duplicates the justified_balances() call from get_proposer_score. Since compute_weights already has balances, this is correct and avoids the cache lookup. Good.

However, is_head_weak and is_parent_strong each call justified_balances separately. If called in sequence, that's two cache hits (or worse, two potential rebuilds). Consider if these are ever called together and whether passing &JustifiedBalances would help.

Rust Idioms & Safety

6. JustifiedBalances::new — constructor pattern

File: crates/common/types/src/beacon/fork_choice.rs, lines 125-132

pub fn new(checkpoint: Checkpoint, balances: Box<[Gwei]>, total_active_balance: Gwei) -> Self

Consider making this pub(crate) or adding validation: total_active_balance should equal the sum of active balances (including slashed), not just any passed value. The current trust model is fine (only build_justified_balances constructs this), but documenting the invariant helps.

7. set_justified_balances takes &self

File: crates/storage/src/store.rs, lines 3428-3432

pub fn set_justified_balances(&self, balances: Arc<JustifiedBalances>) {
    self.beacon.lock().unwrap().justified_balances = Some(balances);
}

This is consistent with cache_state as noted, but it's unusual for a "set" method to take &self. The comment explains why. Consider naming it cache_justified_balances for consistency with cache_state.

8. latest_messages resize — potential for large allocation on first vote

File: crates/storage/src/store.rs, lines 3374-3377

if beacon.latest_messages.len() <= slot {
    beacon.latest_messages.resize(slot + 1, None);
}

If the first vote seen is from validator 1,000,000, this allocates ~8MB immediately (1M * Option<LatestMessage>). This is probably fine given the MAX_PLAUSIBLE_VALIDATOR_INDEX bound, but worth noting.

Error Handling & Edge Cases

9. check_weights_against_the_spec — fragile error string matching

File: crates/blockchain/state_transition/tests/beacon_spec/fork_choice.rs, lines 719-722

Err(err) if format!("{err:?}").contains("root in store.blocks") => continue,

This is fragile: it depends on the Debug format of an error type, which can change. Use a typed error or downcast_ref if possible.

Suggestion: Define a specific error variant for missing roots, or use matches! on the error enum if it's an enum. If the error type is opaque, consider adding an is_missing_root() method.

10. build_justified_balances — empty active set handling

File: crates/blockchain/state_transition/src/beacon/fork_choice.rs, line 1194

total.max(preset::EFFECTIVE_BALANCE_INCREMENT)

Correctly floors at one increment per spec. Good. The test at line 3082 verifies this.

Documentation & Maintainability

11. Comment typo in docs/beacon_stf.md

File: docs/beacon_stf.md, line 339

"This is ethlambda-specific, not part of the leanMetrics spec."

Should be "specification" or "consensus spec," not "leanMetrics spec" (appears to be a copy-paste from metrics docs).

12. JustifiedBalances field documentation

File: crates/common/types/src/beacon/fork_choice.rs, lines 119-124

The doc comment says "Zero unless the validator is active at checkpoint.epoch and unslashed" — but per the code, activity is at get_current_epoch(state), not checkpoint.epoch. The comment in build_justified_balances acknowledges this, but the struct-level doc is slightly misleading.

Tests

13. Test coverage — good but missing one case

The tests cover:

• Slashed but active validators (count in total, zero weight) ✓
• Inactive validators (zero weight, not in total) ✓
• Past-registry indices (zero weight) ✓
• Empty active set (floor at increment) ✓
• Cache hit/miss on checkpoint change ✓
• Proposer score with slashed validator in total ✓

Missing: What if checkpoint_state returns a state from a different fork/epoch than expected? The justified_balances function trusts checkpoint_state — this is correct per spec but could use a test for the mismatch case (e.g., checkpoint epoch 5, but state epoch 6 due to caching bug).

Summary

Severity	Issue	Location
Medium	MAX_PLAUSIBLE_VALIDATOR_INDEX is debug-only	store.rs:3370
Low	Fragile error string matching in tests	fork_choice.rs:720
Low	usize truncation on 32-bit systems	fork_choice.rs:147
Low	Doc comment epoch mismatch	fork_choice.rs:119
Nit	set_justified_balances naming	store.rs:3428

The core logic is correct and well-tested. The JustifiedBalances snapshot correctly replicates spec semantics while avoiding per-vote state lookups. The dense latest_messages table is a good optimization with acceptable memory tradeoffs.

Most important fix: Harden MAX_PLAUSIBLE_VALIDATOR_INDEX to a release assertion or bounded growth policy.

---

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

[ilitteri]

Medium: the committee-fraction total must exclude active slashed validators.

calculate_committee_fraction in specs/phase0/fork-choice.md:300-307 builds unslashed_and_active_indices and computes total_balance over those indices. build_justified_balances instead adds every active validator to total before checking validator.slashed (crates/blockchain/state_transition/src/beacon/fork_choice.rs:1321-1333). That total feeds proposer boost and reorg thresholds, so slashed-but-active stake currently inflates them.

Lighthouse's JustifiedBalances::from_justified_state adds only active non-slashed validators to total_effective_balance; Prysm likewise supplies ActiveNonSlashedBalancesByRoot for this calculation.

This behavior existed before the PR, but the new snapshot and the_proposer_score_counts_a_slashed_but_active_validator test now codify the non-spec behavior. Please compute the committee total from the active non-slashed snapshot balances and update that test's expectation (9 active non-slashed validators in its fixture, not 10 including the slashed one). The separate get_total_active_balance used by state-transition rewards is not the quantity this fork-choice helper should use.

[MegaRedHand]

Fixed in ec477bc: the committee-fraction total (proposer boost and both reorg thresholds) now counts active, unslashed validators only, and the_proposer_score_leaves_out_a_slashed_but_active_validator expects 9.

Note on the spec reference: the pinned fixtures (v1.7.0-beta.2) still define calculate_committee_fraction with get_total_active_balance, which is what the old code followed. The change is consensus-specs PR 5679, released in v1.7.0-beta.4. Implemented ahead of the pin, since Lighthouse already excluded slashed stake. Checks:

• v1.7.0-beta.2 minimal fork_choice suite: 416 passed.
• v1.7.0-beta.4's get_head/proposer_boost_excludes_slashed_validators, copied into the local tree: passes on altair through gloas with this change, fails on all 7 without it.

get_total_active_balance is unchanged for the state transition.