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

Conversation

saltiniroberto
Copy link
Contributor

@saltiniroberto saltiniroberto commented Mar 28, 2023

This PR introduces the function get_checkpoint_block and replaces any computation of checkpoint blocks in the fork choice spec with a call to the newly introduced function.

The objective of this PR is to increase the readability of the spec by better clarifying when we are interested in calculating an checkpoint block.

Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

looks good just from a refactor perspective and I like the increased semantic content

Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

It looks like a nice-to-have to me. No strong opinion here.

/cc @adiasg

Some CircleCI didn't start with the latest commit. It may need a kick with a new commit. 😬

@@ -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

@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.

specs/phase0/fork-choice.md Outdated Show resolved Hide resolved
Copy link
Contributor

@potuz potuz left a comment

Choose a reason for hiding this comment

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

You need to change the corresponding files in different forks that changed on_block, eg. Bellatrix

@saltiniroberto
Copy link
Contributor Author

saltiniroberto commented Mar 30, 2023

You need to change the corresponding files in different forks that changed on_block, eg. Bellatrix

👍
Done.

@ppopth
Copy link
Member

ppopth commented Apr 5, 2023

Please change the p2p network specs as well.

specs/phase0/p2p-interface.md:  `get_ancestor(store, block.parent_root, compute_start_slot_at_epoch(store.finalized_checkpoint.epoch))
specs/phase0/p2p-interface.md:  `get_ancestor(store, aggregate.data.beacon_block_root, compute_start_slot_at_epoch(store.finalized_checkpoint.epoch))
specs/phase0/p2p-interface.md:  `get_ancestor(store, attestation.data.beacon_block_root, compute_start_slot_at_epoch(attestation.data.target.epoch)) == attestation.data.target.root`
specs/phase0/p2p-interface.md:  `get_ancestor(store, attestation.data.beacon_block_root, compute_start_slot_at_epoch(store.finalized_checkpoint.epoch))

@ppopth
Copy link
Member

ppopth commented Apr 5, 2023

We should also update the pyspec correspondingly, if we want to apply this patch.

For example, in tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py, we should change the following.

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)

Please run git grep get_ancestor tests/ to see all the places that need change.

Comment on lines 85 to 89
assert store.finalized_checkpoint.root == get_ancestor_at_epoch_boundary(
store,
block.parent_root,
store.finalized_checkpoint.epoch,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Doing an assignment before the assert is preferable to reduce line count and increase readability. ditto at a few other places in this patch

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done here in all other places marked with "ditto"

@@ -170,7 +170,11 @@ 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
assert store.finalized_checkpoint.root == get_ancestor_at_epoch_boundary(
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

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(
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@@ -506,7 +520,11 @@ 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
assert store.finalized_checkpoint.root == get_ancestor_at_epoch_boundary(
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor

@adiasg adiasg left a comment

Choose a reason for hiding this comment

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

The fork choice & p2p spec do not refer to epoch boundary blocks before this PR. More generally, the spec refers to epoch boundary blocks as part of Checkpoints.

I'd prefer to avoid introducing the "epoch boundary block" nomenclature here, and instead rename get_ancestor_at_epoch_boundary to get_checkpoint_block.

@saltiniroberto
Copy link
Contributor Author

make test report was

================================================================================================================================================= short test summary info ==================================================================================================================================================
FAILED eth2spec/test/deneb/unittests/polynomial_commitments/test_polynomial_commitments.py::test_validate_kzg_g1_not_in_g1 - AssertionError: expected an assertion error, but got none.
FAILED eth2spec/test/deneb/unittests/polynomial_commitments/test_polynomial_commitments.py::test_validate_kzg_g1_not_on_curve - AssertionError: expected an assertion error, but got none.
================================================================================================================================= 2 failed, 806 passed, 146 skipped in 2537.18s (0:42:17) ==================================================================================================================================

@saltiniroberto
Copy link
Contributor Author

We should also update the pyspec correspondingly, if we want to apply this patch.

For example, in tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py, we should change the following.

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)

Please run git grep get_ancestor tests/ to see all the places that need change.

Done

@saltiniroberto
Copy link
Contributor Author

The fork choice & p2p spec do not refer to epoch boundary blocks before this PR. More generally, the spec refers to epoch boundary blocks as part of Checkpoints.

I'd prefer to avoid introducing the "epoch boundary block" nomenclature here, and instead rename get_ancestor_at_epoch_boundary to get_checkpoint_block.

Done

@saltiniroberto saltiniroberto changed the title Add get_epoch_boundary_block to the fork choice spec Add get_checkpoint_block to the fork choice spec Apr 18, 2023
@hwwhww hwwhww merged commit acccbd7 into ethereum:dev May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants