Simplify signature verification and document gossip counter safety

[pablodeymo]

Motivation

Two findings from a full codebase review:

1. Redundant iteration in signature verification -- verify_all_signatures collected all validator IDs into a Vec, iterated it for bounds checking, then iterated it again to collect public keys. This runs on every block verification (hot path), allocating an unnecessary intermediate Vec<u64> per attestation.

2. Undocumented safety assumption on gossip_signatures_count -- The AtomicUsize counter tracks GossipSignatures table entries for Prometheus metrics. The insert_gossip_signature method uses a read-then-write pattern (check if key exists, then conditionally increment) that has a TOCTOU gap. This is safe today because all Store mutations run on the BlockChain actor's single thread, but the Arc<AtomicUsize> wrapping suggests the Store is designed to be shared -- making the assumption non-obvious to future contributors.

Description

• Combine validator ID collection, bounds checking, and public key lookup into a single iterator pass in verify_all_signatures, removing one Vec allocation and one full iteration per attestation
• Add a comment on the gossip_signatures_count field documenting the single-threaded safety assumption and the risk if concurrent writes are ever introduced

How to Test

• make test -- no behavioral change, all tests pass
• make lint -- clippy clean

[github-actions]

🤖 Claude Code Review

The PR is small and clean. Here's the review:

---

PR Review: Simplify signature verification and document gossip counter safety

Overall: LGTM. Two focused improvements — a hot-path optimization and a safety comment. Both changes are correct and well-motivated.

---

Change 1 — Single-pass validator bounds check + pubkey collection (crates/blockchain/src/store.rs)

Correctness: The refactor is semantically equivalent. The old code did:

1. Collect all validator IDs into Vec<u64>
2. Scan for out-of-bounds IDs → early return on first bad one
3. Collect pubkeys (in a second pass)

The new code does the bounds check inline during .map(), propagating the first error via collect::<Result<_,_>>()?. The error-on-first-bad-index behavior is preserved because collect on a Result-typed iterator short-circuits on the first Err.

Performance: Removes one Vec<u64> allocation and one full iteration per attestation. On the block-verification hot path (called for every incoming block), this is a real gain. The public-keys Vec is still allocated, but that's unavoidable since verify_aggregated_signature takes owned keys.

Minor observation: The .map(|vid| { if ... return Err(...); ... }) closure uses an early-return inside a closure, which is idiomatic but could alternatively be written as:

.map(|vid| {
    (vid < num_validators)
        .then_some(vid)
        .ok_or(StoreError::InvalidValidatorIndex)
        .and_then(|v| validators[v as usize].get_pubkey()
            .map_err(|_| StoreError::PubkeyDecodingFailed(v)))
})

This is purely stylistic — the current form is fine and arguably more readable.

---

Change 2 — Document TOCTOU assumption on gossip_signatures_count (crates/storage/src/store.rs)

Accuracy: The comment correctly identifies the risk. The pattern at lines 1101–1118 is:

read → check is_new → write → conditionally increment

There is no atomic CAS here; the safety relies entirely on single-threaded mutation. The comment makes this contract explicit, which is the right thing to do given that Arc<AtomicUsize> structurally implies shared-state intent.

One small suggestion: The comment mentions "the read-then-write in insert_gossip_signature" — it would be marginally clearer to also mention the specific ordering used (Ordering::Relaxed) and that the relaxed ordering is also only safe under the single-threaded assumption, since relaxed provides no cross-thread synchronization guarantee. Not blocking — this is a docs-level nit.

---

Summary: Both changes are correct, the optimization is meaningful on a hot path, and the safety documentation reduces future contributor risk. No issues found.

---

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

[github-actions]

🤖 Codex Code Review

No findings.

The functional change in store.rs looks correct: it still rejects out-of-bounds validator IDs before indexing validators, preserves the existing PubkeyDecodingFailed path, and removes one intermediate Vec<u64> allocation / extra pass over aggregation_bits. I don’t see a fork-choice, attestation-validation, XMSS verification, or SSZ correctness regression in that hunk.

The change in store.rs is comment-only.

I could not run cargo tests in this sandbox because the toolchain tries to write under /home/runner/.rustup/tmp, which is read-only here.

---

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

[greptile-apps]

Greptile Summary

This PR makes two small, independent improvements identified during a codebase review: a performance micro-optimisation in the hot-path signature verifier and a clarifying safety comment on a counter field.

crates/blockchain/src/store.rs – single-pass validator check

The original verify_signatures loop allocated an intermediate Vec<u64> from validator_indices, iterated it once for the bounds check, then iterated it again to collect public keys. The new code chains the bounds check directly into the same .map() closure and collects in one pass, removing the intermediate allocation and the extra scan. The semantics are identical: the first out-of-bounds index short-circuits via collect::<Result<_, _>>() and ? just as before. The use of return Err(...) inside the closure is idiomatic — it returns from the closure (not the outer function), producing an Err value that the collect call then propagates.

crates/storage/src/store.rs – document TOCTOU safety assumption

insert_gossip_signature uses a read-then-write pattern to conditionally increment gossip_signatures_count. The Arc<AtomicUsize> wrapping makes the field look safe for concurrent access, but the check-then-act sequence around the counter is only race-free because all Store mutations run on a single actor thread. The new doc comment captures this invariant in-place and warns future contributors of the risk if that threading model ever changes.

Confidence Score: 5/5

Safe to merge — both changes are non-behavioural: a semantically equivalent iterator refactor and a documentation-only addition.

No logic changes: the single-pass refactor produces the same errors in the same order and removes an allocation with no observable side effects. The comment addition has no runtime impact. No P0/P1 findings.

No files require special attention.

Important Files Changed

Filename	Overview
crates/blockchain/src/store.rs	Combines bounds check and public key collection into a single iterator pass in verify_signatures, removing one intermediate Vec<u64> allocation and one full iteration per attestation; logic is semantically identical to the original.
crates/storage/src/store.rs	Adds a documentation comment to the gossip_signatures_count field explaining its single-threaded safety assumption and the TOCTOU risk if concurrent writes are introduced; no behavioral change.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[verify_signatures called] --> B[For each attestation + proof pair]
    B --> C{aggregation_bits == participants?}
    C -- No --> D[Err: ParticipantsMismatch]
    C -- Yes --> E[validator_indices iterator]
    E --> F["For each vid (single pass)"]
    F --> G{vid >= num_validators?}
    G -- Yes --> H[Err: InvalidValidatorIndex]
    G -- No --> I[validators vid .get_pubkey]
    I --> J{Pubkey decoded?}
    J -- No --> K[Err: PubkeyDecodingFailed]
    J -- Yes --> L[Collect into public_keys Vec]
    L --> M{More indices?}
    M -- Yes --> F
    M -- No --> N[verify_aggregated_signature]
    N --> O{Valid?}
    O -- No --> P[Err: AggregateVerificationFailed]
    O -- Yes --> Q{More attestations?}
    Q -- Yes --> B
    Q -- No --> R[Verify proposer signature]
    R --> S[Ok]

Loading

_{Reviews (1): Last reviewed commit: "Simplify signature verification and docu..." | Re-trigger Greptile}

[MegaRedHand]

LGTM