Merge remote-tracking branch 'upstream/develop' into jg/module_experiment

op-e2e/setup.go

@@ -459,7 +459,7 @@ func (cfg SystemConfig) Start() (*System, error) {
 			c.P2P = p
 
 			if c.Driver.SequencerEnabled {
-				c.P2PSigner = &p2p.PreparedSigner{Signer: p2p.NewLocalSigner(cfg.Secrets.SequencerP2P)}
+				c.P2PSigner = &p2p.PreparedSigner{Signer: p2p.NewLegacyLocalSigner(cfg.Secrets.SequencerP2P)}
 			}
 		}
 

op-node/p2p/cli/load_signer.go

@@ -24,7 +24,7 @@ func LoadSignerSetup(ctx *cli.Context) (p2p.SignerSetup, error) {
 			return nil, fmt.Errorf("failed to read batch submitter key: %w", err)
 		}
 
-		return &p2p.PreparedSigner{Signer: p2p.NewLocalSigner(priv)}, nil
+		return &p2p.PreparedSigner{Signer: p2p.NewLegacyLocalSigner(priv)}, nil
 	}
 
 	// TODO: create remote signer

op-node/p2p/gossip.go

@@ -268,30 +268,9 @@ func BuildBlocksValidator(log log.Logger, cfg *rollup.Config, runCfg GossipRunti
 		signatureBytes, payloadBytes := data[:65], data[65:]
 
 		// [REJECT] if the signature by the sequencer is not valid
-		signingHash, err := BlockSigningHash(cfg, payloadBytes)
-		if err != nil {
-			log.Warn("failed to compute block signing hash", "err", err, "peer", id)
-			return pubsub.ValidationReject
-		}
-
-		pub, err := crypto.SigToPub(signingHash[:], signatureBytes)
-		if err != nil {
-			log.Warn("invalid block signature", "err", err, "peer", id)
-			return pubsub.ValidationReject
-		}
-		addr := crypto.PubkeyToAddress(*pub)
-
-		// In the future we may load & validate block metadata before checking the signature.
-		// And then check the signer based on the metadata, to support e.g. multiple p2p signers at the same time.
-		// For now we only have one signer at a time and thus check the address directly.
-		// This means we may drop old payloads upon key rotation,
-		// but this can be recovered from like any other missed unsafe payload.
-		if expected := runCfg.P2PSequencerAddress(); expected == (common.Address{}) {
-			log.Warn("no configured p2p sequencer address, ignoring gossiped block", "peer", id, "addr", addr)
-			return pubsub.ValidationIgnore
-		} else if addr != expected {
-			log.Warn("unexpected block author", "err", err, "peer", id, "addr", addr, "expected", expected)
-			return pubsub.ValidationReject
+		result := verifyBlockSignature(log, cfg, runCfg, id, signatureBytes, payloadBytes)
+		if result != pubsub.ValidationAccept {
+			return result
 		}
 
 		// [REJECT] if the block encoding is not valid
@@ -348,6 +327,43 @@ func BuildBlocksValidator(log log.Logger, cfg *rollup.Config, runCfg GossipRunti
 	}
 }
 
+func verifyBlockSignature(log log.Logger, cfg *rollup.Config, runCfg GossipRuntimeConfig, id peer.ID, signatureBytes []byte, payloadBytes []byte) pubsub.ValidationResult {
+	result := verifyBlockSignatureWithHasher(log, cfg, runCfg, id, signatureBytes, payloadBytes, BlockSigningHash)
+	if result != pubsub.ValidationAccept {
+		return verifyBlockSignatureWithHasher(log, cfg, runCfg, id, signatureBytes, payloadBytes, LegacyBlockSigningHash)
+	}
+	return result
+}
+
+func verifyBlockSignatureWithHasher(log log.Logger, cfg *rollup.Config, runCfg GossipRuntimeConfig, id peer.ID, signatureBytes []byte, payloadBytes []byte, hasher func(cfg *rollup.Config, payloadBytes []byte) (common.Hash, error)) pubsub.ValidationResult {
+	signingHash, err := hasher(cfg, payloadBytes)
+	if err != nil {
+		log.Warn("failed to compute block signing hash", "err", err, "peer", id)
+		return pubsub.ValidationReject
+	}
+
+	pub, err := crypto.SigToPub(signingHash[:], signatureBytes)
+	if err != nil {
+		log.Warn("invalid block signature", "err", err, "peer", id)
+		return pubsub.ValidationReject
+	}
+	addr := crypto.PubkeyToAddress(*pub)
+
+	// In the future we may load & validate block metadata before checking the signature.
+	// And then check the signer based on the metadata, to support e.g. multiple p2p signers at the same time.
+	// For now we only have one signer at a time and thus check the address directly.
+	// This means we may drop old payloads upon key rotation,
+	// but this can be recovered from like any other missed unsafe payload.
+	if expected := runCfg.P2PSequencerAddress(); expected == (common.Address{}) {
+		log.Warn("no configured p2p sequencer address, ignoring gossiped block", "peer", id, "addr", addr)
+		return pubsub.ValidationIgnore
+	} else if addr != expected {
+		log.Warn("unexpected block author", "err", err, "peer", id, "addr", addr, "expected", expected)
+		return pubsub.ValidationReject
+	}
+	return pubsub.ValidationAccept
+}
+
 type GossipIn interface {
 	OnUnsafeL2Payload(ctx context.Context, from peer.ID, msg *eth.ExecutionPayload) error
 }

op-node/p2p/gossip_test.go

@@ -2,8 +2,16 @@ package p2p
 
 import (
 	"context"
+	"crypto/ecdsa"
+	"math/big"
 	"testing"
 
+	"github.com/ethereum-optimism/optimism/op-e2e/e2eutils"
+	"github.com/ethereum-optimism/optimism/op-node/rollup"
+	"github.com/ethereum-optimism/optimism/op-node/testutils"
+	"github.com/ethereum/go-ethereum/common"
+	"github.com/ethereum/go-ethereum/crypto"
+
 	"github.com/ethereum/go-ethereum/log"
 	pubsub "github.com/libp2p/go-libp2p-pubsub"
 	"github.com/libp2p/go-libp2p/core/peer"
@@ -32,3 +40,65 @@ func TestGuardGossipValidator(t *testing.T) {
 	require.Equal(t, pubsub.ValidationAccept, val(context.Background(), "alice", nil))
 	require.Equal(t, pubsub.ValidationIgnore, val(context.Background(), "bob", nil))
 }
+
+func TestVerifyBlockSignature(t *testing.T) {
+	// Should accept signatures over both the legacy and updated signature hashes
+	tests := []struct {
+		name      string
+		newSigner func(priv *ecdsa.PrivateKey) *LocalSigner
+	}{
+		{
+			name:      "Legacy",
+			newSigner: NewLegacyLocalSigner,
+		},
+		{
+			name:      "Updated",
+			newSigner: NewLocalSigner,
+		},
+	}
+
+	logger := testlog.Logger(t, log.LvlCrit)
+	cfg := &rollup.Config{
+		L2ChainID: big.NewInt(100),
+	}
+	peerId := peer.ID("foo")
+	secrets, err := e2eutils.DefaultMnemonicConfig.Secrets()
+	require.NoError(t, err)
+	msg := []byte("any msg")
+
+	for _, test := range tests {
+		t.Run("Valid "+test.name, func(t *testing.T) {
+			runCfg := &testutils.MockRuntimeConfig{P2PSeqAddress: crypto.PubkeyToAddress(secrets.SequencerP2P.PublicKey)}
+			signer := &PreparedSigner{Signer: test.newSigner(secrets.SequencerP2P)}
+			sig, err := signer.Sign(context.Background(), SigningDomainBlocksV1, cfg.L2ChainID, msg)
+			require.NoError(t, err)
+			result := verifyBlockSignature(logger, cfg, runCfg, peerId, sig[:65], msg)
+			require.Equal(t, pubsub.ValidationAccept, result)
+		})
+
+		t.Run("WrongSigner "+test.name, func(t *testing.T) {
+			runCfg := &testutils.MockRuntimeConfig{P2PSeqAddress: common.HexToAddress("0x1234")}
+			signer := &PreparedSigner{Signer: test.newSigner(secrets.SequencerP2P)}
+			sig, err := signer.Sign(context.Background(), SigningDomainBlocksV1, cfg.L2ChainID, msg)
+			require.NoError(t, err)
+			result := verifyBlockSignature(logger, cfg, runCfg, peerId, sig[:65], msg)
+			require.Equal(t, pubsub.ValidationReject, result)
+		})
+
+		t.Run("InvalidSignature "+test.name, func(t *testing.T) {
+			runCfg := &testutils.MockRuntimeConfig{P2PSeqAddress: crypto.PubkeyToAddress(secrets.SequencerP2P.PublicKey)}
+			sig := make([]byte, 65)
+			result := verifyBlockSignature(logger, cfg, runCfg, peerId, sig, msg)
+			require.Equal(t, pubsub.ValidationReject, result)
+		})
+
+		t.Run("NoSequencer "+test.name, func(t *testing.T) {
+			runCfg := &testutils.MockRuntimeConfig{}
+			signer := &PreparedSigner{Signer: test.newSigner(secrets.SequencerP2P)}
+			sig, err := signer.Sign(context.Background(), SigningDomainBlocksV1, cfg.L2ChainID, msg)
+			require.NoError(t, err)
+			result := verifyBlockSignature(logger, cfg, runCfg, peerId, sig[:65], msg)
+			require.Equal(t, pubsub.ValidationIgnore, result)
+		})
+	}
+}

op-node/p2p/signer.go

@@ -20,7 +20,7 @@ type Signer interface {
 	io.Closer
 }
 
-func SigningHash(domain [32]byte, chainID *big.Int, payloadBytes []byte) (common.Hash, error) {
+func LegacySigningHash(domain [32]byte, chainID *big.Int, payloadBytes []byte) (common.Hash, error) {
 	var msgInput [32 + 32 + 32]byte
 	// domain: first 32 bytes
 	copy(msgInput[:32], domain[:])
@@ -35,24 +35,48 @@ func SigningHash(domain [32]byte, chainID *big.Int, payloadBytes []byte) (common
 	return crypto.Keccak256Hash(msgInput[:]), nil
 }
 
+func SigningHash(domain [32]byte, chainID *big.Int, payloadBytes []byte) (common.Hash, error) {
+	var msgInput [32 + 32 + 32]byte
+	// domain: first 32 bytes
+	copy(msgInput[:32], domain[:])
+	// chain_id: second 32 bytes
+	if chainID.BitLen() > 256 {
+		return common.Hash{}, errors.New("chain_id is too large")
+	}
+	chainID.FillBytes(msgInput[32:64])
+	// payload_hash: third 32 bytes, hash of encoded payload
+	copy(msgInput[64:], crypto.Keccak256(payloadBytes))
+
+	return crypto.Keccak256Hash(msgInput[:]), nil
+}
+
 func BlockSigningHash(cfg *rollup.Config, payloadBytes []byte) (common.Hash, error) {
 	return SigningHash(SigningDomainBlocksV1, cfg.L2ChainID, payloadBytes)
 }
 
+func LegacyBlockSigningHash(cfg *rollup.Config, payloadBytes []byte) (common.Hash, error) {
+	return LegacySigningHash(SigningDomainBlocksV1, cfg.L2ChainID, payloadBytes)
+}
+
 // LocalSigner is suitable for testing
 type LocalSigner struct {
-	priv *ecdsa.PrivateKey
+	priv   *ecdsa.PrivateKey
+	hasher func(domain [32]byte, chainID *big.Int, payloadBytes []byte) (common.Hash, error)
+}
+
+func NewLegacyLocalSigner(priv *ecdsa.PrivateKey) *LocalSigner {
+	return &LocalSigner{priv: priv, hasher: LegacySigningHash}
 }
 
 func NewLocalSigner(priv *ecdsa.PrivateKey) *LocalSigner {
-	return &LocalSigner{priv: priv}
+	return &LocalSigner{priv: priv, hasher: SigningHash}
 }
 
 func (s *LocalSigner) Sign(ctx context.Context, domain [32]byte, chainID *big.Int, encodedMsg []byte) (sig *[65]byte, err error) {
 	if s.priv == nil {
 		return nil, errors.New("signer is closed")
 	}
-	signingHash, err := SigningHash(domain, chainID, encodedMsg)
+	signingHash, err := s.hasher(domain, chainID, encodedMsg)
 	if err != nil {
 		return nil, err
 	}

op-node/p2p/signer_test.go

+package p2p
+
+import (
+	"math/big"
+	"testing"
+
+	"github.com/ethereum-optimism/optimism/op-node/rollup"
+	"github.com/stretchr/testify/require"
+)
+
+func TestSigningHash_DifferentDomain(t *testing.T) {
+	cfg := &rollup.Config{
+		L2ChainID: big.NewInt(100),
+	}
+
+	payloadBytes := []byte("arbitraryData")
+	hash, err := SigningHash(SigningDomainBlocksV1, cfg.L2ChainID, payloadBytes)
+	require.NoError(t, err, "creating first signing hash")
+
+	hash2, err := SigningHash([32]byte{3}, cfg.L2ChainID, payloadBytes)
+	require.NoError(t, err, "creating second signing hash")
+
+	require.NotEqual(t, hash, hash2, "signing hash should be different when domain is different")
+}
+
+func TestSigningHash_DifferentChainID(t *testing.T) {
+	cfg1 := &rollup.Config{
+		L2ChainID: big.NewInt(100),
+	}
+	cfg2 := &rollup.Config{
+		L2ChainID: big.NewInt(101),
+	}
+
+	payloadBytes := []byte("arbitraryData")
+	hash, err := SigningHash(SigningDomainBlocksV1, cfg1.L2ChainID, payloadBytes)
+	require.NoError(t, err, "creating first signing hash")
+
+	hash2, err := SigningHash(SigningDomainBlocksV1, cfg2.L2ChainID, payloadBytes)
+	require.NoError(t, err, "creating second signing hash")
+
+	require.NotEqual(t, hash, hash2, "signing hash should be different when chain ID is different")
+}
+
+func TestSigningHash_DifferentMessage(t *testing.T) {
+	cfg := &rollup.Config{
+		L2ChainID: big.NewInt(100),
+	}
+
+	hash, err := SigningHash(SigningDomainBlocksV1, cfg.L2ChainID, []byte("msg1"))
+	require.NoError(t, err, "creating first signing hash")
+
+	hash2, err := SigningHash(SigningDomainBlocksV1, cfg.L2ChainID, []byte("msg2"))
+	require.NoError(t, err, "creating second signing hash")
+
+	require.NotEqual(t, hash, hash2, "signing hash should be different when message is different")
+}
+
+func TestSigningHash_LimitChainID(t *testing.T) {
+	// ChainID with bitlen 257
+	chainID := big.NewInt(1)
+	chainID = chainID.SetBit(chainID, 256, 1)
+	cfg := &rollup.Config{
+		L2ChainID: chainID,
+	}
+	_, err := SigningHash(SigningDomainBlocksV1, cfg.L2ChainID, []byte("arbitraryData"))
+	require.ErrorContains(t, err, "chain_id is too large")
+}