refactor(core): drop dead stats accounting from insertChain abort paths - #2551
refactor(core): drop dead stats accounting from insertChain abort paths#2551gzliudan wants to merge 1 commit 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: Advanced 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.
🟢 Approval recommended
The focused cleanup removes only unreachable reporting state, with no unresolved correctness issues.
Pull request overview
Removes dead block-import statistics accounting from early-return paths without changing chain behavior.
Changes:
- Removes unused queued/ignored counter updates.
- Removes the obsolete
queuedfield, logging context, and iterator helpers.
File summaries
| File | Description |
|---|---|
core/blockchain.go |
Removes unobservable statistics updates from abort paths. |
core/blockchain_insert.go |
Removes obsolete stats state, logging, and helpers. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
stats is a function-local insertStats and stats.report is only called from the main import loop, so every stats write on a path that returns before reaching the loop is dead and never logged: - stats.queued += it.processed() on the first-block future path is not only unreachable for reporting but off by one: it.processed() returns it.index+1, which over-counts by one both when the whole batch is drained (it.index == len(chain) at exhaustion) and when the loop aborts on a non-exempt error (it.index points at the block that was never queued). Both queued counters date back to ff435e0. - stats.queued++ in the tail future loop (which also skipped the block that triggered the queueing) and stats.ignored += it.remaining() on both future paths are written then dropped on return. - stats.ignored += len(it.chain) in the first-block error abort has the same write-then-return shape. Remove all of them. Drop the insertStats.queued field and the "queued" log context in report() that could never fire once the counters are gone, matching upstream geth whose insertStats no longer carries a queued field, and remove the now-unused insertIterator helpers processed() and remaining(). The live stats.ignored accounting (the ErrKnownBlock skip loop that falls through to the import loop and the InsertReceiptChain stats) is untouched.
2579203 to
aad4974
Compare
Proposed changes
statsis a function-localinsertStatsininsertChainandstats.reportis only called from the main import loop, so every stats write on a path that returns before reaching the loop is dead and never logged. This PR removes all such writes:stats.queued += it.processed()on the first-block future path (dead and off by one —processed()returnsit.index+1, which over-counts both when the whole batch drains and when the loop aborts on a non-exempt error),stats.queued++in the tail future loop (which also skipped the block that triggered the queueing),stats.ignored += it.remaining()on both future paths, andstats.ignored += len(it.chain)in the first-block error abort.It also drops the
insertStats.queuedfield and thequeuedlog context inreport()that could never fire once the counters are gone, matching upstream geth whoseinsertStatsno longer carries aqueuedfield, and removes the now-unusedinsertIterator.processed()/remaining()helpers. The livestats.ignoredaccounting (theErrKnownBlockskip loop that falls through to the import loop, and theInsertReceiptChainstats) is untouched.No behavior change: all removed lines only mutated a function-local struct that was discarded on return without ever being logged. Verified with
gofmt,go build ./core/...andmake quick-test(no failures). Test coverage rationale: the removed statements are unreachable for reporting by construction, so there is no observable behavior to unit-test; the changes are compile-verified and the existing core test suite passes.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