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.

Sign up for GitHub

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

beacon/engine: don't omit empty withdrawals in ExecutionPayloadBodies #26698

Merged
merged 8 commits into from
Mar 7, 2023
Merged

beacon/engine: don't omit empty withdrawals in ExecutionPayloadBodies #26698

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0e1ecc7
eth/catalyst: set withdrawals in getPayloads test
MariusVanDerWijden Feb 15, 2023
b3490a7
test
MariusVanDerWijden Feb 15, 2023
52d1c85
test
MariusVanDerWijden Feb 15, 2023
9cc2a54
test
MariusVanDerWijden Feb 15, 2023
9450dbf
beacon/engine: marshall withdrawals even if nil
MariusVanDerWijden Feb 15, 2023
87af877
eth/catalyst: better equality
MariusVanDerWijden Mar 7, 2023
fc6ffdb
eth/catalyst: use bytes.Equal for []byte
fjl Mar 7, 2023
6501cec
eth/catalyst: resolve deprecation warning
fjl Mar 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion beacon/engine/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
}
108 changes: 76 additions & 32 deletions eth/catalyst/api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,11 @@ package catalyst
import (
"bytes"
"context"
crand "crypto/rand"
"fmt"
"math/big"
"math/rand"
"reflect"
"sync"
"testing"
"time"
Expand All @@ -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"
)
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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()
Expand All @@ -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)},
}
}

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(preMergeBlocks, postMergeBlocks...)
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) {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
}