ethereum / ethereum/consensus-specs

childless payload-status variants are never FFG-tested, so `get_head` can return a non-viable node

Open
#5,496 3 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
4k
Forks
1.3k
Avg merge
1d 7h
Merged PRs (30d)
58

Description

### Description

In pre-Gloas, [`filter_block_tree`](https://github.com/ethereum/consensus-specs/blob/v1.7.0-alpha.12/specs/phase0/fork-choice.md#filter_block_tree) keeps a block in the filtered block tree if at least one of its children is kept. A childless block is kept only if it passes the FFG test itself ie.
https://github.com/ethereum/consensus-specs/blob/0ddf13b3dcdae1cc855c9aa8aa28af5e8c6f3c82/specs/phase0/fork-choice.md?plain=1#L437

The recursion therefore FFG-tests the leaf of every branch and keeps the ancestors of the ones that pass and discarding the rest.
So every node that `get_head` returns must have passed the FFG test.

Post-Gloas, [`get_node_children`](https://github.com/ethereum/consensus-specs/blob/v1.7.0-alpha.12/specs/gloas/fork-choice.md#modified-get_node_children) expands a `PENDING` node into its `EMPTY`/`FULL` variants without consulting the filtered block tree or any FFG test. So a childless variant of a block not only can stay in the tree but also become a leaf or head while never having been tested.

### Example

Pre-Gloas: B fails the FFG test, K passes, K' fails:

```
┌── K (passes)
J ── B ─┤
└── K' (fails)
```

K' is pruned. K is kept because it passes. B is kept because its child K is. B itself is never tested, but the walk can only pass *through* B, never stop at it.

Post-Gloas: same B, with K built on B's *empty* variant:

```
┌── EMPTY(B) ── K (passes)
J ── PENDING(B) ─┤
└── FULL(B) childless, never tested
```

`FULL(B)` is not pruned because it is never tested. `filter_block_tree` operates on blocks, not on variants. But now FULL(B) is a leaf, and the walk can stop here, and thus `get_head` can return it albeit it fails the FFG test.

I am not sure whether this is intended. Lodestar applies the FFG test per variant, so it prunes `FULL(B)` and can never return it as head, whereas the spec as written keeps it eligible. Several comments in the spec contradict the current behavior:

https://github.com/ethereum/consensus-specs/blob/0ddf13b3dcdae1cc855c9aa8aa28af5e8c6f3c82/specs/phase0/fork-choice.md?plain=1#L448-L452

https://github.com/ethereum/consensus-specs/blob/0ddf13b3dcdae1cc855c9aa8aa28af5e8c6f3c82/specs/gloas/fork-choice.md?plain=1#L572-L573

Contributor guide

No contributing guide indexed for this repository

Research direction

Read filter_block_tree in specs/phase0/fork-choice.md alongside get_node_children and get_head in specs/gloas/fork-choice.md. Trace the EMPTY/FULL childless-variant example and compare it with the cited FFG-test comments. Done means resolving whether variants require per-variant FFG filtering and updating the specification consistently.

Written by the indexing model from the issue text.

Assessment

Domain
distributed-systems
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.