refactor(core/txpool): remove locals-tracking from pools #30559#2176
refactor(core/txpool): remove locals-tracking from pools #30559#2176gzliudan wants to merge 3 commits intoXinFinOrg:dev-upgradefrom
Conversation
…ereum#29083 * core/txpool: no need to run rotate if no local txs Signed-off-by: jsvisa <delweng@gmail.com> * Revert "core/txpool: no need to run rotate if no local txs" This reverts commit 17fab17. Signed-off-by: jsvisa <delweng@gmail.com> * use Debug if todo is empty Signed-off-by: jsvisa <delweng@gmail.com> --------- Signed-off-by: jsvisa <delweng@gmail.com>
|
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
Refactors the transaction pool stack to remove “local transaction” tracking/exemptions from the pools themselves, shifting local journaling/resubmission into a dedicated core/txpool/locals tracker and simplifying pool APIs accordingly.
Changes:
- Remove the
localflag from txpool/subpoolAddAPIs and deleteLocals()exposure fromTxPool/SubPool. - Strip local-specific eviction/pricing/journaling behavior from
LegacyPool, and adjust tests/benchmarks to match the new semantics. - Introduce
core/txpool/localsjournaling + periodic resubmission tracker; exportSortedMapfrom legacypool for reuse.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| eth/protocol_test.go | Update txpool Add call signature in protocol tests. |
| eth/protocol.go | Update txPool interface: remove local param from Add. |
| eth/helper_test.go | Update test txpool stub Add signature. |
| eth/handler.go | Update inbound-tx handling to use new Add(txs, sync) API. |
| eth/api_backend.go | Update RPC send path to new txpool Add signature. |
| core/txpool/txpool.go | Remove local param plumbing to subpools; delete TxPool.Locals(). |
| core/txpool/subpool.go | Remove Locals() and local param from SubPool interface. |
| core/txpool/locals/tx_tracker.go | Add new local-tx tracker with journal + periodic recheck/resubmit. |
| core/txpool/locals/journal.go | Fix package to locals and adjust journal rotation logging verbosity. |
| core/txpool/legacypool/list.go | Export SortedMap; remove local-only behaviors in priced list / lookup iteration. |
| core/txpool/legacypool/legacypool_test.go | Remove/adjust tests that relied on local exemptions and journaling. |
| core/txpool/legacypool/legacypool.go | Remove local allowlist/journal/metrics and local-exemption logic; unify lookup storage. |
| core/txpool/legacypool/journal_shared.go | Add shared journal helpers for legacypool journals. |
| contracts/utils.go | Update internal tx injection to new txpool Add signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| package legacypool | ||
|
|
||
| import "errors" |
There was a problem hiding this comment.
New file is missing the standard go-ethereum/XDPoSChain license header comment block used throughout core/txpool/legacypool (e.g., list.go, order_tx_journal.go, noncer.go). Add the usual header to keep licensing/attribution consistent.
|
|
||
| // RemotesBelowTip finds all remote transactions below the given tip threshold. | ||
| func (t *lookup) RemotesBelowTip(threshold *big.Int) types.Transactions { | ||
| // TxsBelowTip finds all remote transactions below the given tip threshold. |
There was a problem hiding this comment.
The comment for TxsBelowTip says it finds "remote" transactions, but the lookup no longer distinguishes local/remote and Range now iterates all transactions. Update the comment to reflect current behavior (all transactions below the threshold).
| // TxsBelowTip finds all remote transactions below the given tip threshold. | |
| // TxsBelowTip finds all transactions below the given tip threshold. |
| // New creates a new TxTracker | ||
| func New(journalPath string, journalTime time.Duration, chainConfig *params.ChainConfig, next *txpool.TxPool) *TxTracker { | ||
| pool := &TxTracker{ | ||
| all: make(map[common.Hash]*types.Transaction), | ||
| byAddr: make(map[common.Address]*legacypool.SortedMap), | ||
| signer: types.LatestSigner(chainConfig), | ||
| shutdownCh: make(chan struct{}), | ||
| pool: next, | ||
| } | ||
| if journalPath != "" { | ||
| pool.journal = newTxJournal(journalPath) | ||
| pool.rejournal = journalTime | ||
| } | ||
| return pool | ||
| } |
There was a problem hiding this comment.
The new locals TxTracker/journal behavior (tracking, periodic recheck/resubmit, and journal rotation) is introduced without any accompanying tests. Adding unit tests around recheck/resubmit (including nonce-sorting and stale removal) and journal load/rotate would help ensure the refactor doesn't regress reliability.
| for _, list := range rejournal { | ||
| // cmp(a, b) should return a negative number when a < b, | ||
| slices.SortFunc(list, func(a, b *types.Transaction) int { | ||
| return int(a.Nonce() - b.Nonce()) |
There was a problem hiding this comment.
The comparator used in slices.SortFunc is incorrect for uint64 nonces: int(a.Nonce() - b.Nonce()) will underflow when a<b and can overflow/truncate when converting to int, breaking ordering and potentially causing sort to misbehave. Use a safe compare (e.g., cmp.Compare(a.Nonce(), b.Nonce()) or explicit if/else returning -1/0/1) instead of subtraction/casting.
| return int(a.Nonce() - b.Nonce()) | |
| if a.Nonce() < b.Nonce() { | |
| return -1 | |
| } | |
| if a.Nonce() > b.Nonce() { | |
| return 1 | |
| } | |
| return 0 |
| // Track adds a transaction to the tracked set. | ||
| // Note: blob-type transactions are ignored. | ||
| func (tracker *TxTracker) Track(tx *types.Transaction) { | ||
| tracker.TrackAll([]*types.Transaction{tx}) | ||
| } | ||
|
|
||
| // TrackAll adds a list of transactions to the tracked set. | ||
| // Note: blob-type transactions are ignored. | ||
| func (tracker *TxTracker) TrackAll(txs []*types.Transaction) { |
There was a problem hiding this comment.
The comments say blob-type transactions are ignored, but Track/TrackAll don't check tx type at all. Either implement the intended filter (if blob txs are possible here) or remove/update the comment to match behavior.
core/txpool/locals/tx_tracker.go
Outdated
| "github.com/XinFinOrg/XDPoSChain/log" | ||
| "github.com/XinFinOrg/XDPoSChain/metrics" | ||
| "github.com/XinFinOrg/XDPoSChain/params" | ||
| "golang.org/x/exp/slices" |
There was a problem hiding this comment.
This file imports golang.org/x/exp/slices, but the module is on Go 1.25 and the rest of the codebase already uses the stdlib slices package. Prefer slices from the standard library here to avoid the extra exp import and keep consistency.
| "golang.org/x/exp/slices" | |
| "slices" |
| checkJournal := tracker.journal != nil && time.Since(lastJournal) > tracker.rejournal | ||
| resubmits, rejournal := tracker.recheck(checkJournal) | ||
| if len(resubmits) > 0 { | ||
| tracker.pool.Add(resubmits, false) |
There was a problem hiding this comment.
TxTracker resubmits any missing txs every interval but ignores the errors returned by pool.Add. If a tracked tx is permanently rejected (e.g., underpriced/invalid), it will be retried forever and can cause repeated work/log spam. Consider handling the Add result (e.g., drop from the tracker on non-retriable errors or add backoff/attempt limits).
| tracker.pool.Add(resubmits, false) | |
| if errs := tracker.pool.Add(resubmits, false); len(errs) > 0 { | |
| log.Debug("Failed to resubmit some local transactions", "count", len(errs)) | |
| } |
gzliudan
force-pushed
the
local_pool_part2
branch
from
5510277 to
c5c46e1
Compare
gzliudan
force-pushed
the
local_pool_part2
branch
from
c5c46e1 to
0f3106d
Compare
Proposed changes
Ref: ethereum#30559
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