Merge pull request #5226 from ethereum-optimism/aj/reject-old-gossip-…

…hash op-node/p2p: Remove support for legacy gossip signing hashes
ethereum-optimism · Apr 3, 2023 · 37dddde · 37dddde
2 parents e30fd3f + ebd3b0b
commit 37dddde
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 92 deletions.
diff --git a/op-node/p2p/gossip.go b/op-node/p2p/gossip.go
@@ -338,27 +338,15 @@ 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(nil, cfg, runCfg, id, signatureBytes, payloadBytes, LegacyBlockSigningHash)
-	if result != pubsub.ValidationAccept {
-		return verifyBlockSignatureWithHasher(log, cfg, runCfg, id, signatureBytes, payloadBytes, BlockSigningHash)
-	}
-	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)
+	signingHash, err := BlockSigningHash(cfg, payloadBytes)
 	if err != nil {
-		if log != nil {
-			log.Warn("failed to compute block signing hash", "err", err, "peer", id)
-		}
+		log.Warn("failed to compute block signing hash", "err", err, "peer", id)
 		return pubsub.ValidationReject
 	}
 
 	pub, err := crypto.SigToPub(signingHash[:], signatureBytes)
 	if err != nil {
-		if log != nil {
-			log.Warn("invalid block signature", "err", err, "peer", id)
-		}
+		log.Warn("invalid block signature", "err", err, "peer", id)
 		return pubsub.ValidationReject
 	}
 	addr := crypto.PubkeyToAddress(*pub)
@@ -369,14 +357,10 @@ func verifyBlockSignatureWithHasher(log log.Logger, cfg *rollup.Config, runCfg G
 	// 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{}) {
-		if log != nil {
-			log.Warn("no configured p2p sequencer address, ignoring gossiped block", "peer", id, "addr", addr)
-		}
+		log.Warn("no configured p2p sequencer address, ignoring gossiped block", "peer", id, "addr", addr)
 		return pubsub.ValidationIgnore
 	} else if addr != expected {
-		if log != nil {
-			log.Warn("unexpected block author", "err", err, "peer", id, "addr", addr, "expected", expected)
-		}
+		log.Warn("unexpected block author", "err", err, "peer", id, "addr", addr, "expected", expected)
 		return pubsub.ValidationReject
 	}
 	return pubsub.ValidationAccept

diff --git a/op-node/p2p/gossip_test.go b/op-node/p2p/gossip_test.go
@@ -2,7 +2,6 @@ package p2p
 
 import (
 	"context"
-	"crypto/ecdsa"
 	"math/big"
 	"testing"
 
@@ -42,21 +41,6 @@ func TestGuardGossipValidator(t *testing.T) {
 }
 
 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),
@@ -66,43 +50,37 @@ func TestVerifyBlockSignature(t *testing.T) {
 	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("Valid", func(t *testing.T) {
+		runCfg := &testutils.MockRuntimeConfig{P2PSeqAddress: crypto.PubkeyToAddress(secrets.SequencerP2P.PublicKey)}
+		signer := &PreparedSigner{Signer: NewLocalSigner(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("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("WrongSigner", func(t *testing.T) {
+		runCfg := &testutils.MockRuntimeConfig{P2PSeqAddress: common.HexToAddress("0x1234")}
+		signer := &PreparedSigner{Signer: NewLocalSigner(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("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)
-		})
-	}
-}
+	t.Run("InvalidSignature", 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)
+	})
 
-func newLegacyLocalSigner(priv *ecdsa.PrivateKey) *LocalSigner {
-	return &LocalSigner{priv: priv, hasher: LegacySigningHash}
+	t.Run("NoSequencer", func(t *testing.T) {
+		runCfg := &testutils.MockRuntimeConfig{}
+		signer := &PreparedSigner{Signer: NewLocalSigner(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)
+	})
 }
diff --git a/op-node/p2p/signer.go b/op-node/p2p/signer.go
@@ -20,21 +20,6 @@ type Signer interface {
 	io.Closer
 }
 
-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[:])
-	// 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[32:], crypto.Keccak256(payloadBytes))
-
-	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
@@ -54,10 +39,6 @@ func BlockSigningHash(cfg *rollup.Config, payloadBytes []byte) (common.Hash, err
 	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