address @hwwhww feedback

specs/phase1/beacon-chain.md

@@ -48,8 +48,10 @@
     - [`get_start_shard`](#get_start_shard)
     - [`get_shard`](#get_shard)
     - [`get_latest_slot_for_shard`](#get_latest_slot_for_shard)
+    - [`compute_offset_slots`](#compute_offset_slots)
     - [`get_offset_slots`](#get_offset_slots)
   - [Predicates](#predicates)
+    - [`is_winning_attestation`](#is_winning_attestation)
     - [Updated `is_valid_indexed_attestation`](#updated-is_valid_indexed_attestation)
   - [Block processing](#block-processing)
     - [Operations](#operations)
@@ -533,6 +535,24 @@ def get_offset_slots(state: BeaconState, shard: Shard) -> Sequence[Slot]:
 
 ### Predicates
 
+#### `is_winning_attestation`
+
+```python
+def is_winning_attestation(state: BeaconState,
+                           attestation: PendingAttestation,
+                           committee_index: CommitteeIndex,
+                           winning_root: Root) -> bool:
+    """
+    Check if ``attestation`` helped contribute to the successful crosslink of
+    ``winning_root`` formed by ``committee_index`` committee at the current slot.
+    """
+    return (
+        attestation.slot == state.slot
+        and attestation.data.index == committee_index
+        and attestation.data.shard_transition_root == winning_root
+    )
+```
+
 #### Updated `is_valid_indexed_attestation`
 
 Note that this replaces the Phase 0 `is_valid_indexed_attestation`.
@@ -638,7 +658,7 @@ def validate_attestation(state: BeaconState, attestation: Attestation) -> None:
 
     shard = get_shard(state, attestation)
 
-    # Type 1: on-time attestations
+    # Type 1: on-time attestations, the custody bits should be non-empty.
     if attestation.custody_bits_blocks != []:
         # Ensure on-time attestation
         assert data.slot + MIN_ATTESTATION_INCLUSION_DELAY == state.slot
@@ -646,7 +666,7 @@ def validate_attestation(state: BeaconState, attestation: Attestation) -> None:
         assert len(attestation.custody_bits_blocks) == len(get_offset_slots(state, shard))
         # Correct parent block root
         assert data.beacon_block_root == get_block_root_at_slot(state, get_previous_slot(state.slot))
-    # Type 2: no shard transition, no custody bits  # TODO: could only allow for older attestations.
+    # Type 2: no shard transition, no custody bits
     else:
         # Ensure delayed attestation
         assert data.slot + MIN_ATTESTATION_INCLUSION_DELAY < state.slot  
@@ -783,11 +803,7 @@ def process_crosslinks(state: BeaconState,
         if winning_root != Root():
             # Mark relevant pending attestations as creating a successful crosslink
             for pending_attestation in state.current_epoch_attestations:
-                if (
+                if is_winning_attestation(state, pending_attestation, committee_index, winning_root):
-                    pending_attestation.slot == state.slot and pending_attestation
-                    and pending_attestation.data.index == committee_index
-                    and pending_attestation.data.shard_transition_root == winning_root
-                ):
                     pending_attestation.crosslink_success = True
 ```
 
@@ -801,8 +817,8 @@ def process_attestation(state: BeaconState, attestation: Attestation) -> None:
         aggregation_bits=attestation.aggregation_bits,
         data=attestation.data,
         inclusion_delay=state.slot - attestation.data.slot,
-        crosslink_success=False,  # To be filled in during process_crosslinks
         proposer_index=get_beacon_proposer_index(state),
+        crosslink_success=False,  # To be filled in during process_crosslinks
     )
     if attestation.data.target.epoch == get_current_epoch(state):
         state.current_epoch_attestations.append(pending_attestation)

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

@@ -16,7 +16,7 @@ def get_valid_on_time_attestation(spec, state, index=None, signed=False):
     shard = spec.get_shard(state, attestation)
     offset_slots = spec.compute_offset_slots(spec.get_latest_slot_for_shard(state, shard), state.slot + 1)
 
-    for offset_slot in offset_slots:
+    for _ in offset_slots:
         attestation.custody_bits_blocks.append(
             Bitlist[spec.MAX_VALIDATORS_PER_COMMITTEE]([0 for _ in attestation.aggregation_bits])
         )

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

@@ -22,13 +22,16 @@ def run_process_rewards_and_penalties(spec, state):
 
 
 def prepare_state_with_full_attestations(spec, state, empty=False):
+    # Go to start of next epoch to ensure can have full participation
+    next_epoch(spec, state)
+
     start_slot = state.slot
     start_epoch = spec.get_current_epoch(state)
-    next_start_epoch = spec.compute_start_slot_at_epoch(start_epoch + 1)
+    next_epoch_start_slot = spec.compute_start_slot_at_epoch(start_epoch + 1)
     attestations = []
-    for slot in range(spec.SLOTS_PER_EPOCH + spec.MIN_ATTESTATION_INCLUSION_DELAY):
+    for _ in range(spec.SLOTS_PER_EPOCH + spec.MIN_ATTESTATION_INCLUSION_DELAY):
         # create an attestation for each index in each slot in epoch
-        if state.slot < next_start_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)
@@ -39,7 +42,7 @@ def prepare_state_with_full_attestations(spec, state, empty=False):
             add_attestations_to_state(spec, state, include_attestations, state.slot)
         next_slot(spec, state)
 
-    assert spec.compute_epoch_at_slot(state.slot) == start_epoch + 1
+    assert state.slot == next_epoch_start_slot + spec.MIN_ATTESTATION_INCLUSION_DELAY
     assert len(state.previous_epoch_attestations) == len(attestations)
 
     return attestations
@@ -87,8 +90,6 @@ def test_genesis_epoch_full_attestations_no_rewards(spec, state):
 @with_all_phases
 @spec_state_test
 def test_full_attestations(spec, state):
-    # Go to start of next epoch to ensure can have full participation
-    next_epoch(spec, state)
     attestations = prepare_state_with_full_attestations(spec, state)
 
     pre_state = state.copy()
@@ -132,8 +133,6 @@ 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):
-    # Go to start of next epoch to ensure can have full participation
-    next_epoch(spec, state)
     attestations = prepare_state_with_full_attestations(spec, state)
 
     pre_state = state.copy()
@@ -178,6 +177,7 @@ def test_full_attestations_one_validaor_one_gwei(spec, state):
 @with_all_phases
 @spec_state_test
 def test_no_attestations_all_penalties(spec, state):
+    # Move to next epoch to ensure rewards/penalties are processed
     next_epoch(spec, state)
     pre_state = state.copy()
 
@@ -254,7 +254,6 @@ def test_attestations_some_slashed(spec, state):
     for i in range(spec.MIN_PER_EPOCH_CHURN_LIMIT):
         spec.slash_validator(state, attesting_indices_before_slashings[i])
 
-    assert spec.compute_epoch_at_slot(state.slot) == spec.GENESIS_EPOCH + 1
     assert len(state.previous_epoch_attestations) == len(attestations)
 
     pre_state = state.copy()