Wire safeTarget validation into fork-choice spec test harness

[pablodeymo]

Summary

Follow-up to #316. Wires safeTarget, safeTargetSlot, and safeTargetRootLabel into the fork-choice spec-test harness so regenerated fixtures actually assert on update_safe_target behavior.

Motivation

#316 changed update_safe_target to consume only the new attestation pool, in line with leanSpec PR #680. The behavior change is correct but had no fixture-level coverage:

• The legacy safeTarget field already exists on StoreChecks but the harness explicitly rejects it with "'safeTarget' check not supported" (crates/blockchain/tests/forkchoice_spectests.rs:224-226). Any fixture using it errors out instead of validating.
• The new safeTargetSlot / safeTargetRootLabel fields from leanSpec #680 aren't on the struct at all, so serde silently drops them when fixtures are regenerated.

The behavior is therefore only observable indirectly via attestationTargetSlot (because safe_target clamps the attestation walk-back in get_attestation_target_with_checkpoints), which doesn't pin down where safe_target actually landed. #316's PR description already flagged this gap; this PR closes it.

Changes

• crates/blockchain/tests/types.rs:

  • Add safeTargetSlot: Option<u64> and safeTargetRootLabel: Option<String> to StoreChecks, mirroring the latestJustified* / latestFinalized* pattern.
  • Reorganize fields into logical groups (time, head, justified, finalized, safe-target, attestation/lexicographic).
  • Drop the misleading // Unsupported fields (will error if present) comment block. Only safeTarget actually errored; the rest (headRootLabel, latestJustified*, latestFinalized*, lexicographicHeadAmong) were all wired and validated.

• crates/blockchain/tests/forkchoice_spectests.rs:

  • Replace the safeTarget rejection branch with the same label-resolution pattern used for justified/finalized roots: safeTarget falls back to safeTargetRootLabel resolved through the step's block_registry.
  • Add a safeTargetSlot validation block comparing against st.safe_target_slot().
  • Add a resolved-root validation block comparing against st.safe_target().

Forward compatibility

No current fixture in leanSpec/fixtures/consensus/ carries any safeTarget* field (verified via rg -l "safeTarget" leanSpec/fixtures/consensus), so the new validation paths are dormant until LEAN_SPEC_COMMIT_HASH is bumped to a revision that ships leanSpec #680. Zero risk of breaking existing tests; the change only takes effect once upstream regenerates fixtures with the new schema.

Test plan

• cargo fmt --all
• cargo clippy --workspace --all-targets -- -D warnings
• cargo test --workspace --release --lib --bins — all unit tests pass
• cargo test -p ethlambda-blockchain --release --test forkchoice_spectests — 47/47 fixtures pass against the currently-pinned LEAN_SPEC_COMMIT_HASH
• Once leanSpec #680 lands and LEAN_SPEC_COMMIT_HASH is bumped, regenerated fixtures will exercise the new assertions end-to-end

Related

• Closes the test-coverage half of fix(forkchoice): use only "new" pool for safe target (leanSpec #680) #316.
• Mirrors the validation patterns used for latestJustifiedRoot / latestFinalizedRoot.

[github-actions]

🤖 Kimi Code Review

The changes correctly extend the fork choice test runner to support the leanSpec #680 schema for safe target validation. The implementation follows existing patterns for root resolution and validation.

crates/blockchain/tests/forkchoice_spectests.rs

• Lines 224-229: The resolution logic correctly prioritizes the direct safe_target root over the label-based lookup, consistent with how head_root and justified_root are handled.
• Lines 336-359: Validation logic properly mirrors the existing attestation_target_slot and finalized root checks. Error messages are consistent with the codebase style.

crates/blockchain/tests/types.rs

• Lines 141-185: Documentation improvements are excellent—clarifying the optional nature of fields and the label resolution mechanism improves maintainability.
• Lines 166-172: The field grouping (justified, finalized, safe target) is logical and follows the struct's existing organization.

Minor suggestion:

In types.rs, consider adding a brief doc comment for lexicographic_head_among (line 185) to maintain consistency with the newly documented fields, even if it's just noting that it's currently unsupported or used for tie-breaking assertions.

Verification note: Ensure that st.safe_target() and st.safe_target_slot() implementations in the production code correctly implement the 3SF-mini (Three-Slot Finality) safe target logic as specified in leanSpec #680. The test infrastructure changes assume these methods exist and return the correct types (H256 and u64 respectively).

Approval: LGTM. The pattern correctly generalizes the existing root-label resolution infrastructure to support the new safe target fields without breaking legacy test fixtures that specify safeTarget directly.

---

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

[greptile-apps]

Greptile Summary

This PR wires safeTarget, safeTargetSlot, and safeTargetRootLabel into the fork-choice spec-test harness by adding the new fields to StoreChecks and replacing the old rejection branch with the same label-resolution pattern used for latestJustified* / latestFinalized*. The changes are dormant until the upstream LEAN_SPEC_COMMIT_HASH is bumped to a leanSpec #680 revision that ships fixtures with these fields.

Confidence Score: 4/5

Safe to merge; all existing 47 fixtures pass and new code paths are dormant until upstream fixtures change.

Only P2 findings — a minor forward-compat struct symmetry concern. No logic errors, no security issues, and the changes closely mirror the well-tested justified/finalized patterns.

No files require special attention; the one note is in types.rs regarding a potentially missing safeTargetRoot field for future schema symmetry.

Important Files Changed

Filename	Overview
crates/blockchain/tests/types.rs	Adds safe_target_slot and safe_target_root_label to StoreChecks, reorganizes fields into logical groups, and removes the misleading "Unsupported fields" comment — straightforward struct extension that mirrors the existing justified/finalized pattern.
crates/blockchain/tests/forkchoice_spectests.rs	Replaces the safeTarget rejection branch with label-resolution logic and adds safeTargetSlot / safeTarget root validation blocks; logic is consistent with existing justified/finalized patterns but a missing safeTargetRoot (non-label) direct-root field could silently drop fixtures that supply it.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[validate_checks called] --> B{safe_target set?}
    B -- yes --> C[resolved_safe_target_root = safe_target H256]
    B -- no --> D{safe_target_root_label set?}
    D -- yes --> E[lookup label in block_registry]
    E --> F[resolved_safe_target_root = root from registry]
    D -- no --> G[resolved_safe_target_root = None]

    C --> H{safe_target_slot set?}
    F --> H
    G --> H

    H -- yes --> I[compare st.safe_target_slot vs expected_slot]
    I -- mismatch --> J[return Err]
    I -- match --> K{resolved_safe_target_root set?}
    H -- no --> K

    K -- yes --> L[compare st.safe_target vs expected_root]
    L -- mismatch --> M[return Err]
    L -- match --> N[continue validation]
    K -- no --> N

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: crates/blockchain/tests/types.rs
Line: 170-178

Comment:
**Missing `safeTargetRoot` (direct-root) field for new schema**

The new leanSpec #680 schema is paired as `safeTargetSlot` + `safeTargetRootLabel`. If a future fixture revision also ships a direct-root counterpart named `safeTargetRoot` (mirroring the `latestJustifiedRoot` / `latestFinalizedRoot` pattern rather than reusing the legacy `safeTarget` field), serde will silently discard it and the validation block in `forkchoice_spectests.rs` will never fire. Consider adding `safeTargetRoot: Option<H256>` now so the struct schema is symmetric with `latestJustified*` / `latestFinalized*`, even if it stays dormant until fixtures use it.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "Wire safeTarget store checks in the fork..." | Re-trigger Greptile}

[greptile-apps]

Missing safeTargetRoot (direct-root) field for new schema

The new leanSpec #680 schema is paired as safeTargetSlot + safeTargetRootLabel. If a future fixture revision also ships a direct-root counterpart named safeTargetRoot (mirroring the latestJustifiedRoot / latestFinalizedRoot pattern rather than reusing the legacy safeTarget field), serde will silently discard it and the validation block in forkchoice_spectests.rs will never fire. Consider adding safeTargetRoot: Option<H256> now so the struct schema is symmetric with latestJustified* / latestFinalized*, even if it stays dormant until fixtures use it.

Prompt To Fix With AI

This is a comment left during a code review.
Path: crates/blockchain/tests/types.rs
Line: 170-178

Comment:
**Missing `safeTargetRoot` (direct-root) field for new schema**

The new leanSpec #680 schema is paired as `safeTargetSlot` + `safeTargetRootLabel`. If a future fixture revision also ships a direct-root counterpart named `safeTargetRoot` (mirroring the `latestJustifiedRoot` / `latestFinalizedRoot` pattern rather than reusing the legacy `safeTarget` field), serde will silently discard it and the validation block in `forkchoice_spectests.rs` will never fire. Consider adding `safeTargetRoot: Option<H256>` now so the struct schema is symmetric with `latestJustified*` / `latestFinalized*`, even if it stays dormant until fixtures use it.

How can I resolve this? If you propose a fix, please make it concise.

[github-actions]

🤖 Claude Code Review

Now I have enough context to write the review. Here it is:

---

PR 322 Review: Wire safeTarget validation into fork-choice spec test harness

Overview

This PR closes a test-coverage gap introduced by PR 316: the safeTarget field on StoreChecks was always present in the struct but its validation was hardwired to error out ("'safeTarget' check not supported"), meaning no fixture could ever exercise update_safe_target behavior. The PR replaces that rejection with the same label-resolution pattern already used for justified/finalized roots, and adds the two new leanSpec #680 fields (safeTargetSlot, safeTargetRootLabel).

---

Code Correctness

Resolution logic is correct. The .or_else() chain in forkchoice_spectests.rs:224-229 mirrors the resolved_head_root / resolved_justified_root / resolved_finalized_root patterns identically. If a fixture supplies safeTarget (raw H256, legacy schema), it takes priority; otherwise the label lookup fires. No off-by-one or short-circuit mistake.

Schema asymmetry is intentional and correct. The justified/finalized pattern has three fields each (*Slot, *Root, *RootLabel). The safe-target pattern has only safeTargetSlot and safeTargetRootLabel for the new schema — no safeTargetRoot — because leanSpec #680 emits a label, not a raw root. The legacy safeTarget: Option<H256> is reused as the raw-root fallback. This is fine as long as leanSpec #680 never emits a raw safeTargetRoot field; the PR description confirms this.

Validation order is consistent. Slot check precedes root check (lines 336–358), matching the justified/finalized ordering.

---

Potential Issues

Silent skip when safeTargetRootLabel doesn't resolve (pre-existing, not introduced here). If a fixture carries safeTargetRootLabel: "some_block" but block_registry doesn't contain "some_block", the and_then returns None, resolved_safe_target_root is None, and the validation block at line 349 silently passes. The same behavior exists for headRootLabel, latestJustifiedRootLabel, and latestFinalizedRootLabel. Worth raising as a follow-up, but not a regression introduced by this PR.

safe_target_slot() panics on missing header (pre-existing). storage/src/store.rs:1167 uses .expect("safe target exists"). In the spec-test harness KEY_SAFE_TARGET is always initialized to anchor_block_root, so this is sound; but if a future fixture somehow reaches a state where safe_target() returns a root with no stored header, the test harness would panic rather than emit a structured error. Again not introduced here.

---

Style and Conventions

Struct reorganization is a clear improvement. Grouping fields by logical concern (time → head → justified → finalized → safe-target → attestation/lexicographic) with blank-line separators makes StoreChecks much easier to scan. No issues.

Doc comment on StoreChecks is warranted. CLAUDE.md says "only add a comment when the WHY is non-obvious." The non-obvious constraint here — that label companions mirror the leanSpec fixture schema — is exactly what the doc comment explains. It's appropriately placed on the struct rather than bloating individual fields.

Field-level /// comments on safe_target and safe_target_slot/safe_target_root_label are proportionate. These document the schema-version split (legacy vs leanSpec #680), which would genuinely confuse a future reader without the PR description context.

{:?} format for roots in error messages (lines 353–354) is consistent with how justified and finalized root mismatches are reported, so no concern.

---

Test Coverage

No existing fixture carries any safeTarget* field (per rg -l "safeTarget" leanSpec/fixtures/consensus), so all new validation paths are dormant until LEAN_SPEC_COMMIT_HASH is bumped to a revision shipping leanSpec #680. The 47 existing fixtures continue to pass unchanged. This is the correct staged approach — plumbing before fixtures.

---

Summary

The change is minimal, focused, and correct. It implements exactly what was described in the PR 316 gap note. The pattern is a faithful copy of the existing label-resolution infrastructure and introduces no new logic risks. The one concern worth tracking as a follow-up (silent skip on unresolved labels) predates this PR and applies equally to all *RootLabel fields.

No blocking issues. Ready to merge once CI is green.

---

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

[github-actions]

🤖 Codex Code Review

Findings

1. crates/blockchain/tests/forkchoice_spectests.rs:224 and crates/blockchain/tests/forkchoice_spectests.rs:349: safeTargetRootLabel is resolved with block_registry.get(...).copied(), and a missing label just turns into None, which silently skips the root assertion. That means a typo or out-of-order fixture label can make the safe-target root check pass without checking anything, which weakens consensus test coverage. I’d make missing labels fail loudly, the same way crates/blockchain/tests/forkchoice_spectests.rs:472 already does for lexicographicHeadAmong.

2. crates/blockchain/tests/forkchoice_spectests.rs:338: the new safeTargetSlot path calls Store::safe_target_slot(), which expects that the safe-target header exists. If a fork-choice regression ever leaves safe_target pointing at a missing/pruned root, this test will panic instead of returning a step-local failure with useful context. In a consensus test harness, that makes debugging harder than an explicit Err(...).

Notes
No production fork-choice, attestation, state-transition, XMSS, or SSZ code changed here, so I don’t see a direct runtime security/performance risk in the PR itself beyond the harness behavior above.

I couldn’t run cargo +stable test -p ethlambda_blockchain forkchoice_spectests -- --nocapture in this sandbox: cargo needs to fetch/write the leanMultisig git dependency, and the environment blocks that.

---

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

[MegaRedHand]

I don't like that we are skipping a test here. Let's check again once #317 is merged, since it bumps leanSpec commit to latest.