node: make a half-applied UTXO batch impossible to continue past - #602
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds checksummed transition records, typed durability barriers, startup refusal for unfinished or invalid transitions, and atomic UTXO batch and reorganization publication. It also tracks undo files and adds lifecycle, durability, serialization, and crash-boundary tests. ChangesUTXO transition durability
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant utxo_build_task
participant block_chain
participant internal_database
participant block_store
participant utxoz_database
utxo_build_task->>block_chain: Begin in-progress transition
block_chain->>internal_database: Persist and sync transition record
utxo_build_task->>block_store: Write and flush undo files
utxo_build_task->>utxoz_database: Synchronize UTXO state
utxo_build_task->>block_chain: Publish heights and clear transition
block_chain->>internal_database: Atomically publish and clear record
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/node/src/sync/block_tasks.cpp`:
- Around line 2216-2225: Move the clear_utxo_batch_dirty() success check from
the completion block to immediately after set_utxo_built_height() succeeds,
before mempool_remove_for_block() reconciliation. Preserve the existing critical
logging and on_fatal handling for marker-clear failure, and keep mempool failure
handling after the marker has been cleared.
In `@src/node/test/utxo_batch_atomicity.cpp`:
- Around line 115-117: Strengthen the successful UTXO batch test around the
existing get_utxo_batch_dirty checks by adding a controlled synchronization or
failure point after set_utxo_batch_dirty persists the marker and before batch
completion. At that point, assert that the marker exists and contains the
expected height, then retain the final CHECK_FALSE(*dirty) assertion to verify
successful completion clears it.
- Around line 60-71: Extend the test after run_build_and_collect_fatals to read
the persisted marker via chain.get_utxo_batch_dirty() and assert it still
contains value 1. Keep the existing fatal-count, message, and built-height
assertions unchanged.
🪄 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: c5169b64-4c1f-4c11-a61a-46d620b32860
⛔ Files ignored due to path filters (2)
docs/utxo-store-api-requirements.mdis excluded by none and included by nonesrc/database/include/kth/database/databases/internal_database.ippis excluded by!**/*.ippand included bysrc/database/**
📒 Files selected for processing (8)
src/blockchain/include/kth/blockchain/interface/block_chain.hppsrc/blockchain/src/interface/block_chain.cppsrc/database/include/kth/database/databases/internal_database.hppsrc/database/include/kth/database/databases/property_code.hppsrc/node/CMakeLists.txtsrc/node/src/sync/block_tasks.cppsrc/node/test/sync_harness.hppsrc/node/test/utxo_batch_atomicity.cpp
fpelliccioni
added a commit
that referenced
this pull request
Aug 6, 2026
…abel as one thing `chain_state()` was computed once, in `start()`, and never advanced. Its only mutator, `set_chain_state`, had no callers. So a running node validated every transaction against the height it had when the process began — along with the median time past and the fork-activation flags in force then. Immediately after starting on a synced directory that is about right; a thousand blocks later it is a thousand blocks wrong, and nothing says so. Admission was one of four readers, and the only one anyone had proposed to fix. The other three are mining, and two of them already combined the frozen value with a fresh read: - `fetch_mining_info` reported the current height beside the difficulty in force at startup; - `fetch_mining_template` derived the template's parent from the frozen height — `get_block_hash(height - 1)` — so on a node that had connected anything since starting, that was not the tip. The template cache guards against exactly this, refusing to serve a snapshot from a previous tip, and it decides by comparing that hash: a guard whose input never changes is a guard that keeps passing. The state, the tip and a number labelling the pair now travel together, in one immutable object behind one pointer, swapped once. A reader holds the previous triple or the next one; there is no third possibility, and the two mixing readers have nothing left to mix — `get_block_hash` and `fetch_last_height` are gone from those paths, not by discipline but because there is nothing to call. `publish_chain_view` takes one thing: the height of the last block whose state is coherently applied. State and hash are derived from it here and the generation advances internally, so a caller cannot assemble a combination that never existed. It fails rather than keeping the previous view: at the close of a batch or a reorganization, being unable to describe the state just reached is not something to carry on from — the old view is still coherent and says nothing about being stale, so the return value is the only signal there is. The height is given and not read, because the two tips differ. `populate()` built from `heights.header`, and during a sync that runs thousands of blocks ahead of what is connected: a state carrying a height, a median time past and activation flags for a block whose UTXO delta has not been applied. Internally consistent, describing nothing. The overload that could do that is gone. Published at three points, and nowhere else. At startup from `heights.block`. At the close of a batch, once the delta is applied *in full* — including the deletions the store deferred, which are part of that delta and not work that follows it — the undo records are written, the height marker has moved and the mempool no longer holds what those blocks confirmed. Those deletions used to run after the publication and to be logged when they failed; a spent output left in the set is a double spend the node would accept, so they now run first and a failure is fatal. What makes that window recoverable rather than merely fatal is the marker of #600, which #602 adds; this orders the work and refuses to continue past a failure, and does not make the window crash-safe. And at the close of a reorganization, while the writers are still parked — with the validated tip, not the branch head: the switch rewinds the connected chain to the fork, and the blocks above it are headers until the ordinary path applies their deltas. So a reorganization moves the height down while the generation moves up, which is what counting published states rather than heights is for. The generation is taken last, after everything that can fail. Reading it while building the argument to the allocation would let a throw leave a number no published view carries — a reader comparing generations would see one it can never be given and conclude something was published that was not. The allocation is also caught and returned as a code, because callers publish at the close of a batch or a reorganization and decide what to do about a failure; an exception out of an API that promises a code would bypass that decision. Nine tests. The initial publication; headers arriving without connecting anything, which move neither the view nor the generation; a batch, and a second; the triple read whole while batches are published, checking that the hash a view carries is the hash of the block below the height it carries; mining info from one publication; a template naming the new tip after a batch rather than the cached one; a failed publication leaving both the view and the generation untouched, with several failures followed by a success that lands exactly one above where it started — so the counter is shown to have no hole in it; and the reorganization, where the height falls, the hash changes and the generation rises. The admission tests from #607 no longer restart the chain. That restart was explicitly a way around this bug, and it is not something a running node could have done. What this does not fix: during a transition, template construction can still run against stores that are mid-mutation. This makes the published state coherent and whole; keeping readers out of the window while it is being reached is separate, and depends on settling ownership and readiness with #590. Closes #605
fpelliccioni
added a commit
that referenced
this pull request
Aug 7, 2026
…abel as one thing `chain_state()` was computed once, in `start()`, and never advanced. Its only mutator, `set_chain_state`, had no callers. So a running node validated every transaction against the height it had when the process began — along with the median time past and the fork-activation flags in force then. Immediately after starting on a synced directory that is about right; a thousand blocks later it is a thousand blocks wrong, and nothing says so. Admission was one of four readers, and the only one anyone had proposed to fix. The other three are mining, and two of them already combined the frozen value with a fresh read: - `fetch_mining_info` reported the current height beside the difficulty in force at startup; - `fetch_mining_template` derived the template's parent from the frozen height — `get_block_hash(height - 1)` — so on a node that had connected anything since starting, that was not the tip. The template cache guards against exactly this, refusing to serve a snapshot from a previous tip, and it decides by comparing that hash: a guard whose input never changes is a guard that keeps passing. The state, the tip and a number labelling the pair now travel together, in one immutable object behind one pointer, swapped once. A reader holds the previous triple or the next one; there is no third possibility, and the two mixing readers have nothing left to mix — `get_block_hash` and `fetch_last_height` are gone from those paths, not by discipline but because there is nothing to call. `publish_chain_view` takes one thing: the height of the last block whose state is coherently applied. State and hash are derived from it here and the generation advances internally, so a caller cannot assemble a combination that never existed. It fails rather than keeping the previous view: at the close of a batch or a reorganization, being unable to describe the state just reached is not something to carry on from — the old view is still coherent and says nothing about being stale, so the return value is the only signal there is. The height is given and not read, because the two tips differ. `populate()` built from `heights.header`, and during a sync that runs thousands of blocks ahead of what is connected: a state carrying a height, a median time past and activation flags for a block whose UTXO delta has not been applied. Internally consistent, describing nothing. The overload that could do that is gone. Published at three points, and nowhere else. At startup from `heights.block`. At the close of a batch, once the delta is applied *in full* — including the deletions the store deferred, which are part of that delta and not work that follows it — the undo records are written, the height marker has moved and the mempool no longer holds what those blocks confirmed. Those deletions used to run after the publication and to be logged when they failed; a spent output left in the set is a double spend the node would accept, so they now run first and a failure is fatal. What makes that window recoverable rather than merely fatal is the marker of #600, which #602 adds; this orders the work and refuses to continue past a failure, and does not make the window crash-safe. And at the close of a reorganization, while the writers are still parked — with the validated tip, not the branch head: the switch rewinds the connected chain to the fork, and the blocks above it are headers until the ordinary path applies their deltas. So a reorganization moves the height down while the generation moves up, which is what counting published states rather than heights is for. The generation is taken last, after everything that can fail. Reading it while building the argument to the allocation would let a throw leave a number no published view carries — a reader comparing generations would see one it can never be given and conclude something was published that was not. The allocation is also caught and returned as a code, because callers publish at the close of a batch or a reorganization and decide what to do about a failure; an exception out of an API that promises a code would bypass that decision. Nine tests. The initial publication; headers arriving without connecting anything, which move neither the view nor the generation; a batch, and a second; the triple read whole while batches are published, checking that the hash a view carries is the hash of the block below the height it carries; mining info from one publication; a template naming the new tip after a batch rather than the cached one; a failed publication leaving both the view and the generation untouched, with several failures followed by a success that lands exactly one above where it started — so the counter is shown to have no hole in it; and the reorganization, where the height falls, the hash changes and the generation rises. The branch path — the one seeded from the published state, for block validation — answers null before anything is published rather than reading through nothing. That window is a chain constructed and not started, since start() is what publishes the first, and it is reachable through the public API; a test builds exactly that and asks. The admission tests from #607 no longer restart the chain. That restart was explicitly a way around this bug, and it is not something a running node could have done. What this does not fix: during a transition, template construction can still run against stores that are mid-mutation. This makes the published state coherent and whole; keeping readers out of the window while it is being reached is separate, and depends on settling ownership and readiness with #590. Closes #605
fpelliccioni
added a commit
that referenced
this pull request
Aug 7, 2026
…abel as one thing (#615) `chain_state()` was computed once, in `start()`, and never advanced. Its only mutator, `set_chain_state`, had no callers. So a running node validated every transaction against the height it had when the process began — along with the median time past and the fork-activation flags in force then. Immediately after starting on a synced directory that is about right; a thousand blocks later it is a thousand blocks wrong, and nothing says so. Admission was one of four readers, and the only one anyone had proposed to fix. The other three are mining, and two of them already combined the frozen value with a fresh read: - `fetch_mining_info` reported the current height beside the difficulty in force at startup; - `fetch_mining_template` derived the template's parent from the frozen height — `get_block_hash(height - 1)` — so on a node that had connected anything since starting, that was not the tip. The template cache guards against exactly this, refusing to serve a snapshot from a previous tip, and it decides by comparing that hash: a guard whose input never changes is a guard that keeps passing. The state, the tip and a number labelling the pair now travel together, in one immutable object behind one pointer, swapped once. A reader holds the previous triple or the next one; there is no third possibility, and the two mixing readers have nothing left to mix — `get_block_hash` and `fetch_last_height` are gone from those paths, not by discipline but because there is nothing to call. `publish_chain_view` takes one thing: the height of the last block whose state is coherently applied. State and hash are derived from it here and the generation advances internally, so a caller cannot assemble a combination that never existed. It fails rather than keeping the previous view: at the close of a batch or a reorganization, being unable to describe the state just reached is not something to carry on from — the old view is still coherent and says nothing about being stale, so the return value is the only signal there is. The height is given and not read, because the two tips differ. `populate()` built from `heights.header`, and during a sync that runs thousands of blocks ahead of what is connected: a state carrying a height, a median time past and activation flags for a block whose UTXO delta has not been applied. Internally consistent, describing nothing. The overload that could do that is gone. Published at three points, and nowhere else. At startup from `heights.block`. At the close of a batch, once the delta is applied *in full* — including the deletions the store deferred, which are part of that delta and not work that follows it — the undo records are written, the height marker has moved and the mempool no longer holds what those blocks confirmed. Those deletions used to run after the publication and to be logged when they failed; a spent output left in the set is a double spend the node would accept, so they now run first and a failure is fatal. What makes that window recoverable rather than merely fatal is the marker of #600, which #602 adds; this orders the work and refuses to continue past a failure, and does not make the window crash-safe. And at the close of a reorganization, while the writers are still parked — with the validated tip, not the branch head: the switch rewinds the connected chain to the fork, and the blocks above it are headers until the ordinary path applies their deltas. So a reorganization moves the height down while the generation moves up, which is what counting published states rather than heights is for. The generation is taken last, after everything that can fail. Reading it while building the argument to the allocation would let a throw leave a number no published view carries — a reader comparing generations would see one it can never be given and conclude something was published that was not. The allocation is also caught and returned as a code, because callers publish at the close of a batch or a reorganization and decide what to do about a failure; an exception out of an API that promises a code would bypass that decision. Nine tests. The initial publication; headers arriving without connecting anything, which move neither the view nor the generation; a batch, and a second; the triple read whole while batches are published, checking that the hash a view carries is the hash of the block below the height it carries; mining info from one publication; a template naming the new tip after a batch rather than the cached one; a failed publication leaving both the view and the generation untouched, with several failures followed by a success that lands exactly one above where it started — so the counter is shown to have no hole in it; and the reorganization, where the height falls, the hash changes and the generation rises. The branch path — the one seeded from the published state, for block validation — answers null before anything is published rather than reading through nothing. That window is a chain constructed and not started, since start() is what publishes the first, and it is reachable through the public API; a test builds exactly that and asks. The admission tests from #607 no longer restart the chain. That restart was explicitly a way around this bug, and it is not something a running node could have done. What this does not fix: during a transition, template construction can still run against stores that are mid-mutation. This makes the published state coherent and whole; keeping readers out of the window while it is being reached is separate, and depends on settling ownership and readiness with #590. Closes #605
fpelliccioni
added a commit
that referenced
this pull request
Aug 10, 2026
The visible half of this bump is a rename: UTXO-Z's second storage mode is called reference rather than compact, so compact_db becomes reference_db, compact_find_result becomes reference_find_result, and the option that selects it follows. That half is safe because getting it wrong does not compile. The dangerous half is what four functions now return. process_pending_lookups, process_pending_deletions and erase went from plain values to result<>, and the compiler found all three. compact_all did not: it used to return void, so a caller that kept invoking it as a statement compiled exactly as before while discarding every failure. That one is the reason this is not a version-number change. erase is the one worth reading twice. It returned a count, and the natural migration writes `*erased > 0 ? success : key_not_found` — which turns "the erase failed" into "the key was not there", about an entry that may still exist and still be spendable. Both drains have the same shape: an unreadable catalogue is not an empty queue, and a caller reads absence as spent. Also integrated, because they are new rather than changed: sync(), which close() does not do for you, and platform_durability(), which says what a successful sync is worth — under contents_only the file contents reach the disk and the directory entries naming them do not. And the exclusive claim, where open() now distinguishes another instance holding the database from a claim that could not be attempted at all; the library's own header notes those send an operator looking in different places, so they are logged as different things. The error enum grew from five values to twenty-one, so failures are logged by name — the numbers moved, and an old note now names a different fault. Reporting the error was not enough: the first version of this logged it and returned an empty pair anyway, which is the same defect wearing a log line. The result now travels through utxoz_database, block_chain and every consumer. batch_validate stops as a local failure instead of answering missing_previous_output, which would reject a possibly-valid block on the strength of a read that never happened; the UTXO builders refuse to advance rather than reporting success. find() and find_raw() map only utxoz::error_code::not_found to key_not_found — closed, catalog_unreadable and recovery_required are read upstream as "queued, sweep it", and a sweep over a database that cannot be read answers "absent", which the validator reads as "spent". process_compact_block_utxos gets an error channel for the same reason. A negative file_number was guarded by KTH_ASSERT, which evaporates in Release and leaves -1 to become UINT32_MAX in every reference the block writes. That is not an impossible precondition — the header index and the block store are separate files and a crash can leave them disagreeing — so it is reported, and both callers stop the batch rather than storing entries that can never be resolved. Eight negative controls assert the shape of these contracts rather than the behaviour of a database, because a runtime test only fails after the wrong call has been written and run, and they reach Knuth's own layers rather than only UTXO-Z's types. Each is proven to fire: reverting compact() to its 0.8.0 form fails the build with "utxoz_database::compact() must report whether compaction happened"; returning void from block_chain::utxo_compact() fails with "the failure dies between the wrapper and every caller"; and making the database library report the opposite storage mode fails the coherence test with "database library reports reference, this target was compiled as full". That last one replaces a check that asked the same macro twice and could only ever agree with itself — the option is declared in two CMake lists, so the answer worth comparing is the one compiled into the database library against the one compiled here. The compaction OPERATION keeps its name throughout, and so do the things that merely contain the word: BIP152 compact blocks, history_compact, stealth_compact, chain::compact, CompactSize, and secp256k1's compact signatures. Each rename rule names an exact identifier rather than the substring, so none of those could be caught by accident. Full mode remains the default, in the recipe and in both CMake lists. Reference mode is compiled and tested here as a check: it is the half of the #ifdef that nothing in CI builds, so a rename that broke it would go unnoticed until someone turned the flag on. One review comment is deliberately not addressed here. open() takes the path as std::string_view, and path.string() converts through the active code page on Windows, so a data directory with non-ASCII characters cannot be opened. UTXO-Z 0.9.0 exposes no std::filesystem::path or wide overload, and substituting u8string() would be guessing at how the library reconstructs the path on the other side. Reported as utxo-z/utxo-z#109 instead of patched blind. Three further review points are fixed here. A reference the sweep FOUND but could not materialise, and stored bytes that will not decode, are both reads that failed against entries that demonstrably exist; neither is now demoted into the not-found list or dropped silently. And the negative file_number check is gated on reference mode, which is the only mode that stores that field — full mode keeps the whole output and never reads it. 0.9.0 could not have landed. It documented the second list returned by process_pending_lookups() as UNRESOLVED, not absent: a version file that could not be read left its keys there, indistinguishable from keys that exist nowhere, and Knuth reads that list as "spent". Nothing on this side could separate the two. 0.9.1 closes it upstream — the sweep now fails with version_unreadable rather than dropping unreachable keys into the answer, so the second list is absence that was proven, and the failure arrives as an error through the result<> chain this PR already wired. Which makes a comment in this file wrong in a useful way: it claimed the exhaustive switch over error_code would stop compiling when UTXO-Z added a code. 0.9.1 added version_unreadable and this built clean — the -Wswitch that would have warned is neither enabled nor promoted here. The case was added by hand, and the comment now says that adding one is a manual step on every bump. populate_prevout() carried the same defect one layer out: get_utxo() already returned expected<output_info, result_code> and every failure was read as absence, which then probed the mempool. A miss there is indistinguishable from a prevout that does not exist, so a disk that did not answer became a verdict about the transaction. Only key_not_found now enables that fallback; anything else returns error::operation_failed, and the chain from populate_prevout through populate_inputs_sync to populate() carries it to whoever decides. That chain needed the error channel: populate_prevout returned void and populate_inputs_sync returned error::success unconditionally, so changing the `if` alone would have dropped the answer one frame later. The three behavioural controls close the gap the type assertions left: a signature can be right while the body is wrong. Each has an exact negation and a positive that stops it passing by always failing. find() on a closed database must not answer key_not_found, and on an open empty one it must — otherwise an implementation returning `other` for everything would pass the negation and destroy the mempool fallback. An entry the store holds must never come back as absent when materialising or decoding it fails, and the raw payload is asserted present so the case is about the read and not an empty database. A negative file number fails in reference mode and builds in full, with a valid number building in both. A later review round found the same "log it and carry on" shape in four more places. apply_delta() and apply_delta_raw() discarded db_->erase() entirely, so a storage failure ended in result_code::success and a batch was recorded as applied while the outputs it spent were still in the set; not_found stays tolerated, since a delta can legitimately delete a pruned output. apply_delta_raw read the 8-byte reference with two memcpys off a caller-supplied buffer without checking its length, so a short payload read past its end and a long one was truncated into a reference pointing elsewhere. The sync harness swept the deferred queue on any get_utxo error rather than only key_not_found. And utxo_builder trusted KTH_ASSERT for a null header index, which is compiled out in Release, where it would have gone on to read a file number from it — block_tasks already refused at the same fork. The ordering defect is the one that did not need a failure to bite. block_tasks persisted utxo_built_height = batch_end and drained the deferred deletions afterwards, though the comment beside it claimed the opposite order. A crash between the two left a marker saying the batch was complete over a set that still held every output those blocks spent, and the restart trusted the marker. The drain now runs first, which closes that window in one direction: a crash after the drain and before the marker replays the batch, which is safe. The other direction is what #600 specifies and #602 implements — a durable record of which of the two happened, so recovery does not infer it.
fpelliccioni
added a commit
that referenced
this pull request
Aug 10, 2026
The visible half of this bump is a rename: UTXO-Z's second storage mode is called reference rather than compact, so compact_db becomes reference_db, compact_find_result becomes reference_find_result, and the option that selects it follows. That half is safe because getting it wrong does not compile. The dangerous half is what four functions now return. process_pending_lookups, process_pending_deletions and erase went from plain values to result<>, and the compiler found all three. compact_all did not: it used to return void, so a caller that kept invoking it as a statement compiled exactly as before while discarding every failure. That one is the reason this is not a version-number change. erase is the one worth reading twice. It returned a count, and the natural migration writes `*erased > 0 ? success : key_not_found` — which turns "the erase failed" into "the key was not there", about an entry that may still exist and still be spendable. Both drains have the same shape: an unreadable catalogue is not an empty queue, and a caller reads absence as spent. Also integrated, because they are new rather than changed: sync(), which close() does not do for you, and platform_durability(), which says what a successful sync is worth — under contents_only the file contents reach the disk and the directory entries naming them do not. And the exclusive claim, where open() now distinguishes another instance holding the database from a claim that could not be attempted at all; the library's own header notes those send an operator looking in different places, so they are logged as different things. The error enum grew from five values to twenty-one, so failures are logged by name — the numbers moved, and an old note now names a different fault. Reporting the error was not enough: the first version of this logged it and returned an empty pair anyway, which is the same defect wearing a log line. The result now travels through utxoz_database, block_chain and every consumer. batch_validate stops as a local failure instead of answering missing_previous_output, which would reject a possibly-valid block on the strength of a read that never happened; the UTXO builders refuse to advance rather than reporting success. find() and find_raw() map only utxoz::error_code::not_found to key_not_found — closed, catalog_unreadable and recovery_required are read upstream as "queued, sweep it", and a sweep over a database that cannot be read answers "absent", which the validator reads as "spent". process_compact_block_utxos gets an error channel for the same reason. A negative file_number was guarded by KTH_ASSERT, which evaporates in Release and leaves -1 to become UINT32_MAX in every reference the block writes. That is not an impossible precondition — the header index and the block store are separate files and a crash can leave them disagreeing — so it is reported, and both callers stop the batch rather than storing entries that can never be resolved. Eight negative controls assert the shape of these contracts rather than the behaviour of a database, because a runtime test only fails after the wrong call has been written and run, and they reach Knuth's own layers rather than only UTXO-Z's types. Each is proven to fire: reverting compact() to its 0.8.0 form fails the build with "utxoz_database::compact() must report whether compaction happened"; returning void from block_chain::utxo_compact() fails with "the failure dies between the wrapper and every caller"; and making the database library report the opposite storage mode fails the coherence test with "database library reports reference, this target was compiled as full". That last one replaces a check that asked the same macro twice and could only ever agree with itself — the option is declared in two CMake lists, so the answer worth comparing is the one compiled into the database library against the one compiled here. The compaction OPERATION keeps its name throughout, and so do the things that merely contain the word: BIP152 compact blocks, history_compact, stealth_compact, chain::compact, CompactSize, and secp256k1's compact signatures. Each rename rule names an exact identifier rather than the substring, so none of those could be caught by accident. Full mode remains the default, in the recipe and in both CMake lists. Reference mode is compiled and tested here as a check: it is the half of the #ifdef that nothing in CI builds, so a rename that broke it would go unnoticed until someone turned the flag on. One review comment is deliberately not addressed here. open() takes the path as std::string_view, and path.string() converts through the active code page on Windows, so a data directory with non-ASCII characters cannot be opened. UTXO-Z 0.9.0 exposes no std::filesystem::path or wide overload, and substituting u8string() would be guessing at how the library reconstructs the path on the other side. Reported as utxo-z/utxo-z#109 instead of patched blind. Three further review points are fixed here. A reference the sweep FOUND but could not materialise, and stored bytes that will not decode, are both reads that failed against entries that demonstrably exist; neither is now demoted into the not-found list or dropped silently. And the negative file_number check is gated on reference mode, which is the only mode that stores that field — full mode keeps the whole output and never reads it. 0.9.0 could not have landed. It documented the second list returned by process_pending_lookups() as UNRESOLVED, not absent: a version file that could not be read left its keys there, indistinguishable from keys that exist nowhere, and Knuth reads that list as "spent". Nothing on this side could separate the two. 0.9.1 closes it upstream — the sweep now fails with version_unreadable rather than dropping unreachable keys into the answer, so the second list is absence that was proven, and the failure arrives as an error through the result<> chain this PR already wired. Which makes a comment in this file wrong in a useful way: it claimed the exhaustive switch over error_code would stop compiling when UTXO-Z added a code. 0.9.1 added version_unreadable and this built clean — the -Wswitch that would have warned is neither enabled nor promoted here. The case was added by hand, and the comment now says that adding one is a manual step on every bump. populate_prevout() carried the same defect one layer out: get_utxo() already returned expected<output_info, result_code> and every failure was read as absence, which then probed the mempool. A miss there is indistinguishable from a prevout that does not exist, so a disk that did not answer became a verdict about the transaction. Only key_not_found now enables that fallback; anything else returns error::operation_failed, and the chain from populate_prevout through populate_inputs_sync to populate() carries it to whoever decides. That chain needed the error channel: populate_prevout returned void and populate_inputs_sync returned error::success unconditionally, so changing the `if` alone would have dropped the answer one frame later. The three behavioural controls close the gap the type assertions left: a signature can be right while the body is wrong. Each has an exact negation and a positive that stops it passing by always failing. find() on a closed database must not answer key_not_found, and on an open empty one it must — otherwise an implementation returning `other` for everything would pass the negation and destroy the mempool fallback. An entry the store holds must never come back as absent when materialising or decoding it fails, and the raw payload is asserted present so the case is about the read and not an empty database. A negative file number fails in reference mode and builds in full, with a valid number building in both. A later review round found the same "log it and carry on" shape in four more places. apply_delta() and apply_delta_raw() discarded db_->erase() entirely, so a storage failure ended in result_code::success and a batch was recorded as applied while the outputs it spent were still in the set; not_found stays tolerated, since a delta can legitimately delete a pruned output. apply_delta_raw read the 8-byte reference with two memcpys off a caller-supplied buffer without checking its length, so a short payload read past its end and a long one was truncated into a reference pointing elsewhere. The sync harness swept the deferred queue on any get_utxo error rather than only key_not_found. And utxo_builder trusted KTH_ASSERT for a null header index, which is compiled out in Release, where it would have gone on to read a file number from it — block_tasks already refused at the same fork. The ordering defect is the one that did not need a failure to bite. block_tasks persisted utxo_built_height = batch_end and drained the deferred deletions afterwards, though the comment beside it claimed the opposite order. A crash between the two left a marker saying the batch was complete over a set that still held every output those blocks spent, and the restart trusted the marker. The drain now runs first, which closes that window in one direction: a crash after the drain and before the marker replays the batch, which is safe. The other direction is what #600 specifies and #602 implements — a durable record of which of the two happened, so recovery does not infer it.
fpelliccioni
added a commit
that referenced
this pull request
Aug 10, 2026
#645) The visible half of this bump is a rename: UTXO-Z's second storage mode is called reference rather than compact, so compact_db becomes reference_db, compact_find_result becomes reference_find_result, and the option that selects it follows. That half is safe because getting it wrong does not compile. The dangerous half is what four functions now return. process_pending_lookups, process_pending_deletions and erase went from plain values to result<>, and the compiler found all three. compact_all did not: it used to return void, so a caller that kept invoking it as a statement compiled exactly as before while discarding every failure. That one is the reason this is not a version-number change. erase is the one worth reading twice. It returned a count, and the natural migration writes `*erased > 0 ? success : key_not_found` — which turns "the erase failed" into "the key was not there", about an entry that may still exist and still be spendable. Both drains have the same shape: an unreadable catalogue is not an empty queue, and a caller reads absence as spent. Also integrated, because they are new rather than changed: sync(), which close() does not do for you, and platform_durability(), which says what a successful sync is worth — under contents_only the file contents reach the disk and the directory entries naming them do not. And the exclusive claim, where open() now distinguishes another instance holding the database from a claim that could not be attempted at all; the library's own header notes those send an operator looking in different places, so they are logged as different things. The error enum grew from five values to twenty-one, so failures are logged by name — the numbers moved, and an old note now names a different fault. Reporting the error was not enough: the first version of this logged it and returned an empty pair anyway, which is the same defect wearing a log line. The result now travels through utxoz_database, block_chain and every consumer. batch_validate stops as a local failure instead of answering missing_previous_output, which would reject a possibly-valid block on the strength of a read that never happened; the UTXO builders refuse to advance rather than reporting success. find() and find_raw() map only utxoz::error_code::not_found to key_not_found — closed, catalog_unreadable and recovery_required are read upstream as "queued, sweep it", and a sweep over a database that cannot be read answers "absent", which the validator reads as "spent". process_compact_block_utxos gets an error channel for the same reason. A negative file_number was guarded by KTH_ASSERT, which evaporates in Release and leaves -1 to become UINT32_MAX in every reference the block writes. That is not an impossible precondition — the header index and the block store are separate files and a crash can leave them disagreeing — so it is reported, and both callers stop the batch rather than storing entries that can never be resolved. Eight negative controls assert the shape of these contracts rather than the behaviour of a database, because a runtime test only fails after the wrong call has been written and run, and they reach Knuth's own layers rather than only UTXO-Z's types. Each is proven to fire: reverting compact() to its 0.8.0 form fails the build with "utxoz_database::compact() must report whether compaction happened"; returning void from block_chain::utxo_compact() fails with "the failure dies between the wrapper and every caller"; and making the database library report the opposite storage mode fails the coherence test with "database library reports reference, this target was compiled as full". That last one replaces a check that asked the same macro twice and could only ever agree with itself — the option is declared in two CMake lists, so the answer worth comparing is the one compiled into the database library against the one compiled here. The compaction OPERATION keeps its name throughout, and so do the things that merely contain the word: BIP152 compact blocks, history_compact, stealth_compact, chain::compact, CompactSize, and secp256k1's compact signatures. Each rename rule names an exact identifier rather than the substring, so none of those could be caught by accident. Full mode remains the default, in the recipe and in both CMake lists. Reference mode is compiled and tested here as a check: it is the half of the #ifdef that nothing in CI builds, so a rename that broke it would go unnoticed until someone turned the flag on. One review comment is deliberately not addressed here. open() takes the path as std::string_view, and path.string() converts through the active code page on Windows, so a data directory with non-ASCII characters cannot be opened. UTXO-Z 0.9.0 exposes no std::filesystem::path or wide overload, and substituting u8string() would be guessing at how the library reconstructs the path on the other side. Reported as utxo-z/utxo-z#109 instead of patched blind. Three further review points are fixed here. A reference the sweep FOUND but could not materialise, and stored bytes that will not decode, are both reads that failed against entries that demonstrably exist; neither is now demoted into the not-found list or dropped silently. And the negative file_number check is gated on reference mode, which is the only mode that stores that field — full mode keeps the whole output and never reads it. 0.9.0 could not have landed. It documented the second list returned by process_pending_lookups() as UNRESOLVED, not absent: a version file that could not be read left its keys there, indistinguishable from keys that exist nowhere, and Knuth reads that list as "spent". Nothing on this side could separate the two. 0.9.1 closes it upstream — the sweep now fails with version_unreadable rather than dropping unreachable keys into the answer, so the second list is absence that was proven, and the failure arrives as an error through the result<> chain this PR already wired. Which makes a comment in this file wrong in a useful way: it claimed the exhaustive switch over error_code would stop compiling when UTXO-Z added a code. 0.9.1 added version_unreadable and this built clean — the -Wswitch that would have warned is neither enabled nor promoted here. The case was added by hand, and the comment now says that adding one is a manual step on every bump. populate_prevout() carried the same defect one layer out: get_utxo() already returned expected<output_info, result_code> and every failure was read as absence, which then probed the mempool. A miss there is indistinguishable from a prevout that does not exist, so a disk that did not answer became a verdict about the transaction. Only key_not_found now enables that fallback; anything else returns error::operation_failed, and the chain from populate_prevout through populate_inputs_sync to populate() carries it to whoever decides. That chain needed the error channel: populate_prevout returned void and populate_inputs_sync returned error::success unconditionally, so changing the `if` alone would have dropped the answer one frame later. The three behavioural controls close the gap the type assertions left: a signature can be right while the body is wrong. Each has an exact negation and a positive that stops it passing by always failing. find() on a closed database must not answer key_not_found, and on an open empty one it must — otherwise an implementation returning `other` for everything would pass the negation and destroy the mempool fallback. An entry the store holds must never come back as absent when materialising or decoding it fails, and the raw payload is asserted present so the case is about the read and not an empty database. A negative file number fails in reference mode and builds in full, with a valid number building in both. A later review round found the same "log it and carry on" shape in four more places. apply_delta() and apply_delta_raw() discarded db_->erase() entirely, so a storage failure ended in result_code::success and a batch was recorded as applied while the outputs it spent were still in the set; not_found stays tolerated, since a delta can legitimately delete a pruned output. apply_delta_raw read the 8-byte reference with two memcpys off a caller-supplied buffer without checking its length, so a short payload read past its end and a long one was truncated into a reference pointing elsewhere. The sync harness swept the deferred queue on any get_utxo error rather than only key_not_found. And utxo_builder trusted KTH_ASSERT for a null header index, which is compiled out in Release, where it would have gone on to read a file number from it — block_tasks already refused at the same fork. The ordering defect is the one that did not need a failure to bite. block_tasks persisted utxo_built_height = batch_end and drained the deferred deletions afterwards, though the comment beside it claimed the opposite order. A crash between the two left a marker saying the batch was complete over a set that still held every output those blocks spent, and the restart trusted the marker. The drain now runs first, which closes that window in one direction: a crash after the drain and before the marker replays the batch, which is safe. The other direction is what #600 specifies and #602 implements — a durable record of which of the two happened, so recovery does not infer it.
fpelliccioni
force-pushed
the
fix/utxo-batch-atomicity
branch
from
August 10, 2026 21:22
4c20d07 to
7d59178
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
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/databases/utxoz_database.hpp (1)
140-148: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign
raw_storedwithapply_delta_raw's insert contract.raw_storedexposesvalue, butapply_delta_rawreadsdata. Passing araw_storedrange directly does not compile. Rename the member todataor state that callers must adapt it.🤖 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/databases/utxoz_database.hpp` around lines 140 - 148, Rename raw_stored::value to data so the struct matches the insert element contract consumed by apply_delta_raw. Update the accompanying documentation and all raw_stored construction and access sites to use data, preserving the stored payload semantics.
🧹 Nitpick comments (12)
src/node/test/utxo_batch_atomicity.cpp (1)
40-56: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThe done predicate never becomes true, so every call burns the full 30 seconds.
[] { return false; }at Line 50 tellsutxo_build_taskit is never finished.ctx.run_for(std::chrono::seconds(30))at Line 54 is therefore the only exit, including for the success case at Lines 517-529, where the expected outcome is an emptyfatalsvector. That adds a fixed 30 seconds of wall time to the suite for a test that asserts nothing happened.Pass a predicate that reports completion, as
run_connect_tasksdoes insrc/node/test/sync_harness.hpp(Lines 124-127), and reduce the budget.♻️ Proposed change
std::vector<std::string> run_build_and_collect_fatals(chain_fixture& fixture, - uint32_t start_height) { + uint32_t start_height, + uint32_t done_height) { std::vector<std::string> fatals; ::asio::io_context ctx; std::atomic<uint32_t> contiguous{start_height}; ::asio::co_spawn(ctx, utxo_build_task(fixture.chain(), contiguous, start_height, domain::config::network::regtest, - [] { return false; }, + [&fixture, done_height] { + auto const built = fixture.chain().get_utxo_built_height(); + return built && *built >= done_height; + }, [&fatals](std::string const& reason) { fatals.push_back(reason); }), ::asio::detached); - ctx.run_for(std::chrono::seconds(30)); + ctx.run_for(std::chrono::seconds(30)); // upper bound, not the normal path return fatals; }🤖 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/utxo_batch_atomicity.cpp` around lines 40 - 56, Update run_build_and_collect_fatals so its utxo_build_task completion predicate reports when the build is finished, following the pattern used by run_connect_tasks, instead of always returning false. Then reduce the ctx.run_for timeout to a short budget appropriate for the completion-aware flow while preserving fatal collection and the empty-fatals success expectation.src/blockchain/test/utxoz_contract.cpp (1)
285-301: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the exclusively claimed handle with RAII.
If the assertion at Line 292 or Line 293 fails, Catch2 throws and
first->close()at Line 295 never runs. The exclusive claim then stays held for the rest of the process, so any later open of the same path fails for an unrelated reason. That is the leak the comment at Lines 51-54 describes, applied to the database handle instead of the directory.
scoped_open_dbtakesutxoz_database&, so it cannot hold this handle. Add a small guard for theopen_for_testingresult.♻️ Proposed guard
+// The claim outlives a thrown REQUIRE unless something releases it. +template <typename Db> +struct scoped_claim { + Db& db; + ~scoped_claim() { db.close(); } + scoped_claim(scoped_claim const&) = delete; + scoped_claim& operator=(scoped_claim const&) = delete; +};auto first = utxoz_db::open_for_testing(dir.path.string(), true); REQUIRE(first.has_value()); + { + scoped_claim<utxoz_db> const held{*first}; - // The claim is held. A second open of the same path must fail rather than - // hand out a second writer — the whole reason KTH can stop policing this - // itself is that the library now does. - auto second = utxoz_db::open_for_testing(dir.path.string(), false); - REQUIRE_FALSE(second.has_value()); - REQUIRE(second.error() == utxoz::error_code::database_in_use); - - first->close(); + // The claim is held. A second open of the same path must fail rather + // than hand out a second writer. + auto second = utxoz_db::open_for_testing(dir.path.string(), false); + REQUIRE_FALSE(second.has_value()); + REQUIRE(second.error() == utxoz::error_code::database_in_use); + } // ...and once released, the path opens again. Without this half, a library // that refused every open would pass the assertion above. auto third = utxoz_db::open_for_testing(dir.path.string(), false); REQUIRE(third.has_value()); - third->close(); + scoped_claim<utxoz_db> const released{*third};🤖 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/utxoz_contract.cpp` around lines 285 - 301, Guard the successful handle returned by utxoz_db::open_for_testing with a local RAII cleanup object so its close operation runs during Catch2 assertion unwinding. Ensure the guard releases first before the later third open, and remove reliance on the manual first->close() while preserving the existing second-open failure and post-release reopen assertions.src/blockchain/src/interface/block_chain.cpp (1)
1288-1320: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute the barriers through the member wrappers.
This function reaches the stores directly:
block_store_->flush_undoat Line 1294,utxoz_db_.sync()at Line 1302, anddatabase::node_durability_level()at Lines 1306 and 1309. The class already exposesflush_undo,utxo_syncanddurabilityfor exactly these three barriers, and Lines 1326 and 1339 use the wrappers for steps 10 and 11.The
flush_undowrapper adds a null check onblock_store_and reports it asfile_number == -1. The call at Line 1294 skips that check, so the two paths for the same barrier carry different safety contracts.♻️ Proposed refactor
- if (auto const flushed = block_store_->flush_undo(std::span<int32_t const>{}); ! flushed) { + if (auto const flushed = flush_undo(std::span<int32_t const>{}); ! flushed) { spdlog::critical("[blockchain] Reorg: the undo directory could not be put on stable " "storage after the switch (operation {:`#018x`})", operation_id); return false; } @@ - switch (utxoz_db_.sync()) { + switch (utxo_sync()) { case database::barrier_outcome::crossed: break; case database::barrier_outcome::unsupported: - if (database::node_durability_level() != database::durability_level::none) { + if (durability() != database::durability_level::none) { spdlog::critical("[blockchain] Reorg: the UTXO store reports no durability " "barrier while this node claims '{}'; the two disagree about the same " - "machine", database::to_string(database::node_durability_level())); + "machine", database::to_string(durability())); return false; }🤖 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 1288 - 1320, Update the reorganization barrier logic to use the class member wrappers: replace direct block store undo flushing with flush_undo, direct UTXO synchronization with utxo_sync, and direct durability-level checks with durability. Preserve the existing failure handling and logging while ensuring these steps use the same safety contracts as the later barrier steps.src/database/src/block_store.cpp (2)
1045-1051: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the
undo_checksumhelper instead of rebuilding the input here.Lines 1046-1051 rebuild
prev_hash || undo_dataand hash it. The anonymous-namespace helperundo_checksum(Lines 77-83) does exactly that, andscan_undo_positions(Line 658) andread_undo(Line 821) both verify against it. The writer and the two verifiers must agree forever. One copy makes that structural rather than a convention.♻️ 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); + // The one place this is computed, so the writer and the two verifiers + // (scan_undo_positions, read_undo) cannot drift apart. + 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 1045 - 1051, Replace the duplicated checksum-input construction in the write path with the existing undo_checksum helper, passing prev_hash and undo_data directly. Keep the resulting checksum assignment unchanged so it remains consistent with scan_undo_positions and read_undo.
870-876: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
directory_durability()restates the platform split a third time.See the consolidated comment for the single source.
🤖 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 870 - 876, Update block_store::directory_durability() to reuse the existing single-source platform capability definition instead of repeating the _WIN32 conditional. Preserve the current unsupported result on Windows and available result on other platforms through that centralized symbol or helper.src/database/include/kth/database/databases/utxoz_database.hpp (1)
341-351: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMark
compact()as[[nodiscard]].The comment states the problem precisely: a caller that keeps calling
compact_all()as a statement still compiles and drops every failure.compact()now returnsbool, but without[[nodiscard]]the same call sitechain.utxo_compact();still compiles and still drops the failure. Every other reporting member in this struct carries the attribute.♻️ Proposed change
/// `@return` true if the database compacted, false if it is closed or the /// operation failed (the reason is logged). + [[nodiscard]] bool compact();
block_chain::utxo_compact()insrc/blockchain/src/interface/block_chain.cppforwards this value, so consider the attribute there too.🤖 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/databases/utxoz_database.hpp` around lines 341 - 351, Mark the bool-returning compact() declaration in the UTXO-Z database interface as [[nodiscard]] so ignored compaction results are diagnosed. Also apply [[nodiscard]] to block_chain::utxo_compact(), which forwards the result, while preserving the existing return behavior.src/database/src/databases/utxoz_database.cpp (2)
388-391: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDecode the outpoint index with the explicit little-endian reader this file already uses.
point_to_key(Line 579) writes the index as four explicit little-endian bytes, andkey_to_point(Line 593) reads it back the same way. Line 390 instead usesstd::memcpyinto auint32_t, which takes the host byte order. On a big-endian host the three functions disagree about the same four bytes, and this one produces the wrongoutput_index.♻️ Proposed change
for (auto const& [key, ref] : found) { - uint32_t index; - std::memcpy(&index, key.data() + 32, sizeof(index)); // outpoint index (LE) - auto entry = resolve_reference_ref(ref, index); + // Same explicit little-endian layout point_to_key writes and + // key_to_point reads; memcpy would take the host order instead. + auto const index = key_to_point(key).index(); + auto entry = resolve_reference_ref(ref, index);🤖 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/databases/utxoz_database.cpp` around lines 388 - 391, Replace the std::memcpy-based index decoding in the loop over found with the file’s existing explicit little-endian reader used by key_to_point, preserving the offset at key.data() + 32 so output_index matches point_to_key on all host architectures.
374-437: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe two
process_pending_lookupsbranches duplicate the drain, the guard, and the comment.Lines 375-386 and Lines 407-418 are the same code and the same comment block. Only the per-entry materialization differs:
resolve_reference_refagainstbytes_to_entry. Drain once outside the#ifdef, then keep only the loop body under it. That removes the risk of one copy of the comment or the error handling drifting from the other.🤖 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/databases/utxoz_database.cpp` around lines 374 - 437, Refactor the surrounding lookup-resolution flow to call db_->process_pending_lookups(), validate its result, and unpack found/fail only once before the `#ifdef`. Keep the shared drain error handling and reservation outside the conditional, then retain only the per-entry materialization difference inside each branch: resolve_reference_ref for reference mode and bytes_to_entry otherwise; preserve the existing failure propagation and failed-key handling.src/database/include/kth/database/block_store.hpp (2)
151-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that
undo_scan_result::foundis empty for every non-clean_eofstatus.Line 181 says
foundis "Only meaningful when clean_eof".block_store::scan_undo_positionsgoes further: itsfaillambda callsresult.found.clear(), sofoundis guaranteed empty on failure. State the guarantee in the header, so a caller does not add a defensivefound.clear()of its own.🤖 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 151 - 199, Update the documentation for undo_scan_result::found to explicitly guarantee that it is empty for every status other than undo_scan_status::clean_eof. Preserve the existing statement that recovered locations are meaningful on clean_eof, matching the clearing behavior in block_store::scan_undo_positions.
259-261: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
directory_durability()restates a platform fact that two other places also encode.The same Windows/POSIX split appears in
sync_directoryinsrc/database/include/kth/database/native_file.hppand inkth_own_barriers()insrc/database/src/durability.cpp. See the consolidated comment for the suggested single source.🤖 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 259 - 261, Remove the duplicated platform-specific durability logic exposed by block_store::directory_durability() and make callers use the consolidated source shared with native_file::sync_directory and durability.cpp::kth_own_barriers. Update affected declarations, definitions, and call sites while preserving the existing Windows/POSIX behavior.src/database/src/durability.cpp (1)
49-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThree sites derive the directory-barrier fact from
_WIN32independently.native_file.hppalready owns the authoritative answer throughdirectory_barrier, butdurability.cppandblock_store.cppre-derive the same platform split from the macro. The three can disagree after one edit. A disagreement indurability.cppis the one with consequences: it sets the level the node claims, and a claim offullover a barrier that does not run is the exact failure mode this header set is built to prevent.Add one accessor in
src/database/include/kth/database/native_file.hpp, for exampleconstexpr directory_barrier platform_directory_barrier(), and have the other two call it.
src/database/src/durability.cpp#L49-L55: replace the_WIN32branch inkth_own_barriers()with a test onplatform_directory_barrier(), returningcontents_onlyforunsupportedandfullforavailable. This also makes the already-presentnative_file.hppinclude at Line 11 used.src/database/src/block_store.cpp#L870-L876: returnplatform_directory_barrier()fromdirectory_durability()and drop the_WIN32branch.src/database/include/kth/database/native_file.hpp#L74-L76: defineplatform_directory_barrier()here and havesync_directoryreturn it in theunsupportedresult, so the barrier value and the reported capability come from one expression.🤖 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/durability.cpp` around lines 49 - 55, Centralize the platform directory-barrier value in platform_directory_barrier() within src/database/include/kth/database/native_file.hpp, and have sync_directory return it for the unsupported result (src/database/include/kth/database/native_file.hpp#L74-L76). In src/database/src/durability.cpp#L49-L55, update kth_own_barriers() to return contents_only for unsupported and full for available using that accessor. In src/database/src/block_store.cpp#L870-L876, replace the _WIN32 branch in directory_durability() with a direct return of platform_directory_barrier().src/database/include/kth/database/native_file.hpp (1)
78-84: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRetry
openandfsynconEINTR.
sync_directoryreports any failure as{available, false}.block_store::flush_undothen treats that as a fatalundo_flush_error, and the batch is refused. A singleEINTRfrom::openor::fsynctherefore stops a transition that had no real durability problem.fsyncis permitted to returnEINTR, and it does so in practice on network filesystems.♻️ Proposed retry on `EINTR`
- int const fd = ::open(dir.c_str(), O_RDONLY | O_DIRECTORY); + int fd = -1; + do { + fd = ::open(dir.c_str(), O_RDONLY | O_DIRECTORY); + } while (fd < 0 && errno == EINTR); if (fd < 0) { return {directory_barrier::available, false}; } - bool const ok = ::fsync(fd) == 0; + int rc = 0; + do { + rc = ::fsync(fd); + } while (rc != 0 && errno == EINTR); + bool const ok = rc == 0; ::close(fd); return {directory_barrier::available, ok};This also needs
#include <cerrno>in the non-Windows branch.🤖 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/native_file.hpp` around lines 78 - 84, Update sync_directory to retry ::open and ::fsync when they fail with EINTR, continuing until they succeed or return a different error; preserve the existing {directory_barrier::available, false} result for non-EINTR failures. Add the required <cerrno> include in the non-Windows branch.
🤖 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/src/interface/block_chain.cpp`:
- Around line 1228-1237: Update the divergence exit in switch_to_branch to
return a switch_result with fatal set to true, while preserving the existing
failure, empty-tip, and mutated values. Ensure this result is propagated through
switch_to_branch_async so callers relying on fatal consistently recognize the
unfinished transition record.
In `@src/blockchain/test/transition_lifecycle.cpp`:
- Around line 54-73: Update the test case around chain_fixture and
begin_transition_record to actually close and reopen the environment, then use a
second short-lived chain to read the persisted transition record and compare
operation_id, type, first_height, and intended_last_height with record. Preserve
the restart-failure assertion only if it remains part of the intended behavior;
otherwise remove this duplicate test and its unsupported persistence claim.
In `@src/database/include/kth/database/databases/internal_database.hpp`:
- Around line 92-95: Update publish_transition to reject transition_heights when
both last_block_height and utxo_built_height are absent, before starting the
transaction or clearing utxo_transition. Preserve the existing behavior for
valid heights, and add a test verifying an existing marker remains set when
empty transition_heights is provided.
In `@src/database/src/block_store.cpp`:
- Around line 836-857: Protect the shared file_info_ accesses used by flush_undo
and allocate_block_space with the block_store synchronization mechanism, adding
one if none exists, so resizing and reads cannot race. Ensure every file_info_
read or update in the allocation and undo-flushing paths uses the same mutex or
is serialized on a common executor, while preserving flush_undo’s validation and
error behavior.
In `@src/database/src/utxo_transition_record.cpp`:
- Around line 39-52: Update utxo_transition_record::encode to validate
format_version, type, and state before writing any bytes, rejecting values
unsupported by decode_transition_record. Use KTH_CONTRACT for these
programmer-input checks rather than KTH_ASSERT, while preserving normal
serialization and checksum generation for valid records.
---
Outside diff comments:
In `@src/database/include/kth/database/databases/utxoz_database.hpp`:
- Around line 140-148: Rename raw_stored::value to data so the struct matches
the insert element contract consumed by apply_delta_raw. Update the accompanying
documentation and all raw_stored construction and access sites to use data,
preserving the stored payload semantics.
---
Nitpick comments:
In `@src/blockchain/src/interface/block_chain.cpp`:
- Around line 1288-1320: Update the reorganization barrier logic to use the
class member wrappers: replace direct block store undo flushing with flush_undo,
direct UTXO synchronization with utxo_sync, and direct durability-level checks
with durability. Preserve the existing failure handling and logging while
ensuring these steps use the same safety contracts as the later barrier steps.
In `@src/blockchain/test/utxoz_contract.cpp`:
- Around line 285-301: Guard the successful handle returned by
utxoz_db::open_for_testing with a local RAII cleanup object so its close
operation runs during Catch2 assertion unwinding. Ensure the guard releases
first before the later third open, and remove reliance on the manual
first->close() while preserving the existing second-open failure and
post-release reopen assertions.
In `@src/database/include/kth/database/block_store.hpp`:
- Around line 151-199: Update the documentation for undo_scan_result::found to
explicitly guarantee that it is empty for every status other than
undo_scan_status::clean_eof. Preserve the existing statement that recovered
locations are meaningful on clean_eof, matching the clearing behavior in
block_store::scan_undo_positions.
- Around line 259-261: Remove the duplicated platform-specific durability logic
exposed by block_store::directory_durability() and make callers use the
consolidated source shared with native_file::sync_directory and
durability.cpp::kth_own_barriers. Update affected declarations, definitions, and
call sites while preserving the existing Windows/POSIX behavior.
In `@src/database/include/kth/database/databases/utxoz_database.hpp`:
- Around line 341-351: Mark the bool-returning compact() declaration in the
UTXO-Z database interface as [[nodiscard]] so ignored compaction results are
diagnosed. Also apply [[nodiscard]] to block_chain::utxo_compact(), which
forwards the result, while preserving the existing return behavior.
In `@src/database/include/kth/database/native_file.hpp`:
- Around line 78-84: Update sync_directory to retry ::open and ::fsync when they
fail with EINTR, continuing until they succeed or return a different error;
preserve the existing {directory_barrier::available, false} result for non-EINTR
failures. Add the required <cerrno> include in the non-Windows branch.
In `@src/database/src/block_store.cpp`:
- Around line 1045-1051: Replace the duplicated checksum-input construction in
the write path with the existing undo_checksum helper, passing prev_hash and
undo_data directly. Keep the resulting checksum assignment unchanged so it
remains consistent with scan_undo_positions and read_undo.
- Around line 870-876: Update block_store::directory_durability() to reuse the
existing single-source platform capability definition instead of repeating the
_WIN32 conditional. Preserve the current unsupported result on Windows and
available result on other platforms through that centralized symbol or helper.
In `@src/database/src/databases/utxoz_database.cpp`:
- Around line 388-391: Replace the std::memcpy-based index decoding in the loop
over found with the file’s existing explicit little-endian reader used by
key_to_point, preserving the offset at key.data() + 32 so output_index matches
point_to_key on all host architectures.
- Around line 374-437: Refactor the surrounding lookup-resolution flow to call
db_->process_pending_lookups(), validate its result, and unpack found/fail only
once before the `#ifdef`. Keep the shared drain error handling and reservation
outside the conditional, then retain only the per-entry materialization
difference inside each branch: resolve_reference_ref for reference mode and
bytes_to_entry otherwise; preserve the existing failure propagation and
failed-key handling.
In `@src/database/src/durability.cpp`:
- Around line 49-55: Centralize the platform directory-barrier value in
platform_directory_barrier() within
src/database/include/kth/database/native_file.hpp, and have sync_directory
return it for the unsupported result
(src/database/include/kth/database/native_file.hpp#L74-L76). In
src/database/src/durability.cpp#L49-L55, update kth_own_barriers() to return
contents_only for unsupported and full for available using that accessor. In
src/database/src/block_store.cpp#L870-L876, replace the _WIN32 branch in
directory_durability() with a direct return of platform_directory_barrier().
In `@src/node/test/utxo_batch_atomicity.cpp`:
- Around line 40-56: Update run_build_and_collect_fatals so its utxo_build_task
completion predicate reports when the build is finished, following the pattern
used by run_connect_tasks, instead of always returning false. Then reduce the
ctx.run_for timeout to a short budget appropriate for the completion-aware flow
while preserving fatal collection and the empty-fatals success expectation.
🪄 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: e97b6d41-392a-41c7-8ec7-764c3cc48f38
⛔ Files ignored due to path filters (2)
docs/utxo-store-api-requirements.mdis excluded by none and included by nonesrc/database/include/kth/database/databases/internal_database.ippis excluded by!**/*.ippand included bysrc/database/**
📒 Files selected for processing (24)
src/blockchain/CMakeLists.txtsrc/blockchain/include/kth/blockchain/interface/block_chain.hppsrc/blockchain/src/interface/block_chain.cppsrc/blockchain/test/transition_lifecycle.cppsrc/blockchain/test/undo_barriers.cppsrc/blockchain/test/utxo_transition_record.cppsrc/blockchain/test/utxoz_contract.cppsrc/database/CMakeLists.txtsrc/database/include/kth/database/block_store.hppsrc/database/include/kth/database/databases/internal_database.hppsrc/database/include/kth/database/databases/property_code.hppsrc/database/include/kth/database/databases/utxoz_database.hppsrc/database/include/kth/database/durability.hppsrc/database/include/kth/database/native_file.hppsrc/database/include/kth/database/utxo_transition_record.hppsrc/database/src/block_store.cppsrc/database/src/databases/utxoz_database.cppsrc/database/src/durability.cppsrc/database/src/utxo_transition_record.cppsrc/node/CMakeLists.txtsrc/node/src/sync/block_tasks.cppsrc/node/src/sync/reorg.cppsrc/node/test/sync_harness.hppsrc/node/test/utxo_batch_atomicity.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/node/CMakeLists.txt
Comment on lines
+836
to
+857
| block_store::flush_undo(std::span<int32_t const> file_numbers) { | ||
| // One number per block arrives here, so a thousand-block batch inside one | ||
| // file asks for a thousand barriers on it. Normalized once, not at every | ||
| // call site — and sorted, so the order the files are synced in does not | ||
| // depend on the order the blocks happened to arrive. | ||
| std::vector<int32_t> files(file_numbers.begin(), file_numbers.end()); | ||
| std::sort(files.begin(), files.end()); | ||
| files.erase(std::unique(files.begin(), files.end()), files.end()); | ||
|
|
||
| for (auto const file : files) { | ||
| if (file < 0 || size_t(file) >= file_info_.size()) { | ||
| return std::unexpected(undo_flush_error{result_code::other, file}); | ||
| } | ||
|
|
||
| flat_file_pos const pos{file, file_info_[size_t(file)].undo_size}; | ||
| if ( ! undo_files_.flush(pos, /*finalize*/ false)) { | ||
| // Stop here. The caller cannot act on "some of it reached the disk" | ||
| // any differently than on "none of it did", and continuing would | ||
| // only replace the first failure's file number with the last one's. | ||
| return std::unexpected(undo_flush_error{result_code::other, file}); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether block_store holds any mutex at all.
fd -t f 'block_store' --exec ast-grep outline {} --items all --type class
rg -nP -C 3 '\b(std::mutex|shared_mutex|scoped_lock|lock_guard|unique_lock)\b' \
src/database/include/kth/database/block_store.hpp src/database/src/block_store.cpp
# Find every flush_undo caller and the exclusion it holds.
rg -nP -C 12 '\bflush_undo\s*\(' --type=cpp
# Find every write_undo / find_undo_pos caller for comparison.
rg -nP -C 6 '\b(write_undo|store_block_undo)\s*\(' --type=cppRepository: k-nuth/kth
Length of output: 263
🏁 Script executed:
#!/bin/bash
set +e
printf '%s\n' '--- block_store declarations and members ---'
rg -n -P -C 8 'class block_store|flush_undo|find_undo_pos|write_undo|store_block_undo|file_info_|undo_files_' \
src/database/include/kth/database/block_store.hpp \
src/database/src/block_store.cpp
printf '%s\n' '--- all call sites ---'
rg -n -P -C 16 '\bflush_undo\s*\(|\bfind_undo_pos\s*\(|\bwrite_undo\s*\(|\bstore_block_undo\s*\(' \
src --glob '*.cpp' --glob '*.hpp'
printf '%s\n' '--- synchronization primitives and lock operations in database sources ---'
rg -n -P -C 5 '\b(std::mutex|std::shared_mutex|std::recursive_mutex|std::scoped_lock|std::lock_guard|std::unique_lock|std::shared_lock|\.lock\s*\(|\.unlock\s*\(|try_lock)\b' \
src/database --glob '*.cpp' --glob '*.hpp'Repository: k-nuth/kth
Length of output: 50367
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- block_store thread-safety contract ---'
sed -n '25,45p;70,105p;298,312p' src/database/include/kth/database/block_store.hpp
printf '%s\n' '--- direct flush_undo calls ---'
rg -n '\bflush_undo\s*\(' . --glob '*.{cpp,hpp,h,cc,cxx}' --glob '!build/**' --glob '!dist/**'
printf '%s\n' '--- direct write_undo calls ---'
rg -n '\bwrite_undo\s*\(' . --glob '*.{cpp,hpp,h,cc,cxx}' --glob '!build/**' --glob '!dist/**'
printf '%s\n' '--- allocation and undo-write API calls ---'
rg -n '\b(allocate_block_space|write_block_at|find_undo_pos|write_undo_to_disk)\s*\(' . --glob '*.{cpp,hpp,h,cc,cxx}' --glob '!build/**' --glob '!dist/**'Repository: k-nuth/kth
Length of output: 10581
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- block_chain undo write path ---'
sed -n '535,620p;935,980p;1260,1310p' src/blockchain/src/interface/block_chain.cpp
printf '%s\n' '--- node undo batch and surrounding scheduling ---'
sed -n '2180,2270p' src/node/src/sync/block_tasks.cpp
printf '%s\n' '--- relevant callers of block_chain write/flush APIs ---'
rg -n -P -C 10 '\b(write_block|write_undo|flush_undo|allocate_block_space)\s*\(' \
src/blockchain/src src/node/src --glob '*.cpp'Repository: k-nuth/kth
Length of output: 26347
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- production store_block_undo callers ---'
rg -n -P -C 18 '\bstore_block_undo\s*\(' src --glob '*.cpp' --glob '*.hpp'
printf '%s\n' '--- production publish_reorg_transition callers ---'
rg -n -P -C 18 '\bpublish_reorg_transition\s*\(' src --glob '*.cpp' --glob '*.hpp'
printf '%s\n' '--- block task functions and coroutine boundaries ---'
rg -n -P -C 8 '\b(utxo_build|block_storage_task|store_chunk|process_pending|connect|reorg)\b' \
src/node/src/sync/block_tasks.cpp --glob '*.cpp'
printf '%s\n' '--- block_store mutex includes and complete class declaration ---'
sed -n '1,75p;275,315p' src/database/include/kth/database/block_store.hppRepository: k-nuth/kth
Length of output: 50366
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- task launch sites and executors ---'
rg -n -P -C 14 '\b(block_storage_task|utxo_build_task)\s*\(' src/node/src --glob '*.cpp'
printf '%s\n' '--- executor and pool declarations near task setup ---'
rg -n -P -C 12 '\b(priority_pool_|co_spawn|make_strand|thread_pool|executor)\b' \
src/node/src/sync/block_tasks.cpp src/node/include --glob '*.cpp' --glob '*.hpp'
printf '%s\n' '--- exact file_info_ mutation sites ---'
rg -n -P -C 4 'file_info_(\.resize|\[|\.push_back)|last_block_file_' \
src/database/src/block_store.cpp src/database/include/kth/database/block_store.hppRepository: k-nuth/kth
Length of output: 50366
Protect file_info_ access across block allocation and undo flushing
block_storage_task and utxo_build_task run concurrently. allocate_block_space() can resize and update file_info_ while flush_undo() reads it. block_store has no mutex despite its Thread-safe contract. Protect all file_info_ accesses, or serialize allocation and flushing on one executor.
🤖 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 836 - 857, Protect the shared
file_info_ accesses used by flush_undo and allocate_block_space with the
block_store synchronization mechanism, adding one if none exists, so resizing
and reads cannot race. Ensure every file_info_ read or update in the allocation
and undo-flushing paths uses the same mutex or is serialized on a common
executor, while preserving flush_undo’s validation and error behavior.
Contributor
Author
There was a problem hiding this comment.
Agreed that this is real, and deliberately not fixed here.
block_storage_task and utxo_build_task do run concurrently, block_store has no mutex, and the Thread-safe contract in the header overstates what the type provides. None of that is introduced by this PR — the allocation and flush paths predate it, and this PR touches flush_undo only to route the reorganization through the existing member wrapper.
Folding it in is a "Heavy lift" by your own classification, on a path this PR does not otherwise change, into a branch that is already large and whose correctness argument is about the deferred-deletion queue. Serialising allocation and flushing on one executor is a design decision about the sync architecture, not a local repair, and it belongs next to the staged exclusion work described in docs/utxo-set-synchronization.md.
Left open on purpose so it is not lost. Not resolving this thread.
There was a problem hiding this comment.
@fpelliccioni, acknowledged. The finding remains valid, but the required synchronization design is outside this PR’s intended scope.
I will leave the thread open. I will treat docs/utxo-set-synchronization.md as the source for the staged exclusion work when reviewing follow-up changes.
🐇
✏️ Learnings added
Learnt from: fpelliccioni
Repo: k-nuth/kth PR: 602
File: src/database/src/block_store.cpp:836-857
Timestamp: 2026-08-11T09:42:51.718Z
Learning: In the KTH node synchronization architecture, `block_storage_task` and `utxo_build_task` can run concurrently while sharing `kth::database::block_store`. `block_store` currently has no mutex, so its `Thread-safe` header contract overstates its implementation. The resulting `file_info_` allocation/flush race predates PR `#602`. The required fix is a broader synchronization design, such as serializing allocation and flushing on one executor, and belongs with the staged exclusion work in `docs/utxo-set-synchronization.md`, not with durable UTXO transition changes.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
fpelliccioni
force-pushed
the
fix/utxo-batch-atomicity
branch
from
August 10, 2026 22:53
7d59178 to
d6b57dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/node/test/reorg_deferred_sweep.cpp`:
- Around line 379-385: Update the failed reorganization assertions in the test
around chain.get_last_heights() and chain.read_transition_record() to expect the
previously published height 101u, not trunk_len. Preserve the recovery_required
transition status, ensuring the test verifies no new height publication occurs
when the sweep or durability barriers fail.
🪄 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: 2cfe06f9-0a2a-4d4e-a65b-5043730bed18
⛔ Files ignored due to path filters (1)
src/database/include/kth/database/databases/internal_database.ippis excluded by!**/*.ippand included bysrc/database/**
📒 Files selected for processing (9)
src/blockchain/include/kth/blockchain/interface/block_chain.hppsrc/blockchain/src/interface/block_chain.cppsrc/blockchain/src/utxo_builder.cppsrc/blockchain/test/transition_lifecycle.cppsrc/database/include/kth/database/databases/utxoz_database.hppsrc/database/src/databases/utxoz_database.cppsrc/database/src/utxo_transition_record.cppsrc/node/CMakeLists.txtsrc/node/test/reorg_deferred_sweep.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/blockchain/test/transition_lifecycle.cpp
- src/blockchain/include/kth/blockchain/interface/block_chain.hpp
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #602 +/- ##
==========================================
- Coverage 80.40% 80.22% -0.18%
==========================================
Files 289 292 +3
Lines 14538 14791 +253
==========================================
+ Hits 11689 11866 +177
- Misses 2849 2925 +76 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
fpelliccioni
force-pushed
the
fix/utxo-batch-atomicity
branch
from
August 11, 2026 05:18
d6b57dc to
6bb999b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/node/test/reorg_deferred_sweep.cpp (1)
7-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the standard headers this file uses directly.
The file uses
std::any_of(line 283),std::function(line 165),std::runtime_error(lines 152 and 156), andstd::span(lines 87 and 484). None of<algorithm>,<functional>,<stdexcept>, or<span>is included. The build currently depends on transitive includes fromtest_helpers.hppor the asio headers, which can break with a toolchain or dependency change.♻️ Proposed include additions
+#include <algorithm> `#include` <chrono> +#include <functional> +#include <span> +#include <stdexcept> `#include` <vector>🤖 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_deferred_sweep.cpp` around lines 7 - 18, Add direct standard-library includes for the symbols used in reorg_deferred_sweep.cpp: include <algorithm> for std::any_of, <functional> for std::function, <stdexcept> for std::runtime_error, and <span> for std::span. Keep the existing includes and behavior unchanged.
🤖 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/node/test/reorg_deferred_sweep.cpp`:
- Around line 164-180: Update run_switch to track whether the co_spawn
completion handler ran, retain its std::exception_ptr, and rethrow any non-null
exception after ctx.run_for. Treat an incomplete handler invocation after the
timeout as a failure, and construct reorg_outcome without the unnecessary value
initialization while preserving successful result assignment.
---
Nitpick comments:
In `@src/node/test/reorg_deferred_sweep.cpp`:
- Around line 7-18: Add direct standard-library includes for the symbols used in
reorg_deferred_sweep.cpp: include <algorithm> for std::any_of, <functional> for
std::function, <stdexcept> for std::runtime_error, and <span> for std::span.
Keep the existing includes and behavior unchanged.
🪄 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: da8f5ebc-efa1-4c5c-a2e2-647913d7b748
📒 Files selected for processing (7)
src/blockchain/include/kth/blockchain/interface/block_chain.hppsrc/blockchain/src/interface/block_chain.cppsrc/database/include/kth/database/databases/utxoz_database.hppsrc/database/include/kth/database/native_file.hppsrc/database/src/databases/utxoz_database.cppsrc/database/src/utxo_transition_record.cppsrc/node/test/reorg_deferred_sweep.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- src/database/src/utxo_transition_record.cpp
- src/database/include/kth/database/native_file.hpp
- src/database/include/kth/database/databases/utxoz_database.hpp
- src/blockchain/src/interface/block_chain.cpp
fpelliccioni
force-pushed
the
fix/utxo-batch-atomicity
branch
from
August 11, 2026 06:48
6bb999b to
8a6a4ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/node/test/reorg_deferred_sweep.cpp`:
- Around line 21-25: Update the includes in reorg_deferred_sweep.cpp to add the
direct Asio headers awaitable.hpp, as_tuple.hpp, and use_awaitable.hpp for the
symbols used by the test, and remove detached.hpp since ::asio::detached is
unused.
🪄 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: 210d2df6-0c93-4676-b40e-1d6dd854547f
📒 Files selected for processing (2)
src/blockchain/src/interface/block_chain.cppsrc/node/test/reorg_deferred_sweep.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/blockchain/src/interface/block_chain.cpp
fpelliccioni
force-pushed
the
fix/utxo-batch-atomicity
branch
from
August 11, 2026 09:46
8a6a4ad to
45f36b5
Compare
This was referenced Aug 11, 2026
fpelliccioni
force-pushed
the
fix/utxo-batch-atomicity
branch
3 times, most recently
from
August 11, 2026 19:22
18c871d to
801858a
Compare
Applying a batch's UTXO delta mutates the maps in place. There is no staging, no transaction and nothing to roll back, so the sequence cannot be made reversible. It is made detectable instead. A versioned, checksummed record is written before the first mutation and cleared only in the same transaction that publishes the heights. Between those two points every store the transition touched is forced to disk and every barrier is checked. A start that finds the record refuses to open the database at all — before UTXO-Z, before the block store, before a single height is believed — because the answer to an interrupted transition is a rebuild, and a node that only refused once it reached the build would already have served reads off a set that may be half-applied. The record is one property rather than a field per attribute: publication is then a single put and is atomic by construction, and "clean" is the absence of the key rather than a value each field has to be checked against separately. Its envelope is fixed outside the format version — the version is the first two bytes, the checksum the last four — which is what makes the validation order sound: a field cannot be trusted before the checksum covering it, and the checksum cannot be located by a version not yet trusted. Serialization is explicit and byte-wise, pinned in a test against literal bytes rather than a round trip, which would agree with itself even if both halves drifted onto native layout. The operation id comes from system entropy, never a clock or a pid: those collide exactly when several nodes are started by one script, which is when the id is worth having. Every way of failing to read the record stays distinct from "there is nothing here". A storage error refuses, a record this build cannot decode refuses, and a record that decodes refuses; only the absent key reads as clean. That conflation is the failure this exists to prevent, and its own reader would be a poor place to reintroduce it. None of it means anything without the barriers underneath, and none of the three stores was forced to disk. The LMDB environment is opened MDB_NOSYNC, so a commit returns before anything reaches the platter: a height marker could outlive, or be outlived by, the set it describes. `env_sync` forces the barrier, reports the result, and cannot be discarded; `close` no longer drops its own either, because a failure there means transactions this run called committed were lost and a silent shutdown leaves the next start to find it as corruption with no explanation. `block_store::flush` returned void, discarded both results it got, covered only the last block file, and had no caller anywhere in the repo. A batch that crosses a rotation writes undo into more than one rev*.dat, so the last one was never the right set. `flush_undo` takes the set, normalizes it — one number per block arrives, so duplicates are the normal case — stops at the first failure, and carries the file number in the error rather than in a log line, where a caller deciding whether the node can continue could not act on it. And a file whose every byte is on the platter still does not exist if the directory entry naming it was never written, so the directory barrier runs too, unconditionally: tracking which rev file is new would save one fsync per batch, and a barrier skipped because the tracking was wrong costs a database. UTXO-Z's barrier answered `true` or `false`, which folded "this platform has none" into "one was attempted and failed". Only the second is a defect and only the first may be carried on past, so the wrapper now reports which of the three happened. What the node may claim is the weakest of the four barriers — LMDB, the rev contents, the rev directory, UTXO-Z — computed rather than configured, and reported as KTH's own rather than borrowed from whichever store was asked last. A barrier that was attempted and failed is fatal on every platform: a durability level describes what a platform can promise, and it never turns a refusal into a success. The same protocol runs on both paths. The reorganization records itself before the first block comes off and publishes at the end, and `disconnect_block`'s two height writes become one transaction — separately they left an instant where the stored-block height named one block and the built height another, with neither wrong on its own. A switch that stops cleanly still publishes the state the stores agree on, because clearing the record asserts that and only that; a switch that leaves them disagreeing does not, and the next start refuses. What this does not do is repair anything. There is no rollback and no resume: the delta mutates the maps in place, so an interrupted transition can be neither reversed nor replayed. A rollback-from-undo design was considered and parked — it would depend on the store's inverse delta being idempotent, which was never verified, and nobody should later assume it was. Retrying is safe only before the first mutation. Past it, an indeterminate answer fails closed. The tests stage each durable boundary and restart for real: the record on disk and nothing mutated, the delta applied, the deletions drained, every barrier crossed, and finally the publication — where the node must come back up rather than refuse, or every refusal above would be satisfied by one that refuses unconditionally. A restart after a completed batch is counted rather than sampled: a duplicate insert or a deletion that never ran moves the set's size, and neither shows up in a spot check of one outpoint. Each of them was checked against its own removal. Dropping the startup refusal turns nine red; dropping the record write turns red the one test that provokes a real mid-batch failure and asks what was left behind; dropping the clearing turns three red, including the restart; dropping the reorganization's publication turns its own red. What no test here can show is that an fsync happened: a barrier that did not run is invisible to a process that exits cleanly, and only a power cut tells the difference. Those calls are covered for what they do and report, not for being called. The rewind had the same hole one layer down. A switch's inverse deltas erase the outputs the abandoned blocks created, and an erase is not a deletion: UTXO-Z reaches the mapped version and the cached files, and everything older is queued for a sweep that runs only when it is asked to. That queue is memory. Publishing without draining it declares the rewind finished while those outputs are still in the set — inputs validation and the mempool will resolve — until the restart that loses the queue and leaves them there for good. The connect path already drained before it published. This one flushed, published its heights and cleared the record over a set that still owed deletions. Draining alone would not have been enough, and the drain's own report is where this could have gone quietly wrong. It returns the keys it could not delete, and 0.9.1 documents that list as unresolved rather than absent: a version file it could not read is logged, skipped, and its keys arrive beside keys that genuinely are not stored. Both occur here — an output created and spent inside one abandoned block was never in the set, so failing to erase it is correct — so the list settles nothing by itself. Read as absence it turns a storage fault into an output this node believes it removed; read as failure it makes every ordinary reorganization fatal. It is read as neither. Every key it names goes back through find() and the lookup sweep, which is the one 0.9.1 taught to tell the two apart: absence there is reached only by reading every version below the current one, and a sweep that could not cover them all returns version_unreadable or catalog_unreadable and consumes nothing. Proven absence passes. A key that still resolves, a key the sweep accounts for in neither list, and any failure of the sweep itself are fatal, and each leaves the record standing. Until process_pending_deletions() reports the same way, that second sweep is what carries the guarantee, which is why it cannot be folded away into trusting the first. Four other places had the same shape once it was worth looking for. publish_transition cleared the record when handed no heights at all, which is a transition declared finished without saying where it finished. encode() would checksum a type or state it has no layout for and persist a record every later start declines — a contract rather than a check, because both call sites write literals and there is no input path to it. Compaction reports duplicate_key when it finds one key held by two stored entries, and the build stepped over it. And the bloom filter, whose whole purpose is to let apply_delta_raw SKIP keys, was built from a walk whose failure was discarded: a walk that stopped early licenses skipping every key it never reached, which is a delete that never happens. The reorganization tests fill the queue before they assert anything about it. A block carrying a spend and the spend of that spend puts an output into the set and takes it out again inside one block, so the rewind's erase finds it in no mapped version and defers — the queue is measured non-empty first, and only then is a completed switch asked to leave it empty. Each refusal is checked against its own negation: that the record survives it, that the heights are not published anyway, and that a switch which genuinely completed still restarts and comes up where it left off. Both storage modes run all of it. The proof that the outputs are gone does not ask the store, and that is the substance of it rather than a detail of how. Asking meant find() and process_pending_lookups(), whose pending set is GLOBAL and drained wholesale: a caller borrowing it owns it for the whole check-then-drain sequence, and one producer cannot be excluded. The single-transaction validate path resolves prevouts lock-free — its own comment says so — and the reorganization barrier parks registered writers, which it is not. Between the check and the drain a concurrent lookup could be queued, and the switch would consume it: its owner then reads its own prevouts as absent, which it reports as spent, while the key appears in the switch's results as a rewind that did not finish. One window, two ways to be wrong. A lock does not close it. The exclusion primitive has no shared mode and is already held across suspension points, which the synchronization document calls undefined behaviour that is live rather than latent, and that path suspends three times; excluding it that way would add the defect rather than remove it. So the switch stops being a producer of that queue instead of trying to coordinate the others. Nothing in the sweep reads the store, so nothing in it can race with a reader, whatever the reader does. What replaces it is the delta the rewind itself applied. An output created AND spent inside one block never entered the set — the connect-side delta netted it out — so its inverse erase is a no-op and comes back as a failed deletion, and that is the only failure tolerated. The set is the intersection of the outpoints a block erases with the outpoints it spends: both are the delta's own keys, so it is the answer and not an approximation of it. Per block, and that is where it would go wrong. An output created at one height and spent by a LATER block of the same abandoned branch was restored when that later block was disconnected, so it is in the set when the earlier block's erase runs and that erase must succeed. A branch-wide intersection sweeps it in and licenses exactly the failure this exists to catch. The test that pins this reads the accumulated set directly rather than through its effects, and a branch-wide rule passes every other test here and fails that one. The drain's queue counter is not read after the drain, and the check that used to follow it is gone rather than corrected. UTXO-Z empties the pending set whether or not the individual deletions applied — a key it could not delete is removed and reported in `failed`, not left behind — so a post-drain count is zero however the call went. Asserting the queue was emptied therefore asserted nothing while looking like it asserted the whole property, and reporting that count in the failure log would have told an operator nothing was outstanding at the one moment something was. What is outstanding is in `failed`, which is what decides this, and the count that is still worth printing is the one taken before the call. The test harness no longer lets either of its two failures leave through a return value. co_spawn hands a thrown exception to the completion handler rather than out of the call, and run_for returns on a budget expiring exactly as it does on the work finishing, so discarding the exception and reading the result unconditionally made a hang and a throw both arrive as a default reorg_outcome — not ok, not fatal, no tip, which is indistinguishable from a switch that declined to run. A refusal test handed that would have passed having proven nothing. The result is held where only a real completion can fill it, the exception is rethrown before the budget is even considered so the reported cause is the cause that happened, and both failures are pinned by their own tests. UTXO-Z 0.10.0 takes the queues out of the store, and the half of this that was compensating for them goes with it. erase() is gone. It could not survive the removal: reaching the mapped version and deferring the rest was what it did, and with nowhere to defer to a lone key would pay the whole descent through the version files by itself. Removal is a batch the caller owns and applies once, and the descent drops keys from its working set as it goes, so each further file is searched for fewer of them. The lookup queue is gone the same way. find() reads the active versions, records nothing and returns not_resolved — which is a fact about which files were consulted, not about the database, and is mapped to a code of its own rather than onto key_not_found. Absence is established only by resolve(), over a batch the caller holds from one call to the next. That is what closes the defect where a block validation drained a global queue, took a lookup a concurrent transaction validation had emitted, and reported the absence it proved as a missing prevout of the block it was judging. What a deletion did is now three facts rather than one ambiguous list. `erased` is applied — including when the call also reports a fault, because the walk writes as it goes — and resending it would ask about a key that is now genuinely gone, get absence back, and turn this operation's own success into a refusal. `absent` is proven, established only after every version that could hold the key was read, and never stands for a file that could not be opened. `unresolved` is what is still owed, and the only category that may be sent again. The retry is bounded and lives inside the running operation: an interrupted transition is still refused at the next start and rebuilt, never resumed. Whether a proven absence is legitimate stays a question about the delta, answered without reading the store — which is what keeps this path unable to race with a reader however that reader is scheduled. The classification is derived per block and folded conservatively: a key is tolerated absent only while every block that asked for its deletion created and spent it itself, so one appearance that needs the key to exist dominates every appearance that would excuse it. Outpoints are unique, so today no key is contributed twice and the fold never combines; it combines anyway, because the batch is answered once per distinct key and a union would let one block's legitimate no-op excuse another block's real deletion the moment anything widens the obligation. The policy is one function, shared by the connect batch and the switch, and it is exercised against outcomes storage cannot be asked to produce: a fault partway through, an obligation that never clears, a fault reported with nothing left owed. Each has a test, and each of the six ways to get it wrong — resending what was applied, reading unresolved as absent, publishing before completing, losing the record of partial progress, folding the classification as a union, treating a reported fault as a clean run — turns exactly its own test red and no other. Closes #600. Closes #584. Closes #646.
fpelliccioni
force-pushed
the
fix/utxo-batch-atomicity
branch
from
August 11, 2026 19:37
801858a to
a661ea2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Applying a batch's UTXO delta mutates the maps in place. There is no staging, no
transaction and nothing to roll back, so the sequence cannot be made reversible.
It is made detectable instead.
The record
A versioned, checksummed record in one property (
property_code::utxo_transition),so publication is a single put and is atomic by construction, and "clean" is the
absence of the key rather than a value each field has to be checked against
separately.
format_versionoperation_idoperation_typeconnect_batchorreorg— different rebuild answersfirst_height/intended_last_heightstatein_progressis the only value. The field exists so a later one needs no format bumpchecksumThe envelope is fixed outside the version: version = first two bytes,
checksum = last four. That is what makes the validation order sound — a field
cannot be trusted before the checksum covering it, and the checksum cannot be
located by a version not yet trusted. Serialization is explicit and byte-wise,
pinned against literal bytes rather than a round trip, which agrees with itself
even if both halves drift onto native layout. The operation id comes from system
entropy, never a clock or a pid: those collide exactly when several nodes are
started by one script.
Startup
In
block_chain::start(), immediately after the database opens and beforeUTXO-Z, the block store or any height is believed. A node that only refused once
it reached the UTXO build would already have served reads off a set that may be
half-applied.
"Could not read" is never reported as "clean." Only the absent key is clean.
The durable order, on both paths
Steps 10–11 are what close the window: there is no instant where the height says
"arrived" and the record says "clean" separately. The reorganization runs the same
sequence —
switch_to_branchowns steps 2–11 around its disconnect loop, sincethat is where its mutations are — and
disconnect_block's two height writes becomeone transaction, which previously left an instant where the stored-block height
named one block and the built height another, with neither wrong on its own.
The barriers, which did not exist
MDB_NOSYNC, so a commit returns before anything reaches theplatter.
env_sync()forces it, reports, and cannot be discarded;close()stops discarding its own — a failure there means transactions this run called
committed were lost, and a silent shutdown leaves the next start to find it as
corruption with no explanation.
block_store::flushreturned void, discarded both resultsit got, covered only the last block file, and had no caller anywhere in the
repo. A batch crossing a rotation writes into more than one
rev*.dat.flush_undotakes the set, normalizes it (one number per block arrives, soduplicates are normal), stops at the first failure, and carries which file
failed in the error rather than in a log line.
exist if the entry naming it was never written. Runs unconditionally: tracking
which rev file is new would save one fsync per batch, and a barrier skipped
because the tracking was wrong costs a database. Where the platform has none,
that is reported rather than answered as success.
sync()answeredtrue/false, foldingsync_unsupportedintosync_failed. Only the second is a defect and only the first may be carried onpast, so the wrapper reports which of the three happened.
KTH's guarantee is the weakest of the four, computed rather than configured,
and reported as KTH's own rather than borrowed from whichever store was asked
last.
durability_leveldescribes what a platform can promise; it never turns abarrier that was attempted and refused into a success — a real failure is fatal
on every platform.
Retry, recovery, and the indeterminate case
written changed nothing. UTXO-Z's
sync()has the same property — a failuredischarges nothing, so calling again attempts every barrier it owed.
means an explicit rebuild. A rollback-from-undo design was considered and
parked: it would depend on the store's inverse delta being idempotent, which was
never verified.
nothing reopens the gate and nothing clears the record; the node winds down and
the next start decides. Reopening is earned by a successful publication.
Tests
The record (
utxo_transition_record.cpp, 13 cases): round trip, byte-for-byteencoding, envelope position, every prefix truncation, every single-byte
corruption, future version, unknown type, unknown state, right version at the
wrong length, checksum-before-version, id uniqueness over 2000 draws.
The barriers (
undo_barriers.cpp, 9 cases): one file, a thousand duplicates,which file failed, that it does not stop at the last file, that it stops at the
first failure, the empty set, the directory barrier reported rather than assumed,
a directory that cannot be opened failing rather than passing, and the node's
combined level never exceeding what UTXO-Z reports for the same machine.
The lifecycle (
transition_lifecycle.cpp, 9 cases): a fresh database isclean; a record survives a real close/reopen; both kinds reach the refusal; a
record this build cannot decode refuses rather than passes; publishing clears and
the node comes back up; the refusal names type, id and range;
set_heightsleavesthe record alone.
Crash boundaries (
utxo_batch_atomicity.cpp, 12 cases). Each stages the stateits boundary leaves and then restarts for real — the chain is destroyed, LMDB and
UTXO-Z closed, a new chain opened on the same directory:
Plus: a batch driven through the real connect path is made to fail mid-mutation
(a pre-inserted duplicate key) and must leave its record behind; a completed batch
must leave none; a reorganization records and clears itself; a reorganization left
in flight refuses; and a restart after a real spend neither loses nor duplicates —
counted against
utxo_size(), not sampled, because a duplicate insert or adeletion that never ran moves the number and neither shows up in a spot check.
Each was checked against its own removal, not merely against passing:
What no test here can show: that an fsync happened. A barrier that did not run
is invisible to a process that exits cleanly — only a power cut tells the
difference — and removing step 7 leaves the suite green. Those calls are covered
for what they do and report, not for being called.
Verified
kth_blockchain_testandkth_node_testgreen in both storage modes: full(4000 assertions / 259 cases; 4150 / 181) and
utxoz_reference=True(4011 / 258;4150 / 181), against UTXO-Z 0.9.1.
Closes #600.
Summary by CodeRabbit
New Features
Bug Fixes
Tests