-
Notifications
You must be signed in to change notification settings - Fork 1k
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_registry_updates #14005
Conversation
74e3000
to
09c8c9a
Compare
Working on fixing this up. Just needs more unit tests (spectests are passing) |
c4eda0b
to
8fa05b7
Compare
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.
lgtm
currentEpoch := time.CurrentEpoch(state) | ||
ejectionBal := params.BeaconConfig().EjectionBalance | ||
activationEpoch := helpers.ActivationExitEpoch(currentEpoch) | ||
vals := state.Validators() |
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.
state.Validators()
copies the entire validator set and this is very expensive given the current validator set size.
Have you thought about using ValidatorsReadOnly
or ReadFromEveryValidator
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.
Good point. I'll do something about this
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 is done now. Using the same design as #14130
Design in #14130 was approved. I'll update this PR today to adopt that design. |
8a1b0e2
to
0a1500b
Compare
0a1500b
to
59cbc83
Compare
Co-authored-by: Sammy Rosso <[email protected]>
59cbc83
to
69873db
Compare
What type of PR is this?
Feature
What does this PR do? Why is it needed?
Required for Electra.
Extracted from #13903.
Which issues(s) does this PR fix?
Tracking @ #13849
Other notes for review