Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EIP-1559 tx pool support #22898

Merged
merged 35 commits into from
May 28, 2021
Merged
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
6b16cc8
core/types: add 1559 compatibility to Transaction helpers
adietrichs Feb 5, 2021
4eac968
core/txpool: update TxPool for EIP-1559 compatibility
adietrichs Feb 5, 2021
ba113ee
core/txpool: add Tip as secondary comparison criterion on txPricedLis…
adietrichs Feb 15, 2021
3c1803d
core/txpool: add ErrTipAboveFeeCap error and respective check during …
adietrichs Feb 15, 2021
3502aa7
core/txpool: refactor: create txLookup.RemotesBelowTip() to facilitat…
adietrichs Feb 15, 2021
391db45
core/txpool: tests: add dynamicFeeTransaction() helper function
adietrichs Feb 15, 2021
661731b
core/txpool: tests: create TestTransactionTypeNotSupported()
adietrichs Feb 15, 2021
3596737
core/txpool: tests: create TestTransactionTipAboveFeeCap()
adietrichs Feb 15, 2021
a3d9640
core/txpool: tests: create TestTransactionPoolRepricingDynamicFee()
adietrichs Feb 15, 2021
b7fcf62
core/txpool: tests: add dynamic fee transactions to TestTransactionPo…
adietrichs Feb 15, 2021
667aea2
core/txpool: tests: create TestTransactionPoolUnderpricingDynamicFee()
adietrichs Feb 15, 2021
86411f3
core/txpool: tests: create TestTransactionReplacementDynamicFee()
adietrichs Feb 15, 2021
8503b0b
core/tx_pool_test: update signer and remove invalid type test
lightclient Apr 6, 2021
f745568
core/tx_pool_test: rename dynamicFeeTransaction to dynamicFeeTx
lightclient Apr 27, 2021
0f30d65
core: remove tx_list cap function
lightclient Apr 29, 2021
b68ca25
core/tx_pool: revert log change and simplify test
lightclient Apr 29, 2021
3b3666e
core/types/transaction: add godoc for tip and fee cap cmp functions
lightclient Apr 29, 2021
b192e2b
core/tx_pool_test: increase test space for repricing test
lightclient Apr 29, 2021
17a7392
core/tx_pool_test: clean up tx pool tests
lightclient Apr 29, 2021
0139181
core/tx_pool_test: rewrite underpriced txs test
lightclient Apr 30, 2021
082de1f
core/tx_pool: fix comment
lightclient May 3, 2021
3752760
core/tx_pool: update aleut references to london
lightclient May 6, 2021
eb38e01
core/tx_pool: remove gas target references
lightclient May 10, 2021
047e149
core/tx_list: require both fee cap and tip be bumped to replace exist…
lightclient May 14, 2021
88ad464
core: dual heap transaction eviction ordering
zsfelfoldi May 18, 2021
702b9af
core: add dual heap tx eviction test
zsfelfoldi May 20, 2021
a6cc15b
core: minor tx pool changes
zsfelfoldi May 20, 2021
cf1f32c
core/types: fix rebase problems
holiman May 21, 2021
e852c14
core: removed obsolete functions, added logs
zsfelfoldi May 22, 2021
be64f2f
core: add timer for reheap operation
zsfelfoldi May 24, 2021
238da13
core: reject dynamic fee tx before 1559 is activated
zsfelfoldi May 25, 2021
4ad2bed
core: make queue capacity ratio adjustable, increase default queue size
zsfelfoldi May 26, 2021
dd5f811
core: simplify setupTxPool
zsfelfoldi May 27, 2021
4a66ab6
core: fix Underpriced
zsfelfoldi May 27, 2021
f35555a
core: fix concurrent map read/write in tx pool tests
zsfelfoldi May 28, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
203 changes: 132 additions & 71 deletions core/tx_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"math"
"math/big"
"sort"
"time"

"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/core/types"
Expand Down Expand Up @@ -279,15 +280,23 @@ func (l *txList) Add(tx *types.Transaction, priceBump uint64) (bool, *types.Tran
// If there's an older better transaction, abort
old := l.txs.Get(tx.Nonce())
if old != nil {
// threshold = oldGP * (100 + priceBump) / 100
if old.FeeCapCmp(tx) >= 0 || old.TipCmp(tx) >= 0 {
Copy link
Member

@rjl493456442 rjl493456442 May 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this strategy, we can only replace the transaction with 10% feeCap bump and 10% tip bump. It might hurt the UX to some extent.

For example, if the user already specify a high enough feeCap but a barely enough tip. And he wants to speed up the inclusion, he has to bump the feeCap as well. If the basefee raises up, then the maximum possible payment is increased.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been discussed at the 1559 dev channel, right now this replacement condition seems to be the best choice.

return false, nil
}
// thresholdFeeCap = oldFC * (100 + priceBump) / 100
a := big.NewInt(100 + int64(priceBump))
a = a.Mul(a, old.GasPrice())
aFeeCap := new(big.Int).Mul(a, old.FeeCap())
aTip := a.Mul(a, old.Tip())

// thresholdTip = oldTip * (100 + priceBump) / 100
b := big.NewInt(100)
threshold := a.Div(a, b)
// 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
thresholdFeeCap := aFeeCap.Div(aFeeCap, b)
thresholdTip := aTip.Div(aTip, b)

// Have to ensure that either the new fee cap or tip is higher than the
// old ones as well as checking the percentage threshold to ensure that
// this is accurate for low (Wei-level) gas price replacements
if old.GasPriceCmp(tx) >= 0 || tx.GasPriceIntCmp(threshold) < 0 {
if tx.FeeCapIntCmp(thresholdFeeCap) < 0 || tx.TipIntCmp(thresholdTip) < 0 {
return false, nil
}
}
Expand Down Expand Up @@ -406,52 +415,84 @@ func (l *txList) LastElement() *types.Transaction {
}

// priceHeap is a heap.Interface implementation over transactions for retrieving
// price-sorted transactions to discard when the pool fills up.
type priceHeap []*types.Transaction
// price-sorted transactions to discard when the pool fills up. If baseFee is set
// then the heap is sorted based on the effective tip based on the given base fee.
// If baseFee is nil then the sorting is based on feeCap.
type priceHeap struct {
baseFee *big.Int // heap should always be re-sorted after baseFee is changed
list []*types.Transaction
}

func (h priceHeap) Len() int { return len(h) }
func (h priceHeap) Swap(i, j int) { h[i], h[j] = h[j], h[i] }
func (h *priceHeap) Len() int { return len(h.list) }
func (h *priceHeap) Swap(i, j int) { h.list[i], h.list[j] = h.list[j], h.list[i] }

func (h priceHeap) Less(i, j int) bool {
// Sort primarily by price, returning the cheaper one
switch h[i].GasPriceCmp(h[j]) {
func (h *priceHeap) Less(i, j int) bool {
switch h.cmp(h.list[i], h.list[j]) {
case -1:
return true
case 1:
return false
default:
return h.list[i].Nonce() > h.list[j].Nonce()
}
// If the prices match, stabilize via nonces (high nonce is worse)
return h[i].Nonce() > h[j].Nonce()
}

func (h *priceHeap) cmp(a, b *types.Transaction) int {
if h.baseFee != nil {
// Compare effective tips if baseFee is specified
if c := a.EffectiveTipCmp(b, h.baseFee); c != 0 {
return c
}
}
// Compare fee caps if baseFee is not specified or effective tips are equal
if c := a.FeeCapCmp(b); c != 0 {
return c
}
// Compare tips if effective tips and fee caps are equal
return a.TipCmp(b)
}

func (h *priceHeap) Push(x interface{}) {
*h = append(*h, x.(*types.Transaction))
tx := x.(*types.Transaction)
h.list = append(h.list, tx)
}

func (h *priceHeap) Pop() interface{} {
old := *h
old := h.list
n := len(old)
x := old[n-1]
old[n-1] = nil
*h = old[0 : n-1]
h.list = old[0 : n-1]
return x
}

// txPricedList is a price-sorted heap to allow operating on transactions pool
// contents in a price-incrementing way. It's built opon the all transactions
// in txpool but only interested in the remote part. It means only remote transactions
// will be considered for tracking, sorting, eviction, etc.
//
// Two heaps are used for sorting: the urgent heap (based on effective tip in the next
// block) and the floating heap (based on feeCap). Always the bigger heap is chosen for
// eviction. Transactions evicted from the urgent heap are first demoted into the floating heap.
// In some cases (during a congestion, when blocks are full) the urgent heap can provide
// better candidates for inclusion while in other cases (at the top of the baseFee peak)
// the floating heap is better. When baseFee is decreasing they behave similarly.
type txPricedList struct {
all *txLookup // Pointer to the map of all transactions
remotes *priceHeap // Heap of prices of all the stored **remote** transactions
stales int // Number of stale price points to (re-heap trigger)
all *txLookup // Pointer to the map of all transactions
urgent, floating priceHeap // Heaps of prices of all the stored **remote** transactions
stales int // Number of stale price points to (re-heap trigger)
}

const (
// urgentRatio : floatingRatio is the capacity ratio of the two queues
urgentRatio = 4
floatingRatio = 1
)

// newTxPricedList creates a new price-sorted transaction heap.
func newTxPricedList(all *txLookup) *txPricedList {
return &txPricedList{
all: all,
remotes: new(priceHeap),
all: all,
}
}

Expand All @@ -460,7 +501,8 @@ func (l *txPricedList) Put(tx *types.Transaction, local bool) {
if local {
return
}
heap.Push(l.remotes, tx)
// Insert every new transaction to the urgent heap first; Discard will balance the heaps
heap.Push(&l.urgent, tx)
}

// Removed notifies the prices transaction list that an old transaction dropped
Expand All @@ -469,58 +511,42 @@ func (l *txPricedList) Put(tx *types.Transaction, local bool) {
func (l *txPricedList) Removed(count int) {
// Bump the stale counter, but exit if still too low (< 25%)
l.stales += count
if l.stales <= len(*l.remotes)/4 {
if l.stales <= (len(l.urgent.list)+len(l.floating.list))/4 {
return
}
// Seems we've reached a critical number of stale transactions, reheap
l.Reheap()
}

// Cap finds all the transactions below the given price threshold, drops them
// from the priced list and returns them for further removal from the entire pool.
//
// Note: only remote transactions will be considered for eviction.
func (l *txPricedList) Cap(threshold *big.Int) types.Transactions {
drop := make(types.Transactions, 0, 128) // Remote underpriced transactions to drop
for len(*l.remotes) > 0 {
// Discard stale transactions if found during cleanup
cheapest := (*l.remotes)[0]
if l.all.GetRemote(cheapest.Hash()) == nil { // Removed or migrated
heap.Pop(l.remotes)
l.stales--
continue
}
// Stop the discards if we've reached the threshold
if cheapest.GasPriceIntCmp(threshold) >= 0 {
break
}
heap.Pop(l.remotes)
drop = append(drop, cheapest)
}
return drop
}

// Underpriced checks whether a transaction is cheaper than (or as cheap as) the
// lowest priced (remote) transaction currently being tracked.
func (l *txPricedList) Underpriced(tx *types.Transaction) bool {
return l.underpricedFor(&l.urgent, tx) && l.underpricedFor(&l.floating, tx)
}

// underpricedFor checks whether a transaction is cheaper than (or as cheap as) the
// lowest priced (remote) transaction in the given heap.
func (l *txPricedList) underpricedFor(h *priceHeap, tx *types.Transaction) bool {
// Discard stale price points if found at the heap start
for len(*l.remotes) > 0 {
head := []*types.Transaction(*l.remotes)[0]
for len(h.list) > 0 {
head := h.list[0]
if l.all.GetRemote(head.Hash()) == nil { // Removed or migrated
l.stales--
heap.Pop(l.remotes)
heap.Pop(h)
continue
}
break
}
// Check if the transaction is underpriced or not
if len(*l.remotes) == 0 {
return false // There is no remote transaction at all.
if len(h.list) == 0 {
// Note: since Underpriced is only called when the pool is full, this case is only
// possible if one of the queues has a zero capacity. In this case we should report
// that the transaction in question will not fit in this queue.
return true // There is no remote transaction at all.
}
// If the remote transaction is even cheaper than the
// cheapest one tracked locally, reject it.
cheapest := []*types.Transaction(*l.remotes)[0]
return cheapest.GasPriceCmp(tx) >= 0
return h.cmp(h.list[0], tx) >= 0
}

// Discard finds a number of most underpriced transactions, removes them from the
Expand All @@ -529,21 +555,36 @@ func (l *txPricedList) Underpriced(tx *types.Transaction) bool {
// Note local transaction won't be considered for eviction.
func (l *txPricedList) Discard(slots int, force bool) (types.Transactions, bool) {
drop := make(types.Transactions, 0, slots) // Remote underpriced transactions to drop
for len(*l.remotes) > 0 && slots > 0 {
// Discard stale transactions if found during cleanup
tx := heap.Pop(l.remotes).(*types.Transaction)
if l.all.GetRemote(tx.Hash()) == nil { // Removed or migrated
l.stales--
continue
for slots > 0 {
if len(l.urgent.list)*floatingRatio > len(l.floating.list)*urgentRatio || floatingRatio == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a good idea to rebalance things if they are not full. We should keep shoving txs into the urgent queue until it fills up and only then overflow into the floating one. Am I missing something perhaps? I don't see any upside, only downsides to moving txs into the floating queue while there's room in the urgent queue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In txPricedList the definition of full is that items are being discarded. The urgentRatio : floatingRatio thing is internal. If I wanted to enforce absolute limits here then I would need to deal with slot counting because that's how the global limit is calculated in the tx pool. I don't think that complexity is necessary. I think this is a clearly defined behavior: if we discard for any reason at any point, we discard from the two queues alternating in a 4:1 ratio (before which a rebalancing is automatically performed so that everyone is where they should be). We don't assume anything very specific about the caller but this is a safe and rational strategy for any meaningful use of txPricedList.

// Discard stale transactions if found during cleanup
tx := heap.Pop(&l.urgent).(*types.Transaction)
if l.all.GetRemote(tx.Hash()) == nil { // Removed or migrated
l.stales--
continue
}
// Non stale transaction found, move to floating heap
heap.Push(&l.floating, tx)
} else {
if len(l.floating.list) == 0 {
// Stop if both heaps are empty
break
}
// Discard stale transactions if found during cleanup
tx := heap.Pop(&l.floating).(*types.Transaction)
if l.all.GetRemote(tx.Hash()) == nil { // Removed or migrated
l.stales--
continue
}
// Non stale transaction found, discard it
drop = append(drop, tx)
slots -= numSlots(tx)
}
// Non stale transaction found, discard it
drop = append(drop, tx)
slots -= numSlots(tx)
}
// If we still can't make enough room for the new transaction
if slots > 0 && !force {
for _, tx := range drop {
heap.Push(l.remotes, tx)
heap.Push(&l.urgent, tx)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect. The transaction most probably originated from the floating heap, not the urgent heap. Furthermore, we have done certain urgent -> floating heap movements, which should also be reverted in case we can't discard the required number of slots.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With reverting urgent->floating movements you are theoretically right, I'll think about a clean and simple way to handle this.Putting everything in urgent is good though, we can always add everything to urgent first and balance the queues out before discarding anything.

}
return nil, false
}
Expand All @@ -552,12 +593,32 @@ func (l *txPricedList) Discard(slots int, force bool) (types.Transactions, bool)

// Reheap forcibly rebuilds the heap based on the current remote transaction set.
func (l *txPricedList) Reheap() {
reheap := make(priceHeap, 0, l.all.RemoteCount())

l.stales, l.remotes = 0, &reheap
start := time.Now()
l.stales = 0
l.urgent.list = make([]*types.Transaction, 0, l.all.RemoteCount())
l.all.Range(func(hash common.Hash, tx *types.Transaction, local bool) bool {
*l.remotes = append(*l.remotes, tx)
l.urgent.list = append(l.urgent.list, tx)
return true
}, false, true) // Only iterate remotes
heap.Init(l.remotes)
heap.Init(&l.urgent)

// balance out the two heaps by moving the worse half of transactions into the
// floating heap
// Note: Discard would also do this before the first eviction but Reheap can do
// is more efficiently. Also, Underpriced would work suboptimally the first time
// if the floating queue was empty.
floatingCount := len(l.urgent.list) * floatingRatio / (urgentRatio + floatingRatio)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't floatingCount be only the excess above the urgent cap? E.g. if we have 4+1K txs configured, we should only overflow if above 4K. Currently if I have 10txs, I'll overflow 2, which imho doesn't make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See below

l.floating.list = make([]*types.Transaction, floatingCount)
for i := 0; i < floatingCount; i++ {
l.floating.list[i] = heap.Pop(&l.urgent).(*types.Transaction)
}
heap.Init(&l.floating)
reheapTimer.Update(time.Since(start))
}

// SetBaseFee updates the base fee and triggers a re-heap. Note that Removed is not
// necessary to call right before SetBaseFee when processing a new block.
func (l *txPricedList) SetBaseFee(baseFee *big.Int) {
l.urgent.baseFee = baseFee
l.Reheap()
}
Loading