deps: move to UTXO-Z 0.9.1, and to the contracts it changed underneath - #645
Conversation
|
Warning Review limit reached
Next review available in: 49 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR renames UTXO-Z compact mode to reference mode across configuration, APIs, storage adapters, blockchain processing, and diagnostics. It adds explicit error, durability, locking, reference-resolution, and contract-test handling. ChangesUTXO-Z reference-mode migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant block_tasks
participant block_chain
participant utxoz_database
participant reference_db
participant block_store
block_tasks->>block_chain: build block UTXO delta
block_chain->>utxoz_database: process reference UTXOs
utxoz_database->>reference_db: resolve deferred lookups
reference_db-->>utxoz_database: entries or result_code
utxoz_database->>block_store: read referenced transaction
block_store-->>utxoz_database: transaction data
utxoz_database-->>block_chain: expected UTXO delta
block_chain-->>block_tasks: apply delta or stop on error
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/blockchain/include/kth/blockchain/utxo_builder.hpp`:
- Around line 60-65: Update the Doxygen title immediately preceding
reference_utxo_ref to use “reference” terminology instead of the stale “Compact
UTXO reference” wording, leaving the struct and its documentation unchanged
otherwise.
In `@src/blockchain/src/utxo_builder.cpp`:
- Around line 248-254: Replace the debug-only KTH_ASSERT in the
KTH_UTXOZ_REFERENCE_MODE serialization path with release-safe handling for
file_number < 0: return the surrounding operation’s error, or use KTH_CONTRACT
if this invariant must terminate execution. Ensure reference_utxo_ref is never
constructed with a cast of -1 to UINT32_MAX.
In `@src/blockchain/test/utxoz_contract.cpp`:
- Around line 43-51: Replace the self-referential static_assert in the test
setup with a runtime REQUIRE that compares utxoz_reference_mode() against the
expected reference/full mode selected for the test. Keep the expected mode
derived from the local build configuration, but use the database target’s
utxoz_reference_mode() result to detect divergent definitions.
- Line 24: Update the process-ID usage in the test to be platform-safe: include
<process.h> and call _getpid() under _WIN32, while retaining <unistd.h> and
getpid() for non-Windows builds. Keep the existing PID-dependent test behavior
unchanged.
In `@src/database/src/databases/utxoz_database.cpp`:
- Around line 178-183: Update src/database/src/databases/utxoz_database.cpp at
lines 178-183, 260-269, 315-321, 339-361, and 367-377: preserve UTXO-Z operation
failures instead of converting them to missing-key or empty results. In the
direct lookup path, map only utxoz::error_code::not_found to
result_code::key_not_found and map other errors to result_code::other; return
deferred raw-lookup and deletion drain errors, and keep queue-drain errors
distinct from reference-resolution or genuinely empty results. Change the queue
APIs, wrappers, and callers to return and propagate result_code, stopping each
affected operation on failure.
- Around line 89-93: Update the UTXO-Z open calls in the
KTH_UTXOZ_REFERENCE_MODE branches to preserve the original filesystem path
instead of converting it with path.string(). Use the available UTF-8-preserving
or wide-path overload for both utxoz::reference_db::open and
utxoz::full_db::open, keeping remove_existing 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: 9910b04b-3dfe-4247-ad60-102f83b520b2
⛔ Files ignored due to path filters (3)
conan.lockis excluded by!**/*.lock,!conan.lockand included by noneconanfile.pyis excluded by!**/conanfile.pyand included by nonedocs/utxo-store-api-requirements.mdis excluded by none and included by none
📒 Files selected for processing (17)
src/blockchain/CMakeLists.txtsrc/blockchain/include/kth/blockchain/interface/block_chain.hppsrc/blockchain/include/kth/blockchain/utxo_builder.hppsrc/blockchain/src/interface/block_chain.cppsrc/blockchain/src/utxo_builder.cppsrc/blockchain/test/block_undo.cppsrc/blockchain/test/reorg_undo_roundtrip.cppsrc/blockchain/test/utxoz_contract.cppsrc/blockchain/test/utxoz_roundtrip.cppsrc/c-api/include/kth/capi/node_info.hsrc/c-api/src/node_info.cppsrc/database/CMakeLists.txtsrc/database/include/kth/database/block_undo.hppsrc/database/include/kth/database/databases/utxoz_database.hppsrc/database/include/kth/database/define.hppsrc/database/src/databases/utxoz_database.cppsrc/node/src/executor/executor.cpp
| #ifdef KTH_UTXOZ_REFERENCE_MODE | ||
| auto result = utxoz::reference_db::open(path.string(), remove_existing); | ||
| #else | ||
| auto result = utxoz::full_db::open(path.string(), remove_existing); | ||
| #endif |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
In UTXO-Z version 0.9.0, what argument types does utxoz::reference_db::open accept, and what string encoding does it require for Windows paths?
💡 Result:
In the UTXO-Z C++ library (repository utxo-z/utxo-z), the open method for its database classes (such as utxoz::db::open and utxoz::compact_db::open) accepts two arguments: a path (typically a string or string literal) and a remove_existing boolean flag [1]. For Windows paths, while the library's examples demonstrate the use of standard narrow string literals (e.g., "./utxo_data") [1], Windows file APIs natively utilize UTF-16 encoding [2][3]. To ensure compatibility with non-ASCII characters in paths on Windows, programs interacting with the filesystem typically require the use of wide-character strings (UTF-16) and corresponding Win32 APIs (often denoted by a 'W' suffix) [4][3][5]. The library's open method, as shown in the source, takes a standard C++ string-like argument [1]; if the underlying implementation uses standard narrow C library calls, it may be subject to the system's current code page, which can fail for non-ASCII paths unless specifically configured [4][5].
Citations:
- 1: https://github.com/utxo-z/utxo-z
- 2: https://chrisdenton.github.io/omnipath/Overview.html
- 3: https://runebook.dev/en/docs/sqlite/c3ref/filename
- 4: https://stackoverflow.com/questions/79672383/windows-c-opening-a-file-where-path-contains-a-non-ascii-character-fails
- 5: fix: use UTF-16 Win32 APIs so non-ASCII paths open on Windows halgari/duckdb-level-pivot#1
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find KTH_UTXOZ_REFERENCE_MODE and database open paths"
rg -n "KTH_UTXOZ_REFERENCE_MODE|reference_db::open|full_db::open|std::filesystem::path|string\\(\\)" -S .
echo
echo "Inspect target file around open call"
sed -n '60,110p' src/database/src/databases/utxoz_database.cpp
echo
echo "Locate utxo-z header/reference_db definitions in vendored/source if present"
git ls-files | rg -i "utxo.?z|utxo-z|reference_db|full_db" || trueRepository: k-nuth/kth
Length of output: 50366
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect native_file helper and nearby path utility"
sed -n '1,80p' src/database/include/kth/database/native_file.hpp
rg -n "wstring\\(\\)|open_native|_W|SetConsole|ActiveCodePage|UTF-8|char_code|cp65001|u8string\\(\\)" src/database src/infrastructure -S --max-count 200 || true
echo
echo "Inspect utxoz_database.cpp header implementation"
sed -n '1,170p' src/database/include/kth/database/databases/utxoz_database.hpp
sed -n '1,160p' src/database/src/databases/utxoz_database.cpp
echo
echo "Check dependency/source references for utxo-z string handling"
git ls-files | rg -i "utxo.?z" | sed -n '1,120p'
rg -n "reference_db::open|class reference_db|namespace reference_db|open\\(.*string|u8string|wstring|utf8|utf-16|UTF-8|UTF-16" lib deps external third_party -S 2>/dev/null || trueRepository: k-nuth/kth
Length of output: 24595
🌐 Web query:
UTXO-Z C++ header reference_db open overload string literal wide string u8string Windows
💡 Result:
The UTXO-Z library provides a C++ database interface, typically accessed via the utxoz::db (aliased to utxoz::full_db) or utxoz::compact_db namespaces [1]. These databases are opened using static open methods, such as utxoz::db::open(path, remove_existing) [1]. Regarding the specific technical constraints mentioned: 1. Overloads and String Literals: The open method accepts a path argument [1]. In standard C++ practice, passing a string literal (which has type const char[N]) to a function accepting std::string or a similar string type usually works through implicit conversion, though it may sometimes be ambiguous depending on other overloads [2][3]. If you encounter ambiguity, explicitly constructing the string (e.g., std::string("path")) is the recommended fix [2][3]. 2. Windows, Wide Strings, and UTF-8: The UTXO-Z library uses standard C++ types for its API [1]. On Windows, if your filesystem paths require support for non-ASCII characters, ensure you are providing paths in the format expected by the underlying filesystem library (typically std::filesystem::path). std::filesystem::path natively supports both narrow (UTF-8/ANSI) and wide (UTF-16) string types, allowing you to pass narrow string literals, std::u8string (C++20), or wide strings (std::wstring) directly to the open method if the library implementation leverages std::filesystem internally. 3. Official Documentation: For the most accurate and up-to-date information on the UTXO-Z API, including any recent additions or changes to supported string types, refer to the official repository at github.com/utxo-z/utxo-z [1][4]. The library does not natively implement custom "u8string" overloads unless explicitly defined in its current header files; you should rely on standard library interop through std::filesystem::path where possible [1].
Citations:
- 1: https://github.com/utxo-z/utxo-z
- 2: https://exchangetuts.com/c-literal-string-and-const-string-reference-argument-duplicate-1640592004230127
- 3: https://www.daniweb.com/programming/software-development/threads/388172/how-could-i-pass-literal-as-rvalue-reference
- 4: https://github.com/utxo-z/utxo-z/releases/tag/v0.8.0
🌐 Web query:
github utxo-z utxoz.hpp reference_db open implementation string literal
💡 Result:
The project utxo-z is a high-performance UTXO database designed for Bitcoin Cash, featuring an implementation of a hash table with open addressing to maximize cache locality [1][2]. In the library, the header file utxoz.hpp is the primary interface for interacting with the database [1]. The open method, used for opening the database, accepts a string literal (or std::string) as a path argument [1]. For example, to open a database, you can use: #include <utxoz/utxoz.hpp> // Open database — returns result<full_db> auto r = utxoz::db::open("./utxo_data", true); The implementation utilizes this design to allow for efficient storage and retrieval [1][2]. As of August 2026, the project is under active development, with version 0.8.0 having been released in June 2026 [3].
Citations:
- 1: https://github.com/utxo-z/utxo-z
- 2: https://github.com/utxo-z/utxo-z-docs
- 3: https://github.com/utxo-z/utxo-z/releases/tag/v0.8.0
Use a path-preserving overload for UTXO-Z open.
path.string() narrows the std::filesystem::path through the current code page on Windows before both utxoz::reference_db::open and utxoz::full_db::open receive it, so non-ASCII database directories can fail or open the wrong location. Forward a UTF-8 preserved input or a wide-path overload if UTXO-Z exposes one.
🤖 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 89 - 93, Update
the UTXO-Z open calls in the KTH_UTXOZ_REFERENCE_MODE branches to preserve the
original filesystem path instead of converting it with path.string(). Use the
available UTF-8-preserving or wide-path overload for both
utxoz::reference_db::open and utxoz::full_db::open, keeping remove_existing
unchanged.
Source: Learnings
d057bd2 to
deb63f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/database/include/kth/database/databases/utxoz_database.hpp (2)
222-227: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate the reference payload length before copying it.
apply_delta_raw()currently assumes insert payloads are 8 bytes in reference mode. Copies at lines 225-226 read beyondraw.datawhen the insert has fewer bytes. Reject any insert whose payload size is not exactly 8 bytes before these copies and return the insert error instead of writing an invalid reference.🤖 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 222 - 227, In apply_delta_raw() under KTH_UTXOZ_REFERENCE_MODE, validate that the insert payload is exactly 8 bytes before the memcpy calls populating file_number and tx_offset. For any other size, reject the insert and return the existing insert error without copying or writing the reference.
178-208: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate
db_->erase()failures from both batch methods.Both
apply_delta()andapply_delta_raw()discard the wrapped erase result before returningresult_code::success. Iferase()reports a failure, return that failure instead so callers do not treat the batch as committed.
src/database/include/kth/database/databases/utxoz_database.hpp#L201-L203: Store and handledb_->erase(key, height)inapply_delta().src/database/include/kth/database/databases/utxoz_database.hpp#L243-L244: Store and handledb_->erase(key, height)inapply_delta_raw().🤖 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 178 - 208, Propagate erase failures in both batch methods: update apply_delta() at src/database/include/kth/database/databases/utxoz_database.hpp lines 178-208 and apply_delta_raw() at lines 222-245 to store the result of db_->erase(key, height), return its failure as the appropriate result_code, and only return success when every erase succeeds.src/blockchain/src/utxo_builder.cpp (1)
921-937: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winReplace debug-only header-index assertions with runtime validation.
Line 925 and Line 926 run before
process_compact_block_utxos()at Line 930. In Release builds,KTH_ASSERTdoes not run. Ifactive_at()returnsnull_index, the following header-index calls can access an invalid index. Returndatabase::result_code::otherafter an explicit null-index and height check.Proposed fix
auto const idx = chain.headers().active_at(static_cast<int32_t>(h)); -KTH_ASSERT(idx != header_index::null_index); -KTH_ASSERT(chain.headers().get_height(idx) == static_cast<int32_t>(h)); +if (idx == header_index::null_index || + chain.headers().get_height(idx) != static_cast<int32_t>(h)) { + spdlog::critical("[utxo_builder] Header index has no active block at height {}", h); + co_return database::result_code::other; +} auto const file_num = chain.headers().get_file_number(idx);Based on learnings:
KTH_ASSERTis a debug-only contract check and must not guard a Release-build condition that can produce invalid behavior.🤖 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/utxo_builder.cpp` around lines 921 - 937, Replace the KTH_ASSERT checks after active_at() in the reference-mode block with runtime validation: return database::result_code::other when idx is header_index::null_index or get_height(idx) does not equal h. Only call get_file_number(), get_data_pos(), and process_compact_block_utxos() after both checks pass.Source: Learnings
🤖 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 2199-2208: The block processing flow around
utxo_process_pending_deletions and set_utxo_built_height must not persist
batch_end before deferred deletions succeed. Move or defer the built-height
marker update until after the sweep completes successfully, and ensure failures
leave durable recovery state that prevents a restart from treating the
inconsistent batch as built.
In `@src/node/test/sync_harness.hpp`:
- Around line 175-182: Update the direct get_utxo() error handling in the helper
containing utxo_process_pending_lookups so it throws for every error except
database::result_code::key_not_found. Preserve the deferred lookup sweep for
key_not_found, while propagating other results such as
database::result_code::other instead of treating them as absent.
---
Outside diff comments:
In `@src/blockchain/src/utxo_builder.cpp`:
- Around line 921-937: Replace the KTH_ASSERT checks after active_at() in the
reference-mode block with runtime validation: return
database::result_code::other when idx is header_index::null_index or
get_height(idx) does not equal h. Only call get_file_number(), get_data_pos(),
and process_compact_block_utxos() after both checks pass.
In `@src/database/include/kth/database/databases/utxoz_database.hpp`:
- Around line 222-227: In apply_delta_raw() under KTH_UTXOZ_REFERENCE_MODE,
validate that the insert payload is exactly 8 bytes before the memcpy calls
populating file_number and tx_offset. For any other size, reject the insert and
return the existing insert error without copying or writing the reference.
- Around line 178-208: Propagate erase failures in both batch methods: update
apply_delta() at src/database/include/kth/database/databases/utxoz_database.hpp
lines 178-208 and apply_delta_raw() at lines 222-245 to store the result of
db_->erase(key, height), return its failure as the appropriate result_code, and
only return success when every erase succeeds.
🪄 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: 726e2718-0945-4eca-bfb2-a460597f28c4
📒 Files selected for processing (12)
src/blockchain/include/kth/blockchain/interface/block_chain.hppsrc/blockchain/include/kth/blockchain/utxo_builder.hppsrc/blockchain/src/interface/block_chain.cppsrc/blockchain/src/utxo_builder.cppsrc/blockchain/src/validate/batch_validate.cppsrc/blockchain/test/reorg_undo_roundtrip.cppsrc/blockchain/test/utxoz_contract.cppsrc/blockchain/test/utxoz_roundtrip.cppsrc/database/include/kth/database/databases/utxoz_database.hppsrc/database/src/databases/utxoz_database.cppsrc/node/src/sync/block_tasks.cppsrc/node/test/sync_harness.hpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/blockchain/test/utxoz_contract.cpp
- src/blockchain/test/utxoz_roundtrip.cpp
- src/database/src/databases/utxoz_database.cpp
deb63f6 to
25e5edb
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #645 +/- ##
===========================================
+ Coverage 0.34% 80.40% +80.06%
===========================================
Files 239 289 +50
Lines 12621 14538 +1917
===========================================
+ Hits 43 11689 +11646
+ Misses 12578 2849 -9729 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
25e5edb to
aa00f3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/blockchain/test/utxoz_contract.cpp (3)
118-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the vacuous
is_integral_vassertion.Line 116 already pins
erase_resulttoutxoz::result<size_t>. Line 118 then asserts that this type is not integral, which the preceding assertion guarantees. The check can never fail independently, so it adds no coverage to a file whose purpose is to avoid checks that pass vacuously.♻️ Proposed removal
static_assert(std::is_same_v<erase_result, utxoz::result<size_t>>, "erase() no longer returns result<size_t>; revisit the failure-vs-absence split"); - static_assert( ! std::is_integral_v<erase_result>, - "erase() returns a bare count again: a failure would read as a missing key"); }🤖 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 118 - 119, Remove the redundant static_assert using std::is_integral_v<erase_result> near the existing erase_result type assertion, leaving the assertion that pins erase_result to utxoz::result<size_t> unchanged.
68-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the comment on the tautological
static_assert.The
static_assertat lines 70-76 selects both the alias and the expected type fromKTH_UTXOZ_REFERENCE_MODEas seen in this translation unit. It cannot detect a CMake disagreement. The comment at lines 176-182 states this correctly, but the comment at lines 68-69 still claims the opposite.The runtime check in the "this target and the database agree on the mode" test case is the one that detects divergence. Update the comment here to point at it, or remove the assertion.
♻️ Proposed comment fix
-// The mode this build selected must be the mode the wrapper reports. These are -// set in two CMake lists and read in a third place, so they can disagree. +// Pins the alias to the mode this translation unit was compiled with. This is a +// tautology by construction and cannot detect a CMake disagreement; the runtime +// utxoz_reference_mode() comparison in the "this target and the database agree +// on the mode" test case is what catches divergent target definitions. static_assert(🤖 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 68 - 76, Update the comment immediately above the static_assert in the UTXOZ mode test so it no longer claims to detect disagreement between CMake-selected modes; explain that the assertion only verifies the alias and expected type selected within this translation unit, and point readers to the runtime “this target and the database agree on the mode” check for cross-target divergence, or remove the assertion if that clarification is not useful.
223-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the wrapper return type, as the other test cases do.
Line 224 asserts the return type of the library function
utxoz::platform_durability(). Line 230 then callsutxoz_platform_durability(), which is the KTH wrapper. The wrapper return type is never asserted, so this test case does not apply the pattern used at lines 219-220 forsync().Add a
static_assertfor the wrapper.♻️ Proposed addition
static_assert(std::is_same_v<decltype(utxoz::platform_durability()), utxoz::durability_level>, "platform_durability() no longer answers with a durability_level"); + + static_assert(std::is_same_v<decltype(utxoz_platform_durability()), utxoz::durability_level>, + "the wrapper must pass the durability_level up unchanged");🤖 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 223 - 234, Add a static_assert beside the existing utxoz::platform_durability() assertion to pin the return type of utxoz_platform_durability() to utxoz::durability_level, matching the wrapper-type checks used by the neighboring sync() tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/blockchain/test/utxoz_contract.cpp`:
- Around line 378-383: Update the test around utxoz_database::find so reference
mode explicitly requires the lookup to fail, preventing fabricated entries from
passing; retain the existing conditional error-code check for full mode,
including the assertion that failures are not result_code::key_not_found.
- Around line 262-281: Test cleanup is skipped when Catch2 REQUIRE assertions
throw, leaving database handles and temporary directories behind. In
src/blockchain/test/utxoz_contract.cpp#L262-L281, add the anonymous-namespace
scope_exit helper and guard both first->close()—while preserving its required
ordering—and remove_all(dir); apply equivalent guards for db.close() and
remove_all(dir) at src/blockchain/test/utxoz_contract.cpp#L332-L341 and
src/blockchain/test/utxoz_contract.cpp#L352-L392. Ensure each guard executes
during normal completion and assertion unwinding.
---
Nitpick comments:
In `@src/blockchain/test/utxoz_contract.cpp`:
- Around line 118-119: Remove the redundant static_assert using
std::is_integral_v<erase_result> near the existing erase_result type assertion,
leaving the assertion that pins erase_result to utxoz::result<size_t> unchanged.
- Around line 68-76: Update the comment immediately above the static_assert in
the UTXOZ mode test so it no longer claims to detect disagreement between
CMake-selected modes; explain that the assertion only verifies the alias and
expected type selected within this translation unit, and point readers to the
runtime “this target and the database agree on the mode” check for cross-target
divergence, or remove the assertion if that clarification is not useful.
- Around line 223-234: Add a static_assert beside the existing
utxoz::platform_durability() assertion to pin the return type of
utxoz_platform_durability() to utxoz::durability_level, matching the
wrapper-type checks used by the neighboring sync() tests.
🪄 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: 55e38862-260d-4444-b669-f3c5ec6cdd75
⛔ Files ignored due to path filters (2)
conan.lockis excluded by!**/*.lock,!conan.lockand included by noneconanfile.pyis excluded by!**/conanfile.pyand included by none
📒 Files selected for processing (6)
src/blockchain/include/kth/blockchain/populate/populate_base.hppsrc/blockchain/src/populate/populate_base.cppsrc/blockchain/src/populate/populate_transaction.cppsrc/blockchain/test/utxoz_contract.cppsrc/database/include/kth/database/databases/utxoz_database.hppsrc/database/src/databases/utxoz_database.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/database/include/kth/database/databases/utxoz_database.hpp
- src/database/src/databases/utxoz_database.cpp
| auto first = utxoz_db::open_for_testing(dir.string(), true); | ||
| REQUIRE(first.has_value()); | ||
|
|
||
| // 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.string(), false); | ||
| REQUIRE_FALSE(second.has_value()); | ||
| REQUIRE(second.error() == utxoz::error_code::database_in_use); | ||
|
|
||
| first->close(); | ||
|
|
||
| // ...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.string(), false); | ||
| REQUIRE(third.has_value()); | ||
| third->close(); | ||
|
|
||
| std::filesystem::remove_all(dir); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Cleanup is skipped when an assertion fails in three test cases. Each test case writes its cleanup as trailing statements after the REQUIRE calls. Catch2 throws on a failed REQUIRE, so the trailing statements never run. The result is a held database handle and a leaked temporary directory, which can make later failures harder to read.
Introduce one RAII scope guard in the anonymous namespace and use it at each site.
src/blockchain/test/utxoz_contract.cpp#L262-L281: guardfirst->close()andremove_all(dir). This site leaks the exclusive database claim, which blocks any later open of the same path in the same process.src/blockchain/test/utxoz_contract.cpp#L332-L341: guarddb.close()andremove_all(dir)around the open-and-empty lookup checks.src/blockchain/test/utxoz_contract.cpp#L352-L392: guarddb.close()andremove_all(dir)around the materialisation checks.
🧪 Proposed scope guard
Add the helper to the anonymous namespace:
// Cleanup must run on the throwing path too: a failed Catch2 REQUIRE unwinds,
// and a skipped close() leaves the exclusive claim held for the whole process.
class scope_exit {
public:
explicit scope_exit(std::function<void()> f) : f_(std::move(f)) {}
~scope_exit() { if (f_) f_(); }
scope_exit(scope_exit const&) = delete;
scope_exit& operator=(scope_exit const&) = delete;
private:
std::function<void()> f_;
};Then apply it at the anchor site:
auto first = utxoz_db::open_for_testing(dir.string(), true);
REQUIRE(first.has_value());
+ scope_exit cleanup{[&]{
+ std::error_code ignored;
+ std::filesystem::remove_all(dir, ignored);
+ }};
auto second = utxoz_db::open_for_testing(dir.string(), false);
REQUIRE_FALSE(second.has_value());
REQUIRE(second.error() == utxoz::error_code::database_in_use);
first->close();
@@
auto third = utxoz_db::open_for_testing(dir.string(), false);
REQUIRE(third.has_value());
third->close();
-
- std::filesystem::remove_all(dir);
}Note that first->close() must stay inside the body because the test depends on its ordering, so also guard it, for example with a separate flag or by closing in the guard when still open.
📍 Affects 1 file
src/blockchain/test/utxoz_contract.cpp#L262-L281(this comment)src/blockchain/test/utxoz_contract.cpp#L332-L341src/blockchain/test/utxoz_contract.cpp#L352-L392
🤖 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 262 - 281, Test cleanup
is skipped when Catch2 REQUIRE assertions throw, leaving database handles and
temporary directories behind. In
src/blockchain/test/utxoz_contract.cpp#L262-L281, add the anonymous-namespace
scope_exit helper and guard both first->close()—while preserving its required
ordering—and remove_all(dir); apply equivalent guards for db.close() and
remove_all(dir) at src/blockchain/test/utxoz_contract.cpp#L332-L341 and
src/blockchain/test/utxoz_contract.cpp#L352-L392. Ensure each guard executes
during normal completion and assertion unwinding.
aa00f3d to
ea60e79
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/blockchain/test/utxoz_contract.cpp (1)
397-405: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoth branches of this
#ifdefassert the same two conditions.Lines 398-399 and lines 403-404 are identical. The preprocessor branch adds no distinction here, only the comments differ.
Keep one pair of assertions and keep both explanations in the surrounding comment.
♻️ Proposed simplification
auto const point = utxoz_database::key_to_point(key); auto const found = db.find(point, 0); -#ifdef KTH_UTXOZ_REFERENCE_MODE + // Reference mode: the reference points at a block file no store can serve. + // Full mode: the stored bytes are not a serialised entry, so decoding fails. + // Neither outcome may be reported as the key being absent. REQUIRE_FALSE(found.has_value()); REQUIRE(found.error() != result_code::key_not_found); -#else - // Full mode: the stored bytes are not a serialised entry, so decoding must - // fail — and, again, must not be reported as the key being absent. - REQUIRE_FALSE(found.has_value()); - REQUIRE(found.error() != result_code::key_not_found); -#endif🤖 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 397 - 405, Remove the redundant KTH_UTXOZ_REFERENCE_MODE conditional around the identical REQUIRE assertions, retaining a single pair of assertions and combining both reference-mode and full-mode explanations in the surrounding comment.src/node/src/sync/block_tasks.cpp (1)
2168-2187: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport the unapplied count from the drain result, not the pre-drain size.
deferredis read before the drain. On a failed drain the queue may be partially applied, sodeferredoverstates what remains. The message then reports a count that the store does not hold. Alsodeletedis bound but never used.Use
chain.utxo_deferred_deletions_size()after the failure for the remaining count, and bind onlyfailed.♻️ Proposed refactor
auto drained = chain.utxo_process_pending_deletions(); if ( ! drained) { spdlog::critical("[utxo_build] the deferred deletion queue could not be " "drained at batch {}-{}: {} deletion(s) remain unapplied, so the UTXO " "set still holds outputs these blocks spent", batch_start, batch_end, - deferred); + chain.utxo_deferred_deletions_size()); on_fatal("the deferred deletion queue could not be drained"); co_return; } - auto& [deleted, failed] = *drained; + auto const& failed = drained->second;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/node/src/sync/block_tasks.cpp` around lines 2168 - 2187, In the deferred deletion failure path around utxo_process_pending_deletions, report the remaining queue size by calling chain.utxo_deferred_deletions_size() after the drain fails instead of using the pre-drain deferred value. Also bind only failed from the drain result, removing the unused deleted binding.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/blockchain/test/utxoz_contract.cpp`:
- Around line 397-405: Remove the redundant KTH_UTXOZ_REFERENCE_MODE conditional
around the identical REQUIRE assertions, retaining a single pair of assertions
and combining both reference-mode and full-mode explanations in the surrounding
comment.
In `@src/node/src/sync/block_tasks.cpp`:
- Around line 2168-2187: In the deferred deletion failure path around
utxo_process_pending_deletions, report the remaining queue size by calling
chain.utxo_deferred_deletions_size() after the drain fails instead of using the
pre-drain deferred value. Also bind only failed from the drain result, removing
the unused deleted binding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b43a49cf-74ef-44fe-85fd-a08a36328166
📒 Files selected for processing (5)
src/blockchain/src/utxo_builder.cppsrc/blockchain/test/utxoz_contract.cppsrc/database/include/kth/database/databases/utxoz_database.hppsrc/node/src/sync/block_tasks.cppsrc/node/test/sync_harness.hpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/node/test/sync_harness.hpp
- src/blockchain/src/utxo_builder.cpp
- src/database/include/kth/database/databases/utxoz_database.hpp
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.
ea60e79 to
a168ea2
Compare
1 participant
Moves Knuth from
utxoz/0.8.0toutxoz/0.9.0, withconan.lockregeneratedfrom scratch rather than patched.
The visible half of this bump is a rename. The half that matters is what four
functions now return.
The rename:
compact→reference, for the MODE onlycompact_dbreference_dbcompact_find_resultreference_find_resultstorage_mode::compactstorage_mode::referencecompact_file_size,compact_test_file_sizereference_*Knuth follows: option
utxoz_compact→utxoz_reference, macroKTH_UTXOZ_COMPACT_MODE→KTH_UTXOZ_REFERENCE_MODE,database::utxoz_compact_mode()→utxoz_reference_mode(), C-APIkth_node_utxoz_compact_mode→kth_node_utxoz_reference_mode, and theinternal
compact_utxo_ref/pack_compact_refthat name the reference-modepayload. 70 identifiers in all.
What deliberately keeps the word, because it is the compaction operation
or something else entirely:
utxoz_database::compact()and UTXO-Z'scompact_all();block_chain::utxo_compact(); BIP152compact_blockandsend_compact;history_compactandstealth_compact;chain::compact(thedifficulty-bits encoding);
CompactSizein the consensus serialisers;secp256k1's compact signature format; and
utxo_compact_block, the UTXObuilder's small parsed-block view, which is used in both modes and is not
the storage mode at all.
Each rename rule names an exact identifier rather than the substring
compact,so none of the above could be caught by accident.
The contracts that changed underneath
compact_all()voidresult<>process_pending_lookups()pair<…>result<pair<…>>process_pending_deletions()pair<…>result<pair<…>>erase()size_tresult<size_t>compact_all()is why this is not a version-number change: it used to returnvoid, so a caller that kept invoking it as a bare statement compiled exactlyas before while discarding every failure.
erase()is worth reading twice. It returned a count, and the natural migrationwrites
*erased > 0 ? success : key_not_found— which turns the erase failedinto 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 the caller upstream reads absence as spent. All three now
report failure as failure.
What is new rather than changed
sync(), andclose()does not do it for you. Wrapped asutxoz_database::sync().platform_durability()/durability_level, which say what a successfulsync is worth: under
contents_onlythe file contents reach the disk andthe directory entries naming them do not. Exposed as
utxoz_platform_durability()rather than assumed.open()now distinguishesdatabase_in_use(anotherinstance holds it) from
database_lock_unavailable(the claim could not beattempted at all — no permission, a filesystem without locking, a lock file
that is not a regular file). UTXO-Z's own header notes these send an operator
looking in different places, so they are logged as different things, alongside
recovery_required/recovery_failedand a storage-mode mismatch.the numbers moved, so an old note that said
error 3names a different faulttoday. The naming switch is exhaustive with no
default, so code 22 will stopthe build rather than print "unknown".
Negative controls
src/blockchain/test/utxoz_contract.cppasserts the shape of these contracts,mostly at compile time — a runtime test can only fail once the wrong call has
already been written and executed.
Proven non-vacuous by breaking it on purpose: reverting
compact()to its0.8.0 form fails the build with
and the restored tree builds clean again. The exclusive-claim test has both
halves — a second
open()is refused withdatabase_in_use, and the pathopens again once released, so a library that refused every open would not pass.
Verification
conan lock createfrom scratchutxoz/0.9.0#1641e774…, RC=0conan install(tests, rpc, C++23)kth_infrastructure_testkth_domain_testkth_blockchain_test(full mode)kth_blockchain_test(reference mode)kth_blockchain_vmlimits_testkth_network_testkth_node_testkth_capi_testFull mode remains the default — in the recipe and in both CMake lists.
Reference mode is built and tested here as a check: it is the half of the
#ifdefthat nothing in CI compiles, so a rename that broke it would gounnoticed until someone turned the flag on. Doing that found a real defect this
PR fixes: the executor still printed
UTXO-Z mode: compactfor reference mode,because the identifier was renamed and the string beside it was not.
#602is deliberately not included here.Summary by CodeRabbit
New Features
API Updates
Bug Fixes