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

Fix/hysteresis on active validator #3045

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

leckylao
Copy link

@leckylao leckylao commented Oct 17, 2022

Skipping hysteresis on pending validator

@ethereum ethereum deleted a comment from Where-2 Oct 17, 2022
@ethereum ethereum deleted a comment from Where-2 Oct 17, 2022
@ethereum ethereum deleted a comment from Where-2 Oct 17, 2022
@ethereum ethereum deleted a comment from Where-2 Oct 17, 2022
@ethereum ethereum deleted a comment from Where-2 Oct 17, 2022
@ethereum ethereum deleted a comment from Where-2 Oct 17, 2022
@leckylao leckylao marked this pull request as draft October 17, 2022 21:38
@leckylao leckylao marked this pull request as ready for review October 18, 2022 07:45
@djrtwo
Copy link
Contributor

djrtwo commented Oct 18, 2022

While this is an edgecase affecting a desired functionality for your application, I think it unlikely that this is altered (especially soon) given (1) the functionality of 1 ETH deposits was designed for "top-ups" rather than iterative deposits and (2) that the desired functionality can be achieved trivially through the use of a smart contract on the execution layer.

Given (2) and the design goal of simplicity in CL, my personal preference is to leave as is and to put a warning in the validator guide.

Note: This cannot just be changed in Phase 0 (as specified). It might be changed in a subsequent spec and hard fork. If this is to be entertained for a future hard fork, I suggest putting this in it's own spec folder (e.g. specs/hystereris-fix) as a diff to bellatrix (the latest stable fork)

- duplicating epoch_processing test to make sure things still work
@leckylao leckylao marked this pull request as draft October 21, 2022 01:49
@leckylao leckylao marked this pull request as ready for review October 21, 2022 02:03
@leckylao
Copy link
Author

leckylao commented Oct 21, 2022

While this is an edgecase affecting a desired functionality for your application, I think it unlikely that this is altered (especially soon) given (1) the functionality of 1 ETH deposits was designed for "top-ups" rather than iterative deposits and (2) that the desired functionality can be achieved trivially through the use of a smart contract on the execution layer.

It would be easier for a new protocol for the change and it is a big impact for us. But agreed and we have the same expectation and working on the infrastructure upgrade to fix it.

Given (2) and the design goal of simplicity in CL, my personal preference is to leave as is and to put a warning in the validator guide.

Agreed. Definitely good to put warning to prevent this from happening again. Could also put the stuck validators as example that there's case would need 33 ETH to activate a validator.

Note: This cannot just be changed in Phase 0 (as specified). It might be changed in a subsequent spec and hard fork. If this is to be entertained for a future hard fork, I suggest putting this in it's own spec folder (e.g. specs/hystereris-fix) as a diff to bellatrix (the latest stable fork)

Right, made sense. I have moved that into a separate folder as suggested. Thank you for the feedback.

@leckylao
Copy link
Author

Please note it was added in phrase 0 with all tests pass before moving into sub folder.
https://github.com/ethereum/consensus-specs/pull/3045/files

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.

2 participants