all: implement push withdrawals (EIP-4895) #25838

lightclient · 2022-09-21T09:51:46Z

This PR implements EIP-4895: Beacon chain push withdrawals as operations. WIP.

lightclient · 2022-09-27T21:46:15Z

consensus/consensus.go

@@ -90,15 +90,15 @@ type Engine interface {
 	// Note: The block header and state database might be updated to reflect any
 	// consensus rules that happen at finalization (e.g. block rewards).
 	Finalize(chain ChainHeaderReader, header *types.Header, state *state.StateDB, txs []*types.Transaction,
-		uncles []*types.Header)
+		uncles []*types.Header, withdrawals []*types.Withdrawal)


Curious to know if there are better approaches than changing this interface.

lightclient · 2022-09-27T21:50:32Z

core/beacon/types.go

+	SuggestedFeeRecipient common.Address `json:"suggestedFeeRecipient" gencodec:"required"`
+}
+
+func (a *PayloadAttributesV1) ToV2() *PayloadAttributesV2 {


Please share any suggestions for improving this code. I thought about two other approaches:

Interface -- I could define an interface for these types which is the sum of all the versions. The downside is I believe I will need to then manually define the JSON encoding for each and implement a bunch of simple getters.

Single type -- I could extend the original type (and remove the V1) and then guard invalid combinations in the call sites. The downside for this is then that we always need to be careful about guarding against invalid configurations. Which may not be a big issue.

I ended up trying 2) in f5c9b70. That reduced the overall diff by about 150 lines.

holiman

This change requires some new tests -- specifically blockchain tests with a new format on the block bodies.

consensus/beacon/consensus.go

holiman · 2022-10-20T11:07:48Z

core/beacon/types.go

+		BaseFee:        params.BaseFeePerGas,
+		Extra:          params.ExtraData,
+		MixDigest:      params.Random,
+		WithdrawalHash: withdrawalRoot,


Suggested change

WithdrawalHash: withdrawalRoot,

WithdrawalRoot: withdrawalRoot,

maybe ? I don't know ?

The reason for this name was to follow along the naming convention of TxHash and ReceiptHash (which fwiw, I think would be better if with s/Hash/Root).

holiman · 2022-10-20T11:09:06Z

core/types/block.go

+// The values of TxHash, UncleHash, ReceiptHash and Bloom in header
+// are ignored and set to values derived from the given txs, uncles
+// and receipts.
+func NewBlock2(header *Header, txs []*Transaction, uncles []*Header, receipts []*Receipt, withdrawals []*Withdrawal, hasher TrieHasher) *Block {


Ouch, please use a better name!

NewBlockWithWithdrawals?

holiman · 2022-10-20T11:10:01Z

core/types/block.go

@@ -378,6 +426,17 @@ func (b *Block) WithBody(transactions []*Transaction, uncles []*Header) *Block {
 	return block
 }

+// WithBody2 returns a new block with the given transaction, uncle, and
+// withdrawal contents.
+func (b *Block) WithBody2(transactions []*Transaction, uncles []*Header, withdrawals []*Withdrawal) *Block {


Ah my eyes!
Please use language.

cc @fjl - he recommend this naming convention.

core/types/withdrawal.go

core/beacon/types.go

MariusVanDerWijden

Mostly nitpicks

cmd/evm/internal/t8ntool/execution.go

MariusVanDerWijden · 2022-11-01T11:02:14Z

consensus/clique/clique.go

@@ -743,6 +743,9 @@ func encodeSigHeader(w io.Writer, header *types.Header) {
 	if header.BaseFee != nil {
 		enc = append(enc, header.BaseFee)
 	}
+	if header.WithdrawalHash != nil {


I don't understand this change, shouldn't we just not add the withdrawal hash here, even if the header has it set for some reason? Maybe we should even panic/warn

core/beacon/blockparams_marshaling.go

core/beacon/ed_marashaling.go

core/beacon/types.go

core/chain_makers.go

MariusVanDerWijden · 2022-11-01T11:22:37Z

core/types/block.go

+// The values of TxHash, UncleHash, ReceiptHash and Bloom in header
+// are ignored and set to values derived from the given txs, uncles
+// and receipts.
+func NewBlock2(header *Header, txs []*Transaction, uncles []*Header, receipts []*Receipt, withdrawals []*Withdrawal, hasher TrieHasher) *Block {


NewBlockWithWithdrawals?

MariusVanDerWijden · 2022-11-01T11:31:38Z

eth/catalyst/api.go

 	// Hash
 	hasher := sha256.New()
 	hasher.Write(headBlockHash[:])
 	binary.Write(hasher, binary.BigEndian, params.Timestamp)
 	hasher.Write(params.Random[:])
 	hasher.Write(params.SuggestedFeeRecipient[:])
+	if params.Withdrawals != nil {
+		// TODO: is it okay to throw away this error?


I think throwing away the error is fine, there's no spec around how the payload id should be generated anymore. It just needs to be unique per payload

It might be weird when cls start updating only the withdrawals within a slot with invalid withdrawals, but thats on them and we might just return a payload with 'old' withdrawals

The execution api specs define that this ID can be completely random. It used to be derived from payload attributes in the past, but maybe it's time to use a randomized ID to not break it with any changes to the attributes?

eth/fetcher/block_fetcher_test.go

marioevz · 2022-11-17T23:32:20Z

Found this on a different branch (withdrawals-timestamp) and leaving here as a reminder:
In params/config.go, ShanghaiTime field in ChainConfig is still parsed from the json field shanghaiBlock, but should eventually be changed to be parsed from shanghaiTime.

…ostic

Co-authored-by: lightclient <[email protected]> Co-authored-by: marioevz <[email protected]>

Inphi · 2022-12-02T19:14:44Z

Just a note that the withdrawalsRoot is missing from the JSON-RPC header response.

holiman · 2023-01-12T10:20:51Z

Closing in favour of #26484

lightclient force-pushed the withdrawals branch 2 times, most recently from 0cfa695 to 9c922f8 Compare September 23, 2022 12:27

holiman added the maybe shanghai label Sep 26, 2022

lightclient force-pushed the withdrawals branch from 35ad617 to b5351c6 Compare September 26, 2022 15:32

lightclient commented Sep 27, 2022

View reviewed changes

lightclient changed the title ~~core,eth: implement push withdrawals (EIP-4895)~~ all: implement push withdrawals (EIP-4895) Sep 27, 2022

lightclient force-pushed the withdrawals branch from f5c9b70 to 090bd9f Compare September 28, 2022 18:32

lightclient force-pushed the withdrawals branch 4 times, most recently from d9cdfe1 to 7dbeb21 Compare October 19, 2022 22:05

holiman reviewed Oct 20, 2022

View reviewed changes

lightclient force-pushed the withdrawals branch 4 times, most recently from eab8499 to 49fb63d Compare October 24, 2022 18:01

ralexstokes reviewed Oct 25, 2022

View reviewed changes

core/types/withdrawal.go Outdated Show resolved Hide resolved

lightclient force-pushed the withdrawals branch 9 times, most recently from 03bbe62 to cd6afba Compare October 28, 2022 15:30

roberto-bayardo mentioned this pull request Oct 31, 2022

core/types,core/beacon: add withdrawal object and helpers mdehoog/go-ethereum#42

Closed

roberto-bayardo reviewed Oct 31, 2022

View reviewed changes

core/beacon/types.go Outdated Show resolved Hide resolved

MariusVanDerWijden reviewed Nov 1, 2022

View reviewed changes

lightclient force-pushed the withdrawals branch 5 times, most recently from 0c0f751 to 9e66c16 Compare November 5, 2022 16:54

roberto-bayardo mentioned this pull request Nov 7, 2022

add header & block fields for Withdrawals mdehoog/go-ethereum#45

Merged

lightclient force-pushed the withdrawals branch from 9e66c16 to cc3d8da Compare November 7, 2022 23:35

holiman mentioned this pull request Nov 16, 2022

core, params: prototype withdrawal transactions #24468

Closed

3 tasks

roberto-bayardo mentioned this pull request Nov 29, 2022

copy/pasta more of the Withdrawals PR mdehoog/go-ethereum#65

Merged

lightclient force-pushed the withdrawals branch from 57966d1 to 08d2458 Compare November 30, 2022 01:14

lightclient and others added 13 commits November 29, 2022 18:21

core/types: add withdrawal type

774be0e

core/beacon: add withdrawals and make types version agnostic

c27745f

eth/catalyst,les/catalyst: add withdrawals and make types version agn…

994b761

…ostic

miner: add withdrawals and update catalyst types

a512496

consensus: process withdrawals

7d7fb2f

core/state_processor: finalize with withdrawals

58218c9

core/chain_makers: add withdrawal support

72ec036

core/block_validator: verify withdrawals root

2ebb038

core/rawdb: update block readers with new block format

a6e56c5

eth: add p2p support for withdrawals

a5853bb

cmd/evm: support withdrawals in t8n and b11r

9ad8070

eth/downloader: always use WithBody2 when constructing blocks

97c2474

Co-authored-by: lightclient <[email protected]> Co-authored-by: marioevz <[email protected]>

eth/fetcher: always use WithBody2 when constructing blocks

256e54c

lightclient force-pushed the withdrawals branch from 08d2458 to 256e54c Compare November 30, 2022 01:31

roberto-bayardo mentioned this pull request Nov 30, 2022

Implement EIP-4844 #26283

Closed

MariusVanDerWijden mentioned this pull request Jan 12, 2023

all: implement withdrawals (EIP-4895) #26484

Merged

holiman closed this Jan 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

all: implement push withdrawals (EIP-4895) #25838

all: implement push withdrawals (EIP-4895) #25838

lightclient commented Sep 21, 2022

lightclient Sep 27, 2022

lightclient Sep 27, 2022

lightclient Sep 28, 2022 •

edited

Loading

holiman left a comment

holiman Oct 20, 2022

lightclient Oct 20, 2022

holiman Oct 20, 2022

MariusVanDerWijden Nov 1, 2022

holiman Oct 20, 2022

lightclient Oct 20, 2022

MariusVanDerWijden left a comment

MariusVanDerWijden Nov 1, 2022

MariusVanDerWijden Nov 1, 2022

MariusVanDerWijden Nov 1, 2022

MariusVanDerWijden Nov 1, 2022

protolambda Nov 16, 2022

marioevz commented Nov 17, 2022

Inphi commented Dec 2, 2022

holiman commented Jan 12, 2023

	WithdrawalHash: withdrawalRoot,
	WithdrawalRoot: withdrawalRoot,

all: implement push withdrawals (EIP-4895) #25838

all: implement push withdrawals (EIP-4895) #25838

Conversation

lightclient commented Sep 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightclient Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

holiman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MariusVanDerWijden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marioevz commented Nov 17, 2022

Inphi commented Dec 2, 2022

holiman commented Jan 12, 2023

lightclient Sep 28, 2022 •

edited

Loading