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.

Sign up for GitHub

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

core/txpool: remove "local" notion from the txpool price heap #21478

Merged
merged 10 commits into from
Dec 11, 2020
Merged

core/txpool: remove "local" notion from the txpool price heap #21478

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a2cce31
core: separate the local notion from the pricedHeap
rjl493456442 Aug 24, 2020
ab83f5c
core: add benchmarks
rjl493456442 Aug 24, 2020
b5f07d2
core: improve tests
rjl493456442 Aug 25, 2020
2493234
core: address comments
rjl493456442 Aug 26, 2020
5d7850c
core: degrade the panic to error message
rjl493456442 Aug 26, 2020
309c7d0
core: fix typo
rjl493456442 Sep 29, 2020
4e97b39
core: address comments
rjl493456442 Nov 19, 2020
ad6aca3
core: address comment
rjl493456442 Nov 19, 2020
a6a0ffc
core: use PEAK instead of POP
rjl493456442 Nov 19, 2020
4168919
core: address comments
rjl493456442 Nov 19, 2020
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
149 changes: 64 additions & 85 deletions core/tx_list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (

"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/core/types"
"github.com/ethereum/go-ethereum/log"
)

// nonceHeap is a heap.Interface implementation over 64bit unsigned integers for
Expand Down Expand Up @@ -438,24 +437,29 @@ func (h *priceHeap) Pop() interface{} {
}

// txPricedList is a price-sorted heap to allow operating on transactions pool
// contents in a price-incrementing way.
// 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.
type txPricedList struct {
all *txLookup // Pointer to the map of all transactions
items *priceHeap // Heap of prices of all the stored transactions
stales int // Number of stale price points to (re-heap trigger)
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)
}

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

// Put inserts a new transaction into the heap.
func (l *txPricedList) Put(tx *types.Transaction) {
heap.Push(l.items, tx)
func (l *txPricedList) Put(tx *types.Transaction, local bool) {
if local {
return
}
heap.Push(l.remotes, tx)
}

// Removed notifies the prices transaction list that an old transaction dropped
Expand All @@ -464,121 +468,96 @@ func (l *txPricedList) Put(tx *types.Transaction) {
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.items)/4 {
if l.stales <= len(*l.remotes)/4 {
return
}
// Seems we've reached a critical number of stale transactions, reheap
reheap := make(priceHeap, 0, l.all.Count())

l.stales, l.items = 0, &reheap
l.all.Range(func(hash common.Hash, tx *types.Transaction) bool {
*l.items = append(*l.items, tx)
return true
})
heap.Init(l.items)
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.
func (l *txPricedList) Cap(threshold *big.Int, local *accountSet) types.Transactions {
//
// 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
save := make(types.Transactions, 0, 64) // Local underpriced transactions to keep

for len(*l.items) > 0 {
for len(*l.remotes) > 0 {
// Discard stale transactions if found during cleanup
tx := heap.Pop(l.items).(*types.Transaction)
if l.all.Get(tx.Hash()) == nil {
tx := heap.Pop(l.remotes).(*types.Transaction)
if l.all.GetRemote(tx.Hash()) == nil { // Removed or migrated
l.stales--
continue
}
// Stop the discards if we've reached the threshold
if tx.GasPriceIntCmp(threshold) >= 0 {
save = append(save, tx)
heap.Push(l.remotes, tx)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure it makes any difference, but might be worth benchmarking: doing a heap.Peek instead of Pop + Push. ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I will do a benchmark for it

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, heap has no Peak function...

Copy link
Contributor

Choose a reason for hiding this comment

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

Right... I saw another place that this (guerilla peek) trick is used:

		tx := []*types.Transaction(*l.remotes)[0]

I tested it, seems to be maybe a little faster, not sure if it's worth the extra complexity...

Copy link
Member

@karalabe karalabe Nov 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

guerilla peek

That's not a trick actually, that's a guaranteed API (edit, actually, that's a guaranteed heap data structure concept, no a Go specific thing), alas looks a bit odd:

The minimum element in the tree is the root, at index 0.
https://golang.org/pkg/container/heap/

Re Pop/Push vs Peek, each Pop and Push on a heap is log(n). So if we can peek, it's always much better.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

break
}
// Non stale transaction found, discard unless local
if local.containsTx(tx) {
save = append(save, tx)
} else {
drop = append(drop, tx)
}
}
for _, tx := range save {
heap.Push(l.items, tx)
drop = append(drop, tx)
rjl493456442 marked this conversation as resolved.
Show resolved Hide resolved
}
return drop
}

// Underpriced checks whether a transaction is cheaper than (or as cheap as) the
// lowest priced transaction currently being tracked.
func (l *txPricedList) Underpriced(tx *types.Transaction, local *accountSet) bool {
// Local transactions cannot be underpriced
if local.containsTx(tx) {
return false
}
// lowest priced (remote) transaction currently being tracked.
func (l *txPricedList) Underpriced(tx *types.Transaction) bool {
// Discard stale price points if found at the heap start
for len(*l.items) > 0 {
head := []*types.Transaction(*l.items)[0]
if l.all.Get(head.Hash()) == nil {
for len(*l.remotes) > 0 {
head := []*types.Transaction(*l.remotes)[0]
if l.all.GetRemote(head.Hash()) == nil { // Removed or migrated
l.stales--
heap.Pop(l.items)
heap.Pop(l.remotes)
continue
}
break
}
// Check if the transaction is underpriced or not
if len(*l.items) == 0 {
log.Error("Pricing query for empty pool") // This cannot happen, print to catch programming errors
return false
if len(*l.remotes) == 0 {
return false // There is no remote transaction at all.
Copy link
Contributor

Choose a reason for hiding this comment

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

Previously, there was a comment saying that this couldn't happen. Is that still true?

  • If not (and this can now legitimately happen, is it correct to return 'false'?
  • If yes, then please add back the log.Error

Copy link
Member

Choose a reason for hiding this comment

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

Returning false is correct in case of an empty pool because it just states that "yes, this transaction is priced correctly compared to everything else (none)". That said, I'm unsure why we'd start calling it on an empty pool.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think for the empty remote set it's possible. All the transactions in the pool are locals.

}
cheapest := []*types.Transaction(*l.items)[0]
// If the remote transaction is even cheaper than the
// cheapest one tracked locally, please reject it.
rjl493456442 marked this conversation as resolved.
Show resolved Hide resolved
cheapest := []*types.Transaction(*l.remotes)[0]
return cheapest.GasPriceCmp(tx) >= 0
}

// Discard finds a number of most underpriced transactions, removes them from the
// priced list and returns them for further removal from the entire pool.
func (l *txPricedList) Discard(slots int, local *accountSet) types.Transactions {
// If we have some local accountset, those will not be discarded
if !local.empty() {
// In case the list is filled to the brim with 'local' txs, we do this
// little check to avoid unpacking / repacking the heap later on, which
// is very expensive
discardable := 0
for _, tx := range *l.items {
if !local.containsTx(tx) {
discardable++
}
if discardable >= slots {
break
}
}
if slots > discardable {
slots = discardable
}
}
if slots == 0 {
return nil
}
drop := make(types.Transactions, 0, slots) // Remote underpriced transactions to drop
save := make(types.Transactions, 0, len(*l.items)-slots) // Local underpriced transactions to keep

for len(*l.items) > 0 && slots > 0 {
//
// 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.items).(*types.Transaction)
if l.all.Get(tx.Hash()) == nil {
tx := heap.Pop(l.remotes).(*types.Transaction)
if l.all.GetRemote(tx.Hash()) == nil { // Removed or migrated
l.stales--
continue
}
// Non stale transaction found, discard unless local
if local.containsTx(tx) {
save = append(save, tx)
} else {
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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit unsure of what force means, thus I'm not sure if !force is correct, or if it should be force. Could you please document what force means here?

This looks a bit wrong to me. Essentially, if we don't manage to free up slots slots, then you add back all the ones we found could be dropped?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a special case: the txpool is full of local transactions. However we still want to make a room for the newly arrived local transaction.

In this case, If we only have a part of the slot can be released, these slots are released forcibly(even the slots is 0)

Copy link
Member Author

Choose a reason for hiding this comment

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

This looks a bit wrong to me. Essentially, if we don't manage to free up slots slots, then you add back all the ones we found could be dropped?

Yes it the logic for the remote transactions. But I give some special care to the local transactions.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmmmm, re force + readds, this is interesting.

Previously we've assumed that discard can always drop the needed number of transactions. IMHO this is a bug, made a bit worse by the slot rewrite:

  • If we have local and remote transactions, it can always happen that we cannot discard the requested number of slots simply because there's not enough remote slots left. In that case the old code just discarded as many as it could (potentially 0) and called it a day, letting the adder inters the tx afterwards, overflowing the pool.
  • Without a slot rewrite, we could end up with 1 slot overflowing. With the rewrite however, it's imaginable that we have a multi-slot transaction (e.g. 4) that evacuates multiple-but-not-enough slots (e.g. 3). In that case, we also introduce a weird churn on the pool.

I'm a bit unsure of what force means

Based on my understanding, you useforce as local flag. I.e. if the tx is local, then try to make as much space as possible, but even if we only have 3 remote slots and need 4, drop the 3 rather than keep then in. Local tx always fit, remote ones observe the pool limits.

for _, tx := range drop {
heap.Push(l.remotes, tx)
}
return nil, false
}
for _, tx := range save {
heap.Push(l.items, tx)
}
return drop
return drop, true
}

// Reheap forcibly rebuilds the heap based on the current remote transaction set.
// This function is mainly used in testing for verifying the content of heap.
rjl493456442 marked this conversation as resolved.
Show resolved Hide resolved
func (l *txPricedList) Reheap() {
reheap := make(priceHeap, 0, l.all.RemoteCount())

l.stales, l.remotes = 0, &reheap
l.all.Range(func(hash common.Hash, tx *types.Transaction, local bool) bool {
*l.remotes = append(*l.remotes, tx)
return true
}, false, true) // Only iterate remotes
heap.Init(l.remotes)
}
Loading