internal/ethapi: rework `setDefaults` for tx args so fee logic is separate

[lightclient]

The PR #25193 found that there is a bug in setDefaults where the IsLondon check is always false. I don't think that change really addresses the underlying issue and I feel like the feel logic has gotten complicated enough to pull out of setDefault into its own function and test independently. So I've taken a stab at it here.

[lightclient]

Was able to fill some transactions in the console:

> eth.fillTransaction({ to: a, from:a , value:1, gas: 22000, nonce: 0})
{
  raw: "0x02ec820539808407735940846fc23ac08255f094c0ffee61108b46c8b84c63df14e3a607ac981e930180c0808080",
  tx: {
    accessList: [],
    chainId: "0x539",
    gas: "0x55f0",
    gasPrice: null,
    hash: "0x7e2a8356945abd2c4bc90178bd924268f071fc3c8f81049db6c336fc71029b7f",
    input: "0x",
    maxFeePerGas: "0x6fc23ac0",
    maxPriorityFeePerGas: "0x7735940",
    nonce: "0x0",
    r: "0x0",
    s: "0x0",
    to: "0xc0ffee61108b46c8b84c63df14e3a607ac981e93",
    type: "0x2",
    v: "0x0",
    value: "0x1"
  }
}

> eth.fillTransaction({ to: a, from:a , value:1, gas: 22000, nonce: 0, accessList:[], maxFeePerGas: 10000000000})
{
  raw: "0x02ed8205398084077359408502540be4008255f094c0ffee61108b46c8b84c63df14e3a607ac981e930180c0808080",
  tx: {
    accessList: [],
    chainId: "0x539",
    gas: "0x55f0",
    gasPrice: null,
    hash: "0x9d095c956f1e4038c1bfddf98c00def80aa892d18874e142a9b8d472e695df48",
    input: "0x",
    maxFeePerGas: "0x2540be400",
    maxPriorityFeePerGas: "0x7735940",
    nonce: "0x0",
    r: "0x0",
    s: "0x0",
    to: "0xc0ffee61108b46c8b84c63df14e3a607ac981e93",
    type: "0x2",
    v: "0x0",
    value: "0x1"
  }
}

> eth.fillTransaction({ to: a, from:a , value:1, gas: 22000, accessList:[{ address: me, storageKeys:[] }], gasPrice: 1})
{
  raw: "0x01f83a82053980018255f094c0ffee61108b46c8b84c63df14e3a607ac981e930180d7d694c0ffee61108b46c8b84c63df14e3a607ac981e93c0808080",
  tx: {
    accessList: [{
        address: "0xc0ffee61108b46c8b84c63df14e3a607ac981e93",
        storageKeys: []
    }],
    chainId: "0x539",
    gas: "0x55f0",
    gasPrice: "0x1",
    hash: "0x2f4ad9967fe047a50ae990932291415e44c5882cf77c1c44249b27516d1fdd3d",
    input: "0x",
    maxFeePerGas: null,
    maxPriorityFeePerGas: null,
    nonce: "0x0",
    r: "0x0",
    s: "0x0",
    to: "0xc0ffee61108b46c8b84c63df14e3a607ac981e93",
    type: "0x1",
    v: "0x0",
    value: "0x1"
  }
}

[lightclient]

So I've applied this patch to run the new tests against the old fee logic. Both function pass all the tests. I think that is a good sign, because the main thing this PR is aiming to address is the dead code path where a check never true and improving readability, which should lead to the function (if correctly translated) to produce the same result (which it is).

cc @holiman

diff --git a/internal/ethapi/transaction_args_test.go b/internal/ethapi/transaction_args_test.go
index 92f009aa8..7dc0c4409 100644
--- a/internal/ethapi/transaction_args_test.go
+++ b/internal/ethapi/transaction_args_test.go
@@ -18,6 +18,7 @@ package ethapi

 import (
        "context"
+       "errors"
        "fmt"
        "math/big"
        "reflect"
@@ -195,6 +196,18 @@ func TestSetFeeDefaults(t *testing.T) {
                }
                got := test.in
                err := got.setFeeDefaults(ctx, b)
+
+               got2 := *test.in
+               err2 := got2.old(ctx, b)
+               if err2 != nil && test.err != nil && err2.Error() == test.err.Error() {
+                       // do nothing
+               } else if err2 != nil {
+                       fmt.Printf("test %d (%s): old default logic failed: %s\n", i, test.name, err2)
+
+               } else if !reflect.DeepEqual(&got2, test.want) {
+                       fmt.Printf("test %d (%s): did not fill defaults as expected: (got: %v, want: %v)\n", i, test.name, &got2, test.want)
+               }
+
                if err != nil && err.Error() == test.err.Error() {
                        // Test threw expected error.
                        continue
@@ -207,6 +220,61 @@ func TestSetFeeDefaults(t *testing.T) {
        }
 }

+func (args *TransactionArgs) old(ctx context.Context, b Backend) error {
+       if args.GasPrice != nil && (args.MaxFeePerGas != nil || args.MaxPriorityFeePerGas != nil) {
+               return errors.New("both gasPrice and (maxFeePerGas or maxPriorityFeePerGas) specified")
+       }
+       // After london, default to 1559 unless gasPrice is set
+       head := b.CurrentHeader()
+       // If user specifies both maxPriorityfee and maxFee, then we do not
+       // need to consult the chain for defaults. It's definitely a London tx.
+       if args.MaxPriorityFeePerGas == nil || args.MaxFeePerGas == nil {
+               // In this clause, user left some fields unspecified.
+               if b.ChainConfig().IsLondon(head.Number) && args.GasPrice == nil {
+                       if args.MaxPriorityFeePerGas == nil {
+                               tip, err := b.SuggestGasTipCap(ctx)
+                               if err != nil {
+                                       return err
+                               }
+                               args.MaxPriorityFeePerGas = (*hexutil.Big)(tip)
+                       }
+                       if args.MaxFeePerGas == nil {
+                               gasFeeCap := new(big.Int).Add(
+                                       (*big.Int)(args.MaxPriorityFeePerGas),
+                                       new(big.Int).Mul(head.BaseFee, big.NewInt(2)),
+                               )
+                               args.MaxFeePerGas = (*hexutil.Big)(gasFeeCap)
+                       }
+                       if args.MaxFeePerGas.ToInt().Cmp(args.MaxPriorityFeePerGas.ToInt()) < 0 {
+                               return fmt.Errorf("maxFeePerGas (%v) < maxPriorityFeePerGas (%v)", args.MaxFeePerGas, args.MaxPriorityFeePerGas)
+                       }
+               } else {
+                       if args.MaxFeePerGas != nil || args.MaxPriorityFeePerGas != nil {
+                               return errors.New("maxFeePerGas and maxPriorityFeePerGas are not valid before London is active")
+                       }
+                       if args.GasPrice == nil {
+                               price, err := b.SuggestGasTipCap(ctx)
+                               if err != nil {
+                                       return err
+                               }
+                               if b.ChainConfig().IsLondon(head.Number) {
+                                       // The legacy tx gas price suggestion should not add 2x base fee
+                                       // because all fees are consumed, so it would result in a spiral
+                                       // upwards.
+                                       price.Add(price, head.BaseFee)
+                               }
+                               args.GasPrice = (*hexutil.Big)(price)
+                       }
+               }
+       } else {
+               // Both maxPriorityfee and maxFee set by caller. Sanity-check their internal relation
+               if args.MaxFeePerGas.ToInt().Cmp(args.MaxPriorityFeePerGas.ToInt()) < 0 {
+                       return fmt.Errorf("maxFeePerGas (%v) < maxPriorityFeePerGas (%v)", args.MaxFeePerGas, args.MaxPriorityFeePerGas)
+               }
+       }
+       return nil
+}
+

$ go test -v internal/ethapi/* -run TestSetFeeDefaults
=== RUN   TestSetFeeDefaults
--- PASS: TestSetFeeDefaults (0.00s)
PASS
ok      command-line-arguments  0.139s