Closed
Conversation
Contributor
Summary of ChangesHello, 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 optimizes the handling of Payload Timeliness Committees (PTCs) for the Gloas fork by implementing a direct caching mechanism within the BeaconState. This change aims to enhance performance and simplify the access patterns for PTCs by storing 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. Changelog
Activity
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
|
nflaig
commented
Mar 16, 2026
| /** TODO: Indexed SyncCommitteeCache */ | ||
| nextSyncCommitteeIndexed: SyncCommitteeCache; | ||
|
|
||
| // TODO GLOAS: See if we need to cache PTC for next epoch |
Member
Author
There was a problem hiding this comment.
removed since PTC assignments are not stable for the next epoch, see ethereum/beacon-APIs#586
nflaig
commented
Mar 16, 2026
|
|
||
| // TODO GLOAS: See if we need to cache PTC for next epoch | ||
| // PTC for previous epoch, required for slot N block validating slot N-1 attestations | ||
| previousPayloadTimelinessCommittees: Uint32Array[]; |
Member
Author
There was a problem hiding this comment.
we now longer need to cache this since we only need to access the last slot of previous epoch which is now cached in the state via state.previous_ptc
![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 introduces cached Payload Timeliness Committees (PTCs) to the state, enhancing the efficiency of payload attestation validation. It involves modifications across several files, including adding a new function for computing PTCs, initializing and rotating PTCs within the state, and updating relevant data structures and processes. The changes aim to optimize performance by caching PTC data for faster access during validation.
Contributor
Performance Report✔️ no performance regression detected Full benchmark results
|
nflaig
changed the title
![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: ced9835441
ℹ️ 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".
nflaig
commented
Mar 17, 2026
| export function initializePayloadTimelinessCommittee(state: CachedBeaconStateGloas): void { | ||
| state.currentPtc = ssz.gloas.PayloadTimelinessCommittee.toViewDU( | ||
| // TODO: Array.from shouldn't be required here | ||
| Array.from(computePayloadTimelinessCommittee(state)) |
Member
Author
There was a problem hiding this comment.
the Array.from here does an unnecessary copy just to make types happy, we could do ChainSafe/ssz#512 to resolve this
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3463cefbc
ℹ️ 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".
nflaig
commented
Mar 17, 2026
Comment on lines
+217
to
+238
| const attestingIndices = payloadAttestation.aggregationBits.intersectValues(ptc); | ||
| const attestingIndices: number[] = []; | ||
|
|
||
| for (let i = 0; i < ptc.length; i++) { | ||
| if (payloadAttestation.aggregationBits.get(i)) { | ||
| attestingIndices.push(ptc.get(i)); | ||
| } | ||
| } |
Member
Author
There was a problem hiding this comment.
wondering if the previous approach here was better using intersectValues() but having to do getAll() on the ptc cached in the state isn't great
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f9a4fb81d
ℹ️ 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".
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76809448da
ℹ️ 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".
twoeths
reviewed
Mar 18, 2026
| return headState; | ||
| } | ||
|
|
||
| async getHeadStateAtSlot(slot: Slot, regenCaller: RegenCaller): Promise<CachedBeaconStateAllForks> { |
Contributor
There was a problem hiding this comment.
this method should be bound to currentSlot only to prevent a DOS attack
twoeths
reviewed
Mar 18, 2026
| ); | ||
| } | ||
|
|
||
| export function getPayloadTimelinessCommittee(state: CachedBeaconStateGloas, slot: Slot): PtcCommitteeView { |
Contributor
There was a problem hiding this comment.
Suggested change
| export function getPayloadTimelinessCommittee(state: CachedBeaconStateGloas, slot: Slot): PtcCommitteeView { | |
| export function getPayloadTimelinessCommittee(state: CachedBeaconStateGloas, slot: Slot): Uint32Array { |
we can get from state.epochCtx.payloadTimelinessCommittees[slot % SLOTS_PER_EPOCH] to avoid traversing the state tree
Member
Author
There was a problem hiding this comment.
we can for the current epoch but not for previous epoch, I actually explicitly wanted to use the ptcs in the state here to minimize risk of cache/state ptc not being in sync for some reason, it shouldn't happen but just to minimize potential bugs
if we wanna use state.epochCtx can add a separate item to epoch cache for only the first slot of previous epoch, my thinking here though was that we kinda wanna avoid that and if we can use info we have anyways in state
twoeths
reviewed
Mar 18, 2026
| }); | ||
| } | ||
|
|
||
| const state = chain.getHeadState() as CachedBeaconStateGloas; |
Contributor
There was a problem hiding this comment.
we can just use the state at the same epoch and query epochCtx from there?
then reusing chain.getHeadStateAtEpoch should be enough
Contributor
There was a problem hiding this comment.
the reason is getHeadStateAtSlot is not as cheap as in the past anymore
Member
Author
There was a problem hiding this comment.
we can just use the state at the same epoch and query epochCtx from there?
if we add previous ptc to the epochCtx this might be fine, there was a concern that the state is not dialed forward correctly since ptcs change across slots even if empty, need to make sure this code is safe and handles epoch transitions correctly
twoeths
reviewed
Mar 18, 2026
| validatorCommitteeIndex: number; | ||
| }; | ||
|
|
||
| export async function validateApiPayloadAttestationMessage( |
Contributor
There was a problem hiding this comment.
api PayloadAttestationMessage should have higher priority to verify signature, similar to the regular attestation
something like:
const isValid = await chain.bls.verifySignatureSets([signatureSet], {batchable: true, priority: prioritizeBls});
twoeths
reviewed
Mar 18, 2026
| } | ||
|
|
||
| if (fork >= ForkSeq.gloas) { | ||
| rotatePayloadTimelinessCommittees(postState as CachedBeaconStateGloas); |
Contributor
There was a problem hiding this comment.
add a comment saying that payloadTimelinessCommittees is computed in finalProcessEpoch above
twoeths
reviewed
Mar 18, 2026
Contributor
twoeths
left a comment
twoeths
left a comment
There was a problem hiding this comment.
I think this ptc validation is a great candidate to validate in batch to save computeSigningRoot() time + verifySignatureSetsSameMessage
but it's too much to start with, we can explore at later devnets
Member
Author
|
putting as draft, will propose a different approach in the spec, let's revisit after spec PR is merged |
Member
Author
|
ethereum/consensus-specs#4979 was merged on the spec side |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #9047 +/- ##
=========================================
Coverage 52.32% 52.32%
=========================================
Files 848 848
Lines 62472 62470 -2
Branches 4597 4597
=========================================
- Hits 32691 32690 -1
+ Misses 29716 29715 -1
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
2 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#4992