feat: force the proposer to reorg unavailable blocks - #9387
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pul l request upd a t e s t h e G l o a s i m p l e m e n t a t i o n t o a l i g n w i t h c o n s e n s u s - s p e c s v 1 . 7 . 0 - a l p h a . 8 . K e y c h a n g e s i n c l u d e r e n a m i n g g a s _ l i m i t t o t a r g e t _ g a s _ l i m i t a c r o s s t h e v a l i d a t o r a n d e x e c u t i o n l a y e r s , i m p l e m e n t i n g t h e s h o u l d _ b u i l d _ o n _ f u l l f o r k c h o i c e l o g i c , a n d r e f i n i n g p a y l o a d t i m e l i n e s s c h e c k s b a s e d o n a r r i v a l t i m e s t a m p s . T h e P r o t o A r r a y w a s e x p a n d e d t o t r a c k b l o b d a t a a v a i l a b i l i t y a n d e x p l i c i t ' n o ' v o t e s f r o m t h e P a y l o a d T i m e l i n e s s C o m m i t t e e ( P T C ) . R e v i e w f e e d b a c k h i g h l i g h t s t h e n e e d t o r e f i n e t h e f a l l b a c k l o g i c f o r t h e p r o p o s e r ' s t a r g e t g a s l i m i t w h e n p a r e n t b l o c k s a r e e m p t y a n d s u g g e s t s a d d i n g l e n g t h v a l i d a t i o n t o t h e n e w b i t w i s e v o t e c o u n t i n g u t i l i t y t o p r e v e n t p o t e n t i a l o u t - o f - b o u n d s a c c e s s .
Comment on lines
+917
to
+919
| // TODO GLOAS: state.latestExecutionPayloadBid is the latest *bid*, not the latest *executed* | ||
| // payload — for EMPTY parents this drifts. Consider having a default value like Prysm's DefaultBuilderGasLimit. | ||
| return Number(pref ? pref.message.targetGasLimit : state.latestExecutionPayloadBid.gasLimit); |
Contributor
There was a problem hiding this comment.
The fallback to state.latestExecutionPayloadBid.gasLimit when no proposer preferences are pooled might be problematic if the parent block was EMPTY. In that case, the bid in the state reflects the proposer's intent for the skipped payload, which may have drifted from the actual gas limit of the last executed block. Consider using a safer default or a value derived from the last successfully executed payload header if available.
Comment on lines
+34
to
+46
| export function countNoVotes(attended: BitArray, yes: BitArray): number { | ||
| const a = attended.uint8Array; | ||
| const y = yes.uint8Array; | ||
| let count = 0; | ||
| for (let i = 0; i < a.length; i++) { | ||
| let byte = a[i] & ~y[i] & 0xff; | ||
| while (byte) { | ||
| byte &= byte - 1; | ||
| count++; | ||
| } | ||
| } | ||
| return count; | ||
| } |
Contributor
There was a problem hiding this comment.
The countNoVotes function is a critical utility for determining reorg triggers. While the current bitwise implementation is correct and efficient, ensure that the BitArray lengths for attended and yes are always consistent before accessing their uint8Array directly to avoid potential out-of-bounds issues if the underlying buffers differ in size.
Suggested change
| export function countNoVotes(attended: BitArray, yes: BitArray): number { | |
| const a = attended.uint8Array; | |
| const y = yes.uint8Array; | |
| let count = 0; | |
| for (let i = 0; i < a.length; i++) { | |
| let byte = a[i] & ~y[i] & 0xff; | |
| while (byte) { | |
| byte &= byte - 1; | |
| count++; | |
| } | |
| } | |
| return count; | |
| } | |
| export function countNoVotes(attended: BitArray, yes: BitArray): number { | |
| const a = attended.uint8Array; | |
| const y = yes.uint8Array; | |
| if (a.length !== y.length) { | |
| throw new Error("BitArray lengths mismatch in countNoVotes"); | |
| } | |
| let count = 0; | |
| for (let i = 0; i < a.length; i++) { | |
| let byte = a[i] & ~y[i]; | |
| while (byte) { | |
| byte &= byte - 1; | |
| count++; | |
| } | |
| } | |
| return count; | |
| } |
Contributor
Performance Report✔️ no performance regression detected Full benchmark results
|
wemeetagain
previously approved these changes
May 20, 2026
Comment on lines
+95
to
+108
| /** | ||
| * Blob data availability votes per block. | ||
| * Spec: gloas/fork-choice.md#modified-store (payload_data_availability_vote) | ||
| * | ||
| * Bit i = PTC member i voted blobDataAvailable=true (DA YES vote) | ||
| */ | ||
| private daVotes = new Map<RootHex, BitArray>(); | ||
| /** | ||
| * Tracks which PTC members have attested at all (any payload_status). | ||
| * Without this, we cannot tell "didn't vote" (None) from "voted false" — | ||
| * a distinction required by payload_timeliness/payload_data_availability | ||
| * when called with the negative parameter value. | ||
| */ | ||
| private ptcAttested = new Map<RootHex, BitArray>(); |
Member
There was a problem hiding this comment.
This representation is fine, but worth noting the optimization in #9284 if we don't need attribution of votes. (9x less memory there, but with a max savings of ~55MB in worst-case non-finality) (Or we can just leapfrog to lodestar-z fork choice 😂)
Member
There was a problem hiding this comment.
@wemeetagain I liked the event based approach, can we reopen that PR, see #9387 (comment), it relates to that
Member
There was a problem hiding this comment.
(Or we can just leapfrog to lodestar-z fork choice 😂)
please no, obviously the zig fork choice has a bunch of bugs, I am assuming we just LLM copy-pasted the bugs from lodestar ts fork choice, I would be surprised if that is not the case
unless we have fork choice compliance tests wired up and passing I don't think switching to the zig implementation is viable
wemeetagain
marked this pull request as ready for review
May 20, 2026 14:02
![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: acfe0296e7
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+762
to
+764
| if (head.payloadStatus === PayloadStatus.PENDING) { | ||
| throw new Error("shouldBuildOnFull called with PENDING head"); | ||
| } |
There was a problem hiding this comment.
shouldBuildOnFull now throws for PayloadStatus.PENDING, but proposer flows pass fork-choice heads directly into this method (via recomputeForkChoiceHead() / getProposerHead()), and findHead() explicitly allows Gloas heads to be PENDING. In that state (e.g., before FULL/EMPTY resolution), this exception can bubble out of slot preparation or block production instead of safely defaulting to EMPTY behavior, which can cause missed proposal work.
Useful? React with 👍 / 👎.
wemeetagain
approved these changes
May 21, 2026
1 task
nflaig
reviewed
May 26, 2026
| throw new Error("shouldBuildOnFull called with PENDING head"); | ||
| } | ||
| if (head.payloadStatus === PayloadStatus.EMPTY) return false; | ||
| return !this.isPayloadDataNotAvailable(head.blockRoot); |
Member
There was a problem hiding this comment.
Just a thought, this seems mostly relevant if we are not a (semi) supernode as we cannot validate data availability ourselves, but otherwise, we could ignore this check if we have observed all data. This is still debatable imo if we even wanna consider this case at all or keep as is for any node type
Member
Author
There was a problem hiding this comment.
I don't see supernode/semi-supernode matters here
we should always consult PTC for this, instead of checking how we receive columns/blobs. That's what PTC is designed for, it's a consensus of a committee, instead of deciding it on our own
there should be a spec test for this too
Member
There was a problem hiding this comment.
That's what PTC is designed for
it's mostly relevant for timeliness, if we are a supernode and seed the columns to the network, I don't think it's required to rely on the PTC for data availability as it's very likely that attesters will have all the data by the attestation deadline (since we seeded the columns) and vote for our block if we build on FULL. In any case, this scenario seems unlikely and not worth to handle separately
This was referenced May 28, 2026
glamsterdam-devnet-4: produced blocks reorged due to shouldBuildOnFull() conflict to FULL head
#9415
Closed
This was referenced May 30, 2026
nflaig
pushed a commit
that referenced
this pull request
May 30, 2026
## Motivation Align the private field name in `ProtoArray` with the gloas fork-choice spec, which calls this store entry [`payload_timeliness_vote`](https://github.com/ethereum/consensus-specs/blob/dev/specs/gloas/fork-choice.md#modified-store). The companion field for blob-data availability is already tracked via the existing `daVotes` map. This was the last unmerged piece of lodekeeper#8 — the data-availability tracking landed independently in #9416 and force-reorg in #9387, so only the rename is left. ## Changes - `packages/fork-choice/src/protoArray/protoArray.ts`: rename `private ptcVotes` → `private payloadTimelinessVotes` and all 6 in-class references. - `packages/fork-choice/test/unit/protoArray/gloas.test.ts`: update 2 comments that reference the old field name. Pure rename — no behavior change. Public `getPTCVotes()` API, `isPayloadTimely`/`isPayloadNotTimely`, and surrounding spec comments are unchanged. ## Verification - `pnpm check-types` clean in `packages/fork-choice`. - `pnpm biome check` clean on the two touched files. - `vitest run test/unit/protoArray/gloas.test.ts` → 80/80 pass. 🤖 Generated with AI assistance --------- Co-authored-by: lodekeeper <lodekeeper@users.noreply.github.com>
nflaig
reviewed
May 31, 2026
| if (isStatePostGloas(updatedPrepareState)) { | ||
| if (this.chain.forkChoice.shouldExtendPayload(updatedHead.blockRoot)) { | ||
| // Spec: should_build_on_full(store, head) — see produceBlockBody.ts for context. | ||
| if (this.chain.forkChoice.shouldBuildOnFull(updatedHead)) { |
Member
There was a problem hiding this comment.
worth noting that the timing when this is called is not correct, unless I am missing some other changes we did in the meantime but #9164 (comment) and suggestion along the lines here #9164 (comment) are still relevant
we will still reorg the payload when we call shouldBuildOnFull() == false in produceBlockBody.ts but that's at the start of the next slot and the execution payload we prepared here will be on top of the blockHash and not the parentBlockHash
likely delaying this call to PTC deadline + 1 second is fine, but may not be ideal as we also don't wanna delay the FCU for too long as it gives less time to the execution client
likely the ideal approach is
- call FCU at 8 seconds (as today) if we have verified the parent's payload, or as soon as we receive it afterswards
- evaluate
shouldBuildOnFull()again at ~10 seconds (with PTC votes), or proactively if threshold is reached via event based trigger (something like here) and ifparentBlockHashchanges, call FCU again, overriding the previous call - keep the final decision in
produceBlockBody.tsat start of next slot but it should be rare that theparentBlockHashwill be different from the prepared block hash if we follow the above
also I noticed we should probably add more logs here to know if we do a payload reorg
Member
|
🎉 This PR is included in v1.44.0 🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
shouldBuildOnFull()as in Force the proposer to reorg unavailable blocks ethereum/consensus-specs#5186Description
daVotesptcAttestedshouldBuildOnFull()when producing blockAI Assistance Disclosure
Created with Claude