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

all: implement flat deposit requests encoding #30425

Merged
merged 32 commits into from
Oct 9, 2024

Conversation

// ExecutionPayloadBody is used in the response to GetPayloadBodiesByHash and GetPayloadBodiesByRange
type ExecutionPayloadBody struct {
TransactionData []hexutil.Bytes `json:"transactions"`
Withdrawals []*types.Withdrawal `json:"withdrawals"`
Deposits types.Deposits `json:"depositRequests"`
Requests []hexutil.Bytes `json:"requests"`
Copy link
Member

Choose a reason for hiding this comment

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

This is going to be a problem, no? If we no longer have the requests in the block I don't think we'll be able to return them from GetPayloadBodiesByXXX ?

@@ -473,11 +472,12 @@ func (g *Genesis) toBlockWithRoot(root common.Hash) *types.Block {
}
}
if conf.IsPrague(num, g.Timestamp) {
head.RequestsHash = &types.EmptyRequestsHash
requests = make(types.Requests, 0)
emptyRequests := [][]byte{{0}}
Copy link
Member

Choose a reason for hiding this comment

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

kinda weird to just have the deposit request prefix just as a bare 0 value here instead of named?

eth/catalyst/api.go Outdated Show resolved Hide resolved
eth/catalyst/api.go Outdated Show resolved Hide resolved
@@ -474,6 +457,28 @@ func CalcUncleHash(uncles []*Header) common.Hash {
return rlpHash(uncles)
}

// CalcRequestsHash creates the block requestsHash value for a list of requests.
func CalcRequestsHash(requests [][]byte) common.Hash {
Copy link
Contributor

@islishude islishude Oct 8, 2024

Choose a reason for hiding this comment

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

It can be rewrote to following to eliminate memory allocation

func CalcRequestsHash(requests [][]byte) common.Hash {
	h1, h2 := sha256.New(), sha256.New()
	var buf common.Hash
	for _, item := range requests {
		h1.Reset()
		h1.Write(item)
		h2.Write(h1.Sum(buf[:0]))
	}
	h2.Sum(buf[:0])
	return buf
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a very clever suggestion!

core/chain_makers.go Outdated Show resolved Hide resolved
@fjl fjl merged commit 2936b41 into ethereum:master Oct 9, 2024
2 of 3 checks passed
@fjl fjl added this to the 1.14.12 milestone Oct 9, 2024
@@ -224,22 +224,21 @@ type BlockBody struct {
Transactions []*types.Transaction // Transactions contained within a block
Uncles []*types.Header // Uncles contained within a block
Withdrawals []*types.Withdrawal `rlp:"optional"` // Withdrawals contained within a block
Requests []*types.Request `rlp:"optional"` // Requests contained within a block
Requests [][]byte `rlp:"optional"` // Requests contained within a block
Copy link
Contributor

Choose a reason for hiding this comment

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

I just made a PR to remove it

#30562

fjl pushed a commit that referenced this pull request Oct 10, 2024
Block no longer has Requests. This PR just removes some code that wasn't removed in #30425.
holiman pushed a commit that referenced this pull request Nov 19, 2024
This implements recent changes to EIP-7685, EIP-6110, and
execution-apis.

---------

Co-authored-by: lightclient <[email protected]>
Co-authored-by: Shude Li <[email protected]>
holiman pushed a commit that referenced this pull request Nov 19, 2024
Block no longer has Requests. This PR just removes some code that wasn't removed in #30425.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants