feat: implement epbs state transition #8507
Open
Conversation
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
ensi321
changed the title
Performance Report🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## unstable #8507 +/- ##
============================================
- Coverage 51.99% 51.63% -0.37%
============================================
Files 848 855 +7
Lines 65857 66463 +606
Branches 4808 4814 +6
============================================
+ Hits 34242 34316 +74
- Misses 31547 32079 +532
Partials 68 68 🚀 New features to boost your workflow:
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the state transition function for the gloas fork (EPBS). The changes are extensive, touching many parts of the state transition logic to accommodate the new rules for execution payloads, builder payments, and attestations. New processing functions like processExecutionPayloadBid, processExecutionPayloadEnvelope, and processPayloadAttestation have been added, and existing ones like processBlock, processOperations, and processWithdrawals have been updated with gloas-specific logic. The changes appear to be a faithful implementation of the spec. I've found one critical correctness issue and one area for improvement regarding type safety.
| verifySignature: boolean | ||
| ): boolean { | ||
| const indices = indexedPayloadAttestation.attestingIndices; | ||
| const isSorted = indices.every((val, i, arr) => i === 0 || arr[i - 1] <= val); |
There was a problem hiding this comment.
The check for sorted indices arr[i - 1] <= val allows for duplicate validator indices, but the specification requires indices to be strictly increasing, which also implies uniqueness. This could lead to invalid attestations being considered valid.
According to the is_sorted function in the consensus specs, the check should be for strict inequality (<).
Suggested change
| const isSorted = indices.every((val, i, arr) => i === 0 || arr[i - 1] <= val); | |
| const isSorted = indices.every((val, i, arr) => i === 0 || arr[i - 1] < val); |
Comment on lines
+34
to
+47
| for (const deposit of requests.deposits) { | ||
| // TODO GLOAS: Fix type or properly cast it to the correct state type | ||
| processDepositRequest(state as unknown as CachedBeaconStateElectra, deposit); | ||
| } | ||
|
|
||
| for (const withdrawal of requests.withdrawals) { | ||
| // TODO GLOAS: Fix type or properly cast it to the correct state type | ||
| processWithdrawalRequest(ForkSeq.gloas, state as unknown as CachedBeaconStateElectra, withdrawal); | ||
| } | ||
|
|
||
| for (const consolidation of requests.consolidations) { | ||
| // TODO GLOAS: Fix type or properly cast it to the correct state type | ||
| processConsolidationRequest(ForkSeq.gloas, state as unknown as CachedBeaconStateElectra, consolidation); | ||
| } |
There was a problem hiding this comment.
The state object is cast to unknown as CachedBeaconStateElectra before being passed to processDepositRequest, processWithdrawalRequest, and processConsolidationRequest. This is not type-safe and should be avoided. The TODO comments indicate awareness of this issue.
To improve type safety and maintainability, the signatures of these request processing functions should be updated to accept CachedBeaconStateGloas or a compatible union type, which would eliminate the need for these unsafe casts.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Implement epbs state transition function.
Passes all operations and epoch_transition spec tests on v1.6.0-beta.2
Part of #8439