From 0eda70cfcbf31f5eabe95339fd43d962b00f915e Mon Sep 17 00:00:00 2001 From: Mikhail Kalinin Date: Thu, 12 Sep 2024 20:02:39 +0600 Subject: [PATCH 01/28] Switch to compounding when consolidating with source==target --- specs/electra/beacon-chain.md | 41 +++------ .../test_process_consolidation_request.py | 86 +++++++++++++++---- .../test_process_pending_consolidations.py | 55 +----------- 3 files changed, 84 insertions(+), 98 deletions(-) diff --git a/specs/electra/beacon-chain.md b/specs/electra/beacon-chain.md index 3dca59e1d..d00c969f1 100644 --- a/specs/electra/beacon-chain.md +++ b/specs/electra/beacon-chain.md @@ -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) @@ -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 @@ -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) @@ -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` @@ -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 = ( @@ -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:]) + 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( 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, diff --git a/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py b/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py index b05c20f56..1c6d8d22a 100644 --- a/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py +++ b/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py @@ -395,8 +395,6 @@ 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( @@ -405,7 +403,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(spec, state): current_epoch = spec.get_current_epoch(state) source_index = spec.get_active_validator_indices(state, current_epoch)[0] @@ -425,10 +423,46 @@ 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 ) +@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_with_excess(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 + ) + # 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, + 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 + ) + + +# Failing tests + @with_electra_and_later @with_presets([MINIMAL], "need sufficient consolidation churn limit") @with_custom_state( @@ -815,6 +849,8 @@ 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() @@ -822,29 +858,45 @@ def run_consolidation_processing(spec, state, consolidation, success=True): yield 'consolidation_request', consolidation spec.process_consolidation_request(state, consolidation) + # print(state.validators[target_index].withdrawal_credentials) 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 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 14e151e2e..61714562d 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 @@ -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 == [] @@ -65,9 +61,6 @@ 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") @@ -75,11 +68,6 @@ def test_consolidation_not_yet_withdrawable_validator(spec, state): # 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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 == [] From 747a5a7891a35b6dccbd8b6379ecb9c24936f4ee Mon Sep 17 00:00:00 2001 From: Mikhail Kalinin Date: Mon, 16 Sep 2024 11:01:06 +0400 Subject: [PATCH 02/28] Apply suggestions from @dapplion Co-authored-by: Lion - dapplion <35266934+dapplion@users.noreply.github.com> --- specs/electra/beacon-chain.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/specs/electra/beacon-chain.md b/specs/electra/beacon-chain.md index d00c969f1..9037dd74d 100644 --- a/specs/electra/beacon-chain.md +++ b/specs/electra/beacon-chain.md @@ -1437,7 +1437,8 @@ def process_consolidation_request( # 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:]) + COMPOUNDING_WITHDRAWAL_PREFIX + target_validator.withdrawal_credentials[1:] + ) queue_excess_active_balance(state, target_index) # Verify that source != target, so a consolidation cannot be used as an exit. From 1bf8ca5777cab1327036cea52d559ceee76e47f5 Mon Sep 17 00:00:00 2001 From: Mikhail Kalinin Date: Wed, 18 Sep 2024 09:20:42 +0400 Subject: [PATCH 03/28] Apply suggestions by @ralexstokes Co-authored-by: Alex Stokes --- .../block_processing/test_process_consolidation_request.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py b/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py index 1c6d8d22a..dd4f0b944 100644 --- a/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py +++ b/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py @@ -858,7 +858,6 @@ def run_consolidation_processing(spec, state, consolidation, success=True): yield 'consolidation_request', consolidation spec.process_consolidation_request(state, consolidation) - # print(state.validators[target_index].withdrawal_credentials) yield 'post', state From b29a1d36d1a119db2bebd8e10d3244551be3c3f0 Mon Sep 17 00:00:00 2001 From: Mikhail Kalinin Date: Wed, 18 Sep 2024 13:27:03 +0400 Subject: [PATCH 04/28] Isolate switch to compounding flow --- specs/electra/beacon-chain.md | 81 +++- .../test_process_consolidation_request.py | 396 ++++++++++++++++-- .../eth2spec/test/helpers/withdrawals.py | 5 +- 3 files changed, 425 insertions(+), 57 deletions(-) diff --git a/specs/electra/beacon-chain.md b/specs/electra/beacon-chain.md index 9037dd74d..10c0eebf3 100644 --- a/specs/electra/beacon-chain.md +++ b/specs/electra/beacon-chain.md @@ -61,6 +61,7 @@ - [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) @@ -96,6 +97,7 @@ - [Deposit requests](#deposit-requests) - [New `process_deposit_request`](#new-process_deposit_request) - [Execution layer consolidation requests](#execution-layer-consolidation-requests) + - [New `is_valid_switch_to_compounding_request`](#new-is_valid_switch_to_compounding_request) - [New `process_consolidation_request`](#new-process_consolidation_request) - [Testing](#testing) @@ -677,6 +679,15 @@ 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] + validator.withdrawal_credentials = COMPOUNDING_WITHDRAWAL_PREFIX + validator.withdrawal_credentials[1:] + queue_excess_active_balance(state, index) +``` + #### New `queue_excess_active_balance` ```python @@ -1383,6 +1394,45 @@ def process_deposit_request(state: BeaconState, deposit_request: DepositRequest) ##### Execution layer consolidation requests +###### New `is_valid_switch_to_compounding_request` + +```python +def is_valid_switch_to_compounding_request( + state: BeaconState, + consolidation_request: ConsolidationRequest +) -> bool: + # Switch to compounding requires source and target be equal + if consolidation_request.source_pubkey != consolidation_request.target_pubkey: + return False + + # Verify pubkey exists + source_pubkey = consolidation_request.source_pubkey + validator_pubkeys = [v.pubkey for v in state.validators] + if source_pubkey not in validator_pubkeys: + return False + + source_validator = state.validators[ValidatorIndex(validator_pubkeys.index(source_pubkey))] + + # Verify request has been authorized + if source_validator.withdrawal_credentials[12:] != consolidation_request.source_address: + return False + + # Verify source withdrawal credentials + if not has_eth1_withdrawal_credential(source_validator): + return False + + # Verify the source is active + current_epoch = get_current_epoch(state) + if not is_active_validator(source_validator, current_epoch): + return False + + # Verify exit for source have not been initiated + if source_validator.exit_epoch != FAR_FUTURE_EPOCH: + return False + + return True +``` + ###### New `process_consolidation_request` ```python @@ -1390,6 +1440,16 @@ def process_consolidation_request( state: BeaconState, consolidation_request: ConsolidationRequest ) -> None: + if is_valid_switch_to_compounding_request(state, consolidation_request): + validator_pubkeys = [v.pubkey for v in state.validators] + request_source_pubkey = consolidation_request.source_pubkey + source_index = ValidatorIndex(validator_pubkeys.index(request_source_pubkey)) + switch_to_compounding_validator(state, source_index) + return + + # Verify that source != target, so a consolidation cannot be used as an exit. + if consolidation_request.source_pubkey == consolidation_request.target_pubkey: + return # If the pending consolidations queue is full, consolidation requests are ignored if len(state.pending_consolidations) == PENDING_CONSOLIDATIONS_LIMIT: return @@ -1434,28 +1494,21 @@ 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:] - ) - 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 - state.validators[source_index].exit_epoch = compute_consolidation_epoch_and_update_churn( + source_validator.exit_epoch = compute_consolidation_epoch_and_update_churn( state, source_validator.effective_balance ) - state.validators[source_index].withdrawable_epoch = Epoch( - state.validators[source_index].exit_epoch + MIN_VALIDATOR_WITHDRAWABILITY_DELAY + source_validator.withdrawable_epoch = Epoch( + source_validator.exit_epoch + MIN_VALIDATOR_WITHDRAWABILITY_DELAY ) state.pending_consolidations.append(PendingConsolidation( source_index=source_index, target_index=target_index )) + + # Churn any target excess active balance of target and raise its max + if has_eth1_withdrawal_credential(target_validator): + switch_to_compounding_validator(state, target_index) ``` ## Testing diff --git a/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py b/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py index dd4f0b944..4bc4617c8 100644 --- a/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py +++ b/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py @@ -66,6 +66,106 @@ def test_basic_consolidation_in_current_consolidation_epoch(spec, state): assert state.validators[source_index].exit_epoch == expected_exit_epoch +@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_basic_consolidation_with_excess_target_balance(spec, state): + # This state has 256 validators each with 32 ETH in MINIMAL preset, 128 ETH consolidation churn + 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] + + # 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 with source address + consolidation = spec.ConsolidationRequest( + source_address=source_address, + source_pubkey=state.validators[source_index].pubkey, + target_pubkey=state.validators[target_index].pubkey, + ) + + # Set target to eth1 credentials + set_eth1_withdrawal_credential_with_balance(spec, state, target_index) + + # Set earliest consolidation epoch to the expected exit epoch + expected_exit_epoch = spec.compute_activation_exit_epoch(current_epoch) + state.earliest_consolidation_epoch = expected_exit_epoch + consolidation_churn_limit = spec.get_consolidation_churn_limit(state) + # Set the consolidation balance to consume equal to churn limit + state.consolidation_balance_to_consume = consolidation_churn_limit + + # Add excess balance + state.balances[target_index] = state.balances[target_index] + spec.EFFECTIVE_BALANCE_INCREMENT + + yield from run_consolidation_processing(spec, state, consolidation) + + # Check consolidation churn is decremented correctly + assert ( + state.consolidation_balance_to_consume + == consolidation_churn_limit - spec.MIN_ACTIVATION_BALANCE + ) + # Check exit epoch + assert state.validators[source_index].exit_epoch == expected_exit_epoch + + +@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_basic_consolidation_with_excess_target_balance_and_compounding_credentials(spec, state): + # This state has 256 validators each with 32 ETH in MINIMAL preset, 128 ETH consolidation churn + 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] + + # 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 with source address + consolidation = spec.ConsolidationRequest( + source_address=source_address, + source_pubkey=state.validators[source_index].pubkey, + target_pubkey=state.validators[target_index].pubkey, + ) + + # Set target to eth1 credentials + set_compounding_withdrawal_credential(spec, state, target_index) + + # Set earliest consolidation epoch to the expected exit epoch + expected_exit_epoch = spec.compute_activation_exit_epoch(current_epoch) + state.earliest_consolidation_epoch = expected_exit_epoch + consolidation_churn_limit = spec.get_consolidation_churn_limit(state) + # Set the consolidation balance to consume equal to churn limit + state.consolidation_balance_to_consume = consolidation_churn_limit + + # Add excess balance + state.balances[target_index] = state.balances[target_index] + spec.EFFECTIVE_BALANCE_INCREMENT + + yield from run_consolidation_processing(spec, state, consolidation) + + # Check consolidation churn is decremented correctly + assert ( + state.consolidation_balance_to_consume + == consolidation_churn_limit - spec.MIN_ACTIVATION_BALANCE + ) + # Check exit epoch + assert state.validators[source_index].exit_epoch == expected_exit_epoch + + @with_electra_and_later @with_presets([MINIMAL], "need sufficient consolidation churn limit") @with_custom_state( @@ -235,7 +335,7 @@ def test_basic_consolidation_with_compounding_credentials(spec, state): target_pubkey=state.validators[target_index].pubkey, ) - # Set target to eth1 credentials + # Set target to compounding credentials set_compounding_withdrawal_credential(spec, state, target_index) # Set the consolidation balance to consume equal to churn limit @@ -396,14 +496,8 @@ def test_consolidation_balance_through_two_churn_epochs(spec, state): @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): +@spec_state_test +def test_basic_switch_to_compounding(spec, state): current_epoch = spec.get_current_epoch(state) source_index = spec.get_active_validator_indices(state, current_epoch)[0] @@ -419,23 +513,14 @@ def test_source_equals_target_switches_to_compounding(spec, state): target_pubkey=state.validators[source_index].pubkey, ) - # Check the the return condition - assert consolidation.source_pubkey == consolidation.target_pubkey - - yield from run_consolidation_processing( + yield from run_switch_to_compounding_processing( spec, state, consolidation, success=True ) @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_with_excess(spec, state): +@spec_state_test +def test_switch_to_compounding_with_excess(spec, state): current_epoch = spec.get_current_epoch(state) source_index = spec.get_active_validator_indices(state, current_epoch)[0] @@ -453,10 +538,36 @@ def test_source_equals_target_switches_to_compounding_with_excess(spec, state): target_pubkey=state.validators[source_index].pubkey, ) - # Check the the return condition - assert consolidation.source_pubkey == consolidation.target_pubkey + yield from run_switch_to_compounding_processing( + spec, state, consolidation, success=True + ) - yield from run_consolidation_processing( + +@with_electra_and_later +@spec_state_test +def test_switch_to_compounding_with_pending_consolidations_at_limit(spec, state): + state.pending_consolidations = [ + spec.PendingConsolidation(source_index=0, target_index=1) + ] * spec.PENDING_CONSOLIDATIONS_LIMIT + + 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 + ) + # 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, + source_pubkey=state.validators[source_index].pubkey, + target_pubkey=state.validators[source_index].pubkey, + ) + + yield from run_switch_to_compounding_processing( spec, state, consolidation, success=True ) @@ -831,6 +942,156 @@ def test_incorrect_unknown_target_pubkey(spec, state): ) +@with_electra_and_later +@spec_state_test +def test_switch_to_compounding_exited_source(spec, state): + # Set up an otherwise correct request + current_epoch = spec.get_current_epoch(state) + source_index = spec.get_active_validator_indices(state, current_epoch)[0] + source_address = b"\x22" * 20 + set_eth1_withdrawal_credential_with_balance( + spec, state, source_index, address=source_address + ) + consolidation = spec.ConsolidationRequest( + source_address=source_address, + source_pubkey=state.validators[source_index].pubkey, + target_pubkey=state.validators[source_index].pubkey, + ) + + # exit source + spec.initiate_validator_exit(state, source_index) + + # Check the the return condition + assert state.validators[source_index].exit_epoch != spec.FAR_FUTURE_EPOCH + + yield from run_switch_to_compounding_processing( + spec, state, consolidation, success=False + ) + + +@with_electra_and_later +@spec_state_test +def test_switch_to_compounding_inactive_source(spec, state): + # Set up an otherwise correct request + current_epoch = spec.get_current_epoch(state) + source_index = spec.get_active_validator_indices(state, current_epoch)[0] + source_address = b"\x22" * 20 + set_eth1_withdrawal_credential_with_balance( + spec, state, source_index, address=source_address + ) + consolidation = spec.ConsolidationRequest( + source_address=source_address, + source_pubkey=state.validators[source_index].pubkey, + target_pubkey=state.validators[source_index].pubkey, + ) + + # set source validator as not yet activated + state.validators[source_index].activation_epoch = spec.FAR_FUTURE_EPOCH + + # Check the the return condition + assert not spec.is_active_validator(state.validators[source_index], current_epoch) + + yield from run_switch_to_compounding_processing( + spec, state, consolidation, success=False + ) + + +@with_electra_and_later +@spec_state_test +def test_switch_to_compounding_source_bls_withdrawal_credential(spec, state): + # Set up a correct request, but source does have + # a bls withdrawal credential + current_epoch = spec.get_current_epoch(state) + source_index = spec.get_active_validator_indices(state, current_epoch)[0] + source_address = b"\x22" * 20 + 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 not spec.has_eth1_withdrawal_credential(state.validators[source_index]) + + yield from run_switch_to_compounding_processing( + spec, state, consolidation, success=False + ) + + +@with_electra_and_later +@spec_state_test +def test_switch_to_compounding_source_coumpounding_withdrawal_credential(spec, state): + # Set up a correct request, but source does have + # a compounding withdrawal credential and excess balance + current_epoch = spec.get_current_epoch(state) + source_index = spec.get_active_validator_indices(state, current_epoch)[0] + source_address = b"\x22" * 20 + consolidation = spec.ConsolidationRequest( + source_address=source_address, + source_pubkey=state.validators[source_index].pubkey, + target_pubkey=state.validators[source_index].pubkey, + ) + set_compounding_withdrawal_credential(spec, state, source_index) + state.balances[source_index] = spec.MIN_ACTIVATION_BALANCE + spec.EFFECTIVE_BALANCE_INCREMENT + + # Check the the return condition + assert not spec.has_eth1_withdrawal_credential(state.validators[source_index]) + + yield from run_switch_to_compounding_processing( + spec, state, consolidation, success=False + ) + + +@with_electra_and_later +@spec_state_test +def test_switch_to_compounding_not_authorized(spec, state): + # Set up an otherwise correct request + current_epoch = spec.get_current_epoch(state) + source_index = spec.get_active_validator_indices(state, current_epoch)[0] + source_address = b"\x22" * 20 + set_eth1_withdrawal_credential_with_balance( + spec, state, source_index, address=source_address + ) + # Make request with different source address + consolidation = spec.ConsolidationRequest( + source_address=b"\x33" * 20, + source_pubkey=state.validators[source_index].pubkey, + target_pubkey=state.validators[source_index].pubkey, + ) + + # Check the the return condition + assert not state.validators[source_index].withdrawal_credentials[12:] == consolidation.source_address + + yield from run_switch_to_compounding_processing( + spec, state, consolidation, success=False + ) + + +@with_electra_and_later +@spec_state_test +def test_switch_to_compounding_unknown_source_pubkey(spec, state): + # Set up an otherwise correct request + current_epoch = spec.get_current_epoch(state) + source_index = spec.get_active_validator_indices(state, current_epoch)[0] + source_address = b"\x22" * 20 + set_eth1_withdrawal_credential_with_balance( + spec, state, source_index, address=source_address + ) + # Make consolidation with different source pubkey + consolidation = spec.ConsolidationRequest( + source_address=source_address, + source_pubkey=b"\x00" * 48, + target_pubkey=b"\x00" * 48, + ) + + # Check the the return condition + assert not state.validators[source_index].pubkey == consolidation.source_pubkey + + yield from run_switch_to_compounding_processing( + spec, state, consolidation, success=False + ) + + def run_consolidation_processing(spec, state, consolidation, success=True): """ Run ``process_consolidation``, yielding: @@ -865,12 +1126,28 @@ def run_consolidation_processing(spec, state, consolidation, success=True): # 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) + assert spec.has_compounding_withdrawal_credential(state.validators[target_index]) # 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 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] # 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 == ( @@ -879,23 +1156,58 @@ def run_consolidation_processing(spec, state, consolidation, success=True): 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] + assert state.balances[target_index] == pre_target_balance + else: + assert pre_state == state + + +def run_switch_to_compounding_processing(spec, state, consolidation, success=True): + """ + Run ``process_consolidation``, yielding: + - pre-state ('pre') + - consolidation_request ('consolidation_request') + - post-state ('post'). + If ``success == False``, ``process_consolidation_request`` would return without any state change. + """ + + if success: + validator_pubkeys = [v.pubkey for v in state.validators] + source_index = spec.ValidatorIndex(validator_pubkeys.index(consolidation.source_pubkey)) + target_index = spec.ValidatorIndex(validator_pubkeys.index(consolidation.target_pubkey)) + source_validator = state.validators[source_index] + pre_exit_epoch = source_validator.exit_epoch + pre_pending_consolidations = state.pending_consolidations.copy() + pre_withdrawal_credentials = source_validator.withdrawal_credentials + pre_balance = state.balances[source_index] + else: + pre_state = state.copy() + + yield 'pre', state + yield 'consolidation_request', consolidation + + spec.process_consolidation_request(state, consolidation) + + yield 'post', state + + if success: + # Check that source and target are same + assert source_index == target_index + # Check that the credentials before the switch are of ETH1 type + assert pre_withdrawal_credentials[:1] == spec.ETH1_ADDRESS_WITHDRAWAL_PREFIX + # Check source address in the consolidation fits the withdrawal credentials + assert state.validators[source_index].withdrawal_credentials[12:] == consolidation.source_address + # Check that the source has switched to compounding + assert state.validators[source_index].withdrawal_credentials == ( + spec.COMPOUNDING_WITHDRAWAL_PREFIX + pre_withdrawal_credentials[1:] + ) + # Check excess balance is queued + assert state.balances[source_index] == spec.MIN_ACTIVATION_BALANCE + if pre_balance > spec.MIN_ACTIVATION_BALANCE: + assert state.pending_balance_deposits == [spec.PendingBalanceDeposit( + index=source_index, amount=(pre_balance - spec.MIN_ACTIVATION_BALANCE))] + # Check no consolidation has been initiated + assert state.validators[source_index].exit_epoch == spec.FAR_FUTURE_EPOCH + assert state.pending_consolidations == pre_pending_consolidations else: assert pre_state == state diff --git a/tests/core/pyspec/eth2spec/test/helpers/withdrawals.py b/tests/core/pyspec/eth2spec/test/helpers/withdrawals.py index 0ce476c86..6247c5476 100644 --- a/tests/core/pyspec/eth2spec/test/helpers/withdrawals.py +++ b/tests/core/pyspec/eth2spec/test/helpers/withdrawals.py @@ -30,7 +30,10 @@ def set_validator_fully_withdrawable(spec, state, index, withdrawable_epoch=None def set_eth1_withdrawal_credential_with_balance(spec, state, index, balance=None, address=None): if balance is None: - balance = spec.MAX_EFFECTIVE_BALANCE + if is_post_electra(spec): + balance = spec.MIN_ACTIVATION_BALANCE + else: + balance = spec.MAX_EFFECTIVE_BALANCE if address is None: address = b'\x11' * 20 From d71d9dda9e093b1421f2077be1ba4c316852811a Mon Sep 17 00:00:00 2001 From: Mikhail Kalinin Date: Wed, 18 Sep 2024 13:33:18 +0400 Subject: [PATCH 05/28] Fix lint --- .../block_processing/test_process_consolidation_request.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py b/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py index 4bc4617c8..3259bc2d1 100644 --- a/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py +++ b/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py @@ -1176,7 +1176,6 @@ def run_switch_to_compounding_processing(spec, state, consolidation, success=Tru source_index = spec.ValidatorIndex(validator_pubkeys.index(consolidation.source_pubkey)) target_index = spec.ValidatorIndex(validator_pubkeys.index(consolidation.target_pubkey)) source_validator = state.validators[source_index] - pre_exit_epoch = source_validator.exit_epoch pre_pending_consolidations = state.pending_consolidations.copy() pre_withdrawal_credentials = source_validator.withdrawal_credentials pre_balance = state.balances[source_index] From 23699f596da62980d650c63f9a47feec247c8f56 Mon Sep 17 00:00:00 2001 From: Mikhail Kalinin Date: Thu, 19 Sep 2024 10:44:38 +0400 Subject: [PATCH 06/28] Undo MAX_EB to MIN_AB switch in withdrawal helper --- tests/core/pyspec/eth2spec/test/helpers/withdrawals.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/core/pyspec/eth2spec/test/helpers/withdrawals.py b/tests/core/pyspec/eth2spec/test/helpers/withdrawals.py index 6247c5476..0ce476c86 100644 --- a/tests/core/pyspec/eth2spec/test/helpers/withdrawals.py +++ b/tests/core/pyspec/eth2spec/test/helpers/withdrawals.py @@ -30,10 +30,7 @@ def set_validator_fully_withdrawable(spec, state, index, withdrawable_epoch=None def set_eth1_withdrawal_credential_with_balance(spec, state, index, balance=None, address=None): if balance is None: - if is_post_electra(spec): - balance = spec.MIN_ACTIVATION_BALANCE - else: - balance = spec.MAX_EFFECTIVE_BALANCE + balance = spec.MAX_EFFECTIVE_BALANCE if address is None: address = b'\x11' * 20 From 83857264c2d423b48ad1a378ad6dc1f714611bfd Mon Sep 17 00:00:00 2001 From: terence tsao Date: Thu, 26 Sep 2024 14:16:24 -0700 Subject: [PATCH 07/28] Add a negative test for full exit has partial withdrawal --- .../test_process_withdrawal_request.py | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_withdrawal_request.py b/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_withdrawal_request.py index c216b297c..31ace5593 100644 --- a/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_withdrawal_request.py +++ b/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_withdrawal_request.py @@ -787,6 +787,41 @@ def test_partial_withdrawal_activation_epoch_less_than_shard_committee_period( ) +@with_electra_and_later +@spec_state_test +def test_full_exit_request_has_partial_withdrawal(spec, state): + rng = random.Random(1361) + # move state forward SHARD_COMMITTEE_PERIOD epochs to allow for exit + state.slot += spec.config.SHARD_COMMITTEE_PERIOD * spec.SLOTS_PER_EPOCH + + current_epoch = spec.get_current_epoch(state) + validator_index = rng.choice(spec.get_active_validator_indices(state, current_epoch)) + validator_pubkey = state.validators[validator_index].pubkey + address = b"\x22" * 20 + set_eth1_withdrawal_credential_with_balance( + spec, state, validator_index, address=address + ) + withdrawal_request = spec.WithdrawalRequest( + source_address=address, + validator_pubkey=validator_pubkey, + amount=spec.FULL_EXIT_REQUEST_AMOUNT, + ) + + # Validator can only be exited if there's no pending partial withdrawals in state + state.balances[validator_index] = spec.MAX_EFFECTIVE_BALANCE_ELECTRA + state.pending_partial_withdrawals.append( + spec.PendingPartialWithdrawal( + index=validator_index, + amount=1, + withdrawable_epoch=spec.compute_activation_exit_epoch(current_epoch), + ) + ) + + yield from run_withdrawal_request_processing( + spec, state, withdrawal_request, success=False + ) + + # # Run processing # From 2bc260470671a548de0f612fe2d8311a3ae528a8 Mon Sep 17 00:00:00 2001 From: Mikhail Kalinin Date: Fri, 27 Sep 2024 11:02:22 +0400 Subject: [PATCH 08/28] Apply suggestions from @jtraglia Co-authored-by: Justin Traglia <95511699+jtraglia@users.noreply.github.com> --- specs/electra/beacon-chain.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specs/electra/beacon-chain.md b/specs/electra/beacon-chain.md index 10c0eebf3..e89f5ccce 100644 --- a/specs/electra/beacon-chain.md +++ b/specs/electra/beacon-chain.md @@ -1426,7 +1426,7 @@ def is_valid_switch_to_compounding_request( if not is_active_validator(source_validator, current_epoch): return False - # Verify exit for source have not been initiated + # Verify exit for source has not been initiated if source_validator.exit_epoch != FAR_FUTURE_EPOCH: return False From e46ba7fed227d18d9cacea29444b52fb08f60c5f Mon Sep 17 00:00:00 2001 From: terence tsao Date: Fri, 27 Sep 2024 21:12:37 -0700 Subject: [PATCH 09/28] Fix insufficient effective bal test and add a bal test --- .../test_process_withdrawal_request.py | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_withdrawal_request.py b/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_withdrawal_request.py index c216b297c..587ba6058 100644 --- a/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_withdrawal_request.py +++ b/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_withdrawal_request.py @@ -658,6 +658,8 @@ def test_insufficient_effective_balance(spec, state): state.validators[ validator_index ].effective_balance -= spec.EFFECTIVE_BALANCE_INCREMENT + # Make sure validator has enough balance to withdraw + state.balances[validator_index] += spec.EFFECTIVE_BALANCE_INCREMENT set_compounding_withdrawal_credential(spec, state, validator_index, address=address) withdrawal_request = spec.WithdrawalRequest( @@ -787,6 +789,30 @@ def test_partial_withdrawal_activation_epoch_less_than_shard_committee_period( ) +def test_insufficient_balance(spec, state): + rng = random.Random(1361) + state.slot += spec.config.SHARD_COMMITTEE_PERIOD * spec.SLOTS_PER_EPOCH + current_epoch = spec.get_current_epoch(state) + validator_index = rng.choice(spec.get_active_validator_indices(state, current_epoch)) + validator_pubkey = state.validators[validator_index].pubkey + address = b"\x22" * 20 + amount = spec.EFFECTIVE_BALANCE_INCREMENT + + # Validator will not be able to partial withdrawal because MIN_ACTIVATION_BALANCE + amount > balance + set_compounding_withdrawal_credential(spec, state, validator_index, address=address) + withdrawal_request = spec.WithdrawalRequest( + source_address=address, + validator_pubkey=validator_pubkey, + amount=amount, + ) + + yield from run_withdrawal_request_processing( + spec, + state, + withdrawal_request, + success=False, + ) + # # Run processing # From 29552a78849b94254108bfb896f317ee3e4890f7 Mon Sep 17 00:00:00 2001 From: Justin Traglia <95511699+jtraglia@users.noreply.github.com> Date: Mon, 30 Sep 2024 09:28:07 -0500 Subject: [PATCH 10/28] Capitalize comment --- .../electra/block_processing/test_process_withdrawal_request.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_withdrawal_request.py b/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_withdrawal_request.py index 31ace5593..b29a6b908 100644 --- a/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_withdrawal_request.py +++ b/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_withdrawal_request.py @@ -791,7 +791,7 @@ def test_partial_withdrawal_activation_epoch_less_than_shard_committee_period( @spec_state_test def test_full_exit_request_has_partial_withdrawal(spec, state): rng = random.Random(1361) - # move state forward SHARD_COMMITTEE_PERIOD epochs to allow for exit + # Move state forward SHARD_COMMITTEE_PERIOD epochs to allow for exit state.slot += spec.config.SHARD_COMMITTEE_PERIOD * spec.SLOTS_PER_EPOCH current_epoch = spec.get_current_epoch(state) From a7b0d6f416eec4a54dab2eb4a8a318af052453a6 Mon Sep 17 00:00:00 2001 From: Mikhail Kalinin Date: Tue, 1 Oct 2024 15:37:53 +0400 Subject: [PATCH 11/28] Apply suggestions from code review Co-authored-by: Alex Stokes Co-authored-by: Justin Traglia <95511699+jtraglia@users.noreply.github.com> --- .../test_process_consolidation_request.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py b/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py index 3259bc2d1..cdaf786fe 100644 --- a/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py +++ b/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py @@ -572,7 +572,7 @@ def test_switch_to_compounding_with_pending_consolidations_at_limit(spec, state) ) -# Failing tests +# Tests that should fail @with_electra_and_later @with_presets([MINIMAL], "need sufficient consolidation churn limit") @@ -958,10 +958,10 @@ def test_switch_to_compounding_exited_source(spec, state): target_pubkey=state.validators[source_index].pubkey, ) - # exit source + # Initiate exit for source spec.initiate_validator_exit(state, source_index) - # Check the the return condition + # Check the return condition assert state.validators[source_index].exit_epoch != spec.FAR_FUTURE_EPOCH yield from run_switch_to_compounding_processing( @@ -985,7 +985,7 @@ def test_switch_to_compounding_inactive_source(spec, state): target_pubkey=state.validators[source_index].pubkey, ) - # set source validator as not yet activated + # Set source validator as not yet activated state.validators[source_index].activation_epoch = spec.FAR_FUTURE_EPOCH # Check the the return condition From a6294c6bd03db17a452aeb43c2d38bcbe20ba150 Mon Sep 17 00:00:00 2001 From: terence Date: Wed, 2 Oct 2024 05:45:23 -0700 Subject: [PATCH 12/28] Add a negative test for inactive validator for withdrawal request (#3945) --- .../test_process_withdrawal_request.py | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_withdrawal_request.py b/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_withdrawal_request.py index d2cee33d7..e3ebcae7e 100644 --- a/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_withdrawal_request.py +++ b/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_withdrawal_request.py @@ -850,6 +850,36 @@ def test_full_exit_request_has_partial_withdrawal(spec, state): spec, state, withdrawal_request, success=False ) + +@with_electra_and_later +@spec_state_test +def test_incorrect_inactive_validator(spec, state): + rng = random.Random(1361) + # move state forward SHARD_COMMITTEE_PERIOD epochs to allow for exit + state.slot += spec.config.SHARD_COMMITTEE_PERIOD * spec.SLOTS_PER_EPOCH + + current_epoch = spec.get_current_epoch(state) + validator_index = rng.choice(spec.get_active_validator_indices(state, current_epoch)) + validator_pubkey = state.validators[validator_index].pubkey + address = b"\x22" * 20 + incorrect_address = b"\x33" * 20 + set_eth1_withdrawal_credential_with_balance( + spec, state, validator_index, address=address + ) + withdrawal_request = spec.WithdrawalRequest( + source_address=incorrect_address, + validator_pubkey=validator_pubkey, + amount=spec.FULL_EXIT_REQUEST_AMOUNT, + ) + + # set validator as not yet activated + state.validators[validator_index].activation_epoch = spec.FAR_FUTURE_EPOCH + assert not spec.is_active_validator(state.validators[validator_index], current_epoch) + + yield from run_withdrawal_request_processing( + spec, state, withdrawal_request, success=False + ) + # # Run processing # From 578407a4c3ed5e21e818dec1722c8327d25e8ed9 Mon Sep 17 00:00:00 2001 From: Justin Traglia Date: Wed, 2 Oct 2024 12:56:47 -0500 Subject: [PATCH 13/28] Reject invalid DataColumnSidecar for zero blobs --- specs/_features/eip7594/p2p-interface.md | 7 + .../test/eip7594/unittests/test_networking.py | 147 +++++++++++++++++- 2 files changed, 153 insertions(+), 1 deletion(-) diff --git a/specs/_features/eip7594/p2p-interface.md b/specs/_features/eip7594/p2p-interface.md index 9087a8210..301d5c3dd 100644 --- a/specs/_features/eip7594/p2p-interface.md +++ b/specs/_features/eip7594/p2p-interface.md @@ -71,7 +71,11 @@ def verify_data_column_sidecar_kzg_proofs(sidecar: DataColumnSidecar) -> bool: """ Verify if the proofs are correct. """ + # A sidecar for zero blobs is invalid + assert len(sidecar.kzg_commitments) > 0 + # The sidecar index must be within the valid range assert sidecar.index < NUMBER_OF_COLUMNS + # There should be an equal number of cells/commitments/proofs assert len(sidecar.column) == len(sidecar.kzg_commitments) == len(sidecar.kzg_proofs) # The column index also represents the cell index @@ -93,6 +97,9 @@ def verify_data_column_sidecar_inclusion_proof(sidecar: DataColumnSidecar) -> bo """ Verify if the given KZG commitments included in the given beacon block. """ + # A sidecar for zero blobs is invalid + assert len(sidecar.kzg_commitments) > 0 + gindex = get_subtree_index(get_generalized_index(BeaconBlockBody, 'blob_kzg_commitments')) return is_valid_merkle_branch( leaf=hash_tree_root(sidecar.kzg_commitments), diff --git a/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py b/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py index 2ab52be6c..cad046ec5 100644 --- a/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py +++ b/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py @@ -1,8 +1,26 @@ +import functools +import random +import threading from eth2spec.test.context import ( - spec_test, + expect_assertion_error, single_phase, + spec_state_test, + spec_test, with_eip7594_and_later, ) +from eth2spec.debug.random_value import ( + RandomizationMode, + get_random_ssz_object, +) +from eth2spec.test.helpers.block import ( + sign_block, +) +from eth2spec.test.helpers.execution_payload import ( + compute_el_block_hash, +) +from eth2spec.test.helpers.sharding import ( + get_sample_opaque_tx, +) @with_eip7594_and_later @@ -17,3 +35,130 @@ def test_compute_subnet_for_data_column_sidecar(spec): # next one should be duplicate next_subnet = spec.compute_subnet_for_data_column_sidecar(spec.config.DATA_COLUMN_SIDECAR_SUBNET_COUNT) assert next_subnet == subnet_results[0] + + +@functools.cache +def compute_data_column_sidecars(spec, state): + rng = random.Random(5566) + opaque_tx, blobs, blob_kzg_commitments, _ = get_sample_opaque_tx(spec, blob_count=2) + block = get_random_ssz_object( + rng, + spec.BeaconBlock, + max_bytes_length=2000, + max_list_length=2000, + mode=RandomizationMode, + chaos=True, + ) + block.body.blob_kzg_commitments = blob_kzg_commitments + block.body.execution_payload.transactions = [opaque_tx] + block.body.execution_payload.block_hash = compute_el_block_hash(spec, block.body.execution_payload, state) + signed_block = sign_block(spec, state, block, proposer_index=0) + cells_and_kzg_proofs = [spec.compute_cells_and_kzg_proofs(blob) for blob in blobs] + return spec.get_data_column_sidecars(signed_block, cells_and_kzg_proofs) + + +# Necessary to cache the result of compute_data_column_sidecars(). +# So multiple tests do not attempt to compute the sidecars at the same time. +compute_data_column_sidecars_lock = threading.Lock() + + +@with_eip7594_and_later +@spec_state_test +@single_phase +def test_verify_data_column_sidecar_kzg_proofs__valid(spec, state): + with compute_data_column_sidecars_lock: + sidecar = compute_data_column_sidecars(spec, state)[0] + assert spec.verify_data_column_sidecar_kzg_proofs(sidecar) + + +@with_eip7594_and_later +@spec_state_test +@single_phase +def test_verify_data_column_sidecar_kzg_proofs__invalid_zero_blobs(spec, state): + with compute_data_column_sidecars_lock: + sidecar = compute_data_column_sidecars(spec, state)[0] + sidecar.column = [] + sidecar.kzg_commitments = [] + sidecar.kzg_proofs = [] + expect_assertion_error(lambda: spec.verify_data_column_sidecar_kzg_proofs(sidecar)) + + +@with_eip7594_and_later +@spec_state_test +@single_phase +def test_verify_data_column_sidecar_kzg_proofs_invalid__invalid_index(spec, state): + with compute_data_column_sidecars_lock: + sidecar = compute_data_column_sidecars(spec, state)[0] + sidecar.index = 128 + expect_assertion_error(lambda: spec.verify_data_column_sidecar_kzg_proofs(sidecar)) + + +@with_eip7594_and_later +@spec_state_test +@single_phase +def test_verify_data_column_sidecar_kzg_proofs_invalid__invalid_mismatch_len_column(spec, state): + with compute_data_column_sidecars_lock: + sidecar = compute_data_column_sidecars(spec, state)[0] + sidecar.column = sidecar.column[1:] + expect_assertion_error(lambda: spec.verify_data_column_sidecar_kzg_proofs(sidecar)) + + +@with_eip7594_and_later +@spec_state_test +@single_phase +def test_verify_data_column_sidecar_kzg_proofs_invalid__invalid_mismatch_len_kzg_commitments(spec, state): + with compute_data_column_sidecars_lock: + sidecar = compute_data_column_sidecars(spec, state)[0] + sidecar.kzg_commitments = sidecar.kzg_commitments[1:] + expect_assertion_error(lambda: spec.verify_data_column_sidecar_kzg_proofs(sidecar)) + + +@with_eip7594_and_later +@spec_state_test +@single_phase +def test_verify_data_column_sidecar_kzg_proofs_invalid__invalid_mismatch_len_kzg_proofs(spec, state): + with compute_data_column_sidecars_lock: + sidecar = compute_data_column_sidecars(spec, state)[0] + sidecar.kzg_proofs = sidecar.kzg_proofs[1:] + expect_assertion_error(lambda: spec.verify_data_column_sidecar_kzg_proofs(sidecar)) + + +@with_eip7594_and_later +@spec_state_test +@single_phase +def test_verify_data_column_sidecar_inclusion_proof__valid(spec, state): + with compute_data_column_sidecars_lock: + sidecar = compute_data_column_sidecars(spec, state)[0] + assert spec.verify_data_column_sidecar_inclusion_proof(sidecar) + + +@with_eip7594_and_later +@spec_state_test +@single_phase +def test_verify_data_column_sidecar_inclusion_proof__valid_but_missing_commitment(spec, state): + with compute_data_column_sidecars_lock: + sidecar = compute_data_column_sidecars(spec, state)[0] + sidecar.kzg_commitments = sidecar.kzg_commitments[1:] + assert not spec.verify_data_column_sidecar_inclusion_proof(sidecar) + + +@with_eip7594_and_later +@spec_state_test +@single_phase +def test_verify_data_column_sidecar_inclusion_proof__valid_but_duplicate_commitment(spec, state): + with compute_data_column_sidecars_lock: + sidecar = compute_data_column_sidecars(spec, state)[0] + sidecar.kzg_commitments = sidecar.kzg_commitments + [sidecar.kzg_commitments[0]] + assert not spec.verify_data_column_sidecar_inclusion_proof(sidecar) + + +@with_eip7594_and_later +@spec_state_test +@single_phase +def test_verify_data_column_sidecar_inclusion_proof__invalid_zero_blobs(spec, state): + with compute_data_column_sidecars_lock: + sidecar = compute_data_column_sidecars(spec, state)[0] + sidecar.column = [] + sidecar.kzg_commitments = [] + sidecar.kzg_proofs = [] + expect_assertion_error(lambda: spec.verify_data_column_sidecar_inclusion_proof(sidecar)) From 3f49e6c1557467377c56d11d25a7f94514b954e8 Mon Sep 17 00:00:00 2001 From: Justin Traglia Date: Wed, 2 Oct 2024 14:02:18 -0500 Subject: [PATCH 14/28] Deepcopy sidecar before modifying it --- .../test/eip7594/unittests/test_networking.py | 49 ++++++++++--------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py b/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py index cad046ec5..585508d2d 100644 --- a/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py +++ b/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py @@ -1,3 +1,4 @@ +import copy import functools import random import threading @@ -38,7 +39,7 @@ def test_compute_subnet_for_data_column_sidecar(spec): @functools.cache -def compute_data_column_sidecars(spec, state): +def compute_data_column_sidecar(spec, state): rng = random.Random(5566) opaque_tx, blobs, blob_kzg_commitments, _ = get_sample_opaque_tx(spec, blob_count=2) block = get_random_ssz_object( @@ -54,20 +55,20 @@ def compute_data_column_sidecars(spec, state): block.body.execution_payload.block_hash = compute_el_block_hash(spec, block.body.execution_payload, state) signed_block = sign_block(spec, state, block, proposer_index=0) cells_and_kzg_proofs = [spec.compute_cells_and_kzg_proofs(blob) for blob in blobs] - return spec.get_data_column_sidecars(signed_block, cells_and_kzg_proofs) + return spec.get_data_column_sidecars(signed_block, cells_and_kzg_proofs)[0] -# Necessary to cache the result of compute_data_column_sidecars(). +# Necessary to cache the result of compute_data_column_sidecar(). # So multiple tests do not attempt to compute the sidecars at the same time. -compute_data_column_sidecars_lock = threading.Lock() +compute_data_column_sidecar_lock = threading.Lock() @with_eip7594_and_later @spec_state_test @single_phase def test_verify_data_column_sidecar_kzg_proofs__valid(spec, state): - with compute_data_column_sidecars_lock: - sidecar = compute_data_column_sidecars(spec, state)[0] + with compute_data_column_sidecar_lock: + sidecar = copy.deepcopy(compute_data_column_sidecar(spec, state)) assert spec.verify_data_column_sidecar_kzg_proofs(sidecar) @@ -75,8 +76,8 @@ def test_verify_data_column_sidecar_kzg_proofs__valid(spec, state): @spec_state_test @single_phase def test_verify_data_column_sidecar_kzg_proofs__invalid_zero_blobs(spec, state): - with compute_data_column_sidecars_lock: - sidecar = compute_data_column_sidecars(spec, state)[0] + with compute_data_column_sidecar_lock: + sidecar = copy.deepcopy(compute_data_column_sidecar(spec, state)) sidecar.column = [] sidecar.kzg_commitments = [] sidecar.kzg_proofs = [] @@ -87,8 +88,8 @@ def test_verify_data_column_sidecar_kzg_proofs__invalid_zero_blobs(spec, state): @spec_state_test @single_phase def test_verify_data_column_sidecar_kzg_proofs_invalid__invalid_index(spec, state): - with compute_data_column_sidecars_lock: - sidecar = compute_data_column_sidecars(spec, state)[0] + with compute_data_column_sidecar_lock: + sidecar = copy.deepcopy(compute_data_column_sidecar(spec, state)) sidecar.index = 128 expect_assertion_error(lambda: spec.verify_data_column_sidecar_kzg_proofs(sidecar)) @@ -97,8 +98,8 @@ def test_verify_data_column_sidecar_kzg_proofs_invalid__invalid_index(spec, stat @spec_state_test @single_phase def test_verify_data_column_sidecar_kzg_proofs_invalid__invalid_mismatch_len_column(spec, state): - with compute_data_column_sidecars_lock: - sidecar = compute_data_column_sidecars(spec, state)[0] + with compute_data_column_sidecar_lock: + sidecar = copy.deepcopy(compute_data_column_sidecar(spec, state)) sidecar.column = sidecar.column[1:] expect_assertion_error(lambda: spec.verify_data_column_sidecar_kzg_proofs(sidecar)) @@ -107,8 +108,8 @@ def test_verify_data_column_sidecar_kzg_proofs_invalid__invalid_mismatch_len_col @spec_state_test @single_phase def test_verify_data_column_sidecar_kzg_proofs_invalid__invalid_mismatch_len_kzg_commitments(spec, state): - with compute_data_column_sidecars_lock: - sidecar = compute_data_column_sidecars(spec, state)[0] + with compute_data_column_sidecar_lock: + sidecar = copy.deepcopy(compute_data_column_sidecar(spec, state)) sidecar.kzg_commitments = sidecar.kzg_commitments[1:] expect_assertion_error(lambda: spec.verify_data_column_sidecar_kzg_proofs(sidecar)) @@ -117,8 +118,8 @@ def test_verify_data_column_sidecar_kzg_proofs_invalid__invalid_mismatch_len_kzg @spec_state_test @single_phase def test_verify_data_column_sidecar_kzg_proofs_invalid__invalid_mismatch_len_kzg_proofs(spec, state): - with compute_data_column_sidecars_lock: - sidecar = compute_data_column_sidecars(spec, state)[0] + with compute_data_column_sidecar_lock: + sidecar = copy.deepcopy(compute_data_column_sidecar(spec, state)) sidecar.kzg_proofs = sidecar.kzg_proofs[1:] expect_assertion_error(lambda: spec.verify_data_column_sidecar_kzg_proofs(sidecar)) @@ -127,8 +128,8 @@ def test_verify_data_column_sidecar_kzg_proofs_invalid__invalid_mismatch_len_kzg @spec_state_test @single_phase def test_verify_data_column_sidecar_inclusion_proof__valid(spec, state): - with compute_data_column_sidecars_lock: - sidecar = compute_data_column_sidecars(spec, state)[0] + with compute_data_column_sidecar_lock: + sidecar = copy.deepcopy(compute_data_column_sidecar(spec, state)) assert spec.verify_data_column_sidecar_inclusion_proof(sidecar) @@ -136,8 +137,8 @@ def test_verify_data_column_sidecar_inclusion_proof__valid(spec, state): @spec_state_test @single_phase def test_verify_data_column_sidecar_inclusion_proof__valid_but_missing_commitment(spec, state): - with compute_data_column_sidecars_lock: - sidecar = compute_data_column_sidecars(spec, state)[0] + with compute_data_column_sidecar_lock: + sidecar = copy.deepcopy(compute_data_column_sidecar(spec, state)) sidecar.kzg_commitments = sidecar.kzg_commitments[1:] assert not spec.verify_data_column_sidecar_inclusion_proof(sidecar) @@ -146,8 +147,8 @@ def test_verify_data_column_sidecar_inclusion_proof__valid_but_missing_commitmen @spec_state_test @single_phase def test_verify_data_column_sidecar_inclusion_proof__valid_but_duplicate_commitment(spec, state): - with compute_data_column_sidecars_lock: - sidecar = compute_data_column_sidecars(spec, state)[0] + with compute_data_column_sidecar_lock: + sidecar = copy.deepcopy(compute_data_column_sidecar(spec, state)) sidecar.kzg_commitments = sidecar.kzg_commitments + [sidecar.kzg_commitments[0]] assert not spec.verify_data_column_sidecar_inclusion_proof(sidecar) @@ -156,8 +157,8 @@ def test_verify_data_column_sidecar_inclusion_proof__valid_but_duplicate_commitm @spec_state_test @single_phase def test_verify_data_column_sidecar_inclusion_proof__invalid_zero_blobs(spec, state): - with compute_data_column_sidecars_lock: - sidecar = compute_data_column_sidecars(spec, state)[0] + with compute_data_column_sidecar_lock: + sidecar = copy.deepcopy(compute_data_column_sidecar(spec, state)) sidecar.column = [] sidecar.kzg_commitments = [] sidecar.kzg_proofs = [] From 4578b59ab7334fd10ff548e0c774d7159b6c319f Mon Sep 17 00:00:00 2001 From: Justin Traglia Date: Wed, 2 Oct 2024 14:39:09 -0500 Subject: [PATCH 15/28] Use wrapper function which does the deepcopy --- .../test/eip7594/unittests/test_networking.py | 26 +++++++++++-------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py b/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py index 585508d2d..e91364c64 100644 --- a/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py +++ b/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py @@ -39,7 +39,7 @@ def test_compute_subnet_for_data_column_sidecar(spec): @functools.cache -def compute_data_column_sidecar(spec, state): +def _compute_data_column_sidecar(spec, state): rng = random.Random(5566) opaque_tx, blobs, blob_kzg_commitments, _ = get_sample_opaque_tx(spec, blob_count=2) block = get_random_ssz_object( @@ -57,6 +57,10 @@ def compute_data_column_sidecar(spec, state): cells_and_kzg_proofs = [spec.compute_cells_and_kzg_proofs(blob) for blob in blobs] return spec.get_data_column_sidecars(signed_block, cells_and_kzg_proofs)[0] +def compute_data_column_sidecar(spec, state): + """This function returns a copy of a cached data column sidecar.""" + return copy.deepcopy(_compute_data_column_sidecar(spec, state)) + # Necessary to cache the result of compute_data_column_sidecar(). # So multiple tests do not attempt to compute the sidecars at the same time. @@ -68,7 +72,7 @@ compute_data_column_sidecar_lock = threading.Lock() @single_phase def test_verify_data_column_sidecar_kzg_proofs__valid(spec, state): with compute_data_column_sidecar_lock: - sidecar = copy.deepcopy(compute_data_column_sidecar(spec, state)) + sidecar = compute_data_column_sidecar(spec, state) assert spec.verify_data_column_sidecar_kzg_proofs(sidecar) @@ -77,7 +81,7 @@ def test_verify_data_column_sidecar_kzg_proofs__valid(spec, state): @single_phase def test_verify_data_column_sidecar_kzg_proofs__invalid_zero_blobs(spec, state): with compute_data_column_sidecar_lock: - sidecar = copy.deepcopy(compute_data_column_sidecar(spec, state)) + sidecar = compute_data_column_sidecar(spec, state) sidecar.column = [] sidecar.kzg_commitments = [] sidecar.kzg_proofs = [] @@ -89,7 +93,7 @@ def test_verify_data_column_sidecar_kzg_proofs__invalid_zero_blobs(spec, state): @single_phase def test_verify_data_column_sidecar_kzg_proofs_invalid__invalid_index(spec, state): with compute_data_column_sidecar_lock: - sidecar = copy.deepcopy(compute_data_column_sidecar(spec, state)) + sidecar = compute_data_column_sidecar(spec, state) sidecar.index = 128 expect_assertion_error(lambda: spec.verify_data_column_sidecar_kzg_proofs(sidecar)) @@ -99,7 +103,7 @@ def test_verify_data_column_sidecar_kzg_proofs_invalid__invalid_index(spec, stat @single_phase def test_verify_data_column_sidecar_kzg_proofs_invalid__invalid_mismatch_len_column(spec, state): with compute_data_column_sidecar_lock: - sidecar = copy.deepcopy(compute_data_column_sidecar(spec, state)) + sidecar = compute_data_column_sidecar(spec, state) sidecar.column = sidecar.column[1:] expect_assertion_error(lambda: spec.verify_data_column_sidecar_kzg_proofs(sidecar)) @@ -109,7 +113,7 @@ def test_verify_data_column_sidecar_kzg_proofs_invalid__invalid_mismatch_len_col @single_phase def test_verify_data_column_sidecar_kzg_proofs_invalid__invalid_mismatch_len_kzg_commitments(spec, state): with compute_data_column_sidecar_lock: - sidecar = copy.deepcopy(compute_data_column_sidecar(spec, state)) + sidecar = compute_data_column_sidecar(spec, state) sidecar.kzg_commitments = sidecar.kzg_commitments[1:] expect_assertion_error(lambda: spec.verify_data_column_sidecar_kzg_proofs(sidecar)) @@ -119,7 +123,7 @@ def test_verify_data_column_sidecar_kzg_proofs_invalid__invalid_mismatch_len_kzg @single_phase def test_verify_data_column_sidecar_kzg_proofs_invalid__invalid_mismatch_len_kzg_proofs(spec, state): with compute_data_column_sidecar_lock: - sidecar = copy.deepcopy(compute_data_column_sidecar(spec, state)) + sidecar = compute_data_column_sidecar(spec, state) sidecar.kzg_proofs = sidecar.kzg_proofs[1:] expect_assertion_error(lambda: spec.verify_data_column_sidecar_kzg_proofs(sidecar)) @@ -129,7 +133,7 @@ def test_verify_data_column_sidecar_kzg_proofs_invalid__invalid_mismatch_len_kzg @single_phase def test_verify_data_column_sidecar_inclusion_proof__valid(spec, state): with compute_data_column_sidecar_lock: - sidecar = copy.deepcopy(compute_data_column_sidecar(spec, state)) + sidecar = compute_data_column_sidecar(spec, state) assert spec.verify_data_column_sidecar_inclusion_proof(sidecar) @@ -138,7 +142,7 @@ def test_verify_data_column_sidecar_inclusion_proof__valid(spec, state): @single_phase def test_verify_data_column_sidecar_inclusion_proof__valid_but_missing_commitment(spec, state): with compute_data_column_sidecar_lock: - sidecar = copy.deepcopy(compute_data_column_sidecar(spec, state)) + sidecar = compute_data_column_sidecar(spec, state) sidecar.kzg_commitments = sidecar.kzg_commitments[1:] assert not spec.verify_data_column_sidecar_inclusion_proof(sidecar) @@ -148,7 +152,7 @@ def test_verify_data_column_sidecar_inclusion_proof__valid_but_missing_commitmen @single_phase def test_verify_data_column_sidecar_inclusion_proof__valid_but_duplicate_commitment(spec, state): with compute_data_column_sidecar_lock: - sidecar = copy.deepcopy(compute_data_column_sidecar(spec, state)) + sidecar = compute_data_column_sidecar(spec, state) sidecar.kzg_commitments = sidecar.kzg_commitments + [sidecar.kzg_commitments[0]] assert not spec.verify_data_column_sidecar_inclusion_proof(sidecar) @@ -158,7 +162,7 @@ def test_verify_data_column_sidecar_inclusion_proof__valid_but_duplicate_commitm @single_phase def test_verify_data_column_sidecar_inclusion_proof__invalid_zero_blobs(spec, state): with compute_data_column_sidecar_lock: - sidecar = copy.deepcopy(compute_data_column_sidecar(spec, state)) + sidecar = compute_data_column_sidecar(spec, state) sidecar.column = [] sidecar.kzg_commitments = [] sidecar.kzg_proofs = [] From df987b5e50960969718a2ca1d67d29c1f36ee709 Mon Sep 17 00:00:00 2001 From: Justin Traglia Date: Wed, 2 Oct 2024 15:47:42 -0500 Subject: [PATCH 16/28] Create new verify_data_column_sidecar() function --- specs/_features/eip7594/p2p-interface.md | 22 ++- .../test/eip7594/unittests/test_networking.py | 175 ++++++++++-------- 2 files changed, 110 insertions(+), 87 deletions(-) diff --git a/specs/_features/eip7594/p2p-interface.md b/specs/_features/eip7594/p2p-interface.md index 301d5c3dd..a73a95542 100644 --- a/specs/_features/eip7594/p2p-interface.md +++ b/specs/_features/eip7594/p2p-interface.md @@ -14,6 +14,7 @@ - [Containers](#containers) - [`DataColumnIdentifier`](#datacolumnidentifier) - [Helpers](#helpers) + - [`verify_data_column_sidecar`](#verify_data_column_sidecar) - [`verify_data_column_sidecar_kzg_proofs`](#verify_data_column_sidecar_kzg_proofs) - [`verify_data_column_sidecar_inclusion_proof`](#verify_data_column_sidecar_inclusion_proof) - [`compute_subnet_for_data_column_sidecar`](#compute_subnet_for_data_column_sidecar) @@ -64,12 +65,12 @@ class DataColumnIdentifier(Container): ### Helpers -##### `verify_data_column_sidecar_kzg_proofs` +##### `verify_data_column_sidecar` ```python -def verify_data_column_sidecar_kzg_proofs(sidecar: DataColumnSidecar) -> bool: +def verify_data_column_sidecar(sidecar: DataColumnSidecar) -> bool: """ - Verify if the proofs are correct. + Verify if the data column sidecar is valid. """ # A sidecar for zero blobs is invalid assert len(sidecar.kzg_commitments) > 0 @@ -78,6 +79,16 @@ def verify_data_column_sidecar_kzg_proofs(sidecar: DataColumnSidecar) -> bool: # There should be an equal number of cells/commitments/proofs assert len(sidecar.column) == len(sidecar.kzg_commitments) == len(sidecar.kzg_proofs) + return True +``` + +##### `verify_data_column_sidecar_kzg_proofs` + +```python +def verify_data_column_sidecar_kzg_proofs(sidecar: DataColumnSidecar) -> bool: + """ + Verify if the proofs are correct. + """ # The column index also represents the cell index cell_indices = [CellIndex(sidecar.index)] * len(sidecar.column) @@ -97,9 +108,6 @@ def verify_data_column_sidecar_inclusion_proof(sidecar: DataColumnSidecar) -> bo """ Verify if the given KZG commitments included in the given beacon block. """ - # A sidecar for zero blobs is invalid - assert len(sidecar.kzg_commitments) > 0 - gindex = get_subtree_index(get_generalized_index(BeaconBlockBody, 'blob_kzg_commitments')) return is_valid_merkle_branch( leaf=hash_tree_root(sidecar.kzg_commitments), @@ -155,7 +163,7 @@ The *type* of the payload of this topic is `DataColumnSidecar`. The following validations MUST pass before forwarding the `sidecar: DataColumnSidecar` on the network, assuming the alias `block_header = sidecar.signed_block_header.message`: -- _[REJECT]_ The sidecar's index is consistent with `NUMBER_OF_COLUMNS` -- i.e. `sidecar.index < NUMBER_OF_COLUMNS`. +- _[REJECT]_ The sidecar is valid as verified by `verify_data_column_sidecar(sidecar)`. - _[REJECT]_ The sidecar is for the correct subnet -- i.e. `compute_subnet_for_data_column_sidecar(sidecar.index) == subnet_id`. - _[IGNORE]_ The sidecar is not from a future slot (with a `MAXIMUM_GOSSIP_CLOCK_DISPARITY` allowance) -- i.e. validate that `block_header.slot <= current_slot` (a client MAY queue future sidecars for processing at the appropriate slot). - _[IGNORE]_ The sidecar is from a slot greater than the latest finalized slot -- i.e. validate that `block_header.slot > compute_start_slot_at_epoch(state.finalized_checkpoint.epoch)` diff --git a/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py b/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py index e91364c64..3c3b40013 100644 --- a/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py +++ b/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py @@ -1,7 +1,4 @@ -import copy -import functools import random -import threading from eth2spec.test.context import ( expect_assertion_error, single_phase, @@ -24,22 +21,10 @@ from eth2spec.test.helpers.sharding import ( ) -@with_eip7594_and_later -@spec_test -@single_phase -def test_compute_subnet_for_data_column_sidecar(spec): - subnet_results = [] - for column_index in range(spec.config.DATA_COLUMN_SIDECAR_SUBNET_COUNT): - subnet_results.append(spec.compute_subnet_for_data_column_sidecar(column_index)) - # no duplicates - assert len(subnet_results) == len(set(subnet_results)) - # next one should be duplicate - next_subnet = spec.compute_subnet_for_data_column_sidecar(spec.config.DATA_COLUMN_SIDECAR_SUBNET_COUNT) - assert next_subnet == subnet_results[0] +# Helper functions -@functools.cache -def _compute_data_column_sidecar(spec, state): +def compute_data_column_sidecar(spec, state): rng = random.Random(5566) opaque_tx, blobs, blob_kzg_commitments, _ = get_sample_opaque_tx(spec, blob_count=2) block = get_random_ssz_object( @@ -57,113 +42,143 @@ def _compute_data_column_sidecar(spec, state): cells_and_kzg_proofs = [spec.compute_cells_and_kzg_proofs(blob) for blob in blobs] return spec.get_data_column_sidecars(signed_block, cells_and_kzg_proofs)[0] -def compute_data_column_sidecar(spec, state): - """This function returns a copy of a cached data column sidecar.""" - return copy.deepcopy(_compute_data_column_sidecar(spec, state)) + +# Tests for verify_data_column_sidecar + +@with_eip7594_and_later +@spec_state_test +@single_phase +def test_verify_data_column_sidecar__valid(spec, state): + sidecar = compute_data_column_sidecar(spec, state) + assert spec.verify_data_column_sidecar(sidecar) -# Necessary to cache the result of compute_data_column_sidecar(). -# So multiple tests do not attempt to compute the sidecars at the same time. -compute_data_column_sidecar_lock = threading.Lock() +@with_eip7594_and_later +@spec_state_test +@single_phase +def test_verify_data_column_sidecar__invalid_zero_blobs(spec, state): + sidecar = compute_data_column_sidecar(spec, state) + sidecar.column = [] + sidecar.kzg_commitments = [] + sidecar.kzg_proofs = [] + expect_assertion_error(lambda: spec.verify_data_column_sidecar(sidecar)) + + +@with_eip7594_and_later +@spec_state_test +@single_phase +def test_verify_data_column_sidecar__invalid_index(spec, state): + sidecar = compute_data_column_sidecar(spec, state) + sidecar.index = 128 + expect_assertion_error(lambda: spec.verify_data_column_sidecar(sidecar)) + + +@with_eip7594_and_later +@spec_state_test +@single_phase +def test_verify_data_column_sidecar__invalid_mismatch_len_column(spec, state): + sidecar = compute_data_column_sidecar(spec, state) + sidecar.column = sidecar.column[1:] + expect_assertion_error(lambda: spec.verify_data_column_sidecar(sidecar)) + + +@with_eip7594_and_later +@spec_state_test +@single_phase +def test_verify_data_column_sidecar__invalid_mismatch_len_kzg_commitments(spec, state): + sidecar = compute_data_column_sidecar(spec, state) + sidecar.kzg_commitments = sidecar.kzg_commitments[1:] + expect_assertion_error(lambda: spec.verify_data_column_sidecar_kzg_proofs(sidecar)) + + +@with_eip7594_and_later +@spec_state_test +@single_phase +def test_verify_data_column_sidecars__invalid_mismatch_len_kzg_proofs(spec, state): + sidecar = compute_data_column_sidecar(spec, state) + sidecar.kzg_proofs = sidecar.kzg_proofs[1:] + expect_assertion_error(lambda: spec.verify_data_column_sidecar_kzg_proofs(sidecar)) + + +# Tests for verify_data_column_sidecar_kzg_proofs @with_eip7594_and_later @spec_state_test @single_phase def test_verify_data_column_sidecar_kzg_proofs__valid(spec, state): - with compute_data_column_sidecar_lock: - sidecar = compute_data_column_sidecar(spec, state) - assert spec.verify_data_column_sidecar_kzg_proofs(sidecar) + sidecar = compute_data_column_sidecar(spec, state) + assert spec.verify_data_column_sidecar_kzg_proofs(sidecar) @with_eip7594_and_later @spec_state_test @single_phase -def test_verify_data_column_sidecar_kzg_proofs__invalid_zero_blobs(spec, state): - with compute_data_column_sidecar_lock: - sidecar = compute_data_column_sidecar(spec, state) - sidecar.column = [] - sidecar.kzg_commitments = [] - sidecar.kzg_proofs = [] - expect_assertion_error(lambda: spec.verify_data_column_sidecar_kzg_proofs(sidecar)) +def test_verify_data_column_sidecar_kzg_proofs__valid_but_wrong_column(spec, state): + sidecar = compute_data_column_sidecar(spec, state) + sidecar.column[0] = sidecar.column[1] + assert not spec.verify_data_column_sidecar_kzg_proofs(sidecar) @with_eip7594_and_later @spec_state_test @single_phase -def test_verify_data_column_sidecar_kzg_proofs_invalid__invalid_index(spec, state): - with compute_data_column_sidecar_lock: - sidecar = compute_data_column_sidecar(spec, state) - sidecar.index = 128 - expect_assertion_error(lambda: spec.verify_data_column_sidecar_kzg_proofs(sidecar)) +def test_verify_data_column_sidecar_kzg_proofs__valid_but_wrong_commitment(spec, state): + sidecar = compute_data_column_sidecar(spec, state) + sidecar.kzg_commitments[0] = sidecar.kzg_commitments[1] + assert not spec.verify_data_column_sidecar_kzg_proofs(sidecar) @with_eip7594_and_later @spec_state_test @single_phase -def test_verify_data_column_sidecar_kzg_proofs_invalid__invalid_mismatch_len_column(spec, state): - with compute_data_column_sidecar_lock: - sidecar = compute_data_column_sidecar(spec, state) - sidecar.column = sidecar.column[1:] - expect_assertion_error(lambda: spec.verify_data_column_sidecar_kzg_proofs(sidecar)) +def test_verify_data_column_sidecar_kzg_proofs__valid_but_wrong_proof(spec, state): + sidecar = compute_data_column_sidecar(spec, state) + sidecar.kzg_proofs[0] = sidecar.kzg_proofs[1] + assert not spec.verify_data_column_sidecar_kzg_proofs(sidecar) -@with_eip7594_and_later -@spec_state_test -@single_phase -def test_verify_data_column_sidecar_kzg_proofs_invalid__invalid_mismatch_len_kzg_commitments(spec, state): - with compute_data_column_sidecar_lock: - sidecar = compute_data_column_sidecar(spec, state) - sidecar.kzg_commitments = sidecar.kzg_commitments[1:] - expect_assertion_error(lambda: spec.verify_data_column_sidecar_kzg_proofs(sidecar)) - - -@with_eip7594_and_later -@spec_state_test -@single_phase -def test_verify_data_column_sidecar_kzg_proofs_invalid__invalid_mismatch_len_kzg_proofs(spec, state): - with compute_data_column_sidecar_lock: - sidecar = compute_data_column_sidecar(spec, state) - sidecar.kzg_proofs = sidecar.kzg_proofs[1:] - expect_assertion_error(lambda: spec.verify_data_column_sidecar_kzg_proofs(sidecar)) +# Tests for verify_data_column_sidecar_inclusion_proof @with_eip7594_and_later @spec_state_test @single_phase def test_verify_data_column_sidecar_inclusion_proof__valid(spec, state): - with compute_data_column_sidecar_lock: - sidecar = compute_data_column_sidecar(spec, state) - assert spec.verify_data_column_sidecar_inclusion_proof(sidecar) + sidecar = compute_data_column_sidecar(spec, state) + assert spec.verify_data_column_sidecar_inclusion_proof(sidecar) @with_eip7594_and_later @spec_state_test @single_phase def test_verify_data_column_sidecar_inclusion_proof__valid_but_missing_commitment(spec, state): - with compute_data_column_sidecar_lock: - sidecar = compute_data_column_sidecar(spec, state) - sidecar.kzg_commitments = sidecar.kzg_commitments[1:] - assert not spec.verify_data_column_sidecar_inclusion_proof(sidecar) + sidecar = compute_data_column_sidecar(spec, state) + sidecar.kzg_commitments = sidecar.kzg_commitments[1:] + assert not spec.verify_data_column_sidecar_inclusion_proof(sidecar) @with_eip7594_and_later @spec_state_test @single_phase def test_verify_data_column_sidecar_inclusion_proof__valid_but_duplicate_commitment(spec, state): - with compute_data_column_sidecar_lock: - sidecar = compute_data_column_sidecar(spec, state) - sidecar.kzg_commitments = sidecar.kzg_commitments + [sidecar.kzg_commitments[0]] - assert not spec.verify_data_column_sidecar_inclusion_proof(sidecar) + sidecar = compute_data_column_sidecar(spec, state) + sidecar.kzg_commitments = sidecar.kzg_commitments + [sidecar.kzg_commitments[0]] + assert not spec.verify_data_column_sidecar_inclusion_proof(sidecar) + + +# Tests for compute_subnet_for_data_column_sidecar @with_eip7594_and_later -@spec_state_test +@spec_test @single_phase -def test_verify_data_column_sidecar_inclusion_proof__invalid_zero_blobs(spec, state): - with compute_data_column_sidecar_lock: - sidecar = compute_data_column_sidecar(spec, state) - sidecar.column = [] - sidecar.kzg_commitments = [] - sidecar.kzg_proofs = [] - expect_assertion_error(lambda: spec.verify_data_column_sidecar_inclusion_proof(sidecar)) +def test_compute_subnet_for_data_column_sidecar(spec): + subnet_results = [] + for column_index in range(spec.config.DATA_COLUMN_SIDECAR_SUBNET_COUNT): + subnet_results.append(spec.compute_subnet_for_data_column_sidecar(column_index)) + # no duplicates + assert len(subnet_results) == len(set(subnet_results)) + # next one should be duplicate + next_subnet = spec.compute_subnet_for_data_column_sidecar(spec.config.DATA_COLUMN_SIDECAR_SUBNET_COUNT) + assert next_subnet == subnet_results[0] From 09cc20459be20df3658b56147bced663022c78e0 Mon Sep 17 00:00:00 2001 From: Justin Traglia Date: Wed, 2 Oct 2024 15:53:42 -0500 Subject: [PATCH 17/28] Add an extra blank line for consistency --- .../pyspec/eth2spec/test/eip7594/unittests/test_networking.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py b/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py index 3c3b40013..3797a4ecd 100644 --- a/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py +++ b/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py @@ -45,6 +45,7 @@ def compute_data_column_sidecar(spec, state): # Tests for verify_data_column_sidecar + @with_eip7594_and_later @spec_state_test @single_phase From e02cbab59f007a7541671c497bd8a226f3fb4a4f Mon Sep 17 00:00:00 2001 From: Justin Traglia Date: Wed, 2 Oct 2024 15:55:16 -0500 Subject: [PATCH 18/28] Move index check to top --- specs/_features/eip7594/p2p-interface.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/specs/_features/eip7594/p2p-interface.md b/specs/_features/eip7594/p2p-interface.md index a73a95542..712845c56 100644 --- a/specs/_features/eip7594/p2p-interface.md +++ b/specs/_features/eip7594/p2p-interface.md @@ -72,10 +72,10 @@ def verify_data_column_sidecar(sidecar: DataColumnSidecar) -> bool: """ Verify if the data column sidecar is valid. """ - # A sidecar for zero blobs is invalid - assert len(sidecar.kzg_commitments) > 0 # The sidecar index must be within the valid range assert sidecar.index < NUMBER_OF_COLUMNS + # A sidecar for zero blobs is invalid + assert len(sidecar.kzg_commitments) > 0 # There should be an equal number of cells/commitments/proofs assert len(sidecar.column) == len(sidecar.kzg_commitments) == len(sidecar.kzg_proofs) From 3984bd36041bb974ad22cc624b82d88b6c30e601 Mon Sep 17 00:00:00 2001 From: Justin Traglia Date: Wed, 2 Oct 2024 16:02:39 -0500 Subject: [PATCH 19/28] Convert assert to return False --- specs/_features/eip7594/p2p-interface.md | 11 ++++++++--- .../test/eip7594/unittests/test_networking.py | 11 +++++------ 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/specs/_features/eip7594/p2p-interface.md b/specs/_features/eip7594/p2p-interface.md index 712845c56..3dd199425 100644 --- a/specs/_features/eip7594/p2p-interface.md +++ b/specs/_features/eip7594/p2p-interface.md @@ -73,11 +73,16 @@ def verify_data_column_sidecar(sidecar: DataColumnSidecar) -> bool: Verify if the data column sidecar is valid. """ # The sidecar index must be within the valid range - assert sidecar.index < NUMBER_OF_COLUMNS + if sidecar.index >= NUMBER_OF_COLUMNS + return False + # A sidecar for zero blobs is invalid - assert len(sidecar.kzg_commitments) > 0 + if len(sidecar.kzg_commitments) == 0: + return False + # There should be an equal number of cells/commitments/proofs - assert len(sidecar.column) == len(sidecar.kzg_commitments) == len(sidecar.kzg_proofs) + if len(sidecar.column) != len(sidecar.kzg_commitments) or len(sidecar.column) != len(sidecar.kzg_proofs): + return False return True ``` diff --git a/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py b/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py index 3797a4ecd..a818c746f 100644 --- a/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py +++ b/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py @@ -1,6 +1,5 @@ import random from eth2spec.test.context import ( - expect_assertion_error, single_phase, spec_state_test, spec_test, @@ -62,7 +61,7 @@ def test_verify_data_column_sidecar__invalid_zero_blobs(spec, state): sidecar.column = [] sidecar.kzg_commitments = [] sidecar.kzg_proofs = [] - expect_assertion_error(lambda: spec.verify_data_column_sidecar(sidecar)) + assert not spec.verify_data_column_sidecar(sidecar) @with_eip7594_and_later @@ -71,7 +70,7 @@ def test_verify_data_column_sidecar__invalid_zero_blobs(spec, state): def test_verify_data_column_sidecar__invalid_index(spec, state): sidecar = compute_data_column_sidecar(spec, state) sidecar.index = 128 - expect_assertion_error(lambda: spec.verify_data_column_sidecar(sidecar)) + assert not spec.verify_data_column_sidecar(sidecar) @with_eip7594_and_later @@ -80,7 +79,7 @@ def test_verify_data_column_sidecar__invalid_index(spec, state): def test_verify_data_column_sidecar__invalid_mismatch_len_column(spec, state): sidecar = compute_data_column_sidecar(spec, state) sidecar.column = sidecar.column[1:] - expect_assertion_error(lambda: spec.verify_data_column_sidecar(sidecar)) + assert not spec.verify_data_column_sidecar(sidecar) @with_eip7594_and_later @@ -89,7 +88,7 @@ def test_verify_data_column_sidecar__invalid_mismatch_len_column(spec, state): def test_verify_data_column_sidecar__invalid_mismatch_len_kzg_commitments(spec, state): sidecar = compute_data_column_sidecar(spec, state) sidecar.kzg_commitments = sidecar.kzg_commitments[1:] - expect_assertion_error(lambda: spec.verify_data_column_sidecar_kzg_proofs(sidecar)) + assert not spec.verify_data_column_sidecar(sidecar) @with_eip7594_and_later @@ -98,7 +97,7 @@ def test_verify_data_column_sidecar__invalid_mismatch_len_kzg_commitments(spec, def test_verify_data_column_sidecars__invalid_mismatch_len_kzg_proofs(spec, state): sidecar = compute_data_column_sidecar(spec, state) sidecar.kzg_proofs = sidecar.kzg_proofs[1:] - expect_assertion_error(lambda: spec.verify_data_column_sidecar_kzg_proofs(sidecar)) + assert not spec.verify_data_column_sidecar(sidecar) # Tests for verify_data_column_sidecar_kzg_proofs From 3196487a689056d7d26ef1ed6d7daf4414d70658 Mon Sep 17 00:00:00 2001 From: Justin Traglia Date: Wed, 2 Oct 2024 16:05:10 -0500 Subject: [PATCH 20/28] Add missing semicolon --- specs/_features/eip7594/p2p-interface.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specs/_features/eip7594/p2p-interface.md b/specs/_features/eip7594/p2p-interface.md index 3dd199425..710b9f482 100644 --- a/specs/_features/eip7594/p2p-interface.md +++ b/specs/_features/eip7594/p2p-interface.md @@ -73,7 +73,7 @@ def verify_data_column_sidecar(sidecar: DataColumnSidecar) -> bool: Verify if the data column sidecar is valid. """ # The sidecar index must be within the valid range - if sidecar.index >= NUMBER_OF_COLUMNS + if sidecar.index >= NUMBER_OF_COLUMNS: return False # A sidecar for zero blobs is invalid From 2ceb0fd74da30f081caedd6c136646ef2d367030 Mon Sep 17 00:00:00 2001 From: Justin Traglia Date: Wed, 2 Oct 2024 16:51:46 -0500 Subject: [PATCH 21/28] Fix some nits --- specs/_features/eip7594/p2p-interface.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/specs/_features/eip7594/p2p-interface.md b/specs/_features/eip7594/p2p-interface.md index 710b9f482..4c4ae83ef 100644 --- a/specs/_features/eip7594/p2p-interface.md +++ b/specs/_features/eip7594/p2p-interface.md @@ -80,7 +80,7 @@ def verify_data_column_sidecar(sidecar: DataColumnSidecar) -> bool: if len(sidecar.kzg_commitments) == 0: return False - # There should be an equal number of cells/commitments/proofs + # The column length must be equal to the number of commitments/proofs if len(sidecar.column) != len(sidecar.kzg_commitments) or len(sidecar.column) != len(sidecar.kzg_proofs): return False @@ -92,7 +92,7 @@ def verify_data_column_sidecar(sidecar: DataColumnSidecar) -> bool: ```python def verify_data_column_sidecar_kzg_proofs(sidecar: DataColumnSidecar) -> bool: """ - Verify if the proofs are correct. + Verify if the KZG proofs are correct. """ # The column index also represents the cell index cell_indices = [CellIndex(sidecar.index)] * len(sidecar.column) From 3e80dc8ed02000ab9f3faee036d1fe565461d5eb Mon Sep 17 00:00:00 2001 From: Justin Traglia Date: Thu, 3 Oct 2024 07:42:45 -0500 Subject: [PATCH 22/28] Add new verification to is_data_available --- specs/_features/eip7594/fork-choice.md | 1 + 1 file changed, 1 insertion(+) diff --git a/specs/_features/eip7594/fork-choice.md b/specs/_features/eip7594/fork-choice.md index a6ddf0508..d40d13419 100644 --- a/specs/_features/eip7594/fork-choice.md +++ b/specs/_features/eip7594/fork-choice.md @@ -31,6 +31,7 @@ def is_data_available(beacon_block_root: Root) -> bool: # `MIN_EPOCHS_FOR_DATA_COLUMN_SIDECARS_REQUESTS` epochs. column_sidecars = retrieve_column_sidecars(beacon_block_root) return all( + verify_data_column_sidecar(column_sidecar) and verify_data_column_sidecar_kzg_proofs(column_sidecar) for column_sidecar in column_sidecars ) From 1329410f0ec1506ac3fef69696b164563d585218 Mon Sep 17 00:00:00 2001 From: Justin Traglia <95511699+jtraglia@users.noreply.github.com> Date: Thu, 3 Oct 2024 07:44:25 -0500 Subject: [PATCH 23/28] Bump version to 1.5.0-alpha.7 (#3955) --- tests/core/pyspec/eth2spec/VERSION.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/core/pyspec/eth2spec/VERSION.txt b/tests/core/pyspec/eth2spec/VERSION.txt index f49b638a2..d1cdd9f1e 100644 --- a/tests/core/pyspec/eth2spec/VERSION.txt +++ b/tests/core/pyspec/eth2spec/VERSION.txt @@ -1 +1 @@ -1.5.0-alpha.6 +1.5.0-alpha.7 From 2dd9e82306dd4a1a9a91877b7267087b1b21d13f Mon Sep 17 00:00:00 2001 From: Mikhail Kalinin Date: Thu, 3 Oct 2024 17:51:48 +0400 Subject: [PATCH 24/28] Remove blank lines --- .../block_processing/test_process_consolidation_request.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py b/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py index cdaf786fe..b1ee7e121 100644 --- a/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py +++ b/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py @@ -1100,7 +1100,6 @@ def run_consolidation_processing(spec, state, consolidation, success=True): - post-state ('post'). If ``success == False``, ``process_consolidation_request`` would return without any state change. """ - if success: validator_pubkeys = [v.pubkey for v in state.validators] source_index = spec.ValidatorIndex(validator_pubkeys.index(consolidation.source_pubkey)) @@ -1170,7 +1169,6 @@ def run_switch_to_compounding_processing(spec, state, consolidation, success=Tru - post-state ('post'). If ``success == False``, ``process_consolidation_request`` would return without any state change. """ - if success: validator_pubkeys = [v.pubkey for v in state.validators] source_index = spec.ValidatorIndex(validator_pubkeys.index(consolidation.source_pubkey)) From 66f5c3776cf9aa62e0b324c658bd3dc276a8775c Mon Sep 17 00:00:00 2001 From: Mikhail Kalinin Date: Thu, 3 Oct 2024 17:55:39 +0400 Subject: [PATCH 25/28] Fix switch to compounding tests --- .../block_processing/test_process_consolidation_request.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py b/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py index b1ee7e121..426f4fbe7 100644 --- a/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py +++ b/tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py @@ -1003,9 +1003,8 @@ def test_switch_to_compounding_source_bls_withdrawal_credential(spec, state): # a bls withdrawal credential current_epoch = spec.get_current_epoch(state) source_index = spec.get_active_validator_indices(state, current_epoch)[0] - source_address = b"\x22" * 20 consolidation = spec.ConsolidationRequest( - source_address=source_address, + source_address=state.validators[source_index].withdrawal_credentials[12:], source_pubkey=state.validators[source_index].pubkey, target_pubkey=state.validators[source_index].pubkey, ) @@ -1026,12 +1025,12 @@ def test_switch_to_compounding_source_coumpounding_withdrawal_credential(spec, s current_epoch = spec.get_current_epoch(state) source_index = spec.get_active_validator_indices(state, current_epoch)[0] source_address = b"\x22" * 20 + set_compounding_withdrawal_credential(spec, state, source_index, address=source_address) consolidation = spec.ConsolidationRequest( source_address=source_address, source_pubkey=state.validators[source_index].pubkey, target_pubkey=state.validators[source_index].pubkey, ) - set_compounding_withdrawal_credential(spec, state, source_index) state.balances[source_index] = spec.MIN_ACTIVATION_BALANCE + spec.EFFECTIVE_BALANCE_INCREMENT # Check the the return condition From 30f6aba593572c84c3bc8d21d4256402e5993e06 Mon Sep 17 00:00:00 2001 From: Justin Traglia <95511699+jtraglia@users.noreply.github.com> Date: Thu, 3 Oct 2024 11:04:33 -0500 Subject: [PATCH 26/28] Put "and" at the beginning of the next line Co-authored-by: Hsiao-Wei Wang --- specs/_features/eip7594/fork-choice.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/specs/_features/eip7594/fork-choice.md b/specs/_features/eip7594/fork-choice.md index d40d13419..f406b7472 100644 --- a/specs/_features/eip7594/fork-choice.md +++ b/specs/_features/eip7594/fork-choice.md @@ -31,8 +31,8 @@ def is_data_available(beacon_block_root: Root) -> bool: # `MIN_EPOCHS_FOR_DATA_COLUMN_SIDECARS_REQUESTS` epochs. column_sidecars = retrieve_column_sidecars(beacon_block_root) return all( - verify_data_column_sidecar(column_sidecar) and - verify_data_column_sidecar_kzg_proofs(column_sidecar) + verify_data_column_sidecar(column_sidecar) + and verify_data_column_sidecar_kzg_proofs(column_sidecar) for column_sidecar in column_sidecars ) ``` From 62c32da22bec597b05e5796e10515a78345f6589 Mon Sep 17 00:00:00 2001 From: Justin Traglia Date: Thu, 3 Oct 2024 11:20:38 -0500 Subject: [PATCH 27/28] Be more consistent with test names --- .../eth2spec/test/eip7594/unittests/test_networking.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py b/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py index a818c746f..633508f9a 100644 --- a/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py +++ b/tests/core/pyspec/eth2spec/test/eip7594/unittests/test_networking.py @@ -114,7 +114,7 @@ def test_verify_data_column_sidecar_kzg_proofs__valid(spec, state): @with_eip7594_and_later @spec_state_test @single_phase -def test_verify_data_column_sidecar_kzg_proofs__valid_but_wrong_column(spec, state): +def test_verify_data_column_sidecar_kzg_proofs__invalid_wrong_column(spec, state): sidecar = compute_data_column_sidecar(spec, state) sidecar.column[0] = sidecar.column[1] assert not spec.verify_data_column_sidecar_kzg_proofs(sidecar) @@ -123,7 +123,7 @@ def test_verify_data_column_sidecar_kzg_proofs__valid_but_wrong_column(spec, sta @with_eip7594_and_later @spec_state_test @single_phase -def test_verify_data_column_sidecar_kzg_proofs__valid_but_wrong_commitment(spec, state): +def test_verify_data_column_sidecar_kzg_proofs__invalid_wrong_commitment(spec, state): sidecar = compute_data_column_sidecar(spec, state) sidecar.kzg_commitments[0] = sidecar.kzg_commitments[1] assert not spec.verify_data_column_sidecar_kzg_proofs(sidecar) @@ -132,7 +132,7 @@ def test_verify_data_column_sidecar_kzg_proofs__valid_but_wrong_commitment(spec, @with_eip7594_and_later @spec_state_test @single_phase -def test_verify_data_column_sidecar_kzg_proofs__valid_but_wrong_proof(spec, state): +def test_verify_data_column_sidecar_kzg_proofs__invalid_wrong_proof(spec, state): sidecar = compute_data_column_sidecar(spec, state) sidecar.kzg_proofs[0] = sidecar.kzg_proofs[1] assert not spec.verify_data_column_sidecar_kzg_proofs(sidecar) @@ -152,7 +152,7 @@ def test_verify_data_column_sidecar_inclusion_proof__valid(spec, state): @with_eip7594_and_later @spec_state_test @single_phase -def test_verify_data_column_sidecar_inclusion_proof__valid_but_missing_commitment(spec, state): +def test_verify_data_column_sidecar_inclusion_proof__invalid_missing_commitment(spec, state): sidecar = compute_data_column_sidecar(spec, state) sidecar.kzg_commitments = sidecar.kzg_commitments[1:] assert not spec.verify_data_column_sidecar_inclusion_proof(sidecar) @@ -161,7 +161,7 @@ def test_verify_data_column_sidecar_inclusion_proof__valid_but_missing_commitmen @with_eip7594_and_later @spec_state_test @single_phase -def test_verify_data_column_sidecar_inclusion_proof__valid_but_duplicate_commitment(spec, state): +def test_verify_data_column_sidecar_inclusion_proof__invalid_duplicate_commitment(spec, state): sidecar = compute_data_column_sidecar(spec, state) sidecar.kzg_commitments = sidecar.kzg_commitments + [sidecar.kzg_commitments[0]] assert not spec.verify_data_column_sidecar_inclusion_proof(sidecar) From f97cd619f4a10c851f89f3f7253081da432dcd24 Mon Sep 17 00:00:00 2001 From: Suphanat Chunhapanya Date: Fri, 4 Oct 2024 00:51:04 +0700 Subject: [PATCH 28/28] Correct the use of get_data_column_sidecars --- specs/_features/eip7594/das-core.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specs/_features/eip7594/das-core.md b/specs/_features/eip7594/das-core.md index 25e58a133..6a1d2758e 100644 --- a/specs/_features/eip7594/das-core.md +++ b/specs/_features/eip7594/das-core.md @@ -244,7 +244,7 @@ In this construction, we extend the blobs using a one-dimensional erasure coding ### Parameters -For each column -- use `data_column_sidecar_{subnet_id}` subnets, where `subnet_id` can be computed with the `compute_subnet_for_data_column_sidecar(column_index: ColumnIndex)` helper. The sidecars can be computed with the `get_data_column_sidecars(signed_block: SignedBeaconBlock, blobs: Sequence[Blob])` helper. +For each column -- use `data_column_sidecar_{subnet_id}` subnets, where `subnet_id` can be computed with the `compute_subnet_for_data_column_sidecar(column_index: ColumnIndex)` helper. The sidecars can be computed with `cells_and_kzg_proofs = [compute_cells_and_kzg_proofs(blob) for blob in blobs]` and then `get_data_column_sidecars(signed_block, cells_and_kzg_proofs)`. Verifiable samples from their respective column are distributed on the assigned subnet. To custody a particular column, a node joins the respective gossipsub subnet. If a node fails to get a column on the column subnet, a node can also utilize the Req/Resp protocol to query the missing column from other peers.