Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Unqueue invalid transactions when skipping #13918

Merged
merged 1 commit into from
Apr 14, 2023

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Apr 13, 2023

The check was intially added by: #5121

But a later pr fixed it properly: #9789

TLDR: We don't need to check for skipped anymore, as we already remove all transactions that are being unlocked by an invalid transactions and thus, we will not tag them as invalid directly because the nonce for example is incorrect.

Fixes: #13911

The check was intially added by: #5121

But a later pr fixed it properly: #9789

TLDR: We don't need to check for skipped anymore, as we already remove all transactions that are
being unlocked by an invalid transactions and thus, we will not tag them as invalid directly because
the nonce for example is incorrect.

Fixes: #13911
@bkchr bkchr added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit B1-note_worthy Changes should be noted in the release notes T0-node This PR/Issue is related to the topic “node”. labels Apr 13, 2023
@bkchr bkchr requested a review from a team April 13, 2023 22:24
Copy link
Member

@davxy davxy left a comment

Choose a reason for hiding this comment

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

The modification looks good.

I understand that we don't require to check if skipped > 0 before removal as all the dependent txs were already skipped by the iterator when calling report_invalid for a dependency. I.e. the iterator doesn't return dependent txs (introduced by this #9789).

But I don't get (probably for some lacks of my knowledge on the inner machinery) how this fixes : #13911

I mean, if we have a bunch of validated txs in the pool and we upgrade the runtime. How this modification ends up removing the ones that would panic?
Thank you

@davxy davxy requested a review from a team April 14, 2023 06:57
@bkchr
Copy link
Member Author

bkchr commented Apr 14, 2023

I mean, if we have a bunch of validated txs in the pool and we upgrade the runtime. How this modification ends up removing the ones that would panic?

Because they would panic while applying them, aka return an error here and then we unqueue them.

@davxy
Copy link
Member

davxy commented Apr 14, 2023

I mean, if we have a bunch of validated txs in the pool and we upgrade the runtime. How this modification ends up removing the ones that would panic?

Because they would panic while applying them, aka return an error here and then we unqueue them.

I see. So we remove them after the fact. Thank you

@michalkucharczyk michalkucharczyk requested a review from a team April 14, 2023 07:59
@bkchr bkchr merged commit 6a560f8 into master Apr 14, 2023
@bkchr bkchr deleted the bkchr-unqueue-invalid-transactions-when-skipping branch April 14, 2023 08:21
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
The check was intially added by: paritytech#5121

But a later pr fixed it properly: paritytech#9789

TLDR: We don't need to check for skipped anymore, as we already remove all transactions that are
being unlocked by an invalid transactions and thus, we will not tag them as invalid directly because
the nonce for example is incorrect.

Fixes: paritytech#13911
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unqueue panicking extrinsics from the pool
3 participants