Skip to content

Commit

Permalink
Fix fee currency usage in newTxWithMinerFee
Browse files Browse the repository at this point in the history
Tip and base fee handling was not aware of fee currencies in that
function, causing wrong tx handling inside the tx pool.
  • Loading branch information
palango authored and karlb committed Sep 27, 2024
1 parent 95bea8d commit cdc399d
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 19 deletions.
42 changes: 29 additions & 13 deletions miner/ordering.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"math/big"

"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/common/exchange"
"github.com/ethereum/go-ethereum/core/txpool"
"github.com/ethereum/go-ethereum/core/types"
"github.com/holiman/uint256"
Expand All @@ -36,17 +37,30 @@ type txWithMinerFee struct {
// newTxWithMinerFee creates a wrapped transaction, calculating the effective
// miner gasTipCap if a base fee is provided.
// Returns error in case of a negative effective miner gasTipCap.
func newTxWithMinerFee(tx *txpool.LazyTransaction, from common.Address, baseFee *uint256.Int) (*txWithMinerFee, error) {
func newTxWithMinerFee(tx *txpool.LazyTransaction, from common.Address, baseFee *uint256.Int, rates common.ExchangeRates) (*txWithMinerFee, error) {
tip := new(uint256.Int).Set(tx.GasTipCap)
if baseFee != nil {
if tx.GasFeeCap.Cmp(baseFee) < 0 {
baseFeeConverted := baseFee
if tx.FeeCurrency != nil {
baseFeeBig, _ := exchange.ConvertCeloToCurrency(rates, tx.FeeCurrency, baseFee.ToBig())
baseFeeConverted = uint256.MustFromBig(baseFeeBig)
}

if tx.GasFeeCap.Cmp(baseFeeConverted) < 0 {
return nil, types.ErrGasFeeCapTooLow
}
tip = new(uint256.Int).Sub(tx.GasFeeCap, baseFee)
if tip.Gt(tx.GasTipCap) {
tip = tx.GasTipCap
}
}

// Convert tip back into celo if the transaction is in a different currency
if tx.FeeCurrency != nil {
tipBig, _ := exchange.ConvertCurrencyToCelo(rates, tip.ToBig(), tx.FeeCurrency)
tip = uint256.MustFromBig(tipBig)
}

return &txWithMinerFee{
tx: tx,
from: from,
Expand Down Expand Up @@ -87,18 +101,19 @@ func (s *txByPriceAndTime) Pop() interface{} {
// transactions in a profit-maximizing sorted order, while supporting removing
// entire batches of transactions for non-executable accounts.
type transactionsByPriceAndNonce struct {
txs map[common.Address][]*txpool.LazyTransaction // Per account nonce-sorted list of transactions
heads txByPriceAndTime // Next transaction for each unique account (price heap)
signer types.Signer // Signer for the set of transactions
baseFee *uint256.Int // Current base fee
txs map[common.Address][]*txpool.LazyTransaction // Per account nonce-sorted list of transactions
heads txByPriceAndTime // Next transaction for each unique account (price heap)
signer types.Signer // Signer for the set of transactions
baseFee *uint256.Int // Current base fee
exchangeRates common.ExchangeRates
}

// newTransactionsByPriceAndNonce creates a transaction set that can retrieve
// price sorted transactions in a nonce-honouring way.
//
// Note, the input map is reowned so the caller should not interact any more with
// if after providing it to the constructor.
func newTransactionsByPriceAndNonce(signer types.Signer, txs map[common.Address][]*txpool.LazyTransaction, baseFee *big.Int) *transactionsByPriceAndNonce {
func newTransactionsByPriceAndNonce(signer types.Signer, txs map[common.Address][]*txpool.LazyTransaction, baseFee *big.Int, rates common.ExchangeRates) *transactionsByPriceAndNonce {
// Convert the basefee from header format to uint256 format
var baseFeeUint *uint256.Int
if baseFee != nil {
Expand All @@ -107,7 +122,7 @@ func newTransactionsByPriceAndNonce(signer types.Signer, txs map[common.Address]
// Initialize a price and received time based heap with the head transactions
heads := make(txByPriceAndTime, 0, len(txs))
for from, accTxs := range txs {
wrapped, err := newTxWithMinerFee(accTxs[0], from, baseFeeUint)
wrapped, err := newTxWithMinerFee(accTxs[0], from, baseFeeUint, rates)
if err != nil {
delete(txs, from)
continue
Expand All @@ -119,10 +134,11 @@ func newTransactionsByPriceAndNonce(signer types.Signer, txs map[common.Address]

// Assemble and return the transaction set
return &transactionsByPriceAndNonce{
txs: txs,
heads: heads,
signer: signer,
baseFee: baseFeeUint,
txs: txs,
heads: heads,
signer: signer,
baseFee: baseFeeUint,
exchangeRates: rates,
}
}

Expand All @@ -138,7 +154,7 @@ func (t *transactionsByPriceAndNonce) Peek() (*txpool.LazyTransaction, *uint256.
func (t *transactionsByPriceAndNonce) Shift() {
acc := t.heads[0].from
if txs, ok := t.txs[acc]; ok && len(txs) > 0 {
if wrapped, err := newTxWithMinerFee(txs[0], acc, t.baseFee); err == nil {
if wrapped, err := newTxWithMinerFee(txs[0], acc, t.baseFee, t.exchangeRates); err == nil {
t.heads[0], t.txs[acc] = wrapped, txs[1:]
heap.Fix(&t.heads, 0)
return
Expand Down
4 changes: 2 additions & 2 deletions miner/ordering_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ func testTransactionPriceNonceSort(t *testing.T, baseFee *big.Int) {
expectedCount += count
}
// Sort the transactions and cross check the nonce ordering
txset := newTransactionsByPriceAndNonce(signer, groups, baseFee)
txset := newTransactionsByPriceAndNonce(signer, groups, baseFee, nil)

txs := types.Transactions{}
for tx, _ := txset.Peek(); tx != nil; tx, _ = txset.Peek() {
Expand Down Expand Up @@ -168,7 +168,7 @@ func TestTransactionTimeSort(t *testing.T) {
})
}
// Sort the transactions and cross check the nonce ordering
txset := newTransactionsByPriceAndNonce(signer, groups, nil)
txset := newTransactionsByPriceAndNonce(signer, groups, nil, nil)

txs := types.Transactions{}
for tx, _ := txset.Peek(); tx != nil; tx, _ = txset.Peek() {
Expand Down
10 changes: 6 additions & 4 deletions miner/worker.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ type environment struct {
gasPool *core.GasPool // available gas used to pack transactions
multiGasPool *core.MultiGasPool // available per-fee-currency gas used to pack transactions
feeCurrencyAllowlist common.AddressSet
exchangeRates common.ExchangeRates
coinbase common.Address

header *types.Header
Expand Down Expand Up @@ -273,6 +274,7 @@ func (miner *Miner) prepareWork(genParams *generateParams) (*environment, error)
common.CurrencyAllowlist(context.FeeCurrencyContext.ExchangeRates),
header,
)
env.exchangeRates = context.FeeCurrencyContext.ExchangeRates
if header.ParentBeaconRoot != nil {
vmenv := vm.NewEVM(context, vm.TxContext{}, env.state, miner.chainConfig, vm.Config{})
core.ProcessBeaconBlockRoot(*header.ParentBeaconRoot, vmenv, env.state)
Expand Down Expand Up @@ -554,16 +556,16 @@ func (miner *Miner) fillTransactions(interrupt *atomic.Int32, env *environment)
}
// Fill the block with all available pending transactions.
if len(localPlainTxs) > 0 || len(localBlobTxs) > 0 {
plainTxs := newTransactionsByPriceAndNonce(env.signer, localPlainTxs, env.header.BaseFee)
blobTxs := newTransactionsByPriceAndNonce(env.signer, localBlobTxs, env.header.BaseFee)
plainTxs := newTransactionsByPriceAndNonce(env.signer, localPlainTxs, env.header.BaseFee, env.exchangeRates)
blobTxs := newTransactionsByPriceAndNonce(env.signer, localBlobTxs, env.header.BaseFee, env.exchangeRates)

if err := miner.commitTransactions(env, plainTxs, blobTxs, interrupt); err != nil {
return err
}
}
if len(remotePlainTxs) > 0 || len(remoteBlobTxs) > 0 {
plainTxs := newTransactionsByPriceAndNonce(env.signer, remotePlainTxs, env.header.BaseFee)
blobTxs := newTransactionsByPriceAndNonce(env.signer, remoteBlobTxs, env.header.BaseFee)
plainTxs := newTransactionsByPriceAndNonce(env.signer, remotePlainTxs, env.header.BaseFee, env.exchangeRates)
blobTxs := newTransactionsByPriceAndNonce(env.signer, remoteBlobTxs, env.header.BaseFee, env.exchangeRates)

if err := miner.commitTransactions(env, plainTxs, blobTxs, interrupt); err != nil {
return err
Expand Down

0 comments on commit cdc399d

Please sign in to comment.