perf(crypto): resolve registry public keys through a validator-index table by MegaRedHand · Pull Request #743 · lambdaclass/ethlambda

MegaRedHand · 2026-10-30T16:07:49Z

Stack 4/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 (c).

What changed

IndexedPubkey { index, pubkey } and a sealed trait PubkeySource, implemented for BlsPubkey (byte path) and IndexedPubkey (index path). verify, fast_aggregate_verify, eth_fast_aggregate_verify, key_validate, eth_aggregate_pubkeys, SignatureSet::single, SignatureSet::fast_aggregate, AggregatePubkey::sum and AggregatePubkey::minus are generic over it, so existing &[BlsPubkey] callers compile unchanged. (aggregate_verify stays byte-only: no caller knows indices there.)
PubkeyCache gains an index table: lazily allocated chunks of 4096 OnceLock<(BlsPubkey, PublicKey)>, bounded by the existing MAX_CACHED_PUBKEYS. Reads take no lock and no hash. Entries are set once; the first writer wins.
A lookup hits the table only when the stored bytes equal the bytes the caller read from its state. A mismatch, an empty slot, or an index past the bound goes to the byte-keyed map. A key resolved by index is stored in the table, not the map. A key that loses the race for its slot goes to the map. A key first validated through the byte path and later looked up by index is copied into its empty slot (set-once, first writer wins) and also stays in the map, since byte-path callers cannot see the table and would otherwise validate it again. Such a key is held twice, which is why every caller that reads a key from the registry by index passes the index (gossip included, below). Invalid keys are cached in neither structure.
The module documentation's keying paragraph now covers both structures and why the byte comparison keeps the argument for byte keys: an index names whatever key the caller's state says, so it is never trusted alone.
Call sites switched to IndexedPubkey:
- state transition: the three indexed_attestation_signature_set_with_domain builders, indexed_payload_attestation_signature_set, attesters_aggregate_pubkey (and so attestation_signature_set), warm_key_sums, randao, the proposer block signature, proposer slashing headers, and voluntary exits (phase0, deneb, electra);
- gossip: aggregator selection proof and signature, subnet attestations, data column sidecar headers, voluntary exits, proposer slashings, payload attestations, proposer preferences, sync committee messages and contribution aggregators, and the block precheck;
- Beacon API: submitted attestations and the gloas proposal's RANDAO reveal check.
Left on the byte path: deposits, BLS-to-execution changes (from_bls_pubkey is not a registry key), the sync aggregate and sync contributions (keys come from the sync committee's own key list), gloas builder keys (builder registry, not validator).
Metrics: lean_beacon_pubkey_cache_entries gains a structure label (map, table), and the new lean_beacon_pubkey_cache_index_chunks counts allocated table chunks, which is what the table's memory follows.

Why

Pubkey lookups through the sharded SipHash map cost 0.5-0.7x of the block's pairing (measured on a heavily loaded M3, ratios only; absolutes to be taken on a quiet x86 host). The index table replaces hash + read lock + probe with one array read and a 48-byte compare.

Memory

	Payload	Overhead	Cost
Byte map (hashbrown)	48 + 96 = 144 B	1 control byte per bucket, load factor 7/8 at most	per key held: 166 B at the 7/8 peak, up to 331 B just after a resize
Index table	144 B	`Once` state, padded to alignment 8	152 B per registry index, filled or not

The table's cost follows the registry's index span, not the number of keys in it: a chunk is allocated whole on its first key, and exited validators are spread through every chunk. On a registry of about 2.4M indices with about 1.2M active, every chunk is allocated, about 350 MiB, against about 290 MiB for the map holding the active keys on the parent branch: about +57 MiB. If that is too much, slots holding a pointer to a boxed entry would cut the empty slots to 16 B (about 230 MiB in total) at the price of one pointer chase per hit.

An earlier revision of this PR left gossip on the byte path. Gossip then validated every key import had already put in the table a second time and held it in both structures, so the map refilled with nearly every active key within about a day, about doubling the cache. Every registry read now passes its index.

Behavior notes

No input accepted before is rejected or the other way round. Spec fixtures reuse indices with different keys across states in one process, so the full spec run exercises the mismatch fallback continuously.

Tests

cargo test -p ethlambda-crypto --profile release-fast --lib (new: a byte-path key is copied into the table on its first index lookup, hit after warm and not in the map, different key at a warmed index falls back and verifies, index past the bound, a cold lookup allocates nothing, invalid keys cached nowhere, concurrent readers and writers agree with key_validate).
cargo test -p ethlambda-state-transition --profile release-fast --lib --features beacon-spec-tests: 658 passed.
beacon_spec_tests filters: /attestation/ 356, /sync_aggregate/ 182, sanity/ 595, random/ 1969 passed (175 ignored); full unfiltered: 7493 passed, 977 ignored, 0 failed.
cargo fmt --all; clippy -D warnings clean on ethlambda-crypto and ethlambda-state-transition.
cargo clippy --profile release-fast --workspace --all-targets -- -D warnings: clean (rpc, p2p and blockchain compile against the generic signatures).

Rerun after the review fixes, on the stack's tip (all four PRs, --profile release-fast): ethlambda-crypto --lib 47 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; full beacon_spec_tests 7493 passed, 977 ignored, 0 failed; make lint clean, also on a trial merge of current main.

…table Every signature check resolved each signer through a sharded SipHash map keyed by the 48 compressed bytes: a hash, a read lock and a probe per key. Lookups cost 0.5-0.7x of the block's pairing (ratios on a loaded M3). Callers that read a key from the registry now pass an IndexedPubkey. The cache first reads a set-once slot by index, with no lock and no hash, and trusts it only when the bytes stored in the slot equal the bytes the caller read. A mismatch (another fork or spec-fixture state naming a different key at that index), an empty slot, or an index past the bound falls back to the byte-keyed map, so the soundness argument of keying by bytes is unchanged. A key resolved by index is stored in the table and not the map, which keeps the per-key memory at or below the map's. The key-taking functions are generic over a sealed PubkeySource, so callers holding plain BlsPubkey slices compile unchanged.

… lookup Registry keys are often first seen through byte-path callers (subnet attestations, sync committee messages). Without promotion, every later index lookup of such a key missed the table and paid the map lookup too, so the table never filled for them. An index lookup that finds its slot empty and then hits the map with the same bytes now copies the point into the slot (set-once, first writer wins). The map keeps its entry, since byte-path callers cannot see the table and would have to validate again; such a key is held twice.

…ache

…table (#743) perf(crypto): resolve registry public keys through a validator-index table Every signature check resolved each signer through a sharded SipHash map keyed by the 48 compressed bytes: a hash, a read lock and a probe per key. Lookups cost 0.5-0.7x of the block's pairing (ratios on a loaded M3). Callers that read a key from the registry now pass an IndexedPubkey. The cache first reads a set-once slot by index, with no lock and no hash, and trusts it only when the bytes stored in the slot equal the bytes the caller read. A mismatch (another fork or spec-fixture state naming a different key at that index), an empty slot, or an index past the bound falls back to the byte-keyed map, so the soundness argument of keying by bytes is unchanged. A key resolved by index is stored in the table and not the map, which keeps the per-key memory at or below the map's. The key-taking functions are generic over a sealed PubkeySource, so callers holding plain BlsPubkey slices compile unchanged. perf(crypto): copy byte-path keys into the index table on first index lookup Registry keys are often first seen through byte-path callers (subnet attestations, sync committee messages). Without promotion, every later index lookup of such a key missed the table and paid the map lookup too, so the table never filled for them. An index lookup that finds its slot empty and then hits the map with the same bytes now copies the point into the slot (set-once, first writer wins). The map keeps its entry, since byte-path callers cannot see the table and would have to validate again; such a key is held twice. Cherry-picked from perf/index-keyed-pubkey-cache at a2156d3.

Gossip and the Beacon API still resolved registry keys through the byte-keyed map, so every key import had already put in the index table was validated a second time on its first gossip sighting and then held in both structures: within a day the map refilled with nearly every active key, about doubling the cache's memory. Every site that reads a key from the registry by index now passes the index, leaving the byte path to keys with no registry index. The entries gauge now says which structure holds a key, a new gauge counts the table's allocated chunks (its memory follows those, not its entries), and the docs no longer claim the table costs at most what the map did.

…ache

github-actions · 2026-10-30T21:44:06Z

🤖 Claude Code Review

Review of PR 743: index-keyed pubkey cache

The change looks sound. I read the diff only and did not build it or run the tests.

Correctness: no blocking issues

The index is only a hint. PubkeyCache::get hits the table only when the slot's stored 48 bytes equal the bytes the caller read from its own state. A point is therefore always the key_validate result for those exact bytes, even when two forks or two spec-fixture states disagree about an index. The module doc states this argument well.
Slots are OnceLocks that are written once and never replaced, so readers never see a torn entry.
When a racing writer holds different bytes, insert falls back to the map. The Err(_) if slot.get()... arm covers a racing writer with the same bytes.
Invalid keys are never inserted in either structure, and the tests cover that for the default key and the identity key.
The index_bound check uses usize::try_from(...).filter(< bound), so u64::MAX and 32-bit targets can't index out of range. chunk_cell indexes chunks[index / INDEX_CHUNK_LEN], which is safe because chunks.len() == ceil(capacity / CHUNK).
A lookup never allocates, and a chunk is allocated only after a key validates, so unvalidated attacker-supplied keys can't grow the table. Indices come from the node's own registry, not from the wire.
The sealed PubkeySource trait with the generic K: PubkeySource signatures is clean. The call-site changes pair each registry-read key with its index consistently.

Minor points

bls.rs, PubkeyCache::get: on a byte-map hit with an index, the code calls slot_or_allocate and slot.set. If the slot already holds other bytes (a cross-fork index collision), every such lookup repeats a failed set. It also allocates a chunk that can stay mostly empty. This is rare and cheap, so it is acceptable. You could skip the promotion when slot.get().is_some(), which would avoid the failed set.
Memory: a chunk is 4096 slots, and each slot is roughly 48 bytes of key, 96 bytes of point and the OnceLock state. A chunk is therefore about 0.6 MB, and the cap of 4M indices allows about 650 MB in the worst case. On a mainnet-sized registry the table is about 170 MB. The module doc already admits it costs more than a map when many validators have exited, and the new index_chunks gauge covers that. No change needed.
Metrics: lean_beacon_pubkey_cache_entries gained a structure label. Any dashboard or alert that reads the unlabeled series needs a sum(...). Mention this in the PR notes if the Grafana boards live in the repo.
Tests: a_different_key_at_a_warmed_index_falls_back_and_verifies and the concurrency test use fixed indices in the global VALIDATED_PUBKEYS (3_000_001, 3_100_000.., 3_200_000.., and MAX-1). They don't collide today. A short comment saying that these ranges must stay disjoint would help future test authors.

Overall this is good to merge once you've considered Point 3.

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

github-actions · 2026-10-30T21:44:29Z

🤖 Kimi Code Review

I'll review this PR which introduces an index-keyed pubkey cache to optimize BLS validation performance. This is a significant optimization for consensus-layer hot paths.

Overall Assessment

The PR introduces a two-tier pubkey cache (index table + byte-keyed map) to avoid hashing for registry keys. The design is sound but I have several concerns about edge cases, metrics correctness, and potential race conditions.

Critical Issues

1. Race condition in `get()` promotion path (`crates/common/crypto/src/bls.rs:355-365`)

if let Some(index) = key.index()
    && let Some(slot) = self.slot_or_allocate(index)
    && slot.set((*pubkey, point)).is_ok()
{
    crate::metrics::inc_pubkey_cache_table_entries();
}

Problem: The promotion from byte-map to table uses slot.set() which fails if the slot already holds any value. However, the code doesn't distinguish between:

Slot already holds the same bytes (idempotent, should be fine)
Slot holds different bytes (correctly rejected, but then we don't increment metrics)

More critically: if two threads race to promote the same byte-map key, one wins and increments table_entries, the other silently loses. The metrics could undercount, but worse—the losing thread returns Some(point) without ensuring the table actually has the entry. This is actually fine functionally (the point is valid), but the get() method's comment says "A key first seen through the byte path... is copied into its empty slot"—this isn't guaranteed under race.

Fix: The race is benign for correctness but the metrics skew is real. Consider using compare_exchange pattern or accepting that table_entries metric is approximate.

2. Missing `Sync` bound on `K` in `validated_pubkeys` (`crates/common/crypto/src/bls.rs:443`)

fn validated_pubkeys<K: PubkeySource>(pubkeys: &[K]) -> Option<Vec<PublicKey>> {

Problem: The PubkeySource trait requires Sync via sealed::Sealed: Sync, but validated_pubkeys sends &K across rayon parallel iterators. The K: PubkeySource bound implies K: sealed::Sealed which implies Sync, so this is technically correct. However, validated_pubkey<K: PubkeySource> at line 432 doesn't have the &K cross-thread issue since it's single-threaded.

Verification needed: Confirm BlsPubkey and IndexedPubkey are both Sync—they are (BlsPubkey is a wrapper around fixed array, IndexedPubkey is Copy). This is fine.

3. Potential integer overflow in `index_bound` calculation (`crates/common/crypto/src/bls.rs:303`)

index_bound: capacity,  // capacity is usize, but ValidatorIndex is u64

Problem: MAX_CACHED_PUBKEYS is 1 << 22 (~4M), well within u64. But PubkeyCache::new takes capacity: usize. On 32-bit systems, usize is u32, and while 4M fits, the index_bound field stores usize but ValidatorIndex is u64. The chunk_cell method does:

let index = usize::try_from(index).ok().filter(|&i| i < self.index_bound)?;

This correctly handles the conversion, but the struct field type mismatch is slightly awkward. Not a bug, but worth noting for portability.

Security Concerns

4. Cache poisoning via index collision across forks — Addressed correctly, but verify

The design document correctly identifies that index-based lookup must verify bytes match. The implementation at line 343-347:

if let Some(index) = key.index()
    && let Some((bytes, point)) = self.slot(index).and_then(OnceLock::get)
    && bytes == pubkey
{
    return Some(*point);
}

This is correct and secure. The bytes comparison prevents a stale cached key from being used after a validator rotation or fork. The OnceLock provides publication safety.

However, I want to flag: what about the insert path when a slot is already occupied with different bytes?

Err(_) if slot.get().is_some_and(|(bytes, _)| *bytes == pubkey) => return,
Err(_) => {}  // falls through to map

This correctly handles the case where the same bytes are already present (idempotent) vs. different bytes (fallback to map). The security argument holds.

Performance Issues

5. Double lookup in `get()` for index misses (`crates/common/crypto/src/bls.rs:343-365`)

if let Some(index) = key.index()
    && let Some((bytes, point)) = self.slot(index).and_then(OnceLock::get)
    && bytes == pubkey
{
    return Some(*point);
}
let point = self.get_by_bytes(pubkey)?;
// ... then try to promote to table

Problem: For an index miss (first time seeing this validator), we:

Check the table (cache miss)
Check the byte map (cache hit or miss)
If byte map hit, try to promote to table (allocation + OnceLock::set)

This is correct but the promotion attempt on every index miss adds work. For a cold cache, every validator's first signature check pays: table miss → map lookup → table allocation attempt.

Optimization opportunity: The slot_or_allocate call at line 358 allocates a chunk if needed. For a large registry (1M validators), this could trigger many chunk allocations under load. Consider pre-allocating chunks based on expected registry size, or batch-warming the cache.

6. `get_by_bytes` still called even when index table will be used — Unnecessary work in some paths

In get(), when key.index() is Some but the slot is empty or has wrong bytes, we fall through to get_by_bytes. This is correct but means the byte-map is consulted for every indexed key that hasn't been seen before. The byte-map requires hashing + lock acquisition on a shard.

Mitigation: The comment acknowledges this tradeoff: "gossip validation included: the byte path is left to keys with no registry index." The design intentionally accepts this for simplicity.

Code Quality & Rust Idioms

7. Inconsistent `pubkey` variable naming shadows the trait method (multiple files)

In crates/blockchain/state_transition/src/beacon/gossip/aggregate.rs:390-395:

let aggregator_pubkey = bls::IndexedPubkey { ... };

But in crates/blockchain/state_transition/src/beacon/gossip/attestation.rs:193-197:

let attester = bls::IndexedPubkey { ... };  // shadows "attester" from earlier

The variable attester was previously bound to state.validator(attester_index) (a Validator struct). Now it's shadowed with an IndexedPubkey. This is legal Rust but confusing—readers must track types carefully.

Suggestion: Use consistent naming like attester_pubkey or indexed_attester to avoid shadowing.

8. `precheck.rs`: `pubkey` is now `IndexedPubkey` but named `pubkey` (`crates/blockchain/state_transition/src/beacon/precheck.rs:114-128`)

let pubkey = match (state.validator(proposer), reference) {
    (Ok(validator), _) => bls::IndexedPubkey { ... },
    ...
};
if !bls::verify(&pubkey, ...)  // was &validator.pubkey, now &pubkey

The variable name pubkey now holds an IndexedPubkey, not a BlsPubkey. This is misleading. Rename to indexed_pubkey or proposer_key.

9. Missing `Copy` derive on `IndexedPubkey`? — Actually present, good

Verified: #[derive(Clone, Copy, Debug, PartialEq, Eq)] is on IndexedPubkey. This is important for performance since these are passed by reference and copied into closures.

Consensus Correctness

10. No consensus behavior change — purely optimization

I've verified that every call site changes from passing &validator.pubkey (a &BlsPubkey) to passing &IndexedPubkey { index, pubkey } (a &IndexedPubkey). Both implement PubkeySource, and the verify/SignatureSet::single/fast_aggregate_verify functions are generic over K: PubkeySource.

The actual BLS verification logic is unchanged. The cache lookup behavior changes (index path vs. byte path), but:

Cache hits return the same PublicKey point
Cache misses validate and insert the same way
The bytes == pubkey check ensures no stale index entries are used

This is a pure optimization with no consensus impact.

Testing

11. Excellent test coverage for the new cache

The tests added in crates/common/crypto/src/bls.rs cover:

Table warming and map exclusion (an_indexed_key_hits_the_table_after_warming)
Cross-fork key collision (a_different_key_at_a_warmed_index_falls_back_and_verifies)
Cold lookup allocation behavior (a_cold_lookup_allocates_nothing)
Byte-path promotion (a_byte_path_key_is_copied_into_the_table_on_its_first_index_lookup)
Index bound behavior (an_index_past_the_bound_uses_the_map)
Invalid key rejection (an_invalid_indexed_key_is_cached_nowhere)
Concurrent safety (concurrent_readers_and_writers_agree_with_key_validate)

One gap: No test for the fast_aggregate_verify path with mixed IndexedPubkey and bare BlsPubkey in the same call. The API doesn't support this (homogeneous slices), which is correct.

Documentation

12. Metrics documentation update is correct but has typo in original

The docs/metrics.md update correctly documents the new metrics. Note there's a pre-existing typo in the file: "ethlambda-specific, not part of the leanMetrics spec" — should this be "beacon" or is "leanMetrics" intentional? Not introduced by this PR.

Minor Suggestions

13. `gloas_proposal.rs`: Error message could be more specific

let pubkey = bls::IndexedPubkey {
    index: proposer,
    pubkey: state.validator(proposer)
        .map_err(|_| ApiError::Internal("the proposer is not in the registry"))?
        .pubkey,
};

The error says "proposer is not in the registry" but the validator lookup failure could also mean the index is out of bounds. Consider "proposer index is invalid or not in the registry".

Summary

Severity	Count	Items
Critical	0	—
High	0	—
Medium	2	Item 1 (race/metrics skew), Item 5 (promotion overhead)
Low	4	Items 7-9 (naming), Item 13 (error message)
Info	2	Item 3 (portability), Item 11 (test gap)

The PR is well-designed and correct for consensus safety. The two-tier cache design with bytes verification on index lookup is the right approach. Main actionable items are code clarity (naming) and acknowledging that the table_entries metric may undercount under race.

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

github-actions · 2026-10-30T21:44:31Z

🤖 Codex Code Review

Looks good overall. The core change — switching single-signer verification call sites from raw BlsPubkey to IndexedPubkey so they use the validator-index table first — is sound and consistent with the cache design in crates/common/crypto/src/bls.rs:357 and crates/common/crypto/src/bls.rs:383.

Correctness crates/common/crypto/src/bls.rs:357
- The get() path still defends against fork/state mismatches by requiring stored bytes to equal the caller’s bytes before using an indexed slot. That preserves consensus safety when the same validator index maps to different pubkeys across states.
Concurrency / memory safety crates/common/crypto/src/bls.rs:339, crates/common/crypto/src/bls.rs:388
- The OnceLock chunk allocation and per-slot first-writer-wins logic look race-safe. I don’t see torn-read or overwrite risks.
Performance crates/blockchain/state_transition/src/beacon/gossip/aggregate.rs:390, crates/blockchain/state_transition/src/beacon/gossip/attestation.rs:193, crates/blockchain/state_transition/src/beacon/gossip/operations.rs:251
- These hot-path validators now correctly hit the index table instead of hashing into the byte map, which should reduce lookup overhead on gossip and RPC verification paths.
Consensus-layer impact
- No changes to fork choice, justification/finalization, attestation semantics, or SSZ logic.
- Signature-domain handling at the touched call sites remains unchanged, which is the key consensus-sensitive part here.
Metrics / observability crates/common/crypto/src/metrics.rs:22, docs/metrics.md:557
- Splitting cache-entry metrics into map vs table and adding index_chunks matches the new storage model and makes memory behavior more transparent.

I did not find a concrete bug or security issue in the diff. If you want, I can do a second pass focused only on adversarial-state / malformed-signature edge cases in the new pubkey-cache tests.

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

…table (#743): review fixes perf(beacon): look up registry keys by index on the gossip and API paths Gossip and the Beacon API still resolved registry keys through the byte-keyed map, so every key import had already put in the index table was validated a second time on its first gossip sighting and then held in both structures: within a day the map refilled with nearly every active key, about doubling the cache's memory. Every site that reads a key from the registry by index now passes the index, leaving the byte path to keys with no registry index. The entries gauge now says which structure holds a key, a new gauge counts the table's allocated chunks (its memory follows those, not its entries), and the docs no longer claim the table costs at most what the map did. Updates nightly's #743 from perf/index-keyed-pubkey-cache at a2156d3 to f733e97.

MegaRedHand added 3 commits October 30, 2026 12:22

Merge branch 'perf/committee-key-sums' into perf/index-keyed-pubkey-c…

a2156d3

…ache

MegaRedHand added this pull request to stack #744 October 30, 2026 16:08

MegaRedHand added 2 commits October 30, 2026 18:23

Merge branch 'perf/committee-key-sums' into perf/index-keyed-pubkey-c…

f733e97

…ache

MegaRedHand marked this pull request as ready for review October 30, 2026 21:43

Navigation Menu

Repository navigation

perf(crypto): resolve registry public keys through a validator-index table - #743

perf(crypto): resolve registry public keys through a validator-index table#743
MegaRedHand wants to merge 5 commits into
perf/committee-key-sumsfrom
perf/index-keyed-pubkey-cache

MegaRedHand commented Oct 30, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Oct 30, 2026

Uh oh!

github-actions Bot commented Oct 30, 2026

Uh oh!

github-actions Bot commented Oct 30, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

MegaRedHand commented Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed

Why

Memory

Behavior notes

Tests

Uh oh!

github-actions Bot commented Oct 30, 2026

🤖 Claude Code Review

Review of PR 743: index-keyed pubkey cache

Uh oh!

github-actions Bot commented Oct 30, 2026

🤖 Kimi Code Review

Overall Assessment

Critical Issues

1. Race condition in get() promotion path (crates/common/crypto/src/bls.rs:355-365)

2. Missing Sync bound on K in validated_pubkeys (crates/common/crypto/src/bls.rs:443)

3. Potential integer overflow in index_bound calculation (crates/common/crypto/src/bls.rs:303)

Security Concerns

4. Cache poisoning via index collision across forks — Addressed correctly, but verify

Performance Issues

5. Double lookup in get() for index misses (crates/common/crypto/src/bls.rs:343-365)

6. get_by_bytes still called even when index table will be used — Unnecessary work in some paths

Code Quality & Rust Idioms

7. Inconsistent pubkey variable naming shadows the trait method (multiple files)

8. precheck.rs: pubkey is now IndexedPubkey but named pubkey (crates/blockchain/state_transition/src/beacon/precheck.rs:114-128)

9. Missing Copy derive on IndexedPubkey? — Actually present, good

Consensus Correctness

10. No consensus behavior change — purely optimization

Testing

11. Excellent test coverage for the new cache

Documentation

12. Metrics documentation update is correct but has typo in original

Minor Suggestions

13. gloas_proposal.rs: Error message could be more specific

Summary

Uh oh!

github-actions Bot commented Oct 30, 2026

🤖 Codex Code Review

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

MegaRedHand commented Oct 30, 2026 •

edited

Loading

1. Race condition in `get()` promotion path (`crates/common/crypto/src/bls.rs:355-365`)

2. Missing `Sync` bound on `K` in `validated_pubkeys` (`crates/common/crypto/src/bls.rs:443`)

3. Potential integer overflow in `index_bound` calculation (`crates/common/crypto/src/bls.rs:303`)

5. Double lookup in `get()` for index misses (`crates/common/crypto/src/bls.rs:343-365`)

6. `get_by_bytes` still called even when index table will be used — Unnecessary work in some paths

7. Inconsistent `pubkey` variable naming shadows the trait method (multiple files)

8. `precheck.rs`: `pubkey` is now `IndexedPubkey` but named `pubkey` (`crates/blockchain/state_transition/src/beacon/precheck.rs:114-128`)

9. Missing `Copy` derive on `IndexedPubkey`? — Actually present, good

13. `gloas_proposal.rs`: Error message could be more specific