-
Notifications
You must be signed in to change notification settings - Fork 994
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
eip-7251: process consolidations #13983
Conversation
de6f522
to
22039e2
Compare
ee3ad6e
to
5f53644
Compare
de0c18f
to
fefdf12
Compare
This is failing minimal spectests. I was skipping all spectests for consolidations as I thought they were deleted in v1.5.0-alpha.2, but I've learned that only mainnet spectests were missing for consolidations. When unskipping minimal spectests, they are failing for the following reason:
I'll work on fixing this, but the rest of the PR is still ready for review. |
f0641a9
to
d5dd5e7
Compare
Fixed! Good bug catch by spectest, I was using the wrong fork version for computing the domain. |
} | ||
} | ||
|
||
return st, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably needs a broader design discussion and is outside of the scope of this PR, but what's the reason for returning the state here when the function doesn't make a copy of the passed in state? When I see a signature such as func F(state) state
, I expect that I should only use the returned state because it will differ from the passed in state when the function returns. On the contrary, SwitchToCompoundingValidator
doesn't return the state, so I expect the argument to be mutated inside the function.
It would be nice if we were consistent here.
P.S. The comment even says This method makes mutating calls to the beacon state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is even an argument that this method should be a receiver on the state.
if err := st.ProcessPendingConsolidations(ctx, pendingConsolidations); err != nil {
return fmt.Errorf("failed to process pending consolidations: %w", err)
}
I agree that this signature smells as is so I will change it as part of this PR such that it will not return the resulting state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing to note @rkapka, all of the other state transition functions return (state.BeaconState, error)
so these methods will be the exception actually. It's not a big deal, we can also refactor the other methods not to return the state since it mutates it in place.
6bab1fb
to
1f2f82f
Compare
Fix failing spectest //testing/spectest/minimal/electra/operations:go_default_test
1f2f82f
to
0711d91
Compare
Co-authored-by: Radosław Kapka <[email protected]>
Co-authored-by: Radosław Kapka <[email protected]>
0711d91
to
8141096
Compare
* eip-7251: process_pending_consolidations and process_consolidations * Consolidate unit tests + spectests Fix failing spectest //testing/spectest/minimal/electra/operations:go_default_test * Unskip consolidation processing for minimal spectests * PR feedback * Update beacon-chain/core/electra/consolidations_test.go Co-authored-by: Radosław Kapka <[email protected]> * Update beacon-chain/core/electra/consolidations_test.go Co-authored-by: Radosław Kapka <[email protected]> * Move consolidation limit check outside of the loop --------- Co-authored-by: Radosław Kapka <[email protected]>
* eip-7251: process_pending_consolidations and process_consolidations * Consolidate unit tests + spectests Fix failing spectest //testing/spectest/minimal/electra/operations:go_default_test * Unskip consolidation processing for minimal spectests * PR feedback * Update beacon-chain/core/electra/consolidations_test.go Co-authored-by: Radosław Kapka <[email protected]> * Update beacon-chain/core/electra/consolidations_test.go Co-authored-by: Radosław Kapka <[email protected]> * Move consolidation limit check outside of the loop --------- Co-authored-by: Radosław Kapka <[email protected]>
What type of PR is this?
Feature
What does this PR do? Why is it needed?
Extracted from #13903.
Which issues(s) does this PR fix?
Tracking @ #13849
Other notes for review
https:/ethereum/consensus-specs/blob/v1.5.0-alpha.2/specs/electra/beacon-chain.md#new-process_pending_consolidations
https:/ethereum/consensus-specs/blob/v1.5.0-alpha.2/specs/electra/beacon-chain.md#new-process_consolidation
Depends on these PRs:
Merge those before merging this PR.