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

added check for validator balance update against withdrawable epoch #230

Merged
merged 10 commits into from
Oct 10, 2023

Conversation

Sidu28
Copy link
Contributor

@Sidu28 Sidu28 commented Oct 9, 2023

From @gpsanant 👍
Possible bug in eigenpods:

ValidatorInfo memory validatorInfo = _validatorPubkeyHashToInfo[validatorPubkeyHash];

A validator withdraws fully but doesn't prove their withdrawal, a balance update can be proven to bring their balance to 0. You should ideally only be able to prove the full withdrawal once the validator is withdrawn.

This PR addresses this issue by adding a check that only allows balance updates from before withdrawable epoch is set: https://github.com/ethereum/consensus-specs/blob/dev/specs/capella/beacon-chain.md#is_fully_withdrawable_validator

@Sidu28
Copy link
Contributor Author

Sidu28 commented Oct 9, 2023

@gpsanant note that this changes the EP constructor

Comment on lines 75 to 78
32e9,
75e7
75e7,
1616508000
)
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like a good opportunity for named parameter syntax (having a few integer args in a row, could be easy to confuse these)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this script still in commission?

Copy link
Contributor

@ChaoticWalrus ChaoticWalrus left a comment

Choose a reason for hiding this comment

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

Seems technically correct, would appreciate just a little more polish prior to merging -- pointed out a couple nits I'd like to see addressed.

@gpsanant gpsanant self-requested a review October 10, 2023 17:58
Copy link
Contributor

@gpsanant gpsanant left a comment

Choose a reason for hiding this comment

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

comment above

Copy link
Collaborator

@wadealexc wadealexc left a comment

Choose a reason for hiding this comment

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

LGTM pending G and J feedback

@Sidu28 Sidu28 requested a review from gpsanant October 10, 2023 19:24
@Sidu28 Sidu28 requested a review from gpsanant October 10, 2023 20:17
Copy link
Contributor

@gpsanant gpsanant left a comment

Choose a reason for hiding this comment

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

lgtm

@Sidu28 Sidu28 merged commit c3d57b5 into eig-14-addressal Oct 10, 2023
7 of 9 checks passed
@Sidu28 Sidu28 deleted the verifybalanceupdatebugfix branch October 10, 2023 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants