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

Engine API: Refine message ordering. Take 2 #148

Merged
merged 1 commit into from Dec 17, 2021

Conversation

mkalinin
Copy link
Collaborator

Removes Message ordering mechanism based on request IDs. JSON-RPC request IDs might be difficult to get parsed depending on the JSON-RPC library. The new proposal doesn't rely on request IDs at all and removes the logic that is required to reset this ID, which in its turn introduces inconvenience for setups with multiple EL clients behind a load balancer.

The proposed approach doesn't require CL client software to send forkchoiceUpdated calls in a lock-step fashion. But it's based on the assumption that the underlying protocol doesn't mess up with the order of messages, i.e. if CL sends the following sequence of calls: [fcU(A), fcU(B)] then EL client will always receive fcU(A) before fcU(B). This assumption gets broken if HTTP over multiple TCP sessions is used as the underlying communication protocol.

@ajsutton
Copy link

This makes sense to me. Personally I think a CL would be incredibly brave to depend on http pipelining to preserve this order and probably should ensure it only has one in-flight message at a time, but if you control your underlying http library carefully enough it is possible to use pipelining so makes sense for the spec to allow it.

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.

reasonable approach. keep it simple!

Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

None yet

4 participants