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

Jump to bottom

eip7251: Switch to compounding when consolidating with source==target #3918

Merged
merged 11 commits into from
Oct 3, 2024
Merged

eip7251: Switch to compounding when consolidating with source==target #3918

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0eda70c
Switch to compounding when consolidating with source==target
mkalinin Sep 12, 2024
747a5a7
Apply suggestions from @dapplion
mkalinin Sep 16, 2024
1bf8ca5
Apply suggestions by @ralexstokes
mkalinin Sep 18, 2024
b29a1d3
Isolate switch to compounding flow
mkalinin Sep 18, 2024
d71d9dd
Fix lint
mkalinin Sep 18, 2024
23699f5
Undo MAX_EB to MIN_AB switch in withdrawal helper
mkalinin Sep 19, 2024
2bc2604
Apply suggestions from @jtraglia
mkalinin Sep 27, 2024
a7b0d6f
Apply suggestions from code review
mkalinin Oct 1, 2024
11cfd96
Merge branch 'dev' into consolidation-switch-to-comp
mkalinin Oct 1, 2024
2dd9e82
Remove blank lines
mkalinin Oct 3, 2024
66f5c37
Fix switch to compounding tests
mkalinin Oct 3, 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
41 changes: 13 additions & 28 deletions specs/electra/beacon-chain.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@
- [Modified `get_next_sync_committee_indices`](#modified-get_next_sync_committee_indices)
- [Beacon state mutators](#beacon-state-mutators)
- [Modified `initiate_validator_exit`](#modified-initiate_validator_exit)
- [New `switch_to_compounding_validator`](#new-switch_to_compounding_validator)
- [New `queue_excess_active_balance`](#new-queue_excess_active_balance)
- [New `queue_entire_balance_and_reset_validator`](#new-queue_entire_balance_and_reset_validator)
- [New `compute_exit_epoch_and_update_churn`](#new-compute_exit_epoch_and_update_churn)
Expand Down Expand Up @@ -678,16 +677,6 @@ def initiate_validator_exit(state: BeaconState, index: ValidatorIndex) -> None:
validator.withdrawable_epoch = Epoch(validator.exit_epoch + MIN_VALIDATOR_WITHDRAWABILITY_DELAY)
```

#### 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 = COMPOUNDING_WITHDRAWAL_PREFIX + validator.withdrawal_credentials[1:]
queue_excess_active_balance(state, index)
```

#### New `queue_excess_active_balance`

```python
Expand Down Expand Up @@ -928,8 +917,6 @@ def process_pending_consolidations(state: BeaconState) -> None:
if source_validator.withdrawable_epoch > next_epoch:
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)
Expand Down Expand Up @@ -1225,14 +1212,6 @@ def apply_deposit(state: BeaconState,
state.pending_balance_deposits.append(
PendingBalanceDeposit(index=index, amount=amount)
) # [Modified in Electra:EIP7251]
# 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`
Expand Down Expand Up @@ -1431,10 +1410,6 @@ def process_consolidation_request(
source_validator = state.validators[source_index]
target_validator = state.validators[target_index]

# Verify that source != target, so a consolidation cannot be used as an exit.
if source_index == target_index:
return

# Verify source withdrawal credentials
has_correct_credential = has_execution_withdrawal_credential(source_validator)
is_correct_source_address = (
Copy link
Member

Choose a reason for hiding this comment

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

lets do this later but I think we can also factor out a helper like is_valid_consolidation_request

Expand All @@ -1459,12 +1434,22 @@ def process_consolidation_request(
if target_validator.exit_epoch != FAR_FUTURE_EPOCH:
return

# Churn any target excess active balance of target and raise its max
if has_eth1_withdrawal_credential(target_validator):
state.validators[target_index].withdrawal_credentials = (
COMPOUNDING_WITHDRAWAL_PREFIX + target_validator.withdrawal_credentials[1:])
mkalinin marked this conversation as resolved.
Show resolved Hide resolved
queue_excess_active_balance(state, target_index)

# Verify that source != target, so a consolidation cannot be used as an exit.
if source_index == target_index:
return

# Initiate source validator exit and append pending consolidation
source_validator.exit_epoch = compute_consolidation_epoch_and_update_churn(
state.validators[source_index].exit_epoch = compute_consolidation_epoch_and_update_churn(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think in Python variable is assigned to a reference. So if you modify a variable that is a reference to an element in an array, the array element will be updated as well.

I don't see why we would need to replace source_validator with state.validators[source_index] explicitly when updating. Unless I am missing something.

Copy link
Collaborator Author

@mkalinin mkalinin Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, you’re right, and the reason for doing so is the bug somewhere in one of the components that spec relies on, likely in remerklable. Consider the following test:

@with_electra_and_later
@spec_state_test
def test_state_changes_overwritten(spec, state):
    validator_0 = state.validators[0]
    validator_1 = state.validators[1]
    
    validator_0.exit_epoch = spec.Epoch(0)
    validator_1.exit_epoch = spec.Epoch(1)

    assert state.validators[1].exit_epoch == spec.Epoch(1)
    assert state.validators[0].exit_epoch == spec.Epoch(0)

The second assert fails while it must not. This problem requires a separate work, and I decided to use a workaround for this spec change. Filed an issue #3925

state, source_validator.effective_balance
)
source_validator.withdrawable_epoch = Epoch(
source_validator.exit_epoch + MIN_VALIDATOR_WITHDRAWABILITY_DELAY
state.validators[source_index].withdrawable_epoch = Epoch(
state.validators[source_index].exit_epoch + MIN_VALIDATOR_WITHDRAWABILITY_DELAY
)
state.pending_consolidations.append(PendingConsolidation(
source_index=source_index,
Expand Down
84 changes: 68 additions & 16 deletions .../core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,37 @@ def test_consolidation_balance_through_two_churn_epochs(spec, state):
assert state.consolidation_balance_to_consume == expected_balance


# Failing tests
@with_electra_and_later
@with_presets([MINIMAL], "need sufficient consolidation churn limit")
@with_custom_state(
balances_fn=scaled_churn_balances_exceed_activation_exit_churn_limit,
threshold_fn=default_activation_threshold,
)
@spec_test
@single_phase
def test_source_equals_target_switches_to_compounding(spec, state):
current_epoch = spec.get_current_epoch(state)
source_index = spec.get_active_validator_indices(state, current_epoch)[0]

# Set source to eth1 credentials
source_address = b"\x22" * 20
set_eth1_withdrawal_credential_with_balance(
spec, state, source_index, address=source_address
)
# Make consolidation from source to source
consolidation = spec.ConsolidationRequest(
source_address=source_address,
source_pubkey=state.validators[source_index].pubkey,
target_pubkey=state.validators[source_index].pubkey,
)

# Check the the return condition
assert consolidation.source_pubkey == consolidation.target_pubkey

yield from run_consolidation_processing(
spec, state, consolidation, success=True
)


@with_electra_and_later
@with_presets([MINIMAL], "need sufficient consolidation churn limit")
Expand All @@ -405,7 +435,7 @@ def test_consolidation_balance_through_two_churn_epochs(spec, state):
)
@spec_test
@single_phase
def test_incorrect_source_equals_target(spec, state):
def test_source_equals_target_switches_to_compounding_with_excess(spec, state):
current_epoch = spec.get_current_epoch(state)
source_index = spec.get_active_validator_indices(state, current_epoch)[0]

Expand All @@ -414,6 +444,8 @@ def test_incorrect_source_equals_target(spec, state):
set_eth1_withdrawal_credential_with_balance(
spec, state, source_index, address=source_address
)
# Add excess balance
state.balances[source_index] = state.balances[source_index] + spec.EFFECTIVE_BALANCE_INCREMENT
# Make consolidation from source to source
consolidation = spec.ConsolidationRequest(
source_address=source_address,
Expand All @@ -425,10 +457,12 @@ def test_incorrect_source_equals_target(spec, state):
assert consolidation.source_pubkey == consolidation.target_pubkey

yield from run_consolidation_processing(
spec, state, consolidation, success=False
spec, state, consolidation, success=True
)


# Failing tests
mkalinin marked this conversation as resolved.
Show resolved Hide resolved

@with_electra_and_later
@with_presets([MINIMAL], "need sufficient consolidation churn limit")
@with_custom_state(
Expand Down Expand Up @@ -815,36 +849,54 @@ def run_consolidation_processing(spec, state, consolidation, success=True):
pre_exit_epoch_source = source_validator.exit_epoch
pre_exit_epoch_target = target_validator.exit_epoch
pre_pending_consolidations = state.pending_consolidations.copy()
pre_target_withdrawal_credentials = target_validator.withdrawal_credentials
pre_target_balance = state.balances[target_index]
else:
pre_state = state.copy()

yield 'pre', state
yield 'consolidation_request', consolidation

spec.process_consolidation_request(state, consolidation)
# print(state.validators[target_index].withdrawal_credentials)
mkalinin marked this conversation as resolved.
Show resolved Hide resolved

yield 'post', state

if success:
# Check source and target have execution credentials
# Check source has execution credentials
assert spec.has_execution_withdrawal_credential(source_validator)
# Check target has compounding credentials
assert spec.has_execution_withdrawal_credential(target_validator)
# Check source address in the consolidation fits the withdrawal credentials
assert source_validator.withdrawal_credentials[12:] == consolidation.source_address
# Check source and target are not the same
assert source_index != target_index
# Check source and target were not exiting
assert pre_exit_epoch_source == spec.FAR_FUTURE_EPOCH
assert pre_exit_epoch_target == spec.FAR_FUTURE_EPOCH
# Check source is now exiting
assert state.validators[source_index].exit_epoch < spec.FAR_FUTURE_EPOCH
# Check that the exit epoch matches earliest_consolidation_epoch
assert state.validators[source_index].exit_epoch == state.earliest_consolidation_epoch
# Check that the correct consolidation has been appended
expected_new_pending_consolidation = spec.PendingConsolidation(
source_index=source_index,
target_index=target_index,
)
assert state.pending_consolidations == pre_pending_consolidations + [expected_new_pending_consolidation]
# Check excess balance is queued if the target switched to compounding
if pre_target_withdrawal_credentials[:1] == spec.ETH1_ADDRESS_WITHDRAWAL_PREFIX:
assert state.validators[target_index].withdrawal_credentials == (
spec.COMPOUNDING_WITHDRAWAL_PREFIX + pre_target_withdrawal_credentials[1:])
assert state.balances[target_index] == spec.MIN_ACTIVATION_BALANCE
if pre_target_balance > spec.MIN_ACTIVATION_BALANCE:
assert state.pending_balance_deposits == [spec.PendingBalanceDeposit(
index=target_index, amount=(pre_target_balance - spec.MIN_ACTIVATION_BALANCE))]
# If source and target are same, no consolidation must have been initiated
if source_index == target_index:
assert state.validators[source_index].exit_epoch == spec.FAR_FUTURE_EPOCH
assert state.pending_consolidations == []
else:
# Check source is now exiting
assert state.validators[source_index].exit_epoch < spec.FAR_FUTURE_EPOCH
# Check that the exit epoch matches earliest_consolidation_epoch
assert state.validators[source_index].exit_epoch == state.earliest_consolidation_epoch
# Check that the withdrawable_epoch is set correctly
assert state.validators[source_index].withdrawable_epoch == (
state.validators[source_index].exit_epoch + spec.config.MIN_VALIDATOR_WITHDRAWABILITY_DELAY)
# Check that the correct consolidation has been appended
expected_new_pending_consolidation = spec.PendingConsolidation(
source_index=source_index,
target_index=target_index,
)
assert state.pending_consolidations == pre_pending_consolidations + [expected_new_pending_consolidation]
else:
assert pre_state == state
55 changes: 2 additions & 53 deletions ...core/pyspec/eth2spec/test/electra/epoch_processing/test_process_pending_consolidations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,6 @@ def test_basic_pending_consolidation(spec, state):
yield from run_epoch_processing_with(spec, state, "process_pending_consolidations")

# Pending consolidation was successfully processed
assert (
state.validators[target_index].withdrawal_credentials[:1]
== spec.COMPOUNDING_WITHDRAWAL_PREFIX
)
assert state.balances[target_index] == 2 * spec.MIN_ACTIVATION_BALANCE
assert state.balances[source_index] == 0
assert state.pending_consolidations == []
Expand All @@ -65,21 +61,13 @@ def test_consolidation_not_yet_withdrawable_validator(spec, state):

pre_pending_consolidations = state.pending_consolidations.copy()
pre_balances = state.balances.copy()
pre_target_withdrawal_credential = state.validators[
target_index
].withdrawal_credentials[:1]

yield from run_epoch_processing_with(spec, state, "process_pending_consolidations")

# Pending consolidation is not processed
# Balances are unchanged
assert state.balances[source_index] == pre_balances[0]
assert state.balances[target_index] == pre_balances[1]
# Target withdrawal credential is unchanged
assert (
state.validators[target_index].withdrawal_credentials[:1]
== pre_target_withdrawal_credential
)
# Pending consolidation is still in the queue
assert state.pending_consolidations == pre_pending_consolidations

Expand Down Expand Up @@ -121,17 +109,9 @@ def test_skip_consolidation_when_source_slashed(spec, state):
# first pending consolidation should not be processed
assert state.balances[target0_index] == spec.MIN_ACTIVATION_BALANCE
assert state.balances[source0_index] == spec.MIN_ACTIVATION_BALANCE
assert (
state.validators[target0_index].withdrawal_credentials[:1]
== spec.ETH1_ADDRESS_WITHDRAWAL_PREFIX
)
# second pending consolidation should be processed: first one is skipped and doesn't block the queue
assert state.balances[target1_index] == 2 * spec.MIN_ACTIVATION_BALANCE
assert state.balances[source1_index] == 0
assert (
state.validators[target1_index].withdrawal_credentials[:1]
== spec.COMPOUNDING_WITHDRAWAL_PREFIX
)


@with_electra_and_later
Expand Down Expand Up @@ -167,26 +147,14 @@ def test_all_consolidation_cases_together(spec, state):
spec.initiate_validator_exit(state, 2)

pre_balances = state.balances.copy()
pre_target_withdrawal_prefixes = [
state.validators[target_index[i]].withdrawal_credentials[:1]
for i in [0, 1, 2, 3]
]
pre_pending_consolidations = state.pending_consolidations.copy()
yield from run_epoch_processing_with(spec, state, "process_pending_consolidations")

# First consolidation is successfully processed
assert (
state.validators[target_index[0]].withdrawal_credentials[:1]
== spec.COMPOUNDING_WITHDRAWAL_PREFIX
)
assert state.balances[target_index[0]] == 2 * spec.MIN_ACTIVATION_BALANCE
assert state.balances[source_index[0]] == 0
# All other consolidations are not processed
for i in [1, 2, 3]:
assert (
state.validators[target_index[i]].withdrawal_credentials[:1]
== pre_target_withdrawal_prefixes[i]
)
assert state.balances[source_index[i]] == pre_balances[source_index[i]]
assert state.balances[target_index[i]] == pre_balances[target_index[i]]
# First consolidation is processed, second is skipped, last two are left in the queue
Expand Down Expand Up @@ -226,22 +194,11 @@ def test_pending_consolidation_future_epoch(spec, state):

# Pending consolidation was successfully processed
expected_source_balance = state_before_consolidation.balances[source_index] - spec.MIN_ACTIVATION_BALANCE
assert (
state.validators[target_index].withdrawal_credentials[:1]
== spec.COMPOUNDING_WITHDRAWAL_PREFIX
)
assert state.balances[target_index] == 2 * spec.MIN_ACTIVATION_BALANCE
expected_target_balance = state_before_consolidation.balances[target_index] + spec.MIN_ACTIVATION_BALANCE
assert state.balances[source_index] == expected_source_balance
assert state.balances[target_index] == expected_target_balance
assert state.pending_consolidations == []

# Pending balance deposit to the target is created as part of `switch_to_compounding_validator`.
# The excess balance to queue are the rewards accumulated over the previous epoch transitions.
expected_pending_balance = state_before_consolidation.balances[target_index] - spec.MIN_ACTIVATION_BALANCE
assert len(state.pending_balance_deposits) > 0
pending_balance_deposit = state.pending_balance_deposits[len(state.pending_balance_deposits) - 1]
assert pending_balance_deposit.index == target_index
assert pending_balance_deposit.amount == expected_pending_balance


@with_electra_and_later
@spec_state_test
Expand Down Expand Up @@ -280,10 +237,6 @@ def test_pending_consolidation_compounding_creds(spec, state):
expected_target_balance = (
state_before_consolidation.balances[source_index] + state_before_consolidation.balances[target_index]
)
assert (
state.validators[target_index].withdrawal_credentials[:1]
== spec.COMPOUNDING_WITHDRAWAL_PREFIX
)
assert state.balances[target_index] == expected_target_balance
# All source balance is active and moved to the target,
# because the source validator has compounding credentials
Expand Down Expand Up @@ -336,10 +289,6 @@ def test_pending_consolidation_with_pending_deposit(spec, state):
expected_target_balance = (
state_before_consolidation.balances[source_index] + state_before_consolidation.balances[target_index]
)
assert (
state.validators[target_index].withdrawal_credentials[:1]
== spec.COMPOUNDING_WITHDRAWAL_PREFIX
)
assert state.balances[target_index] == expected_target_balance
assert state.balances[source_index] == 0
assert state.pending_consolidations == []
Expand Down