diff --git a/core/txpool/errors.go b/core/txpool/errors.go index 32e49db87c..f369c67038 100644 --- a/core/txpool/errors.go +++ b/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") ) diff --git a/core/txpool/validation.go b/core/txpool/validation.go index 4d53c386b6..510f4299ec 100644 --- a/core/txpool/validation.go +++ b/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< 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 { diff --git a/eth/fetcher/tx_fetcher.go b/eth/fetcher/tx_fetcher.go index 97d1e29862..752cd972c7 100644 --- a/eth/fetcher/tx_fetcher.go +++ b/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. @@ -323,8 +330,9 @@ func (f *TxFetcher) Enqueue(peer string, txs []*types.Transaction, direct bool) // Push all the transactions into the pool, tracking underpriced ones to avoid // re-requesting them and dropping the peer in case of malicious transfers. var ( - added = make([]common.Hash, 0, len(txs)) - metas = make([]txMetadata, 0, len(txs)) + 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,15 +374,29 @@ 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 + } } - knownMeter.Mark(duplicate) - underpricedMeter.Mark(underpriced) - otherRejectMeter.Mark(otherreject) // If 'other reject' is >25% of the deliveries in any batch, sleep a bit. if otherreject > 128/4 { @@ -375,8 +404,9 @@ func (f *TxFetcher) Enqueue(peer string, txs []*types.Transaction, direct bool) 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 }