Filter gossip signatures by attestation subnet membership

[pablodeymo]

Motivation

A spec-to-code compliance audit (F-04) identified that gossip signatures were being stored unconditionally, without checking whether the attester belongs to the same attestation subnet as the local validator. The leanSpec reference implementation filters signatures by subnet in two places:

1. Gossip attestation path (store.py:385-392):

if is_aggregator:
    current_subnet = self.validator_id.compute_subnet_id(ATTESTATION_COMMITTEE_COUNT)
    attester_subnet = validator_id.compute_subnet_id(ATTESTATION_COMMITTEE_COUNT)
    if current_subnet == attester_subnet:
        new_committee_sigs.setdefault(attestation_data, set()).add(...)

2. Proposer signature in on_block (store.py:605-612):

if self.validator_id is not None:
    proposer_subnet_id = proposer_attestation.validator_id.compute_subnet_id(ATTESTATION_COMMITTEE_COUNT)
    current_validator_subnet_id = self.validator_id.compute_subnet_id(ATTESTATION_COMMITTEE_COUNT)
    if proposer_subnet_id == current_validator_subnet_id:
        new_gossip_sigs.setdefault(...).add(...)

The subnet ID is computed as validator_id % ATTESTATION_COMMITTEE_COUNT (validator.py:31-40).

Why this has no impact today

ATTESTATION_COMMITTEE_COUNT = 1 — every validator maps to subnet 0 (validator_id % 1 = 0), so the filter is always true. This PR introduces zero behavioral change.

Why it matters for forward compatibility

If ATTESTATION_COMMITTEE_COUNT increases (e.g., to 4 with 1000 validators):

Concern	Without filter	With filter
Gossip signatures stored	All N (1000)	Only N/C (250)
Memory per attestation round	~3MB (1000 × 3KB XMSS sigs)	~750KB
leanVM aggregation cost at interval 2	4x more signatures to prove	Only same-subnet sigs
Correctness	Valid but wasteful — redundant cross-subnet coverage	Matches spec partitioning

The aggregation cost is the real concern — leanVM proving is the most expensive operation in the system, and aggregating 4x more signatures would directly multiply proving time.

Description

New constant and helper

• ATTESTATION_COMMITTEE_COUNT = 1 — matches spec
• compute_subnet_id(validator_id) — validator_id % ATTESTATION_COMMITTEE_COUNT with #[allow(clippy::modulo_one)] since the constant is 1 today

Signature changes

on_gossip_attestation now takes local_validator_ids: &[u64]:

• After signature verification, checks if the attester's subnet matches any local validator's subnet
• Only stores the gossip signature if there's a match
• Attestation data is still stored unconditionally (fork choice needs it regardless)

on_block / on_block_core now takes local_validator_ids: &[u64]:

• The proposer's gossip signature is only stored if the proposer's subnet matches a local validator's subnet
• on_block_without_verification passes &[] (test-only path, no signatures stored anyway)

BlockChainServer (call sites in lib.rs):

• Both process_block and on_gossip_attestation now pass self.key_manager.validator_ids() to the store functions

Test updates

• signature_spectests.rs: passes &[] to on_block (tests don't have local validators, and with ATTESTATION_COMMITTEE_COUNT=1 all subnets match anyway)

How to test

make fmt    # passes
make lint   # passes (clippy with -D warnings)
make test   # all 112 tests pass

Test plan

• make fmt passes
• make lint passes
• make test passes (all 112 tests, 0 failures)
• Devnet test: verify blocks are still produced and finalized normally
• When ATTESTATION_COMMITTEE_COUNT is increased, verify only same-subnet signatures are stored

[github-actions]

🤖 Kimi Code Review

Review Summary

The PR introduces subnet-based filtering for gossip signatures based on validator committee assignments. While the approach is sound, several issues need attention:

Critical Issues

1. Hard-coded committee count (store.rs:30-31): ATTESTATION_COMMITTEE_COUNT = 1 makes the subnet filtering ineffective since compute_subnet_id always returns 0. This defeats the purpose of subnet filtering entirely.

2. Inconsistent empty slice in on_block_without_verification (store.rs:540): Passing &[] for local_validator_ids means the proposer signature will never be stored when verification is skipped, which could break block building functionality.

Security & Correctness

3. Missing subnet validation in on_gossip_aggregated_attestation (store.rs:450-475): This function doesn't use subnet filtering like on_gossip_attestation, creating inconsistency in signature storage policy.

4. Performance regression: The subnet filtering adds O(n) complexity where n is the number of local validators for every attestation/block, but this is acceptable for small validator sets.

Code Quality

5. Redundant modulo operation (store.rs:35-38): With ATTESTATION_COMMITTEE_COUNT = 1, validator_id % 1 is always 0. The #[allow(clippy::modulo_one)] suggests this is intentional, but it's confusing.

6. Function signature inconsistency: on_block_without_verification should probably accept local_validator_ids parameter for consistency, even if verification is skipped.

Recommendations

1. Make ATTESTATION_COMMITTEE_COUNT configurable or derive it from the actual spec parameters
2. Add subnet filtering to on_gossip_aggregated_attestation for consistency
3. Consider whether on_block_without_verification should use subnet filtering
4. Add tests specifically for subnet filtering logic

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Claude Code Review

Here is the review for PR #221:

---

PR #221 Review — Filter gossip signatures by attestation subnet membership

Summary

Clean, minimal spec-compliance patch. The implementation correctly mirrors the two filter sites in store.py. With ATTESTATION_COMMITTEE_COUNT = 1 the behavior is identical to main, and the path is well-prepared for future committee count increases.

---

Issues

Minor — Misleading comment in signature_spectests.rs:58

// passes &[] to on_block (tests don't have local validators, and with
// ATTESTATION_COMMITTEE_COUNT=1 all subnets match anyway)

The second half of the reasoning is incorrect. When local_validator_ids = &[], any() short-circuits to false regardless of ATTESTATION_COMMITTEE_COUNT — there are simply no validators to match against. The tests pass because signature spec tests don't depend on gossip signatures being stored (they only test block acceptance/rejection), not because subnets "match anyway". The comment should be corrected to avoid future confusion.

---

Minor — Redundant validator_ids() allocation in lib.rs

lib.rs:275 and lib.rs:280:

let validator_ids = self.key_manager.validator_ids();  // Vec allocation
store::on_block(&mut self.store, signed_block, &validator_ids)?;
// ...
metrics::update_validators_count(self.key_manager.validator_ids().len() as u64);  // second allocation

The validator_ids Vec already exists — validator_ids.len() can be used directly on line 280, avoiding a second HashMap::keys() + collect().

---

Observations (no action required)

• on_block_without_verification with &[]: Correct — the !verify branch handles proposer attestation via AggregatedSignatureProof::empty before reaching the gossip-signature block, so the &[] short-circuit in the else branch is never a correctness concern.

• Non-aggregator nodes with local validators: on_gossip_attestation in lib.rs already guards on self.is_aggregator before reaching the store call, so a non-aggregator node never hits the subnet filter. Correct layering.

• Observability at higher committee counts: When a signature is skipped due to subnet mismatch, it is silently dropped (no log, no metric). With ATTESTATION_COMMITTEE_COUNT = 1 this path is never reached, but if the constant is increased in the future, debugging "why isn't my aggregator storing signatures" could be difficult. A trace! log or a counter for skipped-due-to-subnet would help at that point — not urgent now.

• #[allow(clippy::modulo_one)] scope: Correctly scoped to the function rather than file-wide. Good.

• compute_subnet_id is module-private: Appropriate — nothing outside store.rs needs it.

---

Verdict

Two small items: a misleading comment and a redundant allocation. The core logic is correct and faithfully matches the spec. Good to merge once the comment is fixed.

---

Automated review by Claude (Anthropic) · custom prompt

[github-actions]

🤖 Codex Code Review

1. High: subnet filter is functionally disabled and inconsistent with network config

• In crates/blockchain/src/store.rs:31, ATTESTATION_COMMITTEE_COUNT is hardcoded to 1, so compute_subnet_id always returns 0.
• That makes both new filters in on_gossip_attestation and on_block_core effectively “local_validator_ids non-empty” checks, not real subnet checks.
• This conflicts with P2P topic routing, which uses configurable attestation_committee_count (see crates/net/p2p/src/lib.rs:212).
• Impact: for --attestation-committee-count > 1, aggregators will still ingest/store proposer signatures from all subnets, defeating intended committee isolation and increasing aggregation/memory/network load.
• Suggestion: plumb attestation_committee_count into blockchain/store (or store it in Store config) and use that value in compute_subnet_id.

2. Low: avoid per-message allocations in hot gossip/block paths

• self.key_manager.validator_ids() allocates a new Vec<u64> for every block and gossip attestation in crates/blockchain/src/lib.rs:275 and crates/blockchain/src/lib.rs:442.
• In high-throughput conditions this is avoidable overhead.
• Suggestion: cache validator IDs (or derived subnet set) in BlockChainServer, refreshing only when keys change.

I couldn’t run tests locally due sandboxed rustup temp-file permission errors, so this review is static analysis only.

---

Automated review by OpenAI Codex · custom prompt

[greptile-apps]

Greptile Summary

This PR implements a spec compliance fix (audit finding F-04) that filters gossip signature storage based on attestation subnet membership, matching the leanSpec reference implementation. Both on_gossip_attestation and on_block_core now only store a gossip signature when the attester/proposer is in the same subnet as at least one local validator, computed via validator_id % ATTESTATION_COMMITTEE_COUNT.

Key changes:

• New ATTESTATION_COMMITTEE_COUNT = 1 constant and compute_subnet_id(validator_id) helper added to store.rs
• on_gossip_attestation and on_block / on_block_core now accept local_validator_ids: &[u64] and gate gossip signature storage behind a subnet membership check
• BlockChainServer call sites in lib.rs pass self.key_manager.validator_ids() to both functions
• on_block_without_verification (test-only path) passes &[], which is safe because the verify=false branch uses a dummy proof and never reaches the signature storage path
• signature_spectests.rs updated to pass &[], which is safe because those tests only validate block acceptance/rejection, not gossip signature storage

Zero behavioral change today: With ATTESTATION_COMMITTEE_COUNT = 1, every validator_id % 1 = 0, so all attesters and all local validators share subnet 0 — the filter always passes for any node with at least one registered validator.

Minor concern: The .any() predicate on an empty local_validator_ids always returns false, silently dropping all gossip signatures. This is spec-correct ("self.validator_id is None" maps to empty slice) but a clarifying comment at both call sites would prevent future misreading of the "all subnets match with ATTESTATION_COMMITTEE_COUNT=1" rationale, which only holds for non-empty validator ID lists.

Confidence Score: 4/5

• Safe to merge — zero behavioral change today with ATTESTATION_COMMITTEE_COUNT=1, and the subnet filtering logic is correct for production nodes with registered validators.
• The implementation correctly matches the leanSpec reference. With ATTESTATION_COMMITTEE_COUNT=1, the filter is always a no-op for any node with at least one local validator. The only issue is a misleading rationale comment (not a code bug) at two sites where &[] is passed — the actual behavior (no signatures stored) differs from the stated reason ("all subnets match anyway"), which could mislead future contributors. No runtime regressions, no data integrity issues, no build breaks identified.
• crates/blockchain/src/store.rs lines 658-661 and crates/blockchain/tests/signature_spectests.rs line 58 — both warrant a clarifying comment about the empty-slice behavior of .any().

Important Files Changed

Filename	Overview
crates/blockchain/src/store.rs	Core change: adds compute_subnet_id helper and ATTESTATION_COMMITTEE_COUNT constant; both on_gossip_attestation and on_block_core now gate gossip signature storage behind a subnet-membership check. Logic is correct for production (non-empty validator IDs), but the in_our_subnet check silently drops signatures when local_validator_ids is empty, warranting a clarifying comment.
crates/blockchain/src/lib.rs	Call sites in process_block and on_gossip_attestation correctly plumb self.key_manager.validator_ids() to the updated store functions; no logic issues found.
crates/blockchain/tests/signature_spectests.rs	Tests updated to pass &[] to on_block; functionally correct since tests only check block acceptance/rejection, not signature storage. The justification ("all subnets match with ATTESTATION_COMMITTEE_COUNT=1") is misleading — with an empty slice, .any() always returns false regardless of the constant.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Receive gossiped attestation / block] --> B{Signature\nverification\nrequired?}
    B -- "verify=false\n(on_block_without_verification)" --> C[Insert dummy proof\nno signature stored]
    B -- "verify=true" --> D[verify_signatures]
    D --> E{Verification\npassed?}
    E -- No --> F[Return error]
    E -- Yes --> G[Store attestation data\ncontent-addressed, always]
    G --> H["compute_subnet_id(attester)"]
    H --> I{Any local validator\nin same subnet?}
    I -- "local_validator_ids = &[]\n→ any() = false" --> J[Skip signature storage]
    I -- "Subnet match found\n(always true if ATTESTATION_COMMITTEE_COUNT=1\nand node has ≥1 validator)" --> K[insert_gossip_signature\nupdate metrics]
    K --> L[Return Ok]
    J --> L

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: crates/blockchain/tests/signature_spectests.rs
Line: 58

Comment:
**Incorrect justification for empty slice argument**

The comment in the PR description (and by implication this call site) says "with `ATTESTATION_COMMITTEE_COUNT=1` all subnets match anyway" to justify passing `&[]`. This reasoning is wrong: when `local_validator_ids` is empty, `.any()` on an empty iterator **always returns `false`**, regardless of `ATTESTATION_COMMITTEE_COUNT`. So passing `&[]` actually means **no proposer gossip signature is stored** in these tests — not that "all subnets match."

This is harmless here because `signature_spectests.rs` only checks `result.is_ok()`, not gossip signature storage. But the stated rationale is misleading and could cause confusion for future readers who assume `&[]` behaves the same as passing real validators when `ATTESTATION_COMMITTEE_COUNT = 1`.

Consider adding a clarifying comment, e.g.:

```suggestion
        let result = store::on_block(&mut st, signed_block, &[]);
        // &[] means no gossip signatures will be stored; that's intentional here
        // because signature_spectests only validates block acceptance/rejection,
        // not gossip signature collection.
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 658-661

Comment:
**`in_our_subnet` always false when `local_validator_ids` is empty**

When `local_validator_ids` is `&[]` (as passed from tests or any node with no registered validators), `.any()` returns `false` unconditionally — so the proposer's gossip signature is silently dropped, even though the signature was already fully verified by `verify_signatures` above. With `ATTESTATION_COMMITTEE_COUNT = 1` this is currently harmless for production nodes (they always have at least one registered validator), but if a non-validator aggregator node were ever deployed without local keys, it would store no gossip signatures at all, silently degrading aggregation quality.

This is also the exact edge case that makes the comment "with `ATTESTATION_COMMITTEE_COUNT=1` all subnets match anyway" misleading — it holds only when `local_validator_ids` is non-empty. A small defensive guard or doc comment would help:

```rust
// NOTE: if local_validator_ids is empty, no signatures are stored — this matches
// the spec's "self.validator_id is None" path but means non-validator aggregators
// will not collect signatures.
let in_our_subnet = local_validator_ids
    .iter()
    .any(|&vid| compute_subnet_id(vid) == proposer_subnet);
```

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: abd5ad9}

[MegaRedHand]

We still need to support multiple validators at the P2P layer, but this is a good first step.