The // TODO: Looks like a copy of the above comment at txnprovider/txpool/pool.go:1520 has been outstanding since 2023 (added in adef971). The two functions — addTxns (used by AddLocalTxns and processRemoteTxns) and addTxnsOnNewBlock (used by OnNewBlock for unwound reorg txs) — share the bulk of their structure but diverge in several specifics, which makes the duplication non-trivial to collapse.
Material differences today
|
addTxns |
addTxnsOnNewBlock |
| Return value |
(Announcements, []DiscardReason, error) |
(Announcements, error) |
| Duplicate-hash handling |
Records DuplicateHash; pokes rebroadcast for stuck local txs |
Silently skips |
addLocked failure |
Records reason; does not call discardLocked |
Calls discardLocked |
| Sender source |
New-tx senders only |
New-tx senders plus stateChanges.ChangeBatch UPSERTs plus queued-tx senders (added in #20996) |
senderLastActivity |
Initialized to blockNum only if absent (preserve existing) |
Reset to blockNum for state-change senders; not touched for queued-only senders (#20996) |
promote() |
Called inline |
Called by OnNewBlock after this function returns |
collect parameter |
Yes |
No |
The shared scaffolding is the new-tx loop (byHash check → newMetaTxn → addLocked → collect senderID), the per-sender senders.info + onSenderStateChange loop, and the announcements bookkeeping.
Why this is worth doing now
#20996 added the queued-sender re-evaluation pass to addTxnsOnNewBlock only. Strictly speaking the same race that pass exists to recover from can also be hit by processRemoteTxns (which goes through addTxns) — though in practice the next OnNewBlock covers it. A shared helper would make it natural to have one canonical re-evaluation site rather than two slightly-different ones, and would make future invariants easier to keep consistent across both paths.
It would also clear the TODO that's been sitting in the file for two years.
Suggested approach
Extract a addTxnsCore (or similar) helper that takes the new txs and a set of additional senders to re-evaluate, plus a small struct of behavior knobs:
- whether duplicate hashes should produce
DiscardReasons or be silent;
- whether
addLocked failure should call discardLocked;
- the
senderLastActivity policy (initialize-if-absent vs reset);
- whether to call
promote() inline.
addTxns and addTxnsOnNewBlock then become thin wrappers that compose the additional senders (stateChanges UPSERTs + queued-tx senders for the OnNewBlock path) and pass the appropriate behavior flags. The existing test suite (especially TestStalePendingEvictionViaMineNonce, TestQueuedTxnPromotedAfterStaleAddLocal, the unwind-related tests) should catch regressions in the consolidation.
Not urgent — pure refactor with no behavior change intended. Picking up "good first issue" only if someone is already comfortable with this corner of the txpool, since the divergences are subtle.
The
// TODO: Looks like a copy of the abovecomment attxnprovider/txpool/pool.go:1520has been outstanding since 2023 (added in adef971). The two functions —addTxns(used byAddLocalTxnsandprocessRemoteTxns) andaddTxnsOnNewBlock(used byOnNewBlockfor unwound reorg txs) — share the bulk of their structure but diverge in several specifics, which makes the duplication non-trivial to collapse.Material differences today
addTxnsaddTxnsOnNewBlock(Announcements, []DiscardReason, error)(Announcements, error)DuplicateHash; pokes rebroadcast for stuck local txsaddLockedfailurediscardLockeddiscardLockedstateChanges.ChangeBatchUPSERTs plus queued-tx senders (added in #20996)senderLastActivityblockNumonly if absent (preserve existing)blockNumfor state-change senders; not touched for queued-only senders (#20996)promote()OnNewBlockafter this function returnscollectparameterThe shared scaffolding is the new-tx loop (
byHashcheck →newMetaTxn→addLocked→ collect senderID), the per-sendersenders.info+onSenderStateChangeloop, and the announcements bookkeeping.Why this is worth doing now
#20996 added the queued-sender re-evaluation pass to
addTxnsOnNewBlockonly. Strictly speaking the same race that pass exists to recover from can also be hit byprocessRemoteTxns(which goes throughaddTxns) — though in practice the nextOnNewBlockcovers it. A shared helper would make it natural to have one canonical re-evaluation site rather than two slightly-different ones, and would make future invariants easier to keep consistent across both paths.It would also clear the TODO that's been sitting in the file for two years.
Suggested approach
Extract a
addTxnsCore(or similar) helper that takes the new txs and a set of additional senders to re-evaluate, plus a small struct of behavior knobs:DiscardReasonsor be silent;addLockedfailure should calldiscardLocked;senderLastActivitypolicy (initialize-if-absent vs reset);promote()inline.addTxnsandaddTxnsOnNewBlockthen become thin wrappers that compose the additional senders (stateChanges UPSERTs + queued-tx senders for the OnNewBlock path) and pass the appropriate behavior flags. The existing test suite (especiallyTestStalePendingEvictionViaMineNonce,TestQueuedTxnPromotedAfterStaleAddLocal, the unwind-related tests) should catch regressions in the consolidation.Not urgent — pure refactor with no behavior change intended. Picking up "good first issue" only if someone is already comfortable with this corner of the txpool, since the divergences are subtle.