refactor validation logic

core/txpool/blobpool/blobpool.go

@@ -156,28 +156,26 @@ type blobTxMeta struct {
 // blobpool.
 type BlobTxForPool struct {
 	Tx          *types.Transaction // tx without sidecar
-	Version     byte
+	CellSidecar *types.BlobTxCellSidecar
-	Commitments []kzg4844.Commitment
-	Proofs      []kzg4844.Proof
-	Cells       []kzg4844.Cell
-	Custody     types.CustodyBitmap
 }
 
 // Sidecar returns BlobTxSidecar of pooled transaction. Since this function
 // recovers the blob field in sidecar, it is expansive and needs to be
 // avoided if possible. Returns error if recovery fails (e.g. insufficient cells).
 func (ptx *BlobTxForPool) sidecar() (*types.BlobTxSidecar, error) {
-	blobs, err := kzg4844.RecoverBlobs(ptx.Cells, ptx.Custody.Indices())
+	sidecar := ptx.CellSidecar
+	blobs, err := kzg4844.RecoverBlobs(sidecar.Cells, sidecar.Custody.Indices())
 	if err != nil {
 		return nil, err
 	}
-	return types.NewBlobTxSidecar(ptx.Version, blobs, ptx.Commitments, ptx.Proofs), nil
+	return types.NewBlobTxSidecar(sidecar.Version, blobs, sidecar.Commitments, sidecar.Proofs), nil
 }
 
 func (ptx *BlobTxForPool) toV1() error {
 	// todo: If we have a function to compute proofs from cells,
 	// we can avoid blob recovery here
-	blobs, err := kzg4844.RecoverBlobs(ptx.Cells, ptx.Custody.Indices())
+	sidecar := ptx.CellSidecar
+	blobs, err := kzg4844.RecoverBlobs(sidecar.Cells, sidecar.Custody.Indices())
 	if err != nil {
 		return err
 	}
@@ -189,33 +187,37 @@ func (ptx *BlobTxForPool) toV1() error {
 		}
 		proofs = append(proofs, proof...)
 	}
-	ptx.Proofs = proofs
+	sidecar.Proofs = proofs
-	ptx.Version = types.BlobSidecarVersion1
+	sidecar.Version = types.BlobSidecarVersion1
 	return nil
 }
 
 // TxSize returns the transaction size on the network without
 // reconstructing the transaction.
 func (ptx *BlobTxForPool) txSize() uint64 {
+	sidecar := ptx.CellSidecar
+
 	var commitments, proofs uint64
-	for i := range ptx.Commitments {
+	for i := range sidecar.Commitments {
-		commitments += rlp.BytesSize(ptx.Commitments[i][:])
+		commitments += rlp.BytesSize(sidecar.Commitments[i][:])
 	}
-	for i := range ptx.Proofs {
+	for i := range sidecar.Proofs {
-		proofs += rlp.BytesSize(ptx.Proofs[i][:])
+		proofs += rlp.BytesSize(sidecar.Proofs[i][:])
 	}
 	var blob kzg4844.Blob
-	blobs := uint64(len(ptx.Commitments)) * rlp.BytesSize(blob[:])
+	blobs := uint64(len(sidecar.Commitments)) * rlp.BytesSize(blob[:])
 	return ptx.Tx.Size() + rlp.ListSize(rlp.ListSize(blobs)+rlp.ListSize(commitments)+rlp.ListSize(proofs))
 }
 
 func (ptx *BlobTxForPool) txSizeWithoutBlob() uint64 {
+	sidecar := ptx.CellSidecar
+
 	var commitments, proofs uint64
-	for i := range ptx.Commitments {
+	for i := range sidecar.Commitments {
-		commitments += rlp.BytesSize(ptx.Commitments[i][:])
+		commitments += rlp.BytesSize(sidecar.Commitments[i][:])
 	}
-	for i := range ptx.Proofs {
+	for i := range sidecar.Proofs {
-		proofs += rlp.BytesSize(ptx.Proofs[i][:])
+		proofs += rlp.BytesSize(sidecar.Proofs[i][:])
 	}
 	return ptx.Tx.Size() + rlp.ListSize(rlp.ListSize(0)+rlp.ListSize(commitments)+rlp.ListSize(proofs))
 }
@@ -239,13 +241,16 @@ func newBlobTxForPool(tx *types.Transaction) (*BlobTxForPool, error) {
 	if err != nil {
 		return nil, err
 	}
-	return &BlobTxForPool{
+	sidecar := types.BlobTxCellSidecar{
-		Tx:          tx.WithoutBlobTxSidecar(),
 		Version:     sc.Version,
 		Commitments: sc.Commitments,
 		Proofs:      sc.Proofs,
 		Cells:       cells,
 		Custody:     *types.CustodyBitmapAll,
+	}
+	return &BlobTxForPool{
+		Tx:          tx.WithoutBlobTxSidecar(),
+		CellSidecar: &sidecar,
 	}, nil
 }
 
@@ -345,7 +350,7 @@ func newBlobTxMeta(id uint64, storageSize uint32, ptx *BlobTxForPool) *blobTxMet
 	meta := &blobTxMeta{
 		hash:            ptx.Tx.Hash(),
 		vhashes:         ptx.Tx.BlobHashes(),
-		version:         ptx.Version,
+		version:         ptx.CellSidecar.Version,
 		id:              id,
 		storageSize:     storageSize,
 		size:            ptx.txSize(),
@@ -357,7 +362,7 @@ func newBlobTxMeta(id uint64, storageSize uint32, ptx *BlobTxForPool) *blobTxMet
 		blobFeeCap:      uint256.MustFromBig(ptx.Tx.BlobGasFeeCap()),
 		execGas:         ptx.Tx.Gas(),
 		blobGas:         ptx.Tx.BlobGas(),
-		custody:         &ptx.Custody,
+		custody:         &ptx.CellSidecar.Custody,
 	}
 	meta.basefeeJumps = dynamicFeeJumps(meta.execFeeCap)
 	meta.blobfeeJumps = dynamicBlobFeeJumps(meta.blobFeeCap)
@@ -1374,7 +1379,7 @@ func (p *BlobPool) reinject(addr common.Address, txhash common.Hash) error {
 	// could theoretically halt a Geth node for ~1.2s by reorging per block. However,
 	// this attack is financially inefficient to execute.
 	head := p.head.Load()
-	if p.chain.Config().IsOsaka(head.Number, head.Time) && ptx.Version == types.BlobSidecarVersion0 {
+	if p.chain.Config().IsOsaka(head.Number, head.Time) && ptx.CellSidecar.Version == types.BlobSidecarVersion0 {
 		if err := ptx.toV1(); err != nil {
 			log.Error("Failed to convert the legacy sidecar", "err", err)
 			return err
@@ -1845,8 +1850,8 @@ func (p *BlobPool) GetBlobCells(vhashes []common.Hash, mask types.CustodyBitmap)
 			continue
 		}
 		tx := ptx.Tx
-		cellsPerBlob := ptx.Custody.OneCount()
+		cellsPerBlob := ptx.CellSidecar.Custody.OneCount()
-		storedIndices := ptx.Custody.Indices()
+		storedIndices := ptx.CellSidecar.Custody.Indices()
 
 		for blobIdx, hash := range tx.BlobHashes() {
 			indices, ok := vindex[hash]
@@ -1867,11 +1872,11 @@ func (p *BlobPool) GetBlobCells(vhashes []common.Hash, mask types.CustodyBitmap)
 					}
 				}
 				if pos >= 0 {
-					cell := ptx.Cells[blobIdx*cellsPerBlob+pos]
+					cell := ptx.CellSidecar.Cells[blobIdx*cellsPerBlob+pos]
 					blobCells[i] = &cell
 					proofIdx := blobIdx*kzg4844.CellProofsPerBlob + int(cellIdx)
-					if proofIdx < len(ptx.Proofs) {
+					if proofIdx < len(ptx.CellSidecar.Proofs) {
-						proof := ptx.Proofs[proofIdx]
+						proof := ptx.CellSidecar.Proofs[proofIdx]
 						blobProofs[i] = &proof
 					}
 				}
@@ -1940,14 +1945,6 @@ func (p *BlobPool) AddPooledTx(ptx *BlobTxForPool) (err error) {
 // Only for internal use.
 func (p *BlobPool) addLocked(ptx *BlobTxForPool, checkGapped bool) (err error) {
 	tx := ptx.Tx
-	//todo: remove this type and also ToBlobTxCellSidecar function
-	cellSidecar := &types.BlobTxCellSidecar{
-		Version:     ptx.Version,
-		Cells:       ptx.Cells,
-		Commitments: ptx.Commitments,
-		Proofs:      ptx.Proofs,
-		Custody:     ptx.Custody,
-	}
 
 	// Ensure the transaction is valid from all perspectives
 	if err := p.validateTx(tx); err != nil {
@@ -1989,10 +1986,7 @@ func (p *BlobPool) addLocked(ptx *BlobTxForPool, checkGapped bool) (err error) {
 		}
 		return err
 	}
-	if err := txpool.ValidateBlobSidecar(tx, cellSidecar, p.head.Load(), &txpool.ValidationOptions{
+	if err := txpool.ValidateCells(ptx.CellSidecar); err != nil {
-		Config:       p.chain.Config(),
-		MaxBlobCount: maxBlobsPerTx,
-	}); err != nil {
 		return err
 	}
 	// If the address is not yet known, request exclusivity to track the account

core/txpool/blobpool/buffer.go

@@ -138,7 +138,7 @@ func (b *BlobBuffer) AddCells(hash common.Hash, deliveries map[string]*PeerDeliv
 	return nil
 }
 
-// todo returning error here is strange
+// todo: this is very strange
 // add verifies cells per-peer, sorts them, and adds to the pool.
 func (b *BlobBuffer) add(hash common.Hash, tx *types.Transaction, cells *cellEntry) error {
 	sidecar := tx.BlobTxSidecar()
@@ -153,14 +153,18 @@ func (b *BlobBuffer) add(hash common.Hash, tx *types.Transaction, cells *cellEnt
 	blobCount := len(tx.BlobHashes())
 	sorted, custody := sortCells(cells, blobCount)
 
-	pooledTx := &BlobTxForPool{
+	cellSidecar := types.BlobTxCellSidecar{
-		Tx:          tx.WithoutBlobTxSidecar(),
 		Version:     sidecar.Version,
 		Commitments: sidecar.Commitments,
 		Proofs:      sidecar.Proofs,
 		Cells:       sorted,
 		Custody:     *custody,
 	}
+	pooledTx := &BlobTxForPool{
+		Tx:          tx.WithoutBlobTxSidecar(),
+		CellSidecar: &cellSidecar,
+	}
+
 	err := b.addToPool(pooledTx)
 	delete(b.cells, hash)
 	delete(b.txs, hash)

core/txpool/errors.go

@@ -74,4 +74,7 @@ var (
 
 	// ErrKZGVerificationError is returned when a KZG proof was not verified correctly.
 	ErrKZGVerificationError = errors.New("KZG verification error")
+
+	// ErrSidecarFormatError is returned when sidecar is malformed
+	ErrSidecarFormatError = errors.New("Wrong sidecar format")
 )

core/txpool/validation.go

@@ -148,54 +148,72 @@ func ValidateTransaction(tx *types.Transaction, head *types.Header, signer types
 			return errors.New("set code tx must have at least one authorization tuple")
 		}
 	}
+	if tx.Type() == types.BlobTxType {
+		return validateBlobSidecar(tx, head, opts)
+	}
 	return nil
 }
 
-func ValidateBlobSidecar(tx *types.Transaction, sidecar *types.BlobTxCellSidecar, head *types.Header, opts *ValidationOptions) error {
+func validateBlobSidecar(tx *types.Transaction, head *types.Header, opts *ValidationOptions) error {
-	if sidecar.Custody.OneCount() == 0 {
-		return errors.New("blobless blob transaction")
-	}
-	// 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", ErrTxGasPriceTooLow, tx.BlobGasFeeCap(), blobTxMinBlobGasPrice)
 	}
-	// Verify whether the blob count is consistent with other parts of the sidecar and the transaction
+	sidecar := tx.BlobTxSidecar()
-	blobCount := len(sidecar.Cells) / sidecar.Custody.OneCount()
+	if sidecar == nil {
+		return errors.New("missing sidecar in blob transaction")
+	}
 	hashes := tx.BlobHashes()
-	if blobCount == 0 {
-		return errors.New("blobless blob transaction")
-	}
-	if blobCount != len(sidecar.Commitments) || blobCount != len(hashes) {
-		return fmt.Errorf("invalid number of %d blobs compared to %d commitments and %d blob hashes", blobCount, len(sidecar.Commitments), len(tx.BlobHashes()))
-	}
-
-	// Check whether the blob count does not exceed the max blob count
-	if blobCount > opts.MaxBlobCount {
-		return fmt.Errorf("%w: blob count %v, limit %v", ErrTxBlobLimitExceeded, blobCount, opts.MaxBlobCount)
-	}
-
 	if err := sidecar.ValidateBlobCommitmentHashes(hashes); err != nil {
 		return err
 	}
-	// Ensure the sidecar version is correct for the current fork (master: bd77b77ed)
+	if len(hashes) > opts.MaxBlobCount {
-	version := types.BlobSidecarVersion0
+		return fmt.Errorf("%w: blob count %v, limit %v", ErrTxBlobLimitExceeded, len(hashes), opts.MaxBlobCount)
-	if opts.Config.IsOsaka(head.Number, head.Time) {
-		version = types.BlobSidecarVersion1
 	}
-	if sidecar.Version != version {
+
-		return fmt.Errorf("unexpected sidecar version, want: %d, got: %d", version, sidecar.Version)
+	expected := types.BlobSidecarVersion0
+	if opts.Config.IsOsaka(head.Number, head.Time) {
+		expected = types.BlobSidecarVersion1
+	}
+	if sidecar.Version != expected {
+		return fmt.Errorf("%w: unexpected sidecar version, want: %d, got: %d", ErrSidecarFormatError, expected, sidecar.Version)
+	}
+
+	switch sidecar.Version {
+	case types.BlobSidecarVersion0:
+		if len(sidecar.Proofs) != len(sidecar.Commitments) {
+			return fmt.Errorf("%w: invalid number of %d blob proofs expected %d", ErrSidecarFormatError, len(sidecar.Proofs), len(sidecar.Commitments))
+		}
+	case types.BlobSidecarVersion1:
+		if len(sidecar.Proofs) != len(sidecar.Commitments)*kzg4844.CellProofsPerBlob {
+			return fmt.Errorf("%w: invalid number of %d blob proofs expected %d", ErrSidecarFormatError, len(sidecar.Proofs), len(sidecar.Commitments)*kzg4844.CellProofsPerBlob)
+		}
+	}
+	return nil
+}
+
+func ValidateCells(sidecar *types.BlobTxCellSidecar) error {
+	// Two checks here (custody count check and blobCount check) is duplicated in buffer.go
+	// However it is required to 1) serve eth71 peer and direct submission 2) catch any bug in
+	// merging cell delivery.
+	if sidecar.Custody.OneCount() == 0 {
+		return errors.New("blobless blob transaction")
+	}
+	// Verify whether the blob count is consistent with other parts of the sidecar and the transaction
+	blobCount := len(sidecar.Cells) / sidecar.Custody.OneCount()
+	if blobCount == 0 {
+		return errors.New("blobless blob transaction")
+	}
+	if blobCount != len(sidecar.Commitments) {
+		return fmt.Errorf("invalid number of %d blobs compared to %d commitments", blobCount, len(sidecar.Commitments))
 	}
 	// Fork-specific sidecar checks, including proof verification.
 	if sidecar.Version == types.BlobSidecarVersion1 {
-		return validateBlobSidecarOsaka(sidecar, hashes)
+		return validateCellsOsaka(sidecar)
 	}
-	return validateBlobSidecarLegacy(sidecar, hashes)
+	return validateCellsLegacy(sidecar)
 }
 
-func validateBlobSidecarLegacy(sidecar *types.BlobTxCellSidecar, hashes []common.Hash) error {
+func validateCellsLegacy(sidecar *types.BlobTxCellSidecar) error {
-	if len(sidecar.Proofs) != len(hashes) {
-		return fmt.Errorf("invalid number of %d blob proofs expected %d", len(sidecar.Proofs), len(hashes))
-	}
 	blobs, err := kzg4844.RecoverBlobs(sidecar.Cells, sidecar.Custody.Indices())
 	if err != nil {
 		return fmt.Errorf("%w: %v", ErrKZGVerificationError, err)
@@ -208,10 +226,7 @@ func validateBlobSidecarLegacy(sidecar *types.BlobTxCellSidecar, hashes []common
 	return nil
 }
 
-func validateBlobSidecarOsaka(sidecar *types.BlobTxCellSidecar, hashes []common.Hash) error {
+func validateCellsOsaka(sidecar *types.BlobTxCellSidecar) error {
-	if len(sidecar.Proofs) != len(hashes)*kzg4844.CellProofsPerBlob {
-		return fmt.Errorf("invalid number of %d blob proofs expected %d", len(sidecar.Proofs), len(hashes)*kzg4844.CellProofsPerBlob)
-	}
 	indices := sidecar.Custody.Indices()
 	cellProofs := make([]kzg4844.Proof, 0)
 	for blobIdx := range len(sidecar.Commitments) {

core/types/tx_blob.go

@@ -176,20 +176,6 @@ func (sc *BlobTxSidecar) Copy() *BlobTxSidecar {
 	}
 }
 
-func (sc *BlobTxSidecar) ToBlobTxCellSidecar() (*BlobTxCellSidecar, error) {
-	cells, err := kzg4844.ComputeCells(sc.Blobs)
-	if err != nil {
-		return nil, err
-	}
-	return &BlobTxCellSidecar{
-		Version:     sc.Version,
-		Cells:       cells,
-		Commitments: sc.Commitments,
-		Proofs:      sc.Proofs,
-		Custody:     *CustodyBitmapAll,
-	}, nil
-}
-
 type BlobTxCellSidecar struct {
 	Version     byte
 	Cells       []kzg4844.Cell
@@ -198,15 +184,6 @@ type BlobTxCellSidecar struct {
 	Custody     CustodyBitmap
 }
 
-// ValidateBlobCommitmentHashes checks whether the given hashes correspond to the
-// commitments in the sidecar
-func (c *BlobTxCellSidecar) ValidateBlobCommitmentHashes(hashes []common.Hash) error {
-	sc := BlobTxSidecar{
-		Commitments: c.Commitments,
-	}
-	return sc.ValidateBlobCommitmentHashes(hashes)
-}
-
 // blobTxWithBlobs represents blob tx with its corresponding sidecar.
 // This is an interface because sidecars are versioned.
 type blobTxWithBlobs interface {

eth/fetcher/tx_fetcher.go

@@ -369,7 +369,7 @@ func (f *TxFetcher) Enqueue(peer string, txs []*types.Transaction, direct bool)
 			case errors.Is(err, txpool.ErrUnderpriced) || errors.Is(err, txpool.ErrReplaceUnderpriced) || errors.Is(err, txpool.ErrTxGasPriceTooLow):
 				underpriced++
 
-			case errors.Is(err, txpool.ErrKZGVerificationError):
+			case errors.Is(err, txpool.ErrKZGVerificationError) || errors.Is(err, txpool.ErrSidecarFormatError):
 				// KZG verification failed, terminate transaction processing immediately.
 				// Since KZG verification is computationally expensive, this acts as a
 				// defensive measure against potential DoS attacks.

eth/handler_test.go

@@ -257,7 +257,7 @@ func (p *testTxPool) AddPooledTx(pooledTx *blobpool.BlobTxForPool) error {
 	p.lock.Lock()
 	defer p.lock.Unlock()
 	hash := pooledTx.Tx.Hash()
-	p.cellPool[hash] = pooledTx.Cells
+	p.cellPool[hash] = pooledTx.CellSidecar.Cells
 	p.txPool[hash] = pooledTx.Tx
 	return nil
 }