node/cn: verify a blob tx sidecar only once

node/cn/handler.go

@@ -87,6 +87,9 @@ const (
 
 	// ExtraNonSnapPeers is the number of non-snap peers allowed to connect more than snap peers.
 	ExtraNonSnapPeers = 5
+
+	// maxVerifiedBlobTxs bounds verifiedBlobTxs.
+	maxVerifiedBlobTxs = 1024
 )
 
 var (
@@ -96,8 +99,26 @@ var (
 	errUnknownProcessingError  = errors.New("unknown error during the msg processing")
 	errUnsupportedEnginePolicy = errors.New("unsupported engine or policy")
 	errKZGVerificationError    = errors.New("KZG verification error")
+	errBloblessBlobTx          = errors.New("blobless blob transaction")
 )
 
+// blobSidecarKey identifies what ValidateWithBlobHashes consumes. Commitments are left
+// out because each hash is the sha256 of one, so the hashes already pin them.
+func blobSidecarKey(hashes []common.Hash, sc *types.BlobTxSidecar) common.Hash {
+	parts := make([][]byte, 0, len(hashes)+1+len(sc.Blobs)+len(sc.Proofs))
+	for i := range hashes {
+		parts = append(parts, hashes[i][:])
+	}
+	parts = append(parts, []byte{sc.Version})
+	for i := range sc.Blobs {
+		parts = append(parts, sc.Blobs[i][:])
+	}
+	for i := range sc.Proofs {
+		parts = append(parts, sc.Proofs[i][:])
+	}
+	return crypto.Keccak256Hash(parts...)
+}
+
 func errResp(code errCode, format string, v ...interface{}) error {
 	return fmt.Errorf("%v - %v", code, fmt.Sprintf(format, v...))
 }
@@ -114,6 +135,9 @@ type ProtocolManager struct {
 	chainconfig *params.ChainConfig
 	maxPeers    int
 
+	// verifiedBlobTxs holds blobSidecarKey values that already passed KZG verification.
+	verifiedBlobTxs *knownHashSet
+
 	downloader ProtocolManagerDownloader
 	fetcher    ProtocolManagerFetcher
 	peers      PeerSet
@@ -181,6 +205,7 @@ func NewProtocolManager(config *params.ChainConfig, mode downloader.SyncMode, ne
 		handler:           handler,
 		nodetype:          nodetype,
 		txResendUseLegacy: cnconfig.TxResendUseLegacy,
+		verifiedBlobTxs:   newKnownHashSet(maxVerifiedBlobTxs),
 		blobSidecarReqManager: &sidecarReqManager{
 			list:     make(map[common.Hash]*sidecarReq),
 			cooldown: 10 * time.Second,
@@ -1561,14 +1586,22 @@ func handleTxMsg(pm *ProtocolManager, p Peer, msg p2p.Msg) error {
 		// KZG verification is computationally expensive, so this acts as a
 		// defensive measure against potential DoS attacks.
 		if tx.Type() == types.TxTypeEthereumBlob {
+			// Without blob hashes the verification below passes vacuously.
+			if len(tx.BlobHashes()) == 0 {
+				return errResp(ErrDecode, "Invalid blob transaction with sidecar: %v", errBloblessBlobTx)
+			}
 			sidecar := tx.BlobTxSidecar()
 			if sidecar != nil {
-				// If any of the transaction contains invalid KZG sidecar, terminate transaction processing immediately.
-				// KZG verification is computationally expensive, so this acts as a
-				// defensive measure against potential DoS attacks.
-				if err := sidecar.ValidateWithBlobHashes(tx.BlobHashes()); err != nil {
-					logger.Warn("Disconnect peer for protocol violation", "peer", p.GetID(), "error", err)
-					return errResp(ErrDecode, "Invalid blob transaction with sidecar: %v", errKZGVerificationError)
+				key := blobSidecarKey(tx.BlobHashes(), sidecar)
+				if !pm.verifiedBlobTxs.Contains(key) {
+					// If any of the transaction contains invalid KZG sidecar, terminate transaction processing immediately.
+					// KZG verification is computationally expensive, so this acts as a
+					// defensive measure against potential DoS attacks.
+					if err := sidecar.ValidateWithBlobHashes(tx.BlobHashes()); err != nil {
+						logger.Warn("Disconnect peer for protocol violation", "peer", p.GetID(), "error", err)
+						return errResp(ErrDecode, "Invalid blob transaction with sidecar: %v", errKZGVerificationError)
+					}
+					pm.verifiedBlobTxs.Add(key)
 				}
 			}
 		}

node/cn/handler_msg_test.go

@@ -291,61 +291,127 @@ func TestHandleTxMsg(t *testing.T) {
 	}
 }
 
-func TestHandleTxMsg_KZGVerificationError(t *testing.T) {
-	mockCtrl := gomock.NewController(t)
-	defer mockCtrl.Finish()
-
-	pm := &ProtocolManager{}
+// prepareBlobTxMsg returns a protocol manager, a peer and a signed blob transaction
+// with a valid v1 sidecar. Callers corrupt the sidecar via blobTx.BlobTxSidecar().
+func prepareBlobTxMsg(t *testing.T, mockCtrl *gomock.Controller) (*ProtocolManager, *MockPeer, *types.Transaction) {
+	pm := &ProtocolManager{verifiedBlobTxs: newKnownHashSet(maxVerifiedBlobTxs)}
 	pm.acceptTxs.Store(1)
 	mockTxPool := mocks.NewMockTxPool(mockCtrl)
+	mockTxPool.EXPECT().HandleTxMsg(gomock.Any()).AnyTimes()
 	pm.txpool = mockTxPool
 
 	mockPeer := NewMockPeer(mockCtrl)
 	mockPeer.EXPECT().GetVersion().Return(kaia63).AnyTimes()
 	mockPeer.EXPECT().GetID().Return("test-peer").AnyTimes()
+	mockPeer.EXPECT().AddToKnownTxs(gomock.Any()).AnyTimes()
+
+	sidecar, hashes := newBlobSidecar(t)
+	return pm, mockPeer, newBlobTx(t, 0, hashes, sidecar)
+}
+
+// newBlobSidecar returns a valid v1 sidecar and the blob hashes it commits to.
+func newBlobSidecar(t *testing.T) (*types.BlobTxSidecar, []common.Hash) {
+	blob := kzg4844.Blob{}
+	commitment, err := kzg4844.BlobToCommitment(&blob)
+	require.NoError(t, err)
+	proofs, err := kzg4844.ComputeCellProofs(&blob)
+	require.NoError(t, err)
+	return &types.BlobTxSidecar{
+		Version:     types.BlobSidecarVersion1,
+		Blobs:       []kzg4844.Blob{blob},
+		Commitments: []kzg4844.Commitment{commitment},
+		Proofs:      proofs,
+	}, []common.Hash{common.Hash(kzg4844.CalcBlobHashV1(sha256.New(), &commitment))}
+}
 
-	// Generate a blob transaction with invalid KZG proof
-	var (
-		addr          = crypto.PubkeyToAddress(keys[0].PublicKey)
-		signer        = types.MakeSigner(params.TestChainConfig, common.Big0)
-		blob          = kzg4844.Blob{}
-		commitment, _ = kzg4844.BlobToCommitment(&blob)
-		proofs, _     = kzg4844.ComputeCellProofs(&blob)
-		blobhash      = common.Hash(kzg4844.CalcBlobHashV1(sha256.New(), &commitment))
-	)
-	// corrupt a byte in the commitment
-	commitment[0] = commitment[0] ^ 0xFF
-	blobTx, err := types.NewTransactionWithMap(types.TxTypeEthereumBlob, map[types.TxValueKeyType]interface{}{
-		types.TxValueKeyNonce:      uint64(0),
-		types.TxValueKeyTo:         addr,
+// newBlobTx signs a blob transaction carrying the given hashes and sidecar. The nonce is
+// a parameter so a caller can replay one sidecar under a different transaction hash.
+func newBlobTx(t *testing.T, nonce uint64, hashes []common.Hash, sidecar *types.BlobTxSidecar) *types.Transaction {
+	tx, err := types.NewTransactionWithMap(types.TxTypeEthereumBlob, map[types.TxValueKeyType]interface{}{
+		types.TxValueKeyNonce:      nonce,
+		types.TxValueKeyTo:         crypto.PubkeyToAddress(keys[0].PublicKey),
 		types.TxValueKeyAmount:     big.NewInt(0),
 		types.TxValueKeyGasLimit:   uint64(10000000),
 		types.TxValueKeyGasFeeCap:  big.NewInt(25),
 		types.TxValueKeyGasTipCap:  big.NewInt(25),
 		types.TxValueKeyData:       []byte{},
 		types.TxValueKeyAccessList: types.AccessList{},
 		types.TxValueKeyBlobFeeCap: big.NewInt(25),
-		types.TxValueKeyBlobHashes: []common.Hash{blobhash},
-		types.TxValueKeySidecar: &types.BlobTxSidecar{
-			Version:     types.BlobSidecarVersion1,
-			Blobs:       []kzg4844.Blob{blob},
-			Commitments: []kzg4844.Commitment{commitment},
-			Proofs:      proofs,
-		},
-		types.TxValueKeyChainID: params.TestChainConfig.ChainID,
+		types.TxValueKeyBlobHashes: hashes,
+		types.TxValueKeySidecar:    sidecar,
+		types.TxValueKeyChainID:    params.TestChainConfig.ChainID,
 	})
 	require.NoError(t, err)
-	require.NoError(t, blobTx.Sign(signer, keys[0]))
+	require.NoError(t, tx.Sign(types.MakeSigner(params.TestChainConfig, common.Big0), keys[0]))
+	return tx
+}
 
-	txs := types.Transactions{blobTx}
-	msg := generateMsg(t, TxMsg, txs)
+func TestHandleTxMsg_KZGVerificationError(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	pm, mockPeer, blobTx := prepareBlobTxMsg(t, mockCtrl)
+	blobTx.BlobTxSidecar().Commitments[0][0] ^= 0xFF
 
 	// Should return error and disconnect peer
-	err = handleTxMsg(pm, mockPeer, msg)
+	err := handleTxMsg(pm, mockPeer, generateMsg(t, TxMsg, types.Transactions{blobTx}))
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), errKZGVerificationError.Error())
 }
 
+func TestHandleTxMsg_BlobSidecarVerifiedOnce(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	pm, mockPeer, blobTx := prepareBlobTxMsg(t, mockCtrl)
+	require.NoError(t, handleTxMsg(pm, mockPeer, generateMsg(t, TxMsg, types.Transactions{blobTx})))
+	require.Equal(t, 1, pm.verifiedBlobTxs.Len())
+
+	// The same sidecar under a different transaction hash must reuse the entry, or a
+	// sender replays one sidecar for free by bumping the nonce.
+	replay := newBlobTx(t, 1, blobTx.BlobHashes(), blobTx.BlobTxSidecar())
+	require.NotEqual(t, blobTx.Hash(), replay.Hash())
+	require.NoError(t, handleTxMsg(pm, mockPeer, generateMsg(t, TxMsg, types.Transactions{replay})))
+	assert.Equal(t, 1, pm.verifiedBlobTxs.Len(), "the replay was verified again")
+
+	// A sidecar swapped under an already verified transaction hash must not reuse the
+	// entry, whichever half of the proof relation was altered.
+	for name, tamper := range map[string]func(*types.BlobTxSidecar){
+		"blob":    func(sc *types.BlobTxSidecar) { sc.Blobs[0][0] ^= 0xFF },
+		"proof":   func(sc *types.BlobTxSidecar) { sc.Proofs[0][0] ^= 0xFF },
+		"version": func(sc *types.BlobTxSidecar) { sc.Version = 0 },
+	} {
+		t.Run(name, func(t *testing.T) {
+			pm, mockPeer, blobTx := prepareBlobTxMsg(t, mockCtrl)
+			require.NoError(t, handleTxMsg(pm, mockPeer, generateMsg(t, TxMsg, types.Transactions{blobTx})))
+
+			tamper(blobTx.BlobTxSidecar())
+			err := handleTxMsg(pm, mockPeer, generateMsg(t, TxMsg, types.Transactions{blobTx}))
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), errKZGVerificationError.Error())
+		})
+	}
+}
+
+func TestHandleTxMsg_BloblessBlobTx(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	pm, mockPeer, _ := prepareBlobTxMsg(t, mockCtrl)
+
+	// Every check in ValidateWithBlobHashes compares against the declared hashes, so
+	// it passes vacuously when there are none. The pool rejects such a transaction
+	// unconditionally, so the handler must not forward it.
+	sidecar, _ := newBlobSidecar(t)
+	sidecar.Blobs, sidecar.Commitments, sidecar.Proofs = nil, nil, nil
+	blobless := newBlobTx(t, 0, nil, sidecar)
+	require.NoError(t, sidecar.ValidateWithBlobHashes(nil), "expected the vacuous pass this guards")
+
+	err := handleTxMsg(pm, mockPeer, generateMsg(t, TxMsg, types.Transactions{blobless}))
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), errBloblessBlobTx.Error())
+}
+
 func prepareTestHandleBlockHeaderFetchRequestMsg(t *testing.T) (*gomock.Controller, *MockPeer, *mocks.MockBlockChain, *ProtocolManager) {
 	mockCtrl := gomock.NewController(t)
 	mockPeer := NewMockPeer(mockCtrl)