Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add get_checkpoint_block to the fork choice spec #3308

Merged
merged 16 commits into from
May 1, 2023
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion specs/bellatrix/fork-choice.md
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,12 @@ def on_block(store: Store, signed_block: SignedBeaconBlock) -> None:
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 at the checkpoint finalized slot
assert get_ancestor(store, block.parent_root, finalized_slot) == store.finalized_checkpoint.root
finalized_checkpoint_block = get_checkpoint_block(
store,
block.parent_root,
store.finalized_checkpoint.epoch,
)
assert store.finalized_checkpoint.root == finalized_checkpoint_block

# Check the block is valid and compute the post-state
state = pre_state.copy()
Expand Down
7 changes: 6 additions & 1 deletion specs/deneb/fork-choice.md
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,12 @@ def on_block(store: Store, signed_block: SignedBeaconBlock) -> None:
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 at the checkpoint finalized slot
assert get_ancestor(store, block.parent_root, finalized_slot) == store.finalized_checkpoint.root
finalized_checkpoint_block = get_checkpoint_block(
store,
block.parent_root,
store.finalized_checkpoint.epoch,
)
assert store.finalized_checkpoint.root == finalized_checkpoint_block

# [New in Deneb]
# Check if blob data is available
Expand Down
31 changes: 26 additions & 5 deletions specs/phase0/fork-choice.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
- [`get_current_slot`](#get_current_slot)
- [`compute_slots_since_epoch_start`](#compute_slots_since_epoch_start)
- [`get_ancestor`](#get_ancestor)
- [`get_checkpoint_block`](#get_checkpoint_block)
- [`get_weight`](#get_weight)
- [`get_voting_source`](#get_voting_source)
- [`filter_block_tree`](#filter_block_tree)
Expand Down Expand Up @@ -192,6 +193,17 @@ def get_ancestor(store: Store, root: Root, slot: Slot) -> Root:
return root
```

#### `get_checkpoint_block`

```python
def get_checkpoint_block(store: Store, root: Root, epoch: Epoch) -> Root:
"""
Compute the checkpoint block for epoch ``epoch`` in the chain of block ``root``
"""
epoch_first_slot = compute_start_slot_at_epoch(epoch)
return get_ancestor(store, root, epoch_first_slot)
```

#### `get_weight`

```python
Expand Down Expand Up @@ -278,10 +290,15 @@ def filter_block_tree(store: Store, block_root: Root, blocks: Dict[Root, BeaconB
voting_source.epoch + 2 >= current_epoch
)

finalized_slot = compute_start_slot_at_epoch(store.finalized_checkpoint.epoch)
finalized_checkpoint_block = get_checkpoint_block(
store,
block_root,
store.finalized_checkpoint.epoch,
)

correct_finalized = (
store.finalized_checkpoint.epoch == GENESIS_EPOCH
or store.finalized_checkpoint.root == get_ancestor(store, block_root, finalized_slot)
or store.finalized_checkpoint.root == finalized_checkpoint_block
)

# If expected finalized/justified, add to viable block-tree and signal viability to parent.
Expand Down Expand Up @@ -442,8 +459,7 @@ def validate_on_attestation(store: Store, attestation: Attestation, is_from_bloc
assert store.blocks[attestation.data.beacon_block_root].slot <= attestation.data.slot

# LMD vote must be consistent with FFG vote target
target_slot = compute_start_slot_at_epoch(target.epoch)
assert target.root == get_ancestor(store, attestation.data.beacon_block_root, target_slot)
assert target.root == get_checkpoint_block(store, attestation.data.beacon_block_root, target.epoch)

# Attestations can only affect the fork choice of subsequent slots.
# Delay consideration in the fork choice until their slot is in the past.
Expand Down Expand Up @@ -506,7 +522,12 @@ def on_block(store: Store, signed_block: SignedBeaconBlock) -> None:
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 at the checkpoint finalized slot
assert get_ancestor(store, block.parent_root, finalized_slot) == store.finalized_checkpoint.root
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems no need to modify this one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hwwhww Why do you think that there may be no need to modify this one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because we need to define the variable finalized_slot to check assert block.slot > finalized_slot anyway, the change of this change doesn't simplify much here.

Copy link
Contributor Author

@saltiniroberto saltiniroberto Mar 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The objective of this PR is not so much about simplifying the code, but rather making explicit when we are computing an epoch boundary block.

For example, in this line, we are not only checking that store.finalized_checkpoint.root is an ancestor of block.parent_root, but also that store.finalized_checkpoint.root is an epoch boundary block in the chain of block.parent_root, which I don't think that it is immediate in the original code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the difference between checking whether a block is just an ancestor or it is also an epoch boundary block is significant when performing security analysis on the code.

finalized_checkpoint_block = get_checkpoint_block(
store,
block.parent_root,
store.finalized_checkpoint.epoch,
)
assert store.finalized_checkpoint.root == finalized_checkpoint_block

# Check the block is valid and compute the post-state
state = pre_state.copy()
Expand Down
8 changes: 4 additions & 4 deletions specs/phase0/p2p-interface.md
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ The following validations MUST pass before forwarding the `signed_beacon_block`
- _[REJECT]_ The block's parent (defined by `block.parent_root`) passes validation.
- _[REJECT]_ The block is from a higher slot than its parent.
- _[REJECT]_ The current `finalized_checkpoint` is an ancestor of `block` -- i.e.
`get_ancestor(store, block.parent_root, compute_start_slot_at_epoch(store.finalized_checkpoint.epoch))
`get_checkpoint_block(store, block.parent_root, store.finalized_checkpoint.epoch)
== store.finalized_checkpoint.root`
- _[REJECT]_ The block is proposed by the expected `proposer_index` for the block's slot
in the context of the current shuffling (defined by `parent_root`/`slot`).
Expand Down Expand Up @@ -356,7 +356,7 @@ The following validations MUST pass before forwarding the `signed_aggregate_and_
(a client MAY queue aggregates for processing once block is retrieved).
- _[REJECT]_ The block being voted for (`aggregate.data.beacon_block_root`) passes validation.
- _[IGNORE]_ The current `finalized_checkpoint` is an ancestor of the `block` defined by `aggregate.data.beacon_block_root` -- i.e.
`get_ancestor(store, aggregate.data.beacon_block_root, compute_start_slot_at_epoch(store.finalized_checkpoint.epoch))
`get_checkpoint_block(store, aggregate.data.beacon_block_root, finalized_checkpoint.epoch)
== store.finalized_checkpoint.root`


Expand Down Expand Up @@ -425,9 +425,9 @@ The following validations MUST pass before forwarding the `attestation` on the s
(a client MAY queue attestations for processing once block is retrieved).
- _[REJECT]_ The block being voted for (`attestation.data.beacon_block_root`) passes validation.
- _[REJECT]_ The attestation's target block is an ancestor of the block named in the LMD vote -- i.e.
`get_ancestor(store, attestation.data.beacon_block_root, compute_start_slot_at_epoch(attestation.data.target.epoch)) == attestation.data.target.root`
`get_checkpoint_block(store, attestation.data.beacon_block_root, attestation.data.target.epoch) == attestation.data.target.root`
- _[IGNORE]_ The current `finalized_checkpoint` is an ancestor of the `block` defined by `attestation.data.beacon_block_root` -- i.e.
`get_ancestor(store, attestation.data.beacon_block_root, compute_start_slot_at_epoch(store.finalized_checkpoint.epoch))
`get_checkpoint_block(store, attestation.data.beacon_block_root, store.finalized_checkpoint.epoch)
== store.finalized_checkpoint.root`


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,7 @@ def test_voting_source_within_two_epoch(spec, state):
- store.voting_source[block_root].epoch != store.justified_checkpoint.epoch, and
- store.unrealized_justifications[block_root].epoch >= store.justified_checkpoint.epoch, and
- store.voting_source[block_root].epoch + 2 >= current_epoch, and
- store.finalized_checkpoint.root == get_ancestor(store, block_root, finalized_slot)
- store.finalized_checkpoint.root == get_checkpoint_block(store, block_root, store.finalized_checkpoint.epoch)
"""
test_steps = []
# Initialization
Expand Down Expand Up @@ -536,8 +536,11 @@ def test_voting_source_within_two_epoch(spec, state):
assert store.unrealized_justifications[last_fork_block_root].epoch >= store.justified_checkpoint.epoch
# assert store.voting_source[last_fork_block_root].epoch + 2 >= \
# spec.compute_epoch_at_slot(spec.get_current_slot(store))
finalized_slot = spec.compute_start_slot_at_epoch(store.finalized_checkpoint.epoch)
assert store.finalized_checkpoint.root == spec.get_ancestor(store, last_fork_block_root, finalized_slot)
assert store.finalized_checkpoint.root == spec.get_checkpoint_block(
store,
last_fork_block_root,
store.finalized_checkpoint.epoch
)
assert spec.get_head(store) == last_fork_block_root

yield 'steps', test_steps
Expand All @@ -552,7 +555,7 @@ def test_voting_source_beyond_two_epoch(spec, state):
- store.voting_source[block_root].epoch != store.justified_checkpoint.epoch, and
- store.unrealized_justifications[block_root].epoch >= store.justified_checkpoint.epoch, and
- store.voting_source[block_root].epoch + 2 < current_epoch, and
- store.finalized_checkpoint.root == get_ancestor(store, block_root, finalized_slot)
- store.finalized_checkpoint.root == get_checkpoint_block(store, block_root, store.finalized_checkpoint.epoch)
"""
test_steps = []
# Initialization
Expand Down Expand Up @@ -617,8 +620,11 @@ def test_voting_source_beyond_two_epoch(spec, state):
assert store.unrealized_justifications[last_fork_block_root].epoch >= store.justified_checkpoint.epoch
# assert store.voting_source[last_fork_block_root].epoch + 2 < \
# spec.compute_epoch_at_slot(spec.get_current_slot(store))
finalized_slot = spec.compute_start_slot_at_epoch(store.finalized_checkpoint.epoch)
assert store.finalized_checkpoint.root == spec.get_ancestor(store, last_fork_block_root, finalized_slot)
assert store.finalized_checkpoint.root == spec.get_checkpoint_block(
store,
last_fork_block_root,
store.finalized_checkpoint.epoch
)
assert spec.get_head(store) == correct_head

yield 'steps', test_steps
Expand All @@ -641,7 +647,7 @@ def test_incorrect_finalized(spec, state):
# Check that the store doesn't allow for a head block that has:
# - store.voting_source[block_root].epoch == store.justified_checkpoint.epoch, and
# - store.finalized_checkpoint.epoch != GENESIS_EPOCH, and
# - store.finalized_checkpoint.root != get_ancestor(store, block_root, finalized_slot)
# - store.finalized_checkpoint.root != get_checkpoint_block(store, block_root, store.finalized_checkpoint.epoch)
test_steps = []
# Initialization
store, anchor_block = get_genesis_forkchoice_store_and_block(spec, state)
Expand Down Expand Up @@ -718,7 +724,11 @@ def test_incorrect_finalized(spec, state):
assert store.voting_source[last_fork_block_root].epoch == store.justified_checkpoint.epoch
assert store.finalized_checkpoint.epoch != spec.GENESIS_EPOCH
finalized_slot = spec.compute_start_slot_at_epoch(store.finalized_checkpoint.epoch)
assert store.finalized_checkpoint.root != spec.get_ancestor(store, last_fork_block_root, finalized_slot)
assert store.finalized_checkpoint.root != spec.get_checkpoint_block(
store,
block_root,
store.finalized_checkpoint.epoch
)
assert spec.get_head(store) != last_fork_block_root
assert spec.get_head(store) == head_root

Expand Down
46 changes: 34 additions & 12 deletions tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -352,8 +352,11 @@ def test_new_finalized_slot_is_not_justified_checkpoint_ancestor(spec, state):
# 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)
ancestor_at_finalized_slot = spec.get_checkpoint_block(
store,
pre_store_justified_checkpoint_root,
store.finalized_checkpoint.epoch
)
assert ancestor_at_finalized_slot != store.finalized_checkpoint.root

assert store.finalized_checkpoint == another_state.finalized_checkpoint
Expand Down Expand Up @@ -428,8 +431,11 @@ def test_new_finalized_slot_is_justified_checkpoint_ancestor(spec, state):
for block in all_blocks:
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)
ancestor_at_finalized_slot = spec.get_checkpoint_block(
store,
pre_store_justified_checkpoint_root,
store.finalized_checkpoint.epoch
)
assert ancestor_at_finalized_slot == store.finalized_checkpoint.root

assert store.finalized_checkpoint == another_state.finalized_checkpoint
Expand Down Expand Up @@ -857,10 +863,18 @@ def test_incompatible_justification_update_start_of_epoch(spec, state):
# Now add the blocks & check that justification update was triggered
for signed_block in signed_blocks:
yield from tick_and_add_block(spec, store, signed_block, test_steps)
finalized_slot = spec.compute_start_slot_at_epoch(state.finalized_checkpoint.epoch)
assert spec.get_ancestor(store, last_block_root, finalized_slot) == state.finalized_checkpoint.root
justified_slot = spec.compute_start_slot_at_epoch(state.current_justified_checkpoint.epoch)
assert spec.get_ancestor(store, last_block_root, justified_slot) != state.current_justified_checkpoint.root
finalized_checkpoint_block = spec.get_checkpoint_block(
store,
last_block_root,
state.finalized_checkpoint.epoch,
)
assert finalized_checkpoint_block == state.finalized_checkpoint.root
justified_checkpoint_block = spec.get_checkpoint_block(
store,
last_block_root,
state.current_justified_checkpoint.epoch,
)
assert justified_checkpoint_block != state.current_justified_checkpoint.root
assert store.finalized_checkpoint.epoch == 4
assert store.justified_checkpoint.epoch == 6

Expand Down Expand Up @@ -934,10 +948,18 @@ def test_incompatible_justification_update_end_of_epoch(spec, state):
# Now add the blocks & check that justification update was triggered
for signed_block in signed_blocks:
yield from tick_and_add_block(spec, store, signed_block, test_steps)
finalized_slot = spec.compute_start_slot_at_epoch(state.finalized_checkpoint.epoch)
assert spec.get_ancestor(store, last_block_root, finalized_slot) == state.finalized_checkpoint.root
justified_slot = spec.compute_start_slot_at_epoch(state.current_justified_checkpoint.epoch)
assert spec.get_ancestor(store, last_block_root, justified_slot) != state.current_justified_checkpoint.root
finalized_checkpoint_block = spec.get_checkpoint_block(
store,
last_block_root,
state.finalized_checkpoint.epoch,
)
assert finalized_checkpoint_block == state.finalized_checkpoint.root
justified_checkpoint_block = spec.get_checkpoint_block(
store,
last_block_root,
state.current_justified_checkpoint.epoch,
)
assert justified_checkpoint_block != state.current_justified_checkpoint.root
assert store.finalized_checkpoint.epoch == 4
assert store.justified_checkpoint.epoch == 6

Expand Down