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 <fjl@twurst.com>

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"`
 }

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)
+		payload := getNewPayload(t, api, parent, w)
-
+		execResp, err := api.NewPayloadV2(*payload)
-		execResp, err := api.NewPayloadV1(*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(), &params)
@@ -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)
+	genesis, blocks := generateMergeChain(10, true)
-	n, ethservice := startEthService(t, genesis, preMergeBlocks)
+	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)
+	withdrawals := make([][]*types.Withdrawal, 10)
-	postMergeBlocks := make([]*types.Block, len(postMergeHeaders))
+	withdrawals[0] = nil // should be filtered out by miner
-	for i, header := range postMergeHeaders {
+	withdrawals[1] = make([]*types.Withdrawal, 0)
-		postMergeBlocks[i] = ethservice.BlockChain().GetBlock(header.Hash(), header.Number.Uint64())
+	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
+	if len(a.Transactions) != len(b.TransactionData) {
-	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 {
 		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)
 }