fix: store gossip signatures in order

[MegaRedHand]

This PR fixes a regression added in #253, which unordered gossip transactions, causing errors during aggregation

[github-actions]

🤖 Kimi Code Review

This is a solid change that fixes a critical correctness requirement for XMSS signature aggregation while improving performance.

Consensus Correctness ✅

• Line 208-215: The switch from Vec to BTreeMap is essential for XMSS aggregate proofs. The comment correctly notes that verification reconstructs pubkeys from the participation bitfield in ascending validator ID order, so aggregation must preserve this order. BTreeMap guarantees ascending iteration by key, ensuring the aggregated signature will verify correctly against the bitfield.

Performance ✅

• Line 989: entry.signatures.entry(validator_id).or_insert(signature) changes duplicate detection from O(n) linear scan to O(log n) tree lookup.
• Line 954: entry.signatures.remove(&vid) changes deletion from O(n) retain to O(log n).

Code Quality ✅

• Line 971: The destructuring pattern |(&vid, sig)| is idiomatic and avoids unnecessary clones of the validator ID.
• Line 989: The or_insert pattern correctly preserves the "first-seen wins" semantics of the original duplicate check logic.

Minor Observations

• Memory: BTreeMap has higher memory overhead than HashMap, but for transient gossip signatures (consumed at interval 2), this is acceptable and necessary for the ordering guarantee.
• Concurrency: The Mutex wrapping remains appropriate for this transient in-memory state.

No blocking issues found. The PR correctly addresses the order-dependency requirement of XMSS multi-signatures while cleaning up the implementation.

---

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

[greptile-apps]

Greptile Summary

Fixes the regression from #253 by replacing Vec<GossipSignatureEntry> with BTreeMap<u64, ValidatorSignature> for in-memory gossip signature storage, ensuring signatures are iterated in ascending validator_id order as required by XMSS aggregation. The GossipSignatureEntry wrapper struct is removed in favour of using the map key directly.

Confidence Score: 5/5

Safe to merge — targeted, correct fix with no new risks introduced.

The change is minimal and well-scoped: swapping Vec for BTreeMap fixes the ordering regression, the duplicate-suppression logic is preserved via or_insert, and all public API signatures are unchanged. No P1 or P0 findings.

No files require special attention.

Vulnerabilities

No security concerns identified.

Important Files Changed

Filename	Overview
crates/storage/src/store.rs	Replaces Vec-based gossip signature storage with BTreeMap keyed by validator_id to enforce ascending iteration order, fixing XMSS aggregation ordering regression; also removes now-redundant GossipSignatureEntry wrapper struct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Gossip Signature Received\nvalidator_id + ValidatorSignature] --> B[insert_gossip_signature]
    B --> C{data_root entry\nexists?}
    C -- No --> D[Create GossipDataEntry\nBTreeMap::new]
    C -- Yes --> E[Existing GossipDataEntry]
    D --> F[entry.or_insert signature\nkeyed by validator_id]
    E --> F
    F --> G[BTreeMap\nvalidator_id → ValidatorSignature\nauto-sorted ascending]

    G --> H[Interval 2: iter_gossip_signatures]
    H --> I[Iterate BTreeMap in ascending\nvalidator_id order]
    I --> J[XMSS Aggregation\nparticipation bitfield low-to-high]
    J --> K[AggregatedSignatureProof]

    L[delete_gossip_signatures\nvalidator_id + data_root] --> M[BTreeMap::remove - O log n]

Loading

_{Reviews (1): Last reviewed commit: "fix: store gossip signatures in order" | Re-trigger Greptile}

[github-actions]

🤖 Codex Code Review

No correctness or security findings in this diff.

The change in store.rs to store gossip signatures in a BTreeMap<u64, ValidatorSignature> is consistent with how aggregation is built in store.rs and how participant pubkeys are reconstructed in ascending validator order via attestation.rs. For XMSS aggregation, that ordering requirement is real, so this looks like the right fix.

Residual risk: this would benefit from a regression test around iter_gossip_signatures() that inserts validators out of order and asserts ascending iteration order, because the correctness here depends on that ordering contract remaining intact.

I could not run the Rust tests in this environment: cargo is blocked by read-only/network restrictions when resolving toolchains and dependencies.

---

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

[github-actions]

🤖 Claude Code Review

Here is the review for PR #267:

---

Review: fix: store gossip signatures in order

Summary: This PR correctly fixes a regression from #253 by replacing Vec<GossipSignatureEntry> with BTreeMap<u64, ValidatorSignature> to guarantee ascending validator-ID order when iterating gossip signatures for XMSS aggregation.

---

Correctness

The fix is sound. In aggregate_committee_signatures (crates/blockchain/src/store.rs:150-160), pubkeys and sigs are built in lockstep from the same iteration:

for (vid, sig) in validator_sigs {
    ...
    sigs.push(sig.clone());
    pubkeys.push(pubkey);
    ids.push(*vid);
}

BTreeMap guarantees ascending key order, so pubkeys and sigs are now consistently ordered by ascending validator_id. This matches the low-to-high order expected by xmss_aggregate_signatures when reconstructing pubkeys from the participation bitfield. With the old Vec, network arrival order was arbitrary — an unordered input to xmss_aggregate_signatures would produce verification failures.

---

Duplicate handling

The previous check:

if !entry.signatures.iter().any(|e| e.validator_id == validator_id) {
    entry.signatures.push(...)
}

The new code:

entry.signatures.entry(validator_id).or_insert(signature);

Both implement first-seen-wins semantics. The new form is idiomatic and correct.

---

Performance

• delete_gossip_signatures: Vec::retain() O(n) → BTreeMap::remove() O(log n)
• insert_gossip_signature: Vec::any() O(n) duplicate check + push → BTreeMap::entry().or_insert() O(log n)
• The GossipSignatureEntry wrapper struct is cleanly eliminated.

For typical committee sizes (tens to low hundreds of validators) these are small absolute wins, but the asymptotic improvement is real.

---

One observation: no test for ordering invariant

There are currently no tests in crates/storage/src/store.rs that assert the iter_gossip_signatures output is sorted by validator_id. Given that an ordering regression once silently caused aggregation failures in production, a unit test for this property would prevent future regressions:

#[test]
fn gossip_signatures_returned_in_ascending_validator_id_order() {
    let mut store = Store::test_store();
    let hashed = HashedAttestationData::new(make_att_data(1));
    // Insert in reverse order
    for vid in [5u64, 2, 8, 0, 3] {
        store.insert_gossip_signature(hashed.clone(), vid, make_sig());
    }
    let snapshot = store.iter_gossip_signatures();
    let ids: Vec<u64> = snapshot[0].1.iter().map(|(vid, _)| *vid).collect();
    assert_eq!(ids, vec![0, 2, 3, 5, 8]);
}

This is a minor suggestion — the fix itself is correct.

---

Code quality

The doc comment added to GossipDataEntry (store.rs:210-213) accurately explains the invariant and the reason for it. The comment update at line 947 (set[GossipSignature] instead of set[GossipSignatureEntry]) correctly tracks the spec naming.

---

Overall: Clean, targeted fix. The change is minimal, correctly addresses the root cause, and improves both correctness and performance. The only suggestion is adding a unit test to lock in the ordering invariant.

---

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