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.
🟡 Changes recommended
Stateful fork insertion retains an unguarded canonical-head TD dereference.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Prevents nil dereferences when total difficulty data is missing during sidechain and competing-block processing.
Changes:
- Adds
errMissingTotalDifficultyand guards three TD comparisons. - Adds regression tests for missing parent and local TD data.
File summaries
| File | Description |
|---|---|
core/blockchain.go |
Reports missing TD instead of dereferencing nil. |
core/blockchain_test.go |
Tests missing-TD insertion paths. |
Review details
- Files reviewed: 2/2 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.
c135070 to
b68ec05
Compare
… nil The total difficulty the chain weighs a block against comes out of the database, and GetTd answers nil when there is no entry for the block. Comparisons were built on that value without a nil check - the parent's difficulty in the insertSidechain scan, the head's difficulty right after that scan, the head's difficulty in writeBlockWithState, the path every stateful block import goes through, and the two in getResultBlock - so a node whose database is missing one of those entries crashed on a nil pointer dereference instead of reporting anything, on paths that run for every sidechain import, every imported block and every competing block. The stateful path is the easiest to reach: a recently executed side-fork block keeps its parent's difficulty readable, so importing its child needs only the head's entry to be gone. Report errMissingTotalDifficulty instead. The blocks themselves say nothing about a number this node cannot read, so the error names what this node cannot do instead of blaming them. On the stateful path the guard sits before the block and its state are written, since without the head's number there is no way to tell canonical from side. Classifying it as a local condition - one the downloader cancels the content processing for rather than dropping the peer - belongs to the change that introduces the local-insert sentinels. Each site is covered by a regression test that drops the entry from both the database and the read cache: the sidechain scan's parent and head, the stateful insertion of a child of a side-fork block, and the competing-block lookup.
b68ec05 to
1039647
Compare
Summary
BlockChain.GetTdanswersnilwhen the database has no total difficulty entry for the block, and three comparisons built on that value dereferenced it:insertSidechainscan (core/blockchain.go:2081-2083ondev-upgrade):2102-2103)getResultBlock(:2232-2233), the competing-block pathA node whose database is missing one of those entries crashed with a nil pointer dereference instead of reporting anything, on a path that runs for every sidechain import and every competing block. All three now log a warning and report
errMissingTotalDifficultyinstead.Why a new error value, and what it does not do
errMissingTotalDifficultyis unexported and names what this node cannot do rather than blaming the block. Classifying it as a local condition - one the downloader cancels the content processing for instead of dropping the peer - belongs to #2566, which introducescore.IsLocalInsertError; once that lands, one line adds this error to the whitelist. Until then the failure surfaces the way any other unclassifiedInsertChainerror does, but the node no longer crashes.Tests
core/blockchain_test.go:TestInsertSidechainReportsMissingParentTd- the segment's parent has no total difficulty on diskTestInsertSidechainReportsMissingLocalTd- the segment carries one, the head's cannot be readTestGetResultBlockReportsMissingTd- the competitor's parent total difficulty is unreadableAll three fail with
runtime error: invalid memory address or nil pointer dereferencewhen the guards are removed, and pass with them.They are adapted to the
dev-upgradeshape of the code under test:insertSidechaintakes two parameters and the scan is driven throughnewInsertIteratorwith a synthetic result channel, andTestInsertSidechainReportsMissingLocalTdstores its side block withwriteBlockWithoutStateinstead of re-importing a canonical block - the canonical re-import shape depends on the ghost-state false positive that only #2566 fixes.