From 56dbc17793d8a6c62f506de2ced1494ea6bdc43b Mon Sep 17 00:00:00 2001 From: Danny Ryan Date: Wed, 5 Aug 2020 12:44:31 -0600 Subject: [PATCH 1/5] fix active shard count bugs --- specs/phase1/beacon-chain.md | 25 ++++++++++++++----- specs/phase1/phase1-fork.md | 1 - tests/core/pyspec/eth2spec/test/conftest.py | 6 ++++- .../phase1/unittests/test_get_start_shard.py | 13 ++++++++++ 4 files changed, 37 insertions(+), 8 deletions(-) diff --git a/specs/phase1/beacon-chain.md b/specs/phase1/beacon-chain.md index b64c31a57..4858f2575 100644 --- a/specs/phase1/beacon-chain.md +++ b/specs/phase1/beacon-chain.md @@ -106,6 +106,7 @@ Configuration is not namespaced. Instead it is strictly an extension; | Name | Value | | - | - | | `MAX_SHARDS` | `2**10` (= 1024) | +| `INITIAL_ACTIVE_SHARDS` | `2**6` (= 64) | | `LIGHT_CLIENT_COMMITTEE_SIZE` | `2**7` (= 128) | | `GASPRICE_ADJUSTMENT_COEFFICIENT` | `2**3` (= 8) | @@ -520,11 +521,24 @@ def compute_committee_source_epoch(epoch: Epoch, period: uint64) -> Epoch: ### Beacon state accessors +#### New `get_committee_count_per_slot` + +```python +def get_committee_count_per_slot(state: BeaconState, epoch: Epoch) -> uint64: + """ + Return the number of committees in each slot for the given ``epoch``. + """ + return max(uint64(1), min( + get_active_shard_count(state), + uint64(len(get_active_validator_indices(state, epoch))) // SLOTS_PER_EPOCH // TARGET_COMMITTEE_SIZE, + )) +``` + #### `get_active_shard_count` ```python def get_active_shard_count(state: BeaconState) -> uint64: - return len(state.shard_states) # May adapt in the future, or change over time. + return INITIAL_ACTIVE_SHARDS ``` #### `get_online_validator_indices` @@ -545,12 +559,11 @@ def get_shard_committee(beacon_state: BeaconState, epoch: Epoch, shard: Shard) - source_epoch = compute_committee_source_epoch(epoch, SHARD_COMMITTEE_PERIOD) active_validator_indices = get_active_validator_indices(beacon_state, source_epoch) seed = get_seed(beacon_state, source_epoch, DOMAIN_SHARD_COMMITTEE) - active_shard_count = get_active_shard_count(beacon_state) return compute_committee( indices=active_validator_indices, seed=seed, index=shard, - count=active_shard_count, + count=get_active_shard_count(beacon_state), ) ``` @@ -617,10 +630,11 @@ def get_start_shard(state: BeaconState, slot: Slot) -> Shard: else: # Previous epoch shard_delta = get_committee_count_delta(state, start_slot=slot, stop_slot=current_epoch_start_slot) - max_committees_per_epoch = MAX_COMMITTEES_PER_SLOT * SLOTS_PER_EPOCH + max_committees_per_slot = active_shard_count + max_committees_in_span = max_committees_per_slot * (current_epoch_start_slot - slot) return Shard( # Ensure positive - (state.current_epoch_start_shard + max_committees_per_epoch * active_shard_count - shard_delta) + (state.current_epoch_start_shard + max_committees_in_span - shard_delta) % active_shard_count ) ``` @@ -752,7 +766,6 @@ def process_operations(state: BeaconState, body: BeaconBlockBody) -> None: def validate_attestation(state: BeaconState, attestation: Attestation) -> None: data = attestation.data assert data.index < get_committee_count_per_slot(state, data.target.epoch) - assert data.index < get_active_shard_count(state) assert data.target.epoch in (get_previous_epoch(state), get_current_epoch(state)) assert data.target.epoch == compute_epoch_at_slot(data.slot) assert data.slot + MIN_ATTESTATION_INCLUSION_DELAY <= state.slot <= data.slot + SLOTS_PER_EPOCH diff --git a/specs/phase1/phase1-fork.md b/specs/phase1/phase1-fork.md index e3949aca8..a094b01fc 100644 --- a/specs/phase1/phase1-fork.md +++ b/specs/phase1/phase1-fork.md @@ -36,7 +36,6 @@ Warning: this configuration is not definitive. | - | - | | `PHASE_1_FORK_VERSION` | `Version('0x01000000')` | | `PHASE_1_FORK_SLOT` | `Slot(0)` **TBD** | -| `INITIAL_ACTIVE_SHARDS` | `2**6` (= 64) | ## Fork to Phase 1 diff --git a/tests/core/pyspec/eth2spec/test/conftest.py b/tests/core/pyspec/eth2spec/test/conftest.py index 21f7c7abb..addb31dde 100644 --- a/tests/core/pyspec/eth2spec/test/conftest.py +++ b/tests/core/pyspec/eth2spec/test/conftest.py @@ -50,7 +50,11 @@ def config(request): @fixture(autouse=True) def bls_default(request): - disable_bls = request.config.getoption("--disable-bls") + try: + disable_bls = request.config.getoption("--disable-bls") + except ValueError: + disable_bls = False + if disable_bls: context.DEFAULT_BLS_ACTIVE = False diff --git a/tests/core/pyspec/eth2spec/test/phase1/unittests/test_get_start_shard.py b/tests/core/pyspec/eth2spec/test/phase1/unittests/test_get_start_shard.py index c443545b9..a802d6c3c 100644 --- a/tests/core/pyspec/eth2spec/test/phase1/unittests/test_get_start_shard.py +++ b/tests/core/pyspec/eth2spec/test/phase1/unittests/test_get_start_shard.py @@ -74,3 +74,16 @@ def test_get_start_shard_previous_slot(spec, state): - spec.get_committee_count_delta(state, start_slot=slot, stop_slot=current_epoch_start_slot) ) % active_shard_count assert start_shard == expected_start_shard + + +@with_all_phases_except([PHASE0]) +@spec_state_test +def test_get_start_shard_far_past_epoch(spec, state): + initial_epoch = spec.get_current_epoch(state) + initial_start_slot = spec.compute_start_slot_at_epoch(initial_epoch) + initial_start_shard = state.current_epoch_start_shard + + for _ in range(spec.MAX_SHARDS + 2): + next_epoch(spec, state) + + assert spec.get_start_shard(state, initial_start_slot) == initial_start_shard From c3ae85387c8b561425da4bad7597a50565539bb1 Mon Sep 17 00:00:00 2001 From: Danny Ryan Date: Wed, 5 Aug 2020 13:04:13 -0600 Subject: [PATCH 2/5] add larger validator set test for regression test for active_shard_count issue --- specs/phase1/beacon-chain.md | 1 + tests/core/pyspec/eth2spec/test/context.py | 9 ++++ .../test/phase0/sanity/test_blocks.py | 45 +++++++++++++++++++ 3 files changed, 55 insertions(+) diff --git a/specs/phase1/beacon-chain.md b/specs/phase1/beacon-chain.md index 4858f2575..3cba3188d 100644 --- a/specs/phase1/beacon-chain.md +++ b/specs/phase1/beacon-chain.md @@ -47,6 +47,7 @@ - [`compute_updated_gasprice`](#compute_updated_gasprice) - [`compute_committee_source_epoch`](#compute_committee_source_epoch) - [Beacon state accessors](#beacon-state-accessors) + - [New `get_committee_count_per_slot`](#new-get_committee_count_per_slot) - [`get_active_shard_count`](#get_active_shard_count) - [`get_online_validator_indices`](#get_online_validator_indices) - [`get_shard_committee`](#get_shard_committee) diff --git a/tests/core/pyspec/eth2spec/test/context.py b/tests/core/pyspec/eth2spec/test/context.py index 68696ea0a..69be2fd0a 100644 --- a/tests/core/pyspec/eth2spec/test/context.py +++ b/tests/core/pyspec/eth2spec/test/context.py @@ -151,6 +151,15 @@ def low_single_balance(spec): return [1] +def large_validator_set(spec): + """ + Helper method to create a series of default balances. + Usage: `@with_custom_state(balances_fn=default_balances, ...)` + """ + num_validators = 2 * spec.SLOTS_PER_EPOCH * spec.MAX_COMMITTEES_PER_SLOT * spec.TARGET_COMMITTEE_SIZE + return [spec.MAX_EFFECTIVE_BALANCE] * num_validators + + def single_phase(fn): """ Decorator that filters out the phases data. diff --git a/tests/core/pyspec/eth2spec/test/phase0/sanity/test_blocks.py b/tests/core/pyspec/eth2spec/test/phase0/sanity/test_blocks.py index 6d7ff096a..3ec76e8b8 100644 --- a/tests/core/pyspec/eth2spec/test/phase0/sanity/test_blocks.py +++ b/tests/core/pyspec/eth2spec/test/phase0/sanity/test_blocks.py @@ -22,7 +22,10 @@ from eth2spec.test.helpers.shard_transitions import get_shard_transition_of_comm from eth2spec.test.context import ( PHASE0, PHASE1, + spec_test, spec_state_test, with_all_phases, expect_assertion_error, always_bls, with_phases, + with_custom_state, single_phase, + large_validator_set, ) @@ -85,6 +88,28 @@ def test_empty_block_transition(spec, state): assert spec.get_randao_mix(state, spec.get_current_epoch(state)) != spec.Bytes32() +@with_all_phases +@spec_test +@with_custom_state(balances_fn=large_validator_set, threshold_fn=lambda spec: spec.EJECTION_BALANCE) +@single_phase +def test_empty_block_transition_large_validator_set(spec, state): + pre_slot = state.slot + pre_eth1_votes = len(state.eth1_data_votes) + + yield 'pre', state + + block = build_empty_block_for_next_slot(spec, state) + + signed_block = state_transition_and_sign_block(spec, state, block) + + yield 'blocks', [signed_block] + yield 'post', state + + assert len(state.eth1_data_votes) == pre_eth1_votes + 1 + assert spec.get_block_root_at_slot(state, pre_slot) == signed_block.message.parent_root + assert spec.get_randao_mix(state, spec.get_current_epoch(state)) != spec.Bytes32() + + def process_and_sign_block_without_header_validations(spec, state, block): """ Artificially bypass the restrictions in the state transition to transition and sign block @@ -288,6 +313,26 @@ def test_empty_epoch_transition(spec, state): assert spec.get_block_root_at_slot(state, slot) == block.parent_root +@with_all_phases +@spec_test +@with_custom_state(balances_fn=large_validator_set, threshold_fn=lambda spec: spec.EJECTION_BALANCE) +@single_phase +def test_empty_epoch_transition_large_validator_set(spec, state): + pre_slot = state.slot + yield 'pre', state + + block = build_empty_block(spec, state, state.slot + spec.SLOTS_PER_EPOCH) + + signed_block = state_transition_and_sign_block(spec, state, block) + + yield 'blocks', [signed_block] + yield 'post', state + + assert state.slot == block.slot + for slot in range(pre_slot, state.slot): + assert spec.get_block_root_at_slot(state, slot) == block.parent_root + + @with_all_phases @spec_state_test def test_empty_epoch_transition_not_finalizing(spec, state): From 76c96782e24f6aa26b4a0dd129208ae9e205f54b Mon Sep 17 00:00:00 2001 From: Danny Ryan Date: Mon, 10 Aug 2020 12:58:57 -0600 Subject: [PATCH 3/5] fix disable-bls default value --- tests/core/pyspec/eth2spec/test/conftest.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/core/pyspec/eth2spec/test/conftest.py b/tests/core/pyspec/eth2spec/test/conftest.py index addb31dde..b961ee082 100644 --- a/tests/core/pyspec/eth2spec/test/conftest.py +++ b/tests/core/pyspec/eth2spec/test/conftest.py @@ -31,7 +31,7 @@ def pytest_addoption(parser): help="config: make the pyspec use the specified configuration" ) parser.addoption( - "--disable-bls", action="store_true", + "--disable-bls", action="store_true", default=False, help="bls-default: make tests that are not dependent on BLS run without BLS" ) parser.addoption( @@ -50,11 +50,7 @@ def config(request): @fixture(autouse=True) def bls_default(request): - try: - disable_bls = request.config.getoption("--disable-bls") - except ValueError: - disable_bls = False - + disable_bls = request.config.getoption("--disable-bls") if disable_bls: context.DEFAULT_BLS_ACTIVE = False From 8b39d41145e0fedbe6b6207596ee0cff1567ba29 Mon Sep 17 00:00:00 2001 From: Danny Ryan Date: Mon, 10 Aug 2020 13:02:22 -0600 Subject: [PATCH 4/5] PR feedback --- specs/phase1/beacon-chain.md | 8 ++++++-- .../pyspec/eth2spec/test/phase0/sanity/test_blocks.py | 6 ++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/specs/phase1/beacon-chain.md b/specs/phase1/beacon-chain.md index 3cba3188d..f31be03a2 100644 --- a/specs/phase1/beacon-chain.md +++ b/specs/phase1/beacon-chain.md @@ -47,7 +47,7 @@ - [`compute_updated_gasprice`](#compute_updated_gasprice) - [`compute_committee_source_epoch`](#compute_committee_source_epoch) - [Beacon state accessors](#beacon-state-accessors) - - [New `get_committee_count_per_slot`](#new-get_committee_count_per_slot) + - [Updated `get_committee_count_per_slot`](#updated-get_committee_count_per_slot) - [`get_active_shard_count`](#get_active_shard_count) - [`get_online_validator_indices`](#get_online_validator_indices) - [`get_shard_committee`](#get_shard_committee) @@ -522,7 +522,7 @@ def compute_committee_source_epoch(epoch: Epoch, period: uint64) -> Epoch: ### Beacon state accessors -#### New `get_committee_count_per_slot` +#### Updated `get_committee_count_per_slot` ```python def get_committee_count_per_slot(state: BeaconState, epoch: Epoch) -> uint64: @@ -539,6 +539,10 @@ def get_committee_count_per_slot(state: BeaconState, epoch: Epoch) -> uint64: ```python def get_active_shard_count(state: BeaconState) -> uint64: + """ + Return the number of active shards. + Note that this puts an upper bound on the number of committees per slot. + """ return INITIAL_ACTIVE_SHARDS ``` diff --git a/tests/core/pyspec/eth2spec/test/phase0/sanity/test_blocks.py b/tests/core/pyspec/eth2spec/test/phase0/sanity/test_blocks.py index 3ec76e8b8..fa4e690af 100644 --- a/tests/core/pyspec/eth2spec/test/phase0/sanity/test_blocks.py +++ b/tests/core/pyspec/eth2spec/test/phase0/sanity/test_blocks.py @@ -76,6 +76,9 @@ def test_empty_block_transition(spec, state): yield 'pre', state + # Ensure pre-state has default randao + assert spec.get_randao_mix(state, spec.get_current_epoch(state)) == spec.Bytes32() + block = build_empty_block_for_next_slot(spec, state) signed_block = state_transition_and_sign_block(spec, state, block) @@ -98,6 +101,9 @@ def test_empty_block_transition_large_validator_set(spec, state): yield 'pre', state + # Ensure pre-state has default randao + assert spec.get_randao_mix(state, spec.get_current_epoch(state)) == spec.Bytes32() + block = build_empty_block_for_next_slot(spec, state) signed_block = state_transition_and_sign_block(spec, state, block) From b02c2e1c498ef36d65c5b3204208e94f89520a07 Mon Sep 17 00:00:00 2001 From: Danny Ryan Date: Mon, 10 Aug 2020 21:00:23 -0600 Subject: [PATCH 5/5] fix randao mix in sanity tests --- .../eth2spec/test/phase0/sanity/test_blocks.py | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/tests/core/pyspec/eth2spec/test/phase0/sanity/test_blocks.py b/tests/core/pyspec/eth2spec/test/phase0/sanity/test_blocks.py index fa4e690af..5b98abac4 100644 --- a/tests/core/pyspec/eth2spec/test/phase0/sanity/test_blocks.py +++ b/tests/core/pyspec/eth2spec/test/phase0/sanity/test_blocks.py @@ -73,12 +73,10 @@ def test_same_slot_block_transition(spec, state): def test_empty_block_transition(spec, state): pre_slot = state.slot pre_eth1_votes = len(state.eth1_data_votes) + pre_mix = spec.get_randao_mix(state, spec.get_current_epoch(state)) yield 'pre', state - # Ensure pre-state has default randao - assert spec.get_randao_mix(state, spec.get_current_epoch(state)) == spec.Bytes32() - block = build_empty_block_for_next_slot(spec, state) signed_block = state_transition_and_sign_block(spec, state, block) @@ -88,7 +86,7 @@ def test_empty_block_transition(spec, state): assert len(state.eth1_data_votes) == pre_eth1_votes + 1 assert spec.get_block_root_at_slot(state, pre_slot) == signed_block.message.parent_root - assert spec.get_randao_mix(state, spec.get_current_epoch(state)) != spec.Bytes32() + assert spec.get_randao_mix(state, spec.get_current_epoch(state)) != pre_mix @with_all_phases @@ -98,12 +96,10 @@ def test_empty_block_transition(spec, state): def test_empty_block_transition_large_validator_set(spec, state): pre_slot = state.slot pre_eth1_votes = len(state.eth1_data_votes) + pre_mix = spec.get_randao_mix(state, spec.get_current_epoch(state)) yield 'pre', state - # Ensure pre-state has default randao - assert spec.get_randao_mix(state, spec.get_current_epoch(state)) == spec.Bytes32() - block = build_empty_block_for_next_slot(spec, state) signed_block = state_transition_and_sign_block(spec, state, block) @@ -113,7 +109,7 @@ def test_empty_block_transition_large_validator_set(spec, state): assert len(state.eth1_data_votes) == pre_eth1_votes + 1 assert spec.get_block_root_at_slot(state, pre_slot) == signed_block.message.parent_root - assert spec.get_randao_mix(state, spec.get_current_epoch(state)) != spec.Bytes32() + assert spec.get_randao_mix(state, spec.get_current_epoch(state)) != pre_mix def process_and_sign_block_without_header_validations(spec, state, block):