Merge pull request #1579 from ethereum/epochs-in-fork-choice

Properly handle skip slots in fork choice

specs/phase0/fork-choice.md

@@ -136,7 +136,8 @@ def get_ancestor(store: Store, root: Root, slot: Slot) -> Root:
     elif block.slot == slot:
         return root
     else:
-        return Bytes32()  # root is older than queried slot: no results. 
+        # root is older than queried slot, thus a skip slot. Return earliest root prior to slot
+        return root
 ```
 
 #### `get_latest_attesting_balance`
@@ -239,13 +240,8 @@ def should_update_justified_checkpoint(store: Store, new_justified_checkpoint: C
     if compute_slots_since_epoch_start(get_current_slot(store)) < SAFE_SLOTS_TO_UPDATE_JUSTIFIED:
         return True
 
-    new_justified_block = store.blocks[new_justified_checkpoint.root]
-    if new_justified_block.slot <= compute_start_slot_at_epoch(store.justified_checkpoint.epoch):
-        return False
-    if not (
-        get_ancestor(store, new_justified_checkpoint.root, store.blocks[store.justified_checkpoint.root].slot)
-        == store.justified_checkpoint.root
-    ):
+    justified_slot = compute_start_slot_at_epoch(store.justified_checkpoint.epoch)
+    if not get_ancestor(store, new_justified_checkpoint.root, justified_slot) == store.justified_checkpoint.root:
         return False
 
     return True
@@ -283,13 +279,13 @@ def on_block(store: Store, signed_block: SignedBeaconBlock) -> None:
     assert get_current_slot(store) >= block.slot
     # Add new block to the store
     store.blocks[hash_tree_root(block)] = block
-    # Check block is a descendant of the finalized block
-    assert (
-        get_ancestor(store, hash_tree_root(block), store.blocks[store.finalized_checkpoint.root].slot) ==
-        store.finalized_checkpoint.root
-    )
-    # Check that block is later than the finalized epoch slot
-    assert block.slot > compute_start_slot_at_epoch(store.finalized_checkpoint.epoch)
+
+    # Check that block is later than the finalized epoch slot (optimization to reduce calls to get_ancestor)
+    finalized_slot = compute_start_slot_at_epoch(store.finalized_checkpoint.epoch)
+    assert block.slot > finalized_slot
+    # Check block is a descendant of the finalized block at the checkpoint finalized slot
+    assert get_ancestor(store, hash_tree_root(block), finalized_slot) == store.finalized_checkpoint.root
+
     # Check the block is valid and compute the post-state
     state = state_transition(pre_state, signed_block, True)
     # Add new state for this block to the store

tests/core/pyspec/eth2spec/test/fork_choice/test_on_block.py

@@ -135,6 +135,48 @@ def test_on_block_before_finalized(spec, state):
     run_on_block(spec, store, signed_block, False)
 
 
+@with_all_phases
+@spec_state_test
+def test_on_block_finalized_skip_slots(spec, state):
+    # Initialization
+    store = spec.get_genesis_store(state)
+    time = 100
+    spec.on_tick(store, time)
+
+    store.finalized_checkpoint = spec.Checkpoint(
+        epoch=store.finalized_checkpoint.epoch + 2,
+        root=store.finalized_checkpoint.root
+    )
+
+    # Build block that includes the skipped slots up to finality in chain
+    block = build_empty_block(spec, state, spec.compute_start_slot_at_epoch(store.finalized_checkpoint.epoch) + 2)
+    signed_block = state_transition_and_sign_block(spec, state, block)
+    spec.on_tick(store, store.time + state.slot * spec.SECONDS_PER_SLOT)
+    run_on_block(spec, store, signed_block)
+
+
+@with_all_phases
+@spec_state_test
+def test_on_block_finalized_skip_slots_not_in_skip_chain(spec, state):
+    # Initialization
+    store = spec.get_genesis_store(state)
+
+    store.finalized_checkpoint = spec.Checkpoint(
+        epoch=store.finalized_checkpoint.epoch + 2,
+        root=store.finalized_checkpoint.root
+    )
+
+    # First transition through the epoch to ensure no skipped slots
+    state, store, last_signed_block = apply_next_epoch_with_attestations(spec, state, store)
+
+    # Now build a block at later slot than finalized epoch
+    # Includes finalized block in chain, but not at appropriate skip slot
+    block = build_empty_block(spec, state, spec.compute_start_slot_at_epoch(store.finalized_checkpoint.epoch) + 2)
+    signed_block = state_transition_and_sign_block(spec, state, block)
+    spec.on_tick(store, store.time + state.slot * spec.SECONDS_PER_SLOT)
+    run_on_block(spec, store, signed_block, False)
+
+
 @with_all_phases
 @spec_state_test
 def test_on_block_update_justified_checkpoint_within_safe_slots(spec, state):