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

Include FC filtering change #3431 into Deneb #3466

Closed
mkalinin opened this issue Jul 27, 2023 · 5 comments
Closed

Include FC filtering change #3431 into Deneb #3466

mkalinin opened this issue Jul 27, 2023 · 5 comments
Labels
Deneb was called: eip-4844

Comments

@mkalinin
Copy link
Collaborator

mkalinin commented Jul 27, 2023

We propose to include FC filtering change into Deneb:

The change is mainly a two-liner accompanied with a tiny refactor, the PR also includes updates of tests affected by the change.

As it's coming from discussion of this change on ACDC #114, clients don't support changing the fork choice logic at a specified boundary, so the best coordination we can reach without introducing additional engineering complexity is releasing client software with a new FC logic as a part of Deneb release with potentially 3-4 weeks of two FC versions running on the mainnet.

This thread is created around a few related discussion points:

  • Inclusion into Deneb client releases
  • Merging the PR into consensus-specs and updating tests vectors
  • Enabling the change in one of upcoming Dencun Electra
@dapplion
Copy link
Collaborator

dapplion commented Jul 28, 2023

Along with soft consensus I would prefer to not implement forking logic unless there are strong safety concerns. Thus we should merge #3431 at or after Deneb mainnet fork epoch is set.

@paulhauner
Copy link
Contributor

I suspect Lighthouse would be OK with activating this rule change at a specified epoch. I believe all we'd need to do is wrap the old and new code around an if epoch > activation_epoch block. That might not be the case for other clients though (and that would be totally OK).

If the other clients are open to an epoch-based activation then I'd be open to trying it. I expect the status-quo of a loosely coordinated roll-out based on client releases would also work well, so I'm not opposed to that either.

So, consider my input to be a loosely held vote for trying a more tightly coordinated fork choice update.

@hwwhww
Copy link
Contributor

hwwhww commented Aug 3, 2023

In the case of having if epoch > activation_epoch check (#3466 (comment)):

If needed, we can move the #3431 change to deneb/fork-choice.md to reflect the if epoch > activation_epoch check. So that the spec test vectors could match the client implementations. However, IMO, once Deneb is done, it's cleaner to have a post-deneb clean-up to get rid of the checks, i.e., revert it to the status-quo #3421 change in phase0/fork-choice.md. No fork is required for the clean-up because it only changes the older versions. Although it probably makes it more confusing. 😅

@hwwhww hwwhww added the Deneb was called: eip-4844 label Aug 4, 2023
@mkalinin
Copy link
Collaborator Author

From the recap of ACDC#115:

  • when/how to release fork-choice filtering change
    • rough consensus to go with not require activation epoch
    • will bundle this with deneb release
    • blocked by deneb fork epoch, once we have that, we can merge the PR

etan-status added a commit to status-im/nimbus-eth2 that referenced this issue Sep 4, 2023
To allow testing ethereum/consensus-specs#3466
add support for selecting fork choice version at launch. This means we
can deploy a different logic when `DENEB_FORK_EPOCH != FAR_FUTURE_EPOCH`
that won't be used on Mainnet.
etan-status added a commit to status-im/nimbus-eth2 that referenced this issue Sep 12, 2023
To allow testing ethereum/consensus-specs#3466
add support for selecting fork choice version at launch. This means we
can deploy a different logic when `DENEB_FORK_EPOCH != FAR_FUTURE_EPOCH`
that won't be used on Mainnet.
etan-status added a commit to status-im/nimbus-eth2 that referenced this issue 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 issue 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 issue 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)
@mkalinin
Copy link
Collaborator Author

Closing as the proposed change is included into a Deneb spec release: https://github.com/ethereum/consensus-specs/releases/tag/v1.4.0-beta.6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deneb was called: eip-4844
Projects
None yet
Development

No branches or pull requests

5 participants
@mkalinin @paulhauner @hwwhww @dapplion and others