-
Notifications
You must be signed in to change notification settings - Fork 21.4k
core/txpool/legacypool: move queue out of main txpool #32270
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
base: master
Are you sure you want to change the base?
core/txpool/legacypool: move queue out of main txpool #32270
Conversation
@MariusVanDerWijden this looks like a clean change, I think we can make this a PR and merge. |
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.
LGTM
Actually, I think |
Signed-off-by: Csaba Kiraly <[email protected]>
Testing this I've bumped into the following error:
Holding back on merging until I clarify if it is related or not. |
promoteExecutable already removes the txs from the queue. We just need to remove them from the lookup. Signed-off-by: Csaba Kiraly <[email protected]>
Signed-off-by: Csaba Kiraly <[email protected]>
@MariusVanDerWijden the error was due to using |
|
||
addresses = addresses[:len(addresses)-1] | ||
|
||
// Drop all transactions if they are less than the overflow | ||
if size := uint64(list.Len()); size <= drop { | ||
for _, tx := range list.Flatten() { | ||
pool.removeTx(tx.Hash(), true, true) | ||
} | ||
drop -= size | ||
queuedRateLimitMeter.Mark(int64(size)) | ||
continue | ||
} | ||
// Otherwise drop only last few transactions | ||
txs := list.Flatten() | ||
for i := len(txs) - 1; i >= 0 && drop > 0; i-- { | ||
pool.removeTx(txs[i].Hash(), true, true) | ||
drop-- | ||
queuedRateLimitMeter.Mark(1) | ||
} | ||
for _, hash := range removed { | ||
pool.removeTx(hash, true, false) | ||
} |
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 is this removeTx
instead of pool.all.Remove, and why is it with unreserve=false
?
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.
added patch to use pool.all.Remove, and unreserve as needed.
Signed-off-by: Csaba Kiraly <[email protected]>
This PR move the queue out of the main transaction pool.
For now there should be no functional changes.
I see this as a first step to refactor the legacypool and make the queue a fully separate concept from the main pending pool.