feat: implement EIP 7688#9390
Draft
wemeetagain wants to merge 4 commits into
Draft
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 pull request implements support for the Gloas fork (EIP-7688) by transitioning core consensus structures like BeaconState and BeaconBlockBody to progressive SSZ types. Key changes include updating dependencies to @chainsafe/ssz v1.6.0, implementing Gloas-specific light client proof logic, and establishing P2P size limits for progressive gossip objects. Review feedback identifies a potential vulnerability where getGossipSSZMaxSize lacks explicit limits for attestations and points out a performance regression in the participation flag update logic due to O(N) array allocations. Further suggestions include using Gloas-native default headers in the light client server and simplifying fork-specific witness validation logic to ensure consistency and efficiency.
Comment on lines
+143
to
+162
| export function getGossipSSZMaxSize(topic: GossipTopic, maxPayloadSize: number, sszType?: CompositeTypeAny): number { | ||
| const {fork} = topic.boundary; | ||
| // Gloas progressive containers have broad theoretical SSZ max sizes; use the preset p2p bounds instead. | ||
| switch (topic.type) { | ||
| case GossipType.beacon_block: | ||
| return isForkPostGloas(fork) ? MAX_SIGNED_BEACON_BLOCK_SIZE : maxPayloadSize; | ||
| case GossipType.beacon_aggregate_and_proof: | ||
| return isForkPostGloas(fork) ? MAX_SIGNED_AGGREGATE_AND_PROOF_SIZE : (sszType ?? getGossipSSZType(topic)).maxSize; | ||
| case GossipType.attester_slashing: | ||
| return isForkPostGloas(fork) ? MAX_ATTESTER_SLASHING_SIZE : (sszType ?? getGossipSSZType(topic)).maxSize; | ||
| case GossipType.data_column_sidecar: | ||
| return isForkPostGloas(fork) ? MAX_DATA_COLUMN_SIDECAR_SIZE : (sszType ?? getGossipSSZType(topic)).maxSize; | ||
| case GossipType.execution_payload: | ||
| return maxPayloadSize; | ||
| case GossipType.execution_payload_bid: | ||
| return MAX_SIGNED_EXECUTION_PAYLOAD_BID_SIZE; | ||
| default: | ||
| return (sszType ?? getGossipSSZType(topic)).maxSize; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
The getGossipSSZMaxSize function is missing explicit P2P size limits for several progressive types in Gloas, most notably GossipType.attestation. Since ssz.gloas.Attestation is a ProgressiveContainerType, its theoretical maxSize is extremely large. Failing to cap it at the P2P preset limit (MAX_ATTESTATION_SIZE or similar) could expose the node to memory exhaustion attacks via oversized gossip messages.
| // Assign the hightest fork's default value because it can always be typecasted down to correct fork | ||
| finalizedHeader: sszTypesFor(highestFork(forkPostAltair)).LightClientHeader.defaultValue(), | ||
| // Assign the highest pre-Gloas light-client header because post-Gloas light-client updates are skipped for now. | ||
| finalizedHeader: ssz.electra.LightClientHeader.defaultValue(), |
Contributor
There was a problem hiding this comment.
The finalizedHeader is hardcoded to ssz.electra.LightClientHeader.defaultValue(). While the comment mentions that post-Gloas updates are skipped for now, this PR introduces Gloas light-client header support and upgrades. It would be more consistent and forward-compatible to use the highest fork's default value or explicitly handle Gloas if the server is intended to support it eventually.
Suggested change
| finalizedHeader: ssz.electra.LightClientHeader.defaultValue(), | |
| finalizedHeader: ssz.gloas.LightClientHeader.defaultValue(), |
Comment on lines
+659
to
671
| if ( | ||
| isForkPostGloas(attestedFork) && | ||
| (syncCommitteeWitness.currentSyncCommitteeBranch === undefined || | ||
| syncCommitteeWitness.nextSyncCommitteeBranch === undefined) | ||
| ) { | ||
| throw Error("Expected post-Gloas sync committee branches"); | ||
| } | ||
| if (!isForkPostGloas(attestedFork) && isForkPostElectra(attestedFork) && numWitness !== NUM_WITNESS_ELECTRA) { | ||
| throw Error(`Expected ${NUM_WITNESS_ELECTRA} witnesses in post-Electra numWitness=${numWitness}`); | ||
| } | ||
| if (!isForkPostElectra(attestedFork) && numWitness !== NUM_WITNESS) { | ||
| if (!isForkPostGloas(attestedFork) && !isForkPostElectra(attestedFork) && numWitness !== NUM_WITNESS) { | ||
| throw Error(`Expected ${NUM_WITNESS} witnesses in pre-Electra numWitness=${numWitness}`); | ||
| } |
Contributor
There was a problem hiding this comment.
The logic for validating numWitness and sync committee branches can be simplified using else if blocks to avoid redundant fork checks. Additionally, for Gloas, since explicit branches are used, numWitness (the length of the legacy witness array) should be verified to be 0 to ensure consistency with the generator logic in proofs.ts.
if (isForkPostGloas(attestedFork)) {
if (
syncCommitteeWitness.currentSyncCommitteeBranch === undefined ||
syncCommitteeWitness.nextSyncCommitteeBranch === undefined
) {
throw Error("Expected post-Gloas sync committee branches");
}
if (numWitness !== 0) {
throw Error(`Expected 0 witnesses in post-Gloas numWitness=${numWitness}`);
}
} else if (isForkPostElectra(attestedFork)) {
if (numWitness !== NUM_WITNESS_ELECTRA) {
throw Error(`Expected ${NUM_WITNESS_ELECTRA} witnesses in post-Electra numWitness=${numWitness}`);
}
} else if (numWitness !== NUM_WITNESS) {
throw Error(`Expected ${NUM_WITNESS} witnesses in pre-Electra numWitness=${numWitness}`);
}
Comment on lines
+37
to
+39
| state.currentEpochParticipation = ssz.gloas.EpochParticipation.toViewDU( | ||
| new Array<number>(state.currentEpochParticipation.length).fill(0) | ||
| ); |
Contributor
There was a problem hiding this comment.
Using new Array<number>(...).fill(0) followed by toViewDU results in an O(N) memory allocation and a full re-merkleization of the participation list every epoch. For large validator sets (e.g., 1M+), this is a significant performance regression compared to the Altair implementation which uses zeroNode to create a zeroed tree in O(1) or O(log N). If ProgressiveListBasicType supports it, consider using a zero-tree initialization. At minimum, using new Uint8Array(state.currentEpochParticipation.length) would be more memory-efficient than a standard array.
Suggested change
| state.currentEpochParticipation = ssz.gloas.EpochParticipation.toViewDU( | |
| new Array<number>(state.currentEpochParticipation.length).fill(0) | |
| ); | |
| state.currentEpochParticipation = ssz.gloas.EpochParticipation.toViewDU( | |
| new Uint8Array(state.currentEpochParticipation.length) | |
| ); |
Contributor
Performance Report🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
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
1 participant
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
Description
AI Assistance Disclosure