Conversation
Summary of ChangesHello @ensi321, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers the foundational implementation for the ePBS (enshrined Proposer-Builder Separation) fork choice, internally referred to as 'Gloas'. It significantly refactors the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request implements the ePBS (enshrined Proposer-Builder Separation) fork choice rule, also known as Gloas. The changes are extensive and touch many parts of the fork choice logic, introducing concepts like block variants (PENDING, EMPTY, FULL) and new tie-breaking rules. The implementation correctly adapts the ProtoArray and ForkChoice classes to handle these new concepts. I've found a few critical issues related to the new tie-breaking logic where necessary context was not being passed down, and a minor issue with some unused code. Overall, this is a solid implementation of a complex feature, and the new tests are comprehensive.
Performance Report🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
| if (existedFullIndex !== undefined) { | ||
| const existedFullNode = this.nodes[existedFullIndex]; | ||
| if (existedFullNode) { | ||
| // Pre-Gloas: execution payloads are part of the block, no separate event |
There was a problem hiding this comment.
throw error if it is the case, we should only have exactly 1 payload per slot/root
also needs to ensure we have PENDING node + EMPTY node, throw error if not
this is equivalent to
# The corresponding beacon block root needs to be known
assert envelope.beacon_block_root in store.block_states
packages/beacon-node/src/chain/blocks/verifyBlocksSanityChecks.ts
Outdated
Show resolved
Hide resolved
**Motivation** - `getBlockSlotState()` with only block root is ambiguous in the context of ePBS because for each block there are 2 variants: EMPTY vs FULL **Description** - refactor it to accept a ProtoBlock as 1st param. When we implement ePBS forkchoice in #8739 we have the context of variant there - change all consumers to provide ProtoBlock - later on we should enhance our state caches to get the correct BeaconState based on that `ProtoBlock` part of #8439 cc @ensi321 @nflaig --------- Co-authored-by: Tuyen Nguyen <twoeths@users.noreply.github.com>
# Summary of Changes
## Overview
Updated fork choice to store and use payload status for both finalized
and justified checkpoints. This ensures `getFinalizedBlock()` and
`getJustifiedBlock()` return the correct block variant (EMPTY or FULL)
based on checkpoint state.
## 1. Added `CheckpointWithPayload` Type
**New type in `store.ts`:**
```typescript
export type CheckpointWithPayload = CheckpointWithHex & {payloadStatus: PayloadStatus};
```
This type extends `CheckpointWithHex` with a `payloadStatus` field to
track whether the checkpoint uses EMPTY or FULL block variant.
## 2. Updated ForkChoiceStore
**Constructor changes:**
- Now takes `justifiedPayloadStatus` and `finalizedPayloadStatus`
parameters
- Stores both finalized and justified checkpoints as
`CheckpointWithPayload`
**Interface changes:**
- `finalizedCheckpoint` and `unrealizedFinalizedCheckpoint` →
`CheckpointWithPayload`
- `justified` and `unrealizedJustified` → use `CheckpointWithPayload`
(via renamed types)
## 3. Renamed Balance Types
- `CheckpointHexWithBalance` → `CheckpointWithPayloadAndBalance`
- `CheckpointHexWithTotalBalance` →
`CheckpointWithPayloadAndTotalBalance`
Both now use `CheckpointWithPayload` instead of `CheckpointWithHex`.
## 4. Added `getCheckpointPayloadStatus()` Helper
**Purpose:** Determines payload status for a checkpoint by checking
`state.executionPayloadAvailability`
**Logic:**
- Pre-Gloas: always returns `FULL`
- Gloas: checks `state.executionPayloadAvailability` at checkpoint slot
**Signature:**
```typescript
export function getCheckpointPayloadStatus(
state: CachedBeaconStateAllForks,
checkpointEpoch: number
): PayloadStatus
```
## 5. Updated `onBlock()` Processing
**For justified checkpoint:**
- Calls `getCheckpointPayloadStatus()` to compute payload status
- Creates `CheckpointWithPayload` with computed status
- Updates both realized and unrealized justified checkpoints
**For finalized checkpoint:**
- Calls `getCheckpointPayloadStatus()` to compute payload status
- Creates `CheckpointWithPayload` with computed status
- Updates both realized and unrealized finalized checkpoints
## 6. Updated Fork Choice Methods
**`getFinalizedBlock()`:**
- Now uses `this.fcStore.finalizedCheckpoint.payloadStatus` instead of
always using `PayloadStatus.FULL`
**`getJustifiedBlock()`:**
- Now uses `this.fcStore.justified.checkpoint.payloadStatus` instead of
always using `PayloadStatus.FULL`
**`getFinalizedCheckpoint()` and `getJustifiedCheckpoint()`:**
- Return type changed to `CheckpointWithPayload`
## 7. Updated Initialization
**`initializeForkChoiceFromFinalizedState()`:**
- Computes `justifiedPayloadStatus` using `getCheckpointPayloadStatus()`
- Computes `finalizedPayloadStatus` using `getCheckpointPayloadStatus()`
- Passes both to `ForkChoiceStore` constructor
**`initializeForkChoiceFromUnfinalizedState()`:**
- Computes payload status for both justified and finalized checkpoints
- Passes both to `ForkChoiceStore` constructor
twoeths
left a comment
There was a problem hiding this comment.
we need to check all *iterator*Ancestor*() logic not to use isDefaultVariant() there
as a consumer, it needs to know the exact correct EMPTY or FULL node at each slot (not PENDING) based on get_parent_payload_status() function so that it can query correct data from there. The logic should mimic get_ancestor() function in the spec
**Motivation** - right now we model both pre-gloas and gloas variants as Array, which has some disadvantages: - array type is too general, it could be 1000 items while we only have 1/2/3 items - item 0 of pre-gloas block is FULL variant index, while item 0 of gloas blocks is PENDING variant index which is a little bit confusing to maintain - we check `lengh=1` as pre-gloas, which is correct based on the implementation but ideally we can do better typing to know it **Description** - model pre-gloas variant index as number - model gloas variants using tuple: `[number, number]` or `[number, number, number]` part of #8739 cc @ensi321 @nflaig --------- Co-authored-by: Tuyen Nguyen <twoeths@users.noreply.github.com> Co-authored-by: NC <17676176+ensi321@users.noreply.github.com>
**Motivation** - getParentPayloadStatus() is confusing and likely not correct right now, we can just leverage the `getBlockHexAndBlockHash()` instead **Description** - move `getBlockHexAndBlockHash()` from forkchoice to ProtoArray - one small change is not to check PENDING as it's not useful for beacon-node - refactor `getParentPayloadStatus()` to call `getBlockHexAndBlockHash()` - add `getParent()` to also call `getBlockHexAndBlockHash()` - `forkchoice.onBlock()` needs to find exact parent via `protoArray.getParent()` because we always have success state-transition before calling forkchoice - enhance error codes, more comments part of #8739 cc @ensi321 @nflaig --------- Co-authored-by: Tuyen Nguyen <twoeths@users.noreply.github.com>
Gloas Fork Choice Implementation (ePBS)
This PR implements most of the Gloas fork choice specification. Blocks can now exist in multiple payload status variants (PENDING, EMPTY, FULL), allowing the beacon chain to progress before execution payloads arrive.
For high level concept please read https://devlog.lodestar.casa/gloas-fork-choice
Key Concept: Payload Status Variants
New enum (
packages/fork-choice/src/protoArray/interface.ts:27-32):Major Changes
1. Multi-Variant Node Storage
ProtoArray indices structure (
packages/fork-choice/src/protoArray/protoArray.ts:41-50):Pre-Gloas:
variants[0]= FULL index (single variant)Gloas:
variants[PayloadStatus.PENDING/EMPTY/FULL]= respective indices2. Block Creation (
onBlock)Pre-Gloas (
packages/fork-choice/src/protoArray/protoArray.ts:428-451):Gloas (
packages/fork-choice/src/protoArray/protoArray.ts:352-428):parentBlockHash)Fork transition: First Gloas block's PENDING correctly points to last Fulu block's FULL.
3. Payload Arrival (
onExecutionPayload)When execution payload arrives (
packages/fork-choice/src/protoArray/protoArray.ts:519-585):4. PTC (Payload Timeliness Committee)
New voting mechanism (
packages/fork-choice/src/protoArray/protoArray.ts:122-151, 753-823):Used by
shouldExtendPayload()to determine FULL vs EMPTY preference.5. Parent Payload Status Determination
getParentPayloadStatus()(packages/fork-choice/src/protoArray/protoArray.ts:173-203):6. Head Selection Changes
findHead()(packages/fork-choice/src/protoArray/protoArray.ts:882-946):RootHexgetAncestor()(packages/fork-choice/src/protoArray/protoArray.ts:1327-1401):RootHexparentBlockHash7. EMPTY vs FULL Tiebreaker
For comparing variants of same block from slot n-1 or n (
packages/fork-choice/src/protoArray/protoArray.ts:854-874, 1132-1299):shouldExtendPayload()returns true if:8. Vote Tracking (Epoch → Slot)
Changed to slot-based tracking (
packages/fork-choice/src/forkChoice/forkChoice.ts:99-126):Attestation interpretation (Gloas) (
packages/fork-choice/src/forkChoice/forkChoice.ts:836-876):slot == block.slot→ vote PENDINGslot > block.slot && index == 0→ vote EMPTYslot > block.slot && index == 1→ vote FULLNote:
voteCurrentIndicesandvoteNextIndicesnow point to exact variant node indices, not just block indices.9. Tree Traversal (Omit Variants)
All tree walking methods filter to default variants only to avoid noise from EMPTY/FULL siblings (
packages/fork-choice/src/protoArray/protoArray.ts:1430-1632):Affected methods:
iterateAncestorNodes()- Only yields PENDING (Gloas) or FULL (pre-Gloas)getAllAncestorNodes(),getAllNonAncestorNodes()- Filter to default variantsforwardIterateDescendants()- Uses minimum variant indexWhy: EMPTY/FULL are payload variants, not distinct branches. Including them would make chains 3x longer with redundant entries. Tree operations care about beacon block relationships, not payload status.
10. Pruning
Pruning now (
packages/fork-choice/src/protoArray/protoArray.ts:964-1022):11. Fork Choice API Changes
Node Relationships
Testing
New test file
packages/fork-choice/test/unit/protoArray/gloas.test.tscovers:onExecutionPayload()(FULL creation)Outstanding Items