Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix EIP-7251 tests #3656

Merged
merged 21 commits into from
Apr 16, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,19 @@ jobs:
command: make citest fork=eip7002
- store_test_results:
path: tests/core/pyspec/test-reports
test-eip7251:
docker:
- image: circleci/python:3.9
working_directory: ~/specs-repo
steps:
- restore_cache:
key: v3-specs-repo-{{ .Branch }}-{{ .Revision }}
- restore_pyspec_cached_venv
- run:
name: Run py-tests
command: make citest fork=eip7251
- store_test_results:
path: tests/core/pyspec/test-reports
test-eip7549:
docker:
- image: circleci/python:3.9
Expand Down Expand Up @@ -346,6 +359,9 @@ workflows:
- test-eip7002:
requires:
- install_pyspec_test
- test-eip7251:
requires:
- install_pyspec_test
- test-eip7549:
requires:
- install_pyspec_test
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/run-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ jobs:
needs: [preclear,lint,codespell,table_of_contents]
strategy:
matrix:
version: ["phase0", "altair", "bellatrix", "capella", "deneb", "eip6110", "eip7002", "eip7549", "whisk", "eip7594"]
version: ["phase0", "altair", "bellatrix", "capella", "deneb", "eip6110", "eip7002", "eip7251", "eip7549", "whisk", "eip7594"]
steps:
- name: Checkout this repo
uses: actions/checkout@v3.2.0
Expand Down
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ MARKDOWN_FILES = $(wildcard $(SPEC_DIR)/*/*.md) \
$(wildcard $(SPEC_DIR)/_features/*/*/*.md) \
$(wildcard $(SSZ_DIR)/*.md)

ALL_EXECUTABLE_SPEC_NAMES = phase0 altair bellatrix capella deneb eip6110 eip7002 eip7549 whisk
ALL_EXECUTABLE_SPEC_NAMES = phase0 altair bellatrix capella deneb eip6110 eip7002 eip7251 eip7549 whisk
# The parameters for commands. Use `foreach` to avoid listing specs again.
COVERAGE_SCOPE := $(foreach S,$(ALL_EXECUTABLE_SPEC_NAMES), --cov=eth2spec.$S.$(TEST_PRESET_TYPE))
PYLINT_SCOPE := $(foreach S,$(ALL_EXECUTABLE_SPEC_NAMES), ./eth2spec/$S)
Expand Down
2 changes: 1 addition & 1 deletion specs/_features/eip7251/beacon-chain.md
Original file line number Diff line number Diff line change
Expand Up @@ -1055,7 +1055,7 @@ def process_voluntary_exit(state: BeaconState, signed_voluntary_exit: SignedVolu
assert get_current_epoch(state) >= voluntary_exit.epoch
# Verify the validator has been active long enough
assert get_current_epoch(state) >= validator.activation_epoch + SHARD_COMMITTEE_PERIOD
# Only exit validator if it has no pending withdrawals in the queue
# [New in EIP-7251] Only exit validator if it has no pending withdrawals in the queue
assert get_pending_balance_to_withdraw(state, voluntary_exit.validator_index) == 0 # [New in EIP7251]
# Verify signature
domain = compute_domain(DOMAIN_VOLUNTARY_EXIT, CAPELLA_FORK_VERSION, state.genesis_validators_root)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
next_slot,
)
from eth2spec.test.helpers.withdrawals import (
get_expected_withdrawals,
prepare_expected_withdrawals,
set_eth1_withdrawal_credential_with_balance,
set_validator_fully_withdrawable,
Expand Down Expand Up @@ -62,7 +63,7 @@ def run_withdrawals_processing(spec, state, execution_payload, num_expected_with
- post-state ('post').
If ``valid == False``, run expecting ``AssertionError``
"""
expected_withdrawals = spec.get_expected_withdrawals(state)
expected_withdrawals = get_expected_withdrawals(spec, state)
assert len(expected_withdrawals) <= spec.MAX_WITHDRAWALS_PER_PAYLOAD
if num_expected_withdrawals is not None:
assert len(expected_withdrawals) == num_expected_withdrawals
Expand All @@ -87,7 +88,7 @@ def run_withdrawals_processing(spec, state, execution_payload, num_expected_with
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
assert len(get_expected_withdrawals(spec, 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')

Expand All @@ -100,7 +101,7 @@ def run_withdrawals_processing(spec, state, execution_payload, num_expected_with
@with_capella_and_later
@spec_state_test
def test_success_zero_expected_withdrawals(spec, state):
assert len(spec.get_expected_withdrawals(state)) == 0
assert len(get_expected_withdrawals(spec, state)) == 0

next_slot(spec, state)
execution_payload = build_empty_execution_payload(spec, state)
Expand Down
28 changes: 19 additions & 9 deletions tests/core/pyspec/eth2spec/test/capella/sanity/test_blocks.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from eth2spec.test.helpers.constants import MINIMAL
from eth2spec.test.helpers.forks import is_post_eip7251
from eth2spec.test.context import (
with_capella_and_later,
spec_state_test,
Expand All @@ -20,6 +21,7 @@
next_epoch_with_attestations,
)
from eth2spec.test.helpers.withdrawals import (
get_expected_withdrawals,
set_eth1_withdrawal_credential_with_balance,
set_validator_fully_withdrawable,
set_validator_partially_withdrawable,
Expand Down Expand Up @@ -202,7 +204,7 @@ def test_full_withdrawal_in_epoch_transition(spec, state):
index = 0
current_epoch = spec.get_current_epoch(state)
set_validator_fully_withdrawable(spec, state, index, current_epoch)
assert len(spec.get_expected_withdrawals(state)) == 1
assert len(get_expected_withdrawals(spec, state)) == 1

yield 'pre', state

Expand All @@ -214,7 +216,7 @@ def test_full_withdrawal_in_epoch_transition(spec, state):
yield 'post', state

assert state.balances[index] == 0
assert len(spec.get_expected_withdrawals(state)) == 0
assert len(get_expected_withdrawals(spec, state)) == 0


@with_capella_and_later
Expand All @@ -224,7 +226,7 @@ def test_partial_withdrawal_in_epoch_transition(spec, state):
set_validator_partially_withdrawable(spec, state, index, excess_balance=1000000000000)
pre_balance = state.balances[index]

assert len(spec.get_expected_withdrawals(state)) == 1
assert len(get_expected_withdrawals(spec, state)) == 1

yield 'pre', state

Expand All @@ -238,7 +240,7 @@ def test_partial_withdrawal_in_epoch_transition(spec, state):
assert state.balances[index] < pre_balance
# Potentially less than due to sync committee penalty
assert state.balances[index] <= spec.MAX_EFFECTIVE_BALANCE
assert len(spec.get_expected_withdrawals(state)) == 0
assert len(get_expected_withdrawals(spec, state)) == 0


@with_capella_and_later
Expand All @@ -250,7 +252,7 @@ def test_many_partial_withdrawals_in_epoch_transition(spec, state):
index = (i + state.next_withdrawal_index) % len(state.validators)
set_validator_partially_withdrawable(spec, state, index, excess_balance=1000000000000)

assert len(spec.get_expected_withdrawals(state)) == spec.MAX_WITHDRAWALS_PER_PAYLOAD
assert (len(get_expected_withdrawals(spec, state)) == spec.MAX_WITHDRAWALS_PER_PAYLOAD)

yield 'pre', state

Expand All @@ -261,7 +263,7 @@ def test_many_partial_withdrawals_in_epoch_transition(spec, state):
yield 'blocks', [signed_block]
yield 'post', state

assert len(spec.get_expected_withdrawals(state)) == 1
assert len(get_expected_withdrawals(spec, state)) == 1


def _perform_valid_withdrawal(spec, state):
Expand All @@ -272,7 +274,7 @@ def _perform_valid_withdrawal(spec, state):
next_slot(spec, state)
pre_next_withdrawal_index = state.next_withdrawal_index

expected_withdrawals = spec.get_expected_withdrawals(state)
expected_withdrawals = get_expected_withdrawals(spec, state)

pre_state = state.copy()

Expand Down Expand Up @@ -357,7 +359,11 @@ def test_top_up_and_partial_withdrawable_validator(spec, state):

signed_block = state_transition_and_sign_block(spec, state, block)

yield 'blocks', [signed_block]
# ensure we go through an epoch transition, to account for post-EIP-7251 behavior
block_in_next_epoch = build_empty_block(spec, state, slot=state.slot + spec.SLOTS_PER_EPOCH)
signed_block_in_next_epoch = state_transition_and_sign_block(spec, state, block_in_next_epoch)

yield 'blocks', [signed_block, signed_block_in_next_epoch]
Comment on lines +362 to +366
Copy link
Contributor

Choose a reason for hiding this comment

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

should it be only with if is_post_eip7251: ... condition to distinguish the post-EIP-7251 behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think its fine as extending the blocks is backwards-compatible with the capella and deneb behavior

yield 'post', state

# Since withdrawals happen before deposits, it becomes partially withdrawable after state transition.
Expand Down Expand Up @@ -395,9 +401,13 @@ def test_top_up_to_fully_withdrawn_validator(spec, state):

signed_block_1 = state_transition_and_sign_block(spec, state, block)

balance = state.balances[validator_index]
if is_post_eip7251(spec):
balance += state.pending_balance_deposits[0].amount

assert spec.is_fully_withdrawable_validator(
state.validators[validator_index],
state.balances[validator_index],
balance,
spec.get_current_epoch(state)
)

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from eth2spec.test.helpers.keys import pubkeys
from eth2spec.test.helpers.forks import is_post_eip7251
from eth2spec.test.helpers.constants import MINIMAL
from eth2spec.test.context import (
with_deneb_and_later,
Expand Down Expand Up @@ -47,7 +48,9 @@ def run_test_activation_churn_limit(spec, state):
# Half should churn in first run of registry update
for i in range(mock_activations):
index = validator_count_0 + i
if index < validator_count_0 + churn_limit_0:
# NOTE: activations are gated different after EIP-7251
# all eligible validators have been activated
if index < validator_count_0 + churn_limit_0 or is_post_eip7251(spec):
# The eligible validators within the activation churn limit should have been activated
assert state.validators[index].activation_epoch < spec.FAR_FUTURE_EPOCH
else:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,13 +106,12 @@ def test_basic_consolidation_with_compounding_credential(spec, state):
def test_consolidation_churn_limit_balance(spec, state):
# This state has 256 validators each with 32 ETH in MINIMAL preset, 128 ETH consolidation churn
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
current_epoch = spec.get_current_epoch(state)

source_index = spec.get_active_validator_indices(state, current_epoch)[0]
# Set source balance to consolidation churn limit
state.balances[source_index] = consolidation_churn_limit
source_validator = state.validators[source_index]
source_validator.effective_balance = consolidation_churn_limit
updated_consolidation_churn_limit = spec.get_consolidation_churn_limit(state)
target_index = spec.get_active_validator_indices(state, current_epoch)[1]
source_privkey = pubkey_to_privkey[state.validators[source_index].pubkey]
target_privkey = pubkey_to_privkey[state.validators[target_index].pubkey]
Expand All @@ -131,7 +130,7 @@ def test_consolidation_churn_limit_balance(spec, state):

expected_exit_epoch = spec.compute_activation_exit_epoch(current_epoch)
# Check consolidation churn is decremented correctly
assert state.consolidation_balance_to_consume == 0
assert state.consolidation_balance_to_consume == updated_consolidation_churn_limit - consolidation_churn_limit
# Check exit epoch
assert state.validators[0].exit_epoch == expected_exit_epoch

Expand All @@ -145,13 +144,11 @@ def test_consolidation_churn_limit_balance(spec, state):
def test_consolidation_balance_larger_than_churn_limit(spec, state):
# This state has 256 validators each with 32 ETH in MINIMAL preset, 128 ETH consolidation churn
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
current_epoch = spec.get_current_epoch(state)

source_index = spec.get_active_validator_indices(state, current_epoch)[0]
# Set source balance higher than consolidation churn limit
state.balances[source_index] = consolidation_churn_limit + 1
state.validators[source_index].effective_balance = 2 * consolidation_churn_limit
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
state.validators[source_index].effective_balance = 2 * consolidation_churn_limit
state.validators[source_index].effective_balance = consolidation_churn_limit + spec.EFFECTIVE_BALANCE_INCREMENT

I'd maybe do this instead, just to make it easier to interpret what happens. Here one sees the effective balance being 2x the churn limit, and it can be confusing why then expected_exit_epoch only gets a +1 instead of +2

target_index = spec.get_active_validator_indices(state, current_epoch)[1]
source_privkey = pubkey_to_privkey[state.validators[source_index].pubkey]
target_privkey = pubkey_to_privkey[state.validators[target_index].pubkey]
Expand All @@ -160,6 +157,10 @@ def test_consolidation_balance_larger_than_churn_limit(spec, state):
set_compounding_withdrawal_credential(spec, state, source_index)
set_compounding_withdrawal_credential(spec, state, target_index)

new_churn_limit = spec.get_consolidation_churn_limit(state)
remainder = state.validators[source_index].effective_balance % new_churn_limit
expected_balance = new_churn_limit - remainder

signed_consolidation = sign_consolidation(spec, state,
spec.Consolidation(
epoch=current_epoch,
Expand All @@ -170,7 +171,7 @@ def test_consolidation_balance_larger_than_churn_limit(spec, state):

expected_exit_epoch = spec.compute_activation_exit_epoch(current_epoch) + 1
# Check consolidation churn is decremented correctly
assert state.consolidation_balance_to_consume == consolidation_churn_limit - 1
assert state.consolidation_balance_to_consume == expected_balance
# Check exit epoch
assert state.validators[0].exit_epoch == expected_exit_epoch

Expand All @@ -181,11 +182,9 @@ def test_consolidation_balance_larger_than_churn_limit(spec, state):
balances_fn=scaled_churn_balances_exceed_activation_exit_churn_limit, threshold_fn=default_activation_threshold)
@spec_test
@single_phase
def test_consolidation_balance_twice_the_churn_limit(spec, state):
def test_consolidation_balance_through_two_churn_epochs(spec, state):
# This state has 256 validators each with 32 ETH in MINIMAL preset, 128 ETH consolidation churn
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
current_epoch = spec.get_current_epoch(state)

source_index = spec.get_active_validator_indices(state, current_epoch)[0]
Expand All @@ -198,7 +197,11 @@ def test_consolidation_balance_twice_the_churn_limit(spec, state):
set_compounding_withdrawal_credential(spec, state, target_index)

# Set source balance higher than consolidation churn limit
state.balances[source_index] = 2 * consolidation_churn_limit
state.validators[source_index].effective_balance = 3 * consolidation_churn_limit

new_churn_limit = spec.get_consolidation_churn_limit(state)
remainder = state.validators[source_index].effective_balance % new_churn_limit
expected_balance = new_churn_limit - remainder

signed_consolidation = sign_consolidation(spec, state,
spec.Consolidation(
Expand All @@ -212,7 +215,7 @@ def test_consolidation_balance_twice_the_churn_limit(spec, state):
expected_exit_epoch = spec.compute_activation_exit_epoch(current_epoch) + 2
assert state.validators[0].exit_epoch == expected_exit_epoch
# since the earliest exit epoch moves to a new one, consolidation balance is back to full
assert state.consolidation_balance_to_consume == consolidation_churn_limit
assert state.consolidation_balance_to_consume == expected_balance


@with_eip7251_and_later
Expand Down
19 changes: 18 additions & 1 deletion tests/core/pyspec/eth2spec/test/helpers/deposits.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from random import Random

from eth2spec.test.context import expect_assertion_error
from eth2spec.test.helpers.forks import is_post_altair
from eth2spec.test.helpers.forks import is_post_altair, is_post_eip7251
from eth2spec.test.helpers.keys import pubkeys, privkeys
from eth2spec.test.helpers.state import get_balance
from eth2spec.utils import bls
Expand Down Expand Up @@ -241,6 +241,9 @@ def run_deposit_processing(spec, state, deposit, validator_index, valid=True, ef
pre_balance = get_balance(state, validator_index)
pre_effective_balance = state.validators[validator_index].effective_balance

if is_post_eip7251(spec):
assert len(state.pending_balance_deposits) == 0

yield 'pre', state
yield 'deposit', deposit

Expand All @@ -253,6 +256,20 @@ def run_deposit_processing(spec, state, deposit, validator_index, valid=True, ef

yield 'post', state

if is_post_eip7251(spec):
# balance additions are now routed through "pending balance deposits"
# process them here along with any effective balance updates
updates_count = len(state.pending_balance_deposits)
if updates_count != 0:
assert updates_count == 1
pending_balance_deposit = state.pending_balance_deposits[0]
assert pending_balance_deposit.index == validator_index
assert pending_balance_deposit.amount == deposit.data.amount
spec.increase_balance(state, validator_index, deposit.data.amount)
if not is_top_up:
# run effective balance update for new validators
spec.process_effective_balance_updates(state)
ralexstokes marked this conversation as resolved.
Show resolved Hide resolved

if not effective or not bls.KeyValidate(deposit.data.pubkey):
assert len(state.validators) == pre_validator_count
assert len(state.balances) == pre_validator_count
Expand Down
3 changes: 2 additions & 1 deletion tests/core/pyspec/eth2spec/test/helpers/execution_payload.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from rlp.sedes import big_endian_int, Binary, List

from eth2spec.debug.random_value import get_random_bytes_list
from eth2spec.test.helpers.withdrawals import get_expected_withdrawals
from eth2spec.test.helpers.forks import (
is_post_capella,
is_post_deneb,
Expand Down Expand Up @@ -227,7 +228,7 @@ def build_empty_execution_payload(spec, state, randao_mix=None):
transactions=empty_txs,
)
if is_post_capella(spec):
payload.withdrawals = spec.get_expected_withdrawals(state)
payload.withdrawals = get_expected_withdrawals(spec, state)
if is_post_deneb(spec):
payload.blob_gas_used = 0
payload.excess_blob_gas = 0
Expand Down
3 changes: 3 additions & 0 deletions tests/core/pyspec/eth2spec/test/helpers/genesis.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,10 @@ def create_genesis_state(spec, validator_balances, activation_threshold):
state.deposit_balance_to_consume = 0
state.exit_balance_to_consume = 0
state.earliest_exit_epoch = spec.GENESIS_EPOCH
state.consolidation_balance_to_consume = 0
state.earliest_consolidation_epoch = 0
state.pending_balance_deposits = []
state.pending_partial_withdrawals = []
state.pending_consolidations = []

return state
Loading
Loading