Port leanSpec on_tick and attestation validation changes

[pablodeymo]

Motivation

The leanSpec bump to 8b7636b (#134) introduced several spec changes beyond the signature
serialization fix. This PR ports three remaining behavioral changes to align ethlambda
with the reference implementation:

1. Stricter attestation validation — leanSpec added two checks we were missing
2. Safe target attestation pool merge — leanSpec changed which attestations are visible during safe target computation
3. Aggregated attestation broadcasting — leanSpec now broadcasts aggregated attestations after aggregation

Description

1. Stricter attestation validation (validate_attestation_data)

Two new validation rules reject malformed attestations:

• Head >= target check: Reject attestations where the head checkpoint slot is older than the target slot. Well-formed attestations already satisfy head.slot >= target.slot, but without this check we'd accept malformed ones.
• Head slot consistency check: Reject attestations where the head checkpoint's slot field doesn't match the actual block's slot. This mirrors the existing checks for source and target checkpoints.

New StoreError variants: HeadOlderThanTarget, HeadSlotMismatch.

2. Merge attestation pools in update_safe_target

Previously, update_safe_target (called at interval 3) only looked at "new" attestation payloads. However, the migration from "new" to "known" happens at interval 4, and some attestations enter "known" directly without passing through "new":

• Proposer's own attestation bundled in block body → lands in "known" directly via on_block
• Node's own self-attestation → recorded locally in "known"

Without merging both pools, these attestations were invisible to the safe target calculation, causing it to undercount support. Now both pools are merged before computing the safe target. No double-counting risk since extract_attestations_from_aggregated_payloads deduplicates by validator ID (latest slot wins).

3. Broadcast aggregated attestations after aggregation

After aggregating committee signatures at interval 2, the node now broadcasts the resulting SignedAggregatedAttestation messages to the network. The P2PMessage::PublishAggregatedAttestation variant already existed but was never sent from the aggregation path.

Changes:

• aggregate_committee_signatures returns Vec<SignedAggregatedAttestation>
• store::on_tick returns Vec<SignedAggregatedAttestation>
• BlockChainServer::on_tick publishes each aggregate via the P2P channel

Test call sites (forkchoice_spectests, signature_spectests) ignore the return value — Vec<T> is not #[must_use], so this compiles without warnings.

How to Test

make fmt    # Passes
make lint   # Passes
make test   # All 26 fork choice + 14 STF + 8 signature spec tests pass

Change 3 (broadcasting) is a runtime behavior validated in devnet testing.

Related Issues

Stacked on #134 (leanSpec bump to 8b7636b).

[github-actions]

🤖 Kimi Code Review

Review Summary

This PR improves attestation aggregation and validation logic in the consensus layer. The changes are generally correct and address important edge cases, but there are a few concerns:

Issues Found

1. Potential Memory Leak in aggregate_committee_signatures (store.rs:199-203)

   • The function creates SignedAggregatedAttestation objects and adds them to new_aggregates, but these are cloned again when creating StoredAggregatedPayload. Consider reusing the same objects to avoid unnecessary allocations.

2. Race Condition in Attestation Processing (store.rs:87-93)

   • The merge of known and new aggregated payloads happens at interval 3, but the migration from new to known happens at interval 4. This could lead to double-counting attestations if the same attestation appears in both pools. Consider adding deduplication logic.

3. Error Handling Inconsistency (lib.rs:166-170)

   • The error is logged but ignored with let _ =. Since this is a critical P2P communication failure, consider whether this should be propagated or handled more explicitly.

4. Validation Logic Completeness (store.rs:218-270)

   • The new validation checks for head slot consistency are good, but there's no check to ensure the head block is actually in the canonical chain. Consider adding a check that data.head.root is an ancestor of the current head.

Positive Aspects

• The new validation rules for attestation data (head slot checks) correctly implement consensus safety requirements
• The aggregation logic properly handles committee signatures and creates valid aggregated attestations
• The return value from on_tick allows proper P2P propagation of newly created aggregates
• Error messages are descriptive and helpful for debugging

Minor Suggestions

• Consider adding metrics for the number of new aggregates created in aggregate_committee_signatures
• The HeadOlderThanTarget error variant could include the actual head and target roots for better debugging
• Document the interval timing assumptions more explicitly in the on_tick function comments

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Codex Code Review

Review Summary
Good incremental changes for attestation handling and stricter validation. I have two correctness/perf concerns to address; everything else looks solid.

Findings

1. Potential double‑counting of attestation weight in fork choice
   In update_safe_target, you merge known + new aggregated payloads into all_payloads without deduplication. If the same aggregated payload exists in both pools for a validator key, it will be counted twice in extract_attestations_from_aggregated_payloads, inflating vote weight and possibly affecting LMD‑GHOST.
   Suggestion: de‑dup by (attestation data root, aggregation bits, signature) or track a unique ID per payload while merging.
   File: crates/blockchain/src/store.rs (function update_safe_target)

2. Extra allocations on every tick
   Building a new HashMap<SignatureKey, Vec<StoredAggregatedPayload>> each tick allocates and clones payload vectors. This is likely fine for small sets but can grow with network size.
   Suggestion: allow extract_attestations_from_aggregated_payloads to accept a chained iterator of (SignatureKey, &Vec<...>) (or provide a dedicated iterator that yields both pools) to avoid the merge allocation.
   File: crates/blockchain/src/store.rs (function update_safe_target)

Notes

• The new HeadOlderThanTarget and HeadSlotMismatch checks in validate_attestation_data are correct and tighten validation as expected.
• Publishing aggregated attestations from on_tick is a good addition. If you see duplicates on the wire, consider a short‑lived cache keyed by aggregate root, but it’s not required for correctness.

If you want, I can draft a small dedup strategy that minimizes allocations.

---

Automated review by OpenAI Codex · custom prompt

[greptile-apps]

Greptile Summary

Ports three behavioral changes from leanSpec 8b7636b to align ethlambda with the reference implementation:

• Stricter attestation validation: Adds two new checks in validate_attestation_data — rejects attestations where the head checkpoint is older than the target (HeadOlderThanTarget), and where the head checkpoint's slot doesn't match the actual block's slot (HeadSlotMismatch). This closes gaps that previously allowed malformed attestations through.
• Merged attestation pools in update_safe_target: At interval 3, both "known" and "new" attestation pools are now merged before computing the safe target. This ensures attestations that enter "known" directly (e.g., proposer's own attestation in block body, self-attestations) are visible to the safe target calculation. Deduplication in extract_attestations_from_aggregated_payloads (latest slot per validator wins) prevents double-counting.
• Broadcast aggregated attestations: aggregate_committee_signatures and on_tick now return Vec<SignedAggregatedAttestation>, which BlockChainServer::on_tick publishes via the existing P2PMessage::PublishAggregatedAttestation variant. Test call sites correctly ignore the return value.

Confidence Score: 5/5

• This PR is safe to merge — changes are well-scoped, follow existing patterns, and align with the leanSpec reference implementation.
• All three changes are straightforward ports of spec behavior. The attestation validation additions are strictly additive (reject more malformed inputs). The pool merge in update_safe_target is safe due to deduplication in extract_attestations_from_aggregated_payloads. The broadcast change reuses an existing P2P message variant. No new unsafe patterns, no breaking API changes beyond the return type of on_tick (test callers correctly ignore the value).
• No files require special attention.

Important Files Changed

Filename	Overview
crates/blockchain/src/store.rs	Core logic changes: merges known+new attestation pools in update_safe_target, returns aggregated attestations from aggregate_committee_signatures and on_tick, adds HeadOlderThanTarget and HeadSlotMismatch validation checks and error variants. All changes are well-structured and consistent with existing patterns.
crates/blockchain/src/lib.rs	Captures the Vec<SignedAggregatedAttestation> return from store::on_tick and broadcasts each aggregate via the P2P channel using the existing PublishAggregatedAttestation message variant. Error handling uses inspect_err with logging, consistent with surrounding code.

Sequence Diagram

sequenceDiagram
    participant Timer
    participant BCS as BlockChainServer
    participant Store as store::on_tick
    participant Agg as aggregate_committee_signatures
    participant ST as update_safe_target
    participant P2P as P2P Channel

    Timer->>BCS: on_tick(timestamp_ms)
    BCS->>Store: on_tick(store, ts, has_proposal, is_aggregator)

    Note over Store: Interval 0: accept attestations if proposal
    Note over Store: Interval 1: no-op

    Store->>Agg: Interval 2: aggregate_committee_signatures(store)
    Agg-->>Store: Vec<SignedAggregatedAttestation>

    Store->>ST: Interval 3: update_safe_target(store)
    Note over ST: Merge known + new pools
    Note over ST: compute_lmd_ghost_head with merged attestations

    Note over Store: Interval 4: accept_new_attestations

    Store-->>BCS: Vec<SignedAggregatedAttestation>

    loop For each aggregate
        BCS->>P2P: PublishAggregatedAttestation(aggregate)
    end

Loading

_{Last reviewed commit: d72c3a3}

[github-actions]

🤖 Claude Code Review

Here is the review of PR #135:

---

PR #135 — Port leanSpec on_tick and attestation validation changes

Overall

Three focused, well-scoped changes that correctly port behavioral updates from leanSpec 8b7636b. Logic is sound, patterns are idiomatic, and no consensus-critical regressions are visible. A few observations below.

---

Change 1 — Stricter attestation validation (validate_attestation_data)

store.rs:249 — HeadOlderThanTarget check

if data.head.slot < data.target.slot {
    return Err(StoreError::HeadOlderThanTarget { ... });
}

This uses data.head.slot (the claimed checkpoint slot) before the consistency check at line 269 confirms it matches the actual block. This is intentional and correct — the same approach is used for source/target topology checks — but worth noting that a malformed attestation with head.slot >= target.slot but head_header.slot != data.head.slot still reaches the consistency check. The ordering is logically fine: topology → consistency → time.

Ordering of new error variants in StoreError — HeadOlderThanTarget is inserted between SourceExceedsTarget and SourceSlotMismatch, and HeadSlotMismatch is appended after TargetSlotMismatch. Both positions make semantic sense and match the check ordering.

Missing head >= source check — Not needed here: source.slot <= target.slot (existing check) and head.slot >= target.slot (new check) together imply head.slot >= source.slot transitively. No gap.

---

Change 2 — Merge attestation pools in update_safe_target

store.rs:88–94 — Pool merge

let mut all_payloads: HashMap<SignatureKey, Vec<StoredAggregatedPayload>> =
    store.iter_known_aggregated_payloads().collect();
for (key, new_proofs) in store.iter_new_aggregated_payloads() {
    all_payloads.entry(key).or_default().extend(new_proofs);
}

The deduplication argument is correct. extract_attestations_from_aggregated_payloads ignores _payload_list and deduplicates by validator_id (latest slot wins), so:

• A key that exists only in new → added via or_default().extend()
• A key in both pools → payloads are extended in the Vec, but the Vec is ignored by the extractor; the key is iterated once in the HashMap → no double-count

One minor note: the collect() on iter_known_aggregated_payloads() allocates a full HashMap upfront before the merge loop. If the known pool is large, this is an O(n) allocation that didn't previously exist for update_safe_target. Not a correctness concern, just a memory footprint observation at interval 3.

---

Change 3 — Broadcast aggregated attestations

lib.rs:165–170

for aggregate in new_aggregates {
    let _ = self
        .p2p_tx
        .send(P2PMessage::PublishAggregatedAttestation(aggregate))
        .inspect_err(|err| error!(%err, "Failed to publish aggregated attestation"));
}

The let _ = is required here because mpsc::UnboundedSender::send returns a #[must_use] Result. Using inspect_err to log is consistent with project conventions; let _ = suppresses the compiler warning correctly. The error level (error! vs warn!) is appropriate — a send failure means the aggregate is lost and peers won't receive it.

store.rs:197–200 — Clone cost

new_aggregates.push(SignedAggregatedAttestation {
    data: data.clone(),
    proof: proof.clone(),
});
let payload = StoredAggregatedPayload { slot, proof };

proof is moved into StoredAggregatedPayload, so the clone is unavoidable. Given XMSS proofs are ~3KB, this is an explicit, justified cost per aggregate per interval-2 tick. The CLAUDE.md ownership pattern ("Add .clone() at call site — makes cost explicit") applies here.

Aggregation guard — aggregate_committee_signatures is only called when is_aggregator is true (line 339), so new_aggregates is always empty for non-aggregator nodes. The broadcast loop is a no-op for them. Correct.

No unit test coverage for broadcasting — This is noted in the PR as devnet-validated. Acceptable given the P2P dependency; a future integration test mocking p2p_tx could cover this path.

---

Summary

No correctness bugs, security issues, or protocol violations found. The three changes are each internally consistent and well-reasoned. The pool-merge allocation in update_safe_target is the only minor efficiency note, and it's a reasonable trade-off for correctness.

---

Automated review by Claude (Anthropic) · custom prompt