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

eip7251: Add missed exit checks to consolidation processing #4000

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

mkalinin
Copy link
Collaborator

A couple of checks that the protocol does when processing a voluntary exit was not included into consolidation request processing, this PR fixes that, those checks are:

  • assert current_epoch >= source_validator.activation_epoch + SHARD_COMMITTEE_PERIOD
  • assert get_pending_balance_to_withdraw(state, source_index) == 0

Since the exit is the first step of consolidation process, it makes sense to preserve all invariants that we have for voluntary exit conditions. cc @fradamt

Copy link
Member

@ppopth ppopth left a comment

Choose a reason for hiding this comment

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

This makes sense to me.

Copy link
Member

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

Great catch. Thank you! I see no issues.

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; nice catch!

Copy link
Contributor

@rolfyone rolfyone left a comment

Choose a reason for hiding this comment

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

LGTM

@mkalinin
Copy link
Collaborator Author

mkalinin commented Nov 1, 2024

A side note, the cause of this issue makes me think that we need one function that checks all exit conditions to be used in already three places: voluntary_exit, withdrawal_request and consolidation_request processing. This can be done in a separate PR

@fradamt
Copy link
Contributor

fradamt commented Nov 1, 2024

lgtm :)
Also agree about separately maybe refactoring with all the exit checks happening in a separate function

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.

7 participants