From fcca2b5c7153cf7833c8a195e2d330b9d945f9cb Mon Sep 17 00:00:00 2001 From: Mikhail Kalinin Date: Tue, 6 Aug 2024 17:17:26 +0600 Subject: [PATCH 1/4] Fix off-by-one in process_pending_consolidations --- specs/electra/beacon-chain.md | 3 +- .../test_process_pending_consolidations.py | 106 +++++++++++++++++- .../eth2spec/test/helpers/epoch_processing.py | 8 ++ .../pyspec/eth2spec/test/helpers/state.py | 8 ++ 4 files changed, 123 insertions(+), 2 deletions(-) diff --git a/specs/electra/beacon-chain.md b/specs/electra/beacon-chain.md index d9185b2030..f7019f8d30 100644 --- a/specs/electra/beacon-chain.md +++ b/specs/electra/beacon-chain.md @@ -894,13 +894,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 diff --git a/tests/core/pyspec/eth2spec/test/electra/epoch_processing/test_process_pending_consolidations.py b/tests/core/pyspec/eth2spec/test/electra/epoch_processing/test_process_pending_consolidations.py index 6c21a722ff..9cb3886dac 100644 --- a/tests/core/pyspec/eth2spec/test/electra/epoch_processing/test_process_pending_consolidations.py +++ b/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,101 @@ 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 + 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 + compounding_withdrawal_credential_1 = ( + spec.COMPOUNDING_WITHDRAWAL_PREFIX + b"\x00" * 11 + b"\x11" * 20 + ) + compounding_withdrawal_credential_2 = ( + spec.COMPOUNDING_WITHDRAWAL_PREFIX + b"\x00" * 11 + b"\x12" * 20 + ) + state.validators[source_index].withdrawal_credentials = compounding_withdrawal_credential_1 + state.validators[target_index].withdrawal_credentials = compounding_withdrawal_credential_2 + + # 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 + assert state.balances[source_index] == 0 + assert state.pending_consolidations == [] + + # Pending balance deposit to the target is not created + assert len(state.pending_balance_deposits) == 0 diff --git a/tests/core/pyspec/eth2spec/test/helpers/epoch_processing.py b/tests/core/pyspec/eth2spec/test/helpers/epoch_processing.py index 44b42aff91..80302e111d 100644 --- a/tests/core/pyspec/eth2spec/test/helpers/epoch_processing.py +++ b/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 diff --git a/tests/core/pyspec/eth2spec/test/helpers/state.py b/tests/core/pyspec/eth2spec/test/helpers/state.py index 1e64bd4db2..07e7bfb478 100644 --- a/tests/core/pyspec/eth2spec/test/helpers/state.py +++ b/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 From b6656983506dafc6715470d91e8d9beb3309a64f Mon Sep 17 00:00:00 2001 From: Mikhail Kalinin Date: Tue, 6 Aug 2024 17:29:13 +0600 Subject: [PATCH 2/4] Fix off-by-one in process_pending_balance_deposits --- specs/electra/beacon-chain.md | 3 +- .../test_process_pending_consolidations.py | 60 +++++++++++++++++-- 2 files changed, 58 insertions(+), 5 deletions(-) diff --git a/specs/electra/beacon-chain.md b/specs/electra/beacon-chain.md index f7019f8d30..cb5ee6a7a9 100644 --- a/specs/electra/beacon-chain.md +++ b/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: diff --git a/tests/core/pyspec/eth2spec/test/electra/epoch_processing/test_process_pending_consolidations.py b/tests/core/pyspec/eth2spec/test/electra/epoch_processing/test_process_pending_consolidations.py index 9cb3886dac..2682a535b5 100644 --- a/tests/core/pyspec/eth2spec/test/electra/epoch_processing/test_process_pending_consolidations.py +++ b/tests/core/pyspec/eth2spec/test/electra/epoch_processing/test_process_pending_consolidations.py @@ -256,14 +256,12 @@ def test_pending_consolidation_compounding_creds(spec, state): spec.PendingConsolidation(source_index=source_index, target_index=target_index) ) # Set the source and the target withdrawal credential to compounding - compounding_withdrawal_credential_1 = ( + state.validators[source_index].withdrawal_credentials = ( spec.COMPOUNDING_WITHDRAWAL_PREFIX + b"\x00" * 11 + b"\x11" * 20 ) - compounding_withdrawal_credential_2 = ( + state.validators[target_index].withdrawal_credentials = ( spec.COMPOUNDING_WITHDRAWAL_PREFIX + b"\x00" * 11 + b"\x12" * 20 ) - state.validators[source_index].withdrawal_credentials = compounding_withdrawal_credential_1 - state.validators[target_index].withdrawal_credentials = compounding_withdrawal_credential_2 # Advance to withdrawable_epoch - 1 with full participation target_epoch = state.validators[source_index].withdrawable_epoch - spec.Epoch(1) @@ -289,3 +287,57 @@ def test_pending_consolidation_compounding_creds(spec, state): # Pending balance deposit to the target is not created 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 + assert len(state.pending_balance_deposits) == 1 + assert state.pending_balance_deposits[0] == spec.PendingBalanceDeposit( + index=source_index, amount=spec.MIN_ACTIVATION_BALANCE) From 5e06787db8cce44d615f79eccba04809bcb1d715 Mon Sep 17 00:00:00 2001 From: Mikhail Kalinin Date: Wed, 7 Aug 2024 15:54:50 +0600 Subject: [PATCH 3/4] Apply suggestions by @fradamt Co-authored-by: fradamt <104826920+fradamt@users.noreply.github.com> --- .../test_process_pending_consolidations.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/tests/core/pyspec/eth2spec/test/electra/epoch_processing/test_process_pending_consolidations.py b/tests/core/pyspec/eth2spec/test/electra/epoch_processing/test_process_pending_consolidations.py index 2682a535b5..aa0d8a3b97 100644 --- a/tests/core/pyspec/eth2spec/test/electra/epoch_processing/test_process_pending_consolidations.py +++ b/tests/core/pyspec/eth2spec/test/electra/epoch_processing/test_process_pending_consolidations.py @@ -233,7 +233,8 @@ def test_pending_consolidation_future_epoch(spec, state): assert state.balances[source_index] == expected_source_balance assert state.pending_consolidations == [] - # Pending balance deposit to the target is created + # 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] @@ -282,10 +283,13 @@ def test_pending_consolidation_compounding_creds(spec, state): == 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 + # Pending balance deposit to the target is not created, + # because the target already has compounding credentials assert len(state.pending_balance_deposits) == 0 @@ -337,7 +341,8 @@ def test_pending_consolidation_with_pending_deposit(spec, state): assert state.balances[source_index] == 0 assert state.pending_consolidations == [] - # Pending balance deposit to the source was not processed + # 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) From 024ee04373a27d989d59cb49f7cd1b67636365f7 Mon Sep 17 00:00:00 2001 From: Mikhail Kalinin Date: Wed, 7 Aug 2024 16:03:39 +0600 Subject: [PATCH 4/4] Fix lint --- .../epoch_processing/test_process_pending_consolidations.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/core/pyspec/eth2spec/test/electra/epoch_processing/test_process_pending_consolidations.py b/tests/core/pyspec/eth2spec/test/electra/epoch_processing/test_process_pending_consolidations.py index aa0d8a3b97..d750149839 100644 --- a/tests/core/pyspec/eth2spec/test/electra/epoch_processing/test_process_pending_consolidations.py +++ b/tests/core/pyspec/eth2spec/test/electra/epoch_processing/test_process_pending_consolidations.py @@ -283,12 +283,12 @@ def test_pending_consolidation_compounding_creds(spec, state): == spec.COMPOUNDING_WITHDRAWAL_PREFIX ) assert state.balances[target_index] == expected_target_balance - # All source balance is active and moved to the target, + # 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, + # Pending balance deposit to the target is not created, # because the target already has compounding credentials assert len(state.pending_balance_deposits) == 0