From 09cae4b3ccab47fcb7f9eba198af46d3bcdfa3d3 Mon Sep 17 00:00:00 2001 From: protolambda Date: Fri, 1 May 2020 15:17:41 +0200 Subject: [PATCH] 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)