Skip to content

refactor: reorder payload status constants #9176

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nflaig wants to merge 2 commits into
base: unstable
Choose a base branch
from
Open

refactor: reorder payload status constants #9176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa6c1bb
refactor: reorder payload status constants
nflaig Apr 4, 2026
352cdf2
address bot review
nflaig Apr 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions packages/beacon-node/src/chain/stateCache/persistentCheckpointsCache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {routes} from "@lodestar/api";
import {BeaconConfig} from "@lodestar/config";
import {CheckpointWithPayloadStatus} from "@lodestar/fork-choice";
import {CheckpointWithPayloadStatus, PayloadStatus} from "@lodestar/fork-choice";
import {IBeaconStateView, computeStartSlotAtEpoch} from "@lodestar/state-transition";
import {Epoch, RootHex, phase0} from "@lodestar/types";
import {Logger, MapDef, fromHex, sleep, toHex, toRootHex} from "@lodestar/utils";
Expand Down Expand Up @@ -930,8 +930,6 @@ export function toCheckpointHexPayload(checkpoint: phase0.Checkpoint, payloadPre
* @throws Error if checkpoint has PENDING payload status (ambiguous which variant to use)
*/
export function fcCheckpointToHexPayload(checkpoint: CheckpointWithPayloadStatus): CheckpointHexPayload {
const PayloadStatus = {PENDING: 0, EMPTY: 1, FULL: 2} as const;

if (checkpoint.payloadStatus === PayloadStatus.PENDING) {
throw Error(
`Cannot convert checkpoint with PENDING payload status at epoch ${checkpoint.epoch} root ${checkpoint.rootHex}`
Expand Down
4 changes: 2 additions & 2 deletions packages/beacon-node/test/perf/chain/opPools/aggregatedAttestationPool.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ describe.skip(`getAttestationsForBlock vc=${vc}`, () => {
dataAvailabilityStatus: DataAvailabilityStatus.PreData,

parentBlockHash: null,
payloadStatus: 2, // PayloadStatus.FULL
payloadStatus: PayloadStatus.FULL,
},
originalState.slot
);
Expand Down Expand Up @@ -98,7 +98,7 @@ describe.skip(`getAttestationsForBlock vc=${vc}`, () => {
dataAvailabilityStatus: DataAvailabilityStatus.PreData,

parentBlockHash: null,
payloadStatus: 2, // PayloadStatus.FULL
payloadStatus: PayloadStatus.FULL,
},
slot,
null
Expand Down
4 changes: 2 additions & 2 deletions packages/beacon-node/test/utils/validationData/attestation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {BitArray, toHexString} from "@chainsafe/ssz";
import {ExecutionStatus, IForkChoice, ProtoBlock} from "@lodestar/fork-choice";
import {ExecutionStatus, IForkChoice, PayloadStatus, ProtoBlock} from "@lodestar/fork-choice";
import {testLogger} from "@lodestar/logger/test-utils";
import {DOMAIN_BEACON_ATTESTER} from "@lodestar/params";
import {
Expand Down Expand Up @@ -82,7 +82,7 @@ export function getAttestationValidData(opts: AttestationValidDataOpts): {
dataAvailabilityStatus: DataAvailabilityStatus.PreData,

parentBlockHash: null,
payloadStatus: 2, // PayloadStatus.FULL
payloadStatus: PayloadStatus.FULL,
};

const shufflingCache = new ShufflingCache(null, null, {}, [
Expand Down
6 changes: 3 additions & 3 deletions packages/fork-choice/src/protoArray/interface.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@ export enum ExecutionStatus {
* Spec: gloas/fork-choice.md#constants
*/
export enum PayloadStatus {
PENDING = 0,
EMPTY = 1,
FULL = 2,
EMPTY = 0,
FULL = 1,
PENDING = 2,
Comment thread
nflaig marked this conversation as resolved.
Comment thread
nflaig marked this conversation as resolved.
Comment on lines +44 to +46
Copy link
Copy Markdown
Member

@matthewkeil matthewkeil Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to prefer PascalCase for these but not a hill I'm willing to die on

Copy link
Copy Markdown
Member Author

@nflaig nflaig Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are spec constants, I'd rather keep them as is

| `PAYLOAD_STATUS_EMPTY`               | `PayloadStatus(0)`      |
| `PAYLOAD_STATUS_FULL`                | `PayloadStatus(1)`      |
| `PAYLOAD_STATUS_PENDING`             | `PayloadStatus(2)`      |

we just defined them as a single enum

}
Comment thread
nflaig marked this conversation as resolved.

/**
Expand Down
73 changes: 33 additions & 40 deletions packages/fork-choice/src/protoArray/protoArray.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,12 @@ const ZERO_HASH_HEX = toRootHex(Buffer.alloc(32, 0));
/** Pre-Gloas: single element, FULL index (for backward compatibility) */
type PreGloasVariantIndex = number;
/**
* Post-Gloas: array length is 2 or 3
* - Length 2: [PENDING_INDEX, EMPTY_INDEX] when payload hasn't arrived yet
* - Length 3: [PENDING_INDEX, EMPTY_INDEX, FULL_INDEX] when payload has arrived
* Post-Gloas: always length 3, indexed by PayloadStatus enum value
* - [EMPTY_INDEX, FULL_INDEX | undefined, PENDING_INDEX]
* - PayloadStatus.EMPTY=0, PayloadStatus.FULL=1, PayloadStatus.PENDING=2
* - FULL starts as undefined until execution payload arrives
*/
type GloasVariantIndices = [number, number] | [number, number, number];
type GloasVariantIndices = [number, number | undefined, number];
type VariantIndices = PreGloasVariantIndex | GloasVariantIndices;

export class ProtoArray {
Expand All @@ -49,10 +50,10 @@ export class ProtoArray {
/**
* Maps block root to array of node indices for each payload status variant
*
* Array structure: [PENDING, EMPTY, FULL] where indices correspond to PayloadStatus enum values
* - number[0] = PENDING variant index (PayloadStatus.PENDING = 0)
* - number[1] = EMPTY variant index (PayloadStatus.EMPTY = 1)
* - number[2] = FULL variant index (PayloadStatus.FULL = 2)
* Array structure: [EMPTY, FULL, PENDING] indexed by PayloadStatus enum values
* - number[0] = EMPTY variant index (PayloadStatus.EMPTY = 0)
* - number[1] = FULL variant index (PayloadStatus.FULL = 1), undefined until payload arrives
* - number[2] = PENDING variant index (PayloadStatus.PENDING = 2)
*
* Note: undefined array elements indicate that variant doesn't exist for this block
*/
Expand Down Expand Up @@ -143,7 +144,7 @@ export class ProtoArray {
});
}

// Gloas: return the specified variant, or PENDING if not specified
// Gloas: return the specified variant
return variantOrArr[payloadStatus];
}

Expand Down Expand Up @@ -441,11 +442,11 @@ export class ProtoArray {
// Check if parent exists by getting variants array
const parentVariants = this.indices.get(block.parentRoot);
if (parentVariants != null) {
const anyParentIndex = Array.isArray(parentVariants) ? parentVariants[0] : parentVariants;
const anyParentIndex = Array.isArray(parentVariants) ? parentVariants[PayloadStatus.PENDING] : parentVariants;
const anyParentNode = this.nodes[anyParentIndex];

if (!isGloasBlock(anyParentNode)) {
// Fork transition: parent is pre-Gloas, so it only has FULL variant at variants[0]
// Fork transition: parent is pre-Gloas, so it only has FULL variant
parentIndex = anyParentIndex;
} else {
// Both blocks are Gloas: determine which parent payload status to extend
Expand Down Expand Up @@ -481,9 +482,9 @@ export class ProtoArray {
const emptyIndex = this.nodes.length;
this.nodes.push(emptyNode);

// Store both variants in the indices array
// [PENDING, EMPTY, undefined] - FULL will be added later if payload arrives
this.indices.set(block.blockRoot, [pendingIndex, emptyIndex]);
// Store variants indexed by PayloadStatus enum values
// [EMPTY=0, FULL=1(undefined), PENDING=2] - FULL set when payload arrives
this.indices.set(block.blockRoot, [emptyIndex, undefined, pendingIndex]);
Copy link
Copy Markdown
Contributor

@twoeths twoeths Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if the spec changes the constant again?
maybe create an array of size 3 and set by constant instead

variants[EMPTY] = emptyIndex;
variants[PENDING] = pendingIndex;


// Update bestChild pointers
if (parentIndex !== undefined) {
Expand Down Expand Up @@ -603,7 +604,7 @@ export class ProtoArray {
const fullIndex = this.nodes.length;
this.nodes.push(fullNode);

// Add FULL variant to the indices array
// Append FULL variant to the indices array
variants[PayloadStatus.FULL] = fullIndex;

// Update bestChild for PENDING node (may now prefer FULL over EMPTY)
Expand Down Expand Up @@ -937,32 +938,26 @@ export class ProtoArray {
* Get payload status tiebreaker for fork choice comparison
* Spec: gloas/fork-choice.md#new-get_payload_status_tiebreaker
*
* For PENDING nodes: always returns 0
* For EMPTY/FULL variants from slot n-1: implements tiebreaker logic based on should_extend_payload
* For older blocks: returns node.payloadStatus
* For PENDING or not-previous-slot: returns node.payloadStatus
* For EMPTY/FULL from slot n-1: returns hardcoded tiebreaker values per spec
* - EMPTY → 1, FULL+extend → 2, FULL+!extend → 0
*
Copy link
Copy Markdown
Member

@matthewkeil matthewkeil Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment change seems important and non-relevant to the PR. Same with the comment on L#949 and L#954-L#955

Copy link
Copy Markdown
Member Author

@nflaig nflaig Apr 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this is strange I agree, it seems like the tiebreaker uses different values, maybe @ensi321 can take a look at this, it's bit strange

I think the constant values for EMPTY, PENDING, FULL, were initially chosen so they match the tiebreaker, but now they no longer do

Copy link
Copy Markdown
Contributor

@ensi321 ensi321 Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they were chosen because they are in lexicographical order for tiebreaker. Now we change it to be aligned with data.index value.

Copy link
Copy Markdown
Contributor

@ensi321 ensi321 Apr 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EMPTY → 1, FULL+extend → 2, FULL+!extend → 0

I think this comment is too detailed and can cause a lot overthinking.

Just think about 0 < 1 < 2 when comparing two nodes.

1 means EMPTY
2 means we prefer this over EMPTY because should_extend_payload is true
0 means ineligible for comparison. For example the node is PENDING, or the node's slot is outside of tiebreaker comparison window (too old or too new).

These numbers are not 1:1 mapped to PayloadStatus

* Note: pre-gloas logic won't reach here. Pre-Gloas blocks have different roots, so they are always resolved by the weight and root tiebreaker before reaching here.
* Note: pre-gloas logic won't reach here. Pre-Gloas blocks have different roots,
* so they are always resolved by the weight and root tiebreaker before reaching here.
*/
private getPayloadStatusTiebreaker(node: ProtoNode, currentSlot: Slot, proposerBoostRoot: RootHex | null): number {
// PENDING nodes always return PENDING (no tiebreaker needed)
// PENDING=0, EMPTY=1, FULL=2
if (node.payloadStatus === PayloadStatus.PENDING) {
// PENDING or not from previous slot: return raw PayloadStatus value
if (node.payloadStatus === PayloadStatus.PENDING || node.slot + 1 !== currentSlot) {
return node.payloadStatus;
}

// For Gloas: check if from previous slot
if (node.slot + 1 !== currentSlot) {
return node.payloadStatus;
}

// For previous slot blocks in Gloas, decide between FULL and EMPTY
// based on should_extend_payload
// Previous slot EMPTY/FULL: use spec-defined tiebreaker values (not PayloadStatus enum values)
// Spec: return 1 for EMPTY, 2 for FULL+extend, 0 for FULL+!extend
if (node.payloadStatus === PayloadStatus.EMPTY) {
return PayloadStatus.EMPTY;
return 1;
Copy link
Copy Markdown
Member

@matthewkeil matthewkeil Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels weird to not be returning the enum here when the value will be the number at runtime. Same with line L#960

Copy link
Copy Markdown
Member Author

@nflaig nflaig Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see #9176 (comment)

}
// FULL - check should_extend_payload
const shouldExtend = this.shouldExtendPayload(node.blockRoot, proposerBoostRoot);
return shouldExtend ? PayloadStatus.FULL : PayloadStatus.PENDING;
return this.shouldExtendPayload(node.blockRoot, proposerBoostRoot) ? 2 : 0;
}

/**
Expand Down Expand Up @@ -1060,7 +1055,7 @@ export class ProtoArray {
});
}

// For Gloas, PENDING variant (index 0) is always the smallest since it's inserted first
// For Gloas, PENDING has the smallest node index since it's pushed to this.nodes first
const finalizedIndex = Array.isArray(variants) ? variants[PayloadStatus.PENDING] : variants;

if (finalizedIndex < this.pruneThreshold) {
Expand Down Expand Up @@ -1397,7 +1392,7 @@ export class ProtoArray {
* Gloas: Returns (root, payloadStatus) based on actual node state
*/
getAncestor(blockRoot: RootHex, ancestorSlot: Slot): ProtoNode {
// Get any variant to check the block (use variants[0])
// Get any variant to check the block (PENDING for Gloas, FULL for pre-Gloas)
const variantOrArr = this.indices.get(blockRoot);
if (variantOrArr == null) {
throw new ForkChoiceError({
Expand All @@ -1406,13 +1401,11 @@ export class ProtoArray {
});
}

const blockIndex = Array.isArray(variantOrArr) ? variantOrArr[0] : variantOrArr;
const blockIndex = Array.isArray(variantOrArr) ? variantOrArr[PayloadStatus.PENDING] : variantOrArr;
const block = this.nodes[blockIndex];

// If block is at or before queried slot, return PENDING variant (or FULL for pre-Gloas)
// If block is at or before queried slot, return any variant (PENDING for Gloas, FULL for pre-Gloas)
if (block.slot <= ancestorSlot) {
// For pre-Gloas: only FULL exists at variants[0]
// For Gloas: PENDING is at variants[0]
return block;
}

Expand All @@ -1428,7 +1421,7 @@ export class ProtoArray {
});
}

let parentIndex = Array.isArray(parentVariants) ? parentVariants[0] : parentVariants;
let parentIndex = Array.isArray(parentVariants) ? parentVariants[PayloadStatus.PENDING] : parentVariants;
let parentBlock = this.nodes[parentIndex];

// Walk backwards while parent.slot > ancestorSlot
Expand All @@ -1444,7 +1437,7 @@ export class ProtoArray {
});
}

parentIndex = Array.isArray(nextParentVariants) ? nextParentVariants[0] : nextParentVariants;
parentIndex = Array.isArray(nextParentVariants) ? nextParentVariants[PayloadStatus.PENDING] : nextParentVariants;
parentBlock = this.nodes[parentIndex];
}

Expand Down
2 changes: 1 addition & 1 deletion packages/fork-choice/test/unit/forkChoice/forkChoice.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ describe("Forkchoice", () => {
bestDescendant: undefined,
parent: 0,
weight: 0,
payloadStatus: 2, // Pre-Gloas blocks always have PAYLOAD_STATUS_FULL
payloadStatus: PayloadStatus.FULL, // Pre-Gloas blocks always have PAYLOAD_STATUS_FULL
});
});

Expand Down
4 changes: 2 additions & 2 deletions packages/fork-choice/test/unit/protoArray/executionStatusUpdates.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,9 +300,9 @@ describe("executionStatus / invalidate all postmerge chain", () => {
const fcHead = fc.findHead("0", 3);
it("pre merge block should be the FC head", () => {
// findHead returns ProtoNode
// For pre-Gloas blocks, this should have blockRoot "0" and payloadStatus FULL (2)
// For pre-Gloas blocks, this should have blockRoot "0" and payloadStatus FULL
expect(fcHead.blockRoot).toBe("0");
expect(fcHead.payloadStatus).toBe(2); // PayloadStatus.FULL
expect(fcHead.payloadStatus).toBe(PayloadStatus.FULL);
});
});

Expand Down
2 changes: 1 addition & 1 deletion packages/fork-choice/test/unit/protoArray/gloas.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ describe("Gloas Fork Choice", () => {

const variants = (protoArray as any).indices.get("0x02");
expect(variants).toBeDefined();
// Gloas: variants[PENDING] and variants[EMPTY] should be defined
// Gloas: variants indexed by PayloadStatus [EMPTY, FULL|undefined, PENDING]
expect(variants[PayloadStatus.PENDING]).toBeDefined();
expect(variants[PayloadStatus.EMPTY]).toBeDefined();
expect(variants[PayloadStatus.FULL]).toBeUndefined();
Expand Down
Loading