node: drop block work downloaded for an abandoned branch - #576
Conversation
|
Warning Review limit reached
Next review available in: 34 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe change adds active-chain generation tracking. It stamps downloaded chunks with the current generation and discards stale chunks after branch switches. Tests cover generation behavior and end-to-end chain initialization. ChangesChain generation reorganization
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant BlockDownloader
participant ChunkCoordinator
participant FastValidation
participant BlockStorage
participant HeaderIndex
BlockDownloader->>ChunkCoordinator: Read current generation
ChunkCoordinator->>HeaderIndex: generation()
HeaderIndex-->>ChunkCoordinator: Active generation
BlockDownloader->>FastValidation: Pass chunk with generation
FastValidation->>BlockStorage: Forward validated chunk
BlockStorage->>HeaderIndex: Compare generations
BlockStorage-->>BlockStorage: Store matching chunk or discard stale chunk
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/blockchain/test/reorg_chain_fixture.hpp`:
- Around line 44-50: In the chain_fixture destructor, remove the manual calls to
chain_->stop() and chain_->close(), and instead reset the chain_ and organizer_
member variables (using assignment to nullptr or default values) before calling
std::filesystem::remove_all on dir_. This ensures that chain_'s destructor runs
and completes all cleanup operations before the directory is deleted, preventing
issues when the block_chain destructor's own close() call tries to access the
now-deleted directory.
In `@src/database/src/header_index.cpp`:
- Around line 306-314: Update active_set_tip() in
src/database/src/header_index.cpp at lines 306-314 to publish the complete
replacement active chain first, including the null_index path, then increment
generation_ once afterward. In src/node/src/sync/block_tasks.cpp at lines
302-303, capture the generation before the get_block_hash() loop and use that
captured value for downloaded_chunk. Add an interleaving regression test that
switches branches after request hashes resolve but before the chunk is
forwarded.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a9f52d5-b4ea-441c-b099-7ddb5c29f687
📒 Files selected for processing (11)
src/blockchain/CMakeLists.txtsrc/blockchain/include/kth/blockchain/interface/block_chain.hppsrc/blockchain/src/interface/block_chain.cppsrc/blockchain/test/header_organizer.cppsrc/blockchain/test/reorg_chain_fixture.hppsrc/blockchain/test/reorg_e2e.cppsrc/database/include/kth/database/header_index.hppsrc/database/src/header_index.cppsrc/node/include/kth/node/sync/chunk_coordinator.hppsrc/node/include/kth/node/sync/messages.hppsrc/node/src/sync/block_tasks.cpp
e752b8d to
cb27698
Compare
Completes the reorg barrier. Storage and the UTXO build already park while a switch runs, so nothing writes concurrently — but a chunk that was in flight, or buffered in the channel behind the barrier, was still applied afterwards. Its heights name different blocks on the new chain, so storing it wrote the wrong data and marked the wrong hashes have_data. The barrier cannot fix that by growing: the work is already past the point where parking helps. What distinguishes it is not the height (both branches have one) but which chain it was requested for, so the pipeline stamps it: - header_index carries a generation, bumped by active_set_tip ONLY when the tip moves onto a different branch. Ordinary forward extension leaves it alone; bumping per block would discard good in-flight chunks on every new tip and stall sync instead of protecting it. - a downloaded chunk records the generation it was requested under. - block_storage_task drops chunks whose generation is stale rather than storing them. The counter lives on header_index rather than block_chain because that is where the active chain lives, and because the download task reaches it through the chunk coordinator, which has no chain reference. An earlier attempt added this counter with no consumer and it was removed in review, correctly — a documented contract the code did not honour. This is the consumer.
cb27698 to
80da789
Compare
1 participant
Completes the reorg barrier — the remaining gap flagged in #573.
The actual failure mode
Storage and the UTXO build already park while a switch runs, so nothing writes concurrently. That part was solved. What was not: a chunk already in flight, or buffered in the channel behind the barrier, was applied after the switch. Its heights name different blocks on the new chain, so storing it writes the wrong data and marks the wrong hashes
have_data.Growing the barrier cannot fix this — the work is already past the point where parking helps.
The discriminator
Height cannot distinguish the two branches (both have a block at height N). What can is which chain the work was requested for, so the pipeline stamps it:
header_indexcarries a generation, bumped byactive_set_tiponly when the tip moves onto a different branch. Ordinary forward extension leaves it alone — bumping per block would discard good in-flight chunks on every new tip and stall sync instead of protecting it. There is a test pinning exactly that.block_storage_taskdrops stale-generation chunks instead of storing them.The counter lives on
header_indexrather thanblock_chainbecause that is where the active chain lives, and because the download task reaches it through the chunk coordinator, which holds no chain reference.On the history here
An earlier revision of #573 added this same counter with no consumer, and review correctly called it out as a documented contract the code did not honour. It was removed. This is the consumer — the counter now has a reason to exist.
Full suites green (blockchain 1626, node 326); node-exe builds.
Remaining
The end-to-end reorg test (competing chains → disconnect → switch → re-download) is being built separately on top of the fixture in #575. Park/auto-unpark and mempool re-admit (#498) follow.