internal/ethapi: rework setDefaults for tx args so fee logic is separate (#25197)

Co-authored-by: bobpkr <bob.p@krustuniverse.com>

internal/ethapi/transaction_args.go

@@ -75,56 +75,8 @@ func (args *TransactionArgs) data() []byte {
 
 // setDefaults fills in default values for unspecified tx fields.
 func (args *TransactionArgs) setDefaults(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 or maxPriorityFeePerGas specified but london is not active yet")
-			}
-			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)
-		}
+	if err := args.setFeeDefaults(ctx, b); err != nil {
+		return err
 	}
 	if args.Value == nil {
 		args.Value = new(hexutil.Big)
@@ -178,6 +130,72 @@ func (args *TransactionArgs) setDefaults(ctx context.Context, b Backend) error {
 	return nil
 }
 
+// setFeeDefaults fills in default fee values for unspecified tx fields.
+func (args *TransactionArgs) setFeeDefaults(ctx context.Context, b Backend) error {
+	// 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")
+	}
+	// If the tx has completely specified a fee mechanism, no default is needed. This allows users
+	// who are not yet synced past London to get defaults for other tx values. See
+	// https://github.com/ethereum/go-ethereum/pull/23274 for more information.
+	eip1559ParamsSet := args.MaxFeePerGas != nil && args.MaxPriorityFeePerGas != nil
+	if (args.GasPrice != nil && !eip1559ParamsSet) || (args.GasPrice == nil && eip1559ParamsSet) {
+		// Sanity check the EIP-1559 fee parameters if present.
+		if args.GasPrice == nil && args.MaxFeePerGas.ToInt().Cmp(args.MaxPriorityFeePerGas.ToInt()) < 0 {
+			return fmt.Errorf("maxFeePerGas (%v) < maxPriorityFeePerGas (%v)", args.MaxFeePerGas, args.MaxPriorityFeePerGas)
+		}
+		return nil
+	}
+	// Now attempt to fill in default value depending on whether London is active or not.
+	head := b.CurrentHeader()
+	if b.ChainConfig().IsLondon(head.Number) {
+		// 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 {
+			return fmt.Errorf("maxFeePerGas and maxPriorityFeePerGas are not valid before London is active")
+		}
+		// London not active, set gas price.
+		price, err := b.SuggestGasTipCap(ctx)
+		if err != nil {
+			return err
+		}
+		args.GasPrice = (*hexutil.Big)(price)
+	}
+	return nil
+}
+
+// setLondonFeeDefaults fills in reasonable default fee values for unspecified fields.
+func (args *TransactionArgs) setLondonFeeDefaults(ctx context.Context, head *types.Header, b Backend) error {
+	// Set maxPriorityFeePerGas if it is missing.
+	if args.MaxPriorityFeePerGas == nil {
+		tip, err := b.SuggestGasTipCap(ctx)
+		if err != nil {
+			return err
+		}
+		args.MaxPriorityFeePerGas = (*hexutil.Big)(tip)
+	}
+	// Set maxFeePerGas if it is missing.
+	if args.MaxFeePerGas == nil {
+		// Set the max fee to be 2 times larger than the previous block's base fee.
+		// The additional slack allows the tx to not become invalidated if the base
+		// fee is rising.
+		val := new(big.Int).Add(
+			args.MaxPriorityFeePerGas.ToInt(),
+			new(big.Int).Mul(head.BaseFee, big.NewInt(2)),
+		)
+		args.MaxFeePerGas = (*hexutil.Big)(val)
+	}
+	// Both EIP-1559 fee parameters are now set; sanity check them.
+	if args.MaxFeePerGas.ToInt().Cmp(args.MaxPriorityFeePerGas.ToInt()) < 0 {
+		return fmt.Errorf("maxFeePerGas (%v) < maxPriorityFeePerGas (%v)", args.MaxFeePerGas, args.MaxPriorityFeePerGas)
+	}
+	return nil
+}
+
 // ToMessage converts the transaction arguments to the Message type used by the
 // core evm. This method is used in calls and traces that do not require a real
 // live transaction.

internal/ethapi/transaction_args_test.go

+// Copyright 2022 The go-ethereum Authors
+// This file is part of the go-ethereum library.
+//
+// The go-ethereum library is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Lesser General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// The go-ethereum library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU Lesser General Public License for more details.
+//
+// You should have received a copy of the GNU Lesser General Public License
+// along with the go-ethereum library. If not, see <http://www.gnu.org/licenses/>.
+
+package ethapi
+
+import (
+	"context"
+	"fmt"
+	"math/big"
+	"reflect"
+	"testing"
+	"time"
+
+	"github.com/ethereum/go-ethereum"
+	"github.com/ethereum/go-ethereum/accounts"
+	"github.com/ethereum/go-ethereum/common"
+	"github.com/ethereum/go-ethereum/common/hexutil"
+	"github.com/ethereum/go-ethereum/consensus"
+	"github.com/ethereum/go-ethereum/core"
+	"github.com/ethereum/go-ethereum/core/bloombits"
+	"github.com/ethereum/go-ethereum/core/state"
+	"github.com/ethereum/go-ethereum/core/types"
+	"github.com/ethereum/go-ethereum/core/vm"
+	"github.com/ethereum/go-ethereum/ethdb"
+	"github.com/ethereum/go-ethereum/event"
+	"github.com/ethereum/go-ethereum/params"
+	"github.com/ethereum/go-ethereum/rpc"
+)
+
+// TestSetFeeDefaults tests the logic for filling in default fee values works as expected.
+func TestSetFeeDefaults(t *testing.T) {
+	type test struct {
+		name     string
+		isLondon bool
+		in       *TransactionArgs
+		want     *TransactionArgs
+		err      error
+	}
+
+	var (
+		b        = newBackendMock()
+		fortytwo = (*hexutil.Big)(big.NewInt(42))
+		maxFee   = (*hexutil.Big)(new(big.Int).Add(new(big.Int).Mul(b.current.BaseFee, big.NewInt(2)), fortytwo.ToInt()))
+		al       = &types.AccessList{types.AccessTuple{Address: common.Address{0xaa}, StorageKeys: []common.Hash{{0x01}}}}
+	)
+
+	tests := []test{
+		// Legacy txs
+		{
+			"legacy tx pre-London",
+			false,
+			&TransactionArgs{},
+			&TransactionArgs{GasPrice: fortytwo},
+			nil,
+		},
+		{
+			"legacy tx post-London, explicit gas price",
+			true,
+			&TransactionArgs{GasPrice: fortytwo},
+			&TransactionArgs{GasPrice: fortytwo},
+			nil,
+		},
+
+		// Access list txs
+		{
+			"access list tx pre-London",
+			false,
+			&TransactionArgs{AccessList: al},
+			&TransactionArgs{AccessList: al, GasPrice: fortytwo},
+			nil,
+		},
+		{
+			"access list tx post-London, explicit gas price",
+			false,
+			&TransactionArgs{AccessList: al, GasPrice: fortytwo},
+			&TransactionArgs{AccessList: al, GasPrice: fortytwo},
+			nil,
+		},
+		{
+			"access list tx post-London",
+			true,
+			&TransactionArgs{AccessList: al},
+			&TransactionArgs{AccessList: al, MaxFeePerGas: maxFee, MaxPriorityFeePerGas: fortytwo},
+			nil,
+		},
+		{
+			"access list tx post-London, only max fee",
+			true,
+			&TransactionArgs{AccessList: al, MaxFeePerGas: maxFee},
+			&TransactionArgs{AccessList: al, MaxFeePerGas: maxFee, MaxPriorityFeePerGas: fortytwo},
+			nil,
+		},
+		{
+			"access list tx post-London, only priority fee",
+			true,
+			&TransactionArgs{AccessList: al, MaxFeePerGas: maxFee},
+			&TransactionArgs{AccessList: al, MaxFeePerGas: maxFee, MaxPriorityFeePerGas: fortytwo},
+			nil,
+		},
+
+		// Dynamic fee txs
+		{
+			"dynamic tx post-London",
+			true,
+			&TransactionArgs{},
+			&TransactionArgs{MaxFeePerGas: maxFee, MaxPriorityFeePerGas: fortytwo},
+			nil,
+		},
+		{
+			"dynamic tx post-London, only max fee",
+			true,
+			&TransactionArgs{MaxFeePerGas: maxFee},
+			&TransactionArgs{MaxFeePerGas: maxFee, MaxPriorityFeePerGas: fortytwo},
+			nil,
+		},
+		{
+			"dynamic tx post-London, only priority fee",
+			true,
+			&TransactionArgs{MaxFeePerGas: maxFee},
+			&TransactionArgs{MaxFeePerGas: maxFee, MaxPriorityFeePerGas: fortytwo},
+			nil,
+		},
+		{
+			"dynamic fee tx pre-London, maxFee set",
+			false,
+			&TransactionArgs{MaxFeePerGas: maxFee},
+			nil,
+			fmt.Errorf("maxFeePerGas and maxPriorityFeePerGas are not valid before London is active"),
+		},
+		{
+			"dynamic fee tx pre-London, priorityFee set",
+			false,
+			&TransactionArgs{MaxPriorityFeePerGas: fortytwo},
+			nil,
+			fmt.Errorf("maxFeePerGas and maxPriorityFeePerGas are not valid before London is active"),
+		},
+		{
+			"dynamic fee tx, maxFee < priorityFee",
+			true,
+			&TransactionArgs{MaxFeePerGas: maxFee, MaxPriorityFeePerGas: (*hexutil.Big)(big.NewInt(1000))},
+			nil,
+			fmt.Errorf("maxFeePerGas (0x3e) < maxPriorityFeePerGas (0x3e8)"),
+		},
+		{
+			"dynamic fee tx, maxFee < priorityFee while setting default",
+			true,
+			&TransactionArgs{MaxFeePerGas: (*hexutil.Big)(big.NewInt(7))},
+			nil,
+			fmt.Errorf("maxFeePerGas (0x7) < maxPriorityFeePerGas (0x2a)"),
+		},
+
+		// Misc
+		{
+			"set all fee parameters",
+			false,
+			&TransactionArgs{GasPrice: fortytwo, MaxFeePerGas: maxFee, MaxPriorityFeePerGas: fortytwo},
+			nil,
+			fmt.Errorf("both gasPrice and (maxFeePerGas or maxPriorityFeePerGas) specified"),
+		},
+		{
+			"set gas price and maxPriorityFee",
+			false,
+			&TransactionArgs{GasPrice: fortytwo, MaxPriorityFeePerGas: fortytwo},
+			nil,
+			fmt.Errorf("both gasPrice and (maxFeePerGas or maxPriorityFeePerGas) specified"),
+		},
+		{
+			"set gas price and maxFee",
+			true,
+			&TransactionArgs{GasPrice: fortytwo, MaxFeePerGas: maxFee},
+			nil,
+			fmt.Errorf("both gasPrice and (maxFeePerGas or maxPriorityFeePerGas) specified"),
+		},
+	}
+
+	ctx := context.Background()
+	for i, test := range tests {
+		if test.isLondon {
+			b.activateLondon()
+		} else {
+			b.deactivateLondon()
+		}
+		got := test.in
+		err := got.setFeeDefaults(ctx, b)
+		if err != nil && err.Error() == test.err.Error() {
+			// Test threw expected error.
+			continue
+		} else if err != nil {
+			t.Fatalf("test %d (%s): unexpected error: %s", i, test.name, 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)
+		}
+	}
+}
+
+type backendMock struct {
+	current *types.Header
+	config  *params.ChainConfig
+}
+
+func newBackendMock() *backendMock {
+	config := &params.ChainConfig{
+		ChainID:             big.NewInt(42),
+		HomesteadBlock:      big.NewInt(0),
+		DAOForkBlock:        nil,
+		DAOForkSupport:      true,
+		EIP150Block:         big.NewInt(0),
+		EIP155Block:         big.NewInt(0),
+		EIP158Block:         big.NewInt(0),
+		ByzantiumBlock:      big.NewInt(0),
+		ConstantinopleBlock: big.NewInt(0),
+		PetersburgBlock:     big.NewInt(0),
+		IstanbulBlock:       big.NewInt(0),
+		MuirGlacierBlock:    big.NewInt(0),
+		BerlinBlock:         big.NewInt(0),
+		LondonBlock:         big.NewInt(1000),
+	}
+	return &backendMock{
+		current: &types.Header{
+			Difficulty: big.NewInt(10000000000),
+			Number:     big.NewInt(1100),
+			GasLimit:   8_000_000,
+			GasUsed:    8_000_000,
+			Time:       555,
+			Extra:      make([]byte, 32),
+			BaseFee:    big.NewInt(10),
+		},
+		config: config,
+	}
+}
+
+func (b *backendMock) activateLondon() {
+	b.current.Number = big.NewInt(1100)
+}
+
+func (b *backendMock) deactivateLondon() {
+	b.current.Number = big.NewInt(900)
+}
+func (b *backendMock) SuggestGasTipCap(ctx context.Context) (*big.Int, error) {
+	return big.NewInt(42), nil
+}
+func (b *backendMock) CurrentHeader() *types.Header     { return b.current }
+func (b *backendMock) ChainConfig() *params.ChainConfig { return b.config }
+
+// Other methods needed to implement Backend interface.
+func (b *backendMock) SyncProgress() ethereum.SyncProgress { return ethereum.SyncProgress{} }
+func (b *backendMock) FeeHistory(ctx context.Context, blockCount int, lastBlock rpc.BlockNumber, rewardPercentiles []float64) (*big.Int, [][]*big.Int, []*big.Int, []float64, error) {
+	return nil, nil, nil, nil, nil
+}
+func (b *backendMock) ChainDb() ethdb.Database           { return nil }
+func (b *backendMock) AccountManager() *accounts.Manager { return nil }
+func (b *backendMock) ExtRPCEnabled() bool               { return false }
+func (b *backendMock) RPCGasCap() uint64                 { return 0 }
+func (b *backendMock) RPCEVMTimeout() time.Duration      { return time.Second }
+func (b *backendMock) RPCTxFeeCap() float64              { return 0 }
+func (b *backendMock) UnprotectedAllowed() bool          { return false }
+func (b *backendMock) SetHead(number uint64)             {}
+func (b *backendMock) HeaderByNumber(ctx context.Context, number rpc.BlockNumber) (*types.Header, error) {
+	return nil, nil
+}
+func (b *backendMock) HeaderByHash(ctx context.Context, hash common.Hash) (*types.Header, error) {
+	return nil, nil
+}
+func (b *backendMock) HeaderByNumberOrHash(ctx context.Context, blockNrOrHash rpc.BlockNumberOrHash) (*types.Header, error) {
+	return nil, nil
+}
+func (b *backendMock) CurrentBlock() *types.Block { return nil }
+func (b *backendMock) BlockByNumber(ctx context.Context, number rpc.BlockNumber) (*types.Block, error) {
+	return nil, nil
+}
+func (b *backendMock) BlockByHash(ctx context.Context, hash common.Hash) (*types.Block, error) {
+	return nil, nil
+}
+func (b *backendMock) BlockByNumberOrHash(ctx context.Context, blockNrOrHash rpc.BlockNumberOrHash) (*types.Block, error) {
+	return nil, nil
+}
+func (b *backendMock) StateAndHeaderByNumber(ctx context.Context, number rpc.BlockNumber) (*state.StateDB, *types.Header, error) {
+	return nil, nil, nil
+}
+func (b *backendMock) StateAndHeaderByNumberOrHash(ctx context.Context, blockNrOrHash rpc.BlockNumberOrHash) (*state.StateDB, *types.Header, error) {
+	return nil, nil, nil
+}
+func (b *backendMock) PendingBlockAndReceipts() (*types.Block, types.Receipts) { return nil, nil }
+func (b *backendMock) GetReceipts(ctx context.Context, hash common.Hash) (types.Receipts, error) {
+	return nil, nil
+}
+func (b *backendMock) GetTd(ctx context.Context, hash common.Hash) *big.Int { return nil }
+func (b *backendMock) GetEVM(ctx context.Context, msg core.Message, state *state.StateDB, header *types.Header, vmConfig *vm.Config) (*vm.EVM, func() error, error) {
+	return nil, nil, nil
+}
+func (b *backendMock) SubscribeChainEvent(ch chan<- core.ChainEvent) event.Subscription { return nil }
+func (b *backendMock) SubscribeChainHeadEvent(ch chan<- core.ChainHeadEvent) event.Subscription {
+	return nil
+}
+func (b *backendMock) SubscribeChainSideEvent(ch chan<- core.ChainSideEvent) event.Subscription {
+	return nil
+}
+func (b *backendMock) SendTx(ctx context.Context, signedTx *types.Transaction) error { return nil }
+func (b *backendMock) GetTransaction(ctx context.Context, txHash common.Hash) (*types.Transaction, common.Hash, uint64, uint64, error) {
+	return nil, [32]byte{}, 0, 0, nil
+}
+func (b *backendMock) GetPoolTransactions() (types.Transactions, error)         { return nil, nil }
+func (b *backendMock) GetPoolTransaction(txHash common.Hash) *types.Transaction { return nil }
+func (b *backendMock) GetPoolNonce(ctx context.Context, addr common.Address) (uint64, error) {
+	return 0, nil
+}
+func (b *backendMock) Stats() (pending int, queued int) { return 0, 0 }
+func (b *backendMock) TxPoolContent() (map[common.Address]types.Transactions, map[common.Address]types.Transactions) {
+	return nil, nil
+}
+func (b *backendMock) TxPoolContentFrom(addr common.Address) (types.Transactions, types.Transactions) {
+	return nil, nil
+}
+func (b *backendMock) SubscribeNewTxsEvent(chan<- core.NewTxsEvent) event.Subscription { return nil }
+func (b *backendMock) BloomStatus() (uint64, uint64)                                   { return 0, 0 }
+func (b *backendMock) GetLogs(ctx context.Context, blockHash common.Hash) ([][]*types.Log, error) {
+	return nil, nil
+}
+func (b *backendMock) ServiceFilter(ctx context.Context, session *bloombits.MatcherSession) {}
+func (b *backendMock) SubscribeLogsEvent(ch chan<- []*types.Log) event.Subscription         { return nil }
+func (b *backendMock) SubscribePendingLogsEvent(ch chan<- []*types.Log) event.Subscription {
+	return nil
+}
+func (b *backendMock) SubscribeRemovedLogsEvent(ch chan<- core.RemovedLogsEvent) event.Subscription {
+	return nil
+}
+
+func (b *backendMock) Engine() consensus.Engine { return nil }