Skip to content

change store::on_gossip_attestation to take a ref to signed attestations#278

Open
d4m014 wants to merge 2 commits intolambdaclass:mainfrom
d4m014:refactor/gossip-attestation-by-ref
Open

change store::on_gossip_attestation to take a ref to signed attestations#278
d4m014 wants to merge 2 commits intolambdaclass:mainfrom
d4m014:refactor/gossip-attestation-by-ref

Conversation

@d4m014
Copy link
Copy Markdown

@d4m014 d4m014 commented Apr 14, 2026

Closes #271

This pr changes on_gossip_attestation in both store.rs and lib.rs to accept &SignedAttestation instead of taking ownership to avoid an unnecessary clone at the call site when self-delivering attestations to gossip_signatures, since only the inner data field (a small AttestationData) needs to be cloned to construct the Attestation.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 14, 2026

Greptile Summary

This PR changes store::on_gossip_attestation (and the BlockChainServer wrapper) to accept &SignedAttestation instead of consuming ownership, eliminating an unnecessary .clone() of the full SignedAttestation (including a 3112-byte XMSS signature) at the self-delivery call site. Only the small AttestationData field is now cloned inside the function where it is needed.

Confidence Score: 5/5

  • Safe to merge — purely a refactoring that eliminates an unnecessary clone with no functional change.
  • All changes are mechanical signature updates with no behavioral differences. The only trade-off (one additional cheap AttestationData clone inside the function) is clearly acceptable compared to the eliminated 3112-byte XMSS signature clone at the call site. No P0 or P1 findings.
  • No files require special attention.

Important Files Changed

Filename Overview
crates/blockchain/src/store.rs Signature of on_gossip_attestation changed from owned SignedAttestation to &SignedAttestation; internal data field now cloned (cheap AttestationData clone) instead of moved. Logic is unchanged.
crates/blockchain/src/lib.rs Both the BlockChainServer::on_gossip_attestation wrapper and its two call sites updated to pass &SignedAttestation; the expensive .clone() on the self-delivery path is now removed.

Sequence Diagram

sequenceDiagram
    participant P2P as P2P Gossip
    participant H as NewAttestation Handler
    participant BC as BlockChainServer
    participant Store as store on_gossip_attestation

    P2P->>H: "NewAttestation { attestation }"
    H->>BC: "on_gossip_attestation(&msg.attestation)"
    BC->>Store: "on_gossip_attestation(store, &signed_attestation)"
    Note over Store: "clone only AttestationData (small), decode sig bytes from ref"
    Store-->>BC: "Ok(()) / Err(StoreError)"

    Note over BC: "Self-delivery path (is_aggregator only)"
    BC->>Store: "on_gossip_attestation(store, &signed_attestation)"
    Note over Store: "Before: signed_attestation.clone() — 3112-byte XMSS sig copied. Now: borrow only, no large clone"
    Store-->>BC: "Ok(()) / Err(StoreError)"
Loading

Reviews (1): Last reviewed commit: "Merge branch 'main' into refactor/gossip..." | Re-trigger Greptile

Copy link
Copy Markdown
Collaborator

@MegaRedHand MegaRedHand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@MegaRedHand MegaRedHand requested a review from pablodeymo April 14, 2026 13:55
@MegaRedHand
Copy link
Copy Markdown
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Change store::on_gossip_attestation take a reference to signed attestations

3 participants