fix: drop payloadStatus from Checkpoint - #9259
Conversation
| // whose SignedBeaconBlock was archived on the previous run). `previousFinalizedFull` is the FULL | ||
| // variant of the previous finalized block — present once its execution payload has been received. | ||
| // | ||
| // By design, a post-Gloas finalized block is archived across two runs because finalization does |
There was a problem hiding this comment.
the main thing to review
There was a problem hiding this comment.
Reviewed the 2-stage archive + filter approach against pre-gloas / post-gloas.
Pre-gloas path is clean: the FULL filter is a no-op (pre-gloas blocks only have a FULL variant), so migrateDataColumnSidecarsFromHotToColdDb and migrateExecutionPayloadEnvelopesFromHotToColdDb behave the same as before. Boundary block inclusion in finalizedCanonicalBlocks is handled by the getBinary → null ⇒ skip path in migrateBlocksFromHotToColdDb, so re-processing the already-archived previous finalized block doesn't throw — good.
Real gap in the post-gloas path (details in the codex P1 thread on this file): if the newly-finalized block was imported while only its parent's EMPTY variant existed, the ancestor walk during the next-run archive returns EMPTY for that boundary even after onExecutionPayload has added the FULL sibling — because onBlock fixes the child's parent pointer once and never rewires it. The block.payloadStatus === PayloadStatus.FULL filter then drops the boundary permanently, and that specific finalized block's payload envelope + data columns never ship to cold DB.
Options that fit the current shape:
- For the boundary block specifically, resolve FULL at root level (
forkChoice.hasPayload(root)/protoArray.getNode(root, PayloadStatus.FULL)) before deciding to migrate. Narrow fix, no forkchoice API change. - Your plan 3c — re-process the previous finalized boundary in every archive run and lean on the same
null ⇒ skipidempotency for payload/columns that we already have for block data. Matches@wemeetagain's "no special case" ask and covers the EMPTY-boundary case for free.
I'd lean toward 3c — it's uniform across block/payload/columns, doesn't need a new API, and removes the subtle "boundary-was-EMPTY-extended" foot-gun.
Also flagged codex's other P1 about getFinalizedBlock() / getJustifiedBlock() defaulting to PENDING — that's independent of the archive path but regresses FCU finalizedBlockHash / safeBlockHash once the FULL sibling exists for the checkpoint. Reply on that one at forkChoice.ts:1090.
Unrelated: the benchmark-alert "Full columns - reconstruct half of the blobs out of 10" 3.13× regression looks like noise — this PR doesn't touch blob / column reconstruction code (git diff origin/unstable shows no changes in those paths).
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the fork choice and archiver logic by replacing CheckpointWithPayloadStatus with CheckpointWithHex, removing explicit payload status tracking from checkpoints. It introduces 'DefaultStatus' helper methods in the fork choice to simplify traversals and updates the archiver to support a two-stage migration for post-Gloas blocks, ensuring execution payloads and data columns are archived once their FULL variant becomes available. I have no feedback to provide.
Performance Report✔️ no performance regression detected Full benchmark results
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d903110de6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const { | ||
| ancestors: finalizedCanonicalBlocks, | ||
| nonAncestors: finalizedNonCanonicalBlocks, | ||
| previousFinalizedFull, |
There was a problem hiding this comment.
Why do we need to treat the previousFinalizedFull differently? It looks like fork choice takes off the final entry in ancestors, only for it to be re-added here?
I think ideally there would be no special-casing for finalized payloads. (Anything that is an ancestor of the finalized checkpoint gets archived. Anything that is not an ancestor of the finalized checkpoint does not get archived)
There was a problem hiding this comment.
suppose we finalize block slot 32, we move block data from hot db to cold db. But we cannot do the same thing for payload because payload is technically not finalized
after the 1st finalized process, block slot 32 of PENDING/EMPTY/FULL still stay in forkchoice
1 epoch later, we finalize block slot 64, if it sees 32/FULL as it's ancestor, set previousFinalizedFull, archiveBlocks will migrate payload of block slot 32. If not it will not move there
Why do we need to treat the previousFinalizedFull differently? It looks like fork choice takes off the final entry in ancestors, only for it to be re-added here?
previousFinalizedFull also returns from forkchoice
because the block data was archived/finalized in the last run, but payload was not. There is a long discussion here https://discord.com/channels/595666850260713488/874767108809031740/1490930960236154890
there are some ways to simplify it:
- 1 - if we do optimistically migrate payload in the 1st run, there is a tiny chance that it's reorged and we serve wrong data for by range request, and get downscore by peers
- 2 - also delay block data migration to the next run, this makes sure we migrate block data + payload data at the same time. But this is not compliant to what we already have and I'm not sure if it breaks anything.
- 3 - forkchoice always return all nodes including the previous finalized ProtoBlock to call site,
archiveBlocksto decide:- a) if pre-gloas, take off the last node, this keeps the old logic. If post-gloas, only archive block, archive payload in the next run (since it's not finalized)
- b) change the logic from now on for all forks: delay migrating block to the next run so that we always finalize block/payload at the same time
- c) for block data, process what is finalized for this run and for the previous run too. If data is already migrated, don't throw error
I think ideally there would be no special-casing for finalized payloads. (Anything that is an ancestor of the finalized checkpoint gets archived. Anything that is not an ancestor of the finalized checkpoint does not get archived)
seems like 3c which also makes the most sense to me. Forkchoice taking off the final entry is already not ideal, that's the business logic of call site, just also return the finalized ProtoBlock of the last run there
There was a problem hiding this comment.
Agreed on the direction. @twoeths already dropped the previousFinalizedFull special-case field in 496f21c — the forkchoice API now just returns all ancestors (boundary included) + nonAncestors, and the archiver filters by payloadStatus === PayloadStatus.FULL. No re-adding of the last entry.
Edge case that still bites the post-gloas payload/column path (details in the codex P1 thread on forkChoice.ts:null and my review of archiveBlocks.ts): if the boundary ancestor ends up as EMPTY in the ancestor walk even after its FULL sibling later arrives, the filter drops it and we never migrate its payload/columns. Twoeths' plan 3c from the discord thread (re-process the previous finalized boundary every run, rely on the existing null ⇒ skip idempotency) fits your "no special case" ask naturally and covers that case.
There was a problem hiding this comment.
Edge case that still bites the post-gloas payload/column path (details in the codex P1 thread on forkChoice.ts:null and my review of archiveBlocks.ts): if the boundary ancestor ends up as EMPTY in the ancestor walk even after its FULL sibling later arrives, the filter drops it and we never migrate its payload/columns.
@lodekeeper we should not migrate to archive/cold db in that case then, because it's turned out not to be canonical chain. If we persist it, later on a node may send by_range request for us, we may serve it wrong, then peers will downscore us
There was a problem hiding this comment.
Right — the by_range serving argument makes it concrete: archiving a non-canonical FULL payload after a canonical EMPTY decision would have us serve wrong data and get downscored. Retracting the concern, the EMPTY-boundary drop is intended behavior.
|
@lodekeeper could you review this
|
|
I also improved the log to make it easier to debug. This is on mainnet: it all makes sense to me, given the previous finalized root is at slot 14176032
Column migration:
|
|
Already reviewed at Your mainnet logs confirm the pre-gloas path behaves as expected:
So pre-gloas is validated by real logs. The one outstanding concern in the post-gloas path is in the codex-P1-2 thread I replied in: the ancestor walk's parent pointer is frozen at |
lodekeeper
left a comment
lodekeeper
left a comment
There was a problem hiding this comment.
LGTM — all my concerns (both P1s on forkChoice.ts / archiveBlocks.ts) retracted after @twoeths clarified the Gloas spec invariants: Checkpoint is always PENDING/EMPTY by spec, and the PTC vote is canonical for payload status, so dropping non-canonical FULL siblings at finalized EMPTY boundaries is the intended behavior (and avoids serving wrong data in by_range responses).
Pre-gloas path verified clean — FULL filter is a no-op there. Post-gloas archive flow for blocks / payloads / data-columns walks correctly through the 2-stage (finalize → filter by payloadStatus) approach. Improved migration logs also helpful for debugging on mainnet.
CI all green.
nflaig
left a comment
nflaig
left a comment
There was a problem hiding this comment.
based on the review I did with @wemeetagain yesterday, all concerns are addressed
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #9259 +/- ##
============================================
- Coverage 52.54% 52.53% -0.01%
============================================
Files 848 848
Lines 61388 61328 -60
Branches 4519 4511 -8
============================================
- Hits 32254 32218 -36
+ Misses 29069 29045 -24
Partials 65 65 🚀 New features to boost your workflow:
|
|
🎉 This PR is included in v1.43.0 🎉 |
4 participants
Motivation
Drop
payloadStatusfromCheckpoint.Description
Fork-choice:
CheckpointWithPayloadStatus,justifiedPayloadStatus/finalizedPayloadStatusstore fields,getCheckpointPayloadStatus(), andtoCheckpointWithPayload().CheckpointWithPayloadAndBalance→CheckpointWithBalance(+
CheckpointWithPayloadAndTotalBalance→CheckpointWithTotalBalance).getAllAncestorAndNonAncestorBlocksDefaultStatusandgetBlockHexDefaultStatus— archive-side helpers that resolve the canonicalvariant (FULL pre-Gloas, PENDING for Gloas) without callers needing to pass a
PayloadStatus.getAllAncestorAndNonAncestorBlocksnow also returnspreviousFinalizedFull: ProtoBlock | undefined— the FULL variant of theprevious finalized block when its execution payload has been received.
Archive:
archiveBlocksnow archives a post-Gloas finalized block across two runs:SignedBeaconBlockships (payload may still be PENDING/EMPTY).its execution payload envelope and data column sidecars ship.
chain.getBlockByRoot,getBlobSidecars,getDataColumnSidecars,getExecutionPayloadEnvelope, …) already fall backhot→cold, so cross-run intermediate states are transparently handled.
AI Assistance Disclosure
Used Claude Code to refactor the archiver and fork-choice helper, and to draft
tests and comments. Design and review by a human.