txpool/legacypool: fix fillPool TOCTOU and remove debug prints#2269
txpool/legacypool: fix fillPool TOCTOU and remove debug prints#2269lucca30 wants to merge 1 commit into
Conversation
fillPool read pool.Stats() (pool.mu.RLock) and pool.all.Slots() (t.lock.RLock) as two separate critical sections, creating a narrow window where a concurrent pool mutation could make pending != slots even with a healthy pool. Hold pool.mu.RLock across both reads so the snapshot is consistent; all paths that modify pool.all require pool.mu.Lock, so this is sufficient. Also removes three leftover fmt.Println debug statements from list_test.go that polluted CI output.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2269 +/- ##
===========================================
- Coverage 53.39% 53.39% -0.01%
===========================================
Files 896 896
Lines 159713 159713
===========================================
- Hits 85278 85272 -6
- Misses 69100 69112 +12
+ Partials 5335 5329 -6 see 28 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
This PR is stale because it has been open 21 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
1 participant
Summary
fillPoolinlegacypool2_test.goreadpool.Stats()andpool.all.Slots()in two separate critical sections (pool.mu.RLockandt.lock.RLock). A concurrent pool mutation between the two reads could produce an inconsistent snapshot, causingTestLockOrdering_ReplacePendingNoDeadlockto fail withhave 199, want 200.pool.mu.RLockacross both reads infillPool, using the unexportedpool.stats()(valid since the test is in the same package). All paths that mutatepool.allrequirepool.mu.Lock, so the two reads are now atomic with respect to pool mutations.fmt.Printlncalls inlist_test.go(TestFilterTxConditionalKnownAccounts) polluted CI output; replaced with blank identifier assignments and removed the unused"fmt"import.Test plan
go test ./core/txpool/legacypool/... -count=50 -run TestLockOrdering_ReplacePendingNoDeadlock— no failures expectedgo test ./core/txpool/legacypool/... -count=1— full package passes