refactor(fork-choice): unify ancestor-at-slot walks into one helper - #1146
Closed
tcoratger wants to merge 1 commit into
Closed
refactor(fork-choice): unify ancestor-at-slot walks into one helper#1146tcoratger wants to merge 1 commit into
tcoratger wants to merge 1 commit into
Conversation
The two hand-rolled "climb parent links to the block at a target slot" walks in fork choice are now a single private helper that returns the ancestor root at that slot, or None when no block sits there or the chain leaves the known tree. This is behavior-preserving consolidation, not a behavior change. The ancestry-check walk (in attestation validation) and the finalized-root walk (in head update) were verified to be the same underlying lookup: - The ancestry check is now: the resolved block at the ancestor's slot equals the ancestor's exact root. None there means off-chain, so the predicate returns False, exactly as before. The early ancestor-after-descendant guard is kept. - The head-update walk uses the resolved root to rebuild the finalized checkpoint, and falls back to the trusted anchor when the helper returns None. The original landed on a too-high or skipped-slot block and took the same else branch, so the emitted checkpoint is identical. The weight-accumulation walk is a different operation: it credits every block above a start slot rather than finding one ancestor, so it is left untouched. None must be handled explicitly before comparing to a Bytes32, since the strict Bytes32 equality rejects comparison with None. Verified: just check passes; all 118 fork-choice consensus vectors regenerate and pass, and the broader lstar vector set is byte-identical across hash seeds (determinism check passed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The lstar fork choice hand-rolled the same "climb parent links up to the block at a target slot" walk in two places with slightly different stop conditions. This factors them into one private helper that returns the ancestor root at that slot, or
Nonewhen no block sits there or the chain leaves the known tree.None.The third parent-climbing loop (weight accumulation) is a different operation — it credits every block above a start slot rather than finding one ancestor — so it is intentionally left untouched.
Why
The divergent stop conditions across duplicated walks were a correctness hazard. Consolidating to one helper with a single documented stop condition removes that drift risk.
Equivalence argument (behavior-preserving)
The two consolidated walks are the same underlying lookup: "climb from a root until the block whose slot equals the target; return that root, else
None." Edge cases verified to map identically:False(guard kept).None-> ancestryFalse/ head-update keeps the trusted anchor, same as the original's too-low landing.None-> same fallbacks; the original broke and took itselsebranch because the landed slot was still above the target.Noneis handled explicitly before anyBytes32comparison, since the strictBytes32equality rejects comparison withNone.Verification
just check: All checks passed.uv run fill --fork=lstar --clean -k fork_choice -n auto: all 118 fork-choice vectors regenerate and pass; the broader lstar vector set is byte-identical across hash seeds (determinism check passed).🤖 Generated with Claude Code