docs(core/txpool): notice Clear is not for production #31567#2184
docs(core/txpool): notice Clear is not for production #31567#2184gzliudan wants to merge 2 commits intoXinFinOrg:dev-upgradefrom
Clear is not for production #31567#2184Conversation
…thereum#28837 Improve txpool loop synchronization around background resets. This change: - adds an explicit termination channel to signal pool shutdown - tracks forced-reset intent and a waiter channel inside the reset loop - ensures reset waiters are notified on completion or on pool termination - allows an explicit sync request path to trigger an additional reset round when needed Scope is limited to internal txpool concurrency control in core/txpool/txpool.go, with no protocol or RPC behavior change.
|
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)
📝 Coding Plan
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 aims to clarify (and enforce via comments/API) that certain txpool maintenance operations are intended for tests/simulator usage and can be unsafe in production contexts.
Changes:
- Adds a
TxPooltermination signal (term) and a test/simulator synchronization mechanism (sync) plus a new exportedTxPool.Sync()helper. - Extends the txpool event loop to support “forced reset” requests and notify a waiting caller when the reset work has completed.
- Updates
LegacyPoolcomments to warn thatClear()is not appropriate for production/live usage.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| core/txpool/txpool.go | Introduces new channels/Sync API and modifies the reset loop to support forced deterministic resets (test/simulator). |
| core/txpool/legacypool/legacypool.go | Documentation updates: clarifies Add(..., sync) semantics and adds a production-safety warning to Clear(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Sync is a helper method for unit tests or simulator runs where the chain events | ||
| // are arriving in quick succession, without any time in between them to run the | ||
| // internal background reset operations. This method will run an explicit reset | ||
| // operation to ensure the pool stabilises, thus avoiding flakey behavior. |
| // Notify the live reset waiter without blocking if the txpool is closed. | ||
| defer func() { | ||
| if resetWaiter != nil { | ||
| select { | ||
| case resetWaiter <- errors.New("pool already terminated"): | ||
| default: | ||
| } |
| // Sync is a helper method for unit tests or simulator runs where the chain events | ||
| // are arriving in quick succession, without any time in between them to run the | ||
| // internal background reset operations. This method will run an explicit reset | ||
| // operation to ensure the pool stabilises, thus avoiding flakey behavior. | ||
| // | ||
| // Note, this method is only used for testing and is susceptible to DoS vectors. | ||
| // In production code, the pool is meant to reset on a separate thread. | ||
| func (p *TxPool) Sync() error { |
Proposed changes
Ref: ethereum#31567
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