Merge pull request #2727 from ethereum/fc-dev-validate_target_epoch_s…

…cope-patch

always atomically update justified and finalized

specs/merge/fork-choice.md

@@ -185,17 +185,5 @@ def on_block(store: Store, signed_block: SignedBeaconBlock) -> None:
     # Update finalized checkpoint
     if state.finalized_checkpoint.epoch > store.finalized_checkpoint.epoch:
         store.finalized_checkpoint = state.finalized_checkpoint
-
-        # Potentially update justified if different from store
-        if store.justified_checkpoint != state.current_justified_checkpoint:
-            # Update justified if new justified is later than store justified
-            if state.current_justified_checkpoint.epoch > store.justified_checkpoint.epoch:
-                store.justified_checkpoint = state.current_justified_checkpoint
-                return
-
-            # Update justified if store justified is not in chain with finalized checkpoint
-            finalized_slot = compute_start_slot_at_epoch(store.finalized_checkpoint.epoch)
-            ancestor_at_finalized_slot = get_ancestor(store, store.justified_checkpoint.root, finalized_slot)
-            if ancestor_at_finalized_slot != store.finalized_checkpoint.root:
-                store.justified_checkpoint = state.current_justified_checkpoint
+        store.justified_checkpoint = state.current_justified_checkpoint
 ```

specs/phase0/fork-choice.md

@@ -22,6 +22,7 @@
     - [`get_head`](#get_head)
     - [`should_update_justified_checkpoint`](#should_update_justified_checkpoint)
     - [`on_attestation` helpers](#on_attestation-helpers)
+      - [`validate_target_epoch_against_current_time`](#validate_target_epoch_against_current_time)
       - [`validate_on_attestation`](#validate_on_attestation)
       - [`store_target_checkpoint_state`](#store_target_checkpoint_state)
       - [`update_latest_messages`](#update_latest_messages)
@@ -257,10 +258,11 @@ def should_update_justified_checkpoint(store: Store, new_justified_checkpoint: C
 
 #### `on_attestation` helpers
 
-##### `validate_on_attestation`
+
+##### `validate_target_epoch_against_current_time`
 
 ```python
-def validate_on_attestation(store: Store, attestation: Attestation) -> None:
+def validate_target_epoch_against_current_time(store: Store, attestation: Attestation) -> None:
     target = attestation.data.target
 
     # Attestations must be from the current or previous epoch
@@ -269,6 +271,19 @@ def validate_on_attestation(store: Store, attestation: Attestation) -> None:
     previous_epoch = current_epoch - 1 if current_epoch > GENESIS_EPOCH else GENESIS_EPOCH
     # If attestation target is from a future epoch, delay consideration until the epoch arrives
     assert target.epoch in [current_epoch, previous_epoch]
+```
+
+##### `validate_on_attestation`
+
+```python
+def validate_on_attestation(store: Store, attestation: Attestation, is_from_block: bool) -> None:
+    target = attestation.data.target
+
+    # If the given attestation is not from a beacon block message, we have to check the target epoch scope.
+    if not is_from_block:
+        validate_target_epoch_against_current_time(store, attestation)
+
+    # Check that the epoch number and slot number are matching
     assert target.epoch == compute_epoch_at_slot(attestation.data.slot)
 
     # Attestations target be for a known block. If target block is unknown, delay consideration until the block is found
@@ -372,32 +387,21 @@ def on_block(store: Store, signed_block: SignedBeaconBlock) -> None:
     # Update finalized checkpoint
     if state.finalized_checkpoint.epoch > store.finalized_checkpoint.epoch:
         store.finalized_checkpoint = state.finalized_checkpoint
-
-        # Potentially update justified if different from store
-        if store.justified_checkpoint != state.current_justified_checkpoint:
-            # Update justified if new justified is later than store justified
-            if state.current_justified_checkpoint.epoch > store.justified_checkpoint.epoch:
-                store.justified_checkpoint = state.current_justified_checkpoint
-                return
-
-            # Update justified if store justified is not in chain with finalized checkpoint
-            finalized_slot = compute_start_slot_at_epoch(store.finalized_checkpoint.epoch)
-            ancestor_at_finalized_slot = get_ancestor(store, store.justified_checkpoint.root, finalized_slot)
-            if ancestor_at_finalized_slot != store.finalized_checkpoint.root:
-                store.justified_checkpoint = state.current_justified_checkpoint
+        store.justified_checkpoint = state.current_justified_checkpoint
 ```
 
 #### `on_attestation`
 
 ```python
-def on_attestation(store: Store, attestation: Attestation) -> None:
+def on_attestation(store: Store, attestation: Attestation, is_from_block: bool=False) -> None:
     """
     Run ``on_attestation`` upon receiving a new ``attestation`` from either within a block or directly on the wire.
 
     An ``attestation`` that is asserted as invalid may be valid at a later time,
     consider scheduling it for later processing in such case.
     """
-    validate_on_attestation(store, attestation)
+    validate_on_attestation(store, attestation, is_from_block)
+
     store_target_checkpoint_state(store, attestation.data.target)
 
     # Get state at the `target` to fully validate attestation

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

@@ -23,7 +23,7 @@ def add_block_to_store(spec, store, signed_block):
     spec.on_block(store, signed_block)
 
 
-def tick_and_add_block(spec, store, signed_block, test_steps, valid=True, allow_invalid_attestations=False,
+def tick_and_add_block(spec, store, signed_block, test_steps, valid=True,
                        merge_block=False, block_not_found=False):
     pre_state = store.block_states[signed_block.message.parent_root]
     block_time = pre_state.genesis_time + signed_block.message.slot * spec.config.SECONDS_PER_SLOT
@@ -36,14 +36,13 @@ def tick_and_add_block(spec, store, signed_block, test_steps, valid=True, allow_
     post_state = yield from add_block(
         spec, store, signed_block, test_steps,
         valid=valid,
-        allow_invalid_attestations=allow_invalid_attestations,
         block_not_found=block_not_found,
     )
 
     return post_state
 
 
-def tick_and_run_on_attestation(spec, store, attestation, test_steps):
+def tick_and_run_on_attestation(spec, store, attestation, test_steps, is_from_block=False):
     parent_block = store.blocks[attestation.data.beacon_block_root]
     pre_state = store.block_states[spec.hash_tree_root(parent_block)]
     block_time = pre_state.genesis_time + parent_block.slot * spec.config.SECONDS_PER_SLOT
@@ -53,40 +52,21 @@ def tick_and_run_on_attestation(spec, store, attestation, test_steps):
         spec.on_tick(store, next_epoch_time)
         test_steps.append({'tick': int(next_epoch_time)})
 
-    spec.on_attestation(store, attestation)
+    spec.on_attestation(store, attestation, is_from_block=is_from_block)
     yield get_attestation_file_name(attestation), attestation
     test_steps.append({'attestation': get_attestation_file_name(attestation)})
 
 
-def add_attestation(spec, store, attestation, test_steps, valid=True):
-    yield get_attestation_file_name(attestation), attestation
-
-    if not valid:
-        try:
-            run_on_attestation(spec, store, attestation, valid=True)
-        except AssertionError:
-            test_steps.append({
-                'attestation': get_attestation_file_name(attestation),
-                'valid': False,
-            })
-            return
-        else:
-            assert False
-
-    run_on_attestation(spec, store, attestation, valid=True)
-    test_steps.append({'attestation': get_attestation_file_name(attestation)})
-
-
-def run_on_attestation(spec, store, attestation, valid=True):
+def run_on_attestation(spec, store, attestation, is_from_block=False, valid=True):
     if not valid:
         try:
-            spec.on_attestation(store, attestation)
+            spec.on_attestation(store, attestation, is_from_block=is_from_block)
         except AssertionError:
             return
         else:
             assert False
 
-    spec.on_attestation(store, attestation)
+    spec.on_attestation(store, attestation, is_from_block=is_from_block)
 
 
 def get_genesis_forkchoice_store(spec, genesis_state):
@@ -131,7 +111,6 @@ def add_block(spec,
               signed_block,
               test_steps,
               valid=True,
-              allow_invalid_attestations=False,
               block_not_found=False):
     """
     Run on_block and on_attestation
@@ -156,14 +135,8 @@ def add_block(spec,
     test_steps.append({'block': get_block_file_name(signed_block)})
 
     # An on_block step implies receiving block's attestations
-    try:
-        for attestation in signed_block.message.body.attestations:
-            run_on_attestation(spec, store, attestation, valid=True)
-    except AssertionError:
-        if allow_invalid_attestations:
-            pass
-        else:
-            raise
+    for attestation in signed_block.message.body.attestations:
+        run_on_attestation(spec, store, attestation, is_from_block=True, valid=True)
 
     block_root = signed_block.message.hash_tree_root()
     assert store.blocks[block_root] == signed_block.message

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

@@ -543,21 +543,17 @@ def test_new_justified_is_later_than_store_justified(spec, state):
     assert fork_3_state.finalized_checkpoint.epoch == 3
     assert fork_3_state.current_justified_checkpoint.epoch == 4
 
-    # FIXME: pending on the `on_block`, `on_attestation` fix
-    # # Apply blocks of `fork_3_state` to `store`
-    # for block in all_blocks:
-    #     if store.time < spec.compute_time_at_slot(fork_2_state, block.message.slot):
-    #         time = store.genesis_time + block.message.slot * spec.config.SECONDS_PER_SLOT
-    #         on_tick_and_append_step(spec, store, time, test_steps)
-    #     # valid_attestations=False because the attestations are outdated (older than previous epoch)
-    #     yield from add_block(spec, store, block, test_steps, allow_invalid_attestations=False)
-
-    # assert store.finalized_checkpoint == fork_3_state.finalized_checkpoint
-    # assert (store.justified_checkpoint
-    #         == fork_3_state.current_justified_checkpoint
-    #         != store.best_justified_checkpoint)
-    # assert (store.best_justified_checkpoint
-    #         == fork_2_state.current_justified_checkpoint)
+    # Apply blocks of `fork_3_state` to `store`
+    for block in all_blocks:
+        if store.time < spec.compute_time_at_slot(fork_2_state, block.message.slot):
+            time = store.genesis_time + block.message.slot * spec.config.SECONDS_PER_SLOT
+            on_tick_and_append_step(spec, store, time, test_steps)
+        yield from add_block(spec, store, block, test_steps)
+
+    assert store.finalized_checkpoint == fork_3_state.finalized_checkpoint
+    assert store.justified_checkpoint == fork_3_state.current_justified_checkpoint
+    assert store.justified_checkpoint != store.best_justified_checkpoint
+    assert store.best_justified_checkpoint == fork_2_state.current_justified_checkpoint
 
     yield 'steps', test_steps
 
@@ -622,20 +618,19 @@ def test_new_finalized_slot_is_not_justified_checkpoint_ancestor(spec, state):
     assert state.finalized_checkpoint != another_state.finalized_checkpoint
     assert state.current_justified_checkpoint != another_state.current_justified_checkpoint
 
-    # pre_store_justified_checkpoint_root = store.justified_checkpoint.root
+    pre_store_justified_checkpoint_root = store.justified_checkpoint.root
 
-    # FIXME: pending on the `on_block`, `on_attestation` fix
-    # # Apply blocks of `another_state` to `store`
-    # for block in all_blocks:
-    #     # NOTE: Do not call `on_tick` here
-    #     yield from add_block(spec, store, block, test_steps, allow_invalid_attestations=True)
+    # Apply blocks of `another_state` to `store`
+    for block in all_blocks:
+        # NOTE: Do not call `on_tick` here
+        yield from add_block(spec, store, block, test_steps)
 
-    # finalized_slot = spec.compute_start_slot_at_epoch(store.finalized_checkpoint.epoch)
-    # ancestor_at_finalized_slot = spec.get_ancestor(store, pre_store_justified_checkpoint_root, finalized_slot)
-    # assert ancestor_at_finalized_slot != store.finalized_checkpoint.root
+    finalized_slot = spec.compute_start_slot_at_epoch(store.finalized_checkpoint.epoch)
+    ancestor_at_finalized_slot = spec.get_ancestor(store, pre_store_justified_checkpoint_root, finalized_slot)
+    assert ancestor_at_finalized_slot != store.finalized_checkpoint.root
 
-    # assert store.finalized_checkpoint == another_state.finalized_checkpoint
-    # assert store.justified_checkpoint == another_state.current_justified_checkpoint
+    assert store.finalized_checkpoint == another_state.finalized_checkpoint
+    assert store.justified_checkpoint == another_state.current_justified_checkpoint
 
     yield 'steps', test_steps
 
@@ -698,15 +693,13 @@ def test_new_finalized_slot_is_justified_checkpoint_ancestor(spec, state):
 
     pre_store_justified_checkpoint_root = store.justified_checkpoint.root
     for block in all_blocks:
-        # FIXME: Once `on_block` and `on_attestation` logic is fixed,
-        # fix test case and remove allow_invalid_attestations flag
-        yield from tick_and_add_block(spec, store, block, test_steps, allow_invalid_attestations=True)
+        yield from tick_and_add_block(spec, store, block, test_steps)
 
     finalized_slot = spec.compute_start_slot_at_epoch(store.finalized_checkpoint.epoch)
     ancestor_at_finalized_slot = spec.get_ancestor(store, pre_store_justified_checkpoint_root, finalized_slot)
     assert ancestor_at_finalized_slot == store.finalized_checkpoint.root
 
     assert store.finalized_checkpoint == another_state.finalized_checkpoint
-    assert store.justified_checkpoint != another_state.current_justified_checkpoint
+    assert store.justified_checkpoint == another_state.current_justified_checkpoint
 
     yield 'steps', test_steps