fix(core/txpool): fix error pool attempted to unreserve non-reserved address#2239
fix(core/txpool): fix error pool attempted to unreserve non-reserved address#2239gzliudan wants to merge 1 commit intoXinFinOrg:dev-upgradefrom
pool attempted to unreserve non-reserved address#2239Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes recurring txpool error logs (pool attempted to unreserve non-reserved address) by preventing legacypool from releasing reservations it no longer owns, and adds a regression test covering the queue-empty-without-reservation scenario.
Changes:
- Add an ownership check (
Owns) to reservation handles and use it to guard reservation releases inLegacyPool. - Centralize unreserve behavior via
LegacyPool.releaseReservationand replace directreserver.Releasecalls. - Add a focused regression test ensuring stale queued tx cleanup doesn’t attempt to unreserve when the reservation is already gone.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| core/txpool/reserver.go | Extends reservation API with ownership lookup to enable safe guarded releases. |
| core/txpool/legacypool/legacypool.go | Routes all unreserve paths through an ownership-guarded helper to prevent erroneous releases/log spam. |
| core/txpool/legacypool/legacypool_test.go | Adds regression test covering queue cleanup when reservation was externally removed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… address` Symptom: Nodes may repeatedly log "pool attempted to unreserve non-reserved address" during chain import, especially when queue cleanup runs after account state changes. Such as: ```text ERROR[03-27|12:31:22.238] pool attempted to unreserve non-reserved address address=0xb6c1f9dA410d0739efBE934b4c30dFBEE2755233 ERROR[03-27|12:31:26.073] pool attempted to unreserve non-reserved address address=0xb6c1f9dA410d0739efBE934b4c30dFBEE2755233 ERROR[03-27|12:31:36.284] pool attempted to unreserve non-reserved address address=0xb6c1f9dA410d0739efBE934b4c30dFBEE2755233 ERROR[03-27|12:31:56.081] pool attempted to unreserve non-reserved address address=0xb6c1f9dA410d0739efBE934b4c30dFBEE2755233 ERROR[03-27|12:32:10.068] pool attempted to unreserve non-reserved address address=0xb6c1f9dA410d0739efBE934b4c30dFBEE2755233 ``` Cause: The failure happens on block-by-block cleanup paths (notably queue-empty handling during promoteExecutables). Since block import repeatedly triggers these cleanup checks, the same address can be evaluated every round. Without an ownership guard, each round may attempt another Release for the same address and emit the same error again. Address presence in pending/queue and reservation ownership are tracked by different states. Cleanup decisions are based on pending/queue emptiness, while ownership is tracked by the reservation tracker. If ownership was already released (or otherwise no longer held) earlier, a later cleanup round can still satisfy the pending/queue condition and attempt Release again, producing an ownership mismatch. LegacyPool cleanup paths can call Release on addresses that are no longer owned by the current subpool, creating a reservation ownership mismatch. Fix: Add an ownership guard before releasing reservations, so legacypool only releases addresses it still owns. Keep release behavior unchanged for valid ownership cases. Validation: Add and keep a focused regression test covering the queue-empty-without-reservation scenario, and verify legacypool package tests pass.
511fb2e to
abbf357
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Proposed changes
Symptom:
Nodes may repeatedly log "pool attempted to unreserve non-reserved address" during chain import, especially when queue cleanup runs after account state changes. Such as:
Cause:
The failure happens on block-by-block cleanup paths (notably queue-empty handling during promoteExecutables). Since block import repeatedly triggers these cleanup checks, the same address can be evaluated every round. Without an ownership guard, each round may attempt another Release for the same address and emit the same error again.
Address presence in pending/queue and reservation ownership are tracked by different states. Cleanup decisions are based on pending/queue emptiness, while ownership is tracked by the reservation tracker. If ownership was already released (or otherwise no longer held) earlier, a later cleanup round can still satisfy the pending/queue condition and attempt Release again, producing an ownership mismatch.
LegacyPool cleanup paths can call Release on addresses that are no longer owned by the current subpool, creating a reservation ownership mismatch.
Fix:
Add an ownership guard before releasing reservations, so legacypool only releases addresses it still owns. Keep release behavior unchanged for valid ownership cases.
Validation:
Add and keep a focused regression test covering the queue-empty-without-reservation scenario, and verify legacypool package tests pass.
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which parts of the codebase does this PR touch?
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that