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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
Apr 21, 2023
Merged

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

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c1d5eaa
State payloadId must be unique per attributes
mkalinin Apr 11, 2023
bc47130
Update src/engine/paris.md
mkalinin Apr 12, 2023
f4664af
State new build process for every new attributes
mkalinin Apr 13, 2023
c60a297
Update statements about existing build process
mkalinin Apr 19, 2023
b4a3910
Fix grammar
mkalinin Apr 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/engine/paris.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


## Methods

### engine_newPayloadV1
Expand Down