fix: clamp attestation target to source

[MegaRedHand]

Summary

Fixes the SourceSlotExceedsTarget bug where ethlambda produces attestations with source.slot > target.slot.

Closes #97

Root Cause

The store has two fields with different update cadences:

Field	Updated when	Used as
latest_justified	During on_block() — any interval	Attestation source
safe_target	Only at interval 2 via update_safe_target()	Anchor for attestation target computation

When a block at interval 0 advances latest_justified, safe_target remains stale until the next interval 2. During attestation production at interval 1, get_attestation_target() walks back from head toward the stale safe_target, landing on a slot behind the new latest_justified:

Slot N, Interval 2:  update_safe_target()     → safe_target = slot 2
Slot N+1, Interval 0: on_block()              → latest_justified advances to slot 3
                                                 safe_target still = slot 2
Slot N+1, Interval 1: produce_attestations()  → source = slot 3 (justified)
                                                 target = slot 2 (walked to stale safe_target)
                                                 source > target → REJECTED

This is a spec-level gap — the leanSpec get_attestation_target() does not enforce target.slot >= latest_justified.slot.

Fix

Add a clamp guard at the end of get_attestation_target(): if the computed target slot falls behind latest_justified.slot, return latest_justified as the target instead. A warning is logged when clamping occurs.

This matches Zeam's approach.

Test plan

• make lint passes
• make test — all 35 spec tests pass
• Devnet run: confirm SourceSlotExceedsTarget warnings disappear from peer logs
• Confirm the new "Attestation target walked behind justified source, clamping to justified" warning fires during the slots where clamping occurs

[github-actions]

🤖 Kimi Code Review

Security & Correctness Issues

Line 477-485: The new condition target_block.slot > finalized_slot in the while loop creates a critical consensus bug. The original logic correctly walked back until finding a justifiable slot regardless of its relationship to the finalized slot. This change could prevent proper target selection when the chain has advanced significantly beyond finalization, potentially causing attestation targets to be invalid or creating fork choice issues.

Line 488-502: The clamping logic introduces inconsistency with the checkpoint's root. When target_block.slot < latest_justified.slot, the function returns latest_justified (a checkpoint), but this checkpoint's root may not correspond to the block at latest_justified.slot. This breaks the invariant that the checkpoint root should be the block root at that slot.

Performance & Safety

Line 482-484: The .expect("parent block exists") remains dangerous. In a real-world scenario with reorgs or database inconsistencies, this could panic. Consider returning a Result or using proper error handling.

Suggested Fixes

1. Revert the while loop condition (line 477) to the original logic:

   while !slot_is_justifiable_after(target_block.slot, finalized_slot) {

2. Fix the checkpoint consistency by either:

   • Finding the block root at latest_justified.slot instead of returning the checkpoint directly, or
   • Documenting why this inconsistency is acceptable (unlikely)

3. Replace the expect with proper error handling:

   let target_block = store.get_block(&target_block_root)
       .ok_or_else(|| anyhow::anyhow!("Parent block {} not found", target_block_root))?;

The logging addition (line 496-500) is good for debugging but doesn't address the underlying correctness issues.

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Codex Code Review

Findings (ordered by severity)

• Potential fork-choice correctness edge case: the new loop condition stops walking once target_block.slot <= finalized_slot, even if safe_target (or target_block) were below finalized due to a bug or unusual store state; this can return a target earlier than finalized without any clamp. Consider explicitly clamping to latest_finalized or asserting safe_target_block_slot >= finalized_slot to preserve invariants. crates/blockchain/src/store.rs:468-476
• Potential invariant assumption: returning latest_justified directly assumes its root is an ancestor of the current head/target chain. If store invariants are ever briefly inconsistent (reorg or partial state), this could produce an attestation with a target not on the head chain. Consider a sanity check (e.g., ancestor check) or a defensive fallback (walk back from head to latest_justified.slot). crates/blockchain/src/store.rs:486-494

If those invariants are already guaranteed elsewhere, the change looks good and the new clamp is a reasonable safety net.

Possible next steps

1. Add a debug/assertion or clamp around finalized/justified invariants to make the assumptions explicit.

---

Automated review by OpenAI Codex · custom prompt

[github-actions]

🤖 Claude Code Review

Now I have enough context to provide a comprehensive review. Let me create the review document:

PR #98 Review: Fix Attestation Target Clamping

Summary

This PR addresses a critical consensus bug where ethlambda produces invalid attestations with source.slot > target.slot, which violates a fundamental consensus invariant and causes peer rejection.

Root Cause Analysis ✅

The PR description provides excellent analysis of the timing issue:

• latest_justified updates during on_block() (any interval)
• safe_target only updates at interval 2 via update_safe_target()
• Gap window at interval 1 allows get_attestation_target() to walk back to a stale safe_target, landing behind the newly advanced latest_justified

The diagnosis is sound and well-documented.

Code Changes Review

Location: crates/blockchain/src/store.rs:468-494

Positive aspects:

1. Correct placement: The clamping check occurs after both walk-back loops, catching the edge case
2. Clear logging: Warning includes both target_slot and justified_slot for debugging
3. Proper return value: Returns the full latest_justified Checkpoint (with correct root + slot)
4. Comment quality: Excellent inline explanation of why this can occur

Critical Issue: Loop Termination Condition

Line 474-475:

while target_block.slot > finalized_slot
    && !slot_is_justifiable_after(target_block.slot, finalized_slot)

Problem: The loop has a subtle bug that the PR doesn't address. The condition target_block.slot > finalized_slot means the loop will stop if target_block.slot == finalized_slot, even if that slot is not justifiable.

However, looking at slot_is_justifiable_after() line 401-403:

let Some(delta) = slot.checked_sub(finalized_slot) else {
    return false;  // Candidate slot must not be before finalized slot
};

When target_block.slot == finalized_slot, delta = 0, and the function checks delta <= 5, which is true. So finalized_slot itself IS justifiable (delta=0).

This means the loop condition is actually correct - it stops when reaching finalized_slot because that slot is always justifiable. But the guard condition target_block.slot > finalized_slot makes this implicit rather than relying on the justifiability check alone.

Edge Case Analysis

Scenario 1: Target walks all the way back to finalized_slot

• Loop exits with target_block.slot == finalized_slot
• If latest_justified.slot > finalized_slot, clamping triggers ✅
• Returns latest_justified ✅

Scenario 2: Target finds justifiable slot between justified and finalized

• Loop exits with target_block.slot in range (finalized_slot, latest_justified.slot)
• Clamping check at line 487 catches this ✅
• Returns latest_justified ✅

Scenario 3: Target finds justifiable slot at or after justified

• Normal case, no clamping needed ✅
• Returns computed target ✅

Potential Issue: Root Correctness

When returning latest_justified directly, we're using the root from latest_justified, not the root we walked to via target_block_root.

Question: Is latest_justified.root always guaranteed to match the block at latest_justified.slot in the canonical chain?

Looking at line 521 in produce_attestation_data():

source: store.latest_justified(),

The latest_justified checkpoint is retrieved from the store, which should have both the correct root and slot from when justification occurred. This should be correct, but it means we're not attesting to the specific block we walked to - we're attesting to whatever block is recorded in the justified checkpoint.

This is actually correct behavior: When clamping to justified, we should attest to the official justified checkpoint (root + slot), not some arbitrary ancestor block at that slot. The justified checkpoint represents a canonical agreed-upon point.

Log Field Ordering

Line 488-491:

warn!(
    target_slot = target_block.slot,
    justified_slot = latest_justified.slot,
    "Attestation target walked behind justified source, clamping to justified"
);

According to CLAUDE.md logging patterns (temporal → identity → identifiers):

// Should be:
warn!(
    justified_slot = latest_justified.slot,  // temporal context first
    target_slot = target_block.slot,         // derived value second
    "Attestation target walked behind justified source, clamping to justified"
);

Minor style improvement - justified_slot is the reference point (temporal anchor), target_slot is the computed value that's being rejected.

Missing: Test Coverage

The PR description mentions:

• make lint passes
• make test — all 35 spec tests pass
• Devnet run: confirm SourceSlotExceedsTarget warnings disappear

Recommendation: Consider adding a unit test that explicitly exercises this edge case:

1. Set up store with latest_justified.slot = N
2. Set safe_target to slot < N (simulating stale safe_target)
3. Call get_attestation_target()
4. Assert result equals latest_justified
5. Assert warning was logged

This would prevent regression and document the expected behavior.

Security & Correctness

✅ No security vulnerabilities introduced
✅ Maintains consensus invariant: source.slot <= target.slot
✅ Safe fallback: Clamping to justified is conservative and correct
✅ No signature/crypto changes: SSZ encoding unchanged

Performance Impact

✅ Negligible: Single integer comparison added after walk-back loops
✅ No additional storage access: latest_justified already loaded

Alignment with Other Clients

The PR mentions this matches Zeam's approach. Cross-client consistency is important for consensus. ✅

Recommendations

Must Fix (Critical):

None - the logic is correct.

Should Fix (Important):

1. Add unit test for the clamping scenario to prevent regression

Nice to Have (Style):

1. Reorder log fields (justified_slot before target_slot) per project conventions in crates/blockchain/src/store.rs:488-491

Consider:

1. Spec update: The PR notes this is a "spec-level gap" in leanSpec. Consider opening an issue in the leanSpec repo to document this edge case and the clamping solution.

Final Verdict

APPROVE with minor suggestions

This is a well-analyzed and correctly implemented fix for a real consensus bug. The code is safe, the logic is sound, and the documentation is excellent. The only improvements are stylistic (log ordering) and defensive (test coverage).

The fix properly handles the race condition between latest_justified updates and safe_target updates, ensuring ethlambda never produces invalid attestations.

---

Automated review by Claude (Anthropic) · custom prompt

[greptile-apps]

Greptile Overview

Greptile Summary

Fixes SourceSlotExceedsTarget bug by adding a clamp guard in get_attestation_target() to ensure attestation target slot is never behind the justified source slot.

Key Changes:

• Adds check after target computation: if target_block.slot < latest_justified.slot, returns latest_justified as target instead
• Adds warning log when clamping occurs for debugging
• Addresses race condition where latest_justified updates at any interval via on_block(), but safe_target only updates at interval 2

Root Cause:
The bug occurs when a block at interval 0 advances latest_justified to a higher slot, but safe_target remains stale until the next interval 2 update. During attestation production at interval 1, the walk-back algorithm lands on a slot behind the new justified checkpoint, violating the spec invariant source.slot <= target.slot.

Fix Approach:
The clamp ensures the critical invariant is maintained by returning latest_justified when the computed target falls behind. This matches Zeam's approach and addresses a gap in the leanSpec implementation.

Confidence Score: 4/5

• Safe to merge with minimal risk - fixes a critical consensus bug with a simple, well-documented guard
• The fix correctly addresses the root cause by adding a safety clamp that maintains the spec invariant. The logic is sound: when target walks back behind justified, clamping to justified is correct since justified checkpoints are inherently justifiable. The warning log aids debugging. Minor concern: would benefit from additional test coverage for the race condition scenario, but existing 35 spec tests pass.
• No files require special attention

Important Files Changed

Filename	Overview
crates/blockchain/src/store.rs	Adds clamp to ensure attestation target.slot >= source.slot, fixing race condition between justified checkpoint updates and safe_target updates

Sequence Diagram

sequenceDiagram
    participant Block as on_block()
    participant Store as Store
    participant SafeTarget as update_safe_target()
    participant Attest as produce_attestation_data()
    participant GetTarget as get_attestation_target()
    
    Note over Block,Store: Interval 2: safe_target = slot 2
    SafeTarget->>Store: set safe_target = slot 2
    
    Note over Block,Store: Interval 0: Block advances justified
    Block->>Store: on_block() processes new block
    Block->>Store: update latest_justified = slot 3
    Note over Store: safe_target still = slot 2 (stale)
    
    Note over Attest,GetTarget: Interval 1: Produce attestation
    Attest->>GetTarget: get_attestation_target()
    GetTarget->>Store: read safe_target (slot 2)
    GetTarget->>GetTarget: walk back from head to slot 2
    Note over GetTarget: target_block.slot = 2
    GetTarget->>Store: read latest_justified (slot 3)
    
    alt target_block.slot < latest_justified.slot
        GetTarget->>GetTarget: CLAMP: return latest_justified
        Note over GetTarget: target = slot 3 (clamped)
    else target_block.slot >= latest_justified.slot
        GetTarget->>GetTarget: return target_block
        Note over GetTarget: target = computed slot
    end
    
    GetTarget-->>Attest: return target checkpoint
    Attest->>Store: read latest_justified (slot 3)
    Note over Attest: source = slot 3, target = slot 3
    Note over Attest: ✓ Invariant maintained: source.slot <= target.slot

Loading