diff --git a/specs/merge/fork-choice.md b/specs/merge/fork-choice.md index 154766dbd..8c5bc1312 100644 --- a/specs/merge/fork-choice.md +++ b/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 ``` diff --git a/specs/phase0/fork-choice.md b/specs/phase0/fork-choice.md index 276aa8029..6854a8487 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_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 diff --git a/tests/core/pyspec/eth2spec/test/helpers/fork_choice.py b/tests/core/pyspec/eth2spec/test/helpers/fork_choice.py index 7c6ac89a5..098c05bf9 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=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 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..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 @@ -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) + # 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 + 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