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

Switch the rest of the spec to MAX_EFFECTIVE_BALANCE_ELECTRA #3783

Merged
merged 6 commits into from
Jun 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
41 changes: 37 additions & 4 deletions specs/electra/beacon-chain.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
- [New `get_active_balance`](#new-get_active_balance)
- [New `get_pending_balance_to_withdraw`](#new-get_pending_balance_to_withdraw)
- [Modified `get_attesting_indices`](#modified-get_attesting_indices)
- [Modified `get_next_sync_committee_indices`](#modified-get_next_sync_committee_indices)
- [Beacon state mutators](#beacon-state-mutators)
- [Updated `initiate_validator_exit`](#updated--initiate_validator_exit)
- [New `switch_to_compounding_validator`](#new-switch_to_compounding_validator)
Expand Down Expand Up @@ -127,8 +128,6 @@ The following values are (non-configurable) constants used throughout the specif

| Name | Value |
| - | - |
| `BLS_WITHDRAWAL_PREFIX` | `Bytes1('0x00')` |
| `ETH1_ADDRESS_WITHDRAWAL_PREFIX` | `Bytes1('0x01')` |
| `COMPOUNDING_WITHDRAWAL_PREFIX` | `Bytes1('0x02')` |

### Domains
Expand Down Expand Up @@ -441,6 +440,8 @@ class BeaconState(Container):

#### Updated `compute_proposer_index`

*Note*: The function is modified to use `MAX_EFFECTIVE_BALANCE_ELECTRA` preset.

```python
def compute_proposer_index(state: BeaconState, indices: Sequence[ValidatorIndex], seed: Bytes32) -> ValidatorIndex:
"""
Expand Down Expand Up @@ -624,6 +625,36 @@ def get_attesting_indices(state: BeaconState, attestation: Attestation) -> Set[V
return output
```

#### Modified `get_next_sync_committee_indices`

*Note*: The function is modified to use `MAX_EFFECTIVE_BALANCE_ELECTRA` preset.

```python
def get_next_sync_committee_indices(state: BeaconState) -> Sequence[ValidatorIndex]:
"""
Return the sync committee indices, with possible duplicates, for the next sync committee.
"""
epoch = Epoch(get_current_epoch(state) + 1)

MAX_RANDOM_BYTE = 2**8 - 1
active_validator_indices = get_active_validator_indices(state, epoch)
active_validator_count = uint64(len(active_validator_indices))
seed = get_seed(state, epoch, DOMAIN_SYNC_COMMITTEE)
i = 0
sync_committee_indices: List[ValidatorIndex] = []
while len(sync_committee_indices) < SYNC_COMMITTEE_SIZE:
shuffled_index = compute_shuffled_index(uint64(i % active_validator_count), active_validator_count, seed)
candidate_index = active_validator_indices[shuffled_index]
random_byte = hash(seed + uint_to_bytes(uint64(i // 32)))[i % 32]
effective_balance = state.validators[candidate_index].effective_balance
# [Modified in Electra:EIP7251]
if effective_balance * MAX_RANDOM_BYTE >= MAX_EFFECTIVE_BALANCE_ELECTRA * random_byte:
sync_committee_indices.append(candidate_index)
i += 1
return sync_committee_indices
```


### Beacon state mutators

#### Updated `initiate_validator_exit`
Expand Down Expand Up @@ -1409,8 +1440,10 @@ def initialize_beacon_state_from_eth1(eth1_block_hash: Hash32,
# Process activations
for index, validator in enumerate(state.validators):
balance = state.balances[index]
validator.effective_balance = min(balance - balance % EFFECTIVE_BALANCE_INCREMENT, MAX_EFFECTIVE_BALANCE)
if validator.effective_balance == MAX_EFFECTIVE_BALANCE:
# [Modified in Electra:EIP7251]
validator.effective_balance = min(
mkalinin marked this conversation as resolved.
Show resolved Hide resolved
balance - balance % EFFECTIVE_BALANCE_INCREMENT, get_validator_max_effective_balance(validator))
if validator.effective_balance >= MIN_ACTIVATION_BALANCE:
validator.activation_eligibility_epoch = GENESIS_EPOCH
validator.activation_epoch = GENESIS_EPOCH

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@
with_presets,
spec_state_test,
always_bls,
single_phase,
with_custom_state,
spec_test,
default_balances_electra,
default_activation_threshold,
)


Expand Down Expand Up @@ -143,7 +148,9 @@ def is_duplicate_sync_committee(committee_indices):

@with_altair_and_later
@with_presets([MINIMAL], reason="to create nonduplicate committee")
@spec_state_test
@spec_test
@with_custom_state(balances_fn=default_balances_electra, threshold_fn=default_activation_threshold)
@single_phase
def test_sync_committee_rewards_nonduplicate_committee(spec, state):
committee_indices = compute_committee_indices(state)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
with_custom_state,
with_presets,
spec_test,
default_balances_electra,
misc_balances_electra,
)


Expand Down Expand Up @@ -132,7 +134,9 @@ def test_random_with_exits_with_duplicates(spec, state):

@with_altair_and_later
@with_presets([MINIMAL], reason="to create nonduplicate committee")
@spec_state_test
@spec_test
@with_custom_state(balances_fn=default_balances_electra, threshold_fn=default_activation_threshold)
@single_phase
def test_random_only_one_participant_without_duplicates(spec, state):
rng = random.Random(501)
yield from _test_harness_for_randomized_test_case(
Expand All @@ -144,7 +148,9 @@ def test_random_only_one_participant_without_duplicates(spec, state):

@with_altair_and_later
@with_presets([MINIMAL], reason="to create nonduplicate committee")
@spec_state_test
@spec_test
@with_custom_state(balances_fn=default_balances_electra, threshold_fn=default_activation_threshold)
@single_phase
def test_random_low_participation_without_duplicates(spec, state):
rng = random.Random(601)
yield from _test_harness_for_randomized_test_case(
Expand All @@ -156,7 +162,9 @@ def test_random_low_participation_without_duplicates(spec, state):

@with_altair_and_later
@with_presets([MINIMAL], reason="to create nonduplicate committee")
@spec_state_test
@spec_test
@with_custom_state(balances_fn=default_balances_electra, threshold_fn=default_activation_threshold)
@single_phase
def test_random_high_participation_without_duplicates(spec, state):
rng = random.Random(701)
yield from _test_harness_for_randomized_test_case(
Expand All @@ -168,7 +176,9 @@ def test_random_high_participation_without_duplicates(spec, state):

@with_altair_and_later
@with_presets([MINIMAL], reason="to create nonduplicate committee")
@spec_state_test
@spec_test
@with_custom_state(balances_fn=default_balances_electra, threshold_fn=default_activation_threshold)
@single_phase
def test_random_all_but_one_participating_without_duplicates(spec, state):
rng = random.Random(801)
yield from _test_harness_for_randomized_test_case(
Expand All @@ -181,7 +191,7 @@ def test_random_all_but_one_participating_without_duplicates(spec, state):
@with_altair_and_later
@with_presets([MINIMAL], reason="to create nonduplicate committee")
@spec_test
@with_custom_state(balances_fn=misc_balances, threshold_fn=default_activation_threshold)
@with_custom_state(balances_fn=misc_balances_electra, threshold_fn=default_activation_threshold)
@single_phase
def test_random_misc_balances_and_half_participation_without_duplicates(spec, state):
rng = random.Random(1501)
Expand All @@ -194,7 +204,8 @@ def test_random_misc_balances_and_half_participation_without_duplicates(spec, st

@with_altair_and_later
@with_presets([MINIMAL], reason="to create nonduplicate committee")
@spec_state_test
@spec_test
@with_custom_state(balances_fn=default_balances_electra, threshold_fn=default_activation_threshold)
@single_phase
def test_random_with_exits_without_duplicates(spec, state):
rng = random.Random(1502)
Expand Down
34 changes: 32 additions & 2 deletions tests/core/pyspec/eth2spec/test/context.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
ALLOWED_TEST_RUNNER_FORKS,
LIGHT_CLIENT_TESTING_FORKS,
)
from .helpers.forks import is_post_fork
from .helpers.forks import is_post_fork, is_post_electra
from .helpers.genesis import create_genesis_state
from .helpers.typing import (
Spec,
Expand Down Expand Up @@ -86,7 +86,10 @@ def default_activation_threshold(spec: Spec):
Helper method to use the default balance activation threshold for state creation for tests.
Usage: `@with_custom_state(threshold_fn=default_activation_threshold, ...)`
"""
return spec.MAX_EFFECTIVE_BALANCE
if is_post_electra(spec):
return spec.MIN_ACTIVATION_BALANCE
else:
return spec.MAX_EFFECTIVE_BALANCE


def zero_activation_threshold(spec: Spec):
Expand All @@ -106,6 +109,18 @@ def default_balances(spec: Spec):
return [spec.MAX_EFFECTIVE_BALANCE] * num_validators


def default_balances_electra(spec: Spec):
"""
Helper method to create a series of default balances for Electra.
Usage: `@with_custom_state(balances_fn=default_balances_electra, ...)`
"""
if not is_post_electra(spec):
return default_balances(spec)
Comment on lines +117 to +118
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto. Is it possible that instead of adding new default_balances_electra, we modify default_balances by adding new if is_post_electra(spec) logic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking about this approach but then decided to keep default balances as is because it makes a test run with 32 ETH balance validators which is also important for Electra. Ideally, we should run tests with MIN_ACTIVATION_BALANCE, MAX_EFFECTIVE_BALANCE_ELECTRA and something in between. It is probably better to handle this modification separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably more just the naming problem; when I read default_balances_electra, I thought it is just "the default balances post-electra," but it is actually compatible with pre-electra. But I understand what you wanted to present.

Copy link
Contributor

Choose a reason for hiding this comment

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

what about calling it max_balances instead? To me it explains pretty clearly what it does


num_validators = spec.SLOTS_PER_EPOCH * 8
return [spec.MAX_EFFECTIVE_BALANCE_ELECTRA] * num_validators


def scaled_churn_balances_min_churn_limit(spec: Spec):
"""
Helper method to create enough validators to scale the churn limit.
Expand Down Expand Up @@ -175,6 +190,21 @@ def misc_balances(spec: Spec):
return balances


def misc_balances_electra(spec: Spec):
"""
Helper method to create a series of balances that includes some misc. balances for Electra.
Usage: `@with_custom_state(balances_fn=misc_balances, ...)`
"""
if not is_post_electra(spec):
return misc_balances(spec)
Comment on lines +198 to +199
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible that, instead of adding new misc_balances_electra, we modify misc_balances by adding new if is_post_electra(spec) logic?


num_validators = spec.SLOTS_PER_EPOCH * 8
balances = [spec.MAX_EFFECTIVE_BALANCE_ELECTRA * 2 * i // num_validators for i in range(num_validators)]
rng = Random(1234)
rng.shuffle(balances)
return balances


def misc_balances_in_default_range_with_many_validators(spec: Spec):
"""
Helper method to create a series of balances that includes some misc. balances but
Expand Down
21 changes: 18 additions & 3 deletions tests/core/pyspec/eth2spec/test/helpers/genesis.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,31 @@
def build_mock_validator(spec, i: int, balance: int):
active_pubkey = pubkeys[i]
withdrawal_pubkey = pubkeys[-1 - i]
# insecurely use pubkey as withdrawal key as well
withdrawal_credentials = spec.BLS_WITHDRAWAL_PREFIX + spec.hash(withdrawal_pubkey)[1:]
if is_post_electra(spec):
if balance > spec.MIN_ACTIVATION_BALANCE:
# use compounding withdrawal credentials if the balance is higher than MIN_ACTIVATION_BALANCE
withdrawal_credentials = (
spec.COMPOUNDING_WITHDRAWAL_PREFIX
+ b'\x00' * 11
+ spec.hash(withdrawal_pubkey)[12:]
)
else:
# insecurely use pubkey as withdrawal key as well
withdrawal_credentials = spec.BLS_WITHDRAWAL_PREFIX + spec.hash(withdrawal_pubkey)[1:]
max_effective_balace = spec.MAX_EFFECTIVE_BALANCE_ELECTRA
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
max_effective_balace = spec.MAX_EFFECTIVE_BALANCE_ELECTRA
max_effective_balance = spec.MAX_EFFECTIVE_BALANCE_ELECTRA

Copy link
Contributor

Choose a reason for hiding this comment

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

ooops. will fix it after the release.

Thanks for the review!

else:
# insecurely use pubkey as withdrawal key as well
withdrawal_credentials = spec.BLS_WITHDRAWAL_PREFIX + spec.hash(withdrawal_pubkey)[1:]
max_effective_balace = spec.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
max_effective_balace = spec.MAX_EFFECTIVE_BALANCE
max_effective_balance = spec.MAX_EFFECTIVE_BALANCE


validator = spec.Validator(
pubkey=active_pubkey,
withdrawal_credentials=withdrawal_credentials,
activation_eligibility_epoch=spec.FAR_FUTURE_EPOCH,
activation_epoch=spec.FAR_FUTURE_EPOCH,
exit_epoch=spec.FAR_FUTURE_EPOCH,
withdrawable_epoch=spec.FAR_FUTURE_EPOCH,
effective_balance=min(balance - balance % spec.EFFECTIVE_BALANCE_INCREMENT, spec.MAX_EFFECTIVE_BALANCE)
effective_balance=min(balance - balance % spec.EFFECTIVE_BALANCE_INCREMENT, max_effective_balace)
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
effective_balance=min(balance - balance % spec.EFFECTIVE_BALANCE_INCREMENT, max_effective_balace)
effective_balance=min(balance - balance % spec.EFFECTIVE_BALANCE_INCREMENT, max_effective_balance)

)

return validator
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
)
from eth2spec.test.helpers.forks import (
is_post_altair,
is_post_electra,
)


Expand Down Expand Up @@ -69,9 +70,14 @@ def test_initialize_beacon_state_some_small_balances(spec):
if is_post_altair(spec):
yield 'description', 'meta', get_post_altair_description(spec)

if is_post_electra(spec):
max_effective_balance = spec.MAX_EFFECTIVE_BALANCE_ELECTRA
else:
max_effective_balance = spec.MAX_EFFECTIVE_BALANCE

main_deposit_count = spec.config.MIN_GENESIS_ACTIVE_VALIDATOR_COUNT
main_deposits, _, deposit_data_list = prepare_full_genesis_deposits(
spec, spec.MAX_EFFECTIVE_BALANCE,
spec, max_effective_balance,
deposit_count=main_deposit_count, signed=True,
)
# For deposits above, and for another deposit_count, add a balance of EFFECTIVE_BALANCE_INCREMENT
Expand Down Expand Up @@ -99,6 +105,8 @@ def test_initialize_beacon_state_some_small_balances(spec):
assert state.eth1_data.deposit_count == len(deposits)
assert state.eth1_data.block_hash == eth1_block_hash
# only main deposits participate to the active balance
# NOTE: they are pre-ELECTRA deposits with BLS_WITHDRAWAL_PREFIX,
# so `MAX_EFFECTIVE_BALANCE` is used
assert spec.get_total_active_balance(state) == main_deposit_count * spec.MAX_EFFECTIVE_BALANCE

# yield state
Expand Down