fix(core): use sync.Once for SenderCacher initialization #31029#2173
fix(core): use sync.Once for SenderCacher initialization #31029#2173gzliudan wants to merge 1 commit intoXinFinOrg:dev-upgradefrom
Conversation
gzliudan
requested review from
AnilChinchawale,
anunay-xin,
benjamin202410,
Copilot,
liam-lai and
wanwiset25
|
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
This PR changes how the global transaction sender recovery/caching helper is initialized, switching from eager package-level initialization to a sync.Once-guarded lazy singleton. This affects sender recovery invoked during chain insertion and txpool reorg reinjection.
Changes:
- Replace
core.SenderCacherpackage var with a lazily initialized singleton gettercore.SenderCacher(). - Update sender recovery call sites in blockchain insertion and legacy txpool reset to use the getter.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| core/sender_cacher.go | Introduces sync.OnceValue-based lazy initialization and an exported getter for the sender cacher singleton. |
| core/blockchain.go | Updates block insertion sender recovery to call SenderCacher() instead of referencing a global var. |
| core/txpool/legacypool/legacypool.go | Updates txpool reorg reinjection sender recovery to call core.SenderCacher(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // senderCacherOnce is used to ensure that the SenderCacher is initialized only once. | ||
| var senderCacherOnce = sync.OnceValue(func() *txSenderCacher { | ||
| return newTxSenderCacher(runtime.NumCPU()) | ||
| }) | ||
|
|
||
| // SenderCacher returns the singleton instance of SenderCacher, initializing it if called for the first time. | ||
| // This function is thread-safe and ensures that initialization happens only once. | ||
| func SenderCacher() *txSenderCacher { | ||
| return senderCacherOnce() | ||
| } |
There was a problem hiding this comment.
Changing SenderCacher from an exported variable to an exported function is a breaking API change for any downstream code importing core (call sites must switch from core.SenderCacher.Recover... to core.SenderCacher().Recover...). If preserving public API compatibility matters, consider keeping the exported var SenderCacher and moving the sync.Once-guarded initialization inside its methods (or introducing a new getter name while keeping the old var during a deprecation window).
| // Inject any transactions discarded due to reorgs | ||
| log.Debug("Reinjecting stale transactions", "count", len(reinject)) | ||
| core.SenderCacher.Recover(pool.signer, reinject) | ||
| core.SenderCacher().Recover(pool.signer, reinject) |
There was a problem hiding this comment.
core.SenderCacher() is now lazily initialized and may start runtime.NumCPU() goroutines on first use. In this code path reset is executed under pool.mu (see runReorg), so the first call could add noticeable latency while holding the txpool lock. Consider ensuring the cacher is initialized before taking the pool mutex (e.g., warm it up earlier in the reorg loop) so initialization work doesn’t happen in this critical section.
|
|
||
| // Start a parallel signature recovery (signer will fluke on fork transition, minimal perf loss) | ||
| SenderCacher.RecoverFromBlocks(types.MakeSigner(bc.chainConfig, chain[0].Number()), chain) | ||
| SenderCacher().RecoverFromBlocks(types.MakeSigner(bc.chainConfig, chain[0].Number()), chain) |
There was a problem hiding this comment.
SenderCacher() is now lazily initialized and will allocate the task channel and spawn worker goroutines on first use. insertChain runs with the chain mutex held, so the first call can shift initialization cost into a lock-held critical path. Consider initializing the cacher before acquiring bc.chainmu (or during blockchain startup) to avoid a one-time pause during the first chain insertion.
Proposed changes
Ref: ethereum#31029
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