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

Use Capella fork version for BLSToExecution #3176

Closed
wants to merge 3 commits into from
Closed

Conversation

mkalinin
Copy link
Collaborator

A stopgap fix for BLSToExecution gossip validation:

  • Use Capella fork version If a node is in the pre-Capella state according to a wall clock,
  • Otherwise, use a fork version that is relevant.

This fix only make sense if we can't afford a general fix of gossip messages validation because of additional engineering complexity. The general fix would be to always use a fork version from a gossip topic to compute signing domain across all topics.

@mkalinin mkalinin requested a review from djrtwo December 23, 2022 10:23
@dapplion
Copy link
Collaborator

I would apply the rule to use the gossip topic's fork version for BLSToExecution topics, even if it's not standard across all topics

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 okay with this but am not too worried about the UX issue if we don't patch. That said, if engineers are comfortable with this, it will ensure we don't get a bunch of dropped messages

@@ -59,11 +59,16 @@ See Capella [state transition document](./beacon-chain.md#beaconblockbody) for f

This topic is used to propagate signed bls to execution change messages to be included in future blocks.

Let `head_state` be a post state of the head of canonical chain.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just the chain or including any empty transitions to get to the current slot wrt wall time?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, I think we don't need the whole state in the definition of this change. I'll switch to system_clock_epoch (Thought this name for the variable would be less ambiguous than current_epoch)

@michaelsproul
Copy link
Contributor

I am in favour of this approach over the partial patch in #3135. I'm also open to the full cleanup of gossip topics, but think we could add that in a future fork to avoid delaying Capella. It doesn't seem like there would be any issue in spec or impl, but the devil is in the details and it would take time from the spec side and every team to roll it out.

@hwwhww
Copy link
Contributor

hwwhww commented Jan 12, 2023

Per the fork version validation, another solution is making SignedBLSToExecution signature fork-agnostic, i.e., use GENESIS_FORK_VERSION for all SignedBLSToExecution. Although that would make this operation not distinguishable when chain forks.

@mkalinin
Copy link
Collaborator Author

Closing in favour of #3206

@mkalinin mkalinin closed this Jan 13, 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.

5 participants