-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: shed: Add --below-basefee to mpool clear #10651
Conversation
84c453b
to
8e43ffa
Compare
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'm not sure I understand the core logic here, simple as it is. There's arguably a few different ways we can implement this, and we need to worry about "holes" in our mSets (I think). It looks like you're trying to handle that, but I'm not sure I understand how.
chain/messagepool/messagepool.go
Outdated
for nonce, msg := range ms.msgs { | ||
if types.BigCmp(msg.Message.GasFeeCap, baseFee) < 0 { | ||
noncesToRemove = append(noncesToRemove, nonce) | ||
if nonce > ms.nextNonce && firstNonceToRemove == math.MaxUint64 { |
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'm not sure I understand the nonce > ms.nextNonce
check. Why do we only set firstNonceToRemove
in this case?
Depending on what the intended logic we want for this, we probably just want to find the lowest nonce that has GasFeeCap < baseFee
, and then drop all subsequent messages?
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.
That would be the much less dumb way to do that, assuming that message sets are sorted we could just truncate.
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.
They're not, they're a map.
chain/messagepool/messagepool.go
Outdated
@@ -1584,6 +1614,11 @@ func (mp *MessagePool) Clear(ctx context.Context, local bool) { | |||
|
|||
if ok { | |||
for _, m := range mset.msgs { | |||
if types.BigCmp(co.MinBaseFee, big.Zero()) > 0 && | |||
types.BigCmp(m.Message.GasFeeCap, co.MinBaseFee) < 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.
We don't delete messages with GasFeeCap < MinBaseFee
? Because?
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.
This.. maay be in the wrong order
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.
That is we should remove messages below the specified minimum basefee
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.
That makes more sense.
8e43ffa
to
8c10b2a
Compare
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'm gonna say let's not merge this unless a pressing need emerges? The mpool really should be doing this kinda thinking itself when necessary.
(Closing for now, but feel free to yell)
Related Issues
Proposed Changes
Additional Info
Not tested at all
Checklist
Before you mark the PR ready for review, please make sure that:
<PR type>: <area>: <change being made>
fix: mempool: Introduce a cache for valid signatures
PR type
: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, testarea
, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps