chore: review epbs state transition changes (part 2)#8660
chore: review epbs state transition changes (part 2)#8660ensi321 merged 3 commits intonc/epbs-stffrom
Conversation
|
/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 primarily focuses on extending state transition logic to support the gloas fork, which is a significant and well-executed part of the changes. Many functions are updated to handle CachedBeaconStateGloas, improving type safety and preparing for the new fork. The changes also include several valuable code quality improvements, such as removing TODOs, eliminating unsafe type casts, and enhancing error logging for better debuggability. However, I've identified one critical issue in packages/state-transition/src/block/processWithdrawals.ts that appears to be an off-by-one error in a loop condition, potentially affecting consensus by not including the maximum number of withdrawals in a block. This should be addressed.
| : stateGloas.builderPendingWithdrawals.getReadonly(i); | ||
|
|
||
| if (withdrawal.withdrawableEpoch > epoch || withdrawals.length === MAX_WITHDRAWALS_PER_PAYLOAD) { | ||
| if (withdrawal.withdrawableEpoch > epoch || withdrawals.length + 1 === MAX_WITHDRAWALS_PER_PAYLOAD) { |
There was a problem hiding this comment.
This loop condition appears to have an off-by-one error. The check withdrawals.length + 1 === MAX_WITHDRAWALS_PER_PAYLOAD will cause the loop to break when withdrawals.length is MAX_WITHDRAWALS_PER_PAYLOAD - 1, resulting in one less withdrawal being processed than the maximum allowed.
For example, if MAX_WITHDRAWALS_PER_PAYLOAD is 16 and withdrawals.length becomes 15, this condition 15 + 1 === 16 will be true, and the loop will break, leaving only 15 withdrawals in the list instead of the maximum of 16.
The condition should probably be withdrawals.length >= MAX_WITHDRAWALS_PER_PAYLOAD to allow the list to be filled up to its maximum capacity. This is also consistent with the check used later in this function.
| if (withdrawal.withdrawableEpoch > epoch || withdrawals.length + 1 === MAX_WITHDRAWALS_PER_PAYLOAD) { | |
| if (withdrawal.withdrawableEpoch > epoch || withdrawals.length >= MAX_WITHDRAWALS_PER_PAYLOAD) { |
There was a problem hiding this comment.
that's same as the spec here
for withdrawal in state.builder_pending_withdrawals:
if (
withdrawal.withdrawable_epoch > epoch
or len(withdrawals) + 1 == MAX_WITHDRAWALS_PER_PAYLOAD
):
There was a problem hiding this comment.
I am surprised spec test didn't catch this bug
| * Merge is complete when the state includes execution layer data: | ||
| * state.latestExecutionPayloadHeader NOT EMPTY | ||
| */ | ||
| // TODO GLOAS: See if we need to update this for gloas |
There was a problem hiding this comment.
we should just remove the merge transition code, it's probably simpler than making it somehow work with gloas
| : stateGloas.builderPendingWithdrawals.getReadonly(i); | ||
|
|
||
| if (withdrawal.withdrawableEpoch > epoch || withdrawals.length === MAX_WITHDRAWALS_PER_PAYLOAD) { | ||
| if (withdrawal.withdrawableEpoch > epoch || withdrawals.length + 1 === MAX_WITHDRAWALS_PER_PAYLOAD) { |
There was a problem hiding this comment.
I am surprised spec test didn't catch this bug
| }); | ||
| withdrawalIndex++; | ||
| withdrawnBalances.set(withdrawal.builderIndex, totalWithdrawn + withdrawableBalance); | ||
| if (withdrawableBalance > 0) { |
There was a problem hiding this comment.
I am surprised spec test didn't catch this bug
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## nc/epbs-stf #8660 +/- ##
===============================================
+ Coverage 51.69% 51.71% +0.01%
===============================================
Files 855 855
Lines 66341 66300 -41
Branches 4809 4808 -1
===============================================
- Hits 34294 34284 -10
+ Misses 31979 31948 -31
Partials 68 68 🚀 New features to boost your workflow:
|
Yes, was thinking the same, it's concerning that we passed spec tests while deviating from the spec like this. There seems to be an effort in progress to improve coverage ethereum/consensus-specs#4759 |
2 participants
No description provided.