From 730a7afe49210ce8b326d672ef478c3f48a83e43 Mon Sep 17 00:00:00 2001 From: Hsiao-Wei Wang Date: Thu, 12 Jan 2023 23:10:22 +0800 Subject: [PATCH 1/5] Always use `GENESIS_FORK_VERSION` to sign `BLSToExecutionChange` message --- specs/capella/beacon-chain.md | 3 ++- .../test_process_bls_to_execution_change.py | 17 +++++++++++------ .../test/helpers/bls_to_execution_changes.py | 8 ++++---- 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/specs/capella/beacon-chain.md b/specs/capella/beacon-chain.md index 9a04f2f38..1be41e7eb 100644 --- a/specs/capella/beacon-chain.md +++ b/specs/capella/beacon-chain.md @@ -472,7 +472,8 @@ def process_bls_to_execution_change(state: BeaconState, assert validator.withdrawal_credentials[:1] == BLS_WITHDRAWAL_PREFIX assert validator.withdrawal_credentials[1:] == hash(address_change.from_bls_pubkey)[1:] - domain = get_domain(state, DOMAIN_BLS_TO_EXECUTION_CHANGE) + # Fork-agnostic domain since address changes are valid across forks + domain = compute_domain(DOMAIN_BLS_TO_EXECUTION_CHANGE, genesis_validators_root=state.genesis_validators_root) signing_root = compute_signing_root(address_change, domain) assert bls.Verify(address_change.from_bls_pubkey, signing_root, signed_address_change.signature) diff --git a/tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_bls_to_execution_change.py b/tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_bls_to_execution_change.py index 4e8bd7502..3c692816d 100644 --- a/tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_bls_to_execution_change.py +++ b/tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_bls_to_execution_change.py @@ -177,16 +177,21 @@ def test_invalid_bad_signature(spec, state): @with_capella_and_later @spec_state_test @always_bls -def test_current_fork_version(spec, state): - """ - It should be identical to the `test_success` test case. - It is just for comparing with `test_invalid_previous_fork_version`. - """ - signed_address_change = get_signed_address_change(spec, state, fork_version=state.fork.current_version) +def test_genesis_fork_version(spec, state): + signed_address_change = get_signed_address_change(spec, state, fork_version=spec.config.GENESIS_FORK_VERSION) yield from run_bls_to_execution_change_processing(spec, state, signed_address_change) +@with_capella_and_later +@spec_state_test +@always_bls +def test_invalid_current_fork_version(spec, state): + signed_address_change = get_signed_address_change(spec, state, fork_version=state.fork.current_version) + + yield from run_bls_to_execution_change_processing(spec, state, signed_address_change, valid=False) + + @with_capella_and_later @spec_state_test @always_bls diff --git a/tests/core/pyspec/eth2spec/test/helpers/bls_to_execution_changes.py b/tests/core/pyspec/eth2spec/test/helpers/bls_to_execution_changes.py index ce0211646..0fd7e1fe8 100644 --- a/tests/core/pyspec/eth2spec/test/helpers/bls_to_execution_changes.py +++ b/tests/core/pyspec/eth2spec/test/helpers/bls_to_execution_changes.py @@ -17,10 +17,10 @@ def get_signed_address_change(spec, state, validator_index=None, withdrawal_pubk if to_execution_address is None: to_execution_address = b'\x42' * 20 - if fork_version is None: - domain = spec.get_domain(state, spec.DOMAIN_BLS_TO_EXECUTION_CHANGE) - else: - domain = spec.compute_domain(spec.DOMAIN_BLS_TO_EXECUTION_CHANGE, fork_version, state.genesis_validators_root) + domain = spec.compute_domain( + spec.DOMAIN_BLS_TO_EXECUTION_CHANGE, + fork_version=fork_version, + genesis_validators_root=state.genesis_validators_root) address_change = spec.BLSToExecutionChange( validator_index=validator_index, From 4af46e7e7841cc71ee7d949291d09d433f370e57 Mon Sep 17 00:00:00 2001 From: Hsiao-Wei Wang Date: Thu, 12 Jan 2023 23:30:09 +0800 Subject: [PATCH 2/5] Ignore `bls_to_execution_change` messages before `CAPELLA_FORK_EPOCH` --- specs/capella/p2p-interface.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/specs/capella/p2p-interface.md b/specs/capella/p2p-interface.md index 4cb23a67e..89655409c 100644 --- a/specs/capella/p2p-interface.md +++ b/specs/capella/p2p-interface.md @@ -59,8 +59,10 @@ See Capella [state transition document](./beacon-chain.md#beaconblockbody) for f This topic is used to propagate signed bls to execution change messages to be included in future blocks. +Let `system_clock_epoch` be an epoch number according to the local system clock of a node. The following validations MUST pass before forwarding the `signed_bls_to_execution_change` on the network: +- _[IGNORE]_ `system_clock_epoch >= CAPELLA_FORK_EPOCH`. - _[IGNORE]_ The `signed_bls_to_execution_change` is the first valid signed bls to execution change received for the validator with index `signed_bls_to_execution_change.message.validator_index`. - _[REJECT]_ All of the conditions within `process_bls_to_execution_change` pass validation. From 6a19cf568a734418de4e57922bfb3cf934443f28 Mon Sep 17 00:00:00 2001 From: Hsiao-Wei Wang Date: Fri, 13 Jan 2023 16:49:24 +0800 Subject: [PATCH 3/5] PR feedback from @terencechain: re-use `current_epoch` definition --- specs/capella/p2p-interface.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/specs/capella/p2p-interface.md b/specs/capella/p2p-interface.md index 89655409c..834fd44d8 100644 --- a/specs/capella/p2p-interface.md +++ b/specs/capella/p2p-interface.md @@ -59,10 +59,10 @@ See Capella [state transition document](./beacon-chain.md#beaconblockbody) for f This topic is used to propagate signed bls to execution change messages to be included in future blocks. -Let `system_clock_epoch` be an epoch number according to the local system clock of a node. The following validations MUST pass before forwarding the `signed_bls_to_execution_change` on the network: -- _[IGNORE]_ `system_clock_epoch >= CAPELLA_FORK_EPOCH`. +- _[IGNORE]_ `current_epoch >= CAPELLA_FORK_EPOCH`, + where `current_epoch` is defined by the current wall-clock time. - _[IGNORE]_ The `signed_bls_to_execution_change` is the first valid signed bls to execution change received for the validator with index `signed_bls_to_execution_change.message.validator_index`. - _[REJECT]_ All of the conditions within `process_bls_to_execution_change` pass validation. From d4eaf4bff4eac9d7a84455f1939b0be87c2e35e3 Mon Sep 17 00:00:00 2001 From: djrtwo Date: Fri, 13 Jan 2023 08:07:21 -0700 Subject: [PATCH 4/5] add invalid test for BLSChange genesis_validators_root --- .../test_process_bls_to_execution_change.py | 9 +++++++++ .../test/helpers/bls_to_execution_changes.py | 14 +++++++++++--- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_bls_to_execution_change.py b/tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_bls_to_execution_change.py index 3c692816d..ab7e14d56 100644 --- a/tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_bls_to_execution_change.py +++ b/tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_bls_to_execution_change.py @@ -199,3 +199,12 @@ def test_invalid_previous_fork_version(spec, state): signed_address_change = get_signed_address_change(spec, state, fork_version=state.fork.previous_version) yield from run_bls_to_execution_change_processing(spec, state, signed_address_change, valid=False) + + +@with_capella_and_later +@spec_state_test +@always_bls +def test_invalid_genesis_validators_root(spec, state): + signed_address_change = get_signed_address_change(spec, state, genesis_validators_root=b'\x99' * 32) + + yield from run_bls_to_execution_change_processing(spec, state, signed_address_change, valid=False) diff --git a/tests/core/pyspec/eth2spec/test/helpers/bls_to_execution_changes.py b/tests/core/pyspec/eth2spec/test/helpers/bls_to_execution_changes.py index 0fd7e1fe8..a71e5780e 100644 --- a/tests/core/pyspec/eth2spec/test/helpers/bls_to_execution_changes.py +++ b/tests/core/pyspec/eth2spec/test/helpers/bls_to_execution_changes.py @@ -2,8 +2,12 @@ from eth2spec.utils import bls from eth2spec.test.helpers.keys import pubkeys, privkeys, pubkey_to_privkey -def get_signed_address_change(spec, state, validator_index=None, withdrawal_pubkey=None, to_execution_address=None, - fork_version=None): +def get_signed_address_change(spec, state, + validator_index=None, + withdrawal_pubkey=None, + to_execution_address=None, + fork_version=None, + genesis_validators_root=None): if validator_index is None: validator_index = 0 @@ -17,10 +21,14 @@ def get_signed_address_change(spec, state, validator_index=None, withdrawal_pubk if to_execution_address is None: to_execution_address = b'\x42' * 20 + if genesis_validators_root is None: + genesis_validators_root = spec.genesis_validators_root + domain = spec.compute_domain( spec.DOMAIN_BLS_TO_EXECUTION_CHANGE, fork_version=fork_version, - genesis_validators_root=state.genesis_validators_root) + genesis_validators_root=genesis_validators_root, + ) address_change = spec.BLSToExecutionChange( validator_index=validator_index, From e1df31818bd27fc5b1c210ada93ad4d701289595 Mon Sep 17 00:00:00 2001 From: Hsiao-Wei Wang Date: Fri, 13 Jan 2023 23:18:29 +0800 Subject: [PATCH 5/5] fix typo --- .../pyspec/eth2spec/test/helpers/bls_to_execution_changes.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/core/pyspec/eth2spec/test/helpers/bls_to_execution_changes.py b/tests/core/pyspec/eth2spec/test/helpers/bls_to_execution_changes.py index a71e5780e..df8af1616 100644 --- a/tests/core/pyspec/eth2spec/test/helpers/bls_to_execution_changes.py +++ b/tests/core/pyspec/eth2spec/test/helpers/bls_to_execution_changes.py @@ -22,7 +22,7 @@ def get_signed_address_change(spec, state, to_execution_address = b'\x42' * 20 if genesis_validators_root is None: - genesis_validators_root = spec.genesis_validators_root + genesis_validators_root = state.genesis_validators_root domain = spec.compute_domain( spec.DOMAIN_BLS_TO_EXECUTION_CHANGE,