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

Optimistic rule for post-Bellatrix forks #3390

Open
zilm13 opened this issue May 28, 2023 · 3 comments
Open

Optimistic rule for post-Bellatrix forks #3390

zilm13 opened this issue May 28, 2023 · 3 comments

Comments

@zilm13
Copy link
Contributor

zilm13 commented May 28, 2023

Currently we have this check in https://github.com/ethereum/consensus-specs/blob/dev/sync/optimistic.md#helpers

def is_execution_block(block: BeaconBlock) -> bool:
    return block.body.execution_payload != ExecutionPayload()

It will fail for first slot (check on parent, which is genesis) in networks starting from post-Bellatrix forks, like Capella or Deneb. I think this rule should be updated to be true in any post-Bellatrix fork without any checks.

@hwwhww
Copy link
Contributor

hwwhww commented May 30, 2023

To clarify the fork difference in the presentation:

  1. A simple solution is to move sync/optimistic.md to specs/bellatrix and then add specs/capella/optimistic.md.
  2. An alternative solution is just to add a comment about it. Given that optimistic sync is not part of the executable pyspec so it's okay.

I prefer (1) here because it's more future-proof.

/cc @paulhauner What do you think?

@mkalinin
Copy link
Collaborator

Although, the execution is always enabled for forks post-Bellatrix, and I can understand why we'd want to change the spec, i am curious when this situation occurs on practice? If EL returns SYNCING in response to the first block which is a child of a genesis block it must be something weird in EL clients setup or an edge case. There can be an edge case when genesis block has several children. In this case some ELs can response ACCEPTED when they receive non-canonical child of a genesis block and if CL switches to such a block after that it should apply it optimistically and a node can get stuck at this point if it's not allowed to switch to an optimistic mode.

Alternative fix is to return True when the parent is genesis and current block has execution payload, i.e.

if block.slot == GENESIS_SLOT + 1 and is_execution_block(block):
    return True

@zilm13
Copy link
Contributor Author

zilm13 commented May 30, 2023

Though I like more first solution, it looks cleaner, what will happen if we have Bellatrix/Paris genesis start? It's a similar case probably and only @mkalinin's fix will pass.
I'm pretty unsure where it could happen outside the tests but looking at the spec like on a set of the formal rules, I think it's better to clarify this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants
@mkalinin @zilm13 @hwwhww and others