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 EIP-6110, execution layer triggered deposits #29431

Merged
merged 4 commits into from
Sep 4, 2024

Conversation

lightclient
Copy link
Member

@lightclient lightclient commented Apr 1, 2024

This PR implements EIP-6110: Supply validator deposits on chain. It also sketches out the base for Prague in the engine API types. I plan to base future Prague EIP work against this PR.

@rjl493456442
Copy link
Member

We added some logics in downloader when we introduce withdrawal, i guess it's same for deposit?

if deposits == nil {
b.header.DepositsHash = nil
} else if len(deposits) == 0 {
b.header.DepositsHash = &EmptyWithdrawalsHash
Copy link
Member

Choose a reason for hiding this comment

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

EmptyDepositsHash

b = b.WithWithdrawals(withdrawals)

// TODO(matt): copy this
b.deposits = deposits
Copy link
Member

Choose a reason for hiding this comment

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

e.g. b.deposits = slices.Clone(deposits) ?

@MariusVanDerWijden
Copy link
Member

We need to allow prague in fcuV3 as well

@lightclient lightclient force-pushed the eip-6110 branch 8 times, most recently from e942406 to 8ea1e62 Compare May 14, 2024 14:51
@lightclient lightclient force-pushed the eip-6110 branch 2 times, most recently from e631d4c to fc447bb Compare May 23, 2024 08:23
Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

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

Added some review comments, the thing I dislike the most is that we need to use abigen, which panics btw

beacon/engine/types.go Show resolved Hide resolved
beacon/engine/types.go Outdated Show resolved Hide resolved
cmd/evm/internal/t8ntool/execution.go Outdated Show resolved Hide resolved
core/blockchain.go Outdated Show resolved Hide resolved
core/blockchain_test.go Outdated Show resolved Hide resolved
core/types/deposit.go Outdated Show resolved Hide resolved
core/types/deposit.go Outdated Show resolved Hide resolved
core/types/request.go Outdated Show resolved Hide resolved
core/types/request.go Show resolved Hide resolved
eth/catalyst/api.go Outdated Show resolved Hide resolved
@lightclient lightclient force-pushed the eip-6110 branch 3 times, most recently from f1e9ed7 to 517c2d9 Compare July 19, 2024 19:05
@MariusVanDerWijden
Copy link
Member

I like the handwritten decoder much better, I did a bunch of fuzzing of the types and nothing came up, but I haven't verified that the decoding is_actually_ correct

if header.RequestsHash != nil {
depositSha := types.DeriveSha(res.Requests, trie.NewStackTrie(nil))
if depositSha != *header.RequestsHash {
return fmt.Errorf("invalid deposit root hash (remote: %x local: %x)", *header.RequestsHash, depositSha)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps invalid request root?

Copy link
Member

Choose a reason for hiding this comment

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

and also please change the var name to requestSha

core/types/request.go Outdated Show resolved Hide resolved
@lightclient lightclient force-pushed the eip-6110 branch 2 times, most recently from a2df8b2 to 94ea9c8 Compare August 16, 2024 15:09
@islishude
Copy link
Contributor

I think there's a missing validation in consensus.verifyHeader

fyi

	prague := chain.Config().IsPrague(header.Number, header.Time)
	if !prague {
		if header.RequestsHash != nil {
			return fmt.Errorf("invalid requestsRoot: have %d, expected nil", header.RequestsHash)
		}
	} else {
		if header.RequestsHash == nil {
			return errors.New("header is missing requestsRoot")
		}
	}

@lightclient
Copy link
Member Author

@islishude that occurs in state_validator.go.

@islishude
Copy link
Contributor

that occurs in state_validator.go.

it doesn't have a validation for the chain fork rule

core/state_processor.go Outdated Show resolved Hide resolved
core/types.go Outdated Show resolved Hide resolved
@fjl fjl merged commit dfd33c7 into ethereum:master Sep 4, 2024
2 of 3 checks passed
@fjl fjl added this to the 1.14.9 milestone Sep 4, 2024
@islishude
Copy link
Contributor

Hi, there.

I found 2 bugs, please help review the pr #30422 and #30423

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.

6 participants