diff --git a/node/cn/handler.go b/node/cn/handler.go index 42e40a006..8b89cb521 100644 --- a/node/cn/handler.go +++ b/node/cn/handler.go @@ -23,6 +23,7 @@ package cn import ( + "encoding/binary" "encoding/json" "errors" "fmt" @@ -87,6 +88,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 +100,34 @@ 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. The counts are prefixed +// because every element is fixed-size, so an unprefixed concatenation is ambiguous. +func blobSidecarKey(hashes []common.Hash, sc *types.BlobTxSidecar) common.Hash { + counts := binary.BigEndian.AppendUint32(nil, uint32(len(hashes))) + counts = binary.BigEndian.AppendUint32(counts, uint32(len(sc.Blobs))) + counts = binary.BigEndian.AppendUint32(counts, uint32(len(sc.Commitments))) + counts = binary.BigEndian.AppendUint32(counts, uint32(len(sc.Proofs))) + + parts := make([][]byte, 0, 2+len(hashes)+len(sc.Blobs)+len(sc.Commitments)+len(sc.Proofs)) + parts = append(parts, counts, []byte{sc.Version}) + for i := range hashes { + parts = append(parts, hashes[i][:]) + } + for i := range sc.Blobs { + parts = append(parts, sc.Blobs[i][:]) + } + for i := range sc.Commitments { + parts = append(parts, sc.Commitments[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 +144,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 +214,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 +1595,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) } } } diff --git a/node/cn/handler_msg_test.go b/node/cn/handler_msg_test.go index ba54da252..36dbd36f6 100644 --- a/node/cn/handler_msg_test.go +++ b/node/cn/handler_msg_test.go @@ -291,33 +291,45 @@ 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), @@ -325,27 +337,82 @@ func TestHandleTxMsg_KZGVerificationError(t *testing.T) { 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 input of the verification 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 }, + "commitment": func(sc *types.BlobTxSidecar) { sc.Commitments[0][0] ^= 0xFF }, + } { + 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)