test: exercise the reorg undo round-trip against a real UTXO-Z database - #574
Conversation
|
Warning Review limit reached
Next review available in: 51 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 (5)
📝 WalkthroughWalkthroughThe change documents standalone UTXO-Z compact-mode configuration and adds database-backed blockchain tests for undo restoration, same-block cancellation, payload preservation, creation heights, and unknown outpoints. ChangesUTXO-Z undo validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/blockchain/test/reorg_undo_roundtrip.cpp`:
- Around line 93-124: Replace the manually constructed undo capture, delete map,
and restore logic around the test’s spend/disconnect flow with the production
block-delta path using process_compact_block_utxos and capture_block_undo. Build
the delta from the block, capture its undo record before applying it, then apply
that record to restore the database, including the internal-output case covered
by the later setup. Ensure the test exercises production handling of spending
heights, deferred lookups, and same-block output cancellation.
- Line 164: Update the `find_raw` assertion in the reorg undo round-trip test to
verify that the unknown outpoint returns `result_code::key_not_found`, rather
than only checking that no value is present. Preserve the existing outpoint and
lookup arguments while asserting the documented miss result code.
🪄 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: 42a423e4-6ccf-4bff-a2ad-0df1c36d68b6
📒 Files selected for processing (3)
src/blockchain/CMakeLists.txtsrc/blockchain/test/reorg_undo_roundtrip.cppsrc/database/CMakeLists.txt
The undo record's correctness claim — that a spent output can be put back exactly as it was — had never been executed. This covers it: - capture -> spend -> restore over three UTXOs created at DIFFERENT heights, asserting each comes back with its ORIGINAL creation height rather than the height of the block that spent it. Restoring the wrong height would corrupt coinbase maturity and, in compact mode, the median-time-past window used when the UTXO is later resolved — silently, and only for outputs a reorg touched. The height spread is deliberate: a restore that reused one height for all of them would still pass a single-entry test. - an output created and spent inside one block produces no undo entry, since it did not exist before the block and must not be restored. - find_raw reports a miss for an unknown outpoint, so capture fails loudly instead of writing an empty record. Run in both storage modes (compact and full); green in each. Also aligns the KTH_UTXOZ_COMPACT_MODE option default, which disagreed between modules (database ON, blockchain OFF) while the real value comes from Conan (utxoz_compact=False). Both branch on the same macro for the stored value layout, so a standalone build could have compiled the write path and the storage layer against different formats.
92dad0c to
d76d98a
Compare
First real execution of the reorg storage path. Everything merged so far (#571 undo/disconnect, #572 active chain, #573 chain switch) compiled and passed unit tests, but the undo record's central claim — a spent output can be put back exactly as it was — had never actually run.
What it covers
find_rawreports a miss for an unknown outpoint, so capture fails loudly instead of writing an empty record.Runs against a real
utxoz_databaseon a temp directory, which is the layerdisconnect_blockdepends on.Both storage modes
Green in compact (1581 assertions / 165 cases) and full (1606 / 166 — the extra case is the full-mode-only
utxoz_roundtripregression).Worth flagging: I had been asserting compact was the default. It is not —
conanfile.pysetsutxoz_compact=False, so normal builds are full mode and the compact path had never been exercised at all. It has now.Config fix
KTH_UTXOZ_COMPACT_MODEdisagreed between modules —databasedefaulted ON,blockchainOFF — while the real value comes from Conan. Both branch on that macro for the stored value layout, so a standalone build could compile the write path and the storage layer against different formats. Aligned to Conan's value, with a comment saying where the real default lives.Still not covered
This is the storage layer only. A full reorg (disconnect → switch → re-download) still has no end-to-end test — that needs a harness able to build competing chains and drive the sync pipeline, which is the next step.
Summary by CodeRabbit
Bug Fixes
Documentation