From df6f92c435049ba90119eeb37d6fe9bef1e8b0a9 Mon Sep 17 00:00:00 2001 From: Thomas Coratger <60488569+tcoratger@users.noreply.github.com> Date: Sun, 17 May 2026 23:48:51 +0200 Subject: [PATCH 1/2] fix(forkchoice): correct LMD-GHOST orphan-skip, tighten checkpoint advance, assert start_root Three correctness fixes surfaced by the consensus-researcher review. Behavior on the hot path is preserved; an inconsistency between on_block and build_block tie semantics is resolved in favor of the documented "store-authoritative on tie" rule. 1. _compute_lmd_ghost_head orphan-skip was dead code. `if not block.parent_root: continue` was always False because `block.parent_root` is `Bytes32` (always 32 bytes, never empty). Genesis blocks landed under children_map[Bytes32.zero()] instead of being skipped. The bucket was never consulted because the walk anchors at the justified root and only descends. The misleading filter is removed; a comment now explains why genesis cannot pollute the walk. 2. Checkpoint advance semantics centralized in Checkpoint.advance_to. Two `max(...)` call sites in lstar/spec.py had opposite argument orders, which silently produced opposite tie behavior. The comment at on_block explicitly documents "store wins on tie"; build_block contradicted that. Both sites now use store.latest_*.advance_to(...) which encodes the documented intent in the type itself. 3. _compute_lmd_ghost_head now asserts start_root is a known block. Previously a bad anchor produced a cryptic KeyError deep in the weight loop. The assert states the invariant up front. An existing test that constructed a malformed store is updated to expect the clearer AssertionError. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lean_spec/forks/lstar/spec.py | 40 ++++++++++--------- src/lean_spec/types/checkpoint.py | 10 +++++ .../forkchoice/test_compute_block_weights.py | 16 +++++++- .../forks/lstar/forkchoice/test_validator.py | 5 ++- .../subspecs/containers/test_checkpoint.py | 29 ++++++++++++++ 5 files changed, 80 insertions(+), 20 deletions(-) diff --git a/src/lean_spec/forks/lstar/spec.py b/src/lean_spec/forks/lstar/spec.py index ae210fb59..8739e9efc 100644 --- a/src/lean_spec/forks/lstar/spec.py +++ b/src/lean_spec/forks/lstar/spec.py @@ -1287,14 +1287,14 @@ def on_block( # Propagate checkpoint advances from the post-state. # - # Keep the checkpoint with the higher slot. - # On slot ties, prefer the store's own checkpoint. + # A candidate replaces the store's checkpoint only when its slot is strictly higher. + # On slot ties the store's view stays authoritative. # - # The store's checkpoint is pinned to the anchor at init and only - # moves forward via real justification/finalization events. - # On ties the store's view is authoritative. - latest_justified = max(store.latest_justified, post_state.latest_justified) - latest_finalized = max(store.latest_finalized, post_state.latest_finalized) + # Why: the store's checkpoint is pinned at init. + # It advances only on real justification or finalization events. + # An incoming tie must not silently swap roots. + latest_justified = store.latest_justified.advance_to(post_state.latest_justified) + latest_finalized = store.latest_finalized.advance_to(post_state.latest_finalized) store = store.model_copy( update={ @@ -1416,6 +1416,10 @@ def _compute_lmd_ghost_head( When two branches have equal weight, the one with the lexicographically larger hash is chosen to break ties. """ + # Invariant: the anchor must be a block the store already knows. + # A loud failure here beats a cryptic missing-key error deep in the weight loop. + assert start_root in store.blocks, f"start_root {start_root.hex()} not in store.blocks" + # Remember the slot of the anchor once and reuse it during the walk. # # This avoids repeated lookups inside the inner loop. @@ -1439,17 +1443,15 @@ def _compute_lmd_ghost_head( weights[current_root] += 1 current_root = store.blocks[current_root].parent_root - # Build the adjacency tree (parent -> children). + # Build the parent -> children adjacency. # - # We use a defaultdict to avoid checking if keys exist. + # Genesis blocks land in the bucket keyed by the zero hash. + # That bucket is never consulted. + # The walk anchors at the latest justified root and only descends. children_map: dict[Bytes32, list[Bytes32]] = defaultdict(list) for root, block in store.blocks.items(): - # 1. Structural check: skip blocks without parents (e.g., purely genesis/orphans) - if not block.parent_root: - continue - - # 2. Heuristic check: prune branches early if they lack sufficient weight + # Prune low-weight branches early when a threshold is set. if min_score > 0 and weights[root] < min_score: continue @@ -1948,10 +1950,12 @@ def produce_block_with_signatures( # Update checkpoints from post-state. # # Locally produced blocks bypass normal block processing. - # We must manually propagate any checkpoint advances. - # Higher slots indicate more recent justified/finalized states. - latest_justified = max(final_post_state.latest_justified, store.latest_justified) - latest_finalized = max(final_post_state.latest_finalized, store.latest_finalized) + # Checkpoint advances must be propagated manually here. + # + # Tie semantics mirror the block-import path. + # A candidate needs a strictly higher slot to replace the store's view. + latest_justified = store.latest_justified.advance_to(final_post_state.latest_justified) + latest_finalized = store.latest_finalized.advance_to(final_post_state.latest_finalized) # Persist block and state immutably. new_store = store.model_copy( diff --git a/src/lean_spec/types/checkpoint.py b/src/lean_spec/types/checkpoint.py index d689f986e..2a019637b 100644 --- a/src/lean_spec/types/checkpoint.py +++ b/src/lean_spec/types/checkpoint.py @@ -27,3 +27,13 @@ def __lt__(self, other: "Checkpoint") -> bool: return NotImplemented # Slot drives the order; equal slots leave the pair incomparable. return self.slot < other.slot + + def advance_to(self, candidate: "Checkpoint") -> "Checkpoint": + """ + Return the later of two checkpoints, keeping self on a slot tie. + + Forward-only progression for justified and finalized checkpoints. + + The candidate replaces the receiver only when its slot is strictly higher. + """ + return candidate if candidate.slot > self.slot else self diff --git a/tests/lean_spec/forks/lstar/forkchoice/test_compute_block_weights.py b/tests/lean_spec/forks/lstar/forkchoice/test_compute_block_weights.py index 94dcfa4b6..2bcc04ce0 100644 --- a/tests/lean_spec/forks/lstar/forkchoice/test_compute_block_weights.py +++ b/tests/lean_spec/forks/lstar/forkchoice/test_compute_block_weights.py @@ -2,12 +2,14 @@ from __future__ import annotations +import pytest + from lean_spec.forks.lstar import Store from lean_spec.forks.lstar.containers.attestation import AttestationData from lean_spec.forks.lstar.spec import LstarSpec from lean_spec.subspecs.ssz.hash import hash_tree_root from lean_spec.subspecs.xmss.aggregation import AggregatedSignatureProof -from lean_spec.types import Checkpoint, Slot, ValidatorIndex, ValidatorIndices +from lean_spec.types import Bytes32, Checkpoint, Slot, ValidatorIndex, ValidatorIndices from lean_spec.types.byte_arrays import ByteListMiB from tests.lean_spec.helpers import make_bytes32, make_signed_block @@ -125,3 +127,15 @@ def test_multiple_attestations_accumulate(spec: LstarSpec, base_store: Store) -> # Both validators contribute weight to block1 assert weights == {block1_root: 2} + + +def test_compute_lmd_ghost_head_rejects_unknown_start_root( + spec: LstarSpec, base_store: Store +) -> None: + """An anchor missing from the store fails the head-walk invariant loudly.""" + # A 32-byte root that is guaranteed not to be in the store. + unknown_root = Bytes32(b"\xee" * 32) + assert unknown_root not in base_store.blocks + + with pytest.raises(AssertionError, match="not in store.blocks"): + spec._compute_lmd_ghost_head(base_store, start_root=unknown_root, attestations={}) diff --git a/tests/lean_spec/forks/lstar/forkchoice/test_validator.py b/tests/lean_spec/forks/lstar/forkchoice/test_validator.py index d14e800fc..1da6c6d3b 100644 --- a/tests/lean_spec/forks/lstar/forkchoice/test_validator.py +++ b/tests/lean_spec/forks/lstar/forkchoice/test_validator.py @@ -407,7 +407,10 @@ def test_produce_block_missing_parent_state(self, spec: LstarSpec) -> None: validator_id=TEST_VALIDATOR_ID, ) - with pytest.raises(KeyError): # Missing head in get_proposal_head + # The forkchoice head walk asserts that the justified root is known. + # Calling produce_block on a store whose latest_justified.root is + # missing from blocks must fail loudly with that invariant message. + with pytest.raises(AssertionError, match="not in store.blocks"): spec.produce_block_with_signatures(store, Slot(1), ValidatorIndex(1)) def test_validator_operations_invalid_parameters( diff --git a/tests/lean_spec/subspecs/containers/test_checkpoint.py b/tests/lean_spec/subspecs/containers/test_checkpoint.py index 5d87d0e29..064c2a107 100644 --- a/tests/lean_spec/subspecs/containers/test_checkpoint.py +++ b/tests/lean_spec/subspecs/containers/test_checkpoint.py @@ -66,3 +66,32 @@ def test_max_keeps_first_argument_on_slot_tie() -> None: b = Checkpoint(root=ROOT_B, slot=Slot(5)) assert max(a, b) == a assert max(b, a) == b + + +def test_advance_to_returns_candidate_on_higher_slot() -> None: + """A candidate at a strictly higher slot replaces the receiver.""" + current = Checkpoint(root=ROOT_A, slot=Slot(3)) + candidate = Checkpoint(root=ROOT_B, slot=Slot(4)) + assert current.advance_to(candidate) == candidate + + +def test_advance_to_keeps_self_on_lower_slot() -> None: + """A candidate at a lower slot is ignored.""" + current = Checkpoint(root=ROOT_A, slot=Slot(4)) + candidate = Checkpoint(root=ROOT_B, slot=Slot(3)) + assert current.advance_to(candidate) == current + + +def test_advance_to_keeps_self_on_slot_tie() -> None: + """On a slot tie the receiver wins regardless of root.""" + current = Checkpoint(root=ROOT_A, slot=Slot(7)) + candidate = Checkpoint(root=ROOT_B, slot=Slot(7)) + assert current.advance_to(candidate) == current + # Symmetric: the receiver of the call always wins on a tie. + assert candidate.advance_to(current) == candidate + + +def test_advance_to_is_idempotent() -> None: + """Calling against the same checkpoint returns the receiver unchanged.""" + cp = Checkpoint(root=ROOT_A, slot=Slot(2)) + assert cp.advance_to(cp) == cp From 6947d3ea9a71eb7432ba2db359debf00c2e479c8 Mon Sep 17 00:00:00 2001 From: Thomas Coratger <60488569+tcoratger@users.noreply.github.com> Date: Sun, 17 May 2026 23:49:29 +0200 Subject: [PATCH 2/2] style(checkpoint): split module docstring sentences per doc rules Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lean_spec/types/checkpoint.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/lean_spec/types/checkpoint.py b/src/lean_spec/types/checkpoint.py index 2a019637b..85299f9ef 100644 --- a/src/lean_spec/types/checkpoint.py +++ b/src/lean_spec/types/checkpoint.py @@ -1,9 +1,11 @@ """ -Checkpoint Container. +Checkpoint container. -A checkpoint marks a specific moment in the chain. It combines a block -identifier with a slot number. Checkpoints are used for justification and -finalization. +A checkpoint marks a specific moment in the chain. + +It combines a block identifier with a slot number. + +Checkpoints are used for justification and finalization. """ from lean_spec.types.byte_arrays import Bytes32