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
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 15 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_ancestor_at_epoch_boundary`](#get_ancestor_at_epoch_boundary)
- [`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_ancestor_at_epoch_boundary`

```python
def get_ancestor_at_epoch_boundary(store: Store, root: Root, epoch: Epoch) -> Root:
"""
Compute the epoch boundary 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,9 @@ 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)
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 == get_ancestor_at_epoch_boundary(store, block_root, store.finalized_checkpoint.epoch)
)

# If expected finalized/justified, add to viable block-tree and signal viability to parent.
Expand Down Expand Up @@ -442,8 +453,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_ancestor_at_epoch_boundary(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 +516,7 @@ 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

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.

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

# Check the block is valid and compute the post-state
state = pre_state.copy()
Expand Down