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: deprecate exchangeTransitionConfiguration #339

Closed
wants to merge 1 commit into from

Conversation

mkalinin
Copy link
Collaborator

An alternative to #320. This PR deprecates engine_exchangeTransitionConfiguration by marking the method as deprecated rather than making a change into its specification.

There are two ways to deprecate the call on Mainnet:

  1. As a part of Capella/Shanghai deployment process. Shanghai compliant EL clients don't expect this call to be made (may be entirely removed) and Capella compliant CL clients stop making this call. There could be a hiccup in this process when a user upgrades one layer but leave the other one not upgraded yet and start seeing worrisome messages in logs.
  2. In an uncoordinated fashion. CL keeps calling this method but adds a check that this method does exist, and if EL responds with this method doesn't exist message CL doesn't make any noise in logs. This allows for EL to remove the method support, and once every EL client removes it then CL is free to do the same.

The first option looks simpler to me but I am fine either way.

@lightclient
Copy link
Member

Approach 1) makes sense to me. Should "deprecated" have a definition somewhere in the spec? Does this mean that clients can officially remove them? Do we want to require clients still support it for one additional fork? Do we need to change the spec for the method at all (e.g. make a similar change as #320 so that ELs don't continue complaining)?

@mkalinin
Copy link
Collaborator Author

Approach 1) makes sense to me.

I have just noticed that the point of synchronization when CL and EL switches their behaviour would be processing the first Capella block. I don't know how complicated it is engineering-wise to link block processing to exchangeTransitionConfiguration switch off, and if it is non-trivial for some clients then we should rather use approach 2) to avoid delaying Capella because of that.

Should "deprecated" have a definition somewhere in the spec?

Good point. I thought it should, we can have a Method deprecation section in the common.md.

Does this mean that clients can officially remove them?

Yes, it does. And this is the intention, it should also be reflected in the section I've mentioned above. And if we take Approach 2) then we can't deprecate method until CL clients are ready for this.

Do we need to change the spec for the method at all (e.g. make a similar change as #320 so that ELs don't continue complaining)?

Good point. As method deprecation in the spec doesn't mean it is actually removed from EL clients with all the logic relying on it. I would not do any changes to the finalized part of the spec, we can probably add a notice to the shanghai.md about deprecation of this method and what actions EL and CL clients should take.

@djrtwo
Copy link
Contributor

djrtwo commented Jan 11, 2023

Do teams rely on this as a ping to see if their connection is alive? (as opposed to just not receiving blocks) and surface errors accordingly?

I'd like to sanity check that before deprecating this method

@mkalinin
Copy link
Collaborator Author

I also wonder if infrastructure providers would like to have a ping functionality in replacement to this method? cc @ryanschneider

@arnetheduck
Copy link
Contributor

I also wonder if infrastructure providers would like to have a ping functionality in replacement to this method

why would they be implementing the execution api?

@mkalinin
Copy link
Collaborator Author

I also wonder if infrastructure providers would like to have a ping functionality in replacement to this method

why would they be implementing the execution api?

They would not be implementing it. But AFAIK they have used this particular method call to check whether their EL clients are Merge ready with properly configured and available Engine API endpoint. Checking the latter might still be useful, thought, we have a bunch of eth_ methods exposed at the same endpoint which might be leveraged.

@ryanschneider
Copy link

ryanschneider commented Jan 19, 2023 via email

@arnetheduck
Copy link
Contributor

Do teams rely on this as a ping to see if their connection is alive?

not really - it's just annoying (we get to know whether the connection is alive for every fcu/payload)

@mkalinin
Copy link
Collaborator Author

mkalinin commented Feb 3, 2023

closing in favour of #375

@mkalinin mkalinin closed this Feb 3, 2023
@mkalinin mkalinin deleted the mkalinin-patch-1 branch September 14, 2023 05:32
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.

5 participants