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 prerequisite - fork choice filter change #3431

Merged

Conversation

saltiniroberto
Copy link
Contributor

@saltiniroberto saltiniroberto commented Jun 15, 2023

This PR specifies the changes to the Fork Choice specification required for the safe execution of the confirmation rule specified in #3339.

By implementing these changes, the confirmation rule can be safely run by means of beacon APIs calls, though there would be no connection to the execution layer.

See sections 3.1 to 3.4 of this document for the rationale about the changes proposed and the related security analysis.

TODO:

  • Fix tests
  • Add changes to Bellatrix fork choice spec.

specs/phase0/fork-choice.md Outdated Show resolved Hide resolved
specs/phase0/fork-choice.md Show resolved Hide resolved
Co-authored-by: Hsiao-Wei Wang <[email protected]>
@mkalinin
Copy link
Collaborator

The change looks good to me. I don't think Bellatrix FC is affected by the change introduce in this PR. A question, would we need to add new tests along fixing the existing ones? (I guess no but asking just in case)

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

I'm not sure I follow -- is this to pull out a bit of functionality into specifically defined functions so that the beacon-api's can just reference these functions instead of trying to (re)define the behaviour there?

voting_source = get_voting_source(store, block_root)

# The voting source should be at the same height as the store's justified checkpoint
# The voting source should be either at the same height as the store's justified checkpoint or
# not more than two epochs ago
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, this is the substantive change

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

I'm not sure I understand the connection to the Beacon-APIs here. can you elaborate?

@saltiniroberto saltiniroberto changed the title Fork choice changes to enable confirmation rule execution via beacon APIs Fork choice changes to enable safe execution of the confirmation rule via beacon APIs Jul 7, 2023
@saltiniroberto saltiniroberto changed the title Fork choice changes to enable safe execution of the confirmation rule via beacon APIs Fork choice changes to enable the safe execution of the confirmation rule via beacon APIs Jul 7, 2023
@saltiniroberto
Copy link
Contributor Author

I'm not sure I understand the connection to the Beacon-APIs here. can you elaborate?

This PR includes the changes to the Fork Choice required to ensure the safety of the confirmation rule.

This means that, once this PR is implemented, the confirmation rule can be safely executed via standard Beacon API calls as done here.

I have updated the title and PR description accordingly.

I hope that this clarifies.

@mkalinin mkalinin mentioned this pull request Jul 17, 2023
12 tasks
Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

This looks good/correct.

Need to port to non-Phase0 functions as needed and then we can merge

@adiasg adiasg changed the title Fork choice changes to enable the safe execution of the confirmation rule via beacon APIs Confirmation rule prerequisite - fork choice filter change Jul 18, 2023
@saltiniroberto
Copy link
Contributor Author

This looks good/correct.

Need to port to non-Phase0 functions as needed and then we can merge

Just checked and it looks like that no change is required to any of the non-Phase 0 specs.

@saltiniroberto
Copy link
Contributor Author

saltiniroberto commented Jul 24, 2023

We just need to confirm the test fix with @adiasg and @mkalinin
(Currently ongoing)

@hwwhww
Copy link
Contributor

hwwhww commented Jul 25, 2023

The PR itself looks good to me. 👍

But since it's a substantive change, we may need to be clear about when it can be on devnet/testnet/mainnet before merging it.

etan-status added a commit to status-im/nimbus-eth2 that referenced this pull request Sep 20, 2023
To support confirmation rule via beacon-APIs as described in spec PR,
add `--debug-fork-choice-version=pr3431` option and enable when Deneb
fork is scheduled. To opt-out, `--debug-fork-choice-version=stable`,
or don't schedule Deneb.

- ethereum/consensus-specs#3431
- ethereum/consensus-specs#3466

"will bundle this with deneb release":

- ethereum/pm#844 (comment)
etan-status added a commit to status-im/nimbus-eth2 that referenced this pull request Sep 20, 2023
To support confirmation rule via beacon-APIs as described in spec PR,
add `--debug-fork-choice-version=pr3431` option and enable when Deneb
fork is scheduled. To opt-out, `--debug-fork-choice-version=stable`,
or don't schedule Deneb.

- ethereum/consensus-specs#3431
- ethereum/consensus-specs#3466

"will bundle this with deneb release":

- ethereum/pm#844 (comment)
etan-status added a commit to status-im/nimbus-eth2 that referenced this pull request Sep 20, 2023
…5450)

To support confirmation rule via beacon-APIs as described in spec PR,
add `--debug-fork-choice-version=pr3431` option and enable when Deneb
fork is scheduled. To opt-out, `--debug-fork-choice-version=stable`,
or don't schedule Deneb.

- ethereum/consensus-specs#3431
- ethereum/consensus-specs#3466

"will bundle this with deneb release":

- ethereum/pm#844 (comment)
@hwwhww hwwhww mentioned this pull request Jan 15, 2024
8 tasks
@djrtwo djrtwo merged commit 9a54a32 into ethereum:dev Jan 16, 2024
29 checks passed
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.

6 participants