Only return viable payload-status variants from get_head

[0xsamalt]

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]

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]

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]

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

[0xsamalt]

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]

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]

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]

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.

[ensi321]

I think this leads to possibility that get_head returns PENDING variant, which has an effect downstream to the consumers of this function. We should make sure this always return EMPTY or FULL

[0xsamalt]

Good point, traced it: get_head can only return PENDING if the justified root's subtree has zero viable EMPTY/FULL nodes (any PENDING node in the set has a viable EMPTY/FULL child, so only base can be returned). That can't happen for a well-formed store, but it isn't asserted anywhere. want me to add an assert to Gloas get_head?

[mkalinin]

Every viable PENDING node has at least a viable EMPTY child because the viability in both cases is determined by a node.root. Also, store.justified_checkpoint.root block must have at least a single viable descendant with post_state.justified_checkpoint == store.justified_checkpoint in the store.

I don’t think it worth adding any additional asserts.

[ensi321]

To clarify, if get_filtered_node_tree is empty, under the current status quo, get_head will return EMPTY(justified) because it calls children = get_node_children(store, blocks, head) unconditionally.

The new change here

children = [
            child for child in get_node_children(store, head) if child in filtered_node_tree 
        ]

means children will be empty because filtered_node_tree is empty.

Then get_head will return ForkChoiceNode(root=store.justified_checkpoint.root) which has default payload status to PENDING.

This will have downstream effect, eg. should_build_on_full will always fail because it asserts head to not be PENDING.

I think the fix is not having assertion in get_head, but rather a soft override. Something like

    if head.payload_status == PAYLOAD_STATUS_PENDING:
        # Entire viable tree is empty fall back to the EMPTY variant of the justified root
        head = ForkChoiceNode(root=head.root, payload_status=PAYLOAD_STATUS_EMPTY)
    return head

to line 496

[mkalinin]

Wdyt about the following mod to get_head prior to the tree traversal:

# [New in Gloas:EIP7732]
if not any(filtered_node_tree):
     # Must never return a pending node
     return ForkChoiceNode(
          root=store.justified_checkpoint.root,
          payload_status=PAYLOAD_STATUS_EMPTY
     )

[ensi321]

Yes I think this is good too

[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]

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

[ensi321]

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)?

[ensi321]

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

[mkalinin]

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