Merge pull request #3868 from mkalinin/fix-consolidation

Fix off-by-one in process_pending_consolidations

Author: hwwhww

specs/electra/beacon-chain.md

@@ -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
@@ -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:
@@ -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

tests/core/pyspec/eth2spec/test/electra/epoch_processing/test_process_pending_consolidations.py

@@ -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 *
@@ -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")
+
+    # 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
+    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)

tests/core/pyspec/eth2spec/test/helpers/epoch_processing.py

@@ -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',
@@ -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

tests/core/pyspec/eth2spec/test/helpers/state.py

@@ -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