From 78429f7733812395ba8f704e728a5c1e8380c934 Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Tue, 7 Mar 2023 16:30:04 +0100 Subject: [PATCH] beacon/engine: don't omit empty withdrawals in ExecutionPayloadBodies (#26698) This ensures the "withdrawals" field will always be present in responses to getPayloadBodiesByRangeV1 and getPayloadBodiesByHashV1. --------- Co-authored-by: Felix Lange --- beacon/engine/types.go | 2 +- eth/catalyst/api_test.go | 108 +++++++++++++++++++++++++++------------ 2 files changed, 77 insertions(+), 33 deletions(-) diff --git a/beacon/engine/types.go b/beacon/engine/types.go index 58f726311..07ebe544b 100644 --- a/beacon/engine/types.go +++ b/beacon/engine/types.go @@ -233,5 +233,5 @@ func BlockToExecutableData(block *types.Block, fees *big.Int) *ExecutionPayloadE // ExecutionPayloadBodyV1 is used in the response to GetPayloadBodiesByHashV1 and GetPayloadBodiesByRangeV1 type ExecutionPayloadBodyV1 struct { TransactionData []hexutil.Bytes `json:"transactions"` - Withdrawals []*types.Withdrawal `json:"withdrawals,omitempty"` + Withdrawals []*types.Withdrawal `json:"withdrawals"` } diff --git a/eth/catalyst/api_test.go b/eth/catalyst/api_test.go index 099726917..fb6e6935e 100644 --- a/eth/catalyst/api_test.go +++ b/eth/catalyst/api_test.go @@ -19,8 +19,11 @@ package catalyst import ( "bytes" "context" + crand "crypto/rand" "fmt" "math/big" + "math/rand" + "reflect" "sync" "testing" "time" @@ -41,7 +44,6 @@ import ( "github.com/ethereum/go-ethereum/node" "github.com/ethereum/go-ethereum/p2p" "github.com/ethereum/go-ethereum/params" - "github.com/ethereum/go-ethereum/rlp" "github.com/ethereum/go-ethereum/rpc" "github.com/ethereum/go-ethereum/trie" ) @@ -473,18 +475,21 @@ func TestFullAPI(t *testing.T) { ethservice.TxPool().AddLocal(tx) } - setupBlocks(t, ethservice, 10, parent, callback) + setupBlocks(t, ethservice, 10, parent, callback, nil) } -func setupBlocks(t *testing.T, ethservice *eth.Ethereum, n int, parent *types.Header, callback func(parent *types.Header)) []*types.Header { +func setupBlocks(t *testing.T, ethservice *eth.Ethereum, n int, parent *types.Header, callback func(parent *types.Header), withdrawals [][]*types.Withdrawal) []*types.Header { api := NewConsensusAPI(ethservice) var blocks []*types.Header for i := 0; i < n; i++ { callback(parent) + var w []*types.Withdrawal + if withdrawals != nil { + w = withdrawals[i] + } - payload := getNewPayload(t, api, parent) - - execResp, err := api.NewPayloadV1(*payload) + payload := getNewPayload(t, api, parent, w) + execResp, err := api.NewPayloadV2(*payload) if err != nil { t.Fatalf("can't execute payload: %v", err) } @@ -676,10 +681,10 @@ func TestEmptyBlocks(t *testing.T) { api := NewConsensusAPI(ethservice) // Setup 10 blocks on the canonical chain - setupBlocks(t, ethservice, 10, commonAncestor, func(parent *types.Header) {}) + setupBlocks(t, ethservice, 10, commonAncestor, func(parent *types.Header) {}, nil) // (1) check LatestValidHash by sending a normal payload (P1'') - payload := getNewPayload(t, api, commonAncestor) + payload := getNewPayload(t, api, commonAncestor, nil) status, err := api.NewPayloadV1(*payload) if err != nil { @@ -693,7 +698,7 @@ func TestEmptyBlocks(t *testing.T) { } // (2) Now send P1' which is invalid - payload = getNewPayload(t, api, commonAncestor) + payload = getNewPayload(t, api, commonAncestor, nil) payload.GasUsed += 1 payload = setBlockhash(payload) // Now latestValidHash should be the common ancestor @@ -711,7 +716,7 @@ func TestEmptyBlocks(t *testing.T) { } // (3) Now send a payload with unknown parent - payload = getNewPayload(t, api, commonAncestor) + payload = getNewPayload(t, api, commonAncestor, nil) payload.ParentHash = common.Hash{1} payload = setBlockhash(payload) // Now latestValidHash should be the common ancestor @@ -727,11 +732,12 @@ func TestEmptyBlocks(t *testing.T) { } } -func getNewPayload(t *testing.T, api *ConsensusAPI, parent *types.Header) *engine.ExecutableData { +func getNewPayload(t *testing.T, api *ConsensusAPI, parent *types.Header, withdrawals []*types.Withdrawal) *engine.ExecutableData { params := engine.PayloadAttributes{ Timestamp: parent.Time + 1, Random: crypto.Keccak256Hash([]byte{byte(1)}), SuggestedFeeRecipient: parent.Coinbase, + Withdrawals: withdrawals, } payload, err := assembleBlock(api, parent.Hash(), ¶ms) @@ -799,7 +805,7 @@ func TestTrickRemoteBlockCache(t *testing.T) { commonAncestor := ethserviceA.BlockChain().CurrentBlock() // Setup 10 blocks on the canonical chain - setupBlocks(t, ethserviceA, 10, commonAncestor, func(parent *types.Header) {}) + setupBlocks(t, ethserviceA, 10, commonAncestor, func(parent *types.Header) {}, nil) commonAncestor = ethserviceA.BlockChain().CurrentBlock() var invalidChain []*engine.ExecutableData @@ -808,7 +814,7 @@ func TestTrickRemoteBlockCache(t *testing.T) { //invalidChain = append(invalidChain, payload1) // create an invalid payload2 (P2) - payload2 := getNewPayload(t, apiA, commonAncestor) + payload2 := getNewPayload(t, apiA, commonAncestor, nil) //payload2.ParentHash = payload1.BlockHash payload2.GasUsed += 1 payload2 = setBlockhash(payload2) @@ -817,7 +823,7 @@ func TestTrickRemoteBlockCache(t *testing.T) { head := payload2 // create some valid payloads on top for i := 0; i < 10; i++ { - payload := getNewPayload(t, apiA, commonAncestor) + payload := getNewPayload(t, apiA, commonAncestor, nil) payload.ParentHash = head.BlockHash payload = setBlockhash(payload) invalidChain = append(invalidChain, payload) @@ -855,10 +861,10 @@ func TestInvalidBloom(t *testing.T) { api := NewConsensusAPI(ethservice) // Setup 10 blocks on the canonical chain - setupBlocks(t, ethservice, 10, commonAncestor, func(parent *types.Header) {}) + setupBlocks(t, ethservice, 10, commonAncestor, func(parent *types.Header) {}, nil) // (1) check LatestValidHash by sending a normal payload (P1'') - payload := getNewPayload(t, api, commonAncestor) + payload := getNewPayload(t, api, commonAncestor, nil) payload.LogsBloom = append(payload.LogsBloom, byte(1)) status, err := api.NewPayloadV1(*payload) if err != nil { @@ -1233,8 +1239,10 @@ func TestNilWithdrawals(t *testing.T) { } func setupBodies(t *testing.T) (*node.Node, *eth.Ethereum, []*types.Block) { - genesis, preMergeBlocks := generateMergeChain(10, false) - n, ethservice := startEthService(t, genesis, preMergeBlocks) + genesis, blocks := generateMergeChain(10, true) + n, ethservice := startEthService(t, genesis, blocks) + // enable shanghai on the last block + ethservice.BlockChain().Config().ShanghaiTime = &blocks[len(blocks)-1].Header().Time var ( parent = ethservice.BlockChain().CurrentBlock() @@ -1249,12 +1257,38 @@ func setupBodies(t *testing.T) (*node.Node, *eth.Ethereum, []*types.Block) { ethservice.TxPool().AddLocal(tx) } - postMergeHeaders := setupBlocks(t, ethservice, 10, parent, callback) - postMergeBlocks := make([]*types.Block, len(postMergeHeaders)) - for i, header := range postMergeHeaders { - postMergeBlocks[i] = ethservice.BlockChain().GetBlock(header.Hash(), header.Number.Uint64()) + withdrawals := make([][]*types.Withdrawal, 10) + withdrawals[0] = nil // should be filtered out by miner + withdrawals[1] = make([]*types.Withdrawal, 0) + for i := 2; i < len(withdrawals); i++ { + addr := make([]byte, 20) + crand.Read(addr) + withdrawals[i] = []*types.Withdrawal{ + {Index: rand.Uint64(), Validator: rand.Uint64(), Amount: rand.Uint64(), Address: common.BytesToAddress(addr)}, + } } - return n, ethservice, append(preMergeBlocks, postMergeBlocks...) + + postShanghaiHeaders := setupBlocks(t, ethservice, 10, parent, callback, withdrawals) + postShanghaiBlocks := make([]*types.Block, len(postShanghaiHeaders)) + for i, header := range postShanghaiHeaders { + postShanghaiBlocks[i] = ethservice.BlockChain().GetBlock(header.Hash(), header.Number.Uint64()) + } + return n, ethservice, append(blocks, postShanghaiBlocks...) +} + +func allHashes(blocks []*types.Block) []common.Hash { + var hashes []common.Hash + for _, b := range blocks { + hashes = append(hashes, b.Hash()) + } + return hashes +} +func allBodies(blocks []*types.Block) []*types.Body { + var bodies []*types.Body + for _, b := range blocks { + bodies = append(bodies, b.Body()) + } + return bodies } func TestGetBlockBodiesByHash(t *testing.T) { @@ -1296,6 +1330,11 @@ func TestGetBlockBodiesByHash(t *testing.T) { results: []*types.Body{blocks[0].Body(), nil, blocks[0].Body(), blocks[0].Body()}, hashes: []common.Hash{blocks[0].Hash(), {1, 2}, blocks[0].Hash(), blocks[0].Hash()}, }, + // all blocks + { + results: allBodies(blocks), + hashes: allHashes(blocks), + }, } for k, test := range tests { @@ -1364,6 +1403,12 @@ func TestGetBlockBodiesByRange(t *testing.T) { start: 22, count: 2, }, + // allBlocks + { + results: allBodies(blocks), + start: 1, + count: hexutil.Uint64(len(blocks)), + }, } for k, test := range tests { @@ -1434,15 +1479,14 @@ func equalBody(a *types.Body, b *engine.ExecutionPayloadBodyV1) bool { } else if a == nil || b == nil { return false } - var want []hexutil.Bytes - for _, tx := range a.Transactions { - data, _ := tx.MarshalBinary() - want = append(want, hexutil.Bytes(data)) - } - aBytes, errA := rlp.EncodeToBytes(want) - bBytes, errB := rlp.EncodeToBytes(b.TransactionData) - if errA != errB { + if len(a.Transactions) != len(b.TransactionData) { return false } - return bytes.Equal(aBytes, bBytes) + for i, tx := range a.Transactions { + data, _ := tx.MarshalBinary() + if !bytes.Equal(data, b.TransactionData[i]) { + return false + } + } + return reflect.DeepEqual(a.Withdrawals, b.Withdrawals) }