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

MUST use GENESIS_FORK_VERSION to sign BLSToExecutionChange message #3206

Merged
merged 5 commits into from
Jan 13, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
3 changes: 2 additions & 1 deletion specs/capella/beacon-chain.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
2 changes: 2 additions & 0 deletions specs/capella/p2p-interface.md
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ This topic is used to propagate signed bls to execution change messages to be in

The following validations MUST pass before forwarding the `signed_bls_to_execution_change` on the network:

- _[IGNORE]_ `current_epoch >= CAPELLA_FORK_EPOCH`,
Copy link
Contributor

Choose a reason for hiding this comment

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

should we have the wall-clock skew variability here (MAXIMUM_GOSSIP_CLOCK_DISPARITY) as we do on some gossip conditions (e.g. beacon block)?

I guess that gives the hackers something to try to game more. but some of the initial broadcasts, otherwise, will be IGNORE'd by accident due to subtle clock skew

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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -177,20 +177,34 @@ 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
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)
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,12 @@
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

Expand All @@ -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 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)
if genesis_validators_root is None:
genesis_validators_root = spec.genesis_validators_root
hwwhww marked this conversation as resolved.
Show resolved Hide resolved

domain = spec.compute_domain(
djrtwo marked this conversation as resolved.
Show resolved Hide resolved
spec.DOMAIN_BLS_TO_EXECUTION_CHANGE,
fork_version=fork_version,
genesis_validators_root=genesis_validators_root,
)

address_change = spec.BLSToExecutionChange(
validator_index=validator_index,
Expand Down