From 0be9d76e3702cfae92f042cf6a113e22479de3f5 Mon Sep 17 00:00:00 2001 From: lightclient <14004106+lightclient@users.noreply.github.com> Date: Thu, 11 Aug 2022 02:56:53 -0600 Subject: [PATCH] internal/ethapi: rework setDefaults for tx args so fee logic is separate (#25197) Co-authored-by: bobpkr --- internal/ethapi/transaction_args.go | 118 ++++---- internal/ethapi/transaction_args_test.go | 342 +++++++++++++++++++++++ 2 files changed, 410 insertions(+), 50 deletions(-) create mode 100644 internal/ethapi/transaction_args_test.go diff --git a/internal/ethapi/transaction_args.go b/internal/ethapi/transaction_args.go index cb2782ca05..787ac65777 100644 --- a/internal/ethapi/transaction_args.go +++ b/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. diff --git a/internal/ethapi/transaction_args_test.go b/internal/ethapi/transaction_args_test.go new file mode 100644 index 0000000000..92f009aa84 --- /dev/null +++ b/internal/ethapi/transaction_args_test.go @@ -0,0 +1,342 @@ +// 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 . + +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 := ¶ms.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 }