remove preparePayload

[djrtwo]

As discussed at the amphora interop, we are combining engine_preparePayload functionality as an extension of engine_forkchoiceUpdated via an additional parameter. This avoids potential synchronicity issues that arise when the methods are decoupled.

Intuitively the call semantics become

• "update your fork choice" (no payload build requested)
• "update your fork choice and start building something valuable on top of it!" (payload build requested)

---

Changes enumerated below. I apologize that the diff isn't the easiest to parse:

• Move prepare payload functionality to forkchoiceUpdated:
  • Remove preparePayload
  • Define PayloadAttributes object (the former fields from preparePayload) for use as an optional parameter in forkchoiceUpdated
  • Port all preparePayload specs to a subset of forkchoiceUpdated specs
  • Move definition of getPayload to be below forkchoiceUpdated to aid in readability/natural sequentiality of calls
• Define payloadId procedurally based on input parameters (make bytes-8 instead of integer to avoid dealing with bytes to integer encoding). This greatly simplifies the requisite various return values for forkchoiceUpdated by not needing to return payloadId
• Clean up forkchoiceUpdated return values. Now status SUCCESS or SYNCING OR return an Error

[mkalinin]

Looks good to me! 👍 Added a couple of minor suggestions

[djrtwo]

Got a tentative 👍 from @mkalinin out of band.

Can @lightclient or @mkalinin give a final approval here?

[lightclient]

Doesn't appear the errors that can be returned are described?

[djrtwo]

Must we describe all errors? Anything that actually fails on the client during this call might bubble up an error. At interop, the general idea was to have return values not error under normal operation (thus SYNCING instead of "error: block missing") and for errors to actually represent errors in the operation.

Peter made it seem like if any number of things fail in geth, they'd surface that here.

[djrtwo]

You can have SUCCESS or you could be or enter into SYNCING. In the event that something fails outside of those two, you then send an error.

Can we define all such errors?

[lightclient]

Yeah that's fair. I think the small thing that's bothering me is that Object|Error when Error doesn't feel very well defined. I will take a stab fixing this myself after we merge this though!

[djrtwo]

I'm not sure we should restrict errors to only those specifically specified

See discussion points below

[djrtwo]

Must we describe all errors? Anything that actually fails on the client during this call might bubble up an error. At interop, the general idea was to have return values not error under normal operation (thus SYNCING instead of "error: block missing") and for errors to actually represent errors in the operation.

Peter made it seem like if any number of things fail in geth, they'd surface that here.

[djrtwo]

You can have SUCCESS or you could be or enter into SYNCING. In the event that something fails outside of those two, you then send an error.

Can we define all such errors?

[lightclient]

Fair points - I'm happy to merge this PR and improve the clarity of the error codes in a subsequent PR :)

[mkalinin]

What is SUCCESS status supposed to mean? Success fo receiving the call or updating the fork choice state? The former doesn't seem valuable as JSON-RPC would send a respond in any case indicating whether it's been successfully received or not. The latter implies the failure scenario, one of such scenarios is the absence of either finalized or head block coming in the method call; and if we have a success scenario signalled in the response then the failure should be signalled as well. Or we may rename SUCCESS to ACK to avoid confusion which won't be useful by itself but would allow for SYNCING to be returned as an alternative.