Conversation
After a reorg a child indexer kept one section too many: it reported N+1 sections while the parent had already dropped the blocks that section covers, and the count only came back down once the new fork blocks were written and verifyLastHead() rolled the stale section back. TestChainIndexerWithChildren trips over that window and fails with 'Canonical section count mismatch: have N, want N+1' whenever the random section sizes make a child section straddle the reorg point. newHead() cascaded the reorg to children as changed*sectionSize, the first invalidated block, while the prefix it keeps itself ends at changed*sectionSize-1, the number the forward cascade hands over. A child taking the invalidated block for a valid one keeps the section it built before the reorg that covers it. Cascade the end of the retained prefix instead, so a child can only keep sections the parent itself still indexes, and mirror the same number in testChainIndexBackend.reorg() so the test model stays in step. Add TestMinReorgStraddle, a deterministic two level case (parent sectionSize 1, child sectionSize 2, reorg at block 50) that fails before the change and passes after it, instead of relying on the random parameters of TestChainIndexerWithChildren to hit the window. No upstream fix to port: go-ethereum never changed this cascade after ethereum#19748 and ethereum#20506, which this tree already carries, and it removed the chain indexer along with the old bloombits logic in ethereum#31081.
|
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 |
a15dbd8 to
92b6c64
Compare
gzliudan
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
The empty retained-prefix case still incorrectly preserves child data as though block zero were retained.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes child chain-indexer section counts after reorgs.
Changes:
- Cascades the retained section’s final block.
- Updates test reorg expectations.
- Adds deterministic straddling-section coverage.
File summaries
| File | Description |
|---|---|
core/chain_indexer.go |
Corrects reorg cascading. |
core/chain_indexer_test.go |
Updates expected cascade boundaries. |
core/chain_indexer_reorg_test.go |
Adds deterministic regression coverage. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This fixes the intermittent
TestChainIndexerWithChildrenfailure:https://github.com/XinFinOrg/XDPoSChain/actions/runs/35316718324/job/105509861843
After a reorg a child indexer kept one section too many.
newHead()cascaded the reorg to children aschanged*sectionSize, the first invalidated block, while the prefix it keeps itself ends atchanged*sectionSize-1, the number the forward cascade hands over. A child taking the invalidated block for a valid one kept the section it had built before the reorg that covers it, so its count stayed one above the blocks really on the chain untilverifyLastHead()rolled that section back once the new fork was written. The cascade now hands over the end of the retained prefix, so a child can only keep sections the parent itself still indexes, and the reorg expectation helper of the test mirrors the same number.Not an intermittent timing failure: with section sizes that make a child section straddle the reorg point it fails every time, which
TestMinReorgStraddlenow pins down deterministically (parent sectionSize 1, child sectionSize 2, reorg at block 50, new fork above it). Without the fix it fails 3/3 withhave 25, want 26; with it the case passes in 0.01s.No upstream fix exists for this one: go-ethereum never changed this cascade after ethereum#19748 and ethereum#20506, which this tree already carries, and it removed the chain indexer together with the old bloombits logic in ethereum#31081.
AddChildIndexerhas no caller outside the tests, so the reorg cascade only runs there today.Verification:
make all,make quick-testand one fullmake testat this commit. Under load (8 busy loops plus 12 concurrent package test runs) the randomizedTestChainIndexerWithChildrenfailed about once per 150 runs before the change and 240 runs stayed green with it.