core/txpool,eth/fetcher: eth/fetcher, core/txpool: surface an exported error from the pool when attempting to add a blob transaction where the computed hashes of the sidecar commitments doesn't match the value in the tx header. In fetcher: drop sending peer but don't remove the hashes from tracking for future request/delivery.

core/txpool/errors.go

@@ -69,4 +69,8 @@ var (
 	// ErrAuthorityNonce is returned if a transaction has an authorization with
 	// a nonce that is not currently valid for the authority.
 	ErrAuthorityNonceTooLow = errors.New("authority nonce too low")
+
+	// ErrInvalidAuxiliaryData conveys transaction validation failure from
+	// verifying the cryptographic integrity of extra-header data.
+	ErrInvalidAuxiliaryData = errors.New("invalid auxiliary data")
 )

core/txpool/validation.go

@@ -60,6 +60,30 @@ type ValidationFunction func(tx *types.Transaction, head *types.Header, signer t
 // This check is public to allow different transaction pools to check the basic
 // rules without duplicating code and running the risk of missed updates.
 func ValidateTransaction(tx *types.Transaction, head *types.Header, signer types.Signer, opts *ValidationOptions) error {
+	if tx.Type() == types.BlobTxType {
+		// Ensure the blob fee cap satisfies the minimum blob gas price
+		if tx.BlobGasFeeCapIntCmp(blobTxMinBlobGasPrice) < 0 {
+			return fmt.Errorf("%w: blob fee cap %v, minimum needed %v", ErrUnderpriced, tx.BlobGasFeeCap(), blobTxMinBlobGasPrice)
+		}
+		sidecar := tx.BlobTxSidecar()
+		if sidecar == nil {
+			return fmt.Errorf("%w: missing sidecar in blob transaction", ErrInvalidAuxiliaryData)
+		}
+		// Ensure the number of items in the blob transaction and various side
+		// data match up before doing any expensive validations
+		hashes := tx.BlobHashes()
+		if len(hashes) == 0 {
+			return errors.New("blobless blob transaction")
+		}
+		maxBlobs := eip4844.MaxBlobsPerBlock(opts.Config, head.Time)
+		if len(hashes) > maxBlobs {
+			return fmt.Errorf("too many blobs in transaction: have %d, permitted %d", len(hashes), maxBlobs)
+		}
+		// Ensure the hash of the commitments are valid
+		if err := validateBlobSidecarHashes(hashes, sidecar); err != nil {
+			return err
+		}
+	}
 	// Ensure transactions not implemented by the calling pool are rejected
 	if opts.Accept&(1<<tx.Type()) == 0 {
 		return fmt.Errorf("%w: tx type %v not supported by this pool", core.ErrTxTypeNotSupported, tx.Type())
@@ -135,26 +159,9 @@ func ValidateTransaction(tx *types.Transaction, head *types.Header, signer types
 		return fmt.Errorf("%w: gas tip cap %v, minimum needed %v", ErrUnderpriced, tx.GasTipCap(), opts.MinTip)
 	}
 	if tx.Type() == types.BlobTxType {
-		// Ensure the blob fee cap satisfies the minimum blob gas price
-		if tx.BlobGasFeeCapIntCmp(blobTxMinBlobGasPrice) < 0 {
-			return fmt.Errorf("%w: blob fee cap %v, minimum needed %v", ErrUnderpriced, tx.BlobGasFeeCap(), blobTxMinBlobGasPrice)
-		}
+		// ensure the commitments and proof are valid
 		sidecar := tx.BlobTxSidecar()
-		if sidecar == nil {
-			return errors.New("missing sidecar in blob transaction")
-		}
-		// Ensure the number of items in the blob transaction and various side
-		// data match up before doing any expensive validations
-		hashes := tx.BlobHashes()
-		if len(hashes) == 0 {
-			return errors.New("blobless blob transaction")
-		}
-		maxBlobs := eip4844.MaxBlobsPerBlock(opts.Config, head.Time)
-		if len(hashes) > maxBlobs {
-			return fmt.Errorf("too many blobs in transaction: have %d, permitted %d", len(hashes), maxBlobs)
-		}
-		// Ensure commitments, proofs and hashes are valid
-		if err := validateBlobSidecar(hashes, sidecar); err != nil {
+		if err := validateBlobSidecarProof(sidecar); err != nil {
 			return err
 		}
 	}
@@ -166,15 +173,19 @@ func ValidateTransaction(tx *types.Transaction, head *types.Header, signer types
 	return nil
 }
 
-func validateBlobSidecar(hashes []common.Hash, sidecar *types.BlobTxSidecar) error {
+// validateBlobSidecarHashes ensures that a blob tx sidecar is well-formed and where the
+// hashes of the commitments in the sidecar match those in the header.
+//
+// TODO (q): we can't really do this at the decoder level because we don't have access to the tx in the sidecar decoder?
+func validateBlobSidecarHashes(hashes []common.Hash, sidecar *types.BlobTxSidecar) error {
 	if len(sidecar.Blobs) != len(hashes) {
-		return fmt.Errorf("invalid number of %d blobs compared to %d blob hashes", len(sidecar.Blobs), len(hashes))
+		return fmt.Errorf("%w, invalid number of %d blobs compared to %d blob hashes", ErrInvalidAuxiliaryData, len(sidecar.Blobs), len(hashes))
 	}
 	if len(sidecar.Commitments) != len(hashes) {
-		return fmt.Errorf("invalid number of %d blob commitments compared to %d blob hashes", len(sidecar.Commitments), len(hashes))
+		return fmt.Errorf("%w: invalid number of %d blob commitments compared to %d blob hashes", ErrInvalidAuxiliaryData, len(sidecar.Commitments), len(hashes))
 	}
 	if len(sidecar.Proofs) != len(hashes) {
-		return fmt.Errorf("invalid number of %d blob proofs compared to %d blob hashes", len(sidecar.Proofs), len(hashes))
+		return fmt.Errorf("%w: invalid number of %d blob proofs compared to %d blob hashes", ErrInvalidAuxiliaryData, len(sidecar.Proofs), len(hashes))
 	}
 	// Blob quantities match up, validate that the provers match with the
 	// transaction hash before getting to the cryptography
@@ -182,9 +193,13 @@ func validateBlobSidecar(hashes []common.Hash, sidecar *types.BlobTxSidecar) err
 	for i, vhash := range hashes {
 		computed := kzg4844.CalcBlobHashV1(hasher, &sidecar.Commitments[i])
 		if vhash != computed {
-			return fmt.Errorf("blob %d: computed hash %#x mismatches transaction one %#x", i, computed, vhash)
+			return fmt.Errorf("%w: blob %d: computed hash %#x mismatches transaction one %#x", ErrInvalidAuxiliaryData, i, computed, vhash)
 		}
 	}
+	return nil
+}
+
+func validateBlobSidecarProof(sidecar *types.BlobTxSidecar) error {
 	// Blob commitments match with the hashes in the transaction, verify the
 	// blobs themselves via KZG
 	for i := range sidecar.Blobs {

eth/fetcher/tx_fetcher.go

@@ -145,6 +145,13 @@ type txDelivery struct {
 	hashes []common.Hash // Batch of transaction hashes having been delivered
 	metas  []txMetadata  // Batch of metadata associated with the delivered hashes
 	direct bool          // Whether this is a direct reply or a broadcast
+
+	// set of blob transactions that failed to add to the pool due to a mismatch between the
+	// sidecar commitments and the commitment hashes in the header.
+	//
+	// this flag signals to the cleanup routine that the sender peer should be dropped, while any
+	// offending transactions will not be included in hashes.
+	missingAuxData bool
 }
 
 // txDrop is the notification that a peer has disconnected.
@@ -325,6 +332,7 @@ func (f *TxFetcher) Enqueue(peer string, txs []*types.Transaction, direct bool)
 	var (
 		added          = make([]common.Hash, 0, len(txs))
 		metas          = make([]txMetadata, 0, len(txs))
+		missingAuxData bool
 	)
 	// proceed in batches
 	for i := 0; i < len(txs); i += 128 {
@@ -346,10 +354,17 @@ func (f *TxFetcher) Enqueue(peer string, txs []*types.Transaction, direct bool)
 			if errors.Is(err, txpool.ErrUnderpriced) || errors.Is(err, txpool.ErrReplaceUnderpriced) {
 				f.underpriced.Add(batch[j].Hash(), batch[j].Time())
 			}
+
 			// Track a few interesting failure types
 			switch {
 			case err == nil: // Noop, but need to handle to not count these
 
+			case errors.Is(err, txpool.ErrInvalidAuxiliaryData):
+				// blob tx where commitment hashes in the header cannot be
+				// recomputed from the given sidecar.
+				// flag the sending peer to be dropped by cleanup.
+				missingAuxData = true
+				otherreject++
 			case errors.Is(err, txpool.ErrAlreadyKnown):
 				duplicate++
 
@@ -359,24 +374,39 @@ func (f *TxFetcher) Enqueue(peer string, txs []*types.Transaction, direct bool)
 			default:
 				otherreject++
 			}
-			added = append(added, batch[j].Hash())
-			metas = append(metas, txMetadata{
+
+			txHash := batch[j].Hash()
+			txMeta := txMetadata{
 				kind: batch[j].Type(),
 				size: uint32(batch[j].Size()),
-			})
 			}
+
+			// In the case of potentially-valid blob transaction with
+			// missing/invalid sidecar: don't mark delivered and remove
+			// the tx hash from the trackers.
+			if !errors.Is(err, txpool.ErrInvalidAuxiliaryData) {
+				added = append(added, txHash)
+				metas = append(metas, txMeta)
+			}
+
 			knownMeter.Mark(duplicate)
 			underpricedMeter.Mark(underpriced)
 			otherRejectMeter.Mark(otherreject)
 
+			if missingAuxData {
+				break
+			}
+		}
+
 		// If 'other reject' is >25% of the deliveries in any batch, sleep a bit.
 		if otherreject > 128/4 {
 			time.Sleep(200 * time.Millisecond)
 			log.Debug("Peer delivering stale transactions", "peer", peer, "rejected", otherreject)
 		}
 	}
+
 	select {
-	case f.cleanup <- &txDelivery{origin: peer, hashes: added, metas: metas, direct: direct}:
+	case f.cleanup <- &txDelivery{origin: peer, hashes: added, metas: metas, direct: direct, missingAuxData: missingAuxData}:
 		return nil
 	case <-f.quit:
 		return errTerminated
@@ -640,6 +670,7 @@ func (f *TxFetcher) loop() {
 			f.rescheduleTimeout(timeoutTimer, timeoutTrigger)
 
 		case delivery := <-f.cleanup:
+
 			// Independent if the delivery was direct or broadcast, remove all
 			// traces of the hash from internal trackers. That said, compare any
 			// advertised metadata with the real ones and drop bad peers.
@@ -650,17 +681,6 @@ func (f *TxFetcher) loop() {
 							if delivery.metas[i].kind != meta.kind {
 								log.Warn("Announced transaction type mismatch", "peer", peer, "tx", hash, "type", delivery.metas[i].kind, "ann", meta.kind)
 								f.dropPeer(peer)
-							} else if delivery.metas[i].size != meta.size {
-								if math.Abs(float64(delivery.metas[i].size)-float64(meta.size)) > 8 {
-									log.Warn("Announced transaction size mismatch", "peer", peer, "tx", hash, "size", delivery.metas[i].size, "ann", meta.size)
-
-									// Normally we should drop a peer considering this is a protocol violation.
-									// However, due to the RLP vs consensus format messyness, allow a few bytes
-									// wiggle-room where we only warn, but don't drop.
-									//
-									// TODO(karalabe): Get rid of this relaxation when clients are proven stable.
-									f.dropPeer(peer)
-								}
 							}
 						}
 						delete(txset, hash)
@@ -717,6 +737,13 @@ func (f *TxFetcher) loop() {
 				// Mark the requesting successful (independent of individual status)
 				txRequestDoneMeter.Mark(int64(len(delivery.hashes)))
 
+				// if the peer transmitted a blob transaction with a mismatch
+				// between blob_commitments_hashes and the hashes computed
+				// from the sidecar, drop the peer.
+				if delivery.missingAuxData {
+					f.dropPeer(delivery.origin)
+				}
+
 				// Make sure something was pending, nuke it
 				req := f.requests[delivery.origin]
 				if req == nil {
@@ -819,7 +846,6 @@ func (f *TxFetcher) loop() {
 				f.scheduleFetches(timeoutTimer, timeoutTrigger, nil)
 				f.rescheduleTimeout(timeoutTimer, timeoutTrigger)
 			}
-
 		case <-f.quit:
 			return
 		}