-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
New higher capacity Txpool #2660
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx so much
core/events.go
Outdated
// Static bool is Whether to send to only Static peer or not. | ||
// This is because at high traffic we still want to broadcast transactions to at least some peers so that we | ||
// minimize the transaction lost. | ||
Static bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would call it differently, maybe BroadcastToStatic or something more meaningful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion-1:
For pool-2: it has its own peers to broadcast the Txs, the peers of pool-2 could be consisted by:
1.StaticNodes
2.A subset of the other peers, sqrt()?
Because, many node may not contain any StaticNodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sqrt() of peers is already what happens in pool-1. So we might do cube root or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sqrt() of peers is already what happens in pool-1
// ==
do you mean the sqrt() logic in BroadcastTransactions? https://github.com/bnb-chain/bsc/blob/master/eth/handler.go#L865?
What I mean here is that: suppose we have 400 connected peers, 10 StaticNodes, then:
1.For pool-1: it has 400 peers, directly send full tx content to 20(sqrt(400) peers, and send announcement to the left 380 peers
2.For pool-2: it has sqrt(400) + 10(size of static nodes), discard the overlapped one, it could have 20 -> 30 peers. Suppose it is 25 peets, base on it, send full tx content to 5(sqrt(25)) peers, and send announcement to the left 20 peers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the above example, there is a chance that all 5 peers are non-static. Is that fine? Or we should have at least 1 static peer (if it exists) to send full tx always.
core/txpool/legacypool/buffer.go
Outdated
txSlots := numSlots(tx) | ||
|
||
// Remove elements until there is enough capacity | ||
for lru.size+txSlots > lru.capacity && lru.buffer.Len() > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why lru.buffer.Len() > 0
?
core/txpool/legacypool/fastcache.go
Outdated
// Move a transaction to the front of the LRU order | ||
func (lru *LRUBufferFastCache) moveToFront(hash common.Hash) { | ||
for i, h := range lru.order { | ||
if h == hash { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iterate all?cost much
core/txpool/legacypool/fastcache.go
Outdated
for i, h := range lru.order { | ||
if h == hash { | ||
// Remove the hash from its current position | ||
lru.order = append(lru.order[:i], lru.order[i+1:]...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huge copy?
core/txpool/legacypool/fastcache.go
Outdated
} | ||
} | ||
// Add it to the front | ||
lru.order = append(lru.order, hash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huge copy?
core/txpool/legacypool/legacypool.go
Outdated
reorgDoneCh: make(chan chan struct{}), | ||
reorgShutdownCh: make(chan struct{}), | ||
initDoneCh: make(chan struct{}), | ||
localBufferPool: NewLRUBufferFastCache(int(config.Pool3Slots)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
core/txpool/legacypool/legacypool.go
Outdated
} | ||
|
||
// addToPool12OrPool3 adds a transaction to pool1 or pool2 or pool3 depending on which one is asked for | ||
func (pool *LegacyPool) addToPool12OrPool3(tx *types.Transaction, from common.Address, isLocal bool, pool1, pool2, pool3 bool) (bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no scene call addToPool12OrPool3 with pool1 or pool2 ?
core/txpool/legacypool/fastcache.go
Outdated
} | ||
} | ||
// Add it to the front | ||
lru.order = append(lru.order, hash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
call move item to the back
as moveToFront?
during the benchmark in QA env, I use the follwing config
then I find the txpool is not the bottleneck of performance, validator always have enough txs to handle based on this, I think we don't need pool2 |
no replacement logic in pool3, so if txs put into pool3, the replacement may fail, does it matter? |
core/txpool/legacypool/heap_test.go
Outdated
pool := NewTxPool3Heap(0) | ||
b.ResetTimer() | ||
for i := 0; i < b.N; i++ { | ||
tx := createRandomTestTx() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to move createRandomTestTx
outside the bench loop, and just using the tx from prepared tx slice. you will get more accurate bench results.
core/txpool/legacypool/heap.go
Outdated
return | ||
} | ||
|
||
tp.sequence++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here needs to add a maxSize
checking to avoid mem abuse. Because of txHeap
is a pretty simple struct, is it ok to check maxSize
by cap(txhep)
?
if len(*h) >= cap(*h) {
// or just pop first to get extra space for newer tx?
h.Pop()
}
Description
This PR works to expand the legacy transaction pool's functionality by adding two new concepts:
The current pool as it is now is simply referred to pool1.
This will help accommodating more number of transactions especially in high traffic when somewhat underpriced and otherwise valid transactions get rejected.
Rationale
tell us why we need these changes...
Example
add an example CLI or API response...
Changes
Notable changes: