Merge pull request #1780 from ethereum/empty-bits-case

Handle empty aggregation bits as discussed in #1713

specs/phase0/beacon-chain.md

@@ -683,11 +683,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]

specs/phase0/p2p-interface.md

@@ -247,7 +247,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.

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.

tests/core/pyspec/eth2spec/test/helpers/attestations.py

@@ -148,10 +148,12 @@ def get_valid_attestation(spec,
                           state,
                           slot=None,
                           index=None,
+                          filter_participant_set=None,
                           shard_transition=None,
-                          empty=False,
                           signed=False,
                           on_time=True):
+    # 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
     if index is None:
@@ -173,10 +175,8 @@ def get_valid_attestation(spec,
         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)
@@ -269,16 +269,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)
 
 

tests/core/pyspec/eth2spec/test/phase_0/block_processing/test_process_attestation.py

@@ -13,7 +13,6 @@
     sign_attestation,
 )
 from eth2spec.test.helpers.state import (
-    next_slot,
     next_slots,
     next_epoch,
     transition_to,
@@ -64,6 +63,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, filter_participant_set=lambda comm: [])  # 0 participants
+    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, 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)
+
+    yield from run_attestation_processing(spec, state, attestation, False)
+
+
 @with_all_phases
 @spec_state_test
 def test_before_inclusion_delay(spec, state):
@@ -260,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):

tests/core/pyspec/eth2spec/test/phase_0/epoch_processing/test_process_rewards_and_penalties.py

@@ -15,13 +15,20 @@
 )
 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):
+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)
 
@@ -33,8 +40,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
@@ -90,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()
 
@@ -108,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
@@ -133,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()
 
@@ -165,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)
 
@@ -189,20 +203,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_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
@@ -247,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.