From 09cae4b3ccab47fcb7f9eba198af46d3bcdfa3d3 Mon Sep 17 00:00:00 2001 From: protolambda Date: Fri, 1 May 2020 15:17:41 +0200 Subject: [PATCH 1/3] Handle empty-aggregation-bits case, and add tests. See #1713 --- specs/phase0/beacon-chain.md | 4 ++-- specs/phase0/p2p-interface.md | 2 +- specs/phase1/beacon-chain.md | 3 ++- .../eth2spec/test/helpers/attestations.py | 3 +++ .../test_process_attestation.py | 23 +++++++++++++++++++ .../test_process_rewards_and_penalties.py | 3 +++ 6 files changed, 34 insertions(+), 4 deletions(-) diff --git a/specs/phase0/beacon-chain.md b/specs/phase0/beacon-chain.md index cdf38dc1e..5ab499bfc 100644 --- a/specs/phase0/beacon-chain.md +++ b/specs/phase0/beacon-chain.md @@ -684,11 +684,11 @@ def is_slashable_attestation_data(data_1: AttestationData, data_2: AttestationDa ```python def is_valid_indexed_attestation(state: BeaconState, indexed_attestation: IndexedAttestation) -> bool: """ - Check if ``indexed_attestation`` has sorted and unique indices and a valid aggregate signature. + Check if ``indexed_attestation`` is not empty, has sorted and unique indices and has a valid aggregate signature. """ # Verify indices are sorted and unique indices = indexed_attestation.attesting_indices - if not indices == sorted(set(indices)): + if len(indices) == 0 or not indices == sorted(set(indices)): return False # Verify aggregate signature pubkeys = [state.validators[i].pubkey for i in indices] diff --git a/specs/phase0/p2p-interface.md b/specs/phase0/p2p-interface.md index 7197581dc..a44de8c8e 100644 --- a/specs/phase0/p2p-interface.md +++ b/specs/phase0/p2p-interface.md @@ -293,7 +293,7 @@ There are two primary global topics used to propagate beacon blocks and aggregat - The `aggregate` is the first valid aggregate received for the aggregator with index `aggregate_and_proof.aggregator_index` for the epoch `aggregate.data.target.epoch`. - The block being voted for (`aggregate.data.beacon_block_root`) passes validation. - `aggregate_and_proof.selection_proof` selects the validator as an aggregator for the slot -- i.e. `is_aggregator(state, aggregate.data.slot, aggregate.data.index, aggregate_and_proof.selection_proof)` returns `True`. - - The aggregator's validator index is within the aggregate's committee -- i.e. `aggregate_and_proof.aggregator_index in get_attesting_indices(state, aggregate.data, aggregate.aggregation_bits)`. + - The aggregator's validator index is within the aggregate's committee -- i.e. `aggregate_and_proof.aggregator_index in get_attesting_indices(state, aggregate.data, aggregate.aggregation_bits)`. This also means that it must never have an empty set of participants. - The `aggregate_and_proof.selection_proof` is a valid signature of the `aggregate.data.slot` by the validator with index `aggregate_and_proof.aggregator_index`. - The aggregator signature, `signed_aggregate_and_proof.signature`, is valid. - The signature of `aggregate` is valid. diff --git a/specs/phase1/beacon-chain.md b/specs/phase1/beacon-chain.md index 596b3818f..1770b7a98 100644 --- a/specs/phase1/beacon-chain.md +++ b/specs/phase1/beacon-chain.md @@ -571,7 +571,8 @@ def is_valid_indexed_attestation(state: BeaconState, indexed_attestation: Indexe attestation = indexed_attestation.attestation domain = get_domain(state, DOMAIN_BEACON_ATTESTER, attestation.data.target.epoch) aggregation_bits = attestation.aggregation_bits - assert len(aggregation_bits) == len(indexed_attestation.committee) + if not any(aggregation_bits) or len(aggregation_bits) != len(indexed_attestation.committee): + return False if len(attestation.custody_bits_blocks) == 0: # fall back on phase0 behavior if there is no shard data. diff --git a/tests/core/pyspec/eth2spec/test/helpers/attestations.py b/tests/core/pyspec/eth2spec/test/helpers/attestations.py index 8215f5c5b..377426006 100644 --- a/tests/core/pyspec/eth2spec/test/helpers/attestations.py +++ b/tests/core/pyspec/eth2spec/test/helpers/attestations.py @@ -114,6 +114,9 @@ def get_valid_late_attestation(spec, state, slot=None, index=None, signed=False) def get_valid_attestation(spec, state, slot=None, index=None, empty=False, signed=False, on_time=True): + # If empty is true, the attestation has 0 participants, and will not be signed. + # Thus strictly speaking invalid when no participant is added later. + if slot is None: slot = state.slot if index is None: diff --git a/tests/core/pyspec/eth2spec/test/phase_0/block_processing/test_process_attestation.py b/tests/core/pyspec/eth2spec/test/phase_0/block_processing/test_process_attestation.py index 8663391aa..df3279faa 100644 --- a/tests/core/pyspec/eth2spec/test/phase_0/block_processing/test_process_attestation.py +++ b/tests/core/pyspec/eth2spec/test/phase_0/block_processing/test_process_attestation.py @@ -64,6 +64,29 @@ def test_invalid_attestation_signature(spec, state): yield from run_attestation_processing(spec, state, attestation, False) +@with_all_phases +@spec_state_test +@always_bls +def test_empty_participants_zeroes_sig(spec, state): + attestation = get_valid_attestation(spec, state, empty=True) + attestation.signature = spec.BLSSignature(b'\x00' * 96) + next_slots(spec, state, spec.MIN_ATTESTATION_INCLUSION_DELAY) + + yield from run_attestation_processing(spec, state, attestation, False) + + +@with_all_phases +@spec_state_test +@always_bls +def test_empty_participants_seemingly_valid_sig(spec, state): + attestation = get_valid_attestation(spec, state, empty=True) + # Special BLS value, valid for zero pubkeys on some (but not all) BLS implementations. + attestation.signature = spec.BLSSignature(b'\xc0' + b'\x00' * 95) + next_slots(spec, state, spec.MIN_ATTESTATION_INCLUSION_DELAY) + + yield from run_attestation_processing(spec, state, attestation, False) + + @with_all_phases @spec_state_test def test_before_inclusion_delay(spec, state): diff --git a/tests/core/pyspec/eth2spec/test/phase_0/epoch_processing/test_process_rewards_and_penalties.py b/tests/core/pyspec/eth2spec/test/phase_0/epoch_processing/test_process_rewards_and_penalties.py index af695fe69..b862b5c48 100644 --- a/tests/core/pyspec/eth2spec/test/phase_0/epoch_processing/test_process_rewards_and_penalties.py +++ b/tests/core/pyspec/eth2spec/test/phase_0/epoch_processing/test_process_rewards_and_penalties.py @@ -22,6 +22,9 @@ def run_process_rewards_and_penalties(spec, state): def prepare_state_with_full_attestations(spec, state, empty=False): + # If empty is true, attestations have 0 participants, and are not signed. + # Thus strictly speaking invalid when no participant is added later. + # Go to start of next epoch to ensure can have full participation next_epoch(spec, state) From 47ed5b6500e3028e745458295cae395b9323ef9e Mon Sep 17 00:00:00 2001 From: protolambda Date: Fri, 1 May 2020 16:10:28 +0200 Subject: [PATCH 2/3] Fix rewards testing for empty/weird participation cases, adding more as well --- .../eth2spec/test/helpers/attestations.py | 25 +++++--- .../test_process_attestation.py | 18 +----- .../test_process_rewards_and_penalties.py | 64 +++++++++++++++---- 3 files changed, 71 insertions(+), 36 deletions(-) diff --git a/tests/core/pyspec/eth2spec/test/helpers/attestations.py b/tests/core/pyspec/eth2spec/test/helpers/attestations.py index 377426006..85b543104 100644 --- a/tests/core/pyspec/eth2spec/test/helpers/attestations.py +++ b/tests/core/pyspec/eth2spec/test/helpers/attestations.py @@ -113,8 +113,8 @@ def get_valid_late_attestation(spec, state, slot=None, index=None, signed=False) return get_valid_attestation(spec, state, slot=slot, index=index, signed=signed, on_time=False) -def get_valid_attestation(spec, state, slot=None, index=None, empty=False, signed=False, on_time=True): - # If empty is true, the attestation has 0 participants, and will not be signed. +def get_valid_attestation(spec, state, slot=None, index=None, filter_participant_set=None, signed=False, on_time=True): + # If filter_participant_set is filters everything, the attestation has 0 participants, and cannot be signed. # Thus strictly speaking invalid when no participant is added later. if slot is None: @@ -136,10 +136,8 @@ def get_valid_attestation(spec, state, slot=None, index=None, empty=False, signe aggregation_bits=aggregation_bits, data=attestation_data, ) - if not empty: - fill_aggregate_attestation(spec, state, attestation) - if signed: - sign_attestation(spec, state, attestation) + # fill the attestation with (optionally filtered) participants, and optionally sign it + fill_aggregate_attestation(spec, state, attestation, signed=signed, filter_participant_set=filter_participant_set) if spec.fork == 'phase1' and on_time: attestation = convert_to_valid_on_time_attestation(spec, state, attestation, signed) @@ -232,16 +230,25 @@ def get_attestation_signature(spec, state, attestation_data, privkey): return bls.Sign(privkey, signing_root) -def fill_aggregate_attestation(spec, state, attestation, signed=False): +def fill_aggregate_attestation(spec, state, attestation, signed=False, filter_participant_set=None): + """ + `signed`: Signing is optional. + `filter_participant_set`: Optional, filters the full committee indices set (default) to a subset that participates + """ beacon_committee = spec.get_beacon_committee( state, attestation.data.slot, attestation.data.index, ) + # By default, have everyone participate + participants = set(beacon_committee) + # But optionally filter the participants to a smaller amount + if filter_participant_set is not None: + participants = filter_participant_set(participants) for i in range(len(beacon_committee)): - attestation.aggregation_bits[i] = True + attestation.aggregation_bits[i] = beacon_committee[i] in participants - if signed: + if signed and len(participants) > 0: sign_attestation(spec, state, attestation) diff --git a/tests/core/pyspec/eth2spec/test/phase_0/block_processing/test_process_attestation.py b/tests/core/pyspec/eth2spec/test/phase_0/block_processing/test_process_attestation.py index df3279faa..bb25a384e 100644 --- a/tests/core/pyspec/eth2spec/test/phase_0/block_processing/test_process_attestation.py +++ b/tests/core/pyspec/eth2spec/test/phase_0/block_processing/test_process_attestation.py @@ -13,7 +13,6 @@ from eth2spec.test.helpers.attestations import ( sign_attestation, ) from eth2spec.test.helpers.state import ( - next_slot, next_slots, next_epoch, transition_to, @@ -68,7 +67,7 @@ def test_invalid_attestation_signature(spec, state): @spec_state_test @always_bls def test_empty_participants_zeroes_sig(spec, state): - attestation = get_valid_attestation(spec, state, empty=True) + attestation = get_valid_attestation(spec, state, filter_participant_set=lambda comm: []) # 0 participants attestation.signature = spec.BLSSignature(b'\x00' * 96) next_slots(spec, state, spec.MIN_ATTESTATION_INCLUSION_DELAY) @@ -79,7 +78,7 @@ def test_empty_participants_zeroes_sig(spec, state): @spec_state_test @always_bls def test_empty_participants_seemingly_valid_sig(spec, state): - attestation = get_valid_attestation(spec, state, empty=True) + attestation = get_valid_attestation(spec, state, filter_participant_set=lambda comm: []) # 0 participants # Special BLS value, valid for zero pubkeys on some (but not all) BLS implementations. attestation.signature = spec.BLSSignature(b'\xc0' + b'\x00' * 95) next_slots(spec, state, spec.MIN_ATTESTATION_INCLUSION_DELAY) @@ -283,19 +282,6 @@ def test_bad_source_root(spec, state): yield from run_attestation_processing(spec, state, attestation, False) -@with_all_phases -@spec_state_test -def test_empty_aggregation_bits(spec, state): - next_slot(spec, state) - attestation = get_valid_attestation(spec, state, empty=True) - next_slots(spec, state, spec.MIN_ATTESTATION_INCLUSION_DELAY) - - assert attestation.aggregation_bits == Bitlist[spec.MAX_VALIDATORS_PER_COMMITTEE]( - *([0b0] * len(attestation.aggregation_bits))) - - yield from run_attestation_processing(spec, state, attestation) - - @with_all_phases @spec_state_test def test_too_many_aggregation_bits(spec, state): diff --git a/tests/core/pyspec/eth2spec/test/phase_0/epoch_processing/test_process_rewards_and_penalties.py b/tests/core/pyspec/eth2spec/test/phase_0/epoch_processing/test_process_rewards_and_penalties.py index b862b5c48..63aafe521 100644 --- a/tests/core/pyspec/eth2spec/test/phase_0/epoch_processing/test_process_rewards_and_penalties.py +++ b/tests/core/pyspec/eth2spec/test/phase_0/epoch_processing/test_process_rewards_and_penalties.py @@ -15,15 +15,15 @@ from eth2spec.test.helpers.attestations import ( ) from eth2spec.test.helpers.attester_slashings import get_indexed_attestation_participants from eth2spec.test.phase_0.epoch_processing.run_epoch_process_base import run_epoch_processing_with +from random import Random def run_process_rewards_and_penalties(spec, state): yield from run_epoch_processing_with(spec, state, 'process_rewards_and_penalties') -def prepare_state_with_full_attestations(spec, state, empty=False): - # If empty is true, attestations have 0 participants, and are not signed. - # Thus strictly speaking invalid when no participant is added later. +def prepare_state_with_full_attestations(spec, state, participation_fn=None): + # participation_fn: (slot, committee_index, committee_indices_set) -> participants_indices_set # Go to start of next epoch to ensure can have full participation next_epoch(spec, state) @@ -36,8 +36,15 @@ def prepare_state_with_full_attestations(spec, state, empty=False): # create an attestation for each index in each slot in epoch if state.slot < next_epoch_start_slot: for committee_index in range(spec.get_committee_count_at_slot(state, state.slot)): - attestation = get_valid_attestation(spec, state, index=committee_index, empty=empty, signed=True) - attestations.append(attestation) + def temp_participants_filter(comm): + if participation_fn is None: + return comm + else: + return participation_fn(state.slot, committee_index, comm) + attestation = get_valid_attestation(spec, state, index=committee_index, + filter_participant_set=temp_participants_filter, signed=True) + if any(attestation.aggregation_bits): # Only if there is at least 1 participant. + attestations.append(attestation) # fill each created slot in state after inclusion delay if state.slot >= start_slot + spec.MIN_ATTESTATION_INCLUSION_DELAY: inclusion_slot = state.slot - spec.MIN_ATTESTATION_INCLUSION_DELAY @@ -192,20 +199,55 @@ def test_no_attestations_all_penalties(spec, state): assert state.balances[index] < pre_state.balances[index] -@with_all_phases -@spec_state_test -def test_empty_attestations(spec, state): - attestations = prepare_state_with_full_attestations(spec, state, empty=True) +def run_with_participation(spec, state, participation_fn): + participated = set() + + def participation_tracker(slot, comm_index, comm): + att_participants = participation_fn(slot, comm_index, comm) + participated.update(att_participants) + return att_participants + + attestations = prepare_state_with_full_attestations(spec, state, participation_fn=participation_tracker) pre_state = state.copy() yield from run_process_rewards_and_penalties(spec, state) attesting_indices = spec.get_unslashed_attesting_indices(state, attestations) - assert len(attesting_indices) == 0 + assert len(attesting_indices) == len(participated) for index in range(len(pre_state.validators)): - assert state.balances[index] < pre_state.balances[index] + if index in participated: + assert state.balances[index] > pre_state.balances[index] + else: + assert state.balances[index] < pre_state.balances[index] + + +@with_all_phases +@spec_state_test +def test_almost_empty_attestations(spec, state): + rng = Random(1234) + yield from run_with_participation(spec, state, lambda slot, comm_index, comm: rng.sample(comm, 1)) + + +@with_all_phases +@spec_state_test +def test_random_fill_attestations(spec, state): + rng = Random(4567) + yield from run_with_participation(spec, state, lambda slot, comm_index, comm: rng.sample(comm, len(comm) // 3)) + + +@with_all_phases +@spec_state_test +def test_almost_full_attestations(spec, state): + rng = Random(8901) + yield from run_with_participation(spec, state, lambda slot, comm_index, comm: rng.sample(comm, len(comm) - 1)) + + +@with_all_phases +@spec_state_test +def test_full_attestation_participation(spec, state): + yield from run_with_participation(spec, state, lambda slot, comm_index, comm: comm) @with_all_phases From 12aa84fc8a7a0c93f5ad52fbed539e662a2e8e59 Mon Sep 17 00:00:00 2001 From: Danny Ryan Date: Thu, 7 May 2020 10:47:20 -0600 Subject: [PATCH 3/3] PR feedback --- .../eth2spec/test/helpers/attestations.py | 2 +- .../test_process_rewards_and_penalties.py | 20 +++++++++++-------- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/tests/core/pyspec/eth2spec/test/helpers/attestations.py b/tests/core/pyspec/eth2spec/test/helpers/attestations.py index 8c2e10edf..2c15a5136 100644 --- a/tests/core/pyspec/eth2spec/test/helpers/attestations.py +++ b/tests/core/pyspec/eth2spec/test/helpers/attestations.py @@ -152,7 +152,7 @@ def get_valid_attestation(spec, shard_transition=None, signed=False, on_time=True): - # If filter_participant_set is filters everything, the attestation has 0 participants, and cannot be signed. + # If filter_participant_set filters everything, the attestation has 0 participants, and cannot be signed. # Thus strictly speaking invalid when no participant is added later. if slot is None: slot = state.slot diff --git a/tests/core/pyspec/eth2spec/test/phase_0/epoch_processing/test_process_rewards_and_penalties.py b/tests/core/pyspec/eth2spec/test/phase_0/epoch_processing/test_process_rewards_and_penalties.py index 63aafe521..337f7f25c 100644 --- a/tests/core/pyspec/eth2spec/test/phase_0/epoch_processing/test_process_rewards_and_penalties.py +++ b/tests/core/pyspec/eth2spec/test/phase_0/epoch_processing/test_process_rewards_and_penalties.py @@ -22,9 +22,13 @@ def run_process_rewards_and_penalties(spec, state): yield from run_epoch_processing_with(spec, state, 'process_rewards_and_penalties') -def prepare_state_with_full_attestations(spec, state, participation_fn=None): - # participation_fn: (slot, committee_index, committee_indices_set) -> participants_indices_set +def prepare_state_with_attestations(spec, state, participation_fn=None): + """ + Prepare state with attestations according to the ``participation_fn``. + If no ``participation_fn``, default to "full" -- max committee participation at each slot. + participation_fn: (slot, committee_index, committee_indices_set) -> participants_indices_set + """ # Go to start of next epoch to ensure can have full participation next_epoch(spec, state) @@ -100,7 +104,7 @@ def test_genesis_epoch_full_attestations_no_rewards(spec, state): @with_all_phases @spec_state_test def test_full_attestations(spec, state): - attestations = prepare_state_with_full_attestations(spec, state) + attestations = prepare_state_with_attestations(spec, state) pre_state = state.copy() @@ -118,7 +122,7 @@ def test_full_attestations(spec, state): @with_all_phases @spec_state_test def test_full_attestations_random_incorrect_fields(spec, state): - attestations = prepare_state_with_full_attestations(spec, state) + attestations = prepare_state_with_attestations(spec, state) for i, attestation in enumerate(state.previous_epoch_attestations): if i % 3 == 0: # Mess up some head votes @@ -143,7 +147,7 @@ def test_full_attestations_random_incorrect_fields(spec, state): @with_custom_state(balances_fn=misc_balances, threshold_fn=lambda spec: spec.MAX_EFFECTIVE_BALANCE // 2) @single_phase def test_full_attestations_misc_balances(spec, state): - attestations = prepare_state_with_full_attestations(spec, state) + attestations = prepare_state_with_attestations(spec, state) pre_state = state.copy() @@ -175,7 +179,7 @@ def test_full_attestations_misc_balances(spec, state): @with_custom_state(balances_fn=low_single_balance, threshold_fn=zero_activation_threshold) @single_phase def test_full_attestations_one_validaor_one_gwei(spec, state): - attestations = prepare_state_with_full_attestations(spec, state) + attestations = prepare_state_with_attestations(spec, state) yield from run_process_rewards_and_penalties(spec, state) @@ -207,7 +211,7 @@ def run_with_participation(spec, state, participation_fn): participated.update(att_participants) return att_participants - attestations = prepare_state_with_full_attestations(spec, state, participation_fn=participation_tracker) + attestations = prepare_state_with_attestations(spec, state, participation_fn=participation_tracker) pre_state = state.copy() @@ -292,7 +296,7 @@ def test_duplicate_attestation(spec, state): @spec_state_test # Case when some eligible attestations are slashed. Modifies attesting_balance and consequently rewards/penalties. def test_attestations_some_slashed(spec, state): - attestations = prepare_state_with_full_attestations(spec, state) + attestations = prepare_state_with_attestations(spec, state) attesting_indices_before_slashings = list(spec.get_unslashed_attesting_indices(state, attestations)) # Slash maximum amount of validators allowed per epoch.