From 62a804cf212a3f8ddf30120b246f6cef2e021226 Mon Sep 17 00:00:00 2001 From: Joshua Gutow Date: Mon, 6 Nov 2023 17:11:04 -0700 Subject: [PATCH] op-node: Check withdrawals hash in P2P validation --- op-node/p2p/gossip.go | 6 ++++ op-node/p2p/gossip_test.go | 72 ++++++++++++++++++++++++++++++++++++++ specs/derivation.md | 7 ++-- specs/rollup-node-p2p.md | 1 + 4 files changed, 84 insertions(+), 2 deletions(-) diff --git a/op-node/p2p/gossip.go b/op-node/p2p/gossip.go index ffefa085dc6f..36951a76767d 100644 --- a/op-node/p2p/gossip.go +++ b/op-node/p2p/gossip.go @@ -326,6 +326,12 @@ func BuildBlocksValidator(log log.Logger, cfg *rollup.Config, runCfg GossipRunti return pubsub.ValidationReject } + // [REJECT] if a V2 Block has non-empty withdrawals + if blockVersion == eth.BlockV2 && len(*payload.Withdrawals) != 0 { + log.Warn("payload is on v2 topic, but has non-empty withdrawals", "bad_hash", payload.BlockHash.String(), "withdrawal_count", len(*payload.Withdrawals)) + return pubsub.ValidationReject + } + seen, ok := blockHeightLRU.Get(uint64(payload.BlockNumber)) if !ok { seen = new(seenBlocks) diff --git a/op-node/p2p/gossip_test.go b/op-node/p2p/gossip_test.go index 66533be2a6ea..0f47787d84ab 100644 --- a/op-node/p2p/gossip_test.go +++ b/op-node/p2p/gossip_test.go @@ -1,18 +1,28 @@ package p2p import ( + "bytes" "context" + "fmt" "math/big" "testing" + "time" "github.com/ethereum-optimism/optimism/op-e2e/e2eutils" "github.com/ethereum-optimism/optimism/op-node/rollup" + "github.com/golang/snappy" + + // "github.com/ethereum-optimism/optimism/op-service/eth" + "github.com/ethereum-optimism/optimism/op-service/eth" "github.com/ethereum-optimism/optimism/op-service/testutils" "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/common/hexutil" + "github.com/ethereum/go-ethereum/core/types" "github.com/ethereum/go-ethereum/crypto" "github.com/ethereum/go-ethereum/log" pubsub "github.com/libp2p/go-libp2p-pubsub" + pubsub_pb "github.com/libp2p/go-libp2p-pubsub/pb" "github.com/libp2p/go-libp2p/core/peer" "github.com/stretchr/testify/require" @@ -89,3 +99,65 @@ func TestVerifyBlockSignature(t *testing.T) { require.Equal(t, pubsub.ValidationIgnore, result) }) } + +func createSignedP2Payload(payload *eth.ExecutionPayload, signer Signer, l2ChainID *big.Int) ([]byte, error) { + var buf bytes.Buffer + buf.Write(make([]byte, 65)) + if _, err := payload.MarshalSSZ(&buf); err != nil { + return nil, fmt.Errorf("failed to encoded execution payload to publish: %w", err) + } + data := buf.Bytes() + payloadData := data[65:] + sig, err := signer.Sign(context.TODO(), SigningDomainBlocksV1, l2ChainID, payloadData) + if err != nil { + return nil, fmt.Errorf("failed to sign execution payload with signer: %w", err) + } + copy(data[:65], sig[:]) + + // compress the full message + // This also copies the data, freeing up the original buffer to go back into the pool + return snappy.Encode(nil, data), nil +} + +// TestBlockValidator does some very basic tests of the p2p block validation logic +func TestBlockValidator(t *testing.T) { + // Params Set 1: Create the validation function + cfg := &rollup.Config{ + L2ChainID: big.NewInt(100), + } + secrets, err := e2eutils.DefaultMnemonicConfig.Secrets() + require.NoError(t, err) + runCfg := &testutils.MockRuntimeConfig{P2PSeqAddress: crypto.PubkeyToAddress(secrets.SequencerP2P.PublicKey)} + signer := &PreparedSigner{Signer: NewLocalSigner(secrets.SequencerP2P)} + + // valFnV1 := BuildBlocksValidator(testlog.Logger(t, log.LvlCrit), rollupCfg, runCfg, eth.BlockV1) + valFnV2 := BuildBlocksValidator(testlog.Logger(t, log.LvlCrit), cfg, runCfg, eth.BlockV2) + + // Params Set 2: Call the validation function + peerID := peer.ID("foo") + + // Valid Case + payload := eth.ExecutionPayload{ + Timestamp: hexutil.Uint64(time.Now().Unix()), + Withdrawals: &types.Withdrawals{}, + } + payload.BlockHash, _ = payload.CheckBlockHash() // hack to generate the block hash easily. + data, err := createSignedP2Payload(&payload, signer, cfg.L2ChainID) + require.NoError(t, err) + message := &pubsub.Message{Message: &pubsub_pb.Message{Data: data}} + res := valFnV2(context.TODO(), peerID, message) + require.Equal(t, res, pubsub.ValidationAccept) + + // Invalid because non-empty withdrawals when Canyon is active + payload = eth.ExecutionPayload{ + Timestamp: hexutil.Uint64(time.Now().Unix()), + Withdrawals: &types.Withdrawals{&types.Withdrawal{Index: 1, Validator: 1}}, + } + payload.BlockHash, _ = payload.CheckBlockHash() + data, err = createSignedP2Payload(&payload, signer, cfg.L2ChainID) + require.NoError(t, err) + message = &pubsub.Message{Message: &pubsub_pb.Message{Data: data}} + res = valFnV2(context.TODO(), peerID, message) + require.Equal(t, res, pubsub.ValidationReject) + +} diff --git a/specs/derivation.md b/specs/derivation.md index 09e4fb1ba267..7201d54a5d98 100644 --- a/specs/derivation.md +++ b/specs/derivation.md @@ -694,9 +694,12 @@ equivalents. The `v2` methods are backwards compatible with `v1` payloads but su [`engine_getPayloadV2`]: exec-engine.md#engine_getpayloadv2 [`engine_newPayloadV2`]: exec-engine.md#engine_newpayloadv2 -The execution payload is an object of type [`ExecutionPayloadV1`][eth-payload]. +The execution payload is an object of type [`ExecutionPayloadV2`][eth-payload]. -[eth-payload]: https://github.com/ethereum/execution-apis/blob/main/src/engine/paris.md#executionpayloadv1 +[eth-payload]: https://github.com/ethereum/execution-apis/blob/main/src/engine/shanghai.md#payloadattributesv2 + +With V2 of the execution payload, before Canyon the withdrawals field is required to be nil. After Canyon the +withdrawals field is required to be non-nil. The op-node should set the withdrawals field to be an empty list. #### Forkchoice synchronization diff --git a/specs/rollup-node-p2p.md b/specs/rollup-node-p2p.md index 828636d0a5d9..2733d4122046 100644 --- a/specs/rollup-node-p2p.md +++ b/specs/rollup-node-p2p.md @@ -291,6 +291,7 @@ An [extended-validator] checks the incoming messages as follows, in order of ope - `[REJECT]` if the `block_hash` in the `payload` is not valid - `[REJECT]` if the block is on the V1 topic and has withdrawals - `[REJECT]` if the block is on the V2 topic and does not have withdrawals +- `[REJECT]` if the block is on the V2 topic and has a non-zero amount of withdrawals - `[REJECT]` if more than 5 different blocks have been seen with the same block height - `[IGNORE]` if the block has already been seen - `[REJECT]` if the signature by the sequencer is not valid