Skip to content

database: give an undo record the identity that lets a restart find it again - #604

Merged
fpelliccioni merged 1 commit into
masterfrom
test/reorg-after-restart-603
Aug 5, 2026
Merged

database: give an undo record the identity that lets a restart find it again#604
fpelliccioni merged 1 commit into
masterfrom
test/reorg-after-restart-603

Conversation

@fpelliccioni

@fpelliccioni fpelliccioni commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

The problem

Undo positions live only in the header index, which is rebuilt from disk at
startup — and that rebuild restored block positions and nothing else. So after
any ordinary restart no block had undo data, the bytes sat unreachable in the rev
files, and a reorganization that had to roll back a block connected before the
restart could not run. This is the state after every clean shutdown, not a crash
window.

Nothing already on disk could attribute a record to its block. Order cannot: a
rev file holds undo for a subset of the blocks in its blk file, and the ones that
never got any leave no gap. The checksum cannot: it is seeded with the parent
hash, so every sibling validates the same record — and two siblings built from
the same transactions with different coinbases produce byte-identical undo data.

The solution

The record carries its owning block's hash, behind a marker of its own so the two
formats can be told apart. scan_undo_positions mirrors scan_block_positions
and runs beside it at startup, validating each record — marker, size, resolvable
owner, payload, checksum — and publishing nothing until every file has been read.

A record naming a block the index no longer holds is skipped and counted: after a
reorganization, a branch that lost is forgotten while its records remain. What
keeps that from hiding a damaged active record is a coverage check — every block
between the checkpoint and the built height must have undo — read from the
by-height table recorded while headers load, since the organizer publishes the
active view only after startup.

A durable index keyed by block hash was the alternative and is worse: it
reintroduces the window #600 is about, between writing the bytes and writing the
reference, across two stores that then need ordering.

Format incompatibility

Databases written before this keep records with no identity, and the association
cannot be recovered from what is on disk. Startup reports the format and refuses,
asking for a rebuild, rather than starting a node that cannot reorganize.

Tests

Two node regressions: connect, restart, then reorganize away blocks connected
before the restart — which fails on master in three places; and altering a single
byte of an active record's block hash, which must stop the start.

Seventeen database-level cases driving block_store against real files: round
trip; sibling identity; legacy records, on both the scan and the read path;
truncated payloads and partial headers; invalid markers; altered contents;
zeroes followed by content; unattributable and duplicate records, including a duplicate the index no longer holds; implausible payload sizes; blocks with no
record; several rev files; and a file that exists and will not open.

Full blockchain and node suites pass.

Closes #603. Independent of #602, which stays draft.

Summary by CodeRabbit

  • Bug Fixes

    • Improved startup recovery and validation of blockchain headers and undo data.
    • Detects corrupted, incomplete, outdated, duplicated, or unrecognized undo records and prevents unsafe startup.
    • Ensures undo data remains correctly associated with its block and supports reliable chain reorganizations after restart.
  • Tests

    • Added comprehensive coverage for undo scanning, corruption handling, legacy formats, truncated files, and restart-based reorganizations.

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

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

Next review available in: 29 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: 930cda34-8613-4a89-aaab-5cbddd312a0e

📥 Commits

Reviewing files that changed from the base of the PR and between a323506 and bb0784e.

📒 Files selected for processing (9)
  • src/blockchain/CMakeLists.txt
  • src/blockchain/src/interface/block_chain.cpp
  • src/blockchain/src/utxo_builder.cpp
  • src/blockchain/test/undo_scan.cpp
  • src/database/include/kth/database/block_store.hpp
  • src/database/include/kth/database/native_file.hpp
  • src/database/src/block_store.cpp
  • src/database/src/flat_file_seq.cpp
  • src/node/test/reorg_cycle.cpp
📝 Walkthrough

Walkthrough

The block store now writes versioned undo records with owning block hashes, scans and validates them at startup, restores undo positions in the header index, and rejects invalid or incompatible data. Tests cover scan failures, attribution, recovery, and post-restart reorganizations.

Changes

Undo recovery

Layer / File(s) Summary
Undo record contract
src/database/include/kth/database/block_store.hpp, src/database/src/block_store.cpp
Undo APIs now require block hashes. New scan result types and versioned record metadata describe recovered positions and scan failures.
Undo storage and scanning
src/database/src/block_store.cpp
Undo serialization stores the owner hash. Scanning validates markers, sizes, boundaries, ownership, checksums, legacy records, unattributed records, and duplicates.
Startup restoration and reorganization
src/blockchain/src/interface/block_chain.cpp, src/node/test/reorg_cycle.cpp
Startup reconstructs the persisted header chain, restores undo positions, and validates active-chain undo coverage. Block undo reads and writes include owner hashes. Restart tests cover successful reorganization and corrupted undo data.
Undo scan and restart tests
src/blockchain/test/undo_scan.cpp, src/blockchain/CMakeLists.txt
The test target includes scan tests for round trips, sibling attribution, legacy data, truncation, corruption, unreadable files, invalid markers, unattributed records, duplicates, and missing undo records.

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

Sequence Diagram(s)

sequenceDiagram
  participant block_chain as block_chain::start
  participant store as block_store
  participant index as header_index
  block_chain->>store: scan_undo_positions(parent lookup)
  store-->>block_chain: scan status and recovered locations
  block_chain->>index: restore undo positions and statuses
  block_chain->>store: read_block_undo(block hash, parent hash)
  store-->>block_chain: validated undo record
Loading

Possibly related PRs

  • k-nuth/kth#571: Extends the earlier undo storage and block-chain undo paths.
  • k-nuth/kth#573: Modifies block-chain reorganization and undo handling.
  • k-nuth/kth#579: Adds related restart persistence coverage in reorg_cycle.cpp.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes satisfy issue [#603] by restoring undo associations at startup and validating coverage for restart-driven reorganizations.
Out of Scope Changes check ✅ Passed The implementation, API changes, startup validation, build update, and regression tests directly support the linked issue [#603].
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: adding block identity to undo records so startup can recover them after restart.
✨ Finishing Touches
📝 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-after-restart-603

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

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

@fpelliccioni
fpelliccioni force-pushed the test/reorg-after-restart-603 branch from c7b7127 to 71a532a Compare August 5, 2026 20:18
@fpelliccioni
fpelliccioni marked this pull request as ready for review August 5, 2026 20:19

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/database/include/kth/database/block_store.hpp (1)

140-152: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Remove the stale read_undo doc block and the duplicated [[nodiscard]].

Lines 140-143 still document the old two-parameter signature and do not mention block_hash. Lines 144 and 150 both apply [[nodiscard]] to the same declaration.

♻️ Proposed cleanup
-    /// Read undo data from disk.
-    /// `@param` pos Position of the undo data.
-    /// `@param` prev_hash Hash of the previous block (for checksum verification).
-    /// `@return` Undo data or error.
-    [[nodiscard]]
     /// Read a block's undo record. `block_hash` is the block the caller believes
     /// owns the record, and the record must agree — a wrong position would
     /// otherwise return some other block's undo, and the checksum cannot catch
     /// the sibling case, since siblings share its seed. `prev_hash` seeds the
     /// checksum, as at write time.
     [[nodiscard]] std::expected<block_undo, result_code>
     read_undo(flat_file_pos const& pos, hash_digest const& block_hash,
               hash_digest const& prev_hash) const;
🤖 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/include/kth/database/block_store.hpp` around lines 140 - 152, In
the read_undo declaration, remove the stale documentation block describing the
old signature and delete the duplicated [[nodiscard]] attribute, retaining only
the current documentation and a single attribute before the declaration.
🧹 Nitpick comments (2)
src/blockchain/src/interface/block_chain.cpp (1)

307-318: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Check the record's file number against the block's file number before you restore it.

set_undo_pos stores only an offset. read_block_undo later pairs that offset with header_index_.get_file_number(idx). The scan result carries location.file_number, and this loop discards it. If the two disagree, or if the block has no block data (get_file_number(idx) < 0), the index marks the block have_undo while the stored offset addresses another file.

Compare the two values here, where the information is still available.

🛡️ Proposed check
         for (auto const& location : undo_scan.found) {
             auto const idx = header_index_.find(location.block_hash);
             if (idx == header_index::null_index) {
                 // The scan already refused an unknown block, so this cannot
                 // happen; if it somehow does, the index and the files disagree.
                 spdlog::critical("[blockchain] An undo record survived the scan for a block the "
                     "index does not hold");
                 return false;
             }
+            // The offset is stored alone; the file number comes from the block's
+            // blk file at read time. A record in another file could not be read
+            // back, so refuse rather than mark the block as having undo data.
+            if (header_index_.get_file_number(idx) != location.file_number) {
+                spdlog::critical("[blockchain] Undo record for a block in file {} was found in "
+                    "file {}", header_index_.get_file_number(idx), location.file_number);
+                return false;
+            }
             header_index_.set_undo_pos(idx, location.position);
             header_index_.add_status(idx, header_status::have_undo);
         }
🤖 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/src/interface/block_chain.cpp` around lines 307 - 318, In the
loop over undo_scan.found, validate location.file_number against
header_index_.get_file_number(idx) before calling set_undo_pos or add_status.
Reject and return false when the file numbers differ or the block file number is
negative; only restore the undo position and have_undo status after this
validation succeeds.
src/database/src/block_store.cpp (1)

914-920: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Reuse undo_checksum in the writer.

The writer still builds the checksum input inline at lines 944-950, while read_undo and scan_undo_positions call undo_checksum. Two definitions of one on-disk value can diverge, and a divergence appears only as invalid_checksum during a later startup.

♻️ Proposed change (lines 944-950)
-    // Calculate checksum: SHA256(prev_hash || undo_data)
-    data_chunk checksum_input;
-    checksum_input.reserve(prev_hash.size() + undo_data.size());
-    checksum_input.insert(checksum_input.end(), prev_hash.begin(), prev_hash.end());
-    checksum_input.insert(checksum_input.end(), undo_data.begin(), undo_data.end());
-
-    auto checksum = bitcoin_hash(checksum_input);
+    auto const checksum = undo_checksum(prev_hash, undo_data);
🤖 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/block_store.cpp` around lines 914 - 920, Update the
undo-writing logic in the function containing the shown header writes to call
the existing undo_checksum helper instead of constructing the checksum input
inline. Pass the same undo marker, block hash, and size values used for the
record, and write the helper’s result so it remains consistent with read_undo
and scan_undo_positions.
🤖 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/database/src/block_store.cpp`:
- Around line 531-549: Add a distinct undo_scan_status enumerator such as
invalid_marker in block_store.hpp, then update the fallback branch in the marker
validation logic of block_store.cpp to return invalid_marker for unrecognized
markers while preserving legacy_format for magic_ and truncated_record for
actual truncation cases.
- Around line 704-732: Reorder the validation in block_store::read_undo so the
file_magic/undo_magic_v2 check runs immediately after reading the marker and
before reading or comparing record_block_hash. Preserve the existing
legacy-format diagnostic and result handling, then perform the owner-hash
comparison only for the current format.

---

Outside diff comments:
In `@src/database/include/kth/database/block_store.hpp`:
- Around line 140-152: In the read_undo declaration, remove the stale
documentation block describing the old signature and delete the duplicated
[[nodiscard]] attribute, retaining only the current documentation and a single
attribute before the declaration.

---

Nitpick comments:
In `@src/blockchain/src/interface/block_chain.cpp`:
- Around line 307-318: In the loop over undo_scan.found, validate
location.file_number against header_index_.get_file_number(idx) before calling
set_undo_pos or add_status. Reject and return false when the file numbers differ
or the block file number is negative; only restore the undo position and
have_undo status after this validation succeeds.

In `@src/database/src/block_store.cpp`:
- Around line 914-920: Update the undo-writing logic in the function containing
the shown header writes to call the existing undo_checksum helper instead of
constructing the checksum input inline. Pass the same undo marker, block hash,
and size values used for the record, and write the helper’s result so it remains
consistent with read_undo and scan_undo_positions.
🪄 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: 534036a7-31fe-4b15-85c8-6d7f446a56cb

📥 Commits

Reviewing files that changed from the base of the PR and between a323506 and 71a532a.

📒 Files selected for processing (6)
  • src/blockchain/CMakeLists.txt
  • src/blockchain/src/interface/block_chain.cpp
  • src/blockchain/test/undo_scan.cpp
  • src/database/include/kth/database/block_store.hpp
  • src/database/src/block_store.cpp
  • src/node/test/reorg_cycle.cpp

Comment thread src/database/src/block_store.cpp
Comment thread src/database/src/block_store.cpp Outdated
@fpelliccioni
fpelliccioni force-pushed the test/reorg-after-restart-603 branch 4 times, most recently from ca46402 to cfbc362 Compare August 5, 2026 22:33
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (5)
src/database/src/block_store.cpp (2)

1019-1025: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use the undo_checksum helper here.

Lines 1019-1025 rebuild the checksum inline. Lines 78-84 define undo_checksum for exactly this computation, and both readers now call it: line 662 in scan_undo_positions and line 825 in read_undo. The writer is the only remaining copy.

The writer and the readers must agree byte for byte. If the helper changes and this copy does not, every stored record fails its checksum and the scan rejects the database. Call the helper so one definition serves all three sites.

♻️ Proposed change
-    // Calculate checksum: SHA256(prev_hash || undo_data)
-    data_chunk checksum_input;
-    checksum_input.reserve(prev_hash.size() + undo_data.size());
-    checksum_input.insert(checksum_input.end(), prev_hash.begin(), prev_hash.end());
-    checksum_input.insert(checksum_input.end(), undo_data.begin(), undo_data.end());
-
-    auto checksum = bitcoin_hash(checksum_input);
+    auto const checksum = undo_checksum(prev_hash, undo_data);
🤖 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/block_store.cpp` around lines 1019 - 1025, Replace the
inline checksum construction in the writer with a call to the existing
undo_checksum helper, passing prev_hash and undo_data in the same order used by
scan_undo_positions and read_undo. Preserve the resulting checksum assignment
while ensuring all writer and reader paths share the helper’s byte-for-byte
computation.

825-828: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Report a checksum mismatch as db_corrupt.

read_undo treats an invalid owner hash and an implausible payload size as result_code::db_corrupt. A stored record whose undo_checksum(prev_hash, undo_data) does not match the checksum stored in the file is the same class of data corruption and should return result_code::db_corrupt instead of result_code::other.

♻️ Proposed change
     if (undo_checksum(prev_hash, undo_data) != stored_checksum) {
         spdlog::error("block_store::read_undo: Checksum mismatch at {}", pos.to_string());
-        return std::unexpected(result_code::other);
+        return std::unexpected(result_code::db_corrupt);
     }

</ details>

🤖 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/block_store.cpp` around lines 825 - 828, Update the
checksum-mismatch branch in read_undo to return result_code::db_corrupt instead
of result_code::other, while preserving the existing error log and early-return
behavior.
src/blockchain/test/undo_scan.cpp (2)

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

Bind the "legacy" magic to the fixture magic with one named constant.

The four bytes {0xe3, 0xe1, 0xf3, 0xe8} are repeated in three places. Line 62 sets the store magic. Lines 203 and 515 write it as the legacy record marker. The two tests depend on those values being identical: scan_undo_positions reports legacy_format only when the marker equals magic_, and reports invalid_marker otherwise. If someone changes the fixture magic at line 62 alone, both tests stop exercising the legacy path and pass for a different reason.

Declare one constant in the anonymous namespace and use it in all three places.

♻️ Proposed refactor
 namespace {
 
+// The store magic. The legacy record marker is the same four bytes, which is
+// what makes a legacy record distinguishable from an unknown marker.
+constexpr block_store::magic_t regtest_magic{{0xe3, 0xe1, 0xf3, 0xe8}};
+
 hash_digest make_hash(uint8_t seed) {

Then use regtest_magic at line 62, and write regtest_magic.data() at lines 204 and 516 in place of the local network_magic arrays.

Also applies to: 203-204, 515-516

🤖 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/undo_scan.cpp` at line 62, Declare a single named
regtest_magic constant in the anonymous namespace for the shared four-byte
value, then use it for the block_store fixture initialization and replace the
local network_magic arrays at both legacy-marker write sites with
regtest_magic.data().

378-379: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Move the undo layout constants into a public header.

undo_header_size and undo_checksum_size exist, but only inside src/database/src/block_store.cpp, so this test cannot reuse them and the literals 40 and 32 can diverge if the record layout changes.

🤖 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/undo_scan.cpp` around lines 378 - 379, Expose the
existing undo layout constants undo_header_size and undo_checksum_size in a
public undo-related header, then update the calculation near first_record_end in
the undo scan test to use those symbols instead of the literal 32 (and any
corresponding layout literal). Remove or reconcile the private definitions in
block_store.cpp so there is one authoritative definition.
src/blockchain/src/interface/block_chain.cpp (1)

334-345: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Also verify the scanned file number when undo positions are restored.

undo_scan_result::found carries file_number, but this loop stores only location.position. read_block_undo later rebuilds the position from header_index_.get_file_number(idx), which is the block's file number. The two agree only because store_block_undo writes undo into the file that matches the block file. If they ever disagree on disk, the mismatch surfaces later as a read failure inside a reorganization instead of at startup, where every other undo inconsistency is refused.

Compare the two here and refuse the start, so the invariant is checked in the same place as the rest of the undo validation.

♻️ Proposed check
         header_index_.set_undo_pos(idx, location.position);
+        if (location.file_number != header_index_.get_file_number(idx)) {
+            spdlog::critical("[blockchain] An undo record for block at index {} was found in file "
+                "{}, but the block is in file {}; the undo position cannot be addressed",
+                idx, location.file_number, header_index_.get_file_number(idx));
+            return false;
+        }
         header_index_.add_status(idx, header_status::have_undo);
🤖 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/src/interface/block_chain.cpp` around lines 334 - 345, In the
loop restoring entries from undo_scan_result::found, compare
location.file_number with header_index_.get_file_number(idx) before calling
set_undo_pos. If they differ, log the inconsistency and return false; only
restore the undo position and have_undo status when the file numbers match.
🤖 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/database/src/block_store.cpp`:
- Around line 614-617: Update the undo-size validation around the block undo
scan to allow the valid empty undo representation encoded as a one-byte zero
count, while still rejecting actual zero-byte payloads if they can occur. Remove
the unconditional undo_size == 0 rejection in the validation used with
block_undo::to_data(), preserve the max_undo_size check, and ensure
serialized_size accounting continues to include the encoded count byte.
- Line 532: Update the file-opening calls in the undo scan and nearby
scan_block_positions path-opening logic to pass a narrow string conversion such
as path.string().c_str() to std::fopen, or centralize that conversion in
flat_file_seq::open; ensure all affected paths compile and open correctly on
Windows.

---

Nitpick comments:
In `@src/blockchain/src/interface/block_chain.cpp`:
- Around line 334-345: In the loop restoring entries from
undo_scan_result::found, compare location.file_number with
header_index_.get_file_number(idx) before calling set_undo_pos. If they differ,
log the inconsistency and return false; only restore the undo position and
have_undo status when the file numbers match.

In `@src/blockchain/test/undo_scan.cpp`:
- Line 62: Declare a single named regtest_magic constant in the anonymous
namespace for the shared four-byte value, then use it for the block_store
fixture initialization and replace the local network_magic arrays at both
legacy-marker write sites with regtest_magic.data().
- Around line 378-379: Expose the existing undo layout constants
undo_header_size and undo_checksum_size in a public undo-related header, then
update the calculation near first_record_end in the undo scan test to use those
symbols instead of the literal 32 (and any corresponding layout literal). Remove
or reconcile the private definitions in block_store.cpp so there is one
authoritative definition.

In `@src/database/src/block_store.cpp`:
- Around line 1019-1025: Replace the inline checksum construction in the writer
with a call to the existing undo_checksum helper, passing prev_hash and
undo_data in the same order used by scan_undo_positions and read_undo. Preserve
the resulting checksum assignment while ensuring all writer and reader paths
share the helper’s byte-for-byte computation.
- Around line 825-828: Update the checksum-mismatch branch in read_undo to
return result_code::db_corrupt instead of result_code::other, while preserving
the existing error log and early-return behavior.
🪄 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: 69323bbc-6f38-465a-9fc4-98abe1f2c6f6

📥 Commits

Reviewing files that changed from the base of the PR and between a323506 and cfbc362.

📒 Files selected for processing (6)
  • src/blockchain/CMakeLists.txt
  • src/blockchain/src/interface/block_chain.cpp
  • src/blockchain/test/undo_scan.cpp
  • src/database/include/kth/database/block_store.hpp
  • src/database/src/block_store.cpp
  • src/node/test/reorg_cycle.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/blockchain/CMakeLists.txt
  • src/database/include/kth/database/block_store.hpp

Comment thread src/database/src/block_store.cpp Outdated
Comment thread src/database/src/block_store.cpp
…t again

Undo positions live only in the header index, which is rebuilt from disk at
startup — and that rebuild restored block positions and nothing else. So after
any ordinary restart no block had undo data, the bytes sat unreachable in the rev
files, and a reorganization that had to roll back a block connected before the
restart could not run. Not a crash window: the state after every clean shutdown.

Nothing already on disk could attribute a record. Order cannot: a rev file holds
undo for a subset of the blocks in the matching blk file, in build order rather
than storage order, and side-branch, never-connected and below-checkpoint blocks
contribute nothing while leaving no gap to notice. Two histories produce the same
observable layout. Nor can the checksum, which is SHA256(prev_hash || undo_data)
with prev_hash a seed rather than content: every sibling of the owning block
validates the same record. And two siblings built from the same transactions with
different coinbases spend the same outputs, so their undo data is byte-identical
— no structural comparison separates them either.

So identity goes in the record, behind a marker of its own. Records written
before this begin with the network magic, the same four bytes blocks use, so
sharing it would leave two formats that cannot be told apart; finding the network
magic where the undo marker belongs now names an older database rather than
corruption, which is what lets a caller ask for a rebuild instead of guessing. A
durable index keyed by block hash was the alternative and is worse: it
reintroduces the window issue #600 is about, between writing the bytes and
writing the reference, across two stores that then need ordering and
synchronization. Inside the record there is no window.

scan_undo_positions mirrors scan_block_positions and runs beside it. Every record
is validated before it counts — marker, bounded size, resolvable owner, payload,
and checksum against that owner's parent — and nothing is applied until every
file has been read, because a half-restored index is worse than an empty one: it
looks complete, so a block with no undo cannot be told from one whose record was
never reached. read_undo now takes the block it is being read for and requires
the record to agree, which the checksum cannot do for siblings, since it
satisfies both.

The result names what stopped it rather than reporting a count, and two of those
cases were found by tests rather than reasoning. A record for a block the index
does not hold is not a disagreement: a restart rebuilds the index from the active
chain, so a branch that lost a reorganization is forgotten while its records
remain — refusing would stop any node that ever reorganized from starting, which
is how it surfaced. And a marker of zeroes is reserved space that was never
written, since rev files are preallocated and their size comes from the file
system rather than from how far writing got. Both are ordinary; only the first is
counted.

Databases written before this keep records with no identity, and those were
proven ambiguous rather than merely inconvenient. Startup reports the format and
refuses, asking for a rebuild. A node that knows no pre-restart reorganization
can work should say so rather than announce itself operational.

Review found four more places where an edge read as an ending, and they share a
shape: something that could not be done was reported as something that was not
there.

A rev file that exists and will not open was skipped like one that was never
written, which would start the node without undo it has. A header with one to
thirty-nine non-zero bytes left in the file was a clean end rather than the
interrupted write it is. Four zero bytes ended the scan wherever they appeared,
so damage in the middle of a file could hide every record beyond it while
reporting a clean read — zeroes are the start of unused reserved space only if
everything after them is unused too. And reading a record that belongs to another
block returned "not found", dressing a damaged database as an ordinary missing
one.

The fourth is the one that mattered most. Skipping a record whose block the index
does not hold is right for an abandoned branch, and locally indistinguishable
from an active record whose stored hash lost a bit — which would leave a
connected block quietly without undo. What separates them is not local but
global: every block on the active chain between the checkpoint and the built
height was connected with undo and must still have it. Startup checks that
coverage and refuses when it is short, so a forgotten branch is tolerated while a
lost active record is not.

Five more tests, each on the edge it belongs to: a file that exists and will not
open; a partial header, which the earlier truncation case does not reach because
cutting a payload leaves a whole header; zeroes followed by content; and records
recovered from two rev files, which also pins that a file number is only known
once its blk file is.

A second review round closed four more places where startup would carry on rather
than refuse, and one of them was the coverage check disabling itself: a built
height that could not be read was taken for nothing having been built, so the
check skipped — the failure it exists to catch, arriving through the check. A
height in the required range with no block is fatal for the same reason: absence
of the block is absence of the proof, not absence of the obligation.

The coverage check reads the chain from the by-height table recorded while
headers load, not from the active view. It cannot use the active view: the
organizer publishes that after startup, so asking it during start returns nothing
— which this learned by asking and being told height one does not exist. The
persisted table is the authoritative source available at that moment, and it is
the one the UTXO set was built against, so it is the right one regardless.

Two tests were passing without testing what they claimed. The partial-header case
computed the first record's end from the value inside the undo rather than its
serialized size, so it cut inside that record and took the payload-does-not-fit
path the truncation case already covers; it now cuts where a header would begin
and writes six bytes of one. And the unreadable-file case dropped permissions,
which Windows ignores and root ignores everywhere; it puts a directory where the
file belongs instead, so the file exists and will not open on any platform.

The regression that justifies skipping an unattributable record is here too:
altering one byte of an active record's stored block hash leaves its checksum
intact — it covers the payload and is seeded with the parent — so nothing local
notices, the scan passes the record over, and only the coverage check refuses the
start.

Two more distinctions, both from review. A marker that is neither this format's
nor the old one's now says so instead of being reported as a truncated record:
they are different diagnoses, and calling the first the second sends whoever
reads the log looking for an interrupted write. And read_undo checks the marker
before reading anything that follows it — in the old layout those next
thirty-two bytes are a size and the start of a payload rather than a hash, so
comparing them first would report a record belonging to another block, which is
the wrong answer under an error code meaning corruption.

Two things the scanner got right and the read path did not. read_undo took the
payload size from the record and handed it to an allocation, where the scanner
bounds it — a size damaged after the scan would ask for gigabytes, and adding the
checksum length to it would overflow first. And uniqueness was checked after the
owning block was resolved, so two records claiming one forgotten block passed as
two ordinary unattributed ones: whether a hash appears twice does not depend on
knowing the block, and the check belongs where that is true.

The scan opens rev files by path, and the way everything here already opened one
does not survive the platform: a path holds wchar_t on Windows, std::fopen only
takes char const*, and narrowing it converts through the active code page — so a
data directory under a name that page cannot spell would stop opening at all.
open_native hands each platform the call that takes its own path verbatim, and
the three older sites that had the same defect now go through it too.
@fpelliccioni
fpelliccioni force-pushed the test/reorg-after-restart-603 branch from cfbc362 to bb0784e Compare August 5, 2026 23:04
@fpelliccioni
fpelliccioni merged commit 24fc255 into master Aug 5, 2026
25 of 26 checks passed
@fpelliccioni
fpelliccioni deleted the test/reorg-after-restart-603 branch August 5, 2026 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

undo data is unreachable after any restart, so a reorganization cannot disconnect

1 participant