-
Notifications
You must be signed in to change notification settings - Fork 989
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: Enforce Activation Rate Limit at Fork Transition #3659
EIP-7251: Enforce Activation Rate Limit at Fork Transition #3659
Conversation
specs/_features/eip7251/fork.md
Outdated
below_minimum.append(index) | ||
|
||
activation_queue.sort(key=lambda index: (post.validators[index].activation_eligibility_epoch, index)) | ||
for index in activation_queue + pre_activation_queue + below_minimum: |
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.
IIUC, the distinction between different states of not yet active validator affects its position in the queue. What if we simplify to:
for index, validator in enumerate(post.validators):
if validator.activation_epoch == FAR_FUTURE_EPOCH:
activation_queue.append(index)
activation_queue.sort(key=lambda index: (post.validators[index].activation_eligibility_epoch, index))
The above code respects the activation_eligibility_epoch
if it was set and puts those validators where it is not yet set to the end of the activation queue. If it is desirable we could add two-dimension sorting (by effective_balance desc) but I don’t see a big value in it tbh.
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 suggestion! I've updated the code. I also removed the secondary sorting condition. My original intention was to order the validators by:
- validators with
activation_eligibility_epoch
already set - validators where
activation_eligibility_epoch
could be set # this is the second group - validators with
activation_eligibility_epoch
unset
but since process_epoch()
runs before this, the second group is absorbed into the first group anyway so we don't need that sorting condition.
I've just added a new adjustment. If we assume at the time of the fork that there are many pending validators in the activation queue with I've updated the logic to appropriately set the
activation_churn_limit = get_validator_activation_churn_limit(post)
adjusted_validators = 0
adjusted_epoch = get_current_epoch(post) - 1
for i, index in enumerate(pre_activation):
if i >= activation_churn_limit * 4
break
if post.validators[index].activation_eligibility_epoch > post.finalized_checkpoint.epoch
break
post.validators[index].activation_eligibility_epoch =
adjusted_epoch + (i // activation_churn_limit)
adjusted_validators += 1
for index in pre_activation[adjusted_validators:]:
queue_entire_balance_and_reset_validator(post, ValidatorIndex(index)) |
Okay I've updated this PR to just move the validators into the activation queue. I'll put the reclaiming of lost epochs into a future PR. |
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!
We no longer rate-limit incoming validators inside
process_registry_updates()
. Instead, we impose rate limiting inprocess_pending_balance_deposits()
. But this means no queuing is applied to validators that have had deposits processed before the fork but are not yet active. This PR attempts to address these validators by skimming off their balance and moving them to the front of thepending_balance_deposits
queue.The proposed fix is simple but it is a bit ugly. We lose a couple epochs worth of activations at the fork transition & some validators
activation_eligibility_epoch
is reset if they end up in this no-mans land during the fork transition. But this is a minimum viable solution anyway.