-
Notifications
You must be signed in to change notification settings - Fork 380
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
Engine API: add engine_exchangeTransitionConfigurationV1 #172
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! One minor question/suggestion
src/engine/specification.md
Outdated
#### Request | ||
|
||
* method: `engine_getTransitionConfigurationV1` | ||
* params: *empty list* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alternative is that it's more of a ping/pong where each send their configuration. Thus both can see a mismatch and both can surface an error.
Do you think this would help reduce chance of user failures by both logs having the error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to this idea, basically params
is the CL's configuration and response
is the EL's, and both sides SHOULD log a warning if the other configuration doesn't match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea! 👍 I think we may re-use the same data structure. When TERMINAL_BLOCK_HASH
is set CL client will send terminalBlockNumber: 0
as a block number is not a part of CL's configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although, what if it's called not by CL but by a script that checks every EL client in a cluster and then matches values with what are expected to be? Do we want to design for this use case? If yes, then we should make the parameter optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A cluster/advanced test could also add the config as a param. I think it's not worth the complxity of making this optional
I've updated the method to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Proposal
Proposes to add
engine_exchangeTransitionConfigurationV1
methodMotivation
This proposal introduces a method that is useful only before the merge transition with intention to deprecate it in a subsequent upgrade. Low implementation complexity should mitigate this downside.