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

Confirmation Rule #3339

Open
wants to merge 171 commits into
base: dev
Choose a base branch
from

Conversation

saltiniroberto
Copy link
Contributor

@saltiniroberto saltiniroberto commented Apr 27, 2023

Introduction

The objective of this PR is to introduce a Confirmation Rule for the Ethereum protocol.
A confirmation rule is an algorithm run by nodes, outputting whether a certain block is confirmed. When that is the case, the block is guaranteed to never be reorged, under certain assumptions, primarily about network synchrony and about the percentage of honest stake.

Detailed Explanation

For a detailed explanation of the algorithm, see this article.
The algorithm specified in this PR corresponds to Algorithm 5 in the paper.

TODO

Here is a non-exclusive list of TODOs

  • Review the spec change for correctness
    • Algorithm correctness
    • Type correctness (e.g. int vs float or uint64)
  • Add preconditions (e.g. parameters >= 0, etc..)
  • Review the naming used for the various functions
  • Improve the documentation in the spec
  • Add more tests
  • Check that the changes to the file setup.py are correct. The current changes allow linting the confirmation rule spec, but they may not be entirely correct.

Last things to do before merging

  • Revert the changes to linter.ini. These changes have been introduced just to speed up the development process by relaxing the requirement on the maximum line length.
  • Move fork_choice/confirmation_rule.md to specs/bellatrix and delete the fork_choice folder.
  • Make sure PR Confirmation rule prerequisite - fork choice filter change #3431 has already been merged
  • Double check any change to the fork choice tests

saltiniroberto and others added 30 commits April 12, 2023 12:41
Copy link

@Mart1i1n Mart1i1n left a comment

Choose a reason for hiding this comment

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

Find type

fork_choice/confirmation-rule.md Outdated Show resolved Hide resolved
@saltiniroberto saltiniroberto marked this pull request as ready for review May 2, 2024 13:28

return (
end_epoch > start_epoch + 1
or (end_epoch == start_epoch + 1 and start_slot % SLOTS_PER_EPOCH == 0))

Choose a reason for hiding this comment

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

If start_slot = 0 and end_slot = 31 (where end_epoch == start_epoch == 0), do we count it as "includes an entire epoch"? If not, should we describe the function as exclusive end_slot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another way would be to change the doc to

Returns True if the range from start_slot to end_slot (inclusive of both) includes an entire epoch

In this way, we are not saying that if the range includes an entire epoch, then the function returns True.
Only that whenever the function returns True than the range includes an entire epoch.

The above works with both inclusive and exclusive.

Comment on lines 211 to 218
if is_full_validator_set_for_block_covered(store, block_root):
return is_one_confirmed(store, block_root)
else:
block = store.blocks[block_root]
return (
is_one_confirmed(store, block_root)
and is_lmd_confirmed(store, block.parent_root)
)

Choose a reason for hiding this comment

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

Is it different from the definition 6 (lmd-safety condition) in the paper? The paper require all the ancestors of the block to be is_lmd_confirmed, regardless of whether full validator set is covered or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It is different.
This does not always yield exactly the same result as in the paper, but it does in most cases, and it is quicker to compute.

current_slot = get_current_slot(store)
block = store.blocks[block_root]
parent_block = store.blocks[block.parent_root]
support = int(get_weight(store, block_root))

Choose a reason for hiding this comment

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

Is this the weight from the fork_choice data, which can be queried from the beacon API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so.


block_epoch = compute_epoch_at_slot(block.slot)

# If `block_epoch` is not either the current or previous epoch, then return `store.finalized_checkpoint.root`

Choose a reason for hiding this comment

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

What is the confirmation rule for the block between finalized checkpoint and justified checkpoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any block descendant of the latest finalized checkpoint is treated in the same way

support = int(get_weight(store, block_root))
justified_state = store.checkpoint_states[store.justified_checkpoint]
maximum_support = int(
get_committee_weight_between_slots(justified_state, Slot(parent_block.slot + 1), Slot(current_slot - 1))

Choose a reason for hiding this comment

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

Do we assume that we run the protocol at the beginning of each slot, when the block for the current slot is not proposed? Otherwise, for the block proposed in the current slot (with its parent proposed in the previous slot), Slot(parent_block.slot + 1) > Slot(current_slot - 1).

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 protocol is run at the beginning of each slot regardless of whether we propose or not a block in that slot. Also, we do not run the protocol on blocks for the current slots regardless.

Comment on lines 358 to 362
min(
ceil_div(total_active_balance * CONFIRMATION_BYZANTINE_THRESHOLD, 100),
CONFIRMATION_SLASHING_THRESHOLD,
ffg_support_for_checkpoint
)

Choose a reason for hiding this comment

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

Is ceil_div(total_active_balance * CONFIRMATION_BYZANTINE_THRESHOLD, 100) always smaller than or equal to CONFIRMATION_SLASHING_THRESHOLD?

Choose a reason for hiding this comment

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

In the paper, it is ceil_div((total_active_balance - remaining_ffg_weight) * CONFIRMATION_BYZANTINE_THRESHOLD, 100). I wonder whether it is a typo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is ceil_div(total_active_balance * CONFIRMATION_BYZANTINE_THRESHOLD, 100) always smaller than or equal to CONFIRMATION_SLASHING_THRESHOLD

Not necessarily.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is ceil_div(total_active_balance * CONFIRMATION_BYZANTINE_THRESHOLD, 100) always smaller than or equal to CONFIRMATION_SLASHING_THRESHOLD?

Which paper are you referring to?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
general:enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants