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.

Sign up for GitHub

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

Merged
merged 32 commits into from
Mar 28, 2024
Merged

Add EIP-7251 spec: Increase MAX_EFFECTIVE_BALANCE #3618

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
668e447
Add EIP-7251 spec
dapplion Mar 8, 2024
c5af391
Add validator doc
dapplion Mar 8, 2024
fc65a6f
Fix CI
dapplion Mar 12, 2024
cf70df2
Address Comments and Cleanup Spec
ethDreamer Mar 19, 2024
f6359f9
Fix Bug in process_pending_balance_deposits
ethDreamer Mar 19, 2024
7c9fc19
Merge pull request #4 from ethDreamer/eip-7251
dapplion Mar 20, 2024
fe35d66
Remove built spec
mkalinin Mar 20, 2024
98f38c7
Introduce MAX_PARTIAL_WITHDRAWALS_PER_PAYLOAD
mkalinin Mar 20, 2024
cdbc2b7
Fix linter
mkalinin Mar 20, 2024
6d140cd
Fix MAX_PARTIAL_WITHDRAWALS_PER_PAYLOAD in mainnet.yaml
mkalinin Mar 20, 2024
b02c3e5
Check MAX_PARTIAL_WITHDRAWALS_PER_PAYLOAD < MAX_WITHDRAWALS_PER_PAYLOAD
mkalinin Mar 20, 2024
be79aab
Fix toc
mkalinin Mar 20, 2024
a127bbf
Merge branch 'dev' into eip-7251
mkalinin Mar 20, 2024
17d65ca
Create eip7251 config invariants test
mkalinin Mar 20, 2024
d48b5e0
Update whistleblower reward for eip7251
mkalinin Mar 20, 2024
8873d02
Fix linter
mkalinin Mar 20, 2024
45f98d6
Set MIN_SLASHING_PENALTY_QUOTIENT_EIP7251=4096
mkalinin Mar 21, 2024
ebdb513
queue_excess_active_balance
dapplion Mar 22, 2024
6d9ebe1
set_compounding_withdrawal_credentials
dapplion Mar 22, 2024
84a5ae9
rename to partial_withdrawals_count
dapplion Mar 22, 2024
72c4f04
@ensi321 review
dapplion Mar 22, 2024
08732e6
fix typo
dapplion Mar 22, 2024
4e7c82c
Remove is_aggregator changes
dapplion Mar 22, 2024
8d7d7a8
add tests
fradamt Mar 25, 2024
97966d8
small fixes
fradamt Mar 25, 2024
4775641
fix broken pending deposits tests and typo
fradamt Mar 25, 2024
83e617a
Merge pull request #5 from fradamt/eip-7251
dapplion Mar 26, 2024
23ad85e
Allow to switch to compounding validator on deposit
dapplion Mar 26, 2024
e6aaa9d
Fix lint
dapplion Mar 26, 2024
06104f2
Fix is_partially_withdrawable_validator
mkalinin Mar 26, 2024
5e32d44
Fix the sweep by enabling 0x02 creds
mkalinin Mar 27, 2024
73ede3a
Fix pending_balance_to_withdraw == 0 check when full exit
mkalinin Mar 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 63 additions & 10 deletions specs/_features/eip7251/beacon-chain.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
- [Helpers](#helpers)
- [Predicates](#predicates)
- [Updated `is_eligible_for_activation_queue`](#updated-is_eligible_for_activation_queue)
- [New `is_compounding_withdrawal_credential`](#new-is_compounding_withdrawal_credential)
- [New `has_compounding_withdrawal_credential`](#new-has_compounding_withdrawal_credential)
- [New `has_execution_withdrawal_credential`](#new-has_execution_withdrawal_credential)
- [Updated `is_fully_withdrawable_validator`](#updated-is_fully_withdrawable_validator)
Expand All @@ -45,6 +46,8 @@
- [Beacon state mutators](#beacon-state-mutators)
- [Updated `initiate_validator_exit`](#updated--initiate_validator_exit)
- [New `set_compounding_withdrawal_credentials`](#new-set_compounding_withdrawal_credentials)
- [New `switch_to_compounding_validator`](#new-switch_to_compounding_validator)
- [New `queue_excess_active_balance`](#new-queue_excess_active_balance)
- [New `compute_exit_epoch_and_update_churn`](#new-compute_exit_epoch_and_update_churn)
- [New `compute_consolidation_epoch_and_update_churn`](#new-compute_consolidation_epoch_and_update_churn)
- [Updated `slash_validator`](#updated-slash_validator)
Expand All @@ -62,7 +65,8 @@
- [Updated `process_operations`](#updated-process_operations)
- [Deposits](#deposits)
- [Updated `apply_deposit`](#updated--apply_deposit)
- [Modified `add_validator_to_registry`](#modified-add_validator_to_registry)
- [New `is_valid_deposit_signature`](#new-is_valid_deposit_signature)
- [Modified `add_validator_to_registry`](#modified-add_validator_to_registry)
- [Updated `get_validator_from_deposit`](#updated-get_validator_from_deposit)
- [Withdrawals](#withdrawals)
- [New `process_execution_layer_withdraw_request`](#new-process_execution_layer_withdraw_request)
Expand Down Expand Up @@ -292,14 +296,21 @@ def is_eligible_for_activation_queue(validator: Validator) -> bool:
)
```

#### New `is_compounding_withdrawal_credential`

```python
def is_compounding_withdrawal_credential(withdrawal_credentials: Bytes32) -> bool:
return withdrawal_credentials[:1] == COMPOUNDING_WITHDRAWAL_PREFIX
```

#### New `has_compounding_withdrawal_credential`

```python
def has_compounding_withdrawal_credential(validator: Validator) -> bool:
"""
Check if ``validator`` has an 0x02 prefixed "compounding" withdrawal credential.
"""
return validator.withdrawal_credentials[:1] == COMPOUNDING_WITHDRAWAL_PREFIX
return is_compounding_withdrawal_credential(validator.withdrawal_credentials)
```

#### New `has_execution_withdrawal_credential`
Expand Down Expand Up @@ -429,6 +440,29 @@ def set_compounding_withdrawal_credentials(state: BeaconState, index: ValidatorI
validator.withdrawal_credentials[:1] = COMPOUNDING_WITHDRAWAL_PREFIX
```

#### New `switch_to_compounding_validator`

```python
def switch_to_compounding_validator(state: BeaconState, index: ValidatorIndex) -> None:
validator = state.validators[index]
if has_eth1_withdrawal_credential(validator):
validator.withdrawal_credentials[:1] = COMPOUNDING_WITHDRAWAL_PREFIX
queue_excess_active_balance(state, index)
```

#### New `queue_excess_active_balance`

```python
def queue_excess_active_balance(state: BeaconState, index: ValidatorIndex) -> None:
balance = state.balances[index]
if balance > MAX_EFFECTIVE_BALANCE:
excess_balance = balance - MAX_EFFECTIVE_BALANCE
state.balances[index] = balance - excess_balance
Copy link
Contributor

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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?

Copy link
Contributor

@mkalinin mkalinin Mar 28, 2024 •
edited
Loading

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

Copy link
Contributor

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 👍

Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Member Author

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

Copy link
Contributor

@fradamt fradamt Mar 29, 2024 •
edited
Loading

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.

state.pending_balance_deposits.append(
PendingBalanceDeposit(index=index, amount=excess_balance)
)
```

#### New `compute_exit_epoch_and_update_churn`


Expand Down Expand Up @@ -584,11 +618,12 @@ def process_pending_consolidations(state: BeaconState) -> None:
if source_validator.withdrawable_epoch > get_current_epoch(state):
break

# Churn any target excess active balance of target and raise its max
switch_to_compounding_validator(state, pending_consolidation.target_index)
# Move active balance to target. Excess balance is withdrawable.
active_balance = get_active_balance(state, pending_consolidation.source_index)
decrease_balance(state, pending_consolidation.source_index, active_balance)
increase_balance(state, pending_consolidation.target_index, active_balance)
set_compounding_withdrawal_credentials(state, pending_consolidation.target_index)
next_pending_consolidation += 1

state.pending_consolidations = state.pending_consolidations[next_pending_consolidation:]
Expand Down Expand Up @@ -752,22 +787,40 @@ def apply_deposit(state: BeaconState,
validator_pubkeys = [v.pubkey for v in state.validators]
if pubkey not in validator_pubkeys:
# Verify the deposit signature (proof of possession) which is not checked by the deposit contract
if is_valid_deposit_signature(pubkey, withdrawal_credentials, amount, signature):
add_validator_to_registry(state, pubkey, withdrawal_credentials, amount)
else:
# Increase balance by deposit amount
index = ValidatorIndex(validator_pubkeys.index(pubkey))
state.pending_balance_deposits.append(PendingBalanceDeposit(index=index, amount=amount)) # [Modified in EIP-7251]
# Check if valid deposit switch to compounding credentials
if (
is_compounding_withdrawal_credential(withdrawal_credentials)
and has_eth1_withdrawal_credential(state.validators[index])
and is_valid_deposit_signature(pubkey, withdrawal_credentials, amount, signature)
):
switch_to_compounding_validator(state, index)

```

###### New `is_valid_deposit_signature`

```python
def is_valid_deposit_signature(pubkey: BLSPubkey,
withdrawal_credentials: Bytes32,
amount: uint64,
signature: BLSSignature) -> None:
deposit_message = DepositMessage(
pubkey=pubkey,
withdrawal_credentials=withdrawal_credentials,
amount=amount,
)
domain = compute_domain(DOMAIN_DEPOSIT) # Fork-agnostic domain since deposits are valid across forks
signing_root = compute_signing_root(deposit_message, domain)
if bls.Verify(pubkey, signing_root, signature):
add_validator_to_registry(state, pubkey, withdrawal_credentials, amount)
else:
# Increase balance by deposit amount
index = ValidatorIndex(validator_pubkeys.index(pubkey))
state.pending_balance_deposits.append(PendingBalanceDeposit(index=index, amount=amount)) # [Modified in EIP-7251]
return bls.Verify(pubkey, signing_root, signature)
```

#### Modified `add_validator_to_registry`
###### Modified `add_validator_to_registry`

```python
def add_validator_to_registry(state: BeaconState,
Expand Down
12 changes: 3 additions & 9 deletions specs/_features/eip7251/fork.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,16 +130,10 @@ def upgrade_to_eip7251(pre: deneb.BeaconState) -> BeaconState:

# Ensure early adopters of compounding credentials go through the activation churn
queue_excess_active_balance(post)
for index, validator in enumerate(state.validators):
if has_compounding_withdrawal_credential(validator):
queue_excess_active_balance(state, index)

return post
```

```python
def queue_excess_active_balance(state: BeaconState):
for index, validator in enumerate(state.validators):
balance = state.balances[index]
if has_compounding_withdrawal_credential(validator) and balance > MAX_EFFECTIVE_BALANCE:
excess_balance = balance - MAX_EFFECTIVE_BALANCE
state.balances[index] = balance - excess_balance
state.pending_balance_deposits.append(PendingBalanceDeposit(index=index, amount=excess_balance))
```
Loading