txpool: fix tx removal from unlocks set #8500
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
iulianbarbu
changed the title
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
|
/cmd prdoc |
|
/cmd fmt |
|
Command "fmt" has failed ❌! See logs here |
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
michalkucharczyk
approved these changes
May 16, 2025
| if let Some(tx_unlocking) = ready.get_mut(hash) { | ||
| remove_item( | ||
| &mut tx_unlocking.unlocks, | ||
| &tx.transaction.transaction.hash, |
There was a problem hiding this comment.
could be just tx_hash ?:
Suggested change
| &tx.transaction.transaction.hash, | |
| &tx_hash, |
| @@ -298,8 +298,11 @@ impl<Hash: hash::Hash + Member + Serialize, Ex> ReadyTransactions<Hash, Ex> { | |||
| // remove from unlocks | |||
| for tag in &tx.transaction.transaction.requires { | |||
| if let Some(hash) = self.provided_tags.get(tag) { | |||
| if let Some(tx) = ready.get_mut(hash) { | |||
| remove_item(&mut tx.unlocks, hash); | |||
| if let Some(tx_unlocking) = ready.get_mut(hash) { | |||
There was a problem hiding this comment.
Like the name change here 👍
| let tx3 = it.next().unwrap(); | ||
| let lock = ready.ready.read(); | ||
| let tx1_unlocks = &lock.get(&tx1.hash).unwrap().unlocks; | ||
| assert_eq!(tx1_unlocks[0], tx2.hash); |
There was a problem hiding this comment.
we could check tx3 is also unlocked by tx1.
| assert_eq!(removed[1].hash, tx3.hash); | ||
| let lock = ready.ready.read(); | ||
| let tx1_unlocks = &lock.get(&tx1.hash).unwrap().unlocks; | ||
| assert!(!tx1_unlocks.contains(&tx2.hash)); |
There was a problem hiding this comment.
and here tx3 shall still be unlocked by tx1 (assuming I read populate_pool correctly :) )
|
note - just for the record - I think this bug was not causing any user-facing problems. Not removing transactions from unlocks set deep in |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Removing a tx subtree means partly removing some txs from the unlocks set of other txs. This logic is buggy and the PR attempts to fix it.
Closes #8498
Integration
N/A
Review Notes
This doesn't seem to be an important bug. Unit tests for txpool still pass after the fix, so txpool behavior isn't changing much.
TODOs