internal/ethapi: fix defaults for blob fields (ethereum#29037)

Co-authored-by: Martin HS <martin@swende.se>
holiman · Feb 21, 2024 · b47cf8f · b47cf8f
1 parent b9ca38b
commit b47cf8f
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 39 deletions.
diff --git a/internal/ethapi/transaction_args.go b/internal/ethapi/transaction_args.go
@@ -177,6 +177,14 @@ func (args *TransactionArgs) setDefaults(ctx context.Context, b Backend) error {
 
 // setFeeDefaults fills in default fee values for unspecified tx fields.
 func (args *TransactionArgs) setFeeDefaults(ctx context.Context, b Backend) error {
+	head := b.CurrentHeader()
+	// Sanity check the EIP-4844 fee parameters.
+	if args.BlobFeeCap != nil && args.BlobFeeCap.ToInt().Sign() == 0 {
+		return errors.New("maxFeePerBlobGas, if specified, must be non-zero")
+	}
+	if err := args.setCancunFeeDefaults(ctx, head, b); err != nil {
+		return err
+	}
 	// If both gasPrice and at least one of the EIP-1559 fee parameters are specified, error.
 	if args.GasPrice != nil && (args.MaxFeePerGas != nil || args.MaxPriorityFeePerGas != nil) {
 		return errors.New("both gasPrice and (maxFeePerGas or maxPriorityFeePerGas) specified")
@@ -186,7 +194,6 @@ func (args *TransactionArgs) setFeeDefaults(ctx context.Context, b Backend) erro
 	// other tx values. See https://github.com/ethereum/go-ethereum/pull/23274
 	// for more information.
 	eip1559ParamsSet := args.MaxFeePerGas != nil && args.MaxPriorityFeePerGas != nil
-
 	// Sanity check the EIP-1559 fee parameters if present.
 	if args.GasPrice == nil && eip1559ParamsSet {
 		if args.MaxFeePerGas.ToInt().Sign() == 0 {
@@ -198,13 +205,7 @@ func (args *TransactionArgs) setFeeDefaults(ctx context.Context, b Backend) erro
 		return nil // No need to set anything, user already set MaxFeePerGas and MaxPriorityFeePerGas
 	}
 
-	// Sanity check the EIP-4844 fee parameters.
-	if args.BlobFeeCap != nil && args.BlobFeeCap.ToInt().Sign() == 0 {
-		return errors.New("maxFeePerBlobGas must be non-zero")
-	}
-
 	// Sanity check the non-EIP-1559 fee parameters.
-	head := b.CurrentHeader()
 	isLondon := b.ChainConfig().IsLondon(head.Number)
 	if args.GasPrice != nil && !eip1559ParamsSet {
 		// Zero gas-price is not allowed after London fork
@@ -215,21 +216,14 @@ func (args *TransactionArgs) setFeeDefaults(ctx context.Context, b Backend) erro
 	}
 
 	// Now attempt to fill in default value depending on whether London is active or not.
-	if b.ChainConfig().IsCancun(head.Number, head.Time) {
-		if err := args.setCancunFeeDefaults(ctx, head, b); err != nil {
-			return err
-		}
-	} else if isLondon {
-		if args.BlobFeeCap != nil {
-			return errors.New("maxFeePerBlobGas is not valid before Cancun is active")
-		}
+	if isLondon {
 		// London is active, set maxPriorityFeePerGas and maxFeePerGas.
 		if err := args.setLondonFeeDefaults(ctx, head, b); err != nil {
 			return err
 		}
 	} else {
-		if args.MaxFeePerGas != nil || args.MaxPriorityFeePerGas != nil || args.BlobFeeCap != nil {
-			return errors.New("maxFeePerGas and maxPriorityFeePerGas and maxFeePerBlobGas are not valid before London is active")
+		if args.MaxFeePerGas != nil || args.MaxPriorityFeePerGas != nil {
+			return errors.New("maxFeePerGas and maxPriorityFeePerGas are not valid before London is active")
 		}
 		// London not active, set gas price.
 		price, err := b.SuggestGasTipCap(ctx)
@@ -245,15 +239,19 @@ func (args *TransactionArgs) setFeeDefaults(ctx context.Context, b Backend) erro
 func (args *TransactionArgs) setCancunFeeDefaults(ctx context.Context, head *types.Header, b Backend) error {
 	// Set maxFeePerBlobGas if it is missing.
 	if args.BlobHashes != nil && args.BlobFeeCap == nil {
+		var excessBlobGas uint64
+		if head.ExcessBlobGas != nil {
+			excessBlobGas = *head.ExcessBlobGas
+		}
 		// ExcessBlobGas must be set for a Cancun block.
-		blobBaseFee := eip4844.CalcBlobFee(*head.ExcessBlobGas)
+		blobBaseFee := eip4844.CalcBlobFee(excessBlobGas)
 		// Set the max fee to be 2 times larger than the previous block's blob base fee.
 		// The additional slack allows the tx to not become invalidated if the base
 		// fee is rising.
 		val := new(big.Int).Mul(blobBaseFee, big.NewInt(2))
 		args.BlobFeeCap = (*hexutil.Big)(val)
 	}
-	return args.setLondonFeeDefaults(ctx, head, b)
+	return nil
 }
 
 // setLondonFeeDefaults fills in reasonable default fee values for unspecified fields.

diff --git a/internal/ethapi/transaction_args_test.go b/internal/ethapi/transaction_args_test.go
@@ -153,14 +153,14 @@ func TestSetFeeDefaults(t *testing.T) {
 			"legacy",
 			&TransactionArgs{MaxFeePerGas: maxFee},
 			nil,
-			errors.New("maxFeePerGas and maxPriorityFeePerGas and maxFeePerBlobGas are not valid before London is active"),
+			errors.New("maxFeePerGas and maxPriorityFeePerGas are not valid before London is active"),
 		},
 		{
 			"dynamic fee tx pre-London, priorityFee set",
 			"legacy",
 			&TransactionArgs{MaxPriorityFeePerGas: fortytwo},
 			nil,
-			errors.New("maxFeePerGas and maxPriorityFeePerGas and maxFeePerBlobGas are not valid before London is active"),
+			errors.New("maxFeePerGas and maxPriorityFeePerGas are not valid before London is active"),
 		},
 		{
 			"dynamic fee tx, maxFee < priorityFee",
@@ -207,20 +207,6 @@ func TestSetFeeDefaults(t *testing.T) {
 			errors.New("both gasPrice and (maxFeePerGas or maxPriorityFeePerGas) specified"),
 		},
 		// EIP-4844
-		{
-			"set maxFeePerBlobGas pre cancun",
-			"london",
-			&TransactionArgs{BlobFeeCap: fortytwo},
-			nil,
-			errors.New("maxFeePerBlobGas is not valid before Cancun is active"),
-		},
-		{
-			"set maxFeePerBlobGas pre london",
-			"legacy",
-			&TransactionArgs{BlobFeeCap: fortytwo},
-			nil,
-			errors.New("maxFeePerGas and maxPriorityFeePerGas and maxFeePerBlobGas are not valid before London is active"),
-		},
 		{
 			"set gas price and maxFee for blob transaction",
 			"cancun",
@@ -235,6 +221,13 @@ func TestSetFeeDefaults(t *testing.T) {
 			&TransactionArgs{BlobHashes: []common.Hash{}, BlobFeeCap: (*hexutil.Big)(big.NewInt(4)), MaxFeePerGas: maxFee, MaxPriorityFeePerGas: fortytwo},
 			nil,
 		},
+		{
+			"fill maxFeePerBlobGas when dynamic fees are set",
+			"cancun",
+			&TransactionArgs{BlobHashes: []common.Hash{}, MaxFeePerGas: maxFee, MaxPriorityFeePerGas: fortytwo},
+			&TransactionArgs{BlobHashes: []common.Hash{}, BlobFeeCap: (*hexutil.Big)(big.NewInt(4)), MaxFeePerGas: maxFee, MaxPriorityFeePerGas: fortytwo},
+			nil,
+		},
 	}
 
 	ctx := context.Background()
@@ -244,11 +237,16 @@ func TestSetFeeDefaults(t *testing.T) {
 		}
 		got := test.in
 		err := got.setFeeDefaults(ctx, b)
-		if err != nil && err.Error() == test.err.Error() {
-			// Test threw expected error.
+		if err != nil {
+			if test.err == nil {
+				t.Fatalf("test %d (%s): unexpected error: %s", i, test.name, err)
+			} else if err.Error() != test.err.Error() {
+				t.Fatalf("test %d (%s): unexpected error: (got: %s, want: %s)", i, test.name, err, test.err)
+			}
+			// Matching error.
 			continue
-		} else if err != nil {
-			t.Fatalf("test %d (%s): unexpected error: %s", i, test.name, err)
+		} else if test.err != nil {
+			t.Fatalf("test %d (%s): expected error: %s", i, test.name, test.err)
 		}
 		if !reflect.DeepEqual(got, test.want) {
 			t.Fatalf("test %d (%s): did not fill defaults as expected: (got: %v, want: %v)", i, test.name, got, test.want)