Merge pull request #2518 from ethereum/fix-store-justified-checkpoint

Fix fork choice store checkpoints inconsistency issue

specs/phase0/fork-choice.md

@@ -327,9 +327,13 @@ def on_tick(store: Store, time: uint64) -> None:
     # Not a new epoch, return
     if not (current_slot > previous_slot and compute_slots_since_epoch_start(current_slot) == 0):
         return
-    # Update store.justified_checkpoint if a better checkpoint is known
+
+    # Update store.justified_checkpoint if a better checkpoint on the store.finalized_checkpoint chain
     if store.best_justified_checkpoint.epoch > store.justified_checkpoint.epoch:
-        store.justified_checkpoint = store.best_justified_checkpoint
+        finalized_slot = compute_start_slot_at_epoch(store.finalized_checkpoint.epoch)    
+        ancestor_at_finalized_slot = get_ancestor(store, store.best_justified_checkpoint.root, finalized_slot)
+        if ancestor_at_finalized_slot == store.finalized_checkpoint.root:
+            store.justified_checkpoint = store.best_justified_checkpoint
 ```
 
 #### `on_block`

tests/core/pyspec/eth2spec/test/phase0/unittests/fork_choice/test_on_tick.py

@@ -1,5 +1,13 @@
 from eth2spec.test.context import with_all_phases, spec_state_test
 from eth2spec.test.helpers.fork_choice import get_genesis_forkchoice_store
+from eth2spec.test.helpers.block import (
+    build_empty_block_for_next_slot,
+)
+from eth2spec.test.helpers.state import (
+    next_epoch,
+    state_transition_and_sign_block,
+    transition_to,
+)
 
 
 def run_on_tick(spec, store, time, new_justified_checkpoint=False):
@@ -26,18 +34,92 @@ def test_basic(spec, state):
 
 @with_all_phases
 @spec_state_test
-def test_update_justified_single(spec, state):
+def test_update_justified_single_on_store_finalized_chain(spec, state):
     store = get_genesis_forkchoice_store(spec, state)
-    next_epoch = spec.get_current_epoch(state) + 1
-    next_epoch_start_slot = spec.compute_start_slot_at_epoch(next_epoch)
-    seconds_until_next_epoch = next_epoch_start_slot * spec.config.SECONDS_PER_SLOT - store.time
 
-    store.best_justified_checkpoint = spec.Checkpoint(
-        epoch=store.justified_checkpoint.epoch + 1,
-        root=b'\x55' * 32,
+    # [Mock store.best_justified_checkpoint]
+    # Create a block at epoch 1
+    next_epoch(spec, state)
+    block = build_empty_block_for_next_slot(spec, state)
+    state_transition_and_sign_block(spec, state, block)
+    store.blocks[block.hash_tree_root()] = block.copy()
+    store.block_states[block.hash_tree_root()] = state.copy()
+    parent_block = block.copy()
+    # To make compute_slots_since_epoch_start(current_slot) == 0, transition to the end of the epoch
+    slot = state.slot + spec.SLOTS_PER_EPOCH - state.slot % spec.SLOTS_PER_EPOCH - 1
+    transition_to(spec, state, slot)
+    # Create a block at the start of epoch 2
+    block = build_empty_block_for_next_slot(spec, state)
+    # Mock state
+    state.current_justified_checkpoint = spec.Checkpoint(
+        epoch=spec.compute_epoch_at_slot(parent_block.slot),
+        root=parent_block.hash_tree_root(),
+    )
+    state_transition_and_sign_block(spec, state, block)
+    store.blocks[block.hash_tree_root()] = block
+    store.block_states[block.hash_tree_root()] = state
+    # Mock store.best_justified_checkpoint
+    store.best_justified_checkpoint = state.current_justified_checkpoint.copy()
+
+    run_on_tick(
+        spec,
+        store,
+        store.genesis_time + state.slot * spec.config.SECONDS_PER_SLOT,
+        new_justified_checkpoint=True
     )
 
-    run_on_tick(spec, store, store.time + seconds_until_next_epoch, True)
+
+@with_all_phases
+@spec_state_test
+def test_update_justified_single_not_on_store_finalized_chain(spec, state):
+    store = get_genesis_forkchoice_store(spec, state)
+    init_state = state.copy()
+
+    # Chain grows
+    # Create a block at epoch 1
+    next_epoch(spec, state)
+    block = build_empty_block_for_next_slot(spec, state)
+    block.body.graffiti = b'\x11' * 32
+    state_transition_and_sign_block(spec, state, block)
+    store.blocks[block.hash_tree_root()] = block.copy()
+    store.block_states[block.hash_tree_root()] = state.copy()
+    # Mock store.finalized_checkpoint
+    store.finalized_checkpoint = spec.Checkpoint(
+        epoch=spec.compute_epoch_at_slot(block.slot),
+        root=block.hash_tree_root(),
+    )
+
+    # [Mock store.best_justified_checkpoint]
+    # Create a block at epoch 1
+    state = init_state.copy()
+    next_epoch(spec, state)
+    block = build_empty_block_for_next_slot(spec, state)
+    block.body.graffiti = b'\x22' * 32
+    state_transition_and_sign_block(spec, state, block)
+    store.blocks[block.hash_tree_root()] = block.copy()
+    store.block_states[block.hash_tree_root()] = state.copy()
+    parent_block = block.copy()
+    # To make compute_slots_since_epoch_start(current_slot) == 0, transition to the end of the epoch
+    slot = state.slot + spec.SLOTS_PER_EPOCH - state.slot % spec.SLOTS_PER_EPOCH - 1
+    transition_to(spec, state, slot)
+    # Create a block at the start of epoch 2
+    block = build_empty_block_for_next_slot(spec, state)
+    # Mock state
+    state.current_justified_checkpoint = spec.Checkpoint(
+        epoch=spec.compute_epoch_at_slot(parent_block.slot),
+        root=parent_block.hash_tree_root(),
+    )
+    state_transition_and_sign_block(spec, state, block)
+    store.blocks[block.hash_tree_root()] = block.copy()
+    store.block_states[block.hash_tree_root()] = state.copy()
+    # Mock store.best_justified_checkpoint
+    store.best_justified_checkpoint = state.current_justified_checkpoint.copy()
+
+    run_on_tick(
+        spec,
+        store,
+        store.genesis_time + state.slot * spec.config.SECONDS_PER_SLOT,
+    )
 
 
 @with_all_phases