Merge bitcoin/bitcoin#25667: assumeutxo: snapshot initialization #1221
Conversation
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WalkthroughThis pull request introduces UTXO snapshot support infrastructure, including new snapshot detection and loading utilities, chainstate initialization refactoring with cache sizing strategy, database wrapper enhancements for storage path tracking, ChainstateManager API additions for snapshot lifecycle management, and test infrastructure updates to support configurable in-memory databases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/txdb.h (1)
13-18: Expose underlying coins DB storage path for snapshot logicIncluding
<fs.h>and addingCCoinsViewDB::StoragePath()as a thin forwarder tom_db->StoragePath()is a minimal, snapshot‑driven change that matches upstream and enables locating the on‑disk chainstate directory. No Dash‑specific behavior is altered.You may consider marking
StoragePath()const(std::optional<fs::path> StoragePath() const) since it does not mutate state, but this is not required for correctness.Also applies to: 55-82
src/validation.h (1)
955-965: InitializeChainstate comment is slightly stale vs parametersThe function now takes
CEvoDB&andconst std::unique_ptr<CChainstateHelper>&but the comment only documentsmempoolandsnapshot_blockhash. Consider updating the docblock so future readers don’t have to infer the EvoDB / chain-helper roles from implementation.src/node/utxo_snapshot.h (1)
9-68: Snapshot header matches implementation; minor dependency coupling is acceptable
- Declarations for
SNAPSHOT_BLOCKHASH_FILENAME,WriteSnapshotBaseBlockhash,ReadSnapshotBaseBlockhash,SNAPSHOT_CHAINSTATE_SUFFIX, andFindSnapshotChainstateDir()all align with the implementations inutxo_snapshot.cppand correctly useCChainStatein place of Bitcoin’sChainstate.SnapshotMetadatafields and serialization (base blockhash + coins count) are consistent with how snapshot metadata is used elsewhere in assumeutxo logic.The extra
#include <validation.h>and redundantextern RecursiveMutex cs_main;aren’t strictly necessary (a forward declaration ofCChainStateplus including<sync.h>in the header and<validation.h>only in the .cpp would be a bit cleaner), but they don’t introduce correctness issues and are acceptable for a backport.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
doc/design/assumeutxo.md(1 hunks)src/Makefile.am(1 hunks)src/dbwrapper.cpp(1 hunks)src/dbwrapper.h(3 hunks)src/node/blockstorage.cpp(1 hunks)src/node/chainstate.cpp(4 hunks)src/node/utxo_snapshot.cpp(1 hunks)src/node/utxo_snapshot.h(2 hunks)src/streams.h(1 hunks)src/test/util/setup_common.cpp(4 hunks)src/test/util/setup_common.h(3 hunks)src/txdb.h(2 hunks)src/validation.cpp(5 hunks)src/validation.h(2 hunks)test/functional/feature_init.py(0 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
💤 Files with no reviewable changes (1)
- test/functional/feature_init.py
🧰 Additional context used
📓 Path-based instructions (4)
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/design/assumeutxo.md
**
⚙️ CodeRabbit configuration file
**: # CodeRabbit AI Review Instructions for Dash BackportsYour Role
You are reviewing Bitcoin Core backports to Dash Core. Your ONLY job is to validate that the Dash commit faithfully represents the original Bitcoin commit with minimal, necessary adaptations.
Critical Validation Rules
1. File Operations Must Match (AUTO-REJECT if violated)
- If Bitcoin modifies an existing file → Dash MUST modify (not create new)
- If Bitcoin creates a new file → Dash creates
- If Bitcoin deletes a file → Dash deletes
- Common failure: Bitcoin modifies keys.txt, Dash creates new file with 58 keys
2. Size Ratio Check (80-150% of Bitcoin)
- Count functional lines changed (exclude comments/whitespace)
- Dash changes should be 80-150% of Bitcoin's size
- Red flag: 2-line Bitcoin fix becoming 150+ lines in Dash
3. No Scope Creep
- Reject if you see: "TODO:", "FIXME:", "while we're here", "also fix"
- No unrelated refactoring or style changes
- Only Bitcoin's intended changes + minimal Dash adaptations
4. Bitcoin-Specific Code Detection
- Auto-reject witness/segwit code:
msg_wtxidrelay,MSG_WTX, witness imports- Auto-reject RBF (replace-by-fee) functionality
- Note: PSBT is supported in Dash (don't flag)
5. Mandatory Adaptations Only
bitcoin→dashin strings/pathsBitcoin→Dashin user-facing text- Port numbers: 8332→9998 (RPC), 8333→9999 (P2P)
- Hardcoded test values specific to Dash
- No other changes unless absolutely required
6. Completeness Check
- All files changed in Bitcoin must be present
- Extra files need clear justification (Dash-specific compatibility)
- Missing files = incomplete backport
Review Process
- First: Check file operations match exactly
- Second: Calculate size ratio
- Third: Scan for scope creep patterns
- Fourth: Detect Bitcoin-specific code
- Fifth: Verify all changes are minimal adaptations
Output Format
VALIDATION: [PASS/FAIL] File Operatio...
Files:
doc/design/assumeutxo.mdsrc/dbwrapper.cppsrc/test/util/setup_common.htest/lint/lint-circular-dependencies.pysrc/node/blockstorage.cppsrc/node/utxo_snapshot.cppsrc/Makefile.amsrc/node/utxo_snapshot.hsrc/txdb.hsrc/test/util/setup_common.cppsrc/dbwrapper.hsrc/node/chainstate.cppsrc/streams.hsrc/validation.cppsrc/validation.h
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
C++20 codebase should be placed under src/
Files:
src/dbwrapper.cppsrc/test/util/setup_common.hsrc/node/blockstorage.cppsrc/node/utxo_snapshot.cppsrc/node/utxo_snapshot.hsrc/txdb.hsrc/test/util/setup_common.cppsrc/dbwrapper.hsrc/node/chainstate.cppsrc/streams.hsrc/validation.cppsrc/validation.h
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/util/setup_common.hsrc/test/util/setup_common.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-31T01:14:55.631Z
Learning: DashCoreAutoGuix successfully completed a complex Bitcoin Core backport (PR #29412) for block mutation detection by implementing the IsBlockMutated function, adding net processing integration, creating comprehensive unit tests, and properly adapting all Bitcoin-specific witness code for Dash compatibility. The backport maintains full security functionality while respecting Dash's non-witness transaction architecture.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T20:34:29.061Z
Learning: During Dash backport verification of Bitcoin Core commit 06d469c26b, scope creep was detected when additional pruning test functionality was added to interface_usdt_utxocache.py beyond what was in the original Bitcoin commit. The fix involved removing the extra test block while maintaining the core compiler flag fixes for USDT compilation errors.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T22:03:12.364Z
Learning: During multiple verification attempts of Bitcoin Core commit 06d469c26b backport to Dash PR #566, DashCoreAutoGuix consistently identified scope creep in interface_usdt_utxocache.py where additional pruning test functionality was added beyond the original Bitcoin commit. The user provided comprehensive fixes including both scope creep removal and missing mempool test file additions, but couldn't push due to authentication restrictions. The scope creep fix was identified as the priority to resolve CI failures.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T19:54:21.426Z
Learning: In Dash backports from Bitcoin Core, including necessary compilation fixes (such as API compatibility changes like UniValue get_int() → getInt<int>()) alongside the core backport is standard and expected practice. These compatibility fixes ensure the backported code compiles in Dash's evolved codebase while preserving Bitcoin's original functionality and intent.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-29T21:29:32.827Z
Learning: DashCoreAutoGuix successfully fixed scope creep in Bitcoin Core commit fcdb39d3ee backport by removing the parse test case from src/test/uint256_tests.cpp that was not part of the original Bitcoin commit. The fix was implemented in commit 16748115ce and verified through range-diff analysis.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-30T14:45:15.700Z
Learning: In Dash backports from Bitcoin Core test files, the `address_to_scriptpubkey` function should be imported from `test_framework.wallet` instead of `test_framework.address` as the import location differs between Bitcoin and Dash test frameworks.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-29T22:13:54.797Z
Learning: In Dash backports from Bitcoin Core, witness transaction-related code (MSG_WTX, wtxid) should be replaced with regular transaction handling (MSG_TX, txid) for compatibility, as demonstrated in the p2p_filter.py test fix where MSG_WTX was replaced with MSG_TX and irr_wtxid usage was replaced with irr_txid.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-27T22:35:10.176Z
Learning: In Dash backports, src/dashbls files are vendored dependencies that should not be modified during Bitcoin Core backports unless there is specific justification. Unauthorized modifications to vendored dependencies should be removed to maintain code integrity.
📚 Learning: 2025-07-28T20:34:29.061Z
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T20:34:29.061Z
Learning: During Dash backport verification of Bitcoin Core commit 06d469c26b, scope creep was detected when additional pruning test functionality was added to interface_usdt_utxocache.py beyond what was in the original Bitcoin commit. The fix involved removing the extra test block while maintaining the core compiler flag fixes for USDT compilation errors.
Applied to files:
src/Makefile.amsrc/test/util/setup_common.cpp
📚 Learning: 2025-07-28T23:09:09.522Z
Learnt from: CR
Repo: DashCoreAutoGuix/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T23:09:09.522Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists use immutable data structures from the Immer library for thread safety
Applied to files:
src/node/utxo_snapshot.h
🧬 Code graph analysis (7)
src/node/utxo_snapshot.h (2)
src/node/utxo_snapshot.cpp (6)
WriteSnapshotBaseBlockhash(19-43)WriteSnapshotBaseBlockhash(19-19)ReadSnapshotBaseBlockhash(45-78)ReadSnapshotBaseBlockhash(45-45)FindSnapshotChainstateDir(80-89)FindSnapshotChainstateDir(80-80)src/node/chainstate.h (1)
fs(31-33)
src/txdb.h (1)
src/node/utxo_snapshot.h (1)
path(48-69)
src/test/util/setup_common.cpp (1)
src/test/util/setup_common.h (1)
m_block_tree_db_in_memory(124-134)
src/dbwrapper.h (2)
src/node/utxo_snapshot.h (1)
path(48-69)src/node/chainstate.h (1)
fs(31-33)
src/node/chainstate.cpp (1)
src/node/interfaces.cpp (3)
chainman(650-652)chainman(1135-1137)chainman(1205-1208)
src/validation.cpp (2)
src/node/utxo_snapshot.cpp (6)
WriteSnapshotBaseBlockhash(19-43)WriteSnapshotBaseBlockhash(19-19)FindSnapshotChainstateDir(80-89)FindSnapshotChainstateDir(80-80)ReadSnapshotBaseBlockhash(45-78)ReadSnapshotBaseBlockhash(45-45)src/node/utxo_snapshot.h (1)
path(48-69)
src/validation.h (2)
src/validation.cpp (7)
DetectSnapshotChainstate(6146-6164)DetectSnapshotChainstate(6146-6148)ResetChainstates(6115-6120)ResetChainstates(6115-6115)CChainState(1648-1660)ActivateExistingSnapshot(6166-6177)ActivateExistingSnapshot(6166-6169)src/evo/chainhelper.cpp (2)
CChainstateHelper(15-26)CChainstateHelper(28-28)
🔇 Additional comments (20)
test/lint/lint-circular-dependencies.py (1)
14-25: UTXO snapshot circular-dependency entry correctly backportedThe added
"node/utxo_snapshot -> validation -> node/utxo_snapshot"entry matches Bitcoin Core’s expected circular dependency for the assumeutxo snapshot module, with no extra changes or Dash-specific scope creep in this file. This is a 1:1 functional backport for this lint entry.src/streams.h (1)
510-518: The change toAutoFile::fclose()returningintinstead ofvoidis correctly part of the assumeutxo backport (bitcoin#25667) and is already merged in this repository (commit 18f9984). The implementation properly captures the return value from::fclose(), sets the file pointer to null, and returns the status—enabling callers to detect file close errors when invoking the method directly. The destructor correctly ignores the return value, maintaining proper RAII semantics. No issues detected.src/dbwrapper.cpp (1)
129-183: Initializer updated to track DB path and memory modeThe added
m_path{path}andm_is_memory{fMemory}initializers are consistent with the newCDBWrapper::StoragePath()API and do not alter existing LevelDB behavior. This matches the upstream assumeutxo change.src/Makefile.am (1)
469-573: Build wiring for utxo snapshot utilities matches upstreamAdding
node/utxo_snapshot.cpptolibbitcoin_node_a_SOURCESis exactly what’s needed to compile the new snapshot utility file and matches the upstream assumeutxo integration, without introducing extra scope.doc/design/assumeutxo.md (1)
77-81: Doc updated to match new snapshot directory naming and detectionThe assumeutxo design text now correctly reflects the
chainstate_snapshotdirectory and startup detection viaDetectSnapshotChainstate(), consistent with the upstream design and the code introduced in this backport.src/node/blockstorage.cpp (1)
557-577: Guard added to avoid accessing empty blockfile info during early flushThe new early return when
m_blockfile_infois empty, plus the postcondition assert, safely prevents out‑of‑bounds access inFlushBlockFileduring early initialization, matching the upstream assumeutxo change and supporting the new cache‑rebalance call sites.src/test/util/setup_common.h (1)
122-134: Test harness extended for configurable in‑memory DBsThe additional
m_coins_db_in_memory/m_block_tree_db_in_memoryflags,LoadVerifyActivateChainstate()helper, and the extended constructors with defaulted bools align with upstream snapshot tests and keep existing call sites source‑compatible. This is the minimal adaptation needed for assumeutxo testing in Dash.Also applies to: 146-153, 214-219
src/node/chainstate.cpp (1)
82-100: Chainstate load sequence and cache handling updated for snapshot supportThe reordered
LoadChainstate()logic:
- Initializes the fully validated chainstate and then invokes
DetectSnapshotChainstate()to create any snapshot‑based chainstate before touching the block index.- Sets
m_total_coinstip_cache/m_total_coinsdb_cacheonce and uses a conservativeinit_cache_fraction(0.2) when callingInitCoinsDBandInitCoinsCacheon each chainstate, avoiding over‑allocating caches during this temporary initialization phase.- Logs each chainstate as it’s initialized and, after all chainstates are consistent and flushable, calls
chainman.MaybeRebalanceCaches()to redistribute caches according to active/snapshot/background roles.This structure matches the upstream assumeutxo initialization design while preserving Dash‑specific wiring (EvoDB, LLMQ, governance, etc.) and does not introduce extra behavior beyond what’s needed for snapshot activation.
Also applies to: 168-173, 177-202, 230-234
src/dbwrapper.h (1)
54-57: This comment can be resolved. The header is already self-contained:dbwrapper.hincludes<streams.h>(line 14), which includes<optional>(line 19), makingstd::optionalavailable. The code will compile without issues. While explicitly including<optional>would be a reasonable defensive practice for clarity, it is not necessary to fix a compilation problem since one does not exist.src/validation.h (1)
1065-1079: Snapshot chainstate API declarations look consistent with Dash chainstate model
DetectSnapshotChainstate,ResetChainstates, andActivateExistingSnapshotmatch thevalidation.cppimplementations and correctly thread the Dash-specificCTxMemPool*,CEvoDB&, andCChainstateHelperparameters through the ChainstateManager, with appropriateEXCLUSIVE_LOCKS_REQUIRED(::cs_main)annotations. This is a faithful adaptation of Bitcoin’s snapshot lifecycle API to Dash’sCChainState/EvoDB setup.src/node/utxo_snapshot.cpp (3)
19-43: WriteSnapshotBaseBlockhash correctly persists the snapshot base blockhashThe function asserts
cs_mainis held, requiresm_from_snapshot_blockhashto be set, derives the on-disk directory fromCoinsDB().StoragePath(), and writes exactly one serializeduint256tobase_blockhash, with proper error checks on open and close. This matches the intended semantics for snapshot base-blockhash persistence while safely rejecting in-memory chainstates via theStoragePath()assert.
45-78: ReadSnapshotBaseBlockhash handles malformed/missing snapshot state robustlyThe implementation:
- Verifies the chainstate directory and
base_blockhashfile exist, logging and returningstd::nulloptotherwise.- Reads a single
uint256from the file viaAutoFile, and logs warnings (but still returns the hash) if there is trailing data or an i/o error after the main read.This behaviour matches the snapshot design—treating malformed or absent metadata as “no usable snapshot” while still being tolerant of benign extra data—so callers like
DetectSnapshotChainstatecan reliably gate snapshot activation.
80-89: FindSnapshotChainstateDir correctly locates the snapshot chainstate directoryUsing
gArgs.GetDataDirNet()plus the_snapshotsuffix to constructchainstate_snapshotand returning it only if it exists is consistent with how the snapshot chainstate directory is created and later detected during startup. There are no extra Bitcoin-specific behaviours, and this is an appropriate adaptation for Dash’s directory layout.src/validation.cpp (4)
1662-1674: Snapshot suffix inInitCoinsDBcorrectly aligns LevelDB dir naming with snapshot utilitiesAppending
node::SNAPSHOT_CHAINSTATE_SUFFIXwhenm_from_snapshot_blockhashis set makes the snapshot chainstate usechainstate_snapshotas its LevelDB directory, which matchesFindSnapshotChainstateDir()and theSNAPSHOT_CHAINSTATE_SUFFIXdesign innode/utxo_snapshot.{h,cpp}. This is the expected adaptation from Bitcoin’s snapshot init and looks correct.
5820-5850:ActivateSnapshotnow persists base blockhash and robustly cleans up failed snapshotsUsing
snapshot_okto combine the result ofPopulateAndValidateSnapshotwithWriteSnapshotBaseBlockhash(for non–in-memory snapshots) is sound: only fully loaded and successfully-persisted snapshots proceed.On failure, you:
- Rebalance caches via
MaybeRebalanceCaches(), undoing the temporary cache split.- Reset the temporary
snapshot_chainstatebefore callingDeleteCoinsDBFromDiskso the LevelDB handle is released.- Abort the node if the snapshot chainstate dir cannot be removed, avoiding repeated confusing startup states.
This matches the snapshot initialization/cleanup behavior from Bitcoin’s assumeutxo code and looks correct for Dash’s
CChainState/CEvoDBsetup.
5719-6177: Backport validation summary forsrc/validation.cpp(assumeutxo snapshot init)VALIDATION: PASS
File Operations: MATCH
- Bitcoin Core’s snapshot initialization adds a helper to delete coins DBs from disk, appends a snapshot suffix to the LevelDB dir name, and introduces snapshot detection/activation helper methods on
ChainstateManager. This Dash file modifies existingsrc/validation.cppin place in the same regions; no extra files are created or omitted for these pieces.Size Ratio: within expected bounds
- The new helpers (
DeleteCoinsDBFromDisk,ResetChainstates,DetectSnapshotChainstate,ActivateExistingSnapshot) and the smallInitCoinsDB/ActivateSnapshotadjustments are narrowly scoped and comparable in size to the upstream logic. There is no evidence of large additional logic beyond what snapshot initialization requires.Scope Creep: NONE
- All touched code deals strictly with UTXO snapshot lifecycle: directory naming, snapshot DB deletion, snapshot activation/cleanup, and detection on startup. No unrelated refactors, stylistic cleanups, or extra features are introduced here.
Bitcoin-Specific Code: NONE
- The changes here are generic snapshot plumbing. No segwit/witness, RBF, or other Bitcoin-specific constructs are introduced. Dash-specific types (e.g.
CChainState,CEvoDB,CChainstateHelper) are used consistently.Completeness (for this file): COMPLETE
- The pieces required by the snapshot utilities in
src/node/utxo_snapshot.{h,cpp}are present:
SNAPSHOT_CHAINSTATE_SUFFIXis used whenm_from_snapshot_blockhashis set.DeleteCoinsDBFromDiskunderstandsSNAPSHOT_BLOCKHASH_FILENAME.ActivateSnapshotnow persistsbase_blockhashviaWriteSnapshotBaseBlockhashand cleans up on failure.DetectSnapshotChainstateandActivateExistingSnapshotprovide the startup detection/activation path expected by the snapshot design.From the perspective of
src/validation.cpp, the Dash backport of the snapshot initialization logic appears faithful to Bitcoin Core’s assumeutxo design, with only the minimal adaptations needed for Dash’s validation stack.
6146-6177: Snapshot detection and activation initialization is correctly sequenced; no refactoring neededThe implementation properly handles snapshot chainstate detection and initialization:
Locking: Both
DetectSnapshotChainstate()andActivateExistingSnapshot()are correctly annotated withEXCLUSIVE_LOCKS_REQUIRED(::cs_main)in the header (src/validation.h:1067, 1075:1079). They are invoked at line 89 of src/node/chainstate.cpp within theLOCK(cs_main)held byLoadChainstate()(line 74).Initialization sequence: After
DetectSnapshotChainstate()returns, thechainman.GetAll()loop (line 177 of src/node/chainstate.cpp) properly initializes all chainstates including any detected snapshot chainstate:
InitCoinsDB()at line 180InitCoinsCache()per chainstateLoadChainTip()at line 214 for non-empty coin viewsThe design correctly follows Bitcoin Core's pattern: snapshot detection delegates creation to
ActivateExistingSnapshot(), and full initialization (coins DB, cache, and chain tip) occurs in the unified initialization loop that covers both IBD and snapshot chainstates. No additional assertion statements or refactoring are required.src/test/util/setup_common.cpp (3)
299-386: LoadVerifyActivateChainstate backport matches upstream behavior with minimal Dash adaptationsThis helper correctly wraps Dash’s
LoadChainstateandVerifyLoadedChainstate, threads through the new in‑memory flags (m_block_tree_db_in_memory,m_coins_db_in_memory), and passes only Dash‑specific managers anddash_dbs_in_memory=truethat are required for Dash’s extended chainstate/evodb/LLMQ setup. Aside from the (benign) choice to keepRegisterAllCoreRPCCommands(tableRPC)inside this helper instead of the constructor, the structure and control flow align with Bitcoin’s 6912a28 changes without any scope creep.
388-398: TestingSetup constructor cleanly reuses LoadVerifyActivateChainstate and preserves old defaultsThe updated
TestingSetupctor adds the two in‑memory flags with defaults oftrue, preserves existing call sites, and delegates initialization toLoadVerifyActivateChainstate(). This mirrors the upstream refactor while exposing the configurability needed for snapshot tests that want on‑disk coins/block-tree DBs, with no extra behavior beyond Bitcoin’s intent.
428-443: TestChain100Setup/TestChainSetup constructor extensions are the minimal Dash-specific plumbingExtending both
TestChain100Setupand the Dash‑onlyTestChainSetupto accept the same two in‑memory flags and forwarding them toTestingSetupis the minimal necessary adaptation so that fixtures (like SnapshotTestSetup) can select on‑disk DBs. Defaults keep prior behavior, and there is no unrelated logic added here versus the Bitcoin change toTestChain100Setup.
| static bool DeleteCoinsDBFromDisk(const fs::path db_path, bool is_snapshot) | ||
| EXCLUSIVE_LOCKS_REQUIRED(::cs_main) | ||
| { | ||
| AssertLockHeld(::cs_main); | ||
|
|
||
| if (is_snapshot) { | ||
| fs::path base_blockhash_path = db_path / node::SNAPSHOT_BLOCKHASH_FILENAME; | ||
|
|
||
| if (fs::exists(base_blockhash_path)) { | ||
| bool removed = fs::remove(base_blockhash_path); | ||
| if (!removed) { | ||
| LogPrintf("[snapshot] failed to remove file %s\n", | ||
| fs::PathToString(base_blockhash_path)); | ||
| } | ||
| } else { | ||
| LogPrintf("[snapshot] snapshot chainstate dir being removed lacks %s file\n", | ||
| fs::PathToString(node::SNAPSHOT_BLOCKHASH_FILENAME)); | ||
| } | ||
| } | ||
|
|
||
| std::string path_str = fs::PathToString(db_path); | ||
| LogPrintf("Removing leveldb dir at %s\n", path_str); | ||
|
|
||
| // We have to destruct before this call leveldb::DB in order to release the db | ||
| // lock, otherwise `DestroyDB` will fail. See `leveldb::~DBImpl()`. | ||
| const bool destroyed = dbwrapper::DestroyDB(path_str, {}).ok(); | ||
|
|
||
| if (!destroyed) { | ||
| LogPrintf("error: leveldb DestroyDB call failed on %s\n", path_str); | ||
| } | ||
|
|
||
| // Datadir should be removed from filesystem; otherwise initialization may detect | ||
| // it on subsequent statups and get confused. | ||
| // | ||
| // If the base_blockhash_path removal above fails in the case of snapshot | ||
| // chainstates, this will return false since leveldb won't remove a non-empty | ||
| // directory. | ||
| return destroyed && !fs::exists(db_path); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
DeleteCoinsDBFromDisk mirrors upstream snapshot cleanup; consider guarding against fs exceptions
The helper correctly:
- Drops the snapshot’s
base_blockhashfile whenis_snapshotis true. - Logs the target LevelDB dir.
- Calls
dbwrapper::DestroyDBand verifies both destruction and directory disappearance, matching the intended semantics from upstream snapshot cleanup.
One robustness difference from upstream is that fs::remove is called without a try/catch wrapper; if fs::remove throws (e.g. permission / transient filesystem error), this would currently bubble up and may abort the process instead of logging a warning and proceeding as best as possible. If you want parity with Bitcoin Core’s behavior, consider wrapping the fs::remove call in a try/catch(const fs::filesystem_error&) and logging the error code, then continuing based on the return of DestroyDB and !fs::exists(db_path).
🤖 Prompt for AI Agents
In src/validation.cpp around lines 5719-5757, wrap the
fs::remove(base_blockhash_path) call in a try/catch that catches const
fs::filesystem_error& (or std::filesystem::filesystem_error depending on
namespace) so filesystem exceptions don't propagate and abort the process; on
exception, log a warning including the path and the exception message/error code
(using fs::PathToString(base_blockhash_path) and e.what()/e.code()), and treat
the removal as failed (i.e., proceed to call DestroyDB and rely on
!fs::exists(db_path) and DestroyDB's result to decide the return value).
| void ChainstateManager::ResetChainstates() | ||
| { | ||
| m_ibd_chainstate.reset(); | ||
| m_snapshot_chainstate.reset(); | ||
| m_active_chainstate = nullptr; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
ResetChainstates is minimal but should be guarded by cs_main for consistency
ResetChainstates() correctly clears both IBD and snapshot chainstates and the active pointer, which is what you want when tearing down and rebuilding chainstates (e.g. during reindex or load errors).
Given that InitializeChainstate() and other mutators over m_ibd_chainstate, m_snapshot_chainstate, and m_active_chainstate all assume cs_main is held, it would be safer and more self-documenting to:
- Add
AssertLockHeld(::cs_main);at the top ofResetChainstates(), and - Ensure the declaration in
validation.his annotatedEXCLUSIVE_LOCKS_REQUIRED(::cs_main).
That keeps lock assumptions uniform and prevents accidental use from an unlocked context later.
🤖 Prompt for AI Agents
In src/validation.cpp around lines 6115-6121, ResetChainstates() currently
clears chainstate pointers without asserting cs_main; add
AssertLockHeld(::cs_main); as the first statement in the function to enforce the
lock is held. Also update the ResetChainstates declaration in validation.h to
include the annotation EXCLUSIVE_LOCKS_REQUIRED(::cs_main) so callers and static
analysis know the lock precondition.
Summary
DetectSnapshotChainstate(),ActivateExistingSnapshot(), andResetChainstates()methodsnode/utxo_snapshot.cppwith snapshot file utilitiesDash-specific Adaptations
ChainstatetoCChainStatethroughout (Dash naming convention)CEvoDB&,chain_helper)Files Changed
src/validation.cpp,src/validation.h,src/node/chainstate.cppsrc/node/utxo_snapshot.cppwith snapshot utilitiessrc/dbwrapper.h,src/txdb.h(StoragePath support)Original commit: 6912a28
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.