test(fork-choice): wire FCR EF spec test runner - #337
Conversation
Add empty FastConfirmation module skeleton with struct fields, init/deinit, and CONFIRMATION_BYZANTINE_THRESHOLD config constant. Spec helpers and algorithm follow in subsequent commits. Spec: ethereum/consensus-specs#4747 Reference: Lighthouse PR sigp/lighthouse#8951
- Drop unused Error.InvalidByzantineThreshold (YAGNI; init silently clamps per Lighthouse precedent, no caller fails on out-of-range yet). - Widen byzantine_threshold and proposer_score_boost from u8 to u64 to match ChainConfig field types and Lighthouse reference, eliminating @intcast at every future call site. - Drop unused Epoch and ValidatorIndex imports (re-introduce in phases that use them).
Implement spec misc helpers (isStartSlotAtEpoch, getBlockSlot, getBlockEpoch, getCheckpointForBlock, isAncestor, getAncestorRoots, getCurrentTarget) and state helpers (BalanceSourceData.rebuild, SlotAssignments.rebuild, getSlotCommittee). Spec: specs/phase0/fast-confirmation.md (Misc + State helpers sections)
- Widen FCR Error to include ForkChoiceError variants. getCheckpointForBlock and isAncestor now match only "block not found" variants (MissingProtoArrayBlock, UnknownAncestor), propagating corruption signals (BeaconStateErr, InvalidParentIndex, etc.) instead of collapsing them. - Change isAncestor signature to Error!bool so non-not-found errors surface. - Tighten BalanceSourceData.rebuild and SlotAssignments.rebuild to explicit Error!void return type. State subsystem errors are mapped to StateMissing; OutOfMemory propagates separately. - Fix misleading TigerStyle comment on BalanceSourceData.rebuild's validators.len > 0 assert (it asserts, doesn't silently return). - Add upper-bound assert on SlotAssignments.rebuild slot range. - Drop stale "(rebuild added in Phase B)" parentheticals from section banners.
Implement 12 LMD-GHOST safety threshold helpers per spec: isFullValidatorSetCovered, adjustCommitteeWeightEstimateToEnsureSafety, estimateCommitteeWeightBetweenSlots, getEquivocationScore, computeAdversarialWeight, getAdversarialWeight, getBlockSupportBetweenSlots, computeEmptySlotSupportDiscount, getSupportDiscount, computeSafetyThreshold, isOneConfirmed, isConfirmedChainSafe. Spec: specs/phase0/fast-confirmation.md (LMD-GHOST helpers section)
- Document BalanceSourceData.rebuild's caller invariant (state's epoch must
match cp.epoch) since active/slashed predicates evaluate at cp.epoch.
- Rename two tests that didn't actually exercise the branch their names
promised:
- "computeAdversarialWeight basic saturation behavior" ->
"empty equivocating set returns positive max" (saturation-to-zero
requires committee-aware setup, out of scope for unit test).
- "computeSafetyThreshold underflow guard saturates to 0" ->
"degenerate zero inputs return zero breakdown" (underflow branch needs
empty-slot parent-support setup, out of scope for unit test).
Implement 4 FFG helpers per spec: getCurrentTargetScore, computeHonestFfgSupportForCurrentTarget, willNoConflictingCheckpointBeJustified, willCurrentTargetBeJustified. Spec: specs/phase0/fast-confirmation.md (FFG helpers section)
- Rename two tests whose names overstated coverage:
- "computeHonestFfgSupportForCurrentTarget bounded by total active
balance" -> "zero-vote upper bound" (only the trivial branch is
exercised; positive-vote case requires more involved fixture).
- "willNoConflictingCheckpointBeJustified insufficient honest yields
false" -> "zero-balance degenerate case yields false" (named branch
requires committee + ffg_weight setup out of scope for unit test).
- Add Phase E TODO note on the redundant getTotalActiveBalance scan in
willNoConflictingCheckpointBeJustified (called once at top, again
transitively via computeHonestFfgSupportForCurrentTarget).
Implement updateFastConfirmationVariables, findLatestConfirmedDescendant, getLatestConfirmed (private algorithm methods) plus onFastConfirmation and runConfirmation public entry points. Algorithm is a direct port of spec pseudocode; spec test mode lets the runner trigger confirmation explicitly. Spec: specs/phase0/fast-confirmation.md (algorithm + handlers sections)
Add fast_confirmation runner under test/spec/runner/, register it in
runner_kind, extend write_spec_tests to discover fast_confirmation
vectors, and implement step parser + 6 FCR-specific check assertions
(previous_epoch_observed_justified_checkpoint,
current_epoch_observed_justified_checkpoint,
previous_epoch_greatest_unrealized_checkpoint, previous_slot_head,
current_slot_head, confirmed_root) on top of the existing fork-choice
fields (head, justified, finalized, proposer_boost_root).
Step types handled: tick, attestation, block, attester_slashing, checks.
attestation/attester_slashing failures are tolerated (best-effort) and
do not abort the run; checks-step failures surface to the test runner.
runConfirmation runs immediately before each checks step in spec_test
mode, matching the spec's implicit "on_fast_confirmation at slot start"
contract.
Generated 1014 fast_confirmation tests (6 forks x 9 suites) from
ethereum/consensus-specs v1.7.0-alpha.6 (PR #4747 merged 2026-04-16).
0 / 1014 currently pass: 1002 fail with FcrPrevUnrealizedRootMismatch
(real Phase E bug — updateFastConfirmationVariables overwrites
greatest_unrealized with the head's zero unrealized at epoch boundaries
instead of preserving the maximum), 12 fail with HeadRootMismatch
(downstream of the same bug). Failure breakdown is documented in
docs/plans/2026-04-29-fcr-known-failing.md; fixes deferred to follow-up
commits per Phase F mandate ("don't hack the FCR to make tests pass").
Two latent compile errors in src/fork_choice/fork_choice.zig surfaced
the moment the runner exercises onBlock; both are fixed minimally:
- computeUnrealizedCheckpoints now passes (allocator, io, state)
matching the helper's 3-arg signature.
- OnBlockBalancesCtx now carries an explicit allocator so that
EffectiveBalanceIncrementsRc.init can be called against the
Zig-0.16 allocator-less ArrayListUnmanaged.
Spec: specs/phase0/fast-confirmation.md
Test format: tests/formats/fast_confirmation/README.md
Vectors: ethereum/consensus-specs v1.7.0-alpha.6
Phase E confused two different unrealized-justified concepts: - store.unrealized_justified_checkpoint (global, used in update_fast_confirmation_variables per spec line 814-816) - store.unrealized_justifications[head] (per-head, used by FFG helpers per spec line 770, 1003) The runner had been passing the head's per-block unrealized_justified to update_fast_confirmation_variables, which at anchor blocks carries ZERO_HASH. This stomped fcr.previous_epoch_greatest_unrealized_checkpoint to zero at every epoch boundary, cascading into all FCR field mismatches. Fix: updateFastConfirmationVariables takes *const ForkChoice and reads the global value directly from fc.fc_store.unrealized_justified.checkpoint. The per-head head_unrealized_justified parameter on entry points (onFastConfirmation/runConfirmation) keeps its meaning and continues feeding the FFG helpers downstream — those usages were correct. Spec test pass rate jumps from 0/1014 to 4006/4798 (~83.5%) on minimal preset; remaining 792 FcrConfirmedRootMismatch failures will be diagnosed in follow-up commits. Surfaced by Phase F EF spec test runner.
Spec line 999 (`get_latest_confirmed`) gates the "restart from observed
justified" branch on the BLOCK's slot's epoch:
is_observed_justified_block_epoch_ok = (
compute_epoch_at_slot(observed_justified_block_slot) + 1 == current_epoch
)
Phase E had an outer guard `observed.epoch + 1 == current_epoch` using the
checkpoint's `epoch` field — these can differ when the checkpoint's root
is an empty-slot block from an earlier epoch (e.g., checkpoint at epoch 2
but its root block is at slot 15, epoch 1). The inner correct check
(observed_block_epoch + 1 == current_epoch) is preserved.
Also add diagnostic logging to test/spec/runner/fast_confirmation.zig so
future failure-debugging can trace expected/actual confirmed_root and
the FCR variables alongside.
Note: this fix is spec-aligned but does not change the spec test pass
count (4006/4798 unchanged). The cases that fail don't hit this gate.
The remaining 792 failures need per-case algorithmic trace, suspected
in `findLatestConfirmedDescendant`'s isOneConfirmed branch (balance
source semantics or safety-threshold computation).
Traced one specific failure (altair fcr_current_epoch_12 slot 18) to its root cause: head's per-block unrealized_justified is not advancing. By slot 18 with attestations applied, spec expects head's UJ to be at epoch 1+, but our impl keeps it at anchor (epoch 0). This makes Loop 1 and Loop 2 of findLatestConfirmedDescendant both fail their guards, preventing confirmation advance. The bug is upstream of FCR — in fork_choice.zig's computeUnrealizedCheckpoints integration with onBlock/onAttestation. Phase A-E unit tests didn't catch it because they inject pre-populated unrealized fields into fixtures rather than computing them. Removing the verbose debug-mismatch logging from runner.zig (added during diagnosis); update known-failing.md with the trace findings.
…c tests The EF FCR spec test runner needs unrealized checkpoints populated on proto-array nodes (so updateFastConfirmationVariables can read previous_epoch_greatest_unrealized_checkpoint correctly), but it does NOT want those unrealized values pulled up into the realized justified/finalized store checkpoints — the spec tests fix the realized checkpoints to anchor values throughout each test. Add a third mode to ForkChoiceOpts: - compute_unrealized: existing — compute + pull up - compute_unrealized_without_pull_up: compute, do NOT pull up The FCR runner uses the new flag; production callers stay on compute_unrealized. Also propagate io: std.Io through fast_confirmation runner's fc.onBlock call to match the Zig 0.16 onBlock signature on this base branch.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates the Fast Confirmation Rule (FCR) algorithm into the repository and establishes the necessary infrastructure to run EF spec tests for it. By extending the existing fork-choice runner and introducing specialized flags, the changes enable accurate validation of FCR logic while maintaining compatibility with the existing fork-choice state machine. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the Fast Confirmation (FCR) spec test runner and integrates the CONFIRMATION_BYZANTINE_THRESHOLD configuration across network presets. Key changes include a new test runner in test/spec/runner/fast_confirmation.zig, updates to ForkChoice to support an unrealized-without-pull-up mode for spec testing, and the addition of FCR test generation logic. Feedback focuses on addressing a potential memory leak in the test runner's state handling, improving YAML parsing robustness to handle quoted strings, and refactoring functions that exceed the style guide's 70-line limit. It is also recommended to use the configured byzantine threshold instead of a hardcoded value.
| defer { | ||
| current_state.deinit(); | ||
| allocator.destroy(current_state); | ||
| } |
There was a problem hiding this comment.
There is a potential memory leak of AnyBeaconState pointers. The any_state allocated at line 387 (and subsequent states returned by applyBlock) are pointers to heap-allocated memory. While current_state.deinit() cleans up internal resources, it does not destroy the state pointer itself. The runner should explicitly destroy the state pointer when destroying the CachedBeaconState wrapper.
var current_state: *CachedBeaconState = initial_cached_state;
defer {
const state_to_destroy = current_state.state;
current_state.deinit();
allocator.destroy(state_to_destroy);
allocator.destroy(current_state);
}
| fn extractFirstToken(s: []const u8) []const u8 { | ||
| var i: usize = 0; | ||
| while (i < s.len and (s[i] == ' ' or s[i] == '\t' or s[i] == '\r' or s[i] == '\n')) : (i += 1) {} | ||
| const start = i; | ||
| while (i < s.len and s[i] != ',' and s[i] != ' ' and s[i] != '\t' and s[i] != '\r' and s[i] != '\n' and s[i] != '}') : (i += 1) {} | ||
| return s[start..i]; | ||
| } |
There was a problem hiding this comment.
The extractFirstToken function does not handle quoted strings (e.g., 'block_0'). If a test case YAML uses quotes for identifiers, the resulting token will include them, which will cause file loading to fail when constructing the path in applyBlock. Trimming quotes from the extracted token would make the parser more robust.
fn extractFirstToken(s: []const u8) []const u8 {
var i: usize = 0;
while (i < s.len and (s[i] == ' ' or s[i] == '\t' or s[i] == '\r' or s[i] == '\n')) : (i += 1) {}
const start = i;
while (i < s.len and s[i] != ',' and s[i] != ' ' and s[i] != '\t' and s[i] != '\r' and s[i] != '\n' and s[i] != '}') : (i += 1) {}
return std.mem.trim(u8, s[start..i], " '\"");
}
| return struct { | ||
| const Self = @This(); | ||
|
|
||
| pub fn execute(allocator: Allocator, pool: *Node.Pool, dir: std.Io.Dir) !void { |
There was a problem hiding this comment.
The execute function body is approximately 230 lines long, which exceeds the hard limit of 70 lines per function defined in the repository style guide. Please consider refactoring this function by extracting logical blocks (e.g., anchor initialization, pubkey cache setup, step loop) into helper functions.
References
- Restrict the length of function bodies to reduce the probability of poorly structured code. We enforce a hard limit of 70 lines per function. (link)
| return s[start..i]; | ||
| } | ||
|
|
||
| fn parseChecks(body: []const u8) !ChecksStep { |
There was a problem hiding this comment.
The parseChecks function body is approximately 113 lines long, which exceeds the hard limit of 70 lines per function defined in the repository style guide. Consider splitting the sub-field parsing logic into a separate helper function.
References
- Restrict the length of function bodies to reduce the probability of poorly structured code. We enforce a hard limit of 70 lines per function. (link)
| defer fc.deinit(allocator); | ||
|
|
||
| // ---------- FastConfirmation ---------- | ||
| var fcr = FastConfirmation.init(anchor_finalized, 25, config.chain.PROPOSER_SCORE_BOOST); |
There was a problem hiding this comment.
The byzantine_threshold is hardcoded to 25. Since ChainConfig now includes CONFIRMATION_BYZANTINE_THRESHOLD, the runner should use the value from the configuration to ensure consistency with the network preset being tested.
var fcr = FastConfirmation.init(anchor_finalized, config.chain.CONFIRMATION_BYZANTINE_THRESHOLD, config.chain.PROPOSER_SCORE_BOOST);
1 participant
Summary
Adds the EF Fast Confirmation Rule spec test runner on top of #305 (
gr/feature/forkchoice-spec-tests) and #336 (FCR algorithm).This PR is stacked on PR #305. The FCR algorithm + module is in PR #336 (which targets
gr/feature/forkchoice-z). This branch combines both worlds: PR 305'sfork_choicerunner infrastructure + PR 336's FCR module + a newfast_confirmationrunner.References:
What's added (on top of #305)
src/fork_choice/fast_confirmation/fast_confirmation.zig(~3870 LOC)src/fork_choice/fast_confirmation/root.zigtest/spec/runner/fast_confirmation.zig(~870 LOC)test/spec/writer/fast_confirmation.zigtest/spec/runner_kind.zig(+1).fast_confirmationenum entrytest/spec/write_spec_tests.zig(+1)src/fork_choice/fork_choice.zig(+38/-14)compute_unrealized_without_pull_upForkChoiceOpts flagbuild.zig.zon,test/spec/version.txtTest status
zig buildsucceeds on Zig 0.16.0zig build test:spec_tests -Dpreset=minimal -Dspec_tests.filters=fast_confirmation: 348/1014 pass on minimal/alpha.5The 666 failures are not FCR algorithm bugs — they're
HeadRootMismatcherrors from upstream fork-choice head selection (well-documented; FCR's own state-machine logic exercises correctly when head selection matches).Stacking note
Once #305 + #336 land, this PR's incremental delta is just the
fast_confirmationrunner + thecompute_unrealized_without_pull_upflag.