fix(core/txpool/legacypool): prevent balance overflow panic and preserve tx replacement accounting, fix #2134#2168
Conversation
|
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 |
gzliudan
requested review from
AnilChinchawale,
anunay-xin,
benjamin202410,
liam-lai and
wanwiset25
There was a problem hiding this comment.
Pull request overview
This PR addresses txpool stability and accounting correctness in core/txpool/legacypool by avoiding uint256.MustFromBig conversions on state balances (which can panic for >256-bit balances) and by fixing total-cost accounting during transaction replacement (including special tx promotion paths).
Changes:
- Switch list cost caps and filter thresholds to
*big.Intand pass account balances directly intolist.Filterto prevent overflow panics. - Fix total-cost replacement accounting by computing totals from a post-replacement base (subtract old cost first, then add new cost with overflow checks).
- Add regression tests covering special-tx totalcost consistency and replacement scenarios involving intermediate overflows.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| core/txpool/legacypool/list.go | Changes list costcap to *big.Int, updates Filter signature/logic, and adjusts Add totalcost replacement computation. |
| core/txpool/legacypool/legacypool.go | Stops converting balances to uint256 for filtering; updates special-tx promotion accounting to mirror replacement-safe totalcost computation. |
| core/txpool/legacypool/legacypool_test.go | Adds regression tests for special-tx promotion totalcost correctness and replacement overflow scenarios. |
| core/txpool/legacypool/list_test.go | Updates benchmark to use *big.Int for the new Filter signature and drops the uint256 import. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cost, overflow := uint256.FromBig(tx.Cost()) | ||
| if overflow { | ||
| return false, nil | ||
| } | ||
| total, overflow := new(uint256.Int).AddOverflow(base, cost) | ||
| if overflow { | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
In promoteSpecialTx, cost/totalcost overflow paths return (false, nil). Since add() returns immediately with this result for special txs, a nil error will make the caller treat the tx as accepted even though it wasn’t inserted (and it can also leak the reserver.Hold() reservation because the deferred Release only runs on err != nil). Return a non-nil error for these overflow cases (and ensure the tx is not partially accounted/inserted).
gzliudan
force-pushed
the
fix-balance-overflow
branch
2 times, most recently
from
fd24767 to
07d25ce
Compare
…able tx filtering, fix XinFinOrg#2134 A runtime panic was triggered in promoteExecutables/demoteUnexecutables when account balance was converted with uint256.MustFromBig(...): panic: overflow ... legacypool.go:1637 Root cause: - pool.currentState.GetBalance(addr) can exceed uint256 range in this code path. - uint256.MustFromBig(balance) panics on overflow, crashing the reorg loop. What this commit changes: - remove uint256.MustFromBig(balance) from executable/non-executable filtering paths - change list.Filter costLimit from *uint256.Int to *big.Int, and compare costs using big.Int directly - keep overflow-safe totalcost accounting for replacements (subtract old cost first, then add new) - return txpool.ErrSpecialTxCostOverflow for special-tx cost/totalcost overflow instead of returning (false, nil) - avoid partial pending-state mutation by attaching a new pending list only after overflow-safe totalcost calculation succeeds Tests: - add regression coverage for special-tx overflow rejection returning non-nil error - verify no pending/lookup/nonce mutation on overflow rejection - cover replacement paths to ensure no intermediate-overflow regressions in list.Add/promoteSpecialTx
gzliudan
force-pushed
the
fix-balance-overflow
branch
from
07d25ce to
20acb19
Compare
Proposed changes
The #2134 txpool changes converted account-balance checks to uint256.MustFromBig, which can panic when state balances exceed uint256 range during promote/demote filtering. A runtime panic was triggered in promoteExecutables/demoteUnexecutables when account balance was converted with uint256.MustFromBig(...):
Root cause:
What this commit changes:
Tests:
panic message:
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