From ff706e5c7a8bc68422595e7d3c2c55c9ca2183db Mon Sep 17 00:00:00 2001 From: Danny Ryan Date: Mon, 10 May 2021 12:57:11 -0600 Subject: [PATCH 1/7] add logic for handling sync committee off by one issue --- specs/altair/p2p-interface.md | 11 ++++++++++- specs/altair/validator.md | 6 +++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/specs/altair/p2p-interface.md b/specs/altair/p2p-interface.md index 85e859191..02a159f7a 100644 --- a/specs/altair/p2p-interface.md +++ b/specs/altair/p2p-interface.md @@ -106,9 +106,18 @@ The following validations MUST pass before forwarding the `signed_contribution_a ```python def get_sync_subcommittee_pubkeys(state: BeaconState, subcommittee_index: uint64) -> Sequence[BLSPubkey]: + # Committees assigned to `slot` sign for `slot - 1` + # This creates the exceptional logic below when transitioning between sync comittee periods + next_slot_epoch = compute_epoch_at_slot(state.slot + 1) + if compute_sync_committee_period(get_current_epoch(state)) == compute_sync_committee_period(next_slot_epoch): + sync_committee = state.current_sync_committee + else: + sync_committee = state.next_sync_committee + + # Return pubkeys for the subcommittee index sync_subcommittee_size = SYNC_COMMITTEE_SIZE // SYNC_COMMITTEE_SUBNET_COUNT i = subcommittee_index * sync_subcommittee_size - return state.current_sync_committee.pubkeys[i:i + sync_subcommittee_size] + return sync_committee.pubkeys[i:i + sync_subcommittee_size] ``` - _[IGNORE]_ The contribution's slot is for the current slot, i.e. `contribution.slot == current_slot`. diff --git a/specs/altair/validator.md b/specs/altair/validator.md index dbcc73486..3a96adb46 100644 --- a/specs/altair/validator.md +++ b/specs/altair/validator.md @@ -143,6 +143,10 @@ A validator determines beacon committee assignments and beacon block proposal du To determine sync committee assignments, a validator can run the following function: `is_assigned_to_sync_committee(state, epoch, validator_index)` where `epoch` is an epoch number within the current or next sync committee period. This function is a predicate indicating the presence or absence of the validator in the corresponding sync committee for the queried sync committee period. +*Note*: Being assigned to a sync committee for a given `slot` means that the validator produces and broadcasts signatures for `slot - 1` for inclusion in `slot`. +This means that when assigned to an `epoch` sync committee signatures must be produced and broadcast for slots on range `[compute_start_slot_at_epoch(epoch) - 1, compute_start_slot_at_epoch(epoch) + SLOTS_PER_EPOCH - 1)` +rather than for the range `[compute_start_slot_at_epoch(epoch), compute_start_slot_at_epoch(epoch) + SLOTS_PER_EPOCH)`. + ```python def compute_sync_committee_period(epoch: Epoch) -> uint64: return epoch // EPOCHS_PER_SYNC_COMMITTEE_PERIOD @@ -261,7 +265,7 @@ This process occurs each slot. ##### Prepare sync committee signature -If a validator is in the current sync committee (i.e. `is_assigned_to_sync_committee()` above returns `True`), then for every slot in the current sync committee period, the validator should prepare a `SyncCommitteeSignature` according to the logic in `get_sync_committee_signature` as soon as they have determined the head block of the current slot. +If a validator is in the current sync committee (i.e. `is_assigned_to_sync_committee()` above returns `True`), then for every `slot` in the current sync committee period, the validator should prepare a `SyncCommitteeSignature` for the previous slot (`slot - 1`) according to the logic in `get_sync_committee_signature` as soon as they have determined the head block of `slot - 1`. This logic is triggered upon the same conditions as when producing an attestation. Meaning, a sync committee member should produce and broadcast a `SyncCommitteeSignature` either when (a) the validator has received a valid block from the expected block proposer for the current `slot` or (b) one-third of the slot has transpired (`SECONDS_PER_SLOT / 3` seconds after the start of the slot) -- whichever comes first. From d8e2d19ecc510df32ba3e86471e8c760f14dcb68 Mon Sep 17 00:00:00 2001 From: Danny Ryan Date: Mon, 10 May 2021 13:01:31 -0600 Subject: [PATCH 2/7] spelling --- specs/altair/p2p-interface.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specs/altair/p2p-interface.md b/specs/altair/p2p-interface.md index 02a159f7a..df2955f13 100644 --- a/specs/altair/p2p-interface.md +++ b/specs/altair/p2p-interface.md @@ -107,7 +107,7 @@ The following validations MUST pass before forwarding the `signed_contribution_a ```python def get_sync_subcommittee_pubkeys(state: BeaconState, subcommittee_index: uint64) -> Sequence[BLSPubkey]: # Committees assigned to `slot` sign for `slot - 1` - # This creates the exceptional logic below when transitioning between sync comittee periods + # This creates the exceptional logic below when transitioning between sync committee periods next_slot_epoch = compute_epoch_at_slot(state.slot + 1) if compute_sync_committee_period(get_current_epoch(state)) == compute_sync_committee_period(next_slot_epoch): sync_committee = state.current_sync_committee From 85198fabfa15d01832484a8e5f4386bbe2964967 Mon Sep 17 00:00:00 2001 From: Danny Ryan Date: Mon, 10 May 2021 13:26:43 -0600 Subject: [PATCH 3/7] lint --- specs/altair/p2p-interface.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specs/altair/p2p-interface.md b/specs/altair/p2p-interface.md index df2955f13..6f250b57e 100644 --- a/specs/altair/p2p-interface.md +++ b/specs/altair/p2p-interface.md @@ -108,7 +108,7 @@ The following validations MUST pass before forwarding the `signed_contribution_a def get_sync_subcommittee_pubkeys(state: BeaconState, subcommittee_index: uint64) -> Sequence[BLSPubkey]: # Committees assigned to `slot` sign for `slot - 1` # This creates the exceptional logic below when transitioning between sync committee periods - next_slot_epoch = compute_epoch_at_slot(state.slot + 1) + next_slot_epoch = compute_epoch_at_slot(Slot(state.slot + 1)) if compute_sync_committee_period(get_current_epoch(state)) == compute_sync_committee_period(next_slot_epoch): sync_committee = state.current_sync_committee else: From 3c609e02eac16e060ec3934cbe1807c5a2b7e5cf Mon Sep 17 00:00:00 2001 From: Danny Ryan Date: Tue, 11 May 2021 07:28:24 -0600 Subject: [PATCH 4/7] pr feedback --- specs/altair/validator.md | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/specs/altair/validator.md b/specs/altair/validator.md index 3a96adb46..92e3a75c0 100644 --- a/specs/altair/validator.md +++ b/specs/altair/validator.md @@ -146,6 +146,7 @@ This function is a predicate indicating the presence or absence of the validator *Note*: Being assigned to a sync committee for a given `slot` means that the validator produces and broadcasts signatures for `slot - 1` for inclusion in `slot`. This means that when assigned to an `epoch` sync committee signatures must be produced and broadcast for slots on range `[compute_start_slot_at_epoch(epoch) - 1, compute_start_slot_at_epoch(epoch) + SLOTS_PER_EPOCH - 1)` rather than for the range `[compute_start_slot_at_epoch(epoch), compute_start_slot_at_epoch(epoch) + SLOTS_PER_EPOCH)`. +To reduce complexity during the Altair fork, sync committees are not expected to produce signatures for `compute_epoch_at_slot(ALTAIR_FORK_EPOCH) - 1`. ```python def compute_sync_committee_period(epoch: Epoch) -> uint64: @@ -296,11 +297,14 @@ The `subnet_id` is derived from the position in the sync committee such that the ```python def compute_subnets_for_sync_committee(state: BeaconState, validator_index: ValidatorIndex) -> Sequence[uint64]: + next_slot_epoch = compute_epoch_at_slot(Slot(state.slot + 1)) + if compute_sync_committee_period(get_current_epoch(state)) == compute_sync_committee_period(next_slot_epoch): + sync_committee = state.current_sync_committee + else: + sync_committee = state.next_sync_committee + target_pubkey = state.validators[validator_index].pubkey - sync_committee_indices = [ - index for index, pubkey in enumerate(state.current_sync_committee.pubkeys) - if pubkey == target_pubkey - ] + sync_committee_indices = [index for index, pubkey in enumerate(sync_committee.pubkeys) if pubkey == target_pubkey] return [ uint64(index // (SYNC_COMMITTEE_SIZE // SYNC_COMMITTEE_SUBNET_COUNT)) for index in sync_committee_indices From dad698f97aec8f6e80ecc74d093e050b9ae72163 Mon Sep 17 00:00:00 2001 From: Hsiao-Wei Wang Date: Wed, 12 May 2021 12:35:47 +0800 Subject: [PATCH 5/7] Update unit tests: add `test_compute_subnets_for_sync_committee_slot_period_boundary` --- .../unittests/validator/test_validator.py | 49 +++++++++++++++++-- 1 file changed, 45 insertions(+), 4 deletions(-) diff --git a/tests/core/pyspec/eth2spec/test/altair/unittests/validator/test_validator.py b/tests/core/pyspec/eth2spec/test/altair/unittests/validator/test_validator.py index c8a894da6..fd1b82c4b 100644 --- a/tests/core/pyspec/eth2spec/test/altair/unittests/validator/test_validator.py +++ b/tests/core/pyspec/eth2spec/test/altair/unittests/validator/test_validator.py @@ -143,20 +143,61 @@ def _subnet_for_sync_committee_index(spec, i): return i // (spec.SYNC_COMMITTEE_SIZE // spec.SYNC_COMMITTEE_SUBNET_COUNT) +def _get_expected_subnets_by_pubkey(sync_committee_members): + expected_subnets_by_pubkey = defaultdict(list) + for (subnet, pubkey) in sync_committee_members: + expected_subnets_by_pubkey[pubkey].append(subnet) + return expected_subnets_by_pubkey + + @with_altair_and_later @with_state def test_compute_subnets_for_sync_committee(state, spec, phases): + # Transition to the head of the next period + transition_to(spec, state, spec.SLOTS_PER_EPOCH * spec.EPOCHS_PER_SYNC_COMMITTEE_PERIOD) + + next_slot_epoch = spec.compute_epoch_at_slot(state.slot + 1) + assert ( + spec.compute_sync_committee_period(spec.get_current_epoch(state)) + == spec.compute_sync_committee_period(next_slot_epoch) + ) some_sync_committee_members = list( ( _subnet_for_sync_committee_index(spec, i), pubkey, ) + # use current_sync_committee for i, pubkey in enumerate(state.current_sync_committee.pubkeys) ) - - expected_subnets_by_pubkey = defaultdict(list) - for (subnet, pubkey) in some_sync_committee_members: - expected_subnets_by_pubkey[pubkey].append(subnet) + expected_subnets_by_pubkey = _get_expected_subnets_by_pubkey(some_sync_committee_members) + + for _, pubkey in some_sync_committee_members: + validator_index = _validator_index_for_pubkey(state, pubkey) + subnets = spec.compute_subnets_for_sync_committee(state, validator_index) + expected_subnets = expected_subnets_by_pubkey[pubkey] + assert subnets == expected_subnets + + +@with_altair_and_later +@with_state +def test_compute_subnets_for_sync_committee_slot_period_boundary(state, spec, phases): + # Transition to the end of the period + transition_to(spec, state, spec.SLOTS_PER_EPOCH * spec.EPOCHS_PER_SYNC_COMMITTEE_PERIOD - 1) + + next_slot_epoch = spec.compute_epoch_at_slot(state.slot + 1) + assert ( + spec.compute_sync_committee_period(spec.get_current_epoch(state)) + != spec.compute_sync_committee_period(next_slot_epoch) + ) + some_sync_committee_members = list( + ( + _subnet_for_sync_committee_index(spec, i), + pubkey, + ) + # use next_sync_committee + for i, pubkey in enumerate(state.next_sync_committee.pubkeys) + ) + expected_subnets_by_pubkey = _get_expected_subnets_by_pubkey(some_sync_committee_members) for _, pubkey in some_sync_committee_members: validator_index = _validator_index_for_pubkey(state, pubkey) From 17820e371165c4f6ee56163e20cff7da36ba1033 Mon Sep 17 00:00:00 2001 From: Hsiao-Wei Wang Date: Wed, 12 May 2021 13:02:15 +0800 Subject: [PATCH 6/7] Skip the mainnet config slow tests --- .../test/altair/unittests/validator/test_validator.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/core/pyspec/eth2spec/test/altair/unittests/validator/test_validator.py b/tests/core/pyspec/eth2spec/test/altair/unittests/validator/test_validator.py index fd1b82c4b..cefaaf694 100644 --- a/tests/core/pyspec/eth2spec/test/altair/unittests/validator/test_validator.py +++ b/tests/core/pyspec/eth2spec/test/altair/unittests/validator/test_validator.py @@ -8,8 +8,12 @@ from eth2spec.utils import bls from eth2spec.utils.bls import only_with_bls from eth2spec.test.context import ( with_altair_and_later, + with_configs, with_state, ) +from eth2spec.test.helpers.constants import ( + MINIMAL, +) rng = random.Random(1337) @@ -91,6 +95,7 @@ def _get_sync_committee_signature( @only_with_bls() @with_altair_and_later +@with_configs([MINIMAL], reason="too slow") @with_state def test_process_sync_committee_contributions(phases, spec, state): # skip over slots at genesis @@ -151,6 +156,7 @@ def _get_expected_subnets_by_pubkey(sync_committee_members): @with_altair_and_later +@with_configs([MINIMAL], reason="too slow") @with_state def test_compute_subnets_for_sync_committee(state, spec, phases): # Transition to the head of the next period @@ -179,6 +185,7 @@ def test_compute_subnets_for_sync_committee(state, spec, phases): @with_altair_and_later +@with_configs([MINIMAL], reason="too slow") @with_state def test_compute_subnets_for_sync_committee_slot_period_boundary(state, spec, phases): # Transition to the end of the period From 8e07ece4927bbc3d7965b92b656abd7e09a73fc1 Mon Sep 17 00:00:00 2001 From: Hsiao-Wei Wang Date: Wed, 12 May 2021 14:04:49 +0800 Subject: [PATCH 7/7] Minor rephrase --- specs/altair/validator.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/specs/altair/validator.md b/specs/altair/validator.md index 92e3a75c0..67e5914d1 100644 --- a/specs/altair/validator.md +++ b/specs/altair/validator.md @@ -271,7 +271,7 @@ If a validator is in the current sync committee (i.e. `is_assigned_to_sync_commi This logic is triggered upon the same conditions as when producing an attestation. Meaning, a sync committee member should produce and broadcast a `SyncCommitteeSignature` either when (a) the validator has received a valid block from the expected block proposer for the current `slot` or (b) one-third of the slot has transpired (`SECONDS_PER_SLOT / 3` seconds after the start of the slot) -- whichever comes first. -`get_sync_committee_signature()` assumes `state` is the head state corresponding to processing the block up to the current slot as determined by the fork choice (including any empty slots up to the current slot processed with `process_slots` on top of the latest block), `block_root` is the root of the head block, `validator_index` is the index of the validator in the registry `state.validators` controlled by `privkey`, and `privkey` is the BLS private key for the validator. +`get_sync_committee_signature(state, block_root, validator_index, privkey)` assumes the parameter `state` is the head state corresponding to processing the block up to the current slot as determined by the fork choice (including any empty slots up to the current slot processed with `process_slots` on top of the latest block), `block_root` is the root of the head block, `validator_index` is the index of the validator in the registry `state.validators` controlled by `privkey`, and `privkey` is the BLS private key for the validator. ```python def get_sync_committee_signature(state: BeaconState, @@ -291,7 +291,7 @@ def get_sync_committee_signature(state: BeaconState, The validator broadcasts the assembled signature to the assigned subnet, the `sync_committee_{subnet_id}` pubsub topic. The `subnet_id` is derived from the position in the sync committee such that the sync committee is divided into "subcommittees". -`subnet_id` can be computed via `compute_subnets_for_sync_committee()` where `state` is a `BeaconState` during the matching sync committee period. +`subnet_id` can be computed via `compute_subnets_for_sync_committee(state, validator_index)` where `state` is a `BeaconState` during the matching sync committee period. *Note*: This function returns multiple subnets if a given validator index is included multiple times in a given sync committee across multiple subcommittees.