warn about broken attestation validation, fix most attester slashings tests

specs/core/1_beacon-chain.md

@@ -142,7 +142,7 @@ class AttestationData(Container):
 class Attestation(Container):
     aggregation_bits: Bitlist[MAX_VALIDATORS_PER_COMMITTEE]
     data: AttestationData
-    custody_bits: List[Bitlist[MAX_VALIDATORS_PER_COMMITTEE], MAX_SHARD_BLOCKS_PER_ATTESTATION]
+    custody_bits_blocks: List[Bitlist[MAX_VALIDATORS_PER_COMMITTEE], MAX_SHARD_BLOCKS_PER_ATTESTATION]
     signature: BLSSignature
 ```
 
@@ -536,7 +536,7 @@ def is_valid_indexed_attestation(state: BeaconState, indexed_attestation: Indexe
     domain=get_domain(state, DOMAIN_BEACON_ATTESTER, attestation.data.target.epoch)
     aggregation_bits = attestation.aggregation_bits
     assert len(aggregation_bits) == len(indexed_attestation.committee)
-    for i, custody_bits in enumerate(attestation.custody_bits):
+    for i, custody_bits in enumerate(attestation.custody_bits_blocks):
         assert len(custody_bits) == len(indexed_attestation.committee)
         for participant, abit, cbit in zip(indexed_attestation.committee, aggregation_bits, custody_bits):
             if abit:
@@ -546,7 +546,10 @@ def is_valid_indexed_attestation(state: BeaconState, indexed_attestation: Indexe
                     AttestationCustodyBitWrapper(hash_tree_root(attestation.data), i, cbit), domain))
             else:
                 assert not cbit
-
+    # WARNING: this is BROKEN. If no custody_bits_blocks,
+    #  a valid empty signature can pass validation, even though aggregate bits are set.
+    # Decide between: force at least 1 shard block (even if empty data),
+    #  or fast-aggregate-verify with attestation data with empty shard data as message (alike to phase0)
     return bls.AggregateVerify(zip(all_pubkeys, all_signing_roots), signature=attestation.signature)
 ```
 
@@ -616,11 +619,11 @@ def validate_attestation(state: BeaconState, attestation: Attestation) -> None:
     # Signature check
     assert is_valid_indexed_attestation(state, get_indexed_attestation(state, attestation))
     # Type 1: on-time attestations
-    if attestation.custody_bits != []:
+    if attestation.custody_bits_blocks != []:
         # Correct slot
         assert data.slot + MIN_ATTESTATION_INCLUSION_DELAY == state.slot
         # Correct data root count
-        assert len(attestation.custody_bits) == len(get_offset_slots(state, shard_start_slot))
+        assert len(attestation.custody_bits_blocks) == len(get_offset_slots(state, shard_start_slot))
         # Correct parent block root
         assert data.beacon_block_root == get_block_root_at_slot(state, get_previous_slot(state.slot))
     # Type 2: delayed attestations

specs/core/1_custody-game.md

@@ -91,7 +91,7 @@ The following types are defined, mapping into `DomainType` (little endian):
 
 ```python
 class CustodySlashing(Container):
-    # Attestation.custody_bits[data_index][committee.index(malefactor_index)] is the target custody bit to check.
+    # Attestation.custody_bits_blocks[data_index][committee.index(malefactor_index)] is the target custody bit to check.
     # (Attestation.data.shard_transition_root as ShardTransition).shard_data_roots[data_index] is the root of the data.
     data_index: uint64
     malefactor_index: ValidatorIndex
@@ -378,7 +378,7 @@ def process_custody_slashing(state: BeaconState, signed_custody_slashing: Signed
     assert bls.Verify(malefactor.pubkey, signing_root, custody_slashing.malefactor_secret)
 
     # Get the custody bit
-    custody_bits = attestation.custody_bits[custody_slashing.data_index]
+    custody_bits = attestation.custody_bits_blocks[custody_slashing.data_index]
     committee = get_beacon_committee(state, attestation.data.slot, attestation.data.index)
     claimed_custody_bit = custody_bits[committee.index(custody_slashing.malefactor_index)]
     

test_libs/pyspec/eth2spec/test/helpers/attester_slashings.py

@@ -16,3 +16,40 @@ def get_valid_attester_slashing(spec, state, signed_1=False, signed_2=False):
         attestation_1=spec.get_indexed_attestation(state, attestation_1),
         attestation_2=spec.get_indexed_attestation(state, attestation_2),
     )
+
+
+def get_indexed_attestation_participants(spec, indexed_att):
+    """
+    Wrapper around index-attestation to return the list of participant indices, regardless of spec phase.
+    """
+    if spec.version == "phase1":
+        return list(spec.get_indices_from_committee(
+            indexed_att.committee,
+            indexed_att.attestation.aggregation_bits,
+        ))
+    else:
+        return list(indexed_att.attesting_indices)
+
+
+def set_indexed_attestation_participants(spec, indexed_att, participants):
+    """
+    Wrapper around index-attestation to return the list of participant indices, regardless of spec phase.
+    """
+    if spec.version == "phase1":
+        indexed_att.attestation.aggregation_bits = [bool(i in participants) for i in indexed_att.committee]
+    else:
+        indexed_att.attesting_indices = participants
+
+
+def get_attestation_1_data(spec, att_slashing):
+    if spec.version == "phase1":
+        return att_slashing.attestation_1.attestation.data
+    else:
+        return att_slashing.attestation_1.data
+
+
+def get_attestation_2_data(spec, att_slashing):
+    if spec.version == "phase1":
+        return att_slashing.attestation_2.attestation.data
+    else:
+        return att_slashing.attestation_2.data

test_libs/pyspec/eth2spec/test/phase_0/block_processing/test_process_attester_slashing.py

@@ -1,6 +1,7 @@
-from eth2spec.test.context import spec_state_test, expect_assertion_error, always_bls, with_all_phases
+from eth2spec.test.context import spec_state_test, expect_assertion_error, always_bls, with_all_phases, with_phases
 from eth2spec.test.helpers.attestations import sign_indexed_attestation
-from eth2spec.test.helpers.attester_slashings import get_valid_attester_slashing
+from eth2spec.test.helpers.attester_slashings import get_valid_attester_slashing, \
+    get_indexed_attestation_participants, get_attestation_2_data, get_attestation_1_data
 from eth2spec.test.helpers.block import apply_empty_block
 from eth2spec.test.helpers.state import (
     get_balance,
@@ -25,7 +26,7 @@ def run_attester_slashing_processing(spec, state, attester_slashing, valid=True)
         yield 'post', None
         return
 
-    slashed_indices = attester_slashing.attestation_1.attesting_indices
+    slashed_indices = get_indexed_attestation_participants(spec, attester_slashing.attestation_1)
 
     proposer_index = spec.get_beacon_proposer_index(state)
     pre_proposer_balance = get_balance(state, proposer_index)
@@ -92,12 +93,12 @@ def test_success_surround(spec, state):
 
     state.current_justified_checkpoint.epoch += 1
     attester_slashing = get_valid_attester_slashing(spec, state, signed_1=False, signed_2=True)
-    attestation_1 = attester_slashing.attestation_1
+    att_1_data = get_attestation_1_data(spec, attester_slashing)
-    attestation_2 = attester_slashing.attestation_2
+    att_2_data = get_attestation_2_data(spec, attester_slashing)
 
     # set attestion1 to surround attestation 2
-    attestation_1.data.source.epoch = attestation_2.data.source.epoch - 1
+    att_1_data.source.epoch = att_2_data.source.epoch - 1
-    attestation_1.data.target.epoch = attestation_2.data.target.epoch + 1
+    att_1_data.target.epoch = att_2_data.target.epoch + 1
 
     sign_indexed_attestation(spec, state, attester_slashing.attestation_1)
 
@@ -109,7 +110,7 @@ def test_success_surround(spec, state):
 @always_bls
 def test_success_already_exited_recent(spec, state):
     attester_slashing = get_valid_attester_slashing(spec, state, signed_1=True, signed_2=True)
-    slashed_indices = attester_slashing.attestation_1.attesting_indices
+    slashed_indices = get_indexed_attestation_participants(spec, attester_slashing.attestation_1)
     for index in slashed_indices:
         spec.initiate_validator_exit(state, index)
 
@@ -121,7 +122,7 @@ def test_success_already_exited_recent(spec, state):
 @always_bls
 def test_success_already_exited_long_ago(spec, state):
     attester_slashing = get_valid_attester_slashing(spec, state, signed_1=True, signed_2=True)
-    slashed_indices = attester_slashing.attestation_1.attesting_indices
+    slashed_indices = get_indexed_attestation_participants(spec, attester_slashing.attestation_1)
     for index in slashed_indices:
         spec.initiate_validator_exit(state, index)
         state.validators[index].withdrawable_epoch = spec.get_current_epoch(state) + 2
@@ -158,7 +159,12 @@ def test_invalid_sig_1_and_2(spec, state):
 def test_same_data(spec, state):
     attester_slashing = get_valid_attester_slashing(spec, state, signed_1=False, signed_2=True)
 
-    attester_slashing.attestation_1.data = attester_slashing.attestation_2.data
+    indexed_att_1 = attester_slashing.attestation_1
+    att_2_data = get_attestation_2_data(spec, attester_slashing)
+    if spec.version == 'phase1':
+        indexed_att_1.attestation.data = att_2_data
+    else:
+        indexed_att_1.data = att_2_data
     sign_indexed_attestation(spec, state, attester_slashing.attestation_1)
 
     yield from run_attester_slashing_processing(spec, state, attester_slashing, False)
@@ -169,10 +175,8 @@ def test_same_data(spec, state):
 def test_no_double_or_surround(spec, state):
     attester_slashing = get_valid_attester_slashing(spec, state, signed_1=False, signed_2=True)
 
-    if spec.version == 'phase0':
+    att_1_data = get_attestation_1_data(spec, attester_slashing)
-        attester_slashing.attestation_1.data.target.epoch += 1
+    att_1_data.target.epoch += 1
-    else:
-        attester_slashing.attestation_1.attestation.data.target.epoch += 1
 
     sign_indexed_attestation(spec, state, attester_slashing.attestation_1)
 
@@ -185,20 +189,23 @@ def test_participants_already_slashed(spec, state):
     attester_slashing = get_valid_attester_slashing(spec, state, signed_1=True, signed_2=True)
 
     # set all indices to slashed
-    validator_indices = attester_slashing.attestation_1.attesting_indices
+    validator_indices = get_indexed_attestation_participants(spec, attester_slashing.attestation_1)
     for index in validator_indices:
         state.validators[index].slashed = True
 
     yield from run_attester_slashing_processing(spec, state, attester_slashing, False)
 
 
-@with_all_phases
+# Some of the following tests are phase0 only: phase 1 lists participants with bitfields instead of index list.
+
+
+@with_phases(['phase0'])
 @spec_state_test
 @always_bls
 def test_att1_bad_extra_index(spec, state):
     attester_slashing = get_valid_attester_slashing(spec, state, signed_1=True, signed_2=True)
 
-    indices = attester_slashing.attestation_1.attesting_indices
+    indices = get_indexed_attestation_participants(spec, attester_slashing.attestation_1)
     options = list(set(range(len(state.validators))) - set(indices))
     indices.append(options[len(options) // 2])  # add random index, not previously in attestation.
     attester_slashing.attestation_1.attesting_indices = sorted(indices)
@@ -208,7 +215,7 @@ def test_att1_bad_extra_index(spec, state):
     yield from run_attester_slashing_processing(spec, state, attester_slashing, False)
 
 
-@with_all_phases
+@with_phases(['phase0'])
 @spec_state_test
 @always_bls
 def test_att1_bad_replaced_index(spec, state):
@@ -224,7 +231,7 @@ def test_att1_bad_replaced_index(spec, state):
     yield from run_attester_slashing_processing(spec, state, attester_slashing, False)
 
 
-@with_all_phases
+@with_phases(['phase0'])
 @spec_state_test
 @always_bls
 def test_att2_bad_extra_index(spec, state):
@@ -240,7 +247,7 @@ def test_att2_bad_extra_index(spec, state):
     yield from run_attester_slashing_processing(spec, state, attester_slashing, False)
 
 
-@with_all_phases
+@with_phases(['phase0'])
 @spec_state_test
 @always_bls
 def test_att2_bad_replaced_index(spec, state):
@@ -256,7 +263,7 @@ def test_att2_bad_replaced_index(spec, state):
     yield from run_attester_slashing_processing(spec, state, attester_slashing, False)
 
 
-@with_all_phases
+@with_phases(['phase0'])
 @spec_state_test
 @always_bls
 def test_att1_duplicate_index_normal_signed(spec, state):
@@ -276,7 +283,7 @@ def test_att1_duplicate_index_normal_signed(spec, state):
     yield from run_attester_slashing_processing(spec, state, attester_slashing, False)
 
 
-@with_all_phases
+@with_phases(['phase0'])
 @spec_state_test
 @always_bls
 def test_att2_duplicate_index_normal_signed(spec, state):
@@ -296,7 +303,7 @@ def test_att2_duplicate_index_normal_signed(spec, state):
     yield from run_attester_slashing_processing(spec, state, attester_slashing, False)
 
 
-@with_all_phases
+@with_phases(['phase0'])
 @spec_state_test
 @always_bls
 def test_att1_duplicate_index_double_signed(spec, state):
@@ -311,7 +318,7 @@ def test_att1_duplicate_index_double_signed(spec, state):
     yield from run_attester_slashing_processing(spec, state, attester_slashing, False)
 
 
-@with_all_phases
+@with_phases(['phase0'])
 @spec_state_test
 @always_bls
 def test_att2_duplicate_index_double_signed(spec, state):
@@ -326,7 +333,7 @@ def test_att2_duplicate_index_double_signed(spec, state):
     yield from run_attester_slashing_processing(spec, state, attester_slashing, False)
 
 
-@with_all_phases
+@with_phases(['phase0'])
 @spec_state_test
 def test_unsorted_att_1(spec, state):
     attester_slashing = get_valid_attester_slashing(spec, state, signed_1=False, signed_2=True)
@@ -339,7 +346,7 @@ def test_unsorted_att_1(spec, state):
     yield from run_attester_slashing_processing(spec, state, attester_slashing, False)
 
 
-@with_all_phases
+@with_phases(['phase0'])
 @spec_state_test
 def test_unsorted_att_2(spec, state):
     attester_slashing = get_valid_attester_slashing(spec, state, signed_1=True, signed_2=False)

test_libs/pyspec/eth2spec/test/sanity/test_blocks.py

@@ -6,7 +6,7 @@ from eth2spec.test.helpers.state import get_balance, state_transition_and_sign_b
 from eth2spec.test.helpers.block import build_empty_block_for_next_slot, build_empty_block, sign_block, \
     transition_unsigned_block
 from eth2spec.test.helpers.keys import privkeys, pubkeys
-from eth2spec.test.helpers.attester_slashings import get_valid_attester_slashing
+from eth2spec.test.helpers.attester_slashings import get_valid_attester_slashing, get_indexed_attestation_participants
 from eth2spec.test.helpers.proposer_slashings import get_valid_proposer_slashing
 from eth2spec.test.helpers.attestations import get_valid_attestation
 from eth2spec.test.helpers.deposits import prepare_state_and_deposit
@@ -220,7 +220,7 @@ def test_attester_slashing(spec, state):
     pre_state = deepcopy(state)
 
     attester_slashing = get_valid_attester_slashing(spec, state, signed_1=True, signed_2=True)
-    validator_index = attester_slashing.attestation_1.attesting_indices[0]
+    validator_index = get_indexed_attestation_participants(spec, attester_slashing.attestation_1)[0]
 
     assert not state.validators[validator_index].slashed