From 37e504e78457733e4ee5c737e23b6623ba8459fc Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Fri, 11 Nov 2022 10:47:53 -0700 Subject: [PATCH 1/4] bound the maximum number of validators considered for withdrawals per sweep --- presets/mainnet/capella.yaml | 5 ++ presets/minimal/capella.yaml | 5 ++ specs/capella/beacon-chain.md | 30 +++++-- .../test_process_withdrawals.py | 83 +++++++++++++++---- .../test/capella/sanity/test_blocks.py | 4 +- .../eth2spec/test/helpers/withdrawals.py | 7 +- 6 files changed, 107 insertions(+), 27 deletions(-) diff --git a/presets/mainnet/capella.yaml b/presets/mainnet/capella.yaml index 62306c8dc..bd80f6beb 100644 --- a/presets/mainnet/capella.yaml +++ b/presets/mainnet/capella.yaml @@ -10,3 +10,8 @@ MAX_BLS_TO_EXECUTION_CHANGES: 16 # --------------------------------------------------------------- # 2**4 (= 16) withdrawals MAX_WITHDRAWALS_PER_PAYLOAD: 16 + +# Withdrawals processing +# --------------------------------------------------------------- +# 2**10 (= 1024) validators +MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP: 1024 diff --git a/presets/minimal/capella.yaml b/presets/minimal/capella.yaml index 1ac8a806c..d27253de8 100644 --- a/presets/minimal/capella.yaml +++ b/presets/minimal/capella.yaml @@ -10,3 +10,8 @@ MAX_BLS_TO_EXECUTION_CHANGES: 16 # --------------------------------------------------------------- # [customized] 2**2 (= 4) MAX_WITHDRAWALS_PER_PAYLOAD: 4 + +# Withdrawals processing +# --------------------------------------------------------------- +# [customized] 2**4 (= 16) validators +MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP: 16 diff --git a/specs/capella/beacon-chain.md b/specs/capella/beacon-chain.md index 90a64925f..779d97a04 100644 --- a/specs/capella/beacon-chain.md +++ b/specs/capella/beacon-chain.md @@ -8,11 +8,11 @@ - [Introduction](#introduction) - [Custom types](#custom-types) -- [Constants](#constants) - [Domain types](#domain-types) - [Preset](#preset) - [Max operations per block](#max-operations-per-block) - [Execution](#execution) + - [Withdrawals processing](#withdrawals-processing) - [Containers](#containers) - [New containers](#new-containers) - [`Withdrawal`](#withdrawal) @@ -58,8 +58,6 @@ We define the following Python custom types for type hinting and readability: | - | - | - | | `WithdrawalIndex` | `uint64` | an index of a `Withdrawal` | -## Constants - ### Domain types | Name | Value | @@ -80,6 +78,12 @@ We define the following Python custom types for type hinting and readability: | - | - | - | | `MAX_WITHDRAWALS_PER_PAYLOAD` | `uint64(2**4)` (= 16) | Maximum amount of withdrawals allowed in each payload | +### Withdrawals processing + +| Name | Value | +| - | - | +| `MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP` | `1024` (= 2**10 ) | + ## Containers ### New containers @@ -288,7 +292,8 @@ def get_expected_withdrawals(state: BeaconState) -> Sequence[Withdrawal]: withdrawal_index = state.next_withdrawal_index validator_index = state.next_withdrawal_validator_index withdrawals: List[Withdrawal] = [] - for _ in range(len(state.validators)): + bound = min(len(state.validators), MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP) + for _ in range(bound): validator = state.validators[validator_index] balance = state.balances[validator_index] if is_fully_withdrawable_validator(validator, balance, epoch): @@ -312,7 +317,7 @@ def get_expected_withdrawals(state: BeaconState) -> Sequence[Withdrawal]: validator_index = ValidatorIndex((validator_index + 1) % len(state.validators)) return withdrawals ``` - + #### New `process_withdrawals` ```python @@ -323,10 +328,21 @@ def process_withdrawals(state: BeaconState, payload: ExecutionPayload) -> None: for expected_withdrawal, withdrawal in zip(expected_withdrawals, payload.withdrawals): assert withdrawal == expected_withdrawal decrease_balance(state, withdrawal.validator_index, withdrawal.amount) - if len(expected_withdrawals) > 0: + + # Update the next withdrawal index if this block contained withdrawals + if len(expected_withdrawals) != 0: latest_withdrawal = expected_withdrawals[-1] state.next_withdrawal_index = WithdrawalIndex(latest_withdrawal.index + 1) - next_validator_index = ValidatorIndex((latest_withdrawal.validator_index + 1) % len(state.validators)) + + # Update the next validator index to start the next withdrawal sweep + if len(expected_withdrawals) == MAX_WITHDRAWALS_PER_PAYLOAD: + # Next sweep starts after the latest withdrawal's validator index + next_validator_index = ValidatorIndex((expected_withdrawals[-1].validator_index + 1) % len(state.validators)) + state.next_withdrawal_validator_index = next_validator_index + else: + # Advance sweep by the max length of the sweep if there was not a full set of withdrawals + next_index = state.next_withdrawal_validator_index + MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP + next_validator_index = ValidatorIndex(next_index % len(state.validators)) state.next_withdrawal_validator_index = next_validator_index ``` diff --git a/tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_withdrawals.py b/tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_withdrawals.py index da3ddcb4d..dc913e5d7 100644 --- a/tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_withdrawals.py +++ b/tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_withdrawals.py @@ -6,7 +6,7 @@ from eth2spec.test.context import ( with_presets, with_phases, ) -from eth2spec.test.helpers.constants import MINIMAL, CAPELLA +from eth2spec.test.helpers.constants import MAINNET, MINIMAL, CAPELLA from eth2spec.test.helpers.execution_payload import ( build_empty_execution_payload, ) @@ -33,8 +33,13 @@ def verify_post_state(state, spec, expected_withdrawals, expected_withdrawals_validator_indices = [withdrawal.validator_index for withdrawal in expected_withdrawals] assert state.next_withdrawal_index == expected_withdrawals[-1].index + 1 - next_withdrawal_validator_index = (expected_withdrawals_validator_indices[-1] + 1) % len(state.validators) - assert state.next_withdrawal_validator_index == next_withdrawal_validator_index + + if len(expected_withdrawals) == spec.MAX_WITHDRAWALS_PER_PAYLOAD: + # NOTE: ideally we would also check in the case with + # fewer than maximum withdrawals but that requires the pre-state info + next_withdrawal_validator_index = (expected_withdrawals_validator_indices[-1] + 1) % len(state.validators) + assert state.next_withdrawal_validator_index == next_withdrawal_validator_index + for index in fully_withdrawable_indices: if index in expected_withdrawals_validator_indices: assert state.balances[index] == 0 @@ -75,9 +80,13 @@ def run_withdrawals_processing(spec, state, execution_payload, num_expected_with yield 'post', state if len(expected_withdrawals) == 0: - assert state == pre_state - elif len(expected_withdrawals) < spec.MAX_WITHDRAWALS_PER_PAYLOAD: - assert len(spec.get_expected_withdrawals(state)) == 0 + next_withdrawal_validator_index = ( + pre_state.next_withdrawal_validator_index + spec.MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP + ) + assert state.next_withdrawal_validator_index == next_withdrawal_validator_index % len(state.validators) + elif len(expected_withdrawals) <= spec.MAX_WITHDRAWALS_PER_PAYLOAD: + bound = min(spec.MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP, spec.MAX_WITHDRAWALS_PER_PAYLOAD) + assert len(spec.get_expected_withdrawals(state)) <= bound elif len(expected_withdrawals) > spec.MAX_WITHDRAWALS_PER_PAYLOAD: raise ValueError('len(expected_withdrawals) should not be greater than MAX_WITHDRAWALS_PER_PAYLOAD') @@ -154,10 +163,32 @@ def test_success_max_per_slot(spec, state): @with_phases([CAPELLA]) +@with_presets([MAINNET], reason="too few validators with minimal config") +@spec_state_test +def test_success_all_fully_withdrawable_in_one_sweep(spec, state): + assert len(state.validators) <= spec.MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP + + withdrawal_count = len(state.validators) + fully_withdrawable_indices, partial_withdrawals_indices = prepare_expected_withdrawals( + spec, state, num_full_withdrawals=withdrawal_count) + + next_slot(spec, state) + execution_payload = build_empty_execution_payload(spec, state) + + yield from run_withdrawals_processing( + spec, state, execution_payload, + fully_withdrawable_indices=fully_withdrawable_indices, + partial_withdrawals_indices=partial_withdrawals_indices) + +@with_phases([CAPELLA]) +@with_presets([MINIMAL], reason="too many validators with mainnet config") @spec_state_test def test_success_all_fully_withdrawable(spec, state): + assert len(state.validators) > spec.MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP + + withdrawal_count = spec.MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP fully_withdrawable_indices, partial_withdrawals_indices = prepare_expected_withdrawals( - spec, state, num_full_withdrawals=len(state.validators)) + spec, state, num_full_withdrawals=withdrawal_count) next_slot(spec, state) execution_payload = build_empty_execution_payload(spec, state) @@ -169,10 +200,32 @@ def test_success_all_fully_withdrawable(spec, state): @with_phases([CAPELLA]) +@with_presets([MAINNET], reason="too few validators with minimal config") +@spec_state_test +def test_success_all_partially_withdrawable_in_one_sweep(spec, state): + assert len(state.validators) <= spec.MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP + + withdrawal_count = len(state.validators) + fully_withdrawable_indices, partial_withdrawals_indices = prepare_expected_withdrawals( + spec, state, num_partial_withdrawals=withdrawal_count) + + next_slot(spec, state) + execution_payload = build_empty_execution_payload(spec, state) + + yield from run_withdrawals_processing( + spec, state, execution_payload, + fully_withdrawable_indices=fully_withdrawable_indices, + partial_withdrawals_indices=partial_withdrawals_indices) + +@with_phases([CAPELLA]) +@with_presets([MINIMAL], reason="too many validators with mainnet config") @spec_state_test def test_success_all_partially_withdrawable(spec, state): + assert len(state.validators) > spec.MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP + + withdrawal_count = spec.MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP fully_withdrawable_indices, partial_withdrawals_indices = prepare_expected_withdrawals( - spec, state, num_partial_withdrawals=len(state.validators)) + spec, state, num_partial_withdrawals=withdrawal_count) next_slot(spec, state) execution_payload = build_empty_execution_payload(spec, state) @@ -302,8 +355,8 @@ def test_fail_a_lot_partially_withdrawable_too_few_in_withdrawals(spec, state): @with_phases([CAPELLA]) @spec_state_test def test_fail_a_lot_mixed_withdrawable_in_queue_too_few_in_withdrawals(spec, state): - prepare_expected_withdrawals(spec, state, num_full_withdrawals=spec.MAX_WITHDRAWALS_PER_PAYLOAD * 4, - num_partial_withdrawals=spec.MAX_WITHDRAWALS_PER_PAYLOAD * 4) + prepare_expected_withdrawals(spec, state, num_full_withdrawals=spec.MAX_WITHDRAWALS_PER_PAYLOAD, + num_partial_withdrawals=spec.MAX_WITHDRAWALS_PER_PAYLOAD) next_slot(spec, state) execution_payload = build_empty_execution_payload(spec, state) @@ -624,7 +677,7 @@ def test_success_excess_balance_but_no_max_effective_balance(spec, state): @with_phases([CAPELLA]) @spec_state_test def test_success_one_partial_withdrawable_not_yet_active(spec, state): - validator_index = len(state.validators) // 2 + validator_index = min(len(state.validators) // 2, spec.MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP - 1) state.validators[validator_index].activation_epoch += 4 set_validator_partially_withdrawable(spec, state, validator_index) @@ -638,7 +691,7 @@ def test_success_one_partial_withdrawable_not_yet_active(spec, state): @with_phases([CAPELLA]) @spec_state_test def test_success_one_partial_withdrawable_in_exit_queue(spec, state): - validator_index = len(state.validators) // 2 + validator_index = min(len(state.validators) // 2, spec.MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP - 1) state.validators[validator_index].exit_epoch = spec.get_current_epoch(state) + 1 set_validator_partially_withdrawable(spec, state, validator_index) @@ -653,7 +706,7 @@ def test_success_one_partial_withdrawable_in_exit_queue(spec, state): @with_phases([CAPELLA]) @spec_state_test def test_success_one_partial_withdrawable_exited(spec, state): - validator_index = len(state.validators) // 2 + validator_index = min(len(state.validators) // 2, spec.MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP - 1) state.validators[validator_index].exit_epoch = spec.get_current_epoch(state) set_validator_partially_withdrawable(spec, state, validator_index) @@ -667,7 +720,7 @@ def test_success_one_partial_withdrawable_exited(spec, state): @with_phases([CAPELLA]) @spec_state_test def test_success_one_partial_withdrawable_active_and_slashed(spec, state): - validator_index = len(state.validators) // 2 + validator_index = min(len(state.validators) // 2, spec.MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP - 1) state.validators[validator_index].slashed = True set_validator_partially_withdrawable(spec, state, validator_index) @@ -681,7 +734,7 @@ def test_success_one_partial_withdrawable_active_and_slashed(spec, state): @with_phases([CAPELLA]) @spec_state_test def test_success_one_partial_withdrawable_exited_and_slashed(spec, state): - validator_index = len(state.validators) // 2 + validator_index = min(len(state.validators) // 2, spec.MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP - 1) state.validators[validator_index].slashed = True state.validators[validator_index].exit_epoch = spec.get_current_epoch(state) set_validator_partially_withdrawable(spec, state, validator_index) diff --git a/tests/core/pyspec/eth2spec/test/capella/sanity/test_blocks.py b/tests/core/pyspec/eth2spec/test/capella/sanity/test_blocks.py index 229d8ecc5..91adc1de3 100644 --- a/tests/core/pyspec/eth2spec/test/capella/sanity/test_blocks.py +++ b/tests/core/pyspec/eth2spec/test/capella/sanity/test_blocks.py @@ -194,8 +194,8 @@ def test_many_partial_withdrawals_in_epoch_transition(spec, state): def _perform_valid_withdrawal(spec, state): fully_withdrawable_indices, partial_withdrawals_indices = prepare_expected_withdrawals( - spec, state, num_partial_withdrawals=spec.MAX_WITHDRAWALS_PER_PAYLOAD * 4, - num_full_withdrawals=spec.MAX_WITHDRAWALS_PER_PAYLOAD * 4) + spec, state, num_partial_withdrawals=spec.MAX_WITHDRAWALS_PER_PAYLOAD * 2, + num_full_withdrawals=spec.MAX_WITHDRAWALS_PER_PAYLOAD * 2) next_slot(spec, state) pre_next_withdrawal_index = state.next_withdrawal_index diff --git a/tests/core/pyspec/eth2spec/test/helpers/withdrawals.py b/tests/core/pyspec/eth2spec/test/helpers/withdrawals.py index a55ac36ef..aebe49f26 100644 --- a/tests/core/pyspec/eth2spec/test/helpers/withdrawals.py +++ b/tests/core/pyspec/eth2spec/test/helpers/withdrawals.py @@ -36,9 +36,10 @@ def set_validator_partially_withdrawable(spec, state, index, excess_balance=1000 def prepare_expected_withdrawals(spec, state, num_full_withdrawals=0, num_partial_withdrawals=0, rng=random.Random(5566)): - assert num_full_withdrawals + num_partial_withdrawals <= len(state.validators) - all_validator_indices = list(range(len(state.validators))) - sampled_indices = rng.sample(all_validator_indices, num_full_withdrawals + num_partial_withdrawals) + bound = min(len(state.validators), spec.MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP) + assert num_full_withdrawals + num_partial_withdrawals <= bound + eligible_validator_indices = list(range(bound)) + sampled_indices = rng.sample(eligible_validator_indices, num_full_withdrawals + num_partial_withdrawals) fully_withdrawable_indices = rng.sample(sampled_indices, num_full_withdrawals) partial_withdrawals_indices = list(set(sampled_indices).difference(set(fully_withdrawable_indices))) From 478b437b053450eccf36ab51d1649973c09936d8 Mon Sep 17 00:00:00 2001 From: Danny Ryan Date: Mon, 12 Dec 2022 08:25:31 -0700 Subject: [PATCH 2/4] lint --- .../test/capella/block_processing/test_process_withdrawals.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_withdrawals.py b/tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_withdrawals.py index dc913e5d7..8292e66ce 100644 --- a/tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_withdrawals.py +++ b/tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_withdrawals.py @@ -180,6 +180,7 @@ def test_success_all_fully_withdrawable_in_one_sweep(spec, state): fully_withdrawable_indices=fully_withdrawable_indices, partial_withdrawals_indices=partial_withdrawals_indices) + @with_phases([CAPELLA]) @with_presets([MINIMAL], reason="too many validators with mainnet config") @spec_state_test @@ -217,6 +218,7 @@ def test_success_all_partially_withdrawable_in_one_sweep(spec, state): fully_withdrawable_indices=fully_withdrawable_indices, partial_withdrawals_indices=partial_withdrawals_indices) + @with_phases([CAPELLA]) @with_presets([MINIMAL], reason="too many validators with mainnet config") @spec_state_test From a06275765650fe61a1e3eaf83d92429485557360 Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Mon, 12 Dec 2022 09:25:40 -0700 Subject: [PATCH 3/4] update mainnet preset value for sweep size --- presets/mainnet/capella.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/presets/mainnet/capella.yaml b/presets/mainnet/capella.yaml index bd80f6beb..913c2956b 100644 --- a/presets/mainnet/capella.yaml +++ b/presets/mainnet/capella.yaml @@ -13,5 +13,5 @@ MAX_WITHDRAWALS_PER_PAYLOAD: 16 # Withdrawals processing # --------------------------------------------------------------- -# 2**10 (= 1024) validators -MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP: 1024 +# 2**14 (= 16384) validators +MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP: 16384 From 1f943f0d64a4330d9435564a9374e72b1413aa3e Mon Sep 17 00:00:00 2001 From: Danny Ryan Date: Mon, 12 Dec 2022 14:22:58 -0700 Subject: [PATCH 4/4] Update specs/capella/beacon-chain.md --- specs/capella/beacon-chain.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specs/capella/beacon-chain.md b/specs/capella/beacon-chain.md index 779d97a04..20f4a5dd4 100644 --- a/specs/capella/beacon-chain.md +++ b/specs/capella/beacon-chain.md @@ -82,7 +82,7 @@ We define the following Python custom types for type hinting and readability: | Name | Value | | - | - | -| `MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP` | `1024` (= 2**10 ) | +| `MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP` | `16384` (= 2**14 ) | ## Containers