Cherry pick tx pool fixes from v1.2.3 and v1.2.4 (ethereum#1442)

* Make tx pool fee check logic match the logic in state_transition.go

* Improve non-native currency fee tx handling (ethereum#2)

Improves multi-currency fee support in the tx pool queue and pending lists.
Previously when paying for a tx in a non-native currency, the balance of the account
would only be checked when inserting the transaction into the tx pool, but as the
balance of the account can change after the tx is admitted, transactions are periodically
checked for validity in `txList.Filter`. This PR changes txList to properly track balances in
non-native currencies. This also adds support for checking the gas price minimum in the txList.

This PR also converts gas prices to CELO prior to comparing if a new transaction at the same
nonce should override an old transaction at the same nonce. It also refactors calculations
of `tx.Fee()`

* Fix nil pointer exception in txList Filter

This occurred because gasPriceMinimums contains the gasPriceMinimum for
every fee currency, not just the fee currencies of the transfer. This
changes the iteration to go through each gasPriceFloor and then get the
gasPriceMinimum from the fee currency associated with the floor. This works
because gasPriceMinimums contains every whitelisted fee currency and the tx
has already been validated to pay in a fee currency.

* Fix panic in 'ConvertToGold()'

Co-authored-by: Joshua Gutow <joshua@celo.org>
Co-authored-by: Joshua Gutow <joshua@clabs.co>

contract_comm/currency/currency.go

@@ -108,19 +108,25 @@ type exchangeRate struct {
 }
 
 func ConvertToGold(val *big.Int, currencyFrom *common.Address) (*big.Int, error) {
+	if currencyFrom == nil {
+		return val, nil
+	}
+
 	celoGoldAddress, err := contract_comm.GetRegisteredAddress(params.GoldTokenRegistryId, nil, nil)
 	if err == errors.ErrSmartContractNotDeployed || err == errors.ErrRegistryContractNotDeployed {
 		log.Warn("Registry address lookup failed", "err", err)
 		return val, err
 	}
 
 	if currencyFrom == celoGoldAddress {
-		return val, err
+		// This function shouldn't really be called with the token's address, but if it is the value
+		// is correct, so return nil as the error
+		return val, nil
 	} else if err != nil {
 		log.Error(err.Error())
 		return val, err
 	}
-	return Convert(val, currencyFrom, celoGoldAddress)
+	return Convert(val, currencyFrom, nil)
 }
 
 // NOTE (jarmg 4/24/19): values are rounded down which can cause
@@ -143,6 +149,12 @@ func Convert(val *big.Int, currencyFrom *common.Address, currencyTo *common.Addr
 	// (val * d1 * n2) / (n1 * d2)
 	numerator := new(big.Int).Mul(val, new(big.Int).Mul(exchangeRateFrom.Denominator, exchangeRateTo.Numerator))
 	denominator := new(big.Int).Mul(exchangeRateFrom.Numerator, exchangeRateTo.Denominator)
+
+	if denominator.Sign() == 0 {
+		log.Error("Convert - Error in retreiving currency exchange rates")
+		return nil, errors.ErrExchangeRateZero
+	}
+
 	return new(big.Int).Div(numerator, denominator), nil
 }
 

contract_comm/errors/errors.go

@@ -9,4 +9,5 @@ var (
 	ErrSmartContractNotDeployed      = errors.New("Contract not in Registry")
 	ErrRegistryContractNotDeployed   = errors.New("Registry not deployed")
 	ErrNoInternalEvmHandlerSingleton = errors.New("No internalEvmHandlerSingleton set for contract communication")
+	ErrExchangeRateZero              = errors.New("Exchange rate returned from the network is zero")
 )

core/tx_list.go

@@ -224,17 +224,23 @@ 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)
+	nativecostcap       *big.Int                    // Price of the highest costing transaction paid with native fees (reset only if exceeds balance)
+	feecaps             map[common.Address]*big.Int // Price of the highest costing transaction per fee currency (reset only if exceeds balance)
+	nativegaspricefloor *big.Int                    // Lowest gas price minimum in the native currency
+	gaspricefloors      map[common.Address]*big.Int // Lowest gas price minimum per currency (reset only if it is below the gpm)
+	gascap              uint64                      // Gas limit of the highest spending transaction (reset only if exceeds block limit)
 }
 
 // newTxList create a new transaction list for maintaining nonce-indexable fast,
 // gapped, sortable transaction lists.
 func newTxList(strict bool) *txList {
 	return &txList{
-		strict:  strict,
-		txs:     newTxSortedMap(),
-		costcap: new(big.Int),
+		strict:              strict,
+		txs:                 newTxSortedMap(),
+		nativecostcap:       new(big.Int),
+		feecaps:             make(map[common.Address]*big.Int),
+		nativegaspricefloor: nil,
+		gaspricefloors:      make(map[common.Address]*big.Int),
 	}
 }
 
@@ -244,27 +250,75 @@ func (l *txList) Overlaps(tx *types.Transaction) bool {
 	return l.txs.Get(tx.Nonce()) != nil
 }
 
+// FeeCurrencies returns a list of each fee currency used to pay for gas in the txList
+func (l *txList) FeeCurrencies() []common.Address {
+	var feeCurrencies []common.Address
+	for feeCurrency := range l.feecaps {
+		feeCurrencies = append(feeCurrencies, feeCurrency)
+	}
+	return feeCurrencies
+}
+
 // Add tries to insert a new transaction into the list, returning whether the
 // transaction was accepted, and if yes, any previous transaction it replaced.
 //
-// If the new transaction is accepted into the list, the lists' cost and gas
-// thresholds are also potentially updated.
+// If the new transaction is accepted into the list, the lists' cost, gas and
+// gasPriceMinimum thresholds are also potentially updated.
 func (l *txList) Add(tx *types.Transaction, priceBump uint64) (bool, *types.Transaction) {
 	// If there's an older better transaction, abort
 	old := l.txs.Get(tx.Nonce())
+	var err error
 	if old != nil {
-		threshold := new(big.Int).Div(new(big.Int).Mul(old.GasPrice(), big.NewInt(100+int64(priceBump))), big.NewInt(100))
+		var oldPrice, newPrice *big.Int
+		// Short circuit conversion if both are the same currency
+		if old.FeeCurrency() == tx.FeeCurrency() {
+			oldPrice = old.GasPrice()
+			newPrice = tx.GasPrice()
+		} else {
+			if fc := old.FeeCurrency(); fc != nil {
+				if oldPrice, err = currency.ConvertToGold(old.GasPrice(), fc); err != nil {
+					return false, nil
+				}
+			} else {
+				oldPrice = old.GasPrice()
+			}
+			if fc := tx.FeeCurrency(); fc != nil {
+				if newPrice, err = currency.ConvertToGold(tx.GasPrice(), fc); err != nil {
+					return false, nil
+				}
+			} else {
+				newPrice = tx.GasPrice()
+			}
+		}
+		threshold := new(big.Int).Div(new(big.Int).Mul(oldPrice, big.NewInt(100+int64(priceBump))), big.NewInt(100))
 		// Have to ensure that the new gas price is higher than the old gas
 		// price as well as checking the percentage threshold to ensure that
 		// this is accurate for low (Wei-level) gas price replacements
-		if old.GasPrice().Cmp(tx.GasPrice()) >= 0 || threshold.Cmp(tx.GasPrice()) > 0 {
+		if oldPrice.Cmp(newPrice) >= 0 || threshold.Cmp(newPrice) > 0 {
 			return false, nil
 		}
 	}
 	// Otherwise overwrite the old transaction with the current one
+	// caps can only increase and floors can only decrease in this function
 	l.txs.Put(tx)
-	if cost := tx.Cost(); l.costcap.Cmp(cost) < 0 {
-		l.costcap = cost
+	if feeCurrency := tx.FeeCurrency(); feeCurrency == nil {
+		if cost := tx.Cost(); l.nativecostcap.Cmp(cost) < 0 {
+			l.nativecostcap = cost
+		}
+		if gasPrice := tx.GasPrice(); l.nativegaspricefloor == nil || l.nativegaspricefloor.Cmp(gasPrice) > 0 {
+			l.nativegaspricefloor = gasPrice
+		}
+	} else {
+		fee := tx.Fee()
+		if oldFee, ok := l.feecaps[*feeCurrency]; !ok || oldFee.Cmp(fee) < 0 {
+			l.feecaps[*feeCurrency] = fee
+		}
+		if gasFloor, ok := l.gaspricefloors[*feeCurrency]; !ok || gasFloor.Cmp(tx.GasPrice()) > 0 {
+			l.gaspricefloors[*feeCurrency] = tx.GasPrice()
+		}
+		if value := tx.Value(); l.nativecostcap.Cmp(value) < 0 {
+			l.nativecostcap = value
+		}
 	}
 	if gas := tx.Gas(); l.gascap < gas {
 		l.gascap = gas
@@ -287,29 +341,99 @@ func (l *txList) Forward(threshold uint64) types.Transactions {
 // This method uses the cached costcap and gascap to quickly decide if there's even
 // 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) (types.Transactions, types.Transactions) {
-	if costLimit == nil {
-		costLimit = l.costcap
+func (l *txList) Filter(nativeCostLimit, nativeGasPriceMinimum *big.Int, feeLimits, gasPriceMinimums map[common.Address]*big.Int, gasLimit uint64) (types.Transactions, types.Transactions) {
+	// native gas price floor is not necessarily set in txList.Add unlike the rest of caps/floors
+	if l.nativegaspricefloor == nil {
+		l.nativegaspricefloor = new(big.Int).Set(nativeGasPriceMinimum)
+	}
+	// check if we can bail & lower caps & raise floors at the same time
+	canBail := true
+	// Ensure that the cost cap <= the cost limit
+	if l.nativecostcap.Cmp(nativeCostLimit) > 0 {
+		canBail = false
+		l.nativecostcap = new(big.Int).Set(nativeCostLimit)
+	}
+	// Ensure that native gas price floor >= the native gas price minimum
+	if l.nativegaspricefloor.Cmp(nativeGasPriceMinimum) < 0 {
+		canBail = false
+		l.nativegaspricefloor = new(big.Int).Set(nativeGasPriceMinimum)
+	}
+	// Ensure that the gas cap <= the gas limit
+	if l.gascap > gasLimit {
+		canBail = false
+		l.gascap = gasLimit
+	}
+	// Ensure that each cost cap <= the per currency cost limit.
+	for feeCurrency, feeLimit := range feeLimits {
+		if l.feecaps[feeCurrency].Cmp(feeLimit) > 0 {
+			canBail = false
+			l.feecaps[feeCurrency] = new(big.Int).Set(feeLimit)
+		}
 	}
-	// If all transactions are below the threshold, short circuit
-	if l.costcap.Cmp(costLimit) <= 0 && l.gascap <= gasLimit {
+	// Ensure that each gas price floor >= the gas price minimum.
+	for feeCurrency, gasPriceFloor := range l.gaspricefloors {
+		if gasPriceMinimum := gasPriceMinimums[feeCurrency]; gasPriceFloor.Cmp(gasPriceMinimum) < 0 {
+			canBail = false
+			l.gaspricefloors[feeCurrency] = new(big.Int).Set(gasPriceMinimum)
+		}
+	}
+	if canBail {
 		return nil, nil
 	}
-	l.costcap = new(big.Int).Set(costLimit) // Lower the caps to the thresholds
-	l.gascap = gasLimit
 
 	// Filter out all the transactions above the account's funds
 	removed := l.txs.Filter(func(tx *types.Transaction) bool {
-		if tx.FeeCurrency() == nil {
-			log.Trace("Transaction Filter", "hash", tx.Hash(), "Fee currency", tx.FeeCurrency(), "Cost", tx.Cost(), "Cost Limit", costLimit, "Gas", tx.Gas(), "Gas Limit", gasLimit)
-			return tx.Cost().Cmp(costLimit) > 0 || tx.Gas() > gasLimit
+		if feeCurrency := tx.FeeCurrency(); feeCurrency == nil {
+			log.Trace("Transaction Filter", "hash", tx.Hash(), "Fee currency", tx.FeeCurrency(), "Cost", tx.Cost(), "Cost Limit", nativeCostLimit, "Gas", tx.Gas(), "Gas Limit", gasLimit)
+			return tx.Cost().Cmp(nativeCostLimit) > 0 || tx.Gas() > gasLimit || tx.GasPrice().Cmp(nativeGasPriceMinimum) < 0
 		} else {
-			// If the fees are being paid in the non-native currency, ensure that the `tx.Value` is less than costLimit
-			// as the fees will be deducted in the non-native currency.
-			log.Trace("Transaction Filter", "hash", tx.Hash(), "Fee currency", tx.FeeCurrency(), "Value", tx.Value(), "Cost Limit", costLimit, "Gas", tx.Gas(), "Gas Limit", gasLimit)
-			return tx.Value().Cmp(costLimit) > 0 || tx.Gas() > gasLimit
+			feeLimit := feeLimits[*feeCurrency]
+			fee := tx.Fee()
+			log.Trace("Transaction Filter", "hash", tx.Hash(), "Fee currency", tx.FeeCurrency(), "Value", tx.Value(), "Cost Limit", feeLimit, "Gas", tx.Gas(), "Gas Limit", gasLimit)
+			// If any of the following is true, the transaction is invalid
+			// The fees are greater than or equal to the balance in the currency
+			return fee.Cmp(feeLimit) >= 0 ||
+				// The value of the tx is greater than the native balance of the account
+				tx.Value().Cmp(nativeCostLimit) > 0 ||
+				// The gas price is less than the gas price minimum
+				tx.GasPrice().Cmp(gasPriceMinimums[*feeCurrency]) < 0 ||
+				// The gas used is greater than the gas limit
+				tx.Gas() > gasLimit
+		}
+	})
+
+	// If the list was strict, filter anything above the lowest nonce
+	var invalids types.Transactions
+
+	if l.strict && len(removed) > 0 {
+		lowest := uint64(math.MaxUint64)
+		for _, tx := range removed {
+			if nonce := tx.Nonce(); lowest > nonce {
+				lowest = nonce
+			}
 		}
+		invalids = l.txs.Filter(func(tx *types.Transaction) bool { return tx.Nonce() > lowest })
+	}
+	return removed, invalids
+}
+
+// FilterOnGasLimit removes all transactions from the list with a gas limit higher
+// than the provided thresholds. Every removed transaction is returned for any
+// post-removal maintenance. Strict-mode invalidated transactions are also
+// returned.
+//
+// This method uses the cached gascap to quickly decide if there's even
+// a point in calculating all the gas used
+func (l *txList) FilterOnGasLimit(gasLimit uint64) (types.Transactions, types.Transactions) {
+	// We can bail if the gas cap <= the gas limit
+	if l.gascap <= gasLimit {
+		return nil, nil
+	}
+	l.gascap = gasLimit
+
+	// Filter out all the transactions above the account's funds
+	removed := l.txs.Filter(func(tx *types.Transaction) bool {
+		return tx.Gas() > gasLimit
 	})
 
 	// If the list was strict, filter anything above the lowest nonce

core/tx_pool.go

@@ -480,7 +480,7 @@ func (pool *TxPool) SetGasLimit(gasLimit uint64) {
 	pool.demoteUnexecutables()
 
 	for _, list := range pool.queue {
-		rm, _ := list.Filter(nil, gasLimit)
+		rm, _ := list.FilterOnGasLimit(gasLimit)
 		for _, tx := range rm {
 			pool.removeTx(tx.Hash(), false)
 		}
@@ -1280,6 +1280,14 @@ func (pool *TxPool) reset(oldHead, newHead *types.Header) {
 func (pool *TxPool) promoteExecutables(accounts []common.Address) []*types.Transaction {
 	// Track the promoted transactions to broadcast them at once
 	var promoted []*types.Transaction
+	// build gas price minimums
+	gasPriceMinimums := make(map[common.Address]*big.Int)
+	allCurrencies, _ := currency.CurrencyWhitelist(nil, nil)
+	for _, currency := range allCurrencies {
+		gasPriceMinimum, _ := gpm.GetGasPriceMinimum(&currency, nil, nil)
+		gasPriceMinimums[currency] = gasPriceMinimum
+	}
+	nativeGPM, _ := gpm.GetGasPriceMinimum(nil, nil, nil)
 
 	// Iterate over all accounts and promote any executable transactions
 	for _, addr := range accounts {
@@ -1294,8 +1302,15 @@ func (pool *TxPool) promoteExecutables(accounts []common.Address) []*types.Trans
 			pool.all.Remove(hash)
 			log.Trace("Removed old queued transaction", "hash", hash)
 		}
+		// Get balances in each currency
+		balances := make(map[common.Address]*big.Int)
+		allCurrencies := list.FeeCurrencies()
+		for _, feeCurrency := range allCurrencies {
+			feeCurrencyBalance, _, _ := currency.GetBalanceOf(addr, feeCurrency, params.MaxGasToReadErc20Balance, nil, nil)
+			balances[feeCurrency] = feeCurrencyBalance
+		}
 		// Drop all transactions that are too costly (low balance or out of gas)
-		drops, _ := list.Filter(pool.currentState.GetBalance(addr), pool.currentMaxGas)
+		drops, _ := list.Filter(pool.currentState.GetBalance(addr), nativeGPM, balances, gasPriceMinimums, pool.currentMaxGas)
 		for _, tx := range drops {
 			hash := tx.Hash()
 			pool.all.Remove(hash)
@@ -1476,6 +1491,15 @@ func (pool *TxPool) truncateQueue() {
 // executable/pending queue and any subsequent transactions that become unexecutable
 // are moved back into the future queue.
 func (pool *TxPool) demoteUnexecutables() {
+	// build gas price minimums
+	gasPriceMinimums := make(map[common.Address]*big.Int)
+	allCurrencies, _ := currency.CurrencyWhitelist(nil, nil)
+	for _, currency := range allCurrencies {
+		gasPriceMinimum, _ := gpm.GetGasPriceMinimum(&currency, nil, nil)
+		gasPriceMinimums[currency] = gasPriceMinimum
+	}
+	nativeGPM, _ := gpm.GetGasPriceMinimum(nil, nil, nil)
+
 	// Iterate over all accounts and demote any non-executable transactions
 	for addr, list := range pool.pending {
 		nonce := pool.currentState.GetNonce(addr)
@@ -1487,8 +1511,15 @@ func (pool *TxPool) demoteUnexecutables() {
 			pool.all.Remove(hash)
 			log.Trace("Removed old pending transaction", "hash", hash)
 		}
+		// Get balances in each currency
+		balances := make(map[common.Address]*big.Int)
+		allCurrencies := list.FeeCurrencies()
+		for _, feeCurrency := range allCurrencies {
+			feeCurrencyBalance, _, _ := currency.GetBalanceOf(addr, feeCurrency, params.MaxGasToReadErc20Balance, nil, nil)
+			balances[feeCurrency] = feeCurrencyBalance
+		}
 		// 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)
+		drops, invalids := list.Filter(pool.currentState.GetBalance(addr), nativeGPM, balances, gasPriceMinimums, pool.currentMaxGas)
 		for _, tx := range drops {
 			hash := tx.Hash()
 			log.Trace("Removed unpayable pending transaction", "hash", hash)
@@ -1539,8 +1570,10 @@ func ValidateTransactorBalanceCoversTx(tx *types.Transaction, from common.Addres
 			return err
 		}
 
-		gasFee := new(big.Int).Mul(tx.GasPrice(), big.NewInt(int64(tx.Gas())))
-		if feeCurrencyBalance.Cmp(new(big.Int).Add(gasFee, tx.GatewayFee())) < 0 {
+		// To match the logic in canPayFee() state_transition.go, we require the balance to be strictly greater than the fee,
+		// which means we reject the transaction if balance <= fee
+		fee := tx.Fee()
+		if feeCurrencyBalance.Cmp(fee) <= 0 {
 			log.Debug("validateTx insufficient fee currency", "feeCurrency", tx.FeeCurrency(), "feeCurrencyBalance", feeCurrencyBalance)
 			return ErrInsufficientFunds
 		}

core/types/transaction.go

@@ -193,6 +193,10 @@ func (tx *Transaction) GatewayFee() *big.Int                 { return tx.data.Ga
 func (tx *Transaction) Value() *big.Int                      { return new(big.Int).Set(tx.data.Amount) }
 func (tx *Transaction) Nonce() uint64                        { return tx.data.AccountNonce }
 func (tx *Transaction) CheckNonce() bool                     { return true }
+func (tx *Transaction) Fee() *big.Int {
+	gasFee := new(big.Int).Mul(tx.data.Price, big.NewInt(int64(tx.data.GasLimit)))
+	return gasFee.Add(gasFee, tx.data.GatewayFee)
+}
 
 // To returns the recipient address of the transaction.
 // It returns nil if the transaction is a contract creation.
@@ -461,3 +465,7 @@ func (m Message) Gas() uint64                          { return m.gasLimit }
 func (m Message) Nonce() uint64                        { return m.nonce }
 func (m Message) Data() []byte                         { return m.data }
 func (m Message) CheckNonce() bool                     { return m.checkNonce }
+func (m Message) Fee() *big.Int {
+	gasFee := new(big.Int).Mul(m.gasPrice, big.NewInt(int64(m.gasLimit)))
+	return gasFee.Add(gasFee, m.gatewayFee)
+}