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 all 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 @@ -168,6 +168,19 @@ jobs:
command: make citest fork=electra
- 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-whisk:
docker:
- image: circleci/python:3.9
Expand Down Expand Up @@ -317,6 +330,9 @@ workflows:
- test-electra:
requires:
- install_pyspec_test
- test-eip7251:
requires:
- install_pyspec_test
- test-whisk:
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", "electra", "whisk", "eip7594"]
version: ["phase0", "altair", "bellatrix", "capella", "deneb", "electra", "eip7251", "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 electra whisk
ALL_EXECUTABLE_SPEC_NAMES = phase0 altair bellatrix capella deneb electra eip7251 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
4 changes: 2 additions & 2 deletions specs/_features/eip7251/beacon-chain.md
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,7 @@ def initiate_validator_exit(state: BeaconState, index: ValidatorIndex) -> None:
def switch_to_compounding_validator(state: BeaconState, index: ValidatorIndex) -> None:
validator = state.validators[index]
if has_eth1_withdrawal_credential(validator):
validator.withdrawal_credentials[:1] = COMPOUNDING_WITHDRAWAL_PREFIX
validator.withdrawal_credentials = COMPOUNDING_WITHDRAWAL_PREFIX + validator.withdrawal_credentials[1:]
queue_excess_active_balance(state, index)
```

Expand Down 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 @@ -2,6 +2,7 @@
spec_state_test,
with_capella_and_later,
)
from eth2spec.test.helpers.forks import is_post_eip7251
from eth2spec.test.helpers.state import next_epoch_via_block
from eth2spec.test.helpers.deposits import (
prepare_state_and_deposit,
Expand Down Expand Up @@ -30,10 +31,25 @@ def test_success_top_up_to_withdrawn_validator(spec, state):

yield from run_deposit_processing(spec, state, deposit, validator_index)

assert state.balances[validator_index] == amount
assert state.validators[validator_index].effective_balance == 0
if is_post_eip7251(spec):
pending_balance_deposits_len = len(state.pending_balance_deposits)
pending_balance_deposit = state.pending_balance_deposits[pending_balance_deposits_len - 1]
assert pending_balance_deposit.amount == amount
assert pending_balance_deposit.index == validator_index
else:
assert state.balances[validator_index] == amount
assert state.validators[validator_index].effective_balance == 0

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

if is_post_eip7251(spec):
has_execution_withdrawal = spec.has_execution_withdrawal_credential(validator)
is_withdrawable = validator.withdrawable_epoch <= current_epoch
has_non_zero_balance = pending_balance_deposit.amount > 0
# NOTE: directly compute `is_fully_withdrawable_validator` conditions here
# to work around how the epoch processing changed balance updates
assert has_execution_withdrawal and is_withdrawable and has_non_zero_balance
else:
assert spec.is_fully_withdrawable_validator(validator, balance, current_epoch)
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
Loading
Loading