fix(core/txpool/legacypool): clarify and fix non-executable tx heartbeat #33704#2220
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 |
163b706 to
a075967
Compare
There was a problem hiding this comment.
Pull request overview
This PR adjusts how the legacy txpool’s non-executable (queued) transaction “heartbeat” is tracked, preventing heartbeats from being created/updated for accounts that don’t currently have queued transactions and clarifying related semantics in comments.
Changes:
- Make
queue.bumpa no-op for unknown addresses (only updates heartbeat for accounts already tracked inbeats). - Clarify
Config.Lifetimemeaning as “staleness” time for queued accounts (not per-tx lifetime). - Update/clarify comments around when queued heartbeats are bumped during replacement/promotion.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| core/txpool/legacypool/queue.go | Changes bump to only update existing heartbeats, avoiding seeding beats for accounts without queued txs. |
| core/txpool/legacypool/legacypool.go | Comment clarifications for Lifetime and for heartbeat bumping during tx replacement/promotion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // bump updates the heartbeat for the given account address. | ||
| // If the address is unknown, the call is a no-op. | ||
| func (q *queue) bump(addr common.Address) { | ||
| q.beats[addr] = time.Now() | ||
| if _, ok := q.beats[addr]; ok { | ||
| q.beats[addr] = time.Now() | ||
| } |
There was a problem hiding this comment.
Consider adding a regression test for the fixed heartbeat behavior: when queue.bump(addr) is called for an address with no queued txs, it should not pre-create a heartbeat that later prevents queue.add from initializing a fresh heartbeat. Without a test, it’s easy to reintroduce the bug where newly-queued txs can be treated as immediately stale because an old beats[addr] already exists.
a075967 to
e6e7773
Compare
Proposed changes
Ref: ethereum#33704
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