Skip to content

Commit

Permalink
Merge pull request #26 from vulcanize/fixes
Browse files Browse the repository at this point in the history 
fix trx pool gas limits (light and normal tx pools); cleanup commitTransactions()
  • Loading branch information
@i-norden
i-norden authored Jan 9, 2020
2 parents 8f8209d + 3367f26 commit 03270d7
Show file tree
Hide file tree
Showing 5 changed files with 90 additions and 62 deletions.
26 changes: 18 additions & 8 deletions core/tx_list.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand All @@ -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())
Expand All @@ -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())
Expand All @@ -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
}
Expand All @@ -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
Expand Down
37 changes: 26 additions & 11 deletions core/tx_pool.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
10 changes: 6 additions & 4 deletions core/tx_pool_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1364,23 +1364,25 @@ 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)
}
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)
}
}

Expand Down
27 changes: 19 additions & 8 deletions light/txpool.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand All @@ -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
}

Expand All @@ -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 {
Expand Down
52 changes: 21 additions & 31 deletions miner/worker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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()
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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.
//
Expand Down

0 comments on commit 03270d7

Please sign in to comment.