-
Notifications
You must be signed in to change notification settings - Fork 983
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-7251 spec: Increase MAX_EFFECTIVE_BALANCE #3618
Conversation
Co-authored-by: fradamt <104826920+fradamt@users.noreply.github.com>
Address Comments and Cleanup Spec
|
if source_validator.withdrawable_epoch > get_current_epoch(state): | ||
break | ||
|
||
next_pending_consolidation += 1 |
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.
Nit: Only increment next_pending_consolidation
after increase and decrease the balances so that it's more aligned with how next_deposit_index
is incremented in process_pending_balance_deposits()
##### Updated `get_expected_withdrawals` | ||
|
||
```python | ||
def get_expected_withdrawals(state: BeaconState) -> Tuple[Sequence[Withdrawal], uint64]: |
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.
Probably want to tag here too since function signature change is important
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.
what do you mean with "tag"?
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.
what do you mean with "tag"?
I mean like adding # [Modified in EIP7251]
after the function signature to signal the return type has changed
@potuz I've added commit ebdb513 with @ethDreamer I've added 6d9ebe1 to switch the target consolidator into compounding credentials if the consolidation is sucessful |
|
IIRC @fradamt has a branch with new EIP-7251 test cases: michaelneuder#20 Can we add them to this PR now? |
How does the interplay with other withdrawal types go? |
add tests, fix small issues
No validator is automatically changed into consolidating credentials. The current paths to switch are:
|
@hwwhww can we merge this as-is so contributors can PR fixes on |
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.
Outstanding work everyone! I think it’s a time to merge this PR and continue polishing and fixing the logic in separate PRs as it becomes inconvenient to introduce new changes to the feature
balance = state.balances[index] | ||
if balance > MAX_EFFECTIVE_BALANCE: | ||
excess_balance = balance - MAX_EFFECTIVE_BALANCE | ||
state.balances[index] = balance - 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.
feels clearer to just say:
state.balances[index] = MAX_EFFECTIVE_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.
state.balances[index] = balance - excess_balance | |
state.balances[index] = MAX_EFFECTIVE_BALANCE |
It feels confusing to use both MAX_EFFECTIVE_BALANCE
and MAX_EFFECTIVE_BALANCE_EIP7251
in the same file. What do you think about adding a duplicate MAX_EFFECTIVE_BALANCE_PRE_EIP7251 := 32
?
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 agree with that, maybe MAX_EFFECTIVE_BALANCE_PHASE0
? The other question: if we have this variable explicitly defined, can it replace MIN_ACTIVATION_BALANCE
? Semantically these are two different things but practically having a single parameter slightly increases readability. So, I am slightly in favour of replacing MIN_ACTIVATION_BALANCE
with MAX_EFFECTIVE_BALANCE_PHASE0
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.
oh yeah using MIN_ACTIVATION_BALANCE
looks better to me 👍
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.
So you suggest the opposite, to replace MAX_EFFECTIVE_BALANCE
with MIN_ACTIVATION_BALANCE
everywhere in the spec? I am fine with either, as long as we use the same param name in all places
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.
If the two options are replacing MIN_ACTIVATION_BALANCE
with MAX_EFFECTIVE_BALANCE_PHASE0
or replacing MAX_EFFECTIVE_BALANCE
with MIN_ACTIVATION_BALANCE
, I would prefer the latter, because MIN_ACTIVATION_BALANCE
accurately represents what this parameter is in the current system
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.
because
MIN_ACTIVATION_BALANCE
accurately represents what this parameter is in the current system
Not in every case, it is also a max effective balance for 0x01 creds. And when we want to get MAXEB of a particular validator it makes more sense to use MAX_EFFECTIVE_BALANCE_PHASE) and *_EIP7251
. But again, I am fine with either, just don’t want to have MAX_EB
, MIN_AB
and MAX_EB_EIP7251
in the spec as this is really confusing.
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.
For bumped constants in the past we have used:
INACTIVITY_PENALTY_QUOTIENT
INACTIVITY_PENALTY_QUOTIENT_ALTAIR
INACTIVITY_PENALTY_QUOTIENT_BELLATRIX
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.
For bumped constants in the past we have used:
INACTIVITY_PENALTY_QUOTIENT INACTIVITY_PENALTY_QUOTIENT_ALTAIR INACTIVITY_PENALTY_QUOTIENT_BELLATRIX
Given that MAX_EFFECTIVE_BALANCE
(the phase 0 one) is a constant that we are still using, as opposed to INACTIVITY_PENALTY_QUOTIENT
which was fully replaced, I think it makes sense to not follow this convention. I think following it would give the impression that MAX_EFFECTIVE_BALANCE_ELECTRA
(or whatever it ends up being) replaces MAX_EFFECTIVE_BALANCE
fully.
For this reason, I'd still lean towards using MIN_ACTIVATION_BALANCE
only. While it is true that it is also used for the max effective balance of 0x00 and 0x01 credentials, this is similar to what happened in phase0 already, i.e., MAX_EFFECTIVE_BALANCE
was used as the min activation balance as well.
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 haven't reviewed it carefully, but I am going to merge it to accelerate the later iterations.
Consensus implementation for EIP-7251: Increase MAX_EFFECTIVE_BALANCE
Extensive discussion and rationale can be found in this PR:
MAX_EFFECTIVE_BALANCE
minimal spec change michaelneuder/consensus-specs#3Accompanying documentation: