From f1697d03e73e6d3adf68ae4f27d5b27ea3892edd Mon Sep 17 00:00:00 2001 From: Danny Ryan Date: Mon, 20 Jan 2020 17:40:36 -0700 Subject: [PATCH 1/3] fix corner case to properly handle skipped slots in get_ancestor --- specs/phase0/fork-choice.md | 20 +++------ .../test/fork_choice/test_on_block.py | 42 +++++++++++++++++++ 2 files changed, 48 insertions(+), 14 deletions(-) diff --git a/specs/phase0/fork-choice.md b/specs/phase0/fork-choice.md index 0d9823fcd..64d48d4b5 100644 --- a/specs/phase0/fork-choice.md +++ b/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 @@ -284,12 +280,8 @@ def on_block(store: Store, signed_block: SignedBeaconBlock) -> None: # 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) + finalized_slot = compute_start_slot_at_epoch(store.finalized_checkpoint.epoch) + 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 diff --git a/tests/core/pyspec/eth2spec/test/fork_choice/test_on_block.py b/tests/core/pyspec/eth2spec/test/fork_choice/test_on_block.py index 10d1c0011..f9995ad13 100644 --- a/tests/core/pyspec/eth2spec/test/fork_choice/test_on_block.py +++ b/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): From e98c1b415445e239c98e3382ed4ba475ee5e3c84 Mon Sep 17 00:00:00 2001 From: Danny Ryan Date: Mon, 20 Jan 2020 18:10:39 -0700 Subject: [PATCH 2/3] don't consider blocks with slots earlier than finalized in on_block fork choice (optimization) --- specs/phase0/fork-choice.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/specs/phase0/fork-choice.md b/specs/phase0/fork-choice.md index 64d48d4b5..39d9bd402 100644 --- a/specs/phase0/fork-choice.md +++ b/specs/phase0/fork-choice.md @@ -279,9 +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 + + # 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 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 From b357e43aab037e8a7f3fd5dced3046f2be765848 Mon Sep 17 00:00:00 2001 From: Danny Ryan Date: Wed, 22 Jan 2020 14:31:23 -0700 Subject: [PATCH 3/3] clarifying comment on call to get_ancestor in on_block --- specs/phase0/fork-choice.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specs/phase0/fork-choice.md b/specs/phase0/fork-choice.md index 39d9bd402..a69252a37 100644 --- a/specs/phase0/fork-choice.md +++ b/specs/phase0/fork-choice.md @@ -283,7 +283,7 @@ def on_block(store: Store, signed_block: SignedBeaconBlock) -> None: # 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 + # 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