Skip to content

forkchoice: use only the "new" pool for update_safe_target#680

Merged
tcoratger merged 1 commit intoleanEthereum:mainfrom
ch4r10t33r:spec/safe-target-new-pool-only
Apr 25, 2026
Merged

forkchoice: use only the "new" pool for update_safe_target#680
tcoratger merged 1 commit intoleanEthereum:mainfrom
ch4r10t33r:spec/safe-target-new-pool-only

Conversation

@ch4r10t33r
Copy link
Copy Markdown
Collaborator

Summary

Treat safe target as an availability signal: compute it strictly from latest_new_aggregated_payloads and stop merging in latest_known_aggregated_payloads.

This aligns the spec with the zeam client's existing behaviour (see blockblaz/zeam#779) and the Ream reference implementation.

Rationale

update_safe_target runs at interval 3, strictly before the migration step at interval 4 that moves new -> known. The tick ordering is the whole argument:

  • 3sf-mini could have swapped the two steps (run accept_new_votes first, then compute_safe_target on a merged pool) and chose not to.
  • At interval 3, votes still in new represent the current slot's online view. Anything already in known represents historical knowledge: block-included attestations, gossip votes migrated in previous slots, self-attestations stored locally without passing through gossipsub.
  • Counting known would let a node keep advancing its safe target on stale evidence even when live participation has collapsed. That is exactly the failure mode safe target is supposed to prevent.

Quoting the zeam review thread:

availability means current online w.r.t to a node's view. [...] safe target is availability protocol imo. otherwise 3sf mini would have merged it (accept_new_votes) before running compute safe target.

What changes

`src/lean_spec/subspecs/forkchoice/store.py` (`update_safe_target`):

  • Drops the shallow-copy-and-overlay merge of `latest_known_aggregated_payloads` and `latest_new_aggregated_payloads`.
  • Passes `self.latest_new_aggregated_payloads` directly into `extract_attestations_from_aggregated_payloads`.
  • Docstring rewritten around the interval-3 / interval-4 ordering and the availability framing.

`tests/consensus/devnet/fc/test_safe_target.py`:

  • `test_safe_target_uses_merged_pools_at_interval_3` renamed to `test_safe_target_ignores_known_pool_at_interval_3`.
  • Same scenario (2 votes in `known` from block body, 2 votes in `new` from gossip, threshold 4) is retained as a regression test, but the expectation flips: safe target now stays at genesis instead of advancing to `block_2`.
  • All other safe-target tests are unaffected.

Notes

  • Needs confirmation from Yaan on whether this is the desired availability semantic before the spec PR lands.
  • Draft PR; I'll un-draft after that confirmation.

Test plan

  • `uv run fill --fork=Devnet --clean -n auto tests/consensus/devnet/fc/test_safe_target.py` — 6 passed.
  • `uv run pytest tests/lean_spec/subspecs/forkchoice -q` — 81 passed.
  • `uv run ruff format` + `uv run ruff check` clean on edited files.
  • Awaiting review confirmation from Yaan on the availability framing.

Treat safe target as an availability signal: compute it strictly from
latest_new_aggregated_payloads and ignore latest_known_aggregated_payloads.

update_safe_target runs at interval 3, before the migration step at
interval 4. That ordering is intentional: votes still in "new" at
interval 3 reflect the current slot's online view, while anything
already in "known" reflects historical knowledge (block-included
attestations, previously migrated gossip, self-attestations stored
locally). Counting "known" would let the safe target keep advancing
on stale evidence even when live participation has collapsed.

This brings the spec in line with the zeam client (see blockblaz/zeam#779)
and the Ream reference implementation.

The merged-pool test is rewritten to pin the new-only behaviour: with
"known" at 2 votes and "new" at 2 votes (threshold 4), the safe target
now stays at genesis instead of advancing to block_2.
@ch4r10t33r ch4r10t33r marked this pull request as ready for review April 24, 2026 12:28
@tcoratger tcoratger merged commit e845240 into leanEthereum:main Apr 25, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants