test: reorganize a real chain end to end, and fix what that found

[fpelliccioni]

Everything built for reorg support so far was verified in pieces. This stands up the whole cycle against the real stack.

The scenario

• A trunk to height 100.
• Branch A: one block at 101 spending the coinbase of block 1 — which matures exactly there (101 − 1 = 100 confirmations).
• Branch B: three blocks from height 100, outweighing A's single one.
• Connect A, switch to B, connect B.

Driven through the same tasks the sync coordinator drives (block_storage_task, utxo_build_task, execute_reorg) against the same header index, flat-file block store, undo files and UTXO-Z the node runs on.

The blocks satisfy consensus — real proof of work, real merkle roots, a real signature over a real prevout — so the connect path runs full validation rather than the shortcuts a synthetic block slips through. Regtest makes that cheap: a hundred blocks are mined in milliseconds. test/regtest_miner.hpp holds the miner; blocks are serialized and re-parsed as light_block so what reaches storage arrived the same way a block off the wire does.

What it verifies

The spend really happened before the reorg (the coinbase it consumed is out of the set, the output it created is in) — without that the undo below would be undoing nothing and everything after it would pass vacuously. Then, after the switch: A's coinbase and its spend are gone from the UTXO set, the coinbase A spent is back at its original creation height (not at 101, which would misdate its maturity and its median time past), heights 101–103 name B's blocks, the validated tip and the UTXO-built height both reach 103, A's abandoned block is still on disk, and a header extending the new tip is read as a plain extension.

The five defects it found

All five are on the path a real reorg takes; none is reachable by a test that stops at the pieces.

1. The header table cannot be rewritten at a height. Headers are written with LMDB's APPEND, which assumes keys only ever grow, so after a switch the abandoned branch's header stays at that height forever — and every reader that addresses blocks by height (median time past, staleness, the RPC surface) answers from the branch the node left. The duplicate was logged and then swallowed as success. A collision now rewrites the entry; the sequential path keeps APPEND.

2. block_storage_task walked the header index by height. The index numbers entries in arrival order, so since side branches are stored (#570) an entry's index is not its height. The contiguous scan counted the abandoned branch's blocks and pushed the validated tip one past where the chain actually reached — visible in the test as last_block_height = 104 on a 103-block chain. Resolved through the active chain now.

3. persist_headers_to_db had the same confusion, and persisted whichever header happened to sit at that index.

4. The organizer kept the abandoned branch as its tip. It decides fork-versus-extension by comparing against the tip it remembers; left behind, it reads every later batch as another fork and asks for a switch whose fork height no longer matches — so switch_to_branch rejects it and header sync stops advancing for good. Adds header_organizer::adopt_tip, and node::sync::execute_reorg to hold the sequence (park the writers at the barrier, switch, move the tip) in one place — which is also what lets the test drive the same thing the coordinator does instead of a re-implementation of it.

5. to_pay_public_key_hash_pattern_unlocking emitted a leading OP_0. That is the dummy multisig needs; in P2PKH it survives to the end of the script and fails CLEANSTACK. The placeholder used for size estimates carried it too, so coin selection was estimating one byte over. No production caller builds real unlocking scripts with it today, which is why nothing had failed yet.

Scope

The barrier is satisfied trivially here (no task is running at the moment of the switch) — its own behaviour is covered by the drain tests in #576. Compact UTXO mode is not exercised: it needs a separate build configuration, so it stays a follow-up.

Local: kth_blockchain_test 1919 assertions / 176 cases, kth_node_test 616 / 110 (the cycle itself is 266 assertions), kth_domain_test 1008351 / 3821, node-exe builds.

Summary by CodeRabbit

• New Features

  • Added safer support for switching to a heavier blockchain branch during synchronization.
  • Improved header persistence and recovery after chain reorganizations.
  • Added regtest helpers for creating valid transactions and mining blocks.

• Bug Fixes

  • Prevented abandoned side-branch blocks from extending validated storage ranges.
  • Corrected P2PKH unlocking scripts by removing an unnecessary multisig placeholder.
  • Improved consistency when adopting a new chain tip.

• Tests

  • Added end-to-end coverage for reorganizations, including UTXO rollback, restoration, and continued block connection.

[coderabbitai]

Warning

Review limit reached

@fpelliccioni, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 41 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: fc61cde0-be6f-4a24-b8fc-776046257f0d

📥 Commits

Reviewing files that changed from the base of the PR and between f8922dc and 11e7d4b.

⛔ Files ignored due to path filters (1)

• src/database/include/kth/database/databases/header_database.ipp is excluded by !**/*.ipp and included by src/database/**

📒 Files selected for processing (21)

• src/blockchain/include/kth/blockchain/interface/block_chain.hpp
• src/blockchain/include/kth/blockchain/pools/header_organizer.hpp
• src/blockchain/src/interface/block_chain.cpp
• src/blockchain/src/pools/header_organizer.cpp
• src/blockchain/test/header_organizer.cpp
• src/blockchain/test/regtest_miner.hpp
• src/database/include/kth/database/data_base.hpp
• src/database/include/kth/database/databases/internal_database.hpp
• src/database/src/data_base.cpp
• src/domain/src/chain/script.cpp
• src/domain/test/chain/script.cpp
• src/node/CMakeLists.txt
• src/node/include/kth/node/full_node.hpp
• src/node/include/kth/node/sync/orchestrator.hpp
• src/node/include/kth/node/sync/reorg.hpp
• src/node/src/executor/executor.cpp
• src/node/src/full_node.cpp
• src/node/src/sync/block_tasks.cpp
• src/node/src/sync/orchestrator.cpp
• src/node/src/sync/reorg.cpp
• src/node/test/reorg_cycle.cpp

📝 Walkthrough

Walkthrough

The PR adds synchronized header-tip adoption, asynchronous chain reorganization, active-chain header persistence, transactional header replacement, active-chain storage checks, corrected P2PKH unlocking scripts, and end-to-end reorganization tests.

Changes

Reorganization synchronization

Layer / File(s)	Summary
Header tip coordination src/blockchain/include/.../header_organizer.hpp, src/blockchain/src/pools/header_organizer.cpp, src/blockchain/test/header_organizer.cpp	The header organizer uses an atomic tip index and mutex-protected publication. adopt_tip updates active mappings. Header batches reject stale publication races.
Chain switching and header replacement src/blockchain/include/.../block_chain.hpp, src/blockchain/src/interface/block_chain.cpp, src/database/include/..., src/database/src/data_base.cpp	switch_to_branch rewinds UTXO state without publishing the active branch. New APIs replace persisted headers from a specified height.
Asynchronous reorganization execution src/node/include/.../sync/reorg.hpp, src/node/src/sync/reorg.cpp, src/node/src/sync/orchestrator.cpp, src/node/src/sync/block_tasks.cpp, src/node/CMakeLists.txt	The node coordinates reorganization barriers, writer pausing, branch switching, tip adoption, active-chain header persistence, abort handling, and active-chain storage checks.
Consensus-valid reorganization coverage src/blockchain/test/regtest_miner.hpp, src/domain/src/chain/script.cpp, src/domain/test/chain/script.cpp, src/node/test/reorg_cycle.cpp	The tests add regtest mining and spending helpers, remove the P2PKH multisig dummy, and verify branch switching, UTXO changes, retained data, reconnection, and later header extension.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Orchestrator
  participant ExecuteReorg
  participant BlockChain
  participant HeaderOrganizer
  Orchestrator->>ExecuteReorg: request reorganization
  ExecuteReorg->>BlockChain: pause writers and wait for barrier
  ExecuteReorg->>BlockChain: switch to branch head
  BlockChain-->>ExecuteReorg: return switch result
  ExecuteReorg->>HeaderOrganizer: adopt new tip
  ExecuteReorg->>BlockChain: replace active headers
  ExecuteReorg-->>Orchestrator: return reorg outcome

Loading

Possibly related PRs

• k-nuth/kth#570: Modifies related header_organizer branch and tip handling.
• k-nuth/kth#571: Shares reorganization infrastructure and tip adoption flow.
• k-nuth/kth#573: Introduces related reorganization flow changes in blockchain and node synchronization code.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 48.57% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the end-to-end reorganization test and the fixes identified by it.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch test/reorg-10-full-cycle

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 5

🧹 Nitpick comments (2)

src/node/test/reorg_cycle.cpp (1)

129-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert that the tasks finished before the timeout expires.

run_for returns when the work completes or when 120 seconds elapse. The test does not distinguish these cases here. If the tasks stall, the failure surfaces later as a wrong height and hides the real cause. Add an explicit progress check after run_for.

♻️ Proposed refactor

     ctx.run_for(std::chrono::seconds(120));
+
+    // Fail at the point of the stall, not at a later height assertion.
+    auto const built = chain.get_utxo_built_height();
+    REQUIRE(built);
+    REQUIRE(*built >= end_height);

🤖 Prompt for 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.

In `@src/node/test/reorg_cycle.cpp` around lines 129 - 133, Update the test flow
around ctx.run_for in reorg_cycle so it explicitly verifies that all tasks
completed before the timeout, using the progress/completion state exposed by the
test context. Fail immediately when the 120-second deadline expires, then retain
the output draining loop for completed tasks.

src/blockchain/test/regtest_miner.hpp (1)

146-153: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Bound the nonce loop and hash the header only.

The loop has no exit other than a valid proof of work. nonce wraps at UINT32_MAX, so a failure to find a solution becomes a silent infinite loop instead of a test failure. The loop also copies the full transaction list on each attempt, which is unnecessary because only the nonce changes. Mutate the header of a single block and add a bound.

♻️ Proposed refactor

-    for (uint32_t nonce = 0; ; ++nonce) {
+    for (uint64_t nonce = 0; nonce <= std::numeric_limits<uint32_t>::max(); ++nonce) {
         domain::chain::block attempt(
-            domain::chain::header{0x20000000, prev, merkle, timestamp, regtest_bits, nonce},
+            domain::chain::header{0x20000000, prev, merkle, timestamp, regtest_bits, uint32_t(nonce)},
             domain::chain::transaction::list(candidate.transactions()));
         if (attempt.header().is_valid_proof_of_work(attempt.hash(), /*retarget*/ false)) {
             return attempt;
         }
     }
+    throw std::runtime_error("regtest_miner: no nonce satisfied the regtest target");

🤖 Prompt for 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.

In `@src/blockchain/test/regtest_miner.hpp` around lines 146 - 153, Update the
mining loop around the regtest block construction to reuse one block/header and
mutate only its nonce, avoiding reconstruction and transaction-list copies on
each attempt. Bound nonce attempts so exhaustion returns a test failure instead
of wrapping indefinitely, while validating proof of work against the updated
header hash only.

🤖 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/regtest_miner.hpp`:
- Around line 111-119: Validate the result of create_endorsement before
dereferencing it in the signed-input construction. If the expected is
unsuccessful, report or propagate its error using the test’s existing
failure-handling convention; only pass the endorsement value to
to_pay_public_key_hash_pattern_unlocking after confirming has_value().

In `@src/node/src/sync/orchestrator.cpp`:
- Around line 888-890: After execute_reorg establishes a fork by making a side
branch active, subsequent request construction paths incorrectly convert
headers_synced_to directly to header indices without accounting for the new
active chain. Replace the direct index conversions of headers_synced_to with
active_at() lookups at each get_hash() call site (around lines 952, 982, 1005,
1070, 1124, and 1206) to resolve the correct entry indices within the now-active
branch, and handle the case where active_at() returns null_hash.

In `@src/node/src/sync/reorg.cpp`:
- Around line 29-49: Use an RAII guard immediately after
chain.request_reorg_pause(true) in the reorganization coroutine to ensure
request_reorg_pause(false) runs during normal completion, abort returns, and
exception unwinding. Remove the manual release calls and preserve the existing
abort logging and outcome handling.

In `@src/node/test/reorg_cycle.cpp`:
- Line 302: Update the test assertion around index.find(a101.hash()) to first
require that the lookup result is not header_index::null_index, then pass the
validated index to has_block_data. Keep the existing block-data assertion after
the explicit lookup check.
- Around line 224-225: Update the assertions around chain.get_last_heights() and
chain.get_utxo_built_height() to check each optional has a value before
dereferencing it, matching the safe assertion pattern used near lines 195-202.
Apply the same guarded form to the unchecked assertions near lines 283 and
290-291, preserving the expected height checks.

---

Nitpick comments:
In `@src/blockchain/test/regtest_miner.hpp`:
- Around line 146-153: Update the mining loop around the regtest block
construction to reuse one block/header and mutate only its nonce, avoiding
reconstruction and transaction-list copies on each attempt. Bound nonce attempts
so exhaustion returns a test failure instead of wrapping indefinitely, while
validating proof of work against the updated header hash only.

In `@src/node/test/reorg_cycle.cpp`:
- Around line 129-133: Update the test flow around ctx.run_for in reorg_cycle so
it explicitly verifies that all tasks completed before the timeout, using the
progress/completion state exposed by the test context. Fail immediately when the
120-second deadline expires, then retain the output draining loop for completed
tasks.

🪄 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: ffdadbea-7a82-4ca2-a987-cd971ab140cb

📥 Commits

Reviewing files that changed from the base of the PR and between a7e94d2 and b5d28c4.

⛔ Files ignored due to path filters (1)

• src/database/include/kth/database/databases/header_database.ipp is excluded by !**/*.ipp and included by src/database/**

📒 Files selected for processing (10)

• src/blockchain/include/kth/blockchain/pools/header_organizer.hpp
• src/blockchain/src/pools/header_organizer.cpp
• src/blockchain/test/regtest_miner.hpp
• src/domain/src/chain/script.cpp
• src/node/CMakeLists.txt
• src/node/include/kth/node/sync/reorg.hpp
• src/node/src/sync/block_tasks.cpp
• src/node/src/sync/orchestrator.cpp
• src/node/src/sync/reorg.cpp
• src/node/test/reorg_cycle.cpp

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

src/node/include/kth/node/sync/reorg.hpp (1)

37-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Move the execute_reorg documentation next to its declaration.

The block at Lines 37-51 documents execute_reorg, but the persist_active_headers block (Lines 52-62) and its declaration (Lines 63-68) come between them. A reader attaches the first block to persist_active_headers.

Move Lines 37-51 to directly above Line 70.

🤖 Prompt for 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.

In `@src/node/include/kth/node/sync/reorg.hpp` around lines 37 - 68, Move the
documentation block describing the reorganization sequence, abort behavior, and
chain switching directly above the execute_reorg declaration. Keep the
persist_active_headers documentation immediately above persist_active_headers,
so each comment is adjacent to its corresponding declaration.

🤖 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/include/kth/blockchain/interface/block_chain.hpp`:
- Around line 151-154: Guard the truncate_headers_from declaration in
block_chain.hpp and its definition in block_chain.cpp with the matching `#if`
!defined(KTH_DB_READONLY) guard, alongside the database truncate_headers_from
availability. Update both affected sites: the header declaration and the
block_chain.cpp implementation; no other behavior needs to change.

In `@src/node/src/sync/reorg.cpp`:
- Around line 113-132: Guard the header truncation path in the reorg flow before
converting new_tip_height to size_t: reject non-positive values and do not call
chain.truncate_headers_from when active_tip_height() is -1 or lower. Preserve
the existing truncation and error logging for positive tip heights, ensuring the
requested truncation height remains at least 1.

---

Nitpick comments:
In `@src/node/include/kth/node/sync/reorg.hpp`:
- Around line 37-68: Move the documentation block describing the reorganization
sequence, abort behavior, and chain switching directly above the execute_reorg
declaration. Keep the persist_active_headers documentation immediately above
persist_active_headers, so each comment is adjacent to its corresponding
declaration.

🪄 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: c0d41180-8373-494d-90ba-dcda77d4f367

📥 Commits

Reviewing files that changed from the base of the PR and between b5d28c4 and dcf32ad.

⛔ Files ignored due to path filters (1)

• src/database/include/kth/database/databases/header_database.ipp is excluded by !**/*.ipp and included by src/database/**

📒 Files selected for processing (17)

• src/blockchain/include/kth/blockchain/interface/block_chain.hpp
• src/blockchain/include/kth/blockchain/pools/header_organizer.hpp
• src/blockchain/src/interface/block_chain.cpp
• src/blockchain/src/pools/header_organizer.cpp
• src/blockchain/test/header_organizer.cpp
• src/blockchain/test/regtest_miner.hpp
• src/database/include/kth/database/data_base.hpp
• src/database/include/kth/database/databases/internal_database.hpp
• src/database/src/data_base.cpp
• src/domain/src/chain/script.cpp
• src/domain/test/chain/script.cpp
• src/node/CMakeLists.txt
• src/node/include/kth/node/sync/reorg.hpp
• src/node/src/sync/block_tasks.cpp
• src/node/src/sync/orchestrator.cpp
• src/node/src/sync/reorg.cpp
• src/node/test/reorg_cycle.cpp

🚧 Files skipped from review as they are similar to previous changes (5)

• src/domain/src/chain/script.cpp
• src/blockchain/test/regtest_miner.hpp
• src/node/src/sync/block_tasks.cpp
• src/node/test/reorg_cycle.cpp
• src/node/CMakeLists.txt

[coderabbitai]

🧹 Nitpick comments (3)

src/node/src/sync/reorg.cpp (1)

91-100: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Bound the barrier wait.

The loop exits only when the barrier is reached or abort returns true. A participant that registers and never parks keeps the pause raised, and every registered writer stays stopped. Add a deadline: after it expires, log the parked/registered counts and return a failed outcome instead of waiting further.

♻️ Proposed deadline

+    constexpr auto barrier_timeout = std::chrono::seconds(60);
+    auto const deadline = std::chrono::steady_clock::now() + barrier_timeout;
     while ( ! chain.reorg_barrier_reached() && ! abort()) {
+        if (std::chrono::steady_clock::now() >= deadline) {
+            spdlog::error("[reorg] The barrier was not reached within {}s; the chain was not touched",
+                barrier_timeout.count());
+            co_return reorg_outcome{};
+        }
         ::asio::steady_timer wait_timer(executor);
         wait_timer.expires_after(std::chrono::milliseconds(50));
         co_await wait_timer.async_wait(::asio::as_tuple(::asio::use_awaitable));
     }

🤖 Prompt for 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.

In `@src/node/src/sync/reorg.cpp` around lines 91 - 100, Bound the barrier-wait
loop in the reorg coroutine so it cannot wait indefinitely when the barrier is
never reached. Add a deadline alongside the existing abort handling; when it
expires, log the current parked and registered counts, then return a failed
reorg_outcome without touching the chain. Preserve the existing successful
barrier and abort paths.

src/database/src/data_base.cpp (1)

292-298: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Define and enforce the degenerate inputs of the header-replacement API. The new replacement path accepts an empty headers list and a start_height of 0. Neither value has a defined outcome: the removal range is expressed relative to the last supplied header, and height 0 holds genesis.

• src/database/src/data_base.cpp#L292-L298: return error::success for an empty headers list, as push_headers_batch does, and reject start_height == 0 before the call to internal_db_->replace_headers_from.
• src/database/include/kth/database/databases/internal_database.hpp#L113-L128: state the behavior for an empty headers list in the contract, next to the existing start_height requirement.

🤖 Prompt for 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.

In `@src/database/src/data_base.cpp` around lines 292 - 298, Update
data_base::replace_headers_from in src/database/src/data_base.cpp: return
error::success immediately when headers is empty, and reject start_height == 0
before calling internal_db_->replace_headers_from. Update the contract near the
existing start_height requirement in
src/database/include/kth/database/databases/internal_database.hpp to explicitly
document the empty-headers behavior.

src/node/include/kth/node/sync/reorg.hpp (1)

55-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Move the execute_reorg documentation next to its declaration.

Lines 55-69 describe execute_reorg, but struct reorg_outcome sits between them and the declaration at line 85. Line 70 starts the reorg_outcome description with no blank line, so the two blocks read as one. Move the execute_reorg paragraphs to just above line 85 and keep the reorg_outcome paragraph with the struct.

🤖 Prompt for 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.

In `@src/node/include/kth/node/sync/reorg.hpp` around lines 55 - 90, Separate the
documentation blocks in the reorg declarations: keep the “What a switch left
behind...” comment immediately above struct reorg_outcome, and move the
execute_reorg workflow and abort-behavior paragraphs to immediately above the
execute_reorg declaration. Add the necessary blank-line separation so each
comment clearly documents its corresponding symbol.

🤖 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.

Nitpick comments:
In `@src/database/src/data_base.cpp`:
- Around line 292-298: Update data_base::replace_headers_from in
src/database/src/data_base.cpp: return error::success immediately when headers
is empty, and reject start_height == 0 before calling
internal_db_->replace_headers_from. Update the contract near the existing
start_height requirement in
src/database/include/kth/database/databases/internal_database.hpp to explicitly
document the empty-headers behavior.

In `@src/node/include/kth/node/sync/reorg.hpp`:
- Around line 55-90: Separate the documentation blocks in the reorg
declarations: keep the “What a switch left behind...” comment immediately above
struct reorg_outcome, and move the execute_reorg workflow and abort-behavior
paragraphs to immediately above the execute_reorg declaration. Add the necessary
blank-line separation so each comment clearly documents its corresponding
symbol.

In `@src/node/src/sync/reorg.cpp`:
- Around line 91-100: Bound the barrier-wait loop in the reorg coroutine so it
cannot wait indefinitely when the barrier is never reached. Add a deadline
alongside the existing abort handling; when it expires, log the current parked
and registered counts, then return a failed reorg_outcome without touching the
chain. Preserve the existing successful barrier and abort paths.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: a5ab31d3-2702-4eb8-a3ad-b9caea67a255

📥 Commits

Reviewing files that changed from the base of the PR and between dcf32ad and f8922dc.

⛔ Files ignored due to path filters (1)

• src/database/include/kth/database/databases/header_database.ipp is excluded by !**/*.ipp and included by src/database/**

📒 Files selected for processing (17)

• src/blockchain/include/kth/blockchain/interface/block_chain.hpp
• src/blockchain/include/kth/blockchain/pools/header_organizer.hpp
• src/blockchain/src/interface/block_chain.cpp
• src/blockchain/src/pools/header_organizer.cpp
• src/blockchain/test/header_organizer.cpp
• src/blockchain/test/regtest_miner.hpp
• src/database/include/kth/database/data_base.hpp
• src/database/include/kth/database/databases/internal_database.hpp
• src/database/src/data_base.cpp
• src/domain/src/chain/script.cpp
• src/domain/test/chain/script.cpp
• src/node/CMakeLists.txt
• src/node/include/kth/node/sync/reorg.hpp
• src/node/src/sync/block_tasks.cpp
• src/node/src/sync/orchestrator.cpp
• src/node/src/sync/reorg.cpp
• src/node/test/reorg_cycle.cpp

🚧 Files skipped from review as they are similar to previous changes (8)

• src/node/CMakeLists.txt
• src/blockchain/test/header_organizer.cpp
• src/node/src/sync/block_tasks.cpp
• src/domain/src/chain/script.cpp
• src/blockchain/include/kth/blockchain/pools/header_organizer.hpp
• src/node/src/sync/orchestrator.cpp
• src/blockchain/src/pools/header_organizer.cpp
• src/blockchain/test/regtest_miner.hpp