fix(core/txpool,core/txpool/locals): fix local tracker resubmits - #2541
fix(core/txpool,core/txpool/locals): fix local tracker resubmits#2541gzliudan wants to merge 2 commits into
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: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 local transaction tracking so permanently rejected or superseded transactions are removed from memory and disk.
Changes:
- Classifies resubmission errors and untracks permanent rejections.
- Removes superseded same-nonce transactions.
- Rotates journals after removals and adds regression tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
core/txpool/locals/tx_tracker.go |
Updates resubmission, untracking, and journal rotation logic. |
core/txpool/locals/tx_tracker_test.go |
Tests rejection, replacement, and journal persistence behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
d34ee67 to
69d8ff1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
core/txpool/locals/tx_tracker_test.go:165
- Handle the key-generation error instead of passing a potentially nil key to
SignTx; the repository's Go error-handling convention does not allow ignored errors.
otherKey, _ := crypto.GenerateKey()
69d8ff1 to
caf9b92
Compare
1c1fb44 to
dd015f2
Compare
dd015f2 to
c5373cf
Compare
c5373cf to
f9b404a
Compare
| if replaced := list.Get(tx.Nonce()); replaced != nil { | ||
| delete(tracker.all, replaced.Hash()) |
f9b404a to
c3233a9
Compare
…aces TrackAll only ever added to the tracked set, while the per-nonce SortedMap silently overwrote the entry a replacement displaced. The replaced transaction was therefore never returned by Forward again, stayed tracked forever, was rewritten into the journal on every rotation and could win the nonce on the next load, resurrecting a transaction the user had already replaced, because rotation writes the tracked set in map order through a non-stable sort. Drop the replaced transaction from the tracked set when its nonce is taken over, unless the pool still holds it: the pool decides which transaction occupies a nonce, and AddLocal tracks a local transaction only after Add has released the subpool lock, so two concurrent submissions can be accepted in one order and reach TrackAll in the other. Keeping the transaction the pool holds then stops a replacement from dropping the live transaction and pinning the superseded one, which recheck could never resubmit successfully and which would leave the live one without local protection. This also converges a journal written by an older version, which can hold both a transaction and its replacement: load feeds the file straight into TrackAll, so the entry later in the file wins the nonce and the tracked set stays consistent, and the next rotation rewrites the journal from it. Build the test environment from an explicit chain config so tests can pin the gas schedule instead of sharing the package level genesis. Cover both orders in which a replacement can reach the tracker, and the concurrent interleaving where the original is added first and tracked last.
c3233a9 to
a9ba231
Compare
…the gas price floor A gas schedule fork raises the floor above transactions that were admitted under the previous tier, and the pool sweeps them out. The local tracker kept resubmitting them every minute: recheck ignored the result of pool.Add, and the transactions never went stale because their nonce never advanced, so they stayed tracked and journalled forever. Hold back any tracked transaction priced below the current floor, resolved for the block pending on top of the head exactly as admission validation resolves it. They stay tracked, so a reorg or a set-head rollback that lowers the floor picks them up again on the next recheck: the pool does not bring back what it swept, so the tracker is the only thing that can. Special transactions stay exempt, as they are during admission. Resolve the floor through a new TxPool.MinGasPrice, which goes through the same pendingBlockNumber helper as admission validation and the sweep, so the tracker and the pool cannot price the same transaction at different heights. Report the held back transactions through txpool/local/belowfloor, since they are still counted by txpool/local. Pin that ErrUnderMinGasPrice is not a temporary reject: the floor only rises as the chain advances, so retrying cannot help and AddLocal must not track the transaction, but the error must not be used to drop a tracked one either. Cover the floor with tests that rewind the head with SetHead across every tier boundary and cover a head without a block number, the hold-back at exactly the floor and one wei below it, resumption once the floor drops, and the exemption special transactions keep.
a9ba231 to
247dfcd
Compare
Summary
The local transaction tracker keeps resubmitting transactions that can never enter the pool again, and keeps tracking transactions the user already replaced.
rechecknow holds back every tracked transaction priced below the gas schedule floor resolved at the block pending on top of the head, exactly as admission resolves it, andTrackAlldrops the transaction a replacement displaces from its nonce.ErrUnderMinGasPriceis pinned as a non-temporary reject, soAddLocaldoes not track a transaction the floor rejected, while the tracker still holds on to transactions it tracked before the fork so a rollback can recover them. closes #NMotivation & Context
A gas tier fork (
Gas50x,Gas2500x) raises the minimum gas price above transactions that were admitted under the previous tier, and the pool sweeps them out. The tracker kept resubmitting them every minute:recheckignored the result of the resubmit, and the transactions never went stale because their nonce never advanced, so they stayed tracked and journalled forever and were re-added on every recheck round.Separately,
TrackAllonly ever added to the tracked set while the per-nonceSortedMapsilently overwrote the entry a replacement displaced. The replaced transaction was therefore never returned byForwardagain, stayed tracked forever, was rewritten into the journal on every rotation, and could win the nonce on the next load, resurrecting a transaction the user had already replaced, because rotation writes the tracked set in map order through a non-stable sort.Changes
core/txpool/validation.go: extractpendingBlockNumber, the shared helper that resolves the fork tier one past the head; admission validation now goes through it instead of inline arithmetic.core/txpool/txpool.go: addTxPool.MinGasPrice, which resolves the floor at the block pending on top of the head withparams.GetMinGasPrice.core/txpool/locals/tx_tracker.go:recheckresolves the floor once per round and holds back non-special transactions priced below it; they stay tracked so a reorg or a set-head rollback past the fork picks them up again on the next round. Held back transactions break out into a newtxpool/local/belowfloorgauge and abelow-floorlog field.core/txpool/locals/tx_tracker.go:TrackAlldeletes the displaced transaction from the tracked set when its nonce is taken over, which also converges a journal written by an older version that can hold both a transaction and its replacement.core/txpool/locals/errors.go: document thatErrUnderMinGasPricestays out ofIsTemporaryReject, because the floor only rises as the chain advances, and that it must not be used to drop a tracked transaction either, because a rollback past the fork lowers the floor again.TestMinGasPriceResolvesAtPendingBlock,TestMinGasPriceFallsBackWithoutHeadNumber,TestRecheckHoldsBackBelowFloorTransactions,TestRecheckResumesAfterFloorDrops,TestRecheckResubmitsSpecialTransactionBelowFloor,TestIsTemporaryRejectExcludesMinGasPrice,TestAddLocalDoesNotTrackMinGasPriceRejectedTx,TestTrackAllDropsReplacedTransaction,TestRecheckDoesNotResubmitReplacedTransaction,TestJournalRotationDropsReplacedTransaction,TestJournalLoadDropsReplacedTransaction,TestJournalLoadKeepsLastEntryPerNonce; the test environment is now built from an explicit chain config throughnewTestEnvWithConfig, so tests pin the gas schedule instead of sharing the package level genesis.Testing
go test ./core/txpool/ ./core/txpool/locals/ -count=1passes at both commits, each verified in its own worktree, alongsidego build ./core/txpool/...and a cleangofmt -l core/txpool/.Floor resolution is pinned by rewinding the head with
SetHeadthrough 200, 199, 198, 99 and 98 withGas50xBlock=100andGas2500xBlock=200, asserting the 2500x, 50x and baseline tiers resolve at head plus one, plus a case for a head that carries no block number. The hold-back is pinned at the boundary: a transaction priced exactly at the floor is resubmitted, one priced a wei below is held back and stays tracked, and resubmission resumes onceSetHeadrolls the head back before the fork. Special transactions are asserted to be resubmitted below the floor, matching admission and the sweep. Journal convergence is asserted after a rotation from the tracked set and for both file orders on load.Related Issues
Risk & Impact
Pool admission rules are unchanged; these changes only affect which tracked transactions the local tracker resubmits, and only for nodes running a local tracker with a journal.
Tracking is now narrower: a local transaction rejected for
ErrUnderMinGasPriceis no longer tracked, so a later rollback past the fork does not resubmit it and the user has to send it again. That is deliberate and pinned by tests, and it is the only behaviour change visible to submitters.The floor is resolved from the same
pendingBlockNumberhelper as admission and from the same height as the sweep, so the three cannot drift apart; a one block lag betweenp.chain.CurrentBlock()and a subpool's own head can only delay a resubmission by up to one recheck interval around a fork boundary.The journal format is unchanged. A journal written by an older version that holds both a transaction and its replacement converges to one entry per nonce, where the entry later in the file wins because entries carry no sequence number; the next rotation rewrites the journal from the converged set. The new
txpool/local/belowfloorgauge counts only held back transactions that the pool no longer has, solocalminusbelowflooris the tracked transactions the pool holds plus the ones resubmitted this round, not just the latter.Notes for Reviewers
Two commits, bookkeeping first (dropping the replaced transaction) and the price floor second; each builds and passes tests on its own.
The call worth a second look is the asymmetry in
errors.go:ErrUnderMinGasPriceis not temporary, so a rejected transaction is not tracked, yet it must never be used to drop an already tracked one, so a rollback can recover it. Both directions are pinned by tests.TestJournalLoadKeepsLastEntryPerNoncedeliberately pins the losing behaviour for journals written by an older version instead of fixing it, since journal entries carry no ordering information.