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

State that payloadId should be unique for each PayloadAttributes instance #401

Merged
merged 5 commits into from
Apr 21, 2023

Conversation

mkalinin
Copy link
Collaborator

Adds a statement that payloadId value returned EL client software should be different for two distinct PayloadAttributes objects

src/engine/paris.md Outdated Show resolved Hide resolved
Copy link

@siladu siladu left a comment

Choose a reason for hiding this comment

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

LGTM

@mkalinin
Copy link
Collaborator Author

I have changed the original statement it was missing the new build process to be set in motion every time when new PayloadAttributes instance is passed which is basically the essence of this fix, i.e. new build process firstly and distinct payloadId identifying the new process secondly.

@@ -136,6 +136,8 @@ The payload build process is specified as follows:

4. Client software **SHOULD** stop the updating process when either a call to `engine_getPayload` with the build process's `payloadId` is made or [`SECONDS_PER_SLOT`](https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/beacon-chain.md#time-parameters-1) (12s in the Mainnet configuration) have passed since the point in time identified by the `timestamp` parameter.

5. Client software **MUST** initiate new payload build process for every distinct `PayloadAttributes` instance. A `payloadId` value **MUST** uniquely identify every new build process.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does distinct mean shallow or deep equality here? I.e. does the payload ID need to be different when the payload attributes match another set of payload attributes fully, but from a different RPC request?

We previously had "hashing to payload ID", but that was removed here: 37c8852

And at one point I think it was also defined as a randomized ID.

With each update that changes the payload attributes definition, the hashing to ID makes things quite difficult: the hashing has to be updated to avoid simple collisions between the new attributes (same basic request, but different additional attribute would hash to the same payload ID if not updated). Customizing payload attributes for EIPs / extensions also results in inconsistent hashing.

And when a collision is hit, the 2nd payload attributes attempt with different unhashed attribute cannot happen because of the first attempt. With randomized ID this is very rare, but with broken hashing it can happen. Some kind of unique id / incremental ID without collisions would be even better.

So I'm in favor of an ID not hard-wired to the attributes contents, and unique per build-process instantiating RPC request.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does distinct mean shallow or deep equality here?

Whenever new payload attributes differ to the previous ones in a set of fields or field values, I suppose it is deep equality. And I can see potential confusion because of the "instance" word in the statement, probably reword to:

Suggested change
5. Client software **MUST** initiate new payload build process for every distinct `PayloadAttributes` instance. A `payloadId` value **MUST** uniquely identify every new build process.
5. Client software **MUST** initiate new payload build process for every new `PayloadAttributes` object, i.e. whenever a set of fields or field values of the new object is different to the previously sent one. A `payloadId` value **MUST** uniquely identify every new build process.

Some kind of unique id / incremental ID without collisions would be even better.

I entirely agree with that. So, the ID must be different for two different build processes and ID value shouldn't be derived from payload attributes.

unique per build-process instantiating RPC request

Does it mean that every time the call to fcU with the same attributes will be responded with new payloadId while the build process remains untouched? So, there are multiple payloadId identifying the same build process. Why would this be helpful?

Copy link

Choose a reason for hiding this comment

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

Agree that random UID would mean easier maintenance and less scope for bugs.

There are cases where the CL replays the FCU so using a random id each time would prevent ELs from ignoring these replays, trigger the EL to start a new block building activity and worst case may cause late/missed proposals.

I think @potuz mentioned why they sometimes replay the FCU in a recent discussion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I re-phrased the original statement, please, take a look

@mkalinin
Copy link
Collaborator Author

From a discussion in discord#consensus-dev: CL sometimes may replay fcU with the same payload attributes, so, the recommendation for EL to not rewind the build process is added by this PR.

In a conversation with @protolambda and in discord#interop we concluded that using a prefix of hash function output to compute payloadId is fine as long as the hash function is strong enough. As it would give us a 2^-32 probability of a clash within one or couple of adjacent slots which is perfectly fine considering trusted communication channel between EL and CL (there seem to be no worthwhile way to exploit this potential attack vector).

@mkalinin mkalinin merged commit 3c1dae6 into main Apr 21, 2023
@mkalinin mkalinin deleted the mkalinin-patch-7 branch September 14, 2023 05:33
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.

4 participants