Revert "fix: use head state's justified checkpoint as attestation source"#595
Conversation
…rce (leanEthereum#506)" This reverts commit 9c30436.
There was a problem hiding this comment.
Pull request overview
This PR reverts #506, restoring the prior forkchoice behavior where attestation production uses the store-wide latest_justified checkpoint (and removing related regression tests).
Changes:
- Revert
Store.produce_attestation_data()to useStore.latest_justifiedas the attestationsource. - Simplify/roll back documentation around
latest_justifiedsemantics. - Remove tests that asserted attestation source should come from the head state’s justified checkpoint (including a devnet scenario test).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/lean_spec/subspecs/forkchoice/store.py |
Reverts attestation source selection to store-wide latest_justified and adjusts docstrings. |
tests/lean_spec/subspecs/forkchoice/test_store_attestations.py |
Removes a regression test and now-unused helper import. |
tests/consensus/devnet/fc/test_attestation_source_divergence.py |
Deletes an end-to-end devnet test for attestation source divergence. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
in update head, we should also add an also where we have checkpoint sync add the spec change that it needs to download one block after the finalized state from the peers which "confirms" the checkpoint state's block as finalized and also helps node "discover" a justified block downstream |
prefer this PR only do revert and create an issue for this. |
…ling test Add an invariant check in update_head that walks backward from the new head to verify it is a descendant of the justified root. This documents the guarantee that LMD-GHOST provides by construction and catches any future regression. Add a fork choice filler test exercising the justified divergence scenario: a minority fork (V1+V2+V3) justifies slot 1 while the head chain (V0) has not seen those attestations. The next block on the head chain self-heals via the fixed-point loop in build_block: 1. Pool contains fork B's attestations (source=0, target=1) 2. Builder starts with current_justified=0 (head state) 3. Fork B's attestations match -> justifies slot 1 4. Divergence closed in one block This proves the revert of PR leanEthereum#506 is safe: the chain does not stall because the fixed-point loop incorporates the justifying attestations. Addresses leanEthereum#596 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
looks good, although
self._compute_lmd_ghost_head(
start_root=self.latest_justified.root,"should" give the head that is downstream from state's latest_justified already so not sure if the assert patch you added is required
however where we NEED to add assert patch is in block production which should asset that the produced block's latest justified is downstream from the current store's latest justified i.e. it includes for e.g. attestations that came in slot4 which you correctly noted should happen as "fix point" of the attestation packing process we have in build block.
and a corresponding test for build block as well
The assert in update_head was redundant — LMD-GHOST starts from the justified root and descends, so the result is always downstream by construction. Remove it. Add the assert where it matters: in produce_block_with_signatures, verifying the fixed-point loop in build_block closed any justified divergence between the store and the head chain. Without this, a produced block could omit justifying attestations, leaving other nodes with a stale justified checkpoint. - Move assert from update_head to produce_block_with_signatures - Add block body checks to the divergence filler test (test vectors) - Add unit test for build_block fixed-point convergence Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reverts #506
Closes #596