Only return viable payload-status variants from `get_head` by 0xsamalt · Pull Request #5509 · ethereum/consensus-specs

0xsamalt · 2026-08-02T20:01:16Z

In Gloas, get_node_children expands a PENDING node into its EMPTY and FULL payload-status variants without consulting the filtered block tree or any FFG test (introduced in #5249). As a result, a childless variant of a block that fails the FFG test is never pruned: it can become a leaf of the LMD-GHOST walk and be returned by get_head, even though it is not viable (#5496).

This PR makes filter_block_tree operate on payload-status variants, keyed by (root, payload_status), so that every variant is FFG-tested independently. The FFG test itself is unchanged and still computed per block root. get_node_children now only returns children present in the filtered block tree, so the head walk can never stop at a non-viable variant, and get_filtered_block_tree seeds the recursion from a pending node at store.justified_checkpoint.root.

Fixes #5496

mkalinin · 2026-08-03T05:58:21Z

What do you think about renaming filter_block_tree to filter_node_tree with the corresponding semantics change as an alternative approach to deal with the original issue? And then apply the same to get_filtered_block_tree

0xsamalt · 2026-08-03T07:08:33Z

Thanks for the suggestion. I like the direction, since the function is now FFG-testing payload-status variants rather than blocks, so filter_node_tree/get_filtered_node_tree would be more accurate names. If we go this route, I'd propose the "semantics change" also means dropping the ad-hoc Tuple[Root, PayloadStatus] key in favor of keying the dict directly by ForkChoiceNode (Dict[ForkChoiceNode, BeaconBlock]), since ForkChoiceNode already models that pair and is hashable (@dataclass(eq=True, frozen=True)).

One thing to flag: pysetup merges same-named functions across forks but has no rename detection, so renaming in gloas/fork-choice.md alone would leave phase0's filter_block_tree/get_filtered_block_tree in the compiled Gloas spec alongside the new ones. I'd add both old names to GloasSpecBuilder.deprecate_functions() in pysetup/spec_builders/gloas.py (same pattern as process_execution_payload), plus a prose-only ##### Removed note for clarity. The deprecation also means any stale callers (the shared get_viable_for_head_checks helper, the block_cover.py compliance runner, and the new test) get updated in the same PR and leftover references will fail loudly rather than silently using phase0 semantics.

Happy to make this change if you think it's worth it, wanted to confirm the approach and the deprecate_functions() step before reworking the PR. Let me know what you think.

mkalinin · 2026-08-03T08:26:58Z

I think we can alter the spec by introducing filter_node_tree since Phase0 and then introduce required modifications in Gloas if any

0xsamalt · 2026-08-03T08:57:27Z

Good call, that also sidesteps the deprecation issue since Gloas redefines these functions rather than inheriting phase0's, so a phase0-origin rename should merge cleanly. I'll rework the PR: rename filter_block_tree to filter_node_tree and get_filtered_block_tree to get_filtered_node_tree starting in phase0, update Gloas on top with the payload-status testing and ForkChoiceNode-keyed dict, and fix the callers. Pushing shortly.

0xsamalt · 2026-08-03T13:05:34Z

Pushed the rename. filter_block_tree/get_filtered_block_tree are now filter_node_tree/get_filtered_node_tree, starting at the phase0 definition, with Gloas and all callers updated to match. I left out the dict-keying change (Tuple[Root, PayloadStatus] to ForkChoiceNode) since that wasn't part of what we settled on here, happy to open a follow-up for it if you think it's worth doing.

mkalinin · 2026-08-03T14:31:02Z

I was thinking about a little bit different design:

get_node_children returns all possible nodes that are children of a given one regardless of the filtering, it encapsulates child-parent relation logic
filter_node_tree relies on get_node_children and returns a Set[ForkChoiceNode] of viable nodes
get_head queries get_node_children and then operates over children that are present in filtered_node_tree returned by get_filtered_node_tree
Corresponding adjustments to the test helpers and tests that are affected by this change

What are your thoughts on this?

0xsamalt · 2026-08-03T22:02:48Z

Implemented as described: get_node_children returns all children regardless of viability, filter_node_tree/get_filtered_node_tree return a Set[ForkChoiceNode] of viable nodes, and get_head filters against that set. Also dropped block_cover.py's is_post_gloas branch since both forks now share the same return shape. Tests and lint are green.

…block-tree-payload-variants

Builtin hash is no longer shadowed in the compiled spec namespace after ethereum#5555, so the dataclass-generated hash can be used.

jtraglia

I pushed some changes to this PR, please double check that those are good. If @mkalinin and @ensi321 approve this PR, we can merge it. I'm not really an expert with fork-choice, so I wouldn't feel comfortable making the final decision here. The code itself looks fine though.

jtraglia · 2026-08-26T19:34:58Z

Also, please take a moment to mark resolved comment as "resolved".

ensi321 · 2026-08-26T21:32:17Z

+@with_gloas_and_later
+@with_presets([MINIMAL], reason="too slow")
+@spec_state_test
+def test_get_head_prunes_childless_unviable_full_variant(spec, state):


Can you add a test that's the mirror of this case?

ie. childless EMPTY variant of a block that fails FFG test and K builds on FULL(B)?

Also add one more:
both FULL(B) and EMPTY(B) pass FFG test, both childless, see if get_filtered_node_tree contains both

added both the tests in 90d435a

mkalinin · 2026-08-27T14:03:35Z

It would be great if @potuz could take a look at this PR as he was opposing to making this change into the spec

…d_node_tree

…ests

potuz

This change looks correct to me, it is equivalent to what we already implement in Prysm anyway so it is zero changes for us.

potuz · 2026-09-03T17:40:19Z

+#### `get_node_children`
+
+```python
+def get_node_children(


This is a deeper semantics change that probably should be commented, previous to this change get_node_children gets a prefiltered list of blocks that are descendant of the justified checkpoint, while now it takes nodes from the store and gives all children.

potuz · 2026-09-03T17:43:04Z

-    node: ForkChoiceNode,
-) -> Sequence[ForkChoiceNode]:
-    return [ForkChoiceNode(root=root) for root in blocks if blocks[root].parent_root == node.root]
+    base = ForkChoiceNode(root=store.justified_checkpoint.root)


It's weird to think in nodes in phase 0 as now we have this bad situation in which we pass a ForkChoiceNode without any PayloadStatus.

potuz · 2026-09-03T17:53:47Z

    while True:
-        children = get_node_children(store, blocks, head)
+        children = [
+            child for child in get_node_children(store, head) if child in filtered_node_tree


This is the semantic change consumer which requires a check in the caller instead of just a single filtering as we did before.

FFG-test payload-status variants individually in filter_block_tree

fc5b903

github-actions Bot added testing CI, actions, tests, testing infra gloas labels Aug 2, 2026

0xsamalt marked this pull request as ready for review August 2, 2026 20:11

Rename filter_block_tree to filter_node_tree

3161090

github-actions Bot added the phase0 label Aug 3, 2026

Decouple get_node_children from fork-choice filtering

f066d02

mkalinin reviewed Aug 4, 2026

View reviewed changes

Comment thread specs/phase0/fork-choice.md Outdated

Comment thread specs/gloas/fork-choice.md Outdated

Comment thread specs/phase0/fork-choice.md Outdated

Apply review feedback on fork-choice node-tree redesign

019e6da

krisoshea-eth mentioned this pull request Aug 5, 2026

[BASELINE-01] Pin unstable and run the baseline capability audit krisoshea-eth/lodestar#6

Closed

8 tasks

mkalinin reviewed Aug 5, 2026

View reviewed changes

Comment thread tests/generators/compliance_runners/fork_choice/instantiators/block_cover.py Outdated

Revert accidental rename of _generate_filter_block_tree

5df3d78