less diff, edit comments

core/txpool/blobpool/blobpool.go

@@ -1984,6 +1984,7 @@ func (p *BlobPool) addLocked(ptx *BlobTxForPool, checkGapped bool) (err error) {
 		}
 		return err
 	}
+	//todo: validation happens twice for eth72
 	if err := txpool.ValidateCells(ptx.CellSidecar); err != nil {
 		return err
 	}
@@ -2235,8 +2236,6 @@ func (p *BlobPool) drop() {
 //
 // The transactions can also be pre-filtered by the dynamic fee components to
 // reduce allocations and load on downstream subsystems.
-// The transactions without enough cells to recover thier blobs would be skipped
-// in the response.
 func (p *BlobPool) Pending(filter txpool.PendingFilter) (map[common.Address][]*txpool.LazyTransaction, int) {
 	// If only plain transactions are requested, this pool is unsuitable as it
 	// contains none, don't even bother.

core/txpool/blobpool/blobpool_test.go

@@ -237,10 +237,7 @@ func makeTx(nonce uint64, gasTipCap uint64, gasFeeCap uint64, blobFeeCap uint64,
 
 // encodeForPool encodes a blob transaction in the blobTxForPool storage format.
 func encodeForPool(tx *types.Transaction) []byte {
-	ptx, err := newBlobTxForPool(tx)
-	if err != nil {
-		panic(err)
-	}
+	ptx, _ := newBlobTxForPool(tx)
 	blob, _ := rlp.EncodeToBytes(ptx)
 	return blob
 }
@@ -2162,6 +2159,51 @@ func TestGetBlobs(t *testing.T) {
 	pool.Close()
 }
 
+// TestEncodeForNetwork verifies that encodeForNetwork produces the correct wire
+// encoding for each (sidecar version, eth protocol version) combination.
+//   - eth/69, eth/70: blobs recovered from cells, output matches the original tx
+//   - eth/72:         blob payload omitted, output matches removeBlobs(tx)
+func TestEncodeForNetwork(t *testing.T) {
+	cases := []struct {
+		name       string
+		sidecarVer byte
+		ethVer     uint
+	}{
+		{"v0/eth70", types.BlobSidecarVersion0, 70},
+		{"v1/eth70", types.BlobSidecarVersion1, 70},
+		{"v0/eth72", types.BlobSidecarVersion0, 72},
+		{"v1/eth72", types.BlobSidecarVersion1, 72},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			testEncodeForNetwork(t, tc.sidecarVer, tc.ethVer)
+		})
+	}
+}
+
+func testEncodeForNetwork(t *testing.T, sidecarVer byte, ethVer uint) {
+	key, _ := crypto.GenerateKey()
+	tx := makeMultiBlobTx(0, 1, 1, 1, 1, 0, key, sidecarVer)
+
+	wantTx := tx
+	if ethVer >= 72 {
+		wantTx = removeBlobs(tx)
+	}
+	wantRLP, err := rlp.EncodeToBytes(wantTx)
+	if err != nil {
+		t.Fatalf("failed to encode tx: %v", err)
+	}
+	storedRLP := encodeForPool(tx)
+
+	gotRLP, err := encodeForNetwork(storedRLP, ethVer)
+	if err != nil {
+		t.Fatalf("encodeForNetwork failed: %v", err)
+	}
+	if !bytes.Equal(gotRLP, wantRLP) {
+		t.Fatalf("network encoding mismatch: got %d bytes, want %d bytes", len(gotRLP), len(wantRLP))
+	}
+}
+
 // fakeBilly is a billy.Database implementation which just drops data on the floor.
 type fakeBilly struct {
 	billy.Database
@@ -2359,48 +2401,3 @@ func TestGetCells(t *testing.T) {
 		})
 	}
 }
-
-// TestEncodeForNetwork verifies that encodeForNetwork produces the correct wire
-// encoding for each (sidecar version, eth protocol version) combination.
-//   - eth/69, eth/70: blobs recovered from cells, output matches the original tx
-//   - eth/72:         blob payload omitted, output matches removeBlobs(tx)
-func TestEncodeForNetwork(t *testing.T) {
-	cases := []struct {
-		name       string
-		sidecarVer byte
-		ethVer     uint
-	}{
-		{"v0/eth70", types.BlobSidecarVersion0, 70},
-		{"v1/eth70", types.BlobSidecarVersion1, 70},
-		{"v0/eth72", types.BlobSidecarVersion0, 72},
-		{"v1/eth72", types.BlobSidecarVersion1, 72},
-	}
-	for _, tc := range cases {
-		t.Run(tc.name, func(t *testing.T) {
-			testEncodeForNetwork(t, tc.sidecarVer, tc.ethVer)
-		})
-	}
-}
-
-func testEncodeForNetwork(t *testing.T, sidecarVer byte, ethVer uint) {
-	key, _ := crypto.GenerateKey()
-	tx := makeMultiBlobTx(0, 1, 1, 1, 1, 0, key, sidecarVer)
-
-	wantTx := tx
-	if ethVer >= 72 {
-		wantTx = removeBlobs(tx)
-	}
-	wantRLP, err := rlp.EncodeToBytes(wantTx)
-	if err != nil {
-		t.Fatalf("failed to encode tx: %v", err)
-	}
-	storedRLP := encodeForPool(tx)
-
-	gotRLP, err := encodeForNetwork(storedRLP, ethVer)
-	if err != nil {
-		t.Fatalf("encodeForNetwork failed: %v", err)
-	}
-	if !bytes.Equal(gotRLP, wantRLP) {
-		t.Fatalf("network encoding mismatch: got %d bytes, want %d bytes", len(gotRLP), len(wantRLP))
-	}
-}

core/txpool/validation.go

@@ -154,6 +154,9 @@ func ValidateTransaction(tx *types.Transaction, head *types.Header, signer types
 	return nil
 }
 
+// validateBlobSidecar implements the blob sidecar validation.
+// Note that this doesn't verify the consistency between blobs(cells) and
+// proofs. For proof verification, use validateCells.
 func validateBlobSidecar(tx *types.Transaction, head *types.Header, opts *ValidationOptions) error {
 	if tx.BlobGasFeeCapIntCmp(blobTxMinBlobGasPrice) < 0 {
 		return fmt.Errorf("%w: blob fee cap %v, minimum needed %v", ErrTxGasPriceTooLow, tx.BlobGasFeeCap(), blobTxMinBlobGasPrice)
@@ -178,6 +181,7 @@ func validateBlobSidecar(tx *types.Transaction, head *types.Header, opts *Valida
 		return fmt.Errorf("%w: unexpected sidecar version, want: %d, got: %d", ErrSidecarFormatError, expected, sidecar.Version)
 	}
 
+	//todo: can we drop the support for v0 blob txs in blobpool?
 	switch sidecar.Version {
 	case types.BlobSidecarVersion0:
 		if len(sidecar.Proofs) != len(sidecar.Commitments) {

eth/catalyst/api_test.go

@@ -1912,12 +1912,7 @@ func newGetBlobEnv(t testing.TB, version byte) (*node.Node, *ConsensusAPI) {
 	tx1 := makeMultiBlobTx(&config, 0, 2, 0, key1, version) // blob[0, 2)
 	tx2 := makeMultiBlobTx(&config, 0, 2, 2, key2, version) // blob[2, 4)
 	tx3 := makeMultiBlobTx(&config, 0, 2, 4, key3, version) // blob[4, 6)
-	errs := ethServ.TxPool().Add([]*types.Transaction{tx1, tx2, tx3}, true)
-	for i, err := range errs {
-		if err != nil {
-			t.Logf("Add tx %d failed: %v", i, err)
-		}
-	}
+	ethServ.TxPool().Add([]*types.Transaction{tx1, tx2, tx3}, true)
 
 	api := newConsensusAPIWithoutHeartbeat(ethServ)
 	return n, api
@@ -2121,15 +2116,6 @@ func runGetBlobs(t testing.TB, getBlobs getBlobsFn, start, limit int, fillRandom
 		}
 	}
 	if !reflect.DeepEqual(result, expect) {
-		t.Logf("result len=%d, expect len=%d", len(result), len(expect))
-		if len(result) > 0 && result[0] != nil && len(expect) > 0 && expect[0] != nil {
-			t.Logf("result[0].Blob len=%d, expect[0].Blob len=%d", len(result[0].Blob), len(expect[0].Blob))
-			t.Logf("result[0].CellProofs len=%d, expect[0].CellProofs len=%d", len(result[0].CellProofs), len(expect[0].CellProofs))
-			t.Logf("result[0].Blob == expect[0].Blob: %v", reflect.DeepEqual(result[0].Blob, expect[0].Blob))
-			t.Logf("result[0].CellProofs == expect[0].CellProofs: %v", reflect.DeepEqual(result[0].CellProofs, expect[0].CellProofs))
-		} else {
-			t.Logf("result[0]=%v, expect[0]=%v", result, expect)
-		}
 		t.Fatalf("Unexpected result for case %s", name)
 	}
 }