fix(forkchoice): use only "new" pool for safe target (leanSpec #680)

[MegaRedHand]

Summary

Ports leanSpec PR #680 into ethlambda. update_safe_target now reads only latest_new_attestations instead of merging the new and known pools.

Rationale

Safe target is an availability signal, not a durable-knowledge signal. The new -> known migration step runs at interval 4, strictly after safe-target computation at interval 3, and 3sf-mini chose that ordering deliberately:

• At interval 3, votes still in new represent the current slot's online view.
• Anything already in known is historical: block-included attestations, gossip migrated in earlier slots, self-attestations stored locally without traversing gossipsub.
• Counting known would let a node keep advancing its safe target on stale evidence even when live participation has collapsed, which is exactly the failure mode safe target is supposed to prevent.

Aligns with zeam (PR #779) and the Ream reference implementation.

Changes

• crates/blockchain/src/store.rs (update_safe_target): swap extract_latest_all_attestations() for extract_latest_new_attestations(). Docstring rewritten around the interval-3 / interval-4 ordering and the availability framing.
• crates/storage/src/store.rs: drop the now-unused extract_latest_all_attestations helper.

Notes

• Marked draft because the upstream spec PR is still draft awaiting confirmation from Yaan on the availability framing. Once leanSpec #680 lands and LEAN_SPEC_COMMIT_HASH is bumped, the regenerated test_safe_target_* fixtures should validate the new behavior end-to-end. (Today the fixture's safeTargetSlot / safeTargetRootLabel checks are silently ignored by our spec-test runner because those fields are not yet wired into StoreChecks.)

Test plan

• cargo build --workspace
• cargo clippy --workspace --all-targets -- -D warnings
• cargo fmt --all
• cargo test --workspace --release -- 323 passed, 6 ignored
• cargo test -p ethlambda-blockchain --release --test forkchoice_spectests -- 70 passed

[github-actions]

🤖 Kimi Code Review

Overall Assessment: This is a correct and important consensus-layer security fix implementing the 3sf-mini (3-slot finality) specification correctly. The change prevents a liveness attack where nodes could advance safe targets using stale attestations.

Detailed Feedback

1. Consensus Correctness (Critical)

File: crates/blockchain/src/store.rs, lines 103-115

The comment addition correctly explains the subtle distinction between availability and durable knowledge signals. The logic change from extract_latest_all_attestations() to extract_latest_new_attestations() is correct:

• Interval timing: At interval 3 (safe target computation), only the "new" pool contains fresh attestations from the current slot
• Migration timing: The promotion to "known" happens at interval 4, so "known" contains stale (previous slot) attestations
• Security property: This prevents the "stale evidence" attack where a node could continue advancing safe targets based on old block-included attestations even when live participation has collapsed

Verification: Ensure extract_latest_new_attestations() only queries self.new_payloads and not self.known_payloads. The removed method was merging both, which violated the 3sf-mini safety property.

2. Code Cleanup

File: crates/storage/src/store.rs, lines 995-1017

Safe removal of dead code. The extract_latest_all_attestations method is no longer used after the call site change, and its existence could encourage incorrect usage of the merged attestation pool.

Suggestion: Verify no other crates or test files reference this removed method. If it exists in integration tests, those tests should be updated to reflect the correct 3sf-mini behavior.

3. Documentation Quality

File: crates/blockchain/src/store.rs, lines 103-115

Excellent inline documentation. The reference to "leanSpec PR #680" provides traceability to the specification change. The explanation of why counting "known" would be dangerous is clear and aids future maintainers.

4. Performance

Positive: Eliminates double-locking (known_payloads + new_payloads) and HashMap merging overhead in the hot path of safe target computation. Now only requires a single lock on the new payloads pool.

5. Minor Suggestions

Line 121 (crates/blockchain/src/store.rs):

let attestations = store.extract_latest_new_attestations();

Consider adding a debug assertion or comment confirming this returns only interval-3 (current slot) attestations, reinforcing the invariant for future readers.

Line 118 (crates/blockchain/src/store.rs):
The expect("head state exists") is appropriate here since the head block must always have a corresponding state in a consistent store, but ensure this is called after any potential state pruning checks.

Conclusion

No blocking issues. This PR correctly implements the availability-signal semantics required by 3sf-mini and removes the vulnerable merged-pool logic. The security fix is well-documented and the code is clean.

---

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

[greptile-apps]

Greptile Summary

This PR ports leanSpec #680 by changing update_safe_target to consult only the new attestation pool (extract_latest_new_attestations) instead of the merged new + known view previously provided by extract_latest_all_attestations, and removes that now-unused helper. The rationale is well-documented: safe target is an availability signal that must reflect current-slot online participation, and known carries historical/stale evidence that would allow the safe target to advance even when live participation has collapsed.

Confidence Score: 5/5

This PR is safe to merge — the change is small, well-motivated by the spec, and the removed helper has no remaining call sites.

Two-file, single-concept change with a clear spec rationale. The removed extract_latest_all_attestations helper is confirmed unused across the codebase. All other callers of extract_latest_known_attestations and extract_latest_new_attestations are unaffected. No logic errors or regressions identified.

No files require special attention.

Important Files Changed

Filename	Overview
crates/blockchain/src/store.rs	Swaps extract_latest_all_attestations() for extract_latest_new_attestations() in update_safe_target; adds a detailed docstring explaining the interval-3/4 ordering and the availability-signal rationale.
crates/storage/src/store.rs	Removes the now-unused extract_latest_all_attestations helper; extract_latest_known_attestations and extract_latest_new_attestations are still present and used by other call sites.

Sequence Diagram

sequenceDiagram
    participant Gossip as Gossip
    participant New as new pool
    participant Known as known pool
    participant FC as update_safe_target (interval 3)
    participant Promote as promote new->known (interval 4)

    Gossip->>New: "attestations arrive"
    FC->>New: "extract_latest_new_attestations()"
    New-->>FC: "current-slot votes only"
    FC->>FC: "compute_lmd_ghost_head() => safe_target"
    Promote->>New: "drain new pool"
    Promote->>Known: "insert into known pool"
    Note over Known: "historical attestations now in known"

Loading

_{Reviews (1): Last reviewed commit: "fix(forkchoice): use only "new" pool for..." | Re-trigger Greptile}

[github-actions]

🤖 Claude Code Review

---

PR #316 Review: fix(forkchoice): use only "new" pool for safe target

Overview

This PR ports leanSpec #680 — changing update_safe_target to read only latest_new_attestations instead of the merged view of both pools. The semantic motivation is that safe target is an availability signal: it should reflect live participation in the current slot, not historical attestations that already migrated into known.

---

Correctness

The interval ordering is verified: on_tick in store.rs:248 dispatches update_safe_target at interval 3, and accept_new_attestations (which calls promote_new_aggregated_payloads, moving new → known) at interval 4. The invariant holds — at interval 3, new_payloads truly contains only fresh gossip for the current slot, while known_payloads has already-migrated older attestations.

The prior rationale comment was wrong: The removed comment read:

"At interval 3 the promotion (interval 4) hasn't run yet, so we must merge both pools to get a complete view."

This conflated "complete" with "current." It was a misleading justification for including stale evidence. Removing it is correct.

Head vs safe target symmetry is now clean:

• update_head (interval 4) → extract_latest_known_attestations() — durable knowledge
• update_safe_target (interval 3) → extract_latest_new_attestations() — live participation

This separation is semantically coherent.

---

Potential Concerns

• Sparse-gossip edge case: If the node receives no fresh gossip between intervals 0 and 3 (e.g., temporarily isolated), extract_latest_new_attestations() returns an empty map. Safe target would stall, not advance. This is intended behavior (you shouldn't claim availability without evidence), but worth documenting as a known consequence, especially for single-validator test setups.

• Self-attestations excluded: Attestations produced locally at interval 1 go into new_payloads via gossip, so they are visible at interval 3 if the aggregator loop has run. However, the PR description notes "self-attestations stored locally without traversing gossipsub" end up in known. If local self-attestations bypass the gossip path and land directly in known_payloads, they will now be invisible to safe target. Confirm this edge case is intentional (the PR description says it is, citing 3sf-mini's design).

• Spec test gap (most significant risk): The PR itself notes that safeTargetSlot/safeTargetRootLabel fields in spec-test fixtures are silently ignored because StoreChecks doesn't wire them yet. This means the behavioral change has no end-to-end validation path until those checks are plumbed in. Consider adding a TODO or tracking issue so this doesn't slip after leanSpec #680 lands.

---

Code Quality

• The new docstring is clear and well-reasoned — it directly references the interval ordering and the failure mode being prevented.
• The deleted extract_latest_all_attestations had its own merge logic (preferring higher-slot attestation on conflict). Removing it is clean since the call site is gone and there are no remaining callers.
• No dead code introduced; no unnecessary abstractions.

---

Minor Notes

• The _weights return from compute_lmd_ghost_head is already ignored (pre-existing), which is fine — safe target only needs the root.
• Draft status is appropriate. Merging before leanSpec #680 is confirmed would be premature given the spec test gap.

---

Summary

The change is correct, minimal, and well-motivated. The logic aligns with zeam PR #779 and Ream. The only actionable concern before merging (once the upstream spec lands) is ensuring the StoreChecks spec-test runner is updated to validate safeTargetSlot/safeTargetRootLabel — otherwise the behavioral change remains untested by the fixture suite. Consider filing a tracking issue for that work.

---

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

[github-actions]

🤖 Codex Code Review

Findings

1. crates/blockchain/src/store.rs: switching safe_target to extract_latest_new_attestations() only removes stale votes from known; it does not actually enforce the new comment’s “current-slot fresh votes only” rule. The new buffer is not slot-scoped (crates/storage/src/store.rs), and attestation validation only rejects future votes, not delayed old ones (crates/blockchain/src/store.rs). A withheld slot n-1 aggregate received during slot n will still live in new and can still advance safe_target on stale evidence. If the intended fix is “live participation only”, this path also needs a slot/round filter before calling compute_lmd_ghost_head.

I didn’t see another concrete bug in the diff itself. I couldn’t run the Rust test suite here because Cargo needs network/write access for the lean-multisig git dependency cache, which is blocked in this environment.

---

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