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

Fix off-by-one in process_pending_consolidations #3868

Merged
merged 4 commits into from
Aug 8, 2024
Merged

Fix off-by-one in process_pending_consolidations #3868

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fcca2b5
Fix off-by-one in process_pending_consolidations
mkalinin Aug 6, 2024
b665698
Fix off-by-one in process_pending_balance_deposits
mkalinin Aug 6, 2024
5e06787
Apply suggestions by @fradamt
mkalinin Aug 7, 2024
024ee04
Fix lint
mkalinin Aug 7, 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
6 changes: 4 additions & 2 deletions specs/electra/beacon-chain.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -854,6 +854,7 @@ def process_registry_updates(state: BeaconState) -> None:

```python
def process_pending_balance_deposits(state: BeaconState) -> None:
next_epoch = Epoch(get_current_epoch(state) + 1)
available_for_processing = state.deposit_balance_to_consume + get_activation_exit_churn_limit(state)
processed_amount = 0
next_deposit_index = 0
Expand All @@ -863,7 +864,7 @@ def process_pending_balance_deposits(state: BeaconState) -> None:
validator = state.validators[deposit.index]
# Validator is exiting, postpone the deposit until after withdrawable epoch
if validator.exit_epoch < FAR_FUTURE_EPOCH:
if get_current_epoch(state) <= validator.withdrawable_epoch:
if next_epoch <= validator.withdrawable_epoch:
deposits_to_postpone.append(deposit)
# Deposited balance will never become active. Increase balance but do not consume churn
else:
Expand Down Expand Up @@ -894,13 +895,14 @@ def process_pending_balance_deposits(state: BeaconState) -> None:

```python
def process_pending_consolidations(state: BeaconState) -> None:
next_epoch = Epoch(get_current_epoch(state) + 1)
next_pending_consolidation = 0
for pending_consolidation in state.pending_consolidations:
source_validator = state.validators[pending_consolidation.source_index]
if source_validator.slashed:
next_pending_consolidation += 1
continue
if source_validator.withdrawable_epoch > get_current_epoch(state):
if source_validator.withdrawable_epoch > next_epoch:
break

# Churn any target excess active balance of target and raise its max
Expand Down
163 changes: 162 additions & 1 deletion ...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
@@ -1,8 +1,14 @@
from eth2spec.test.helpers.epoch_processing import run_epoch_processing_with
from eth2spec.test.helpers.epoch_processing import (
run_epoch_processing_with,
compute_state_by_epoch_processing_to,
)
from eth2spec.test.context import (
spec_state_test,
with_electra_and_later,
)
from eth2spec.test.helpers.state import (
next_epoch_with_full_participation,
)

# ***********************
# * CONSOLIDATION TESTS *
Expand Down Expand Up @@ -185,3 +191,158 @@ def test_all_consolidation_cases_together(spec, state):
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
state.pending_consolidations = pre_pending_consolidations[2:]


@with_electra_and_later
@spec_state_test
def test_pending_consolidation_future_epoch(spec, state):
current_epoch = spec.get_current_epoch(state)
source_index = spec.get_active_validator_indices(state, current_epoch)[0]
target_index = spec.get_active_validator_indices(state, current_epoch)[1]
# initiate source exit
spec.initiate_validator_exit(state, source_index)
# set withdrawable_epoch to exit_epoch + 1
state.validators[source_index].withdrawable_epoch = state.validators[source_index].exit_epoch + spec.Epoch(1)
# append pending consolidation
state.pending_consolidations.append(
spec.PendingConsolidation(source_index=source_index, target_index=target_index)
)
# Set the target withdrawal credential to eth1
eth1_withdrawal_credential = (
spec.ETH1_ADDRESS_WITHDRAWAL_PREFIX + b"\x00" * 11 + b"\x11" * 20
)
state.validators[target_index].withdrawal_credentials = eth1_withdrawal_credential

# Advance to withdrawable_epoch - 1 with full participation
target_epoch = state.validators[source_index].withdrawable_epoch - spec.Epoch(1)
while spec.get_current_epoch(state) < target_epoch:
next_epoch_with_full_participation(spec, state)

# Obtain state before the call to process_pending_consolidations
state_before_consolidation = compute_state_by_epoch_processing_to(spec, state, "process_pending_consolidations")

yield from run_epoch_processing_with(spec, state, "process_pending_consolidations")
Comment on lines +221 to +224
Copy link
Contributor

@hwwhww hwwhww Aug 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

edited: run_epoch_processing_with compute_state_by_epoch_processing_to

Another solution is to make run_epoch_processing_to return values:

def run_epoch_processing_to(spec, state, process_name: str):
    ...
    pre = state.copy()
    ...
    return pre, state

and call it with return values in tests:

    pre, post = yield from compute_state_by_epoch_processing_to(spec, state, "process_pending_consolidations")


# 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
assert state.balances[source_index] == expected_source_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
def test_pending_consolidation_compounding_creds(spec, state):
current_epoch = spec.get_current_epoch(state)
source_index = spec.get_active_validator_indices(state, current_epoch)[0]
target_index = spec.get_active_validator_indices(state, current_epoch)[1]
# initiate source exit
spec.initiate_validator_exit(state, source_index)
# set withdrawable_epoch to exit_epoch + 1
state.validators[source_index].withdrawable_epoch = state.validators[source_index].exit_epoch + spec.Epoch(1)
# append pending consolidation
state.pending_consolidations.append(
spec.PendingConsolidation(source_index=source_index, target_index=target_index)
)
# Set the source and the target withdrawal credential to compounding
state.validators[source_index].withdrawal_credentials = (
spec.COMPOUNDING_WITHDRAWAL_PREFIX + b"\x00" * 11 + b"\x11" * 20
)
state.validators[target_index].withdrawal_credentials = (
spec.COMPOUNDING_WITHDRAWAL_PREFIX + b"\x00" * 11 + b"\x12" * 20
)

# Advance to withdrawable_epoch - 1 with full participation
target_epoch = state.validators[source_index].withdrawable_epoch - spec.Epoch(1)
while spec.get_current_epoch(state) < target_epoch:
next_epoch_with_full_participation(spec, state)

# Obtain state before the call to process_pending_consolidations
state_before_consolidation = compute_state_by_epoch_processing_to(spec, state, "process_pending_consolidations")

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

# Pending consolidation was successfully processed
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
assert state.balances[source_index] == 0
mkalinin marked this conversation as resolved.
Show resolved Hide resolved
assert state.pending_consolidations == []

# Pending balance deposit to the target is not created,
# because the target already has compounding credentials
assert len(state.pending_balance_deposits) == 0


@with_electra_and_later
@spec_state_test
def test_pending_consolidation_with_pending_deposit(spec, state):
current_epoch = spec.get_current_epoch(state)
source_index = spec.get_active_validator_indices(state, current_epoch)[0]
target_index = spec.get_active_validator_indices(state, current_epoch)[1]
# initiate source exit
spec.initiate_validator_exit(state, source_index)
# set withdrawable_epoch to exit_epoch + 1
state.validators[source_index].withdrawable_epoch = state.validators[source_index].exit_epoch + spec.Epoch(1)
# append pending consolidation
state.pending_consolidations.append(
spec.PendingConsolidation(source_index=source_index, target_index=target_index)
)
# append pending deposit
state.pending_balance_deposits.append(
spec.PendingBalanceDeposit(index=source_index, amount=spec.MIN_ACTIVATION_BALANCE)
)
# Set the source and the target withdrawal credential to compounding
state.validators[source_index].withdrawal_credentials = (
spec.COMPOUNDING_WITHDRAWAL_PREFIX + b"\x00" * 11 + b"\x11" * 20
)
state.validators[target_index].withdrawal_credentials = (
spec.COMPOUNDING_WITHDRAWAL_PREFIX + b"\x00" * 11 + b"\x12" * 20
)

# Advance to withdrawable_epoch - 1 with full participation
target_epoch = state.validators[source_index].withdrawable_epoch - spec.Epoch(1)
while spec.get_current_epoch(state) < target_epoch:
next_epoch_with_full_participation(spec, state)

# Obtain state before the call to process_pending_balance_deposits
state_before_consolidation = compute_state_by_epoch_processing_to(spec, state, "process_pending_balance_deposits")

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

# Pending consolidation was successfully processed
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 == []

# Pending balance deposit to the source was not processed.
# It should only be processed in the next epoch transition
assert len(state.pending_balance_deposits) == 1
assert state.pending_balance_deposits[0] == spec.PendingBalanceDeposit(
index=source_index, amount=spec.MIN_ACTIVATION_BALANCE)
8 changes: 8 additions & 0 deletions tests/core/pyspec/eth2spec/test/helpers/epoch_processing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ def get_process_calls(spec):
'charge_confirmed_header_fees', # sharding
'reset_pending_headers', # sharding
'process_eth1_data_reset',
'process_pending_balance_deposits', # electra
'process_pending_consolidations', # electra
'process_effective_balance_updates',
'process_slashings_reset',
'process_randao_mixes_reset',
Expand Down Expand Up @@ -72,3 +74,9 @@ def run_epoch_processing_with(spec, state, process_name: str):
yield 'pre', state
getattr(spec, process_name)(state)
yield 'post', state


def compute_state_by_epoch_processing_to(spec, state, process_name: str):
state_copy = state.copy()
run_epoch_processing_to(spec, state_copy, process_name)
return state_copy
8 changes: 8 additions & 0 deletions tests/core/pyspec/eth2spec/test/helpers/state.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,14 @@ def next_epoch(spec, state):
spec.process_slots(state, slot)


def next_epoch_with_full_participation(spec, state):
"""
Transition to the start slot of the next epoch with full participation
"""
set_full_participation(spec, state)
next_epoch(spec, state)


def next_epoch_via_block(spec, state, insert_state_root=False):
"""
Transition to the start slot of the next epoch via a full block transition
Expand Down