feat: add fork choice compliance test for pre-gloas forks#9670
Open
ensi321 wants to merge 21 commits into
Open
feat: add fork choice compliance test for pre-gloas forks#9670ensi321 wants to merge 21 commits into
ensi321 wants to merge 21 commits into
Conversation
The fork-choice compliance suite (consensus-specs#3831) introduces a `viable_for_head_roots_and_weights` check that asserts the leaves and weights of the internal `filter_block_tree(store, justified.root)`. Head equivalence alone is too weak to catch filtered-tree regressions. Add `IForkChoice.getViableHeads()` that returns the viable-for-head leaves of the filtered tree paired with their weights: - A node is a filter_block_tree leaf iff it is viable AND has no viable descendants. `maybeUpdateBestChildAndDescendant` only sets `bestChild` to a child that `nodeLeadsToViableHead`, so `bestChild === undefined` iff no viable descendant. Combined with `nodeIsViableForHead`, this matches the spec exactly. Same approach as Teku's `ForkChoiceStrategy.getChainHeads`. - Weight is converted from internal increment units to Gwei on read (compliance fixtures expect Gwei). Note the proposer-boost score contribution is also stored in increments and may round down on minimal preset (102 vs 102.4 ETH); attestation weight is exact, the rounding only surfaces while boost is active. Mainnet boost values are integer ETH so this is a non-issue there.
Wire the consensus-specs Fork Choice Compliance suite (#3831) into the existing `forkChoiceTest` runner. The on-disk layout matches the standard spec-test layout (`tests/<preset>/<fork>/fork_choice_compliance/<handler>/<suite>/<case>/`), so it slots in alongside `fork_choice` and `sync` runners. Three test-only accommodations the compliance fixtures require: 1. `bls_setting: 2` — every compliance fixture uses placeholder signatures. Pass `validSignatures: testcase.meta?.bls_setting !== BigInt(1)` to `chain.processBlock` so verification short-circuits. Standard `fork_choice` fixtures use `bls_setting: 1` so behavior there is unchanged. 2. `BLOCK_ERROR_ALREADY_KNOWN` — compliance fixtures intentionally re-import the same block (`dup_shift` mutations in their `meta.yaml`). Spec semantics for `on_block(store, known_block)` is a no-op success. Production block import correctly rejects with ALREADY_KNOWN; this runner treats that case as success only when the step is `valid: true`. 3. Cross-epoch attestation shuffling — `on_attestation` decodes aggregation_bits using the state at the attestation's target checkpoint, not the head state. The runner now resolves the right shuffling via ShufflingCache + regen (mirroring the production validation path) instead of `headState.epochCtx.getIndexedAttestation`, which only worked when the attestation's epoch happened to be in the head's epoch cache (±1 epoch) and broke on cross-epoch fork attestations surfaced by the compliance suite. Adds support for two compliance-only check fields: - `viable_for_head_roots_and_weights` (consensus-specs#3831): compared via `getViableHeads()`. Both sides are sorted by root before comparison since the spec doesn't fix order. - `head_payload_status` (gloas): mapped between our internal enum ordering (PENDING=0, EMPTY=1, FULL=2) and spec ordering (EMPTY=0, FULL=1, PENDING=2). Pass rate against the latest comptests workflow `small.tar.gz` artifact: fulu/fork_choice_compliance: 253/1472 cases pass (17.2%) Top remaining failures: - ~80% `Invalid proposer boost root` — consensus-specs#4807 introduced a `block.proposer_index == get_beacon_proposer_index(head_state)` guard in `update_proposer_boost_root` that we do not yet implement; affects all forks (not just gloas equivocation handling). Tracked for follow-up alongside #9233. - ~1% `Invalid viable heads` — proposer-boost rounding on minimal preset (see `getViableHeads()` weight note).
…e gloas variants Three corrections to the compliance-only getViableHeads getter, plus an API-surface trim: - Restrict leaves to descendants of the justified checkpoint block: the spec's get_filtered_block_tree is rooted at store.justified_checkpoint, so iterating all proto-array nodes over-included FFG-viable leaves hanging off the finalized checkpoint on branches not under the current justified checkpoint (~87 of 100 'Invalid viable heads' compliance failures were this, not proposer-boost rounding as previously assumed). The descendant walk follows parent links to the justified root — a slot-based getAncestor lookup returns the node itself for leaves whose own slot equals the justified epoch start slot — and stops early once it passes the justified block's slot (parent slots are non-increasing). - Emit each block root at most once: gloas payload-status variants share a blockRoot and more than one can be a leaf (EMPTY and FULL are both parented under PENDING), which produced duplicate roots that broke the index-paired comparison in the compliance check. Which variant's weight the spec expects is part of the open gloas vote-attribution divergence; keep the heaviest qualifying variant deterministically. - Drop getViableHeads from IForkChoice: it is compliance-test-only and its single caller accesses the ForkChoice class directly. Verified against consensus-specs compliance vectors (run 28413328593, minimal fulu): viable-head root-set failures 100 -> 0; standard minimal fork_choice+sync spectests unaffected (397 passed).
Changes to the shared fork-choice runner, each anchored to a test-format or spec contract (not compliance-suite identity), so they apply uniformly to fork_choice/sync/fork_choice_compliance: - Register fork_choice_compliance in coveredTestRunners: without it every other spec suite iterating the same fixture tree (ssz_static, operations, sanity, ...) throws 'No test runner for fork_choice_compliance' once compliance fixtures are downloaded. - Attestation timing precondition: spec validate_on_attestation requires get_current_slot(store) >= data.slot + 1. Lodestar enforces this 1-slot delay in the gossip/attestation-pool layer, not forkChoice.onAttestation; the runner calls onAttestation directly, so replicate the precondition (Lighthouse's runner excuses the same case post-hoc: DidntFail tolerated when data.slot >= current_slot). Clears all 298 negative-attestation compliance failures. - Attestation step hardening: an unknown beacon block root on a valid:true step now fails loudly instead of being silently skipped, and the shuffling resolution runs inside the negative-test try/catch so that errors on valid:false steps (e.g. attesting to a future block) count as the expected rejection instead of crashing the case. - Skip gloas ePBS signature verification unless bls_setting == 1: the test format mandates skipping verification for placeholder signatures (bls_setting: 2), which the block-import path already honors via validSignatures. Extend the same handling to the execution_payload and payload_attestation_message steps (Teku disables BLS at spec level for bls_setting IGNORED; Lighthouse compiles compliance under fake_crypto). - Split viable_for_head_roots_and_weights into exact root-set equality plus a 1.2 ETH weight tolerance: the root set is epoch-determined and must match exactly (this strictness caught the getViableHeads justified-subtree bug; Teku's containsAll would not), while weights are stored in EFFECTIVE_BALANCE_INCREMENT units so the proposer-boost score quantizes to whole ETH; the divergence is bounded by (100 - gcd(P,100) + P)/100 = 1.2 ETH independent of preset. Compliance (run 28413328593, minimal): fulu 1459/1472 (99.1%), remaining 11 proposer-boost-root divergences + 1-2 timeouts; gloas 200/1472, blocked by processPayloadAttestation verifying signatures unconditionally (ignores verifySignatures; not batched in getBlockSignatureSets) and a payload-status vote-attribution divergence (weight diffs are exact multiples of 32 ETH between EMPTY/FULL variants of the same root) — both documented follow-ups, out of scope here. Known accommodation limit: ALREADY_KNOWN treated as no-op success cannot re-apply proposer boost for a timely duplicate the way spec on_block re-processing would. Standard minimal fork_choice+sync: 397 passed, unchanged.
Use the single getDefaultNodeIndex + getNodeByIndex idiom already used elsewhere in protoArray instead of the two-hop getDefaultVariant + getNode lookup. Semantically identical for all variant cases (pre-gloas FULL, gloas PENDING, absent root); drops one redundant indices map lookup and removes an unreachable throw path.
- getViableHeads(): emit one entry per gloas payload-status variant (root, payload_status, weight) per consensus-specs #5393; drop the dedupe-by-root heuristic; return increment-unit weights - runner: read nested checks.head.payload_status (alpha.12 shape), replacing the dead flat head_payload_status key - runner: exact viable-head weight comparison via proposer-boost emulation (bigint) instead of the 1.2 ETH tolerance; normalizes the documented boost-quantization divergence using the vector's expected boost root and the production getCommitteeFraction - runner: payload-attestation steps now reject slot mismatch and non-PTC validators, and fail on unexpectedly-successful negative tests - specTestIterator: compose SkipOpts.skippedTests with runner-local shouldSkip (was silently ignored for fork_choice); gate fork_choice_compliance behind RUN_FORK_CHOICE_COMPLIANCE=1; defer gloas compliance suite Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add `pnpm download-comptests` and `pnpm test:comptest` (root + beacon-node), fully outside the regular download-spec-tests/test:spec flow. The comptests.tar.gz release asset exists since consensus-specs #5334; vectors are fetched pinned to v1.7.0-alpha.12 (valid against the alpha.11 pin: the pre-gloas fork-choice spec delta is rename-only). The standard downloader deletes its entire output directory on a cache miss, so it cannot be reused for a second asset sharing spec-tests/. downloadComptests() is asset-scoped instead: extracts to a temp dir, replaces only tests/<preset>/<fork>/fork_choice_compliance/ subtrees, and writes its own comptests-version.json marker only after a successful merge — a failed download leaves existing fixtures and marker untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Covers: additive install alongside standard fixtures, refresh replacing comptest paths + removing stale forks, cache hit, failed extraction leaving fixtures and marker untouched, legacy marker treated as stale. Also fixes the ordering bug the tests caught: validate the temp extraction is non-empty BEFORE removing existing comptest paths — tar can exit 0 on a truncated archive, which would have wiped the existing vectors against an empty extraction. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Covers justified-subtree leaf selection with increment-unit weights, and gloas payload-status variants of one root reported as separate entries (EMPTY-only before the envelope, EMPTY+FULL after onExecutionPayload). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… gating Compliance registration moves out of fork_choice.test.ts into its own test/spec/comptest/ file backed by a dedicated `comptest` vitest project; the shared runner is extracted to utils/forkChoiceTestRunner.ts. The regular spec projects exclude test/spec/comptest/** so test:spec/test:spec:minimal can never pick the suite up, which removes the need for RUN_FORK_CHOICE_COMPLIANCE: `pnpm test:comptest` simply runs the comptest project. Zero-test guard (workspace config has passWithNoTests: true) moves into the comptest entry file, now unconditional. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
block_tree_test_16_201284350_1 fails identically on every pre-gloas fork: the dependent-root gate compares against the cached head, which is stale when the preceding tick crosses an epoch boundary and pulls up the justified checkpoint. Tracked in #9666. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Spec on_payload_attestation_message returns early when the message slot
does not match the attested block's slot ('PTC votes can only change the
vote for their assigned beacon block, return early otherwise') — it is
not a rejection. Restore conditional processing; the PTC-membership
assert and the negative-test assertion remain.
https://github.com/ethereum/consensus-specs/blob/v1.7.0-alpha.12/specs/gloas/fork-choice.md#on_payload_attestation_message
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
comptests-version.txt, matching the standard downloader's version.txt convention; a single version string needs no JSON. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Compliance vectors now live in spec-tests-comptests/ with their own version.txt, downloaded via the plain generic downloader — a flow fully parallel to the standard spec tests, sharing only the fork-choice test runner. download-spec-tests and download-comptests are order-independent (previously, a standard-tests version change wiped the shared directory including the merged compliance vectors). Deletes the asset-scoped merge downloader and its tests — no longer needed without a shared directory. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Scheduled (03:00 UTC) rather than per-PR: most PRs don't touch fork choice, and the vectors are pinned — nightly on unstable is the right cadence to catch regressions. Same pattern as Kurtosis sim tests. Manual runs via workflow_dispatch. Also documents download-comptests/test:comptest in AGENTS.md/CLAUDE.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Own outputDirBase and testsToDownload like the other entries; version and repo URL shared with the standard spec tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
ensi321
commented
Jul 16, 2026
|
|
||
| const logger = testLogger("spec-test"); | ||
|
|
||
| export const forkChoiceTestRunner = |
Member
Author
There was a problem hiding this comment.
Note for reviewers: a lot of the code here is migrated from fork_choice.test.ts so both spec test and compliance test can share a single runner.
Would be easier to view the diff by:
git diff unstable:packages/beacon-node/test/spec/presets/fork_choice.test.ts nc/fork-choice-compliance:packages/beacon-node/test/spec/utils/forkChoiceTestRunner.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48f1f5f479
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
tickTime starts at 0, so an attestation step before the first tick computed slot 0 regardless of the anchor state slot. clock.currentSlot is initialized from the anchor slot and advanced by tick steps, matching get_current_slot(store). Also document the ALREADY_KNOWN duplicate-block accommodation honestly (spec re-processes duplicates; it is not a no-op). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a standalone fork-choice compliance test suite (pnpm test:comptest) parallel to the standard spec tests, extracting the fork-choice test runner into a reusable utility. It also implements helper methods in ProtoArray and ForkChoice to retrieve viable heads and justified balances for compliance assertions. Feedback from the review highlights opportunities to improve type safety in the test runner by adding type guards before casting blockState to IBeaconStateViewGloas, and to optimize performance in protoArray.ts by returning early when justifiedSlot is undefined to avoid unnecessary tree traversals.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+272
to
+282
| if (protoBlock.slot === payloadAttestationMessage.data.slot) { | ||
| const blockState = await chain.regen.getBlockSlotState( | ||
| protoBlock, | ||
| payloadAttestationMessage.data.slot, | ||
| {dontTransferCache: true}, | ||
| RegenCaller.processBlock | ||
| ); | ||
|
|
||
| const ptcIndices = (blockState as IBeaconStateViewGloas).getIndicesInPayloadTimelinessCommittee( | ||
| payloadAttestationMessage.validatorIndex, | ||
| payloadAttestationMessage.data.slot |
Contributor
There was a problem hiding this comment.
The blockState variable is cast to IBeaconStateViewGloas without checking if it is null/undefined or if it is actually a Gloas state. Adding a type guard check ensures type safety and prevents potential runtime TypeErrors.
Suggested change
| if (protoBlock.slot === payloadAttestationMessage.data.slot) { | |
| const blockState = await chain.regen.getBlockSlotState( | |
| protoBlock, | |
| payloadAttestationMessage.data.slot, | |
| {dontTransferCache: true}, | |
| RegenCaller.processBlock | |
| ); | |
| const ptcIndices = (blockState as IBeaconStateViewGloas).getIndicesInPayloadTimelinessCommittee( | |
| payloadAttestationMessage.validatorIndex, | |
| payloadAttestationMessage.data.slot | |
| const blockState = await chain.regen.getBlockSlotState( | |
| protoBlock, | |
| payloadAttestationMessage.data.slot, | |
| {dontTransferCache: true}, | |
| RegenCaller.processBlock | |
| ); | |
| if (!blockState || !isGloasStateType(blockState)) { | |
| throw Error("State is not a Gloas state"); | |
| } | |
| const ptcIndices = blockState.getIndicesInPayloadTimelinessCommittee( | |
| payloadAttestationMessage.validatorIndex, | |
| payloadAttestationMessage.data.slot | |
| ); |
Comment on lines
+550
to
+556
|
|
||
| // Verify envelope against the state | ||
| const protoBlock = chain.forkChoice.getBlockHexDefaultStatus(beaconBlockRoot); | ||
| if (!protoBlock) throw Error(`Block not found for root ${beaconBlockRoot}`); | ||
| const blockState = await chain.regen.getBlockSlotState( | ||
| protoBlock, | ||
| protoBlock.slot, |
Contributor
There was a problem hiding this comment.
The blockState variable is cast to IBeaconStateViewGloas without verifying that it is not null/undefined and is indeed a Gloas state. Adding a type guard check prevents potential runtime TypeErrors when accessing its properties or passing it to verifyExecutionPayloadEnvelope.
Suggested change
| // Verify envelope against the state | |
| const protoBlock = chain.forkChoice.getBlockHexDefaultStatus(beaconBlockRoot); | |
| if (!protoBlock) throw Error(`Block not found for root ${beaconBlockRoot}`); | |
| const blockState = await chain.regen.getBlockSlotState( | |
| protoBlock, | |
| protoBlock.slot, | |
| const blockState = await chain.regen.getBlockSlotState( | |
| protoBlock, | |
| protoBlock.slot, | |
| {dontTransferCache: true}, | |
| RegenCaller.processBlock | |
| ); | |
| if (!blockState || !isGloasStateType(blockState)) { | |
| throw Error("State is not a Gloas state"); | |
| } | |
| verifyExecutionPayloadEnvelope(beaconConfig, blockState, envelope.message); |
Comment on lines
+1572
to
+1580
| private descendsFromJustified(node: ProtoNode, justifiedSlot: Slot | undefined): boolean { | ||
| let current: ProtoNode | undefined = node; | ||
| while (current !== undefined && (justifiedSlot === undefined || current.slot >= justifiedSlot)) { | ||
| if (current.blockRoot === this.justifiedRoot) { | ||
| return true; | ||
| } | ||
| current = current.parent !== undefined ? this.getNodeByIndex(current.parent) : undefined; | ||
| } | ||
| return false; |
Contributor
There was a problem hiding this comment.
If justifiedSlot is undefined, the loop condition justifiedSlot === undefined || current.slot >= justifiedSlot is always true. This causes the loop to walk all the way to the root of the tree for every node in this.nodes, resulting in false immediately when justifiedSlot is undefined avoids this unnecessary traversal.
private descendsFromJustified(node: ProtoNode, justifiedSlot: Slot | undefined): boolean {
if (justifiedSlot === undefined) {
return false;
}
let current: ProtoNode | undefined = node;
while (current !== undefined && current.slot >= justifiedSlot) {
if (current.blockRoot === this.justifiedRoot) {
return true;
}
current = current.parent !== undefined ? this.getNodeByIndex(current.parent) : undefined;
}
return false;
}
Contributor
Performance Report✔️ no performance regression detected Full benchmark results
|
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
3 participants
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.
Description
The suite is a standalone flow parallel to the standard spec tests, sharing only the fork-choice test runner:
pnpm download-comptests— fetches thecomptests.tar.gzrelease asset (same version pin and repo as the standard spec tests, configured inspec-tests-version.json) into its ownspec-tests-comptests/directory, fully order-independent fromdownload-spec-tests.pnpm test:comptest— runs the suite via a dedicatedcomptestvitest project. The regular spec projects exclude it, sotest:spec/test:spec:minimalnever pick it up. A zero-test guard fails the run loudly when fixtures are absent (the workspace vitest config setspassWithNoTests: true).presets/fork_choice.test.tsintoutils/forkChoiceTestRunner.ts(mechanical move); the preset file and the new comptest entry file only register runners.comptests.yml, 03:00 UTC +workflow_dispatch), following the Kurtosis sim-tests pattern — most PRs don't touch fork choice, so per-PR runs are not worth the cost (~10 min per fork, single vitest worker).Runner/fork-choice changes needed for the new checks:
ForkChoice.getViableHeads()(test-only, not onIForkChoice): leaves of the spec's filtered block tree with weights, backing the suite'sviable_for_head_roots_and_weightscheck — head equivalence alone can't catch filtered-tree/weight regressions. Gloas payload-status variants are reported per(root, payload_status, weight)EFFECTIVE_BALANCE_INCREMENTunits and double-floors the boost score (up to 1.2 ETH below the spec's Gwei-precision value). Instead of comparing within a tolerance (which would mask small weight bugs), the runner normalizes the expected weight of the boosted entry using the productiongetCommitteeFractionand compares exactly.checks.head.payload_status(current vector shape),on_attestationone-slot-delay precondition, target-checkpoint shuffling for attestation decoding,bls_setting: 2(placeholder signatures) handling, payload-attestation negative-test completeness (slot mismatch is a spec no-op success; missing PTC membership is a rejection).specTestIterator: centralSkipOpts.skippedTestsnow composes with runner-localshouldSkip(previously silently ignored for runners defining their own, e.g. fork_choice).Results
Supersedes #9314 — the runner and fork-choice work from that PR is carried forward as cherry-picked commits with authorship preserved. Coauthored by @GrapeBaBa