-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Add EIP: Increase the MAX_EFFECTIVE_BALANCE #7251
Conversation
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.
Left a few quick comments but haven't done a deep review of the granular changes
EIPS/eip-increase-maxeb.md
Outdated
***Consensus layer changes*** | ||
|
||
See https://github.com/michaelneuder/consensus-specs/pull/3/files for a diff | ||
view of the CL spec changes. Changes are also copied below for convenience. |
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.
I'd suggest following some of the format we've recently used where changes are just discussed here at a high level and linked out to the actual code snippets. Otherwise, the double-maintenance will not be fun
See this section in eip 7045 for an example
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.
great :-) that was annoying to put together so im happy we don't have to maintain it lol.
EIPS/eip-increase-maxeb.md
Outdated
|
||
### Features | ||
|
||
1. **`0x02` withdrawal credential signals compounding.** These validators will be excluded from the partial withdrawals sweep and their balance compounds. |
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.
Do you think 0x02 is still needed if you have partial withdrawals from triggerable from EL (e.g. an extended 7002)?
It seems to me that such a functionality would potentially preclude the need for a new credential
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.
Ah, I see this is still "in discussion" below even though 7002 is noted as a dependency above. I would note it as a point of discussion in that line item
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.
done!
EIPS/eip-increase-maxeb.md
Outdated
@@ -0,0 +1,95 @@ | |||
--- |
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.
--- | |
--- | |
eip: 7251 |
Issuing EIP number using PR number. Please also update the filename.
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.
done!
EIPS/eip-increase-maxeb.md
Outdated
12. Modify `is_aggregator` to be weight-based. | ||
13. Modify `compute_weak_subjectivity_period` to use new churn limit function. | ||
14. Add `has_compounding_withdrawal_credential` to check for `0x02` credential. | ||
15. Modify `is_fully_withdrawalable_validator` to check for compounding credentials. |
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.
15. Modify `is_fully_withdrawalable_validator` to check for compounding credentials. | |
15. Modify `is_fully_withdrawable_validator` to check for compounding credentials. |
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.
done!
✅ All reviewers have approved. |
The commit 81849e7 (as a parent of 18ac29b) contains errors. |
17. Modify `is_partially_withdrawable_validator` to check for excess balance. | ||
18. Modify `get_expected_withdrawals` to use excess balance. | ||
|
||
|
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.
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.
All Reviewers Have Approved; Performing Automatic Merge...
EIP PR for https://ethereum-magicians.org/t/eip-7251-increase-the-max-effective-balance/15982
See https://gist.github.com/michaelneuder/cafabcfcfcccc45e44ab9d6b1c7b4e1d for related documents