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
The failure test does not exercise the claimed partial-import path and fails during unintended header validation.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Ensures temporary downloader marks are removed after batch imports.
Changes:
- Defers cleanup of all batch download marks.
- Adds success and failure-path regression coverage.
File summaries
| File | Description |
|---|---|
core/blockchain.go |
Removes temporary marks when import exits. |
core/blockchain_downloadingblock_test.go |
Tests mark cleanup. |
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.
insertChain marks every block of the batch in downloadingBlock before it verifies any of them, and nothing ever removed the mark. The cache keeps the fetcher off a block the downloader is importing, but nothing bounds its lifetime to that call: the entry lives on until the LRU evicts it, which can be a very long time. insertBlock reads that mark before anything else and answers with a plain success - no head movement, no error - so a later delivery of any block that once went through a batch import is dropped on the floor while the entry survives. A head that sits below an already executed block cannot be advanced through the fetcher path that way, and that path is how a node follows the chain. Clear the batch's marks on the way out, with a defer so that a batch which fails or is interrupted clears them too: the import is over either way, and nothing in it is a download in progress any more. The cleanup has been missing since the marks were introduced (9f36d37, "Parallel process block from fetcher"). It was found while reviewing the insertion-error work for XinFinOrg#2534.
d9e3c1c to
2c91ba1
Compare
Problem
insertChainwrites every block of a batch intodownloadingBlockbefore any of them is verified, and nothing ever removes the mark. The cache keeps the fetcher off a block the downloader is importing, but its lifetime is not bound to that call: the entry stays until the LRU evicts it.insertBlockreads the mark before anything else and returns a plain success - no head movement, no error - so a later delivery of any block that once went through a batch import is dropped on the floor while the entry survives. When the head sits below an already executed block, that path is how a node follows the chain, and the head cannot advance through it.Fix
Clear the batch's marks on the way out, with a
deferplaced after the loop that adds them. It covers the success, failure and interrupted exits alike: the import is over in all three, so nothing in it is a download in progress any more.The "skip while downloading" behaviour is left untouched, as are the cache's definition and construction.
Tests
TestInsertChainClearsDownloadingBlockMarks(core/blockchain_downloadingblock_test.go) covers a batch that succeeds and one that fails partway, asserting the marks are gone in both. It fails before the fix withblock #1 is still marked as downloading after insertChain returned.gofmt/goimports,make allandmake quick-testall pass.Attribution
The mark has been added without a cleanup since
9f36d37558("Parallel process block from fetcher", 2018); both the write and the read point predate the current baseline, so this is unrelated to #2534 and #2535. It was found while reviewing the work for those two issues - the adoption it adds for already executed blocks is exactly what a stale mark keeps out - and is therefore opened on its own. The patch has been verified withgit apply --checkto apply cleanly onfix-issue-2534-2535.