From 0e1ecc7c63680f25bb7c7b6a9034ebd215d13e2d Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Wed, 15 Feb 2023 10:59:54 +0100 Subject: [PATCH 1/8] eth/catalyst: set withdrawals in getPayloads test --- eth/catalyst/api_test.go | 91 ++++++++++++++++++++++++++++++++-------- 1 file changed, 74 insertions(+), 17 deletions(-) diff --git a/eth/catalyst/api_test.go b/eth/catalyst/api_test.go index 09972691729e..d6510541c816 100644 --- a/eth/catalyst/api_test.go +++ b/eth/catalyst/api_test.go @@ -21,6 +21,7 @@ import ( "context" "fmt" "math/big" + "math/rand" "sync" "testing" "time" @@ -473,18 +474,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 +680,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 +697,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 +715,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 +731,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 +804,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 +813,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 +822,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 +860,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 +1238,10 @@ func TestNilWithdrawals(t *testing.T) { } func setupBodies(t *testing.T) (*node.Node, *eth.Ethereum, []*types.Block) { - genesis, preMergeBlocks := generateMergeChain(10, false) + genesis, preMergeBlocks := generateMergeChain(10, true) n, ethservice := startEthService(t, genesis, preMergeBlocks) + // enable shanghai on the last block + ethservice.BlockChain().Config().ShanghaiTime = &preMergeBlocks[len(preMergeBlocks)-1].Header().Time var ( parent = ethservice.BlockChain().CurrentBlock() @@ -1249,7 +1256,18 @@ 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) + 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) + rand.Read(addr) + withdrawals[i] = []*types.Withdrawal{ + {Index: rand.Uint64(), Validator: rand.Uint64(), Amount: rand.Uint64(), Address: common.BytesToAddress(addr)}, + } + } + + postMergeHeaders := setupBlocks(t, ethservice, 10, parent, callback, withdrawals) postMergeBlocks := make([]*types.Block, len(postMergeHeaders)) for i, header := range postMergeHeaders { postMergeBlocks[i] = ethservice.BlockChain().GetBlock(header.Hash(), header.Number.Uint64()) @@ -1257,6 +1275,21 @@ func setupBodies(t *testing.T) (*node.Node, *eth.Ethereum, []*types.Block) { return n, ethservice, append(preMergeBlocks, postMergeBlocks...) } +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) { node, eth, blocks := setupBodies(t) api := NewConsensusAPI(eth) @@ -1296,6 +1329,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 +1402,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 { @@ -1444,5 +1488,18 @@ func equalBody(a *types.Body, b *engine.ExecutionPayloadBodyV1) bool { if errA != errB { return false } + if !bytes.Equal(aBytes, bBytes) { + return false + } + if a.Withdrawals == nil && b.Withdrawals == nil { + return true + } else if a.Withdrawals == nil || b.Withdrawals == nil { + return false + } + aBytes, errA = rlp.EncodeToBytes(a.Withdrawals) + bBytes, errB = rlp.EncodeToBytes(b.Withdrawals) + if errA != errB { + return false + } return bytes.Equal(aBytes, bBytes) } From b3490a7280f71223d6daae1e5492f3fba19e661d Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Wed, 15 Feb 2023 11:05:18 +0100 Subject: [PATCH 2/8] test --- eth/catalyst/api.go | 1 + 1 file changed, 1 insertion(+) diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index fc7529c8f5d7..7f86b06f5d24 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -835,6 +835,7 @@ func getBody(block *types.Block) *engine.ExecutionPayloadBodyV1 { // Post-shanghai withdrawals MUST be set to empty slice instead of nil if withdrawals == nil && block.Header().WithdrawalsHash != nil { + panic("asdf") withdrawals = make([]*types.Withdrawal, 0) } From 52d1c8528888ea070135c29c22337042a0b1484b Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Wed, 15 Feb 2023 11:21:43 +0100 Subject: [PATCH 3/8] test --- eth/catalyst/api.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index 7f86b06f5d24..c443bdfec987 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -834,8 +834,7 @@ func getBody(block *types.Block) *engine.ExecutionPayloadBodyV1 { } // Post-shanghai withdrawals MUST be set to empty slice instead of nil - if withdrawals == nil && block.Header().WithdrawalsHash != nil { - panic("asdf") + if withdrawals == nil { withdrawals = make([]*types.Withdrawal, 0) } From 9cc2a54917bd5732ea1b03a378626cebb17c6285 Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Wed, 15 Feb 2023 11:29:41 +0100 Subject: [PATCH 4/8] test --- eth/catalyst/api.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index c443bdfec987..fdf9217fc57b 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -825,7 +825,7 @@ func getBody(block *types.Block) *engine.ExecutionPayloadBodyV1 { var ( body = block.Body() txs = make([]hexutil.Bytes, len(body.Transactions)) - withdrawals = body.Withdrawals + withdrawals = make([]*types.Withdrawal, 0) ) for j, tx := range body.Transactions { @@ -834,8 +834,8 @@ func getBody(block *types.Block) *engine.ExecutionPayloadBodyV1 { } // Post-shanghai withdrawals MUST be set to empty slice instead of nil - if withdrawals == nil { - withdrawals = make([]*types.Withdrawal, 0) + if body.Withdrawals != nil { + withdrawals = body.Withdrawals } return &engine.ExecutionPayloadBodyV1{ From 9450dbf7152ab8c84d90ce8ca7380d1f1085703d Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Wed, 15 Feb 2023 11:53:18 +0100 Subject: [PATCH 5/8] beacon/engine: marshall withdrawals even if nil --- beacon/engine/types.go | 2 +- eth/catalyst/api.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/beacon/engine/types.go b/beacon/engine/types.go index 58f72631194f..07ebe544b438 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.go b/eth/catalyst/api.go index fdf9217fc57b..fc7529c8f5d7 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -825,7 +825,7 @@ func getBody(block *types.Block) *engine.ExecutionPayloadBodyV1 { var ( body = block.Body() txs = make([]hexutil.Bytes, len(body.Transactions)) - withdrawals = make([]*types.Withdrawal, 0) + withdrawals = body.Withdrawals ) for j, tx := range body.Transactions { @@ -834,8 +834,8 @@ func getBody(block *types.Block) *engine.ExecutionPayloadBodyV1 { } // Post-shanghai withdrawals MUST be set to empty slice instead of nil - if body.Withdrawals != nil { - withdrawals = body.Withdrawals + if withdrawals == nil && block.Header().WithdrawalsHash != nil { + withdrawals = make([]*types.Withdrawal, 0) } return &engine.ExecutionPayloadBodyV1{ From 87af877ef573e17adca8c27802e122920cc0e966 Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Tue, 7 Mar 2023 15:02:34 +0100 Subject: [PATCH 6/8] eth/catalyst: better equality --- eth/catalyst/api_test.go | 46 ++++++++++++++-------------------------- 1 file changed, 16 insertions(+), 30 deletions(-) diff --git a/eth/catalyst/api_test.go b/eth/catalyst/api_test.go index d6510541c816..b0c5d7f3e5d2 100644 --- a/eth/catalyst/api_test.go +++ b/eth/catalyst/api_test.go @@ -22,6 +22,7 @@ import ( "fmt" "math/big" "math/rand" + "reflect" "sync" "testing" "time" @@ -42,7 +43,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" ) @@ -1238,10 +1238,10 @@ func TestNilWithdrawals(t *testing.T) { } func setupBodies(t *testing.T) (*node.Node, *eth.Ethereum, []*types.Block) { - genesis, preMergeBlocks := generateMergeChain(10, true) - 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 = &preMergeBlocks[len(preMergeBlocks)-1].Header().Time + ethservice.BlockChain().Config().ShanghaiTime = &blocks[len(blocks)-1].Header().Time var ( parent = ethservice.BlockChain().CurrentBlock() @@ -1267,12 +1267,12 @@ func setupBodies(t *testing.T) (*node.Node, *eth.Ethereum, []*types.Block) { } } - postMergeHeaders := setupBlocks(t, ethservice, 10, parent, callback, withdrawals) - postMergeBlocks := make([]*types.Block, len(postMergeHeaders)) - for i, header := range postMergeHeaders { - postMergeBlocks[i] = ethservice.BlockChain().GetBlock(header.Hash(), header.Number.Uint64()) + 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 { @@ -1478,28 +1478,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 { - return false - } - if !bytes.Equal(aBytes, bBytes) { + if len(a.Transactions) != len(b.TransactionData) { return false } - if a.Withdrawals == nil && b.Withdrawals == nil { - return true - } else if a.Withdrawals == nil || b.Withdrawals == nil { - return false - } - aBytes, errA = rlp.EncodeToBytes(a.Withdrawals) - bBytes, errB = rlp.EncodeToBytes(b.Withdrawals) - if errA != errB { - return false + for i, tx := range a.Transactions { + data, _ := tx.MarshalBinary() + if !reflect.DeepEqual(hexutil.Bytes(data), b.TransactionData[i]) { + return false + } } - return bytes.Equal(aBytes, bBytes) + return reflect.DeepEqual(a.Withdrawals, b.Withdrawals) } From fc6ffdbb51b77ff506f3a6af26177d4e9caf5a8d Mon Sep 17 00:00:00 2001 From: Felix Lange Date: Tue, 7 Mar 2023 15:42:41 +0100 Subject: [PATCH 7/8] eth/catalyst: use bytes.Equal for []byte --- eth/catalyst/api_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eth/catalyst/api_test.go b/eth/catalyst/api_test.go index b0c5d7f3e5d2..e7cde70b907b 100644 --- a/eth/catalyst/api_test.go +++ b/eth/catalyst/api_test.go @@ -1483,7 +1483,7 @@ func equalBody(a *types.Body, b *engine.ExecutionPayloadBodyV1) bool { } for i, tx := range a.Transactions { data, _ := tx.MarshalBinary() - if !reflect.DeepEqual(hexutil.Bytes(data), b.TransactionData[i]) { + if !bytes.Equal(data, b.TransactionData[i]) { return false } } From 6501cece945547f427febde7ff1c12463999bf12 Mon Sep 17 00:00:00 2001 From: Felix Lange Date: Tue, 7 Mar 2023 15:50:41 +0100 Subject: [PATCH 8/8] eth/catalyst: resolve deprecation warning --- eth/catalyst/api_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/eth/catalyst/api_test.go b/eth/catalyst/api_test.go index e7cde70b907b..fb6e6935ee46 100644 --- a/eth/catalyst/api_test.go +++ b/eth/catalyst/api_test.go @@ -19,6 +19,7 @@ package catalyst import ( "bytes" "context" + crand "crypto/rand" "fmt" "math/big" "math/rand" @@ -1261,7 +1262,7 @@ func setupBodies(t *testing.T) (*node.Node, *eth.Ethereum, []*types.Block) { withdrawals[1] = make([]*types.Withdrawal, 0) for i := 2; i < len(withdrawals); i++ { addr := make([]byte, 20) - rand.Read(addr) + crand.Read(addr) withdrawals[i] = []*types.Withdrawal{ {Index: rand.Uint64(), Validator: rand.Uint64(), Amount: rand.Uint64(), Address: common.BytesToAddress(addr)}, }