From cd3d2ce69219b80d92eec0dde8d3dec7b1be6a3c Mon Sep 17 00:00:00 2001 From: Danny Ryan Date: Fri, 12 Nov 2021 12:36:02 -0700 Subject: [PATCH 1/6] working through test issues --- specs/phase0/fork-choice.md | 14 +------------- .../pyspec/eth2spec/test/helpers/fork_choice.py | 2 ++ .../test/phase0/fork_choice/test_on_block.py | 7 +++++++ 3 files changed, 10 insertions(+), 13 deletions(-) diff --git a/specs/phase0/fork-choice.md b/specs/phase0/fork-choice.md index 276aa8029..e8f6eacf1 100644 --- a/specs/phase0/fork-choice.md +++ b/specs/phase0/fork-choice.md @@ -372,19 +372,7 @@ 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` diff --git a/tests/core/pyspec/eth2spec/test/helpers/fork_choice.py b/tests/core/pyspec/eth2spec/test/helpers/fork_choice.py index 7c6ac89a5..ae088fccd 100644 --- a/tests/core/pyspec/eth2spec/test/helpers/fork_choice.py +++ b/tests/core/pyspec/eth2spec/test/helpers/fork_choice.py @@ -166,6 +166,7 @@ def add_block(spec, raise block_root = signed_block.message.hash_tree_root() + print(encode_hex(block_root)) assert store.blocks[block_root] == signed_block.message assert store.block_states[block_root].hash_tree_root() == signed_block.message.state_root test_steps.append({ @@ -186,6 +187,7 @@ def add_block(spec, }, } }) + print(test_steps[-1]) return store.block_states[signed_block.message.hash_tree_root()] diff --git a/tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py b/tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py index 79bf353b1..f90d1acbc 100644 --- a/tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py +++ b/tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py @@ -658,6 +658,7 @@ def test_new_finalized_slot_is_justified_checkpoint_ancestor(spec, state): """ test_steps = [] # Initialization + print("INIT") store, anchor_block = get_genesis_forkchoice_store_and_block(spec, state) yield 'anchor_state', state yield 'anchor_block', anchor_block @@ -679,12 +680,14 @@ def test_new_finalized_slot_is_justified_checkpoint_ancestor(spec, state): state, store, _ = yield from apply_next_epoch_with_attestations( spec, state, store, False, True, test_steps=test_steps) + print("INIT FIN") assert state.finalized_checkpoint.epoch == store.finalized_checkpoint.epoch == 2 assert state.current_justified_checkpoint.epoch == store.justified_checkpoint.epoch == 4 assert store.justified_checkpoint == state.current_justified_checkpoint # Create another chain # Forking from epoch 3 + print("FORK") all_blocks = [] slot = spec.compute_start_slot_at_epoch(3) block_root = spec.get_block_root_at_slot(state, slot) @@ -696,6 +699,7 @@ def test_new_finalized_slot_is_justified_checkpoint_ancestor(spec, state): assert another_state.finalized_checkpoint.epoch == 3 assert another_state.current_justified_checkpoint.epoch == 4 + print("APPLY FORK TO FORK CHOICE") pre_store_justified_checkpoint_root = store.justified_checkpoint.root for block in all_blocks: # FIXME: Once `on_block` and `on_attestation` logic is fixed, @@ -707,6 +711,9 @@ def test_new_finalized_slot_is_justified_checkpoint_ancestor(spec, state): assert ancestor_at_finalized_slot == store.finalized_checkpoint.root assert store.finalized_checkpoint == another_state.finalized_checkpoint + # Thus should fail with the fix. Once show fail, swap to == assert store.justified_checkpoint != another_state.current_justified_checkpoint + print(store.finalized_checkpoint) + print(store.justified_checkpoint) yield 'steps', test_steps From f643554aa519ce6efe015ddc97d564e90c0fe248 Mon Sep 17 00:00:00 2001 From: Hsiao-Wei Wang Date: Sat, 13 Nov 2021 18:23:21 +0800 Subject: [PATCH 2/6] Fix issue around on_attestation validation by skipping epoch scope validation if attestation is from a block message --- specs/phase0/fork-choice.md | 21 +++++++-- .../eth2spec/test/helpers/fork_choice.py | 43 ++++--------------- .../test/phase0/fork_choice/test_on_block.py | 33 +++++++------- 3 files changed, 42 insertions(+), 55 deletions(-) diff --git a/specs/phase0/fork-choice.md b/specs/phase0/fork-choice.md index e8f6eacf1..ed572a96e 100644 --- a/specs/phase0/fork-choice.md +++ b/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_scope`](#validate_target_epoch_scope) - [`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_scope` ```python -def validate_on_attestation(store: Store, attestation: Attestation) -> None: +def validate_target_epoch_scope(store: Store, attestation: Attestation) -> None: target = attestation.data.target # Attestations must be from the current or previous epoch @@ -269,6 +271,15 @@ 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) -> None: + target = attestation.data.target + + # 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 @@ -378,7 +389,7 @@ def on_block(store: Store, signed_block: SignedBeaconBlock) -> None: #### `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. @@ -386,6 +397,10 @@ def on_attestation(store: Store, attestation: Attestation) -> None: consider scheduling it for later processing in such case. """ validate_on_attestation(store, attestation) + if not is_from_block: + # If the given attestation is not from a beacon block message, we have to check the target epoch scope. + validate_target_epoch_scope(store, attestation) + store_target_checkpoint_state(store, attestation.data.target) # Get state at the `target` to fully validate attestation diff --git a/tests/core/pyspec/eth2spec/test/helpers/fork_choice.py b/tests/core/pyspec/eth2spec/test/helpers/fork_choice.py index ae088fccd..1c49c102c 100644 --- a/tests/core/pyspec/eth2spec/test/helpers/fork_choice.py +++ b/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 - +def run_on_attestation(spec, store, attestation, is_from_block=False, valid=True): 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): - if not valid: - try: - spec.on_attestation(store, attestation) + spec.on_attestation(store, attestation, is_from_block) except AssertionError: return else: assert False - spec.on_attestation(store, attestation) + spec.on_attestation(store, attestation, 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() print(encode_hex(block_root)) diff --git a/tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py b/tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py index f90d1acbc..1f2f45352 100644 --- a/tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py +++ b/tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py @@ -543,21 +543,19 @@ 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) + # 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 - # != store.best_justified_checkpoint) - # assert (store.best_justified_checkpoint - # == fork_2_state.current_justified_checkpoint) + 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) yield 'steps', test_steps @@ -628,7 +626,7 @@ def test_new_finalized_slot_is_not_justified_checkpoint_ancestor(spec, state): # # 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) + # 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) @@ -704,7 +702,7 @@ def test_new_finalized_slot_is_justified_checkpoint_ancestor(spec, state): 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) @@ -712,8 +710,9 @@ def test_new_finalized_slot_is_justified_checkpoint_ancestor(spec, state): assert store.finalized_checkpoint == another_state.finalized_checkpoint # Thus should fail with the fix. Once show fail, swap to == - assert store.justified_checkpoint != another_state.current_justified_checkpoint + # assert store.justified_checkpoint != another_state.current_justified_checkpoint print(store.finalized_checkpoint) print(store.justified_checkpoint) + print('spec.get_head(store)', spec.get_head(store)) yield 'steps', test_steps From d9e8306c5a587a40f8285184428023f845048f95 Mon Sep 17 00:00:00 2001 From: Hsiao-Wei Wang Date: Sat, 13 Nov 2021 18:38:53 +0800 Subject: [PATCH 3/6] Refactoring --- specs/phase0/fork-choice.md | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/specs/phase0/fork-choice.md b/specs/phase0/fork-choice.md index ed572a96e..6331f496f 100644 --- a/specs/phase0/fork-choice.md +++ b/specs/phase0/fork-choice.md @@ -276,9 +276,13 @@ def validate_target_epoch_scope(store: Store, attestation: Attestation) -> None: ##### `validate_on_attestation` ```python -def validate_on_attestation(store: Store, attestation: Attestation) -> None: +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_scope(store, attestation) + # Check that the epoch number and slot number are matching assert target.epoch == compute_epoch_at_slot(attestation.data.slot) @@ -396,10 +400,7 @@ def on_attestation(store: Store, attestation: Attestation, is_from_block: bool=F 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) - if not is_from_block: - # If the given attestation is not from a beacon block message, we have to check the target epoch scope. - validate_target_epoch_scope(store, attestation) + validate_on_attestation(store, attestation, is_from_block) store_target_checkpoint_state(store, attestation.data.target) From eb00f8f735a1d6e5d3fe614119d0cbcbceeeef77 Mon Sep 17 00:00:00 2001 From: Danny Ryan Date: Wed, 17 Nov 2021 18:15:12 -0700 Subject: [PATCH 4/6] cleanup forkchoice tests --- specs/phase0/fork-choice.md | 11 ++++++---- .../eth2spec/test/helpers/fork_choice.py | 6 ++---- .../test/phase0/fork_choice/test_on_block.py | 20 ++++++++----------- 3 files changed, 17 insertions(+), 20 deletions(-) diff --git a/specs/phase0/fork-choice.md b/specs/phase0/fork-choice.md index 6331f496f..723102e4d 100644 --- a/specs/phase0/fork-choice.md +++ b/specs/phase0/fork-choice.md @@ -22,7 +22,7 @@ - [`get_head`](#get_head) - [`should_update_justified_checkpoint`](#should_update_justified_checkpoint) - [`on_attestation` helpers](#on_attestation-helpers) - - [`validate_target_epoch_scope`](#validate_target_epoch_scope) + - [`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) @@ -259,10 +259,10 @@ def should_update_justified_checkpoint(store: Store, new_justified_checkpoint: C #### `on_attestation` helpers -##### `validate_target_epoch_scope` +##### `validate_target_epoch_against_current_time` ```python -def validate_target_epoch_scope(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 @@ -281,7 +281,7 @@ def validate_on_attestation(store: Store, attestation: Attestation, is_from_bloc # 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_scope(store, attestation) + 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) @@ -386,6 +386,9 @@ def on_block(store: Store, signed_block: SignedBeaconBlock) -> None: # Update finalized checkpoint if state.finalized_checkpoint.epoch > store.finalized_checkpoint.epoch: + print("made it") + print(state.finalized_checkpoint) + print(state.current_justified_checkpoint) store.finalized_checkpoint = state.finalized_checkpoint store.justified_checkpoint = state.current_justified_checkpoint ``` diff --git a/tests/core/pyspec/eth2spec/test/helpers/fork_choice.py b/tests/core/pyspec/eth2spec/test/helpers/fork_choice.py index 1c49c102c..098c05bf9 100644 --- a/tests/core/pyspec/eth2spec/test/helpers/fork_choice.py +++ b/tests/core/pyspec/eth2spec/test/helpers/fork_choice.py @@ -60,13 +60,13 @@ def tick_and_run_on_attestation(spec, store, attestation, test_steps, is_from_bl def run_on_attestation(spec, store, attestation, is_from_block=False, valid=True): if not valid: try: - spec.on_attestation(store, attestation, is_from_block) + spec.on_attestation(store, attestation, is_from_block=is_from_block) except AssertionError: return else: assert False - spec.on_attestation(store, attestation, is_from_block) + spec.on_attestation(store, attestation, is_from_block=is_from_block) def get_genesis_forkchoice_store(spec, genesis_state): @@ -139,7 +139,6 @@ def add_block(spec, run_on_attestation(spec, store, attestation, is_from_block=True, valid=True) block_root = signed_block.message.hash_tree_root() - print(encode_hex(block_root)) assert store.blocks[block_root] == signed_block.message assert store.block_states[block_root].hash_tree_root() == signed_block.message.state_root test_steps.append({ @@ -160,7 +159,6 @@ def add_block(spec, }, } }) - print(test_steps[-1]) return store.block_states[signed_block.message.hash_tree_root()] diff --git a/tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py b/tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py index 1f2f45352..3670a883a 100644 --- a/tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py +++ b/tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py @@ -551,11 +551,9 @@ def test_new_justified_is_later_than_store_justified(spec, state): 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 - != store.best_justified_checkpoint) - assert (store.best_justified_checkpoint - == fork_2_state.current_justified_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 @@ -656,7 +654,6 @@ def test_new_finalized_slot_is_justified_checkpoint_ancestor(spec, state): """ test_steps = [] # Initialization - print("INIT") store, anchor_block = get_genesis_forkchoice_store_and_block(spec, state) yield 'anchor_state', state yield 'anchor_block', anchor_block @@ -678,14 +675,12 @@ def test_new_finalized_slot_is_justified_checkpoint_ancestor(spec, state): state, store, _ = yield from apply_next_epoch_with_attestations( spec, state, store, False, True, test_steps=test_steps) - print("INIT FIN") assert state.finalized_checkpoint.epoch == store.finalized_checkpoint.epoch == 2 assert state.current_justified_checkpoint.epoch == store.justified_checkpoint.epoch == 4 assert store.justified_checkpoint == state.current_justified_checkpoint # Create another chain # Forking from epoch 3 - print("FORK") all_blocks = [] slot = spec.compute_start_slot_at_epoch(3) block_root = spec.get_block_root_at_slot(state, slot) @@ -697,7 +692,6 @@ def test_new_finalized_slot_is_justified_checkpoint_ancestor(spec, state): assert another_state.finalized_checkpoint.epoch == 3 assert another_state.current_justified_checkpoint.epoch == 4 - print("APPLY FORK TO FORK CHOICE") pre_store_justified_checkpoint_root = store.justified_checkpoint.root for block in all_blocks: # FIXME: Once `on_block` and `on_attestation` logic is fixed, @@ -708,11 +702,13 @@ def test_new_finalized_slot_is_justified_checkpoint_ancestor(spec, state): 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 - # Thus should fail with the fix. Once show fail, swap to == - # assert store.justified_checkpoint != another_state.current_justified_checkpoint print(store.finalized_checkpoint) print(store.justified_checkpoint) + print(another_state.current_justified_checkpoint) print('spec.get_head(store)', spec.get_head(store)) + assert store.finalized_checkpoint == another_state.finalized_checkpoint + # Thus should fail with the fix. Once show fail, swap to == + assert store.justified_checkpoint == another_state.current_justified_checkpoint + yield 'steps', test_steps From 3d4ece44df9b0e32bf07820f9ea820358e9cf626 Mon Sep 17 00:00:00 2001 From: Danny Ryan Date: Wed, 17 Nov 2021 18:37:45 -0700 Subject: [PATCH 5/6] port phase0 forkchocie changes to merge --- specs/merge/fork-choice.md | 14 +------------- specs/phase0/fork-choice.md | 3 --- .../test/phase0/fork_choice/test_on_block.py | 8 -------- 3 files changed, 1 insertion(+), 24 deletions(-) diff --git a/specs/merge/fork-choice.md b/specs/merge/fork-choice.md index 3f0c67200..d60a07d1c 100644 --- a/specs/merge/fork-choice.md +++ b/specs/merge/fork-choice.md @@ -186,17 +186,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 ``` diff --git a/specs/phase0/fork-choice.md b/specs/phase0/fork-choice.md index 723102e4d..6854a8487 100644 --- a/specs/phase0/fork-choice.md +++ b/specs/phase0/fork-choice.md @@ -386,9 +386,6 @@ def on_block(store: Store, signed_block: SignedBeaconBlock) -> None: # Update finalized checkpoint if state.finalized_checkpoint.epoch > store.finalized_checkpoint.epoch: - print("made it") - print(state.finalized_checkpoint) - print(state.current_justified_checkpoint) store.finalized_checkpoint = state.finalized_checkpoint store.justified_checkpoint = state.current_justified_checkpoint ``` diff --git a/tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py b/tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py index 3670a883a..ff2d77a8f 100644 --- a/tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py +++ b/tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py @@ -694,21 +694,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) 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 - print(store.finalized_checkpoint) - print(store.justified_checkpoint) - print(another_state.current_justified_checkpoint) - print('spec.get_head(store)', spec.get_head(store)) - assert store.finalized_checkpoint == another_state.finalized_checkpoint - # Thus should fail with the fix. Once show fail, swap to == assert store.justified_checkpoint == another_state.current_justified_checkpoint yield 'steps', test_steps From 2c865e362799470356b703afdef83c49bb55bd38 Mon Sep 17 00:00:00 2001 From: Hsiao-Wei Wang Date: Thu, 18 Nov 2021 11:02:12 +0800 Subject: [PATCH 6/6] Resolve the commented out code in `test_new_finalized_slot_is_not_justified_checkpoint_ancestor` --- .../test/phase0/fork_choice/test_on_block.py | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py b/tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py index ff2d77a8f..7194c6a20 100644 --- a/tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py +++ b/tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py @@ -618,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) + # 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