Skip to content
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: misc changes #3636

Merged
merged 12 commits into from
Apr 6, 2024
Merged

EIP-7251: misc changes #3636

merged 12 commits into from
Apr 6, 2024

Conversation

mkalinin
Copy link
Collaborator

@mkalinin mkalinin commented Mar 29, 2024

Summary of changes:

  • Remove set_compounding_withdrawal_credentials as it is unused
  • Add FULL_EXIT_REQUEST_AMOUNT which is set to 0
  • Add validator.effective_balance == MIN_ACTIVATION_BALANCE check to partial withdrawals
    This check is borrowed from the sweep checks. Although, satisfying this condition with partial withdrawal is not as critical as with the sweep, re-gaining the min activation balance sooner should be aligned with interest of stakers and network
  • Slight refactor of process_execution_layer_withdraw_request
  • Stricter comparison of withdrawal addresses on consolidation: use last 20 bytes instead of last 31 bytes of creds
  • Use validator.effective_balance for consolidation churn to align its logic with the exit churn and with the churn limit computation which is EB based
  • Abort voluntary exit processing if validator has pending partial withdrawals to align its processing with the process_execution_layer_withdraw_request flow

@mkalinin mkalinin requested a review from dapplion April 1, 2024 08:21
@ralexstokes
Copy link
Member

I haven't gone through the changes, but the summary sounds good from a high-level

Copy link
Contributor

@fradamt fradamt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!


return

has_sufficient_effective_balance = validator.effective_balance == MIN_ACTIVATION_BALANCE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
has_sufficient_effective_balance = validator.effective_balance == MIN_ACTIVATION_BALANCE
has_sufficient_effective_balance = validator.effective_balance >= MIN_ACTIVATION_BALANCE

specs/_features/eip7251/beacon-chain.md Outdated Show resolved Hide resolved
Co-authored-by: fradamt <104826920+fradamt@users.noreply.github.com>
@hwwhww hwwhww added the Electra label Apr 4, 2024
Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! I lean towards merging (and ill make an issue for one comment l left, either I don't understand how the new partial withdrawals work or there is a bug...) and then ill make a testing pass in #3648

specs/_features/eip7251/beacon-chain.md Show resolved Hide resolved
@ralexstokes ralexstokes merged commit b18bbd8 into ethereum:dev Apr 6, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants