From 3367f2646646556a79bb6ae2d3c20b8c267361c7 Mon Sep 17 00:00:00 2001 From: Ian Norden Date: Mon, 6 Jan 2020 15:12:39 -0600 Subject: [PATCH] fix trx pool gas limits; cleanup commitTransactions() --- core/tx_list.go | 26 +++++++++++++++------- core/tx_pool.go | 37 +++++++++++++++++++++---------- core/tx_pool_test.go | 10 +++++---- light/txpool.go | 27 ++++++++++++++++------- miner/worker.go | 52 ++++++++++++++++++-------------------------- 5 files changed, 90 insertions(+), 62 deletions(-) diff --git a/core/tx_list.go b/core/tx_list.go index 257bc9e24ab8..f56f372d36fe 100644 --- a/core/tx_list.go +++ b/core/tx_list.go @@ -223,8 +223,9 @@ type txList struct { strict bool // Whether nonces are strictly continuous or not txs *txSortedMap // Heap indexed sorted hash map of the transactions - costcap *big.Int // Price of the highest costing transaction (reset only if exceeds balance) - gascap uint64 // Gas limit of the highest spending transaction (reset only if exceeds block limit) + costcap *big.Int // Price of the highest costing transaction (reset only if exceeds balance) + legacyGasCap uint64 // Gas limit of the highest spending legacy transaction (reset only if exceeds block limit) + eip1559GasCap uint64 // Gas limit of the highest spending eip1559 transaction } // newTxList create a new transaction list for maintaining nonce-indexable fast, @@ -250,6 +251,7 @@ func (l *txList) Overlaps(tx *types.Transaction) bool { // thresholds are also potentially updated. func (l *txList) Add(tx *types.Transaction, priceBump uint64, baseFee *big.Int) (bool, *types.Transaction) { // If there's an older better transaction, abort + eip1559 := false old := l.txs.Get(tx.Nonce()) if old != nil { // Have to ensure that the new gas price is higher than the old gas @@ -262,6 +264,7 @@ func (l *txList) Add(tx *types.Transaction, priceBump uint64, baseFee *big.Int) return false, nil } } else { + eip1559 = true newGasPrice := new(big.Int).Add(baseFee, tx.GasPremium()) if newGasPrice.Cmp(tx.FeeCap()) > 0 { newGasPrice.Set(tx.FeeCap()) @@ -281,6 +284,7 @@ func (l *txList) Add(tx *types.Transaction, priceBump uint64, baseFee *big.Int) return false, nil } } else { + eip1559 = true newGasPrice := new(big.Int).Add(baseFee, tx.GasPremium()) if newGasPrice.Cmp(tx.FeeCap()) > 0 { newGasPrice.Set(tx.FeeCap()) @@ -296,8 +300,11 @@ func (l *txList) Add(tx *types.Transaction, priceBump uint64, baseFee *big.Int) if cost := tx.Cost(baseFee); l.costcap.Cmp(cost) < 0 { l.costcap = cost } - if gas := tx.Gas(); l.gascap < gas { - l.gascap = gas + if eip1559 && l.eip1559GasCap < tx.Gas() { + l.eip1559GasCap = tx.Gas() + } + if !eip1559 && l.legacyGasCap < tx.Gas() { + l.legacyGasCap = tx.Gas() } return true, old } @@ -318,16 +325,19 @@ func (l *txList) Forward(threshold uint64) types.Transactions { // a point in calculating all the costs or if the balance covers all. If the threshold // is lower than the costgas cap, the caps will be reset to a new high after removing // the newly invalidated transactions. -func (l *txList) Filter(costLimit *big.Int, gasLimit uint64, baseFee *big.Int) (types.Transactions, types.Transactions) { +func (l *txList) Filter(costLimit *big.Int, legacyGasLimit, eip1559GasLimit uint64, baseFee *big.Int) (types.Transactions, types.Transactions) { // If all transactions are below the threshold, short circuit - if l.costcap.Cmp(costLimit) <= 0 && l.gascap <= gasLimit { + if l.costcap.Cmp(costLimit) <= 0 && l.legacyGasCap <= legacyGasLimit && l.eip1559GasCap <= eip1559GasLimit { return nil, nil } l.costcap = new(big.Int).Set(costLimit) // Lower the caps to the thresholds - l.gascap = gasLimit + l.legacyGasCap = legacyGasLimit + l.eip1559GasCap = eip1559GasLimit // Filter out all the transactions above the account's funds - removed := l.txs.Filter(func(tx *types.Transaction) bool { return tx.Cost(baseFee).Cmp(costLimit) > 0 || tx.Gas() > gasLimit }) + removed := l.txs.Filter(func(tx *types.Transaction) bool { + return tx.Cost(baseFee).Cmp(costLimit) > 0 || (tx.GasPrice() != nil && tx.Gas() > legacyGasLimit) || (tx.GasPremium()) != nil && tx.Gas() > eip1559GasLimit + }) // If the list was strict, filter anything above the lowest nonce var invalids types.Transactions diff --git a/core/tx_pool.go b/core/tx_pool.go index f151d282fd9b..905465504180 100644 --- a/core/tx_pool.go +++ b/core/tx_pool.go @@ -64,9 +64,13 @@ var ( // than required to start the invocation. ErrIntrinsicGas = errors.New("intrinsic gas too low") - // ErrGasLimit is returned if a transaction's requested gas limit exceeds the - // maximum allowance of the current block. - ErrGasLimit = errors.New("exceeds block gas limit") + // ErrLegacyGasLimit is returned if a transaction's requested gas limit exceeds the + // maximum allowance of the current block for legacy transactions. + ErrLegacyGasLimit = errors.New("exceeds block gas limit for legacy transactions") + + // ErrEIP1559GasLimit is returned if a transaction's requested gas limit exceeds the + // maximum allowance of the current block for EIP1559 transactions. + ErrEIP1559GasLimit = errors.New("exceeds block gas limit for EIP1559 transactions") // ErrNegativeValue is a sanity error to ensure noone is able to specify a // transaction with a negative value. @@ -239,9 +243,10 @@ type TxPool struct { istanbul bool // Fork indicator whether we are in the istanbul stage. - currentState *state.StateDB // Current state in the blockchain head - pendingNonces *txNoncer // Pending state tracking virtual nonces - currentMaxGas uint64 // Current gas limit for transaction caps + currentState *state.StateDB // Current state in the blockchain head + pendingNonces *txNoncer // Pending state tracking virtual nonces + currentLegacyMaxGas uint64 // Current gas limit for legacy transaction caps + currentEIP1559MaxGas uint64 // Current gas limit for EIP1559 transaction caps locals *accountSet // Set of local transaction to exempt from eviction rules journal *txJournal // Journal of local transaction to back up to disk @@ -567,8 +572,11 @@ func (pool *TxPool) validateTx(tx *types.Transaction, local bool) error { return ErrNegativeValue } // Ensure the transaction doesn't exceed the current block limit gas. - if pool.currentMaxGas < tx.Gas() { - return ErrGasLimit + if tx.GasPrice() != nil && pool.currentLegacyMaxGas < tx.Gas() { + return ErrLegacyGasLimit + } + if tx.GasPrice() == nil && pool.currentEIP1559MaxGas < tx.Gas() { + return ErrEIP1559GasLimit } // Make sure the transaction is signed properly from, err := types.Sender(pool.signer, tx) @@ -1189,7 +1197,14 @@ func (pool *TxPool) reset(oldHead, newHead *types.Header) { } pool.currentState = statedb pool.pendingNonces = newTxNoncer(statedb) - pool.currentMaxGas = newHead.GasLimit + + if pool.chainconfig.IsEIP1559(newHead.Number) { + pool.currentLegacyMaxGas = params.MaxGasEIP1559 - newHead.GasLimit + pool.currentEIP1559MaxGas = newHead.GasLimit + } else { + pool.currentLegacyMaxGas = newHead.GasLimit + pool.currentEIP1559MaxGas = 0 + } // Inject any transactions discarded due to reorgs log.Debug("Reinjecting stale transactions", "count", len(reinject)) @@ -1222,7 +1237,7 @@ func (pool *TxPool) promoteExecutables(accounts []common.Address) []*types.Trans log.Trace("Removed old queued transaction", "hash", hash) } // Drop all transactions that are too costly (low balance or out of gas) - drops, _ := list.Filter(pool.currentState.GetBalance(addr), pool.currentMaxGas, pool.chain.CurrentBlock().BaseFee()) + drops, _ := list.Filter(pool.currentState.GetBalance(addr), pool.currentLegacyMaxGas, pool.currentEIP1559MaxGas, pool.chain.CurrentBlock().BaseFee()) for _, tx := range drops { hash := tx.Hash() pool.all.Remove(hash) @@ -1414,7 +1429,7 @@ func (pool *TxPool) demoteUnexecutables() { log.Trace("Removed old pending transaction", "hash", hash) } // Drop all transactions that are too costly (low balance or out of gas), and queue any invalids back for later - drops, invalids := list.Filter(pool.currentState.GetBalance(addr), pool.currentMaxGas, pool.chain.CurrentBlock().BaseFee()) + drops, invalids := list.Filter(pool.currentState.GetBalance(addr), pool.currentLegacyMaxGas, pool.currentEIP1559MaxGas, pool.chain.CurrentBlock().BaseFee()) for _, tx := range drops { hash := tx.Hash() log.Trace("Removed unpayable pending transaction", "hash", hash) diff --git a/core/tx_pool_test.go b/core/tx_pool_test.go index a78b979121e2..baf81d0a3cad 100644 --- a/core/tx_pool_test.go +++ b/core/tx_pool_test.go @@ -1364,14 +1364,16 @@ func TestTransactionDroppingEIP1559(t *testing.T) { t.Errorf("total transaction mismatch: have %d, want %d", pool.all.Count(), 4) } // Reduce the block gas limit, check that invalidated transactions are dropped + // After EIP1559 initialization, the legacy gas limit is `params.MaxGasEIP1559 - gasLimit` and the EIP1559 gas limit is `gasLimit` + // As such by reducing the `gasLimit` we increase the legacy gas limit, this must be accounted for in these tests (tx1 is not over-gased) pool.chain.(*testBlockChain).gasLimit = 100 <-pool.requestReset(nil, nil) if _, ok := pool.pending[account].txs.items[tx0.Nonce()]; !ok { t.Errorf("funded pending transaction missing: %v", tx0) } - if _, ok := pool.pending[account].txs.items[tx1.Nonce()]; ok { - t.Errorf("over-gased pending transaction present: %v", tx1) + if _, ok := pool.pending[account].txs.items[tx1.Nonce()]; !ok { + t.Errorf("funded pending transaction missing: %v", tx1) } if _, ok := pool.queue[account].txs.items[tx10.Nonce()]; !ok { t.Errorf("funded queued transaction missing: %v", tx10) @@ -1379,8 +1381,8 @@ func TestTransactionDroppingEIP1559(t *testing.T) { if _, ok := pool.queue[account].txs.items[tx11.Nonce()]; ok { t.Errorf("over-gased queued transaction present: %v", tx11) } - if pool.all.Count() != 2 { - t.Errorf("total transaction mismatch: have %d, want %d", pool.all.Count(), 2) + if pool.all.Count() != 3 { + t.Errorf("total transaction mismatch: have %d, want %d", pool.all.Count(), 3) } } diff --git a/light/txpool.go b/light/txpool.go index c57a54ddbddb..1db3277ac0bc 100644 --- a/light/txpool.go +++ b/light/txpool.go @@ -343,14 +343,16 @@ func (pool *TxPool) Stats() (pending int) { // validateTx checks whether a transaction is valid according to the consensus rules. func (pool *TxPool) validateTx(ctx context.Context, tx *types.Transaction) error { + header := pool.chain.GetHeaderByHash(pool.head) + // EIP1559 guards - if pool.config.IsEIP1559(pool.chain.CurrentHeader().Number) && pool.chain.CurrentHeader().BaseFee == nil { + if pool.config.IsEIP1559(header.Number) && header.BaseFee == nil { return core.ErrNoBaseFee } - if pool.config.IsEIP1559Finalized(pool.chain.CurrentHeader().Number) && (tx.GasPremium() == nil || tx.FeeCap() == nil || tx.GasPrice() != nil) { + if pool.config.IsEIP1559Finalized(header.Number) && (tx.GasPremium() == nil || tx.FeeCap() == nil || tx.GasPrice() != nil) { return core.ErrTxNotEIP1559 } - if !pool.config.IsEIP1559(pool.chain.CurrentHeader().Number) && (tx.GasPremium() != nil || tx.FeeCap() != nil || tx.GasPrice() == nil) { + if !pool.config.IsEIP1559(header.Number) && (tx.GasPremium() != nil || tx.FeeCap() != nil || tx.GasPrice() == nil) { return core.ErrTxIsEIP1559 } if tx.GasPrice() != nil && (tx.GasPremium() != nil || tx.FeeCap() != nil) { @@ -379,9 +381,18 @@ func (pool *TxPool) validateTx(ctx context.Context, tx *types.Transaction) error // Check the transaction doesn't exceed the current // block limit gas. - header := pool.chain.GetHeaderByHash(pool.head) - if header.GasLimit < tx.Gas() { - return core.ErrGasLimit + var legacyGasLimit, eip1559GasLimit uint64 + if pool.config.IsEIP1559(header.Number) { + legacyGasLimit = params.MaxGasEIP1559 - header.GasLimit + eip1559GasLimit = header.GasLimit + } else { + legacyGasLimit = header.GasLimit + } + if tx.GasPrice() != nil && legacyGasLimit < tx.Gas() { + return core.ErrLegacyGasLimit + } + if tx.GasPremium() != nil && eip1559GasLimit < tx.Gas() { + return core.ErrEIP1559GasLimit } // Transactions can't be negative. This may never happen @@ -393,7 +404,7 @@ func (pool *TxPool) validateTx(ctx context.Context, tx *types.Transaction) error // Transactor should have enough funds to cover the costs // cost == V + GP * GL - if b := currentState.GetBalance(from); b.Cmp(tx.Cost(pool.chain.CurrentHeader().BaseFee)) < 0 { + if b := currentState.GetBalance(from); b.Cmp(tx.Cost(header.BaseFee)) < 0 { return core.ErrInsufficientFunds } @@ -414,7 +425,7 @@ func (pool *TxPool) add(ctx context.Context, tx *types.Transaction) error { hash := tx.Hash() if pool.pending[hash] != nil { - return fmt.Errorf("Known transaction (%x)", hash[:4]) + return fmt.Errorf("known transaction (%x)", hash[:4]) } err := pool.validateTx(ctx, tx) if err != nil { diff --git a/miner/worker.go b/miner/worker.go index d3412e6dc2cd..0b41af4413f4 100644 --- a/miner/worker.go +++ b/miner/worker.go @@ -465,7 +465,7 @@ func (w *worker) mainLoop() { if !w.chainConfig.IsEIP1559(w.current.header.Number) && legacyGasPool != nil && legacyGasPool.Gas() < params.TxGas { continue } - // When we are between EIP1559 activation and finalization we can received transactions of both types + // When we are between EIP1559 activation and finalization we can receive transactions of both types // and one pool could be exhausted while the other is not // If both pools are exhausted we know the block is full but if only one is we could still accept transactions // of the other type so we need to proceed into commitTransactions() @@ -484,7 +484,7 @@ func (w *worker) mainLoop() { txset := types.NewTransactionsByPriceAndNonce(w.current.signer, txs, w.current.header.BaseFee) tcount := w.current.tcount w.commitTransactions(txset, coinbase, nil) - // Only update the snapshot if any new transactons were added + // Only update the snapshot if any new transactions were added // to the pending block if tcount != w.current.tcount { w.updateSnapshot() @@ -756,11 +756,6 @@ func (w *worker) commitTransactions(txs *types.TransactionsByPriceAndNonce, coin w.current.gasPool = new(core.GasPool).AddGas(legacyGasLimit) } - oneTxType := false - if w.chainConfig.IsEIP1559Finalized(w.current.header.Number) || !w.chainConfig.IsEIP1559(w.current.header.Number) { - oneTxType = true - } - var coalescedLogs []*types.Log for { @@ -769,41 +764,20 @@ func (w *worker) commitTransactions(txs *types.TransactionsByPriceAndNonce, coin if tx == nil { break } + // Set which gasPool to use based on the type of transaction eip1559 := false var gp *core.GasPool - if w.chainConfig.IsEIP1559(w.current.header.Number) && tx.GasPrice() == nil && tx.GasPremium() != nil && tx.FeeCap() != nil { + if w.current.gp1559 != nil && tx.GasPrice() == nil && tx.GasPremium() != nil && tx.FeeCap() != nil { gp = w.current.gp1559 eip1559 = true - } else if !w.chainConfig.IsEIP1559Finalized(w.current.header.Number) && tx.GasPremium() == nil && tx.FeeCap() == nil && tx.GasPrice() != nil { + } else if w.current.gasPool != nil && tx.GasPremium() == nil && tx.FeeCap() == nil && tx.GasPrice() != nil { gp = w.current.gasPool } else { log.Error("Transaction does not conform with expected format (legacy or EIP1559)") continue } - // If we processing both types of transactions then we can break if both pools are exhausted - if w.current.gasPool != nil && w.current.gasPool.Gas() < params.TxGas && w.current.gp1559 != nil && w.current.gp1559.Gas() < params.TxGas { - log.Trace("Not enough gas for further transactions", "have legacy pool", w.current.gasPool, "and eip1559 pool", w.current.gp1559, "want", params.TxGas) - break - } - - // If we don't have enough gas for any further transactions of this type - if gp.Gas() < params.TxGas { - if eip1559 { - log.Trace("Not enough gas for further EIP1559 transactions", "have", gp, "want", params.TxGas) - } else { - log.Trace("Not enough gas for further legacy transactions", "have", gp, "want", params.TxGas) - } - // and this is the only type we are processing, then we're done - if oneTxType { - break - } - // Otherwise if only the current pool is exhausted we need to continue - // in case some of the subsequent transactions are for the non-exhausted pool - continue - } - // In the following three cases, we will interrupt the execution of the transaction. // (1) new head block event arrival, the interrupt signal is 1 // (2) worker start or restart, the interrupt signal is 1 @@ -830,6 +804,22 @@ func (w *worker) commitTransactions(txs *types.TransactionsByPriceAndNonce, coin return atomic.LoadInt32(interrupt) == commitInterruptNewHead } + // If all available gas pools are empty, break + if (w.current.gasPool == nil || w.current.gasPool.Gas() < params.TxGas) && (w.current.gp1559 == nil || w.current.gp1559.Gas() < params.TxGas) { + log.Trace("Not enough gas for further transactions", "have legacy pool", w.current.gasPool, "and eip1559 pool", w.current.gp1559, "want", params.TxGas) + break + } + + // If just the current gas pool is empty, continue + if gp.Gas() < params.TxGas { + if eip1559 { + log.Trace("Not enough gas for further EIP1559 transactions", "have", gp, "want", params.TxGas) + } else { + log.Trace("Not enough gas for further legacy transactions", "have", gp, "want", params.TxGas) + } + continue + } + // Error may be ignored here. The error has already been checked // during transaction acceptance is the transaction pool. //