Open
Conversation
![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: aa6c1bb31a
ℹ️ 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".
![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 pull request reorders the PayloadStatus enum and refactors the ProtoArray to use these enum values as indices for block variants, replacing hardcoded numeric values. A significant issue was noted where this reordering breaks the tie-breaker logic in getPayloadStatusTiebreaker, which depends on the specific numeric values of the enum to function correctly.
Contributor
Performance Report✔️ no performance regression detected Full benchmark results
|
matthewkeil
reviewed
Apr 6, 2026
Member
matthewkeil
left a comment
matthewkeil
left a comment
There was a problem hiding this comment.
Mostly its the comments. There were a lot of, what feel like, important comments that were updated/deleted/added in this PR. Maybe put them in their own PR or easier can just change the name of the PR to add that scope so these comment changes don't get buried in here.
Comment on lines
+44
to
+46
| EMPTY = 0, | ||
| FULL = 1, | ||
| PENDING = 2, |
Member
There was a problem hiding this comment.
I tend to prefer PascalCase for these but not a hill I'm willing to die on
Member
Author
There was a problem hiding this comment.
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
| * 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 | ||
| * |
Member
There was a problem hiding this comment.
This comment change seems important and non-relevant to the PR. Same with the comment on L#949 and L#954-L#955
Member
Author
There was a problem hiding this comment.
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
Contributor
There was a problem hiding this comment.
Yes, they were chosen because they are in lexicographical order for tiebreaker. Now we change it to be aligned with data.index value.
Contributor
There was a problem hiding this comment.
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
| // Spec: return 1 for EMPTY, 2 for FULL+extend, 0 for FULL+!extend | ||
| if (node.payloadStatus === PayloadStatus.EMPTY) { | ||
| return PayloadStatus.EMPTY; | ||
| return 1; |
Member
There was a problem hiding this comment.
Feels weird to not be returning the enum here when the value will be the number at runtime. Same with line L#960
twoeths
reviewed
Apr 8, 2026
| 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]); |
Contributor
There was a problem hiding this comment.
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;
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #9176 +/- ##
=========================================
Coverage 52.53% 52.53%
=========================================
Files 848 848
Lines 61429 61429
Branches 4528 4528
=========================================
Hits 32270 32270
Misses 29094 29094
Partials 65 65 🚀 New features to boost your workflow:
|
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
5 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.
See ethereum/consensus-specs#4948