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

beacon/engine: strip type byte in requests #30576

Merged
merged 2 commits into from
Oct 14, 2024
Merged

beacon/engine: strip type byte in requests #30576

merged 2 commits into from
Oct 14, 2024

Conversation

fjl
Copy link
Contributor

@fjl fjl commented Oct 11, 2024

This change brings geth into compliance with the current engine API specification for the Prague fork. I have moved the assignment of ExecutionPayloadEnvelope.Requests into BlockToExecutableData to ensure there is a single place where the type is removed.

While doing so, I noticed that handling of requests in the miner was not quite correct for the empty payload. It would return nil requests for the empty payload even for blocks after the Prague fork. To fix this, I have added the emptyRequests field in miner.Payload.

@fjl
beacon/engine: strip type byte in requests
dbfbf5e 
This change brings geth into compliance with the current engine API specification for the
Prague fork. I have moved the assignment of ExecutionPayloadEnvelope.Requests into
BlockToExecutableData to ensure there is a single place where the type is removed.

While doing so, I noticed that handling of requests in the miner was not quite correct for
the empty payload. It would return `nil` requests for the empty payload even for blocks
after the Prague fork. To fix this, I have added the emptyRequests field in miner.Payload.
@fjl fjl added the prague label Oct 11, 2024
@fjl fjl added this to the 1.14.12 milestone Oct 11, 2024
@islishude
Copy link
Contributor

I don't see benefit to strip the type prefix.

if there is no needs to have the type prefix in the consensus clients, and they're only for the requestsHash calculation in the execution clients.

why do we need to generate the data with the request type prefix?

we can simply add the request type in the CalcRequestsHash func.

func CalcRequestsHash(requests [][]byte) common.Hash {
	h1, h2 := sha256.New(), sha256.New()
	var buf common.Hash
	for idx, item := range requests {
		h1.Reset()
                 h1.Write([]byte{byte(idx)}) // Add the request type here.
		h1.Write(item)
		h2.Write(h1.Sum(buf[:0]))
	}
	h2.Sum(buf[:0])
	return buf
}

lightclient
lightclient reviewed Oct 14, 2024
View reviewed changes
miner/payload_building.go
full *types.Block
fullWitness *stateless.Witness
sidecars []*types.BlobTxSidecar
emptyRequests [][]byte
Copy link
Member

@lightclient lightclient Oct 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This "emptyXXX" pattern is kind of weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We keep two blocks here, the 'empty' one without transactions, and one with.

The emptyRequests are needed because there has to be an item for each request type even when there are no requests.

@fjl fjl merged commit add5709 into ethereum:master Oct 14, 2024
3 checks passed
@lightclient lightclient mentioned this pull request Oct 16, 2024
Merged
fjl pushed a commit that referenced this pull request Oct 17, 2024
@lightclient
miner: send full request when resolving full payload (#30615)
1da34a3 
Fixes an issue missed in #30576 where we send empty requests for a full
payload being resolved, causing hash mismatch later on when we get the
payload back via `NewPayload`.
holiman pushed a commit that referenced this pull request Nov 19, 2024
@fjl @holiman
beacon/engine: strip type byte in requests (#30576)
b755cec 
This change brings geth into compliance with the current engine API
specification for the Prague fork. I have moved the assignment of
ExecutionPayloadEnvelope.Requests into BlockToExecutableData to ensure
there is a single place where the type is removed.

While doing so, I noticed that handling of requests in the miner was not
quite correct for the empty payload. It would return `nil` requests for
the empty payload even for blocks after the Prague fork. To fix this, I
have added the emptyRequests field in miner.Payload.
holiman pushed a commit that referenced this pull request Nov 19, 2024
@lightclient @holiman
miner: send full request when resolving full payload (#30615)
5d41ac5 
Fixes an issue missed in #30576 where we send empty requests for a full
payload being resolved, causing hash mismatch later on when we get the
payload back via `NewPayload`.
zfy0701 pushed a commit to sentioxyz/go-ethereum that referenced this pull request Dec 3, 2024
@fjl @zfy0701
beacon/engine: strip type byte in requests (ethereum#30576)
1227e3f 
This change brings geth into compliance with the current engine API
specification for the Prague fork. I have moved the assignment of
ExecutionPayloadEnvelope.Requests into BlockToExecutableData to ensure
there is a single place where the type is removed.

While doing so, I noticed that handling of requests in the miner was not
quite correct for the empty payload. It would return `nil` requests for
the empty payload even for blocks after the Prague fork. To fix this, I
have added the emptyRequests field in miner.Payload.
zfy0701 pushed a commit to sentioxyz/go-ethereum that referenced this pull request Dec 3, 2024
@lightclient @zfy0701
miner: send full request when resolving full payload (ethereum#30615)
d4ff223 
Fixes an issue missed in ethereum#30576 where we send empty requests for a full
payload being resolved, causing hash mismatch later on when we get the
payload back via `NewPayload`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@islishude islishude islishude left review comments

@lightclient lightclient lightclient approved these changes

@gballet gballet Awaiting requested review from gballet

Assignees
No one assigned
Labels
prague
Projects
None yet
Milestone
1.14.12
Development

Successfully merging this pull request may close these issues.
3 participants
@fjl @islishude @lightclient