[MegaRedHand]

🗒️ Description / Motivation

Profiling the mainnet beacon follower (~2.4M validators) against Lighthouse v8.1.2 on the same blocks showed our block STF at ~200 ms vs Lighthouse's ~92 ms (on a slower CPU). The two largest steps were pubkey scans Lighthouse avoids:

• sync_aggregate: 72 ms per block. apply_sync_aggregate walked the whole registry every block to map the 512 sync committee pubkeys to indices (95 of 117 perf samples in SipHash of &BlsPubkey). Lighthouse resolves them through BeaconState::pubkey_cache, an incrementally extended pubkey -> index map; its process_sync_aggregate had 3 samples in the same window.
• execution_requests: 29 ms per block. Withdrawal and consolidation requests looked validators up with iter_validators().position(..), and most of the cost was compute_total_active_balance (a full registry pass) recomputed several times per request through the churn helpers. Lighthouse caches the total active balance per epoch.

What Changed

• ethlambda-storage: new PubkeyIndexCache (pubkey_index_cache.rs), held by Store like ActiveBalanceCache. A sorted base of (u64 key, u32 index) plus an ordered tail (a BTreeSet, merged into the base at 8192 entries). The base costs 12 bytes an entry, ~29 MB at 2.4M validators. The key is 8 bytes of the compressed pubkey. A bulk ingest (the first sight of a registry) is sorted before the write lock is taken.
• ethlambda-state-transition: PubkeyIndexCacheExt (helpers/accessors.rs) ingests only the part of the registry the cache has not seen, then verifies every candidate against the state (state.validator(i).pubkey == pubkey). A miss or a failed verification falls back to a linear scan for that query, and an index the scan finds is written back (PubkeyIndexCache::insert). The table is never rebuilt.
• Sync aggregate seats, withdrawal requests and consolidation requests (electra, fulu, gloas) resolve through the cache. The sync aggregate scans the registry only for the seats the cache could not verify, and a switch-to-compounding request resolves its source once.
• Exit and consolidation churn take the block's ActiveBalanceCache and use *_for(total, config) formulas, so one registry pass serves the block instead of several per request. That covers voluntary exits, withdrawal and consolidation requests, and the exits that proposer and attester slashings initiate. Epoch processing still passes None.
• Block processing takes its three caches (committees, active balances, pubkeys) as one stf::BlockCaches through state_transition / apply_block / process_block. Callers without a Store (spec tests, era replay, unit tests) hold an stf::OwnedBlockCaches.
• Block production (assemble_block, assemble_gloas_block, assemble_gloas_block_on_bid, gloas_payload_inputs) resolves pubkeys through the store's shared table. Its committee and active-balance tables are still fresh for each build, and pack_operations shares one active-balance table across the slashings and exits it tries.
• Metric lean_beacon_pubkey_index_lookups_total{result="hit"|"fallback"}. docs/beacon_stf.md and docs/metrics.md describe both caches.

Correctness / Behavior Guarantees

• Pubkeys are unique within any reachable registry and the registry is append-only along a chain, so a verified hit is the only index of that pubkey. Verification is what makes one process-wide table safe across forks: unfinalized indices can differ between branches, and a key-prefix collision only fails verification. An index written back after a scan is verified again on every later hit, so one recorded on one fork is harmless on another.
• A pubkey absent from the registry (e.g. an EL withdrawal request naming no validator) costs one scan, as before, and records nothing.
• The total active balance cannot change within an epoch (effective balances and activation/exit epochs for the current epoch move only in epoch processing), which is why ActiveBalanceCache was already used for the sync aggregate and attestation rewards. It is keyed by the epoch and the block root at the last slot of the previous epoch, so it is sound for block processing only: active_balance_key's doc lists every caller that passes Some, and epoch processing and ejections pass None.
• Deposit processing (block deposits and electra's pending deposits) still scans the registry (follow-up).

Benchmarks

Offline ethlambda benchmark import replay on ethlambda-5 (Ryzen 9700X), three mainnet corpora (slots 15567200-15567385), 2 rounds, steady state n=164, paired per block against main 012dc4f49:

	main	this PR	change	95% CI
STF per block	156.7 ms	83.6 ms	-73.1 ms (-46.6%)	[-76.6, -70.3]
wall (import + fork choice)	178.2 ms	105.3 ms	-72.9 ms (-40.9%)

blocks	n	main	this PR
no withdrawal/consolidation requests	143	150.7 ms	83.0 ms
withdrawal requests	17	181.3 ms	90.4 ms
consolidation requests	4	265.6 ms	76.4 ms

Live, on the ethlambda-2 mainnet follower (nightly with this change squashed in, gossip imports): sync_aggregate 74.9 -> 1.6 ms, voluntary_exits 55.1 -> 0.2 ms, stf 175.4 -> 103.5 ms.

These numbers were measured before the review commit bbec4ba2 and have not been re-measured. That commit changes the import path only for slashings, switch-to-compounding requests and cache misses.

Tests Added / Run

• New unit tests:
  • 10 in pubkey_index_cache.rs: tail and bulk ingest, the merge at the threshold, covered positions skipped and gaps refused, a racing bulk ingest, equal keys in the base and in the tail, insert (new and already in the base), the 12-byte base.
  • 7 in accessors.rs: hit after ingest, tail ingestion after growth, a wrong index from another fork failing verification, a fallback scan recording its find, an absent pubkey not growing the table, a key-prefix collision resolving both validators, agreement with a registry scan for every validator.
  • 1 in altair.rs: a sync committee seat the cache cannot verify is scanned for and recorded.
• Locally: cargo test --profile release-fast --lib for ethlambda-storage, ethlambda-state-transition, ethlambda-blockchain and ethlambda-rpc, and clippy with the beacon-spec-tests features on.
• CI on bbec4ba2 passes: Test (consensus), Test (node), Test minimal preset, Beacon spec tests (mainnet and minimal, v1.7.0-beta.2 fixtures), Benchmark smoke, Lint.

Related Issues / PRs

• Part of a set of Lighthouse ports from the same profile, each its own PR.

✅ Verification Checklist

• Ran make fmt — clean
• Ran make lint (clippy with -D warnings) — clean
• make test — not run locally; CI's Test (consensus) and Test (node) jobs, its two halves, pass on bbec4ba2

https://claude.ai/code/session_011vghNj5DNZw778LTBJPURC

[MegaRedHand]

Combined result for the four Lighthouse ports (#759 pubkey index, #760 column KZG, #761 progressive target balances, #762 tree-backed participation), squashed onto nightly b1fc143 as 080bd40..1d06bf9.

Offline import replay on ethlambda-5, same three mainnet corpora, both legs built at the commit and run with --warm-committee-sums, 2 rounds, steady state n=164, paired per block:

	nightly b1fc143	+ all four (1d06bf9)	change	95% CI
STF per block	140.7 ms	20.7 ms	-120.0 ms (-85.3%)	[-123.5, -117.3]
wall (import + fork choice)	162.5 ms	43.8 ms	-118.7 ms (-73.0%)	[-122.1, -116.0]

Per STF step, ordinary blocks (n=162): sync_aggregate 71.4 -> 0.9 ms (#759), execution_requests 5.3 -> 0.0 ms (#759), state_root 24.8 -> 4.0 ms (#762), unattributed 28.0 -> 1.3 ms (#761's removed state clone and scan, #762's cheaper clone). Regressions being fixed: attestations +2.9 ms, process_epoch +9.2 ms at epoch boundaries, get_head +1.0 ms; a per-item attribution run is in progress.

Live on the ethlambda-2 mainnet follower (nightly 1d06bf9): steady stf ~30-40 ms per gossip block (nightly before the ports: ~175 ms).

[github-actions]

🤖 Codex Code Review

⚠️ The review job did not complete (result: failure). See the job log

---

Automated review by OpenAI Codex

[github-actions]

🤖 Claude Code Review

I reviewed the pubkey-index cache code in crates/storage/src/pubkey_index_cache.rs and the PubkeyIndexCacheExt impl in helpers/accessors.rs. The checkout is a shallow merge commit with no PR diff available, so I read the current files. I did not compile or run any tests, and I did not check the callers other than fork_choice.rs:1068.

Summary

The design is sound and safe. Every cache candidate is checked against the asking state's own validator at that index (accessors.rs:457-463). A stale, foreign-fork or prefix-colliding entry therefore costs a scan instead of returning the wrong validator. validator_index falls back to the same first-position search the spec uses, so results are identical to the uncached path. I found no consensus-correctness issues.

Findings

1. Doc claim is slightly wrong (pubkey_index_cache.rs:21-26). The module doc says a fork "only ever makes a candidate wrong, never makes a right one disappear". The tail does disappear entries: it is a HashMap<u64, u32> with first writer wins (:127-129). Suppose fork A ingests pubkey P at index 100 and fork B has P at 101. B's lookup then gets candidate 100, fails verification and scans. The result is still correct, only slower, but the comment should say "falls back to a scan".

2. Persistent slow path after a fork-poisoned ingest (accessors.rs:449-456). covered_len only grows, and the table is never rebuilt. If an unfinalized fork ingests a registry tail that differs from the canonical one, the canonical chain's indices at those positions are never re-ingested. They are only ingested if the registry later grows past covered_len. Lookups for those pubkeys then take the O(n) fallback scan indefinitely. This is bounded, because forking a registry needs deposits and candidates stays correct. Still, consider one of:

   • rebuilding or resetting the table when the finalized state advances;
   • ingesting tail entries from the canonical head preferentially;
   • at minimum, watching the lean_beacon_pubkey_index_lookups_total{result="fallback"} rate on a live network.

3. Minor: bulk-load branch (pubkey_index_cache.rs:121-125). batch.extend(inner.tail.drain()) followed by merge is correct, since merge sorts by (key, index). A second start-past-covered_len refusal returns false, and verified_index ignores it (accessors.rs:456). That is harmless because it only skips the cache update. A debug_assert! that the return is true would still document the invariant.

4. Minor: allocation in candidates (:96-107). It returns a Vec<u64> on every lookup, although the common case is one element. A SmallVec or an iterator callback would avoid a heap allocation on the hot path. This is optional.

5. Style. Tests exist for the cache. Locking is held only for the splice, never during a registry scan, which is good. RwLock::unwrap() on poisoning is consistent with the other caches.

The code looks good. Item 1 is a doc fix, and Item 2 is worth a decision before merge.

---

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

[github-actions]

🤖 Kimi Code Review

I'll review this PR which introduces a PubkeyIndexCache to optimize validator index lookups by pubkey, replacing linear scans with cached lookups. Let me analyze the changes across all files.

Overall Assessment

This is a well-structured performance optimization that introduces a process-wide cache for pubkey-to-index lookups. The design correctly handles fork safety by verifying cached candidates against the caller's state. However, I've identified several issues ranging from correctness concerns to code quality improvements.

---

Critical Issues

1. Race condition in PubkeyIndexCache::extend with concurrent readers (crates/storage/src/pubkey_index_cache.rs:175-195)

The extend method releases the write lock between checking covered_len and updating it. Two threads can see the same covered_len, both scan the same tail, and both try to extend. The skip logic handles double-insertion for the base, but there's a subtle issue:

let skip = (inner.covered_len - start).min(keys.len());  // line 178

If thread A and thread B both call extend(0, &keys) when covered_len is 0, both will process all keys. The or_insert in the tail path prevents duplicates for the same key, but for bulk loads (fresh.len() >= TAIL_MERGE_THRESHOLD), the merge will create duplicate entries with the same (key, index) pair.

Impact: Duplicate entries don't affect correctness (verification filters them), but they waste memory and slightly degrade performance.

Fix: Consider deduplicating in merge, or document this as acceptable. For the bulk path specifically, collect into a HashMap first before merging, or use BTreeMap::from_iter with max_by_key to keep the first index.

---

2. Incorrect covered_len update on gap refusal (crates/storage/src/pubkey_index_cache.rs:175-176)

pub fn extend(&self, start: usize, keys: &[u64]) -> bool {
    let mut inner = self.inner.write().unwrap();
    if start > inner.covered_len {
        return false;  // covered_len NOT updated, but what if start < covered_len and we skip some?
    }

Wait—let me re-read. The actual bug is more subtle: when start < covered_len, we compute skip = inner.covered_len - start, but we don't verify that keys[0..skip] actually matches what was previously inserted. A caller could pass arbitrary keys with a stale start value.

Impact: Low—callers are expected to pass correct data, but this is a trust boundary issue.

Suggestion: Add debug assertion or document the precondition more strongly.

---

3. process_sync_aggregate fallback path doesn't verify all pubkeys exist (crates/blockchain/state_transition/src/beacon/stf/altair.rs:355-365)

let cached: Option<Vec<ValidatorIndex>> = committee_pubkeys
    .iter()
    .map(|pubkey| pubkey_indices.verified_index(state, pubkey))
    .collect();

The verified_index returns None for:

• Pubkey not in cache
• Cache candidate fails verification
• Pubkey not in state

This means if any committee member is not in the cache or fails verification, the entire committee falls back to scan. This is correct but suboptimal—could mix cached and scanned results.

However, the real issue: the fallback resolve_sync_committee_by_scan returns Error::SpecAssert if a pubkey is missing, but the original code's error message was checked in tests. The new path preserves this.

Actually, re-checking: this is fine. The Option<Vec<_>> short-circuit on None is intentional and correct.

---

Security Issues

4. No verification that cached index is within bounds before state access (crates/blockchain/state_transition/src/beacon/helpers/accessors.rs:455-460)

fn verified_index(&self, state: &BeaconState, pubkey: &BlsPubkey) -> Option<ValidatorIndex> {
    // ...
    self.candidates(pubkey).into_iter().find(|index| {
        *index < count as u64  // bounds check
            && state
                .validator(*index)  // potential panic if index >= actual validator count?
                .is_ok_and(|validator| validator.pubkey == *pubkey)
    })
}

The *index < count as u64 check is correct, but count comes from state.validator_count(). If there's a discrepancy between state.validator_count() and state.validator(index) (e.g., due to a bug in iter_validators_from or registry internals), this could panic.

Actually, this is fine—the bounds check is correct. But note that count is usize and cast to u64; on 32-bit systems this is fine, on 64-bit systems with >2^64 validators this would wrap. This is theoretical for Ethereum.

---

5. Cache poisoning via PubkeyIndexCache::key_of collision (crates/storage/src/pubkey_index_cache.rs:102-107)

pub fn key_of(pubkey: &BlsPubkey) -> u64 {
    let mut prefix = [0u8; 8];
    prefix.copy_from_slice(&pubkey.0[1..9]);
    u64::from_be_bytes(prefix)
}

The comment claims "eight bytes taken from the uniformly distributed part of a compressed point make a collision between two real keys negligible." This is cryptographically incorrect for an adversarial setting. While BLS pubkeys are points on a curve, the byte representation is not uniformly random—it's a compressed point with specific structure.

Impact in this design: Low, because candidates are verified. But the "negligible" claim is misleading.

Suggestion: Change comment to "collisions are handled by candidate verification; the prefix is not a cryptographic commitment."

---

Performance Issues

6. Suboptimal fallback for sync committee with single missing pubkey (crates/blockchain/state_transition/src/beacon/stf/altair.rs:355-365)

let cached: Option<Vec<ValidatorIndex>> = committee_pubkeys
    .iter()
    .map(|pubkey| pubkey_indices.verified_index(state, pubkey))
    .collect();

If 511 of 512 sync committee members are cached, but one is missing, this falls back to scanning the entire validator set for all 512. The original inverted-loop optimization handled this better.

Suggestion: Consider a hybrid approach—use cached results where available, scan only for missing entries. This is more complex but avoids worst-case regression.

Actually, re-reading the original code: it was one scan for all, building a hash map. The new code is: try all cache hits, or one scan for all. The cache hit case is much faster; the fallback is same as original. This is acceptable given expected hit rates.

---

7. ActiveBalanceCache::default() created per voluntary exit in block production (crates/blockchain/state_transition/src/beacon/block_production.rs:409-415)

let active_balances = ActiveBalanceCache::default();
let voluntary_exits = pack_each(
    &mut scratch,
    candidates.voluntary_exits,
    preset::MAX_VOLUNTARY_EXITS,
    config,
    |state, exit, config| {
        stf::electra::process_voluntary_exit(state, exit, config, &active_balances)
    },
);

The comment says "One table for the whole scratch pass" but this is a fresh ActiveBalanceCache::default(), not a shared one. This means every process_voluntary_exit call will compute total active balance from scratch, not share it.

Wait—re-reading: the closure captures &active_balances, so all exits share the same cache instance. But ActiveBalanceCache::default() starts empty, so the first call populates it and subsequent calls hit. This is correct.

However: the comment at line 406-408 says "the scratch state is at the block's own slot, so its total active balance cannot change between candidates." This is correct reasoning.

---

Code Quality / Rust Best Practices

8. Inconsistent #[allow(clippy::too_many_arguments)] placement (multiple files)

The attribute is added to some functions but not others with the same parameter count. For example:

• bellatrix::process_block: has attribute
• capella::process_block: has attribute
• deneb::process_block: has attribute
• electra::process_block: has attribute
• fulu::process_block: missing attribute (line 64 of fulu.rs)

Actually checking: fulu.rs line 64 does have it. gloas.rs line 100 does not have it but has 8 parameters (state, block, config, committees, active_balances, pubkey_indices, options = 7 + self). Let me recount...

gloas::process_block signature:

pub fn process_block(
    state: &mut BeaconState,
    block: &gloas::BeaconBlock,
    config: &Config,
    committees: &CommitteeCache,
    active_balances: &ActiveBalanceCache,
    pubkey_indices: &PubkeyIndexCache,
    options: super::block::BlockProcessingOptions,
) -> Result<()>

That's 7 parameters + self = 8. The default clippy threshold is 7. This should have the attribute.

Fix: Add #[allow(clippy::too_many_arguments)] to gloas::process_block.

---

9. Unnecessary &* deref in fork_choice.rs (crates/blockchain/state_transition/src/beacon/fork_choice.rs:1068-1069)

let pubkey_cache = store.pubkey_index_cache();
let pubkey_indices = &*pubkey_cache;

This is redundant. Arc<PubkeyIndexCache> implements Deref, so &pubkey_cache or just passing &pubkey_cache works. Actually, &*pubkey_cache is equivalent to pubkey_cache.as_ref() or just &pubkey_cache in most contexts.

Wait—pubkey_index_cache() returns Arc<PubkeyIndexCache>. The function transition_block takes &PubkeyIndexCache. So you need &*arc or arc.as_ref(). The &* is actually idiomatic Rust for this. Never mind.

---

10. Missing Send + Sync bounds on PubkeyIndexCache (`crates/storage/src/pubkey_index_cache.rs:82)

pub struct PubkeyIndexCache {
    inner: RwLock<Inner>,
}

RwLock is Send + Sync when T: Send, and Inner contains Vec and HashMap which are Send + Sync. So PubkeyIndexCache should auto-derive these. But it's stored in Arc in Store, so this is fine.

However: PubkeyIndexCache is Debug + Default. Consider adding explicit Clone? No, it's meant to be shared via Arc.

---

11. Test helper append_validator modifies validator 0's pubkey (crates/blockchain/state_transition/src/beacon/helpers/accessors.rs:1094-1099)

fn append_validator(state: &mut BeaconState, pubkey: BlsPubkey) {
    let mut validator = state.validator(0).unwrap().clone();
    validator.pubkey = pubkey;
    state.push_validator(validator, 0).unwrap();
}

This copies validator 0's entire state including effective balance, exit epoch, etc. This may create invalid states (e.g., duplicate validators with same withdrawal credentials but different pubkeys).

Impact: Tests only, but could mask real issues.

Suggestion: Document this or use a more principled test fixture.

---

Documentation / Comments

12. Inaccurate comment about "12 bytes per validator" (crates/storage/src/pubkey_index_cache.rs:58-60)

The comment says "12 bytes per validator in the sorted base (an 8 byte key and a 4 byte index)". This is correct for the base, but the heap_bytes calculation uses size_of::<u64>() + size_of::<u32>() which is indeed 12. However, Vec has capacity overhead, and HashMap tail entries are much larger.

The test the_base_costs_twelve_bytes_an_entry asserts heap_bytes() == count * 12, but this ignores Vec capacity rounding. Vec::with_capacity(total) may round up.

Check: Vec::with_capacity doesn't typically round up for small capacities, but the allocator might. This test could be flaky.

---

13. Typo in filename (crates/blockchain/state_transition/src/beacon/gloas_block_production.rs)

The filename appears to be gloas_block_production.rs (not gloas_... vs gloas_...). Actually this is pre-existing and consistent with the fork name "Gloas". Not a new issue.

---

Consensus Correctness

14. Epoch processing passes None for active balances (crates/blockchain/state_transition/src/beacon/stf/epoch/electra.rs:223)

Some(RegistryAction::Eject) => initiate_validator_exit(state, index, config, None)?,

This is correct per the design: epoch processing mutates effective balances, so the cache key would be stale. The comment in electra.rs lines 672-684 explains this well.

15. Fork choice cache sharing is correct (`crates/blockchain/state_transition/src/beacon/fork_choice.rs:1063-1069)

The pubkey_cache is shared across all forks via store.pubkey_index_cache(). The verification step ensures that even if fork A and fork B have different validators at the same index, the cache returns only verified candidates. This is correct and safe.

---

Minor Suggestions

16. Use iter().copied() instead of manual map (`crates/storage/src/pubkey_index_cache.rs:140-143)

let mut found: Vec<u64> = inner.keys[start..]
    .iter()
    .zip(&inner.indices[start..])
    .take_while(|(candidate, _)| **candidate == key)
    .map(|(_, index)| u64::from(*index))
    .collect();

Could be:

.map(|&(_, index)| u64::from(index))

But this is minor style.

---

17. Metrics counter name inconsistency (`crates/blockchain/state_transition/src/metrics.rs:84-94)

"lean_beacon_pubkey_index_lookups_total"

vs. existing:

"lean_beacon_total_active_balance_lookups_total"

The naming is slightly inconsistent (total_active_balance vs pubkey_index). Suggest lean_beacon_pubkey_index_lookups_total or rename the other to match. Actually, this is fine as-is.

---

Summary

Severity	Count	Description
Critical	0	No consensus-breaking bugs found
High	1	Potential duplicate entries in bulk extend (Item 1)
Medium	2	Suboptimal sync committee fallback (Item 6), test helper issue (Item 11)
Low	4	Documentation nits, clippy attribute, naming, bounds

The PR is correct and safe from a consensus perspective. The cache verification mechanism properly handles all edge cases (forks, prefix collisions, missing pubkeys). The main concern is the potential for duplicate entries during concurrent bulk extends, which wastes memory but doesn't affect correctness.

---

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

[MegaRedHand]

Per-PR attribution on nightly, where the four ports are stacked in the order #759, #760 + #761, #762. Offline import replay on ethlambda-5, three mainnet corpora, --warm-committee-sums, 2 rounds, steady state n=164, paired per block:

nightly at	STF per block	change vs before the ports	95% CI
b1fc143 (before the ports)	140.1 ms
+ #759 (080bd40)	65.1 ms	-75.0 ms (-53.5%)	[-78.5, -72.3]
+ #760, #761 (33551eb)	39.9 ms	-100.2 ms (-71.5%)	[-103.7, -97.5]
+ #762 (1d06bf9)	20.8 ms	-119.4 ms (-85.2%)	[-122.9, -116.6]
+ follow-ups in #761 and #762 (2b51a33)	19.1 ms

#760 does not run in the replay (no custody columns), so the second step is #761's.

Regressions found by the per-step split, and what the follow-up commits (now pushed to #761 and #762) did about them:

step	from	added	after the follow-ups
attestations, ordinary blocks	#761: second validator read per attester	+1.4 ms	5.8 -> 4.3 ms together (before the ports: 2.9)
attestations, ordinary blocks	#762: per-attester tree read and write	+1.5 ms
process_epoch, epoch boundaries	#762	+9.1 ms	106.2 -> 104.8 ms (before the ports: 97.2)
get_head	the #760 + #761 step	+1.3 ms	unchanged

get_head's code is untouched by either PR; a perf profile puts the extra time in walk_head's own code, so it looks like codegen rather than work. The same profile shows std SipHash in fork choice's vote tally as most of get_head's ~17 ms per block; that is a separate change, not part of these PRs.

ethlambda-2 (mainnet follower) runs nightly 2b51a33 since 2026-11-03 14:42 UTC.