-
Notifications
You must be signed in to change notification settings - Fork 20.1k
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
core/txpool/blobpool: simplify the drop txs logic in blobpool module. #30447
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.
SGTM
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 change makes the code a bit smaller, that much I grant, but IMO not more simple.
The pre-alloc of ids
and nonces
makes it a bit more brittle, even though it's arguably slightly more performant.
I'd personally rather close this PR than merge it, but I'll leave it open for someone else to decide
The old code had a clearer intent, that's given. But I don't see anything wrong with the new one either and if it's ever so slightly less allocy, why not |
nice |
In light of the latest broken release due to small refactors for the sake of refactoring, we've decided to reject such changes from now on as their value is semi-limited and they always post a danger to potentially breaking something unforeseen. |
ids
andnonces
.