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

MUST use GENESIS_FORK_VERSION to sign BLSToExecutionChange message #3206

Merged
merged 5 commits into from
Jan 13, 2023

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Jan 12, 2023

As ethereum/pm#702 discussion, making BLSToExecutionChange fork-agnostic could make it easier to handle the message generation and validation.

Changes:

  1. MUST use GENESIS_FORK_VERSION to sign BLSToExecutionChange message.
    • Note: state.genesis_validators_root is still part of the source of the BLSToExecutionChange domain. (Deposit domain does NOT use state.genesis_validators_root as part of the source) .
  2. [Replace Use Capella fork version for BLSToExecution #3176]: Ignore signed_bls_to_execution_change message before CAPELLA_FORK_EPOCH

Considerations/discussions:

  • Replay protection: I'd argue that the forked chains may also use the same fork version pattern as Ethereum mainnet anyway. What we can do is encourage and educate users to send BLSToExecutionChange fast.

@mcdee
Copy link
Contributor

mcdee commented Jan 12, 2023

The spec change looks good, and understand the thinking behind it.

The p2p handling of these messages is tricky, though. I believe that @potuz talked about this during the call earlier today, but the handling of these messages prior to the capella hard fork activating is trickier. As it stands, there are the following two wrinkles in the system:

  • a user that submits a credentials change operation to a beacon node will find it accepted (by prysm, as per the call, not sure about other clients), but although it will nominally be broadcast to other nodes on the network, they will ignore it so the operation will not be propagated around the network
  • when a node that does contain a pool of change operations hits the capella hard fork it will (I assume) immediately broadcast the pending operations, however if the broadcasting node is slightly ahead of its peers they may still be on bellatrix and, as such, will ignore the operation

The end result of these two points is that there appears to be no way for publish credentials change operations early with the hope that they will be ahead of potential hackers who may have access to their keys. Would it be unreasonable to drop the IGNORE condition and expect beacon nodes to queue the operations when they see them prior to the hard fork? I recognize that this opens additional questions such as should the nodes store the operations on-disk so that they aren't lost on restart, but even if the queue is purely in-memory that would still give a better flow for the operations both before and during the hard fork.

@potuz
Copy link
Contributor

potuz commented Jan 12, 2023

I agree that the handling of the first broadcasting is a bit tricky. I think there is no good solution to the problem. However, my thinking is slightly orthogonal to Jim's so I'll leave it here briefly noted:

  1. Clients should accept local messages in their bls_to_execution_changes endpoint so that users can populate their pool at their earliest convenience.
  2. Clients should ignore (and not penalize peers) any such change that comes before a prescribed time/event. There's no good option here, so wall clock being at the Capella fork is as bad as any other moment.
  3. Clients should not broadcast messages earlier than the fork.

The reason for this thinking is that hackers and users would be on the same level, fighting for inclusions. Users that have access to exchanges with KYC or similar can try to get large pools/operators or existing projects to have their keys in their pools before hand. Those nodes will not rebroadcast any different change for the same keys, and being very large operators, they will be very well connected so in case of a race to broadcast to a lone validator, they have a better chance than a small hacker with a couple of beacons. Hackers do not gain anything by flooding the network with early messages. It's safe to assume that sophisticated hackers will be trying to get their messages as early as possible. If we start accepting messages at subscription we will see plenty of these multiple times since clients can subscribe at different points. In short, although I don't like any option, I think stock clients should agree to something like 1--3 above and let the hackers/users race for the off-chance that their stolen keys will be picked by a random small staker.

Copy link
Collaborator

@mkalinin mkalinin left a comment

Choose a reason for hiding this comment

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

LGTM!

@djrtwo
Copy link
Contributor

djrtwo commented Jan 13, 2023

The reason for this thinking is that hackers and users would be on the same level, fighting for inclusions.

I think that a hacker (especially if unknown to the victim) is likely to act faster than the unsuspecting victim. So I tend to agree with Potuz that it's best to have them on the same playing field and at the fork boundary as everyone will have a chance to touch their software by that boundary and not some arbitrary amount prior to the fork.

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.

nice!

@@ -61,6 +61,8 @@ This topic is used to propagate signed bls to execution change messages to be in

The following validations MUST pass before forwarding the `signed_bls_to_execution_change` on the network:

- _[IGNORE]_ `current_epoch >= CAPELLA_FORK_EPOCH`,
Copy link
Contributor

Choose a reason for hiding this comment

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

should we have the wall-clock skew variability here (MAXIMUM_GOSSIP_CLOCK_DISPARITY) as we do on some gossip conditions (e.g. beacon block)?

I guess that gives the hackers something to try to game more. but some of the initial broadcasts, otherwise, will be IGNORE'd by accident due to subtle clock skew

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

Successfully merging this pull request may close these issues.

7 participants