perf(beacon): fuse electra epoch steps 2-9 into one registry pass

[MegaRedHand]

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) #632, (c) this PR. Stacked on (b): this PR's diff is the fusion only, and it targets (b)'s branch.

Motivation

After (b), electra and fulu still walk the tree-backed registry once per remaining epoch step: registry updates, slashings, pending deposits and effective-balance updates. Several of those steps also recompute get_total_active_balance, which descends once per active validator. This is option (c) of the plan, the Lighthouse-style single pass (process_epoch_single_pass).

Change: fuse electra/fulu epoch steps 2-9 into one loop

stf::epoch::single_pass visits each validator once, in index order, working on a local copy of the validator and its balance. Per validator, in spec order:

1. inactivity score;
2. rewards and penalties;
3. registry action (queue, eject or activate);
4. slashing penalty;
5. planned deposit top-up;
6. effective-balance update.

Changes are written back after the loop. What depends on more than one validator stays outside it, as in Lighthouse:

• Exit churn: the cursor is advanced locally in index order and written back once.
• Pending deposits: the queue is planned before the loop. A deposit counts as "exited" if the validator already has an exit or this epoch's registry update will eject it. Top-ups of existing validators apply inside the loop; deposits that create validators apply after it, and one registry walk serves all pubkey lookups.
• Consolidations: pending consolidations run after the loop. The effective-balance update of every validator they name, and of every validator created by deposits, waits until then.
• Total active balance: taken from the summary and reused for slashings and every churn limit. It equals get_total_active_balance through step 8.

The hysteresis test both effective-balance updates use is now one checked leaves_hysteresis_band: a balance near u64::MAX is an ArithmeticOverflow, as in the spec and Lighthouse's safe_add, rather than a debug panic or a release wrap that moves the effective balance. It keeps the spec's short-circuit or.

The genesis epoch, and states whose registry-sized lists differ in length, fall back to the step-by-step path. The step functions stay public and callable in isolation for the fixtures. The per-step rules both paths share were factored out so both run the same code: registry action, slashing penalty, effective-balance hysteresis, inactivity score and the exit-cursor advance.

Testing

• In debug builds, a registry of up to 4096 validators also runs through the specification-shaped steps 1-9 on a clone, and the two full post-states must have equal hash_tree_root. A mismatch names the differing fields.
• A randomized fused-vs-unfused test runs 800 electra and fulu cases. They cover ejections that move the churn cursor, activations and the activation queue, slashings, every pending-deposit branch (including new pubkeys with valid and invalid signatures), and processed, slashed and not-yet-withdrawable consolidations. Two deliberately injected bugs were caught within a few seeds. About one state in ten carries a near-u64::MAX balance, since one now fails the whole epoch; those cases check that both paths fail together. As in (b), the finalized epoch stays at or behind the previous epoch.
• Clippy -D warnings and the ethlambda-state-transition lib tests pass, including the dev-profile Test minimal preset run, where overflow checks are on.
• Beacon spec filters epoch_processing, rewards, sanity, finality, random, fork_choice and transition pass on both presets. That run predates the rebase onto (b)'s finality-delay fix and the checked hysteresis; CI's beacon spec jobs cover both.

Over (b), this saves about 200 ms per epoch-start block and nothing on other blocks. That is more than the plan estimated, probably because the fusion also removes the repeated total-active-balance calls and the per-deposit pubkey scans (not profiled). A per-epoch total cache (follow-up 01) and a pubkey-to-index map would recover part of that gain more simply.

Benchmark (all three options)

ethlambda benchmark import replay on ethlambda-5: cargo build --release, mainnet corpus of 128 blocks (slots 15279073..=15279200, 4 epoch-start blocks), legs interleaved base a b c for two rounds. Base against base: 0.996. Every leg imported all 128 blocks.

	base (c913728)	(a) iterator passes	(b) summary pass	(c) single pass
process wall	104.0 s	96.1 s	92.0 s	91.0 s
user CPU	215.1 s	208.4 s	205.1 s	203.8 s
stf, epoch-start block (mean of 4)	1503 ms	1126 ms	912 ms	709 ms
stf p50, other blocks	577 ms	517 ms	478 ms	479 ms
paired stf vs base (ratio of totals)	1	0.869	0.797	0.785
get_head p50	201 ms	214 ms	226 ms	225 ms
peak RSS	3.88 GiB	3.93 GiB	4.04 GiB	4.04 GiB

• The per-block saving comes from the pulled-up tip, which reruns justification on every imported block.
• (c) gains nothing over (b) on ordinary blocks, since their per-block path is the same.
• get_head rises as stf falls, 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

• Moved from lambdaclass/ethlambda_private#62, now that beacon-chain-integration lives on this repo.
• Stacked on perf(beacon): run epoch steps 1-3 over one registry scan #632 (was lambdaclass/ethlambda_private#61); its head, which carries beacon-chain-integration @ c79fabd5, is merged into the branch.

[MegaRedHand]

Import-replay benchmark (re-measured on the current base)

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 (includes #632)	change
stf epoch-start block (mean of 4)	1505 ms	713 ms	−53%
stf ordinary block p50	577 ms	476 ms	−18%
get_head p50	200 ms	223 ms	+11%
block wall p50	777 ms	696 ms	−10%
user CPU (whole run)	214.7 s	203.8 s	−5%
peak RSS	3.90 GiB	4.33 GiB	+0.43
paired stf, ratio of totals (round 1 / round 2)		0.789 / 0.776

Per epoch-start block (stf ms):

slot	base	this PR
15279104	1607	835
15279136	1521	727
15279168	1486	683
15279200	1405	609

This reproduces the earlier measurement on the older base (epoch-start 1503 → 709 ms, ordinary 577 → 479 ms), including the small get_head rise. That rise is probably contention with the background state writer's encode, which now overlaps get_head more; not verified.

All five performance PRs together (#647, #648, #649, #650, this one), 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;
• block wall p50 729 → 185 ms (3.9x); user CPU −30%.

In that build, the fused pass's inactivity-score, balance and validator writes go through #647's in-order write cursor, and the scores are #650's tree list.

[github-actions]

🤖 Codex Code Review

I reviewed the PR diff in /tmp/pr_diff.txt and the touched code paths. Overall this looks careful: the refactors preserve overflow checks, keep state writes deferred, and add a solid fused-vs-unfused equivalence oracle in debug/test builds. I did not spot an obvious consensus, memory-safety, or security bug in the changed logic.

A few concise notes:

• Looks good overall

  • The extracted helpers like ExitChurnCursor, SlashingsContext, and next_inactivity_score keep the spec-sensitive arithmetic checks intact.
  • The fused Electra epoch path is backed by a strong equivalence harness in crates/blockchain/state_transition/src/beacon/stf/epoch/single_pass_equivalence.rs:470 and crates/blockchain/state_transition/src/beacon/stf/epoch/single_pass.rs:86, which is exactly the right safety net for consensus code.

• Performance win

  • Reusing total_active_balance via balance_churn_limit_for / activation_exit_churn_limit_for avoids extra registry scans in hot epoch processing paths; that’s a clean optimization in crates/blockchain/state_transition/src/beacon/helpers/electra.rs:354 and crates/blockchain/state_transition/src/beacon/helpers/electra.rs:378.
  • The fused path in crates/blockchain/state_transition/src/beacon/stf/epoch/single_pass.rs:134 should materially reduce repeated full-registry passes without changing step ordering.

• Minor maintainability nit

  • crates/blockchain/state_transition/src/beacon/stf/epoch/single_pass.rs:232 assumes at most one aggregated top-up entry per validator:
    • if let Some(&&(_, amount)) = top_ups.peek().filter(|&&&(at, _)| at == index) { ... }
  • That is currently true because DepositPlan::new aggregates with a BTreeMap, but the invariant is implicit. A brief comment there would help future readers avoid accidentally changing DepositPlan in a way that breaks this single peek()/next() consumption pattern.

• Minor API/readability nit

  • ExitChurnCursor::read takes &mut BeaconState even though it only reads fields, in crates/blockchain/state_transition/src/beacon/helpers/electra.rs:629. If the underlying accessor permits it, &BeaconState would better signal intent and reduce accidental borrow pressure in future callers.

No blocking issues from me based on the diff provided.

---

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

[github-actions]

🤖 Claude Code Review

Review of PR 633: single-pass electra/fulu epoch processing

I read the fused path in single_pass.rs and the refactors in helpers/electra.rs and altair.rs. I skimmed electra.rs and the docs, and I did not read single_pass_equivalence.rs or mod.rs. I did not build the crate or run any tests.

Overall: the fusion looks sound. The behaviour-preserving refactors hold up too.

What I checked

• Ordering inside the loop. Inactivity score, rewards, registry action, slashing penalty, top-up, then effective balance is the spec's per-validator order. Slashings correctly read the validator after step 4. Ejections can move withdrawable_epoch, so this matters.
• Justification before reward constants. weigh_with_totals runs first. RewardContext::new and is_leaking() are built afterwards, so they see the updated finalized checkpoint.
• Total active balance reuse. Reusing the summary's total through step 8 is justified. Exit and activation epochs are at least compute_activation_exit_epoch, and effective balances only change in step 9.
• Exit-churn cursor. ExitChurnCursor::advance is the old body moved verbatim, and compute_exit_epoch_and_update_churn now delegates to it. It uses checked arithmetic and leaves the cursor untouched on error.
• Deposit planning. The reachable-prefix computation, first-match pubkey lookup, withdrawn/exited/postpone/churn branches and deposit_balance_to_consume all mirror process_pending_deposits. The "ejected now" handling is covered by the argument in the DepositPlan::new doc comment.
• Consolidation deferral. consolidation_participants deliberately over-approximates. That is safe, because a deferred validator whose balance nothing touched ends up with the same effective balance. Newly appended validators from new-pubkey deposits also get step 9 (registry_len..new_registry_len).
• Fallback. can_fuse falls back to the unfused steps for the genesis epoch and for mismatched list lengths. That preserves the per-step error behaviour.

Suggestions

1. Debug oracle panics outside tests (single_pass.rs, process_steps_through_effective_balances).

   • The oracle is gated on debug_assertions and runs for registries up to 4096 validators. It clones the state, runs the slow path, and assert_eq!/panic!s on divergence.
   • The docs say release-fast keeps debug assertions, so any such profile would run it on a real node.
   • Since the oracle only uses cfg(debug_assertions), make sure no shipped or devnet profile enables them. An explicit cfg(test) or a feature flag would make that harder to get wrong.
   • Small registries (minimal-preset devnets) pay a full state clone plus two hash-tree-roots every epoch.

2. DepositPlan::new takes the deposit queue out of state (single_pass.rs, around line 418).

   • It does mem::take(fields.pending_deposits_mut()). If a later step in fused returns an error, state is left with an empty queue.
   • That is harmless if callers always discard the state on Err. The unfused path also mutates partially on error, so this is no worse.
   • A one-line comment saying so would prevent a future caller from relying on the state after an error.

3. first_indices_of is O(registry × reachable deposits) in the worst case.

   • At most MAX_PENDING_DEPOSITS_PER_EPOCH (16 on mainnet) keys are searched, so the inner loop is cheap, and the 8-byte prefix check helps.
   • It also exits early once every key is found. This is fine as written. I'm only noting it because the loop is hot.

4. Minor. write_back re-indexes the validators, balances and scores with get_mut, so every change is a second tree descent. This is much cheaper than before but not free. If the ssz-tree list offers a batch or iterator-based mutation, that would be a possible follow-up.

I found no correctness or security problems. It's worth confirming in CI that the mainnet and minimal epoch_processing and sanity/transition fixtures pass, since those are the real check on the fusion. I'd also run single_pass_equivalence.rs with deposits, consolidations and ejections in the same epoch.

---

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

[github-actions]

🤖 Kimi Code Review

I'll review this PR which introduces a single-pass optimization for Electra/Fulu epoch processing, with extensive testing infrastructure. Let me analyze the code carefully.

Overall Assessment

This is a significant optimization PR that fuses multiple epoch-processing steps into a single registry walk. The approach is well-documented and includes a debug oracle for equivalence checking. However, I've identified several issues ranging from correctness concerns to performance pitfalls.

---

Critical Issues

1. DepositPlan::new — Incorrect handling of is_validator_exited for ejected validators

File: single_pass.rs, lines 530-550

let ejected_now = validator.exit_epoch == FAR_FUTURE_EPOCH
    && matches!(
        registry_action(validator, current_epoch, finalized_epoch, config.ejection_balance),
        Some(RegistryAction::Eject)
    );
(
    validator.exit_epoch < FAR_FUTURE_EPOCH || ejected_now,
    !ejected_now && validator.withdrawable_epoch < next_epoch,
)

Problem: The is_validator_withdrawn calculation uses !ejected_now && validator.withdrawable_epoch < next_epoch, but this is checked before registry updates actually run. For a validator being ejected now, withdrawable_epoch is still FAR_FUTURE_EPOCH (or some old value), not the computed exit_epoch + MIN_VALIDATOR_WITHDRAWABILITY_DELAY.

However, the specification's process_pending_deposits runs after process_registry_updates, so ejected validators in the spec have their exit_epoch and withdrawable_epoch already set when deposits are processed.

In the single pass, you're checking withdrawable_epoch < next_epoch on the pre-ejection state. If a validator was previously exited (exit_epoch < FAR_FUTURE_EPOCH) with withdrawable_epoch < next_epoch, it's correctly marked withdrawn. But for a validator being ejected this epoch, ejected_now is true, so !ejected_now is false, making is_validator_withdrawn false — this matches the spec since the ejection hasn't set withdrawable_epoch yet.

Wait — let me re-check. The issue is: in the spec, process_registry_updates runs before process_pending_deposits. So by the time we process deposits, validators ejected this epoch have:

• exit_epoch = compute_activation_exit_epoch(current_epoch) (or later)
• withdrawable_epoch = exit_epoch + MIN_VALIDATOR_WITHDRAWABILITY_DELAY

In your fused loop, you're computing registry_action to determine if a validator will be ejected, but the withdrawable_epoch hasn't been updated yet. Your code sets is_validator_withdrawn = false for ejected_now validators because !ejected_now is false.

But this is correct behavior! A validator ejected this epoch should NOT have its deposit treated as "withdrawn" — it should be postponed (exited but not withdrawable). Let me trace through more carefully...

Actually, looking at the spec's process_pending_deposits:

• If is_validator_withdrawn(validator) — credit the deposit
• Elif is_validator_exited(validator) — postpone

For a validator ejected this epoch in the spec: after registry updates, exit_epoch is set and withdrawable_epoch = exit_epoch + delay > current_epoch + 1 = next_epoch. So is_validator_withdrawn is false, is_validator_exited is true → postpone.

In your code: ejected_now = true, so is_validator_exited = true || true = true, is_validator_withdrawn = false && ... = false → falls through to the is_validator_exited branch → postpone.

This appears correct. However, there's a subtle issue: what if validator.exit_epoch < FAR_FUTURE_EPOCH already (previously exited) AND ejected_now is also true? That shouldn't happen because registry_action returns Eject only when validator.exit_epoch == FAR_FUTURE_EPOCH.

Actually wait — registry_action checks validator.exit_epoch == FAR_FUTURE_EPOCH for ejection. So ejected_now implies validator.exit_epoch == FAR_FUTURE_EPOCH. The is_validator_exited becomes false || true = true. Correct.

But there's another case: what about a validator that was already exited (exit_epoch < FAR_FUTURE_EPOCH) and whose withdrawable_epoch < next_epoch? Then is_validator_withdrawn = true, and the deposit is credited. This matches the spec.

After deeper analysis, this appears correct. But the complexity warrants a comment explaining why pre-computing registry_action is valid here.

---

2. DepositPlan::new — found lookup uses stale validator state for first_indices_of

File: single_pass.rs, lines 484-495

let found = first_indices_of(
    state,
    deposits[..reachable].iter().map(|deposit| deposit.pubkey),
);

Problem: first_indices_of searches the registry for pubkeys. However, apply_pending_deposit (called in finish) appends new validators to the registry. The specification's process_pending_deposits processes deposits in queue order, and for deposits with "new" pubkeys, each successful deposit creates a validator that subsequent deposits in the same queue might match against.

Your DepositPlan computes found once upfront, but new validators created by earlier "new pubkey" deposits in the queue won't be found by later deposits that might match them. This is a correctness bug.

Example:

• Deposit A: new pubkey X, valid signature
• Deposit B: same pubkey X, any signature

In the spec: Deposit A creates validator for X. Deposit B finds existing validator for X and tops it up.

In your code: Both A and B are classified as NewPubkey (since found doesn't see the validator A will create). Then in finish, you apply new_pubkey_deposits in order, but apply_pending_deposit for A creates the validator, and for B... wait, let me check apply_pending_deposit.

Looking at apply_pending_deposit in electra.rs (line 513), it calls add_validator_from_pending_deposit which appends a new validator. For deposit B with the same pubkey, when apply_pending_deposit runs, it will find the existing validator (since it searches the current state) and top it up.

So the behavior is actually correct! The found classification as NewPubkey for B is conservative — it gets handled correctly in finish because apply_pending_deposit does its own lookup.

But wait — this means your DepositPlan classification is wrong for B (says NewPubkey, should be TopUp), but the final result is correct because apply_pending_deposit re-derives. This is inefficient but not incorrect.

However, there's a worse issue: churn accounting. Deposit B's amount was counted against processed_amount as a NewPubkey deposit. But if B actually gets matched to an existing validator created by A, does the churn limit apply correctly?

Looking at apply_pending_deposit — it doesn't check churn limits! The churn is only checked in DepositPlan::new. So if we have:

• Churn limit: 32 ETH
• Deposit A: 16 ETH, new pubkey
• Deposit B: 16 ETH, same pubkey X

In spec: A creates validator (16 ETH, within churn). B finds existing validator, tops up 16 ETH (total 32, still within churn? Or does top-up not count against churn?).

Actually, re-reading the spec's process_pending_deposits: the churn limit available_for_processing is checked for ALL deposits, including top-ups of existing validators. Wait no — let me re-check...

Looking at your code in DepositPlan::new, lines 570-582:

match processed_amount.checked_add(deposit.amount) {
    Some(sum) if sum <= available_for_processing => {
        processed_amount = sum;
        match existing {
            Some(index) => {
                credit(index, deposit.amount);
                kinds.push(DepositKind::TopUp);
            }
            None => kinds.push(DepositKind::NewPubkey),
        }
    }
    _ => {
        is_churn_limit_reached = true;
        break;
    }
}

So NewPubkey deposits DO count against processed_amount / churn limit. And in finish, apply_pending_deposit for new pubkeys doesn't re-check churn — it just applies.

But for deposit B (same pubkey as A), classified as NewPubkey, it was counted in processed_amount. In finish, apply_pending_deposit for A creates the validator, then for B it finds the existing validator and... wait, does apply_pending_deposit check if the validator exists?

Looking at the code (not fully shown in diff, but implied), apply_pending_deposit likely checks if the pubkey exists and either creates or tops up. If B finds the validator A just created, it would top up without additional churn check. But B's amount was already counted in processed_amount, so this is correct.

After analysis: the behavior is correct, but the found classification is stale for new pubkeys that appear multiple times. This is a minor inefficiency (deposit might be queued as NewPubkey when it could be TopUp), not a correctness bug, because apply_pending_deposit handles the actual logic correctly.

However, I recommend adding a comment explaining this subtlety, or better, updating found dynamically as new validators are conceptually added.

---

3. first_indices_of — O(n × m) complexity with early exit bug potential

File: single_pass.rs, lines 620-651

fn first_indices_of(
    state: &BeaconState,
    pubkeys: impl Iterator<Item = BlsPubkey>,
) -> Vec<Option<usize>> {
    // ...
    for (index, validator) in state.validators().iter().enumerate() {
        let validator_prefix = prefix(&validator.pubkey);
        for (slot, wanted_prefix) in prefixes.iter().enumerate() {
            if found[slot].is_none()
                && *wanted_prefix == validator_prefix
                && wanted[slot] == validator.pubkey
            {
                found[slot] = Some(index);
                missing -= 1;
            }
        }
        if missing == 0 {
            break;
        }
    }
    found
}

Problem: The inner loop checks ALL prefixes for EVERY validator, even after finding a match. The found[slot].is_none() check prevents re-finding, but you still iterate all slots. With MAX_PENDING_DEPOSITS_PER_EPOCH potentially large and millions of validators, this is O(validators × deposits) in the worst case.

The prefix optimization helps (most validators fail the integer comparison), but the inner loop still runs for all deposits. Consider using a HashMap<u64, Vec<usize>> from prefix to list of wanted indices, so you only check deposits with matching prefixes.

More critically: the missing == 0 break only triggers when ALL deposits are found, not when we've scanned enough validators. If deposits reference pubkeys not in the registry, we scan the entire validator set every time. This is correct but slow.

Recommendation: Use a hash-based approach for O(validators + deposits) expected time:

let mut prefix_map: HashMap<u64, Vec<usize>> = HashMap::new();
for (i, prefix) in prefixes.iter().enumerate() {
    prefix_map.entry(*prefix).or_default().push(i);
}
// then for each validator, only check slots with matching prefix

---

4. consolidation_participants — Incorrect deferral logic

File: single_pass.rs, lines 403-420

fn consolidation_participants(state: &mut BeaconState) -> Result<Vec<usize>> {
    let mut fields = pending_queue_fields(state, "single-pass epoch processing")?;
    let mut participants: Vec<usize> = fields
        .pending_consolidations_mut()
        .iter()
        .flat_map(|consolidation| {
            [
                consolidation.source_index as usize,
                consolidation.target_index as usize,
            ]
        })
        .collect();
    participants.sort_unstable();
    participants.dedup();
    Ok(participants)
}

Problem: This collects ALL validators mentioned in pending_consolidations, but process_pending_consolidations (step 8) stops at the first source that is not withdrawable yet. Your comment acknowledges this: "A superset of the ones the consolidation step will actually touch... which is safe."

However, this is overly conservative. If there are many pending consolidations and only the first few are processed, you're still deferring effective-balance updates for ALL mentioned validators. This reduces the benefit of the single pass.

More importantly: the deferral includes target_index, but process_pending_consolidations only reads the source's effective balance and moves balance to the target. The target's effective balance isn't read during consolidation processing — only its balance is increased. So deferring the target's effective-balance update is unnecessary conservatism.

Wait, let me re-check. Looking at process_pending_consolidations in the spec: it decreases source balance and increases target balance. The target's effective balance isn't used for any decision. So targets don't need deferral.

Actually, looking more carefully: if target balance increases, and then effective balance update runs, the target's effective balance might increase. But if consolidation runs AFTER effective balance update in the spec order, then the target's new balance from consolidation IS visible to effective balance update.

In your fused loop, you defer effective balance update for targets, then run consolidation, then apply deferred effective balance updates. This matches the spec's order: consolidation changes balance, then effective balance update sees the new balance.

But if you didn't defer targets, the fused loop would apply effective balance update on the pre-consolidation balance, which would be wrong. So deferring targets IS necessary for correctness!

Wait, let me re-read your loop structure:

1. Loop: for each validator, apply rewards, registry, slashings, top-ups, then effective balance update (unless deferred)
2. After loop: process_pending_consolidations (moves balances)
3. After consolidation: apply deferred effective balance updates

So targets DO need deferral because their balance changes in step 2 after the loop.

But sources also have their balance decreased in step 2. So sources need deferral too. Correct.

However, your consolidation_participants includes ALL pending consolidations, not just the ones that will actually be processed. If consolidation queue is [A->B, C->D] and A is not withdrawable, then neither consolidation runs, but you still defer B, C, D. This is safe but suboptimal.

Not a correctness bug, but a performance issue.

---

5. write_back — Potential panic on inconsistent state

File: single_pass.rs, lines 360-389

fn write_back(
    state: &mut BeaconState,
    // ...
) -> Result<()> {
    let validators = state.validators_mut();
    for (index, validator) in validator_changes {
        *validators
            .get_mut(index)
            .ok_or(Error::UnknownValidator(index as ValidatorIndex))? = validator;
    }
    // similar for balances, scores...
}

Problem: The function uses get_mut(index) and returns Error::UnknownValidator if out of bounds. But validator_changes is built from iterating state.validators() in the same fused function. How can indices become invalid?

If state.validators_mut() returns a different length than state.validators() did earlier, something went terribly wrong. But more critically: apply_pending_deposit in plan.finish() can APPEND validators, which happens AFTER write_back but BEFORE the deferred effective balance loop. The registry_len captured before plan.finish() is used to filter deferred, but new validators from new_pubkey_deposits need effective balance updates too.

Looking at lines 340-355:

let registry_len = state.validators().len();
plan.finish(state, config)?;
// ...
let new_registry_len = state.validators().len();
let mut effective_updates = Vec::new();
let patched = deferred
    .iter()
    .copied()
    .filter(|&index| index < registry_len)  // BUG: ignores new validators!
    .chain(registry_len..new_registry_len);  // adds new validators

Wait, this chains registry_len..new_registry_len which ARE the new validators. So new validators DO get effective balance updates. Good.

But what if a new pubkey deposit's validator is also a target of a consolidation? That can't happen — consolidations reference existing validators by index, and new validators get new indices after registry_len. So no overlap.

This appears correct.

---

6. ExitChurnCursor::read and write — Mutable borrow of state for read

File: electra.rs, lines 625-648

impl ExitChurnCursor {
    pub(crate) fn read(state: &mut BeaconState) -> Result<Self> {
        let fields = electra_state(state, "ExitChurnCursor::read")?;
        Ok(Self { /* ... */ })
    }
}

Problem: read takes &mut BeaconState but only reads from it. This is unnecessarily restrictive and prevents calling read when state is already borrowed. Should be &BeaconState.

Similarly, write correctly needs &mut BeaconState.

Also in single_pass.rs line 268: let mut exit_cursor = ExitChurnCursor::read(state)?; — this mutably borrows state for the read, but you also need state for other things. Looking at the code, state isn't used between read and when it's borrowed again for plan, so this works. But the API is still overly restrictive.

Recommendation: Change read to take &BeaconState.

---

7. DepositPlan::finish — core::mem::take on &mut self field

File: single_pass.rs, lines 597-608

fn finish(&mut self, state: &mut BeaconState, config: &Config) -> Result<()> {
    let remaining = core::mem::take(&mut self.remaining);
    // ...
    for deposit in core::mem::take(&mut self.new_pubkey_deposits) {
        apply_pending_deposit(state, &deposit, config)?;
    }
    Ok(())
}

Problem: This is correct Rust — core::mem::take replaces self.remaining with Vec::new() and returns the old value. But after this call, self.remaining is empty, and subsequent calls to finish would do nothing. This is fine if finish is

---

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