Skip to content

txpool: fix tx removal from unlocks set#8500

Merged
iulianbarbu merged 17 commits intoparitytech:masterfrom
iulianbarbu:ib-fix-subtree-removal
May 21, 2025
Merged

txpool: fix tx removal from unlocks set#8500
iulianbarbu merged 17 commits intoparitytech:masterfrom
iulianbarbu:ib-fix-subtree-removal

Conversation

@iulianbarbu
Copy link
Contributor

@iulianbarbu iulianbarbu commented May 12, 2025

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

  • test with a heavy load test (5 millions txs) - all txs were validated successfully
  • added a unit test

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
@iulianbarbu iulianbarbu self-assigned this May 12, 2025
@iulianbarbu iulianbarbu added the I2-bug The node fails to follow expected behavior. label May 12, 2025
@iulianbarbu iulianbarbu changed the title txpool: fix of tx removal from unlocks set txpool: fix tx removal from unlocks set May 15, 2025
iulianbarbu and others added 3 commits May 15, 2025 14:05
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
@iulianbarbu iulianbarbu marked this pull request as ready for review May 15, 2025 14:19
@iulianbarbu
Copy link
Contributor Author

/cmd prdoc

github-actions bot and others added 2 commits May 15, 2025 14:23
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
@iulianbarbu
Copy link
Contributor Author

/cmd fmt

@github-actions
Copy link
Contributor

Command "fmt" has failed ❌! See logs here

iulianbarbu and others added 3 commits May 15, 2025 17:39
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
@michalkucharczyk
Copy link
Contributor

note - just for the record - I think this bug was not causing any user-facing problems. Not removing transactions from unlocks set deep in graph module should not have been causing any issue with validity of ready set iterator, (or other graph related problems). At least these problems were not identified.

@iulianbarbu
Copy link
Contributor Author

iulianbarbu commented May 18, 2025

note - just for the record - I think this bug was not causing any user-facing problems. Not removing transactions from unlocks set deep in graph module should not have been causing any issue with validity of ready set iterator, (or other graph related problems). At least these problems were not identified.

Fair point. Nonetheless, as we agreed offline, the logic is clearly flawed, so it makes sense to fix it and the unit test should ensure the new logic holds true.

iulianbarbu and others added 3 commits May 18, 2025 17:38
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
@iulianbarbu iulianbarbu enabled auto-merge May 21, 2025 17:35
@iulianbarbu iulianbarbu added this pull request to the merge queue May 21, 2025
Merged via the queue into paritytech:master with commit e44b89f May 21, 2025
243 of 245 checks passed
@iulianbarbu iulianbarbu deleted the ib-fix-subtree-removal branch May 21, 2025 18:18
ordian added a commit that referenced this pull request May 27, 2025
* master: (99 commits)
  Snowbridge: Remove asset location check for compatibility (#8473)
  add poke_deposit extrinsic to pallet-bounties (#8382)
  litep2p/peerset: Reject non-reserved peers in the reserved-only mode (#8650)
  Charge deposit based on key length (#8648)
  [pallet-revive] make subscription task panic on error (#8587)
  tx/metrics: Add metrics for the RPC v2 `transactionWatch_v1_submitAndWatch` (#8345)
  Bridges: Fix - Improve try-state for pallet-xcm-bridge-hub (#8615)
  Introduce CreateBare, deprecated CreateInherent (#7597)
  Use hashbrown hashmap/hashset in validation context (#8606)
  ci: rm gitlab config (#8622)
  🔪 flaky and Zombienet tests (#8600)
  cumulus: adjust unincluded segment size metric buckets (#8617)
  Benchmark storage access on block validation (#8069)
  Revert 7934 es/remove tj changes (#8611)
  collator-protocol: add more collation observability (#8230)
  `fatxpool`: add fallback for ready at light (#8533)
  txpool: fix tx removal from unlocks set (#8500)
  XCMP weight metering: account for the MQ page position (#8344)
  fix epmb solution duplicate issue + add remote mining apparatus to epm (#8585)
  Fix generated address returned by Substrate RPC runtime call (#8504)
  ...
alvicsam pushed a commit that referenced this pull request Oct 17, 2025
# 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

- [x] test with a heavy load test (5 millions txs) - all txs were
validated successfully
- [x] added a unit test

---------

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

I2-bug The node fails to follow expected behavior.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fatxpool: fix incomplete removal for tx subtree

4 participants