backport: assumeutxo M3 — background validation completion and snapshot promotion - #7553
backport: assumeutxo M3 — background validation completion and snapshot promotion#7553PastaPastaPasta wants to merge 16 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
⛔ Blockers found — Opus deferred (commit d04cfdd) |
WalkthroughThis change moves block acceptance, external block loading, candidate handling, block-index checks, and block storage coordination to Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Node
participant ChainstateManager
participant BackgroundChainstate
participant CEvoDB
participant Filesystem
Node->>ChainstateManager: load and detect chainstates
ChainstateManager->>BackgroundChainstate: validate snapshot state
BackgroundChainstate->>ChainstateManager: reach validation tip
ChainstateManager->>CEvoDB: verify and promote snapshot markers
ChainstateManager->>Filesystem: durably rename or remove chainstate paths
ChainstateManager->>Node: disable background chainstate and complete cleanup
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 5
🧹 Nitpick comments (3)
src/test/evo_db_tests.cpp (1)
268-268: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExercise the documented restart path.
CEvoDBuses.memory = true, and the promotion retry at Line [286] runs on the same object. This verifies same-instance idempotence, not recovery after reopening a persisted database. Use.memory = falseand reopen before the retry if restart safety is part of the contract; otherwise change the Line [285] comment to describe the narrower guarantee. Apply the same choice to the discard retry at Lines [301]-[303].🤖 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/test/evo_db_tests.cpp` at line 268, Update the CEvoDB test around the promotion and discard retries to exercise restart recovery: use persistent storage with memory=false, close the initial instance, then reopen the database before each retry. Apply the same reopen flow to both promotion and discard paths; if restart behavior is not intended, revise the nearby comments to state same-instance idempotence instead.src/test/validation_chainstatemanager_tests.cpp (1)
1096-1098: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that
chainstate_todeleteis removed.This test verifies the marker promotion after a completed directory swap. It does not verify that recovery removed the leftover
chainstate_todeletedirectory. The two sibling tests check this: line 1078 inchainstatemanager_snapshot_cleanup_recovers_first_renameand line 1141 inchainstatemanager_snapshot_cleanup_recovers_promoted_swap. Adding the same assertion keeps the three recovery tests symmetric and catches an orphaned chainstate directory.♻️ Proposed addition
this->LoadVerifyActivateChainstate(); + BOOST_CHECK(!fs::exists(data_dir / "chainstate_todelete")); BOOST_CHECK(m_node.evodb->VerifyBestBlock(EvoDbIdentity::NORMAL, snapshot_tip)); BOOST_CHECK(!m_node.evodb->HasDualChainstateMarker());🤖 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/test/validation_chainstatemanager_tests.cpp` around lines 1096 - 1098, Extend the assertions in the test covering completed directory-swap marker promotion after LoadVerifyActivateChainstate() to verify that the chainstate_todelete directory has been removed. Reuse the existing sibling-test assertion and keep the current VerifyBestBlock and HasDualChainstateMarker checks unchanged.src/node/chainstate.cpp (1)
41-44: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse shared constants for snapshot cleanup paths.
The current literals match the cleanup suffixes, and
options.data_dirusesargs.GetDataDirNet(). Define shared constants for_todeleteand_INVALID, and useSNAPSHOT_CHAINSTATE_SUFFIXto prevent future path drift.🤖 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/chainstate.cpp` around lines 41 - 44, Update the path definitions in the chainstate cleanup flow to use shared constants for the `_todelete` and `_INVALID` suffixes, including `SNAPSHOT_CHAINSTATE_SUFFIX` for the snapshot path. Ensure the constants are defined once and applied consistently with the existing `options.data_dir`/network data-directory handling.
🤖 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 `@doc/design/assumeutxo.md`:
- Around line 111-114: Update the assumeutxo completion description around
CompleteSnapshotValidation() to document that validation also compares the
background chainstate’s deterministic masternode-list state against the expected
compiled value before setting m_disabled. Retain the existing UTXO hash
verification and ActivateBestChain() lifecycle details.
In `@src/node/chainstate.cpp`:
- Around line 360-400: Update the snapshot-completion handling around
MaybeCompleteSnapshotValidation so a shutdown/interruption result such as
SnapshotCompletionResult::STATS_FAILED returns ChainstateLoadStatus::INTERRUPTED
before the generic validation-failure branch. Preserve SKIPPED and SUCCESS
behavior, and keep the existing failure message only for genuine snapshot
validation failures.
In `@src/test/validation_chainstatemanager_tests.cpp`:
- Around line 1034-1052: Use a scope guard immediately after saving
DIP0003Height in the test setup, anchored to the mutable_consensus and
old_dip3_height symbols, so restoration runs on both normal and exceptional
exits; remove the manual restore at the end. Also replace the hardcoded "dmn_S3"
key in the EvoDB write with the shared production constant for that record when
available.
In `@src/validation.cpp`:
- Around line 5890-5919: Update MaybeCompleteSnapshotValidation() so the
background marker uses an MN-list hash derived from m_ibd_chainstate, not
snapshot_chainstate, when the background tip is already base_blockhash;
otherwise leave the marker absent so validation can detect divergence. Compute
the snapshot_chainstate hash once and reuse it for WriteSnapshotBaseMNListHash.
- Around line 1648-1653: Update Chainstate::SnapshotBase() to cache and return
nullptr when LookupBlockIndex() cannot find the snapshot base, without calling
Assert(). Guard every caller that dereferences the returned base, including
MaybeCompleteSnapshotValidation() and the assertion sites around lines 3853,
5268, and 5312, so missing bases produce the intended SKIPPED or
BASE_BLOCKHASH_MISMATCH outcomes rather than aborting.
---
Nitpick comments:
In `@src/node/chainstate.cpp`:
- Around line 41-44: Update the path definitions in the chainstate cleanup flow
to use shared constants for the `_todelete` and `_INVALID` suffixes, including
`SNAPSHOT_CHAINSTATE_SUFFIX` for the snapshot path. Ensure the constants are
defined once and applied consistently with the existing
`options.data_dir`/network data-directory handling.
In `@src/test/evo_db_tests.cpp`:
- Line 268: Update the CEvoDB test around the promotion and discard retries to
exercise restart recovery: use persistent storage with memory=false, close the
initial instance, then reopen the database before each retry. Apply the same
reopen flow to both promotion and discard paths; if restart behavior is not
intended, revise the nearby comments to state same-instance idempotence instead.
In `@src/test/validation_chainstatemanager_tests.cpp`:
- Around line 1096-1098: Extend the assertions in the test covering completed
directory-swap marker promotion after LoadVerifyActivateChainstate() to verify
that the chainstate_todelete directory has been removed. Reuse the existing
sibling-test assertion and keep the current VerifyBestBlock and
HasDualChainstateMarker checks 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 41e20b0e-8431-4315-a262-222f7eb44dd6
📒 Files selected for processing (29)
doc/design/assumeutxo.mdsrc/bench/load_external.cppsrc/chain.hsrc/evo/chainhelper.cppsrc/evo/chainhelper.hsrc/evo/deterministicmns.cppsrc/evo/deterministicmns.hsrc/evo/evodb.cppsrc/evo/evodb.hsrc/evo/smldiff.cppsrc/evo/specialtxman.cppsrc/init.cppsrc/llmq/blockprocessor.cppsrc/llmq/snapshot.cppsrc/node/blockstorage.cppsrc/node/blockstorage.hsrc/node/chainstate.cppsrc/node/chainstate.hsrc/node/utxo_snapshot.cppsrc/test/blockmanager_tests.cppsrc/test/coinstatsindex_tests.cppsrc/test/evo_db_tests.cppsrc/test/fuzz/load_external_block_file.cppsrc/test/util/chainstate.hsrc/test/validation_block_tests.cppsrc/test/validation_chainstate_tests.cppsrc/test/validation_chainstatemanager_tests.cppsrc/validation.cppsrc/validation.h
|
Restructured the branch: the review fixes that previously sat as appended commits are now folded into their introducing commits, so each commit in the stack builds and reviews on its own (verified: the two amended adaptation commits compile standalone, and the final tree is byte-identical to the previously tested head a9e8b57). Where the CodeRabbit fixes landed:
The three commits that modify code merged in #7456 (shared unavailable-history sentinel, mempool handoff on snapshot activation, duplicate-commitment comment) remain standalone since their introducing commits are already in develop. 🤖 Posted autonomously by Claude on behalf of pasta. |
a9e8b57 to
88c0091
Compare
|
CI triage for the last run:
🤖 Posted autonomously by Claude on behalf of pasta. |
88c0091 to
488db89
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This M3 backport wires up AssumeUTXO background-validation completion and snapshot promotion, faithfully following upstream bitcoin#25740/bitcoin#27862/bitcoin#28050/bitcoin#27746, with well-tested EvoDB marker promotion, crash recovery, and mempool-handoff logic. Deep tracing of the new unconditional GetDeterministicMNListHash(snapshot_start_block) call in PopulateAndValidateSnapshot() (added by this PR) confirms a real, severe bug in the primary AssumeUTXO cold-start bootstrap case: it poisons the shared CDeterministicMNManager::mnListsCache with a synthetic empty masternode list keyed at the base block hash before the background chainstate has derived real state there, and since mnListsCache.emplace(...) is a no-op on an existing key, the poison survives even after the background chainstate legitimately connects and processes the base block — corrupting oldList/prevList derivation for base+1 and causing a real block to fail bad-cbtx-mnmerkleroot validation, permanently blocking background completion for exactly the bootstrap scenario this milestone targets. No existing test exercises this path because every test either pre-syncs the background chainstate past the base before activating the snapshot, or (for the two reset_chainstate=true tests) only wipes the coins database while leaving the shared EvoDB/mnListsCache state from before the reset intact. All CodeRabbit findings were independently verified against the exact head and found to already be fixed (INTERRUPTED-on-shutdown guard, nullable GetSnapshotBaseBlock()/GetSnapshotBaseHeight() accessors) or correctly withdrawn by CodeRabbit itself after maintainer clarification (the compute-once base MN-list-hash rationale, given the snapshot format currently carries no independent Dash payload). Backport prerequisite chains for all four upstream merges were independently confirmed complete by both agent lanes with no missing hunks.
Source: Codex general/dash-core-commit-history/backport-reviewer backend gpt-5.6-sol; Claude(Sonnet) general/dash-core-commit-history/backport-reviewer backend claude-sonnet-5; final verifier backend claude-sonnet-5. Orchestration only, not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— backport-reviewer (completed)
🔴 1 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/validation.cpp`:
- [BLOCKING] src/validation.cpp:5896-5926: Unconditional base MN-list hash capture poisons the shared dmnman cache on cold-start snapshot activation, breaking background validation of block base+1
`PopulateAndValidateSnapshot()` unconditionally calls `snapshot_chainstate.ChainHelper().GetDeterministicMNListHash(snapshot_start_block)` (line 5900) before `WriteDualChainstateMarker()` is committed (line 5923) and before `evo_db.SetDefaultIdentity(SNAPSHOT)` runs (that happens later, at snapshot-swap time in `ActivateSnapshot()`). This resolves to `CDeterministicMNManager::GetListForBlockInternal(snapshot_start_block)` under `EvoDbIdentity::NORMAL`.
In the realistic AssumeUTXO cold-start bootstrap (a fresh node loading a snapshot before any background sync has occurred — the primary use case this milestone exists to support per the PR description and doc/design/assumeutxo.md), the background/IBD chainstate is at genesis. `GetListForBlockInternal` finds no in-memory cache entry, no `DB_LIST_SNAPSHOT`, and no `DB_LIST_DIFF` for the base block on disk. Since `HasDualChainstateMarker()` is still false at this exact call site (the marker write happens after this call in the same function), the function does NOT throw `BlockDataUnavailableError`; it falls into the 'no snapshot and no diff on disk means initial snapshot' branch (src/evo/deterministicmns.cpp ~810-825): it sets `m_initial_snapshot_index = pindex` and `mnListsCache.emplace(pindex->GetBlockHash(), CDeterministicMNList(pindex->GetBlockHash(), pindex->nHeight, 0))` — fabricating and caching an EMPTY masternode list keyed by the base block's hash. This only happens when DIP0003 is already active at the base block height (the realistic case for any real assumeutxo snapshot on mainnet), since `GetListForBlockInternal` early-returns before touching the cache when DIP0003 isn't yet active.
`CDeterministicMNManager` (and its `mnListsCache`, confirmed as a single `Uint256HashMap` field in src/evo/deterministicmns.h:729) is one shared instance across both chainstates — constructed once in `CompleteChainstateInitialization` (src/node/chainstate.cpp:149) and referenced by both `Chainstate`s' `CChainstateHelper`. When the background chainstate later legitimately connects the base block during real catch-up, `CSpecialTxProcessor::BuildNewListFromBlock` (src/evo/specialtxman.cpp:264-266) correctly calls `m_dmnman.GetListForBlock(pindexPrev)` to derive the real list for the base block from `pindex->pprev` — unaffected by the poison. `Chainstate::RecordBackgroundMNListHash` (src/validation.cpp:2761) also correctly writes the independently-computed `mn_list` parameter to `EVODB_BACKGROUND_MNLIST_HASH`, bypassing the cache entirely — this specific marker is NOT corrupted, matching its own comment at src/evo/specialtxman.cpp:749-753 ('Snapshot activation may populate the shared MN-list cache with seeded state, so completion must not reconstruct this value through that cache'), which shows the author was aware of cache seeding but only guarded this one read path.
However, `CDeterministicMNManager::ProcessBlock` (src/evo/deterministicmns.cpp:685) still calls `mnListsCache.emplace(newList.GetBlockHash(), newList)` when persisting the base block's own correctly-derived list — and `emplace` on `std::unordered_map`/`Uint256HashMap` is a no-op when the key already exists. The poisoned empty entry at the base block's hash therefore survives even after the background chainstate connects the real base block. When the background chainstate next processes the block after the base (base+1), `CSpecialTxProcessor::BuildNewListFromBlock(block, pindexPrev=base_block, ...)` calls `m_dmnman.GetListForBlock(base_block)`, which hits the still-poisoned cache entry and returns the empty list instead of the real historical masternode set. The resulting `newList`/`calculatedMerkleRootMNList` for base+1 is built on the wrong base state and will not match that block's actual on-chain `merkleRootMNList` commitment (mined against the real historical state) — `CSpecialTxProcessor::ProcessSpecialTxsInBlock` (src/evo/specialtxman.cpp:762-769) then rejects a genuinely valid block with `state.Invalid(..., "bad-cbtx-mnmerkleroot")`. This permanently blocks background chainstate progress past the base block for any node that loads a snapshot before syncing to it — exactly the scenario this milestone is meant to complete.
All of the added and pre-existing unit tests (`SnapshotTestSetup::SetupSnapshot()`) call `CreateAndActivateUTXOSnapshot(this)` with the default `reset_chainstate=false`, so the background-to-be chainstate has always fully connected every block (including the base) before the snapshot is activated, meaning `GetListForBlockInternal` always finds real, already-persisted disk data for the base block and never falls into the poisoning branch. The two tests that do pass `reset_chainstate=true` (`chainstate_update_tip` and `chainstate_connectblock_bls_scheme` in src/test/validation_chainstate_tests.cpp) only reset the coins database (`InitCoinsDB`/`InitCoinsCache` on a fresh in-memory view); they reuse the same `CEvoDB`/`CDeterministicMNManager` instance that already has the real MN-list diff/snapshot for the base block cached and persisted from before the reset, so they don't reproduce the truly-cold-cache scenario either.
In `src/test/validation_chainstatemanager_tests.cpp`:
- [SUGGESTION] src/test/validation_chainstatemanager_tests.cpp:254-336: No test exercises snapshot activation against a background chainstate with a genuinely empty EvoDB/mnListsCache at the base block
Every `SnapshotTestSetup::SetupSnapshot()`-based test activates the snapshot only after the would-be background chainstate has already connected every block up to and past the base height, and the two `reset_chainstate=true` tests in `src/test/validation_chainstate_tests.cpp` only wipe the coins database, not the shared `CEvoDB`/`CDeterministicMNManager` state. This masks the cache-poisoning bug in the paired blocking finding, since the deterministic MN list for the base block is always already derived and cached/persisted by the time `PopulateAndValidateSnapshot()` runs. A test that activates a snapshot with real DIP3-registered masternodes at the base height while the background chainstate starts from a freshly constructed `CDeterministicMNManager`/wiped EvoDB (not merely a reset coins view) and then drives it forward through and past the base block would catch this class of bug and any regression of the fix.
| // base MN list, the SNAPSHOT-side marker must come from the payload so the | ||
| // completion comparison checks background revalidation against snapshot | ||
| // content instead of against another read of the same derivation. | ||
| const uint256 base_mn_list_hash{ | ||
| snapshot_chainstate.ChainHelper().GetDeterministicMNListHash(snapshot_start_block)}; | ||
|
|
||
| // The background chainstate never re-connects a base block it has already | ||
| // validated, so RecordBackgroundMNListHash cannot fire for it. Capture the | ||
| // base list on its behalf when its tip is already the base block. | ||
| if (const CBlockIndex* ibd_tip = m_ibd_chainstate->m_chain.Tip(); | ||
| ibd_tip != nullptr && ibd_tip->GetBlockHash() == base_blockhash) { | ||
| auto db_tx = snapshot_chainstate.m_evoDb.BeginTransaction(::EvoDbIdentity::NORMAL); | ||
| snapshot_chainstate.m_evoDb.WriteBackgroundMNListHash(base_blockhash, base_mn_list_hash); | ||
| db_tx->Commit(); | ||
| } | ||
|
|
||
| // Snapshot lifecycle recovery depends on the background chainstate's | ||
| // independently captured MN-list hash. Make all preceding NORMAL writes | ||
| // durable before publishing the snapshot markers. | ||
| if (!snapshot_chainstate.m_evoDb.CommitRootTransaction(EvoDbIdentity::NORMAL, /*sync=*/true)) { | ||
| LogPrintf("[snapshot] failed to sync background EvoDB state\n"); | ||
| return false; | ||
| } | ||
| { | ||
| auto db_tx = snapshot_chainstate.m_evoDb.BeginTransaction(EvoDbIdentity::SNAPSHOT); | ||
| snapshot_chainstate.m_evoDb.WriteBestBlock(EvoDbIdentity::SNAPSHOT, base_blockhash); | ||
| snapshot_chainstate.m_evoDb.WriteSnapshotBaseMNListHash(base_mn_list_hash); | ||
| snapshot_chainstate.m_evoDb.WriteDualChainstateMarker(); | ||
| db_tx->Commit(); | ||
| } | ||
| if (!snapshot_chainstate.m_evoDb.CommitRootTransaction(EvoDbIdentity::SNAPSHOT)) { | ||
| if (!snapshot_chainstate.m_evoDb.CommitRootTransaction(EvoDbIdentity::SNAPSHOT, /*sync=*/true)) { |
There was a problem hiding this comment.
🔴 Blocking: Unconditional base MN-list hash capture poisons the shared dmnman cache on cold-start snapshot activation, breaking background validation of block base+1
PopulateAndValidateSnapshot() unconditionally calls snapshot_chainstate.ChainHelper().GetDeterministicMNListHash(snapshot_start_block) (line 5900) before WriteDualChainstateMarker() is committed (line 5923) and before evo_db.SetDefaultIdentity(SNAPSHOT) runs (that happens later, at snapshot-swap time in ActivateSnapshot()). This resolves to CDeterministicMNManager::GetListForBlockInternal(snapshot_start_block) under EvoDbIdentity::NORMAL.
In the realistic AssumeUTXO cold-start bootstrap (a fresh node loading a snapshot before any background sync has occurred — the primary use case this milestone exists to support per the PR description and doc/design/assumeutxo.md), the background/IBD chainstate is at genesis. GetListForBlockInternal finds no in-memory cache entry, no DB_LIST_SNAPSHOT, and no DB_LIST_DIFF for the base block on disk. Since HasDualChainstateMarker() is still false at this exact call site (the marker write happens after this call in the same function), the function does NOT throw BlockDataUnavailableError; it falls into the 'no snapshot and no diff on disk means initial snapshot' branch (src/evo/deterministicmns.cpp ~810-825): it sets m_initial_snapshot_index = pindex and mnListsCache.emplace(pindex->GetBlockHash(), CDeterministicMNList(pindex->GetBlockHash(), pindex->nHeight, 0)) — fabricating and caching an EMPTY masternode list keyed by the base block's hash. This only happens when DIP0003 is already active at the base block height (the realistic case for any real assumeutxo snapshot on mainnet), since GetListForBlockInternal early-returns before touching the cache when DIP0003 isn't yet active.
CDeterministicMNManager (and its mnListsCache, confirmed as a single Uint256HashMap field in src/evo/deterministicmns.h:729) is one shared instance across both chainstates — constructed once in CompleteChainstateInitialization (src/node/chainstate.cpp:149) and referenced by both Chainstates' CChainstateHelper. When the background chainstate later legitimately connects the base block during real catch-up, CSpecialTxProcessor::BuildNewListFromBlock (src/evo/specialtxman.cpp:264-266) correctly calls m_dmnman.GetListForBlock(pindexPrev) to derive the real list for the base block from pindex->pprev — unaffected by the poison. Chainstate::RecordBackgroundMNListHash (src/validation.cpp:2761) also correctly writes the independently-computed mn_list parameter to EVODB_BACKGROUND_MNLIST_HASH, bypassing the cache entirely — this specific marker is NOT corrupted, matching its own comment at src/evo/specialtxman.cpp:749-753 ('Snapshot activation may populate the shared MN-list cache with seeded state, so completion must not reconstruct this value through that cache'), which shows the author was aware of cache seeding but only guarded this one read path.
However, CDeterministicMNManager::ProcessBlock (src/evo/deterministicmns.cpp:685) still calls mnListsCache.emplace(newList.GetBlockHash(), newList) when persisting the base block's own correctly-derived list — and emplace on std::unordered_map/Uint256HashMap is a no-op when the key already exists. The poisoned empty entry at the base block's hash therefore survives even after the background chainstate connects the real base block. When the background chainstate next processes the block after the base (base+1), CSpecialTxProcessor::BuildNewListFromBlock(block, pindexPrev=base_block, ...) calls m_dmnman.GetListForBlock(base_block), which hits the still-poisoned cache entry and returns the empty list instead of the real historical masternode set. The resulting newList/calculatedMerkleRootMNList for base+1 is built on the wrong base state and will not match that block's actual on-chain merkleRootMNList commitment (mined against the real historical state) — CSpecialTxProcessor::ProcessSpecialTxsInBlock (src/evo/specialtxman.cpp:762-769) then rejects a genuinely valid block with state.Invalid(..., "bad-cbtx-mnmerkleroot"). This permanently blocks background chainstate progress past the base block for any node that loads a snapshot before syncing to it — exactly the scenario this milestone is meant to complete.
All of the added and pre-existing unit tests (SnapshotTestSetup::SetupSnapshot()) call CreateAndActivateUTXOSnapshot(this) with the default reset_chainstate=false, so the background-to-be chainstate has always fully connected every block (including the base) before the snapshot is activated, meaning GetListForBlockInternal always finds real, already-persisted disk data for the base block and never falls into the poisoning branch. The two tests that do pass reset_chainstate=true (chainstate_update_tip and chainstate_connectblock_bls_scheme in src/test/validation_chainstate_tests.cpp) only reset the coins database (InitCoinsDB/InitCoinsCache on a fresh in-memory view); they reuse the same CEvoDB/CDeterministicMNManager instance that already has the real MN-list diff/snapshot for the base block cached and persisted from before the reset, so they don't reproduce the truly-cold-cache scenario either.
source: ['claude']
There was a problem hiding this comment.
Confirmed and fixed — the finding is correct on every step: with the background chainstate below the base, the capture ran before the dual-chainstate marker was durable, so GetListForBlockInternal took the legacy bootstrap branch, fabricated an empty list for the base hash, and emplace kept it forever; the SNAPSHOT-side marker also captured a hash of that fabricated list, which would additionally have quarantined valid snapshots at completion (EVO_STATE_MISMATCH).
Fix (folded into backport: adapt Dash EvoDB completion path for bitcoin#25740):
- The base MN-list hash is captured only when the background tip is already the base block — the one case the state genuinely exists. On a cold start nothing touches
GetListForBlockat activation, so no cache entry is fabricated and no marker is written. MaybeCompleteSnapshotValidationskips the deterministic MN-list comparison with an explicit log when the SNAPSHOT marker is absent, falling back to the upstream UTXO-set-hash criterion. The comparison stays enforced whenever the marker exists, and the loadtxoutset milestone will make it unconditional by deriving the marker from the snapshot payload.
Tests added: chainstatemanager_snapshot_completion_without_base_list_marker (completion succeeds with no captured markers) and an assertion in the reset_chainstate=true fixture that cold activation writes no base MN-list marker. The full-cold-cache end-to-end (fresh EvoDB, DIP3-active base, background re-sync through base+1 over P2P) needs the loadtxoutset functional-test machinery and is deferred to that milestone alongside the payload work.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved in this update — Unconditional base MN-list hash capture poisons the shared dmnman cache on cold-start snapshot activation, breaking background validation of block base+1 no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
488db89 to
14994f4
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
doc/design/assumeutxo.md (1)
111-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the function name and the stray parenthesis.
The code names the function
ChainstateManager::MaybeCompleteSnapshotValidation(), notCompleteSnapshotValidation(). Line 112 also closes a parenthesis that was never opened.📝 Proposed fix
-chainstate, we stop use of the background chainstate by setting `m_disabled`, in -`CompleteSnapshotValidation()`, which is checked in `ActivateBestChain()`). We hash the +chainstate, we stop use of the background chainstate by setting `m_disabled` in +`MaybeCompleteSnapshotValidation()` (which is checked in `ActivateBestChain()`). We hash the background chainstate's UTXO set contents and ensure it matches the compiled value in🤖 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 `@doc/design/assumeutxo.md` around lines 111 - 118, Update the design text to reference ChainstateManager::MaybeCompleteSnapshotValidation() instead of CompleteSnapshotValidation(), and remove the unmatched closing parenthesis in the sentence describing how m_disabled is checked in ActivateBestChain().src/validation.cpp (1)
5995-5999: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSurface the EvoDB sync failure instead of returning silently.
CommitRootTransaction()failure here means a database write error. The function returnsSTATS_FAILED, but the only caller inConnectTip()discards the result. The background chainstate stays enabled and the tip is already at the snapshot base, so no furtherConnectTip()call retries completion. The node then continues on the snapshot tip with the dual-chainstate markers still present, and the operator receives only a log line.Consider
AbortNode()here, in line with the other unrecoverable EvoDB paths in this file.🤖 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/validation.cpp` around lines 5995 - 5999, The snapshot completion path should surface a failed CommitRootTransaction as an unrecoverable database error. In the failure branch within snapshot completion, invoke the existing AbortNode() mechanism with an appropriate error message before returning SnapshotCompletionResult::STATS_FAILED, matching the handling used by other unrecoverable EvoDB paths in validation.cpp.
🤖 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 `@doc/design/assumeutxo.md`:
- Around line 111-118: Update the design text to reference
ChainstateManager::MaybeCompleteSnapshotValidation() instead of
CompleteSnapshotValidation(), and remove the unmatched closing parenthesis in
the sentence describing how m_disabled is checked in ActivateBestChain().
In `@src/validation.cpp`:
- Around line 5995-5999: The snapshot completion path should surface a failed
CommitRootTransaction as an unrecoverable database error. In the failure branch
within snapshot completion, invoke the existing AbortNode() mechanism with an
appropriate error message before returning
SnapshotCompletionResult::STATS_FAILED, matching the handling used by other
unrecoverable EvoDB paths in validation.cpp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 460fd029-df05-42d5-9be0-c59d0d610c17
📒 Files selected for processing (5)
doc/design/assumeutxo.mdsrc/node/chainstate.cppsrc/test/validation_chainstate_tests.cppsrc/test/validation_chainstatemanager_tests.cppsrc/validation.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/node/chainstate.cpp
- src/test/validation_chainstatemanager_tests.cpp
…hen renaming chainstates
Discard Dash snapshot lifecycle markers only after the invalid snapshot directory rename succeeds. If the rename fails, preserving the markers keeps the existing restart recovery state recognizable while the upstream rename error is propagated to the fatal shutdown message.
Keep Dash’s mock shutdown callback while asserting the expected fatal diagnostic. The default callback reaches StartShutdown(), whose unit-test guard aborts the process before this Dash test can complete.
a733dd7 Remove unused function `reliesOnAssumedValid` (Suhas Daftuar) d4a11ab Cache block index entry corresponding to assumeutxo snapshot base blockhash (Suhas Daftuar) 3556b85 Move CheckBlockIndex() from Chainstate to ChainstateManager (Suhas Daftuar) 0ce805b Documentation improvements for assumeutxo (Ryan Ofsky) 768690b Fix initialization of setBlockIndexCandidates when working with multiple chainstates (Suhas Daftuar) d43a1f1 Tighten requirements for adding elements to setBlockIndexCandidates (Suhas Daftuar) d0d40ea Move block-storage-related logic to ChainstateManager (Suhas Daftuar) 3cfc753 test: Clear block index flags when testing snapshots (Suhas Daftuar) 272fbc3 Update CheckBlockIndex invariants for chains based on an assumeutxo snapshot (Suhas Daftuar) 10c0571 Add wrapper for adding entries to a chainstate's block index candidates (Suhas Daftuar) 471da5f Move block-arrival information / preciousblock counters to ChainstateManager (Suhas Daftuar) 1cfc887 Remove CChain dependency in node/blockstorage (Suhas Daftuar) fe86a7c Explicitly track maximum block height stored in undo files (Suhas Daftuar) Pull request description: This PR proposes a clean up of the relationship between block storage and the chainstate objects, by moving the decision of whether to store a block on disk to something that is not chainstate-specific. Philosophically, the decision of whether to store a block on disk is related to validation rules that do not require any UTXO state; for anti-DoS reasons we were using some chainstate-specific heuristics, and those have been reworked here to achieve the proposed separation. This PR also fixes a bug in how a chainstate's `setBlockIndexCandidates` was being initialized; it should always have all the HAVE_DATA block index entries that have more work than the chain tip. During startup, we were not fully populating `setBlockIndexCandidates` in some scenarios involving multiple chainstates. Further, this PR establishes a concept that whenever we have 2 chainstates, that we always know the snapshotted chain's base block and the base block's hash must be an element of our block index. Given that, we can establish a new invariant that the background validation chainstate only needs to consider blocks leading to that snapshotted block entry as potential candidates for its tip. As a followup I would imagine that when writing net_processing logic to download blocks for the background chainstate, that we would use this concept to only download blocks towards the snapshotted entry as well. ACKs for top commit: achow101: ACK a733dd7 jamesob: reACK a733dd7 ([`jamesob/ackr/27746.5.sdaftuar.rework_validation_logic`](https://github.com/jamesob/bitcoin/tree/ackr/27746.5.sdaftuar.rework_validation_logic)) Sjors: Code review ACK a733dd7. ryanofsky: Code review ACK a733dd7. Just suggested changes since the last review. There are various small things that could be followed up on, but I think this is ready for merge. Tree-SHA512: 9ec17746f22b9c27082743ee581b8adceb2bd322fceafa507b428bdcc3ffb8b4c6601fc61cc7bb1161f890c3d38503e8b49474da7b5ab1b1f38bda7aa8668675
Preserve ChainLock candidate exclusions in the new admission wrapper and keep Dash background-notification and EvoDB fixtures consistent with the tightened multi-chainstate candidate invariants.
Peer-penalty exemption for unavailable history hinged on three files repeating one literal string that IsBlockDataUnavailableError() then substring-matched; rewording any copy would silently revert those paths to Misbehaving. Define the suffix once next to BlockDataUnavailableError and use it at every producer and in the matcher.
Both chainstates carried a live mempool pointer after snapshot activation, so background ConnectTip called removeForBlock and removeExpiredAssetUnlock with historical blocks and lower heights. Follow the bitcoin#27596 shape: only the active chainstate keeps the mempool. Runtime activation transfers it to the snapshot chainstate, restart activation clears it from the background chainstate, and the invalid-snapshot revert hands it back.
The background chainstate in this fixture is reset to genesis before activation, so the base MN list is not derivable; a capture at activation would fabricate an empty list and poison the shared list cache (thepastaclaw review finding).
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This delta (488db89..14994f4) is the direct fix for the previously-reported blocking cache-poisoning bug in AssumeUTXO M3 snapshot completion. PopulateAndValidateSnapshot() now captures the base MN-list hash only when the background/IBD chainstate's tip already equals the snapshot base block (verified at src/validation.cpp:5906-5912), so a cold-start activation performs no GetDeterministicMNListHash() lookup, fabricates no synthetic empty-list cache entry, and writes no EVODB_SNAPSHOT_MNLIST_HASH marker. MaybeCompleteSnapshotValidation() correctly treats an absent marker as 'nothing to compare' and falls back to the pre-existing UTXO-set-hash criterion (verified at src/validation.cpp:6108-6129). The final commit (14994f4) adds a direct regression assertion that the reset-to-genesis fixture's cold activation captures no base MN-list marker. All CodeRabbit findings at this head were independently re-verified: the INTERRUPTED-on-shutdown fix and the nullable GetSnapshotBaseBlock()/GetSnapshotBaseHeight() accessors are confirmed present and correct in the code; the 'preserve independent MN-list hash' finding was correctly withdrawn by CodeRabbit itself since the current snapshot format carries no independent Dash payload to compare against before the loadtxoutset milestone, and a TODO documents that future obligation. No blocking or in-scope suggestion findings remain.
Source: codex-general/codex-dash-core-commit-history/codex-backport-reviewer backend gpt-5.6-sol; sonnet-general/sonnet-dash-core-commit-history/sonnet-backport-reviewer backend claude-sonnet-5; final verifier backend claude-sonnet-5. Orchestration only, not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— backport-reviewer (completed)
14994f4 to
e3a8989
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This M3 backport (AssumeUTXO background-validation completion and snapshot promotion) is well-tested and internally consistent at the exact head. The only remaining valid issue is a documentation gap: doc/design/assumeutxo.md describes the deterministic-MN-list comparison as an unconditional part of completion, but the implemented code (src/validation.cpp:5885-5927, 6114-6122) deliberately skips it on cold-start snapshot activation, when no independent Dash state is derivable yet. All eight exact-head CodeRabbit threads were independently re-verified: the INTERRUPTED-on-shutdown guard and the nullable GetSnapshotBaseBlock()/GetSnapshotBaseHeight() accessors are confirmed present and correct; the 'preserve an independent MN-list hash' request was correctly withdrawn given the snapshot format carries no independent Dash payload before the loadtxoutset milestone. No missing backport prerequisites were found by either backport-reviewer lane.
Source: Codex general/dash-core-commit-history/backport-reviewer backend gpt-5.6-sol; Claude(Sonnet) general/dash-core-commit-history/backport-reviewer backend claude-sonnet-5; final verifier backend claude-sonnet-5. Orchestration only, not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— backport-reviewer (completed)
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `doc/design/assumeutxo.md`:
- [SUGGESTION] doc/design/assumeutxo.md:114-118: Document that the base MN-list comparison is skipped after cold-start activation
This paragraph states that completion always compares the background-derived deterministic-masternode-list hash against a hash recorded at snapshot activation. That's not what the code does: `PopulateAndValidateSnapshot()` only captures/writes `EVODB_SNAPSHOT_MNLIST_HASH` when the background/IBD chainstate's tip is already at the base block (src/validation.cpp:5904-5913) — on the primary cold-start bootstrap path (fresh node loading a snapshot before any background sync), no marker is written. `MaybeCompleteSnapshotValidation()` correctly treats the absent marker as 'nothing to compare' and falls back to the UTXO-set-hash criterion alone (src/validation.cpp:6114-6122), logging a skip message. The design doc should describe this conditional behavior so readers don't assume the deterministic-MN-list check is always enforced.
| `CMainParams::m_assumeutxo_data`. In Dash, completion additionally compares the | ||
| deterministic masternode-list hash the background chainstate derived at the base block | ||
| against the hash recorded at snapshot activation, and the EvoDB best-block markers | ||
| against both chainstates' coins tips; any divergence fails completion with | ||
| `EVO_STATE_MISMATCH` and quarantines the snapshot exactly like a UTXO hash mismatch. |
There was a problem hiding this comment.
🟡 Suggestion: Document that the base MN-list comparison is skipped after cold-start activation
This paragraph states that completion always compares the background-derived deterministic-masternode-list hash against a hash recorded at snapshot activation. That's not what the code does: PopulateAndValidateSnapshot() only captures/writes EVODB_SNAPSHOT_MNLIST_HASH when the background/IBD chainstate's tip is already at the base block (src/validation.cpp:5904-5913) — on the primary cold-start bootstrap path (fresh node loading a snapshot before any background sync), no marker is written. MaybeCompleteSnapshotValidation() correctly treats the absent marker as 'nothing to compare' and falls back to the UTXO-set-hash criterion alone (src/validation.cpp:6114-6122), logging a skip message. The design doc should describe this conditional behavior so readers don't assume the deterministic-MN-list check is always enforced.
| `CMainParams::m_assumeutxo_data`. In Dash, completion additionally compares the | |
| deterministic masternode-list hash the background chainstate derived at the base block | |
| against the hash recorded at snapshot activation, and the EvoDB best-block markers | |
| against both chainstates' coins tips; any divergence fails completion with | |
| `EVO_STATE_MISMATCH` and quarantines the snapshot exactly like a UTXO hash mismatch. | |
| `CMainParams::m_assumeutxo_data`. In Dash, completion also verifies that the | |
| EvoDB best-block markers match both chainstates' coins tips. When snapshot | |
| activation finds the background chainstate already at the base block, it also | |
| records the base deterministic masternode-list hash for comparison at | |
| completion. Cold-start activation cannot record that hash until the snapshot | |
| format carries independent Dash state, so the deterministic MN-list comparison | |
| is skipped in that case while the UTXO-set hash and EvoDB tip-marker checks | |
| remain enforced. Any performed Dash-state check that diverges fails completion | |
| with `EVO_STATE_MISMATCH` and quarantines the snapshot. |
source: ['codex']
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If these PRs merge firstThis PR will likely need a rebase:
|
| if (!chainman.DetectSnapshotChainstate(options.mempool, snapshot_error)) { | ||
| return {ChainstateLoadStatus::FAILURE, snapshot_error}; | ||
| } | ||
| dmnman = std::make_unique<CDeterministicMNManager>(evodb, *options.mn_metaman); |
There was a problem hiding this comment.
interesting. I have a small refactoring that pull out dmnman and evodb initialization out from LoadChainstate [now CompleteChainstateInitialization].
I guess let's merge M3 first. Can you ask Fable in the context of assumeutxo m3...M8 if there's any good time to create a PR for refactoring? Is it after M3 good? or better to do as prerequisite? See 8db4870 - draft changes
The idea of my refactoring to initialize isman, evodb, dmnman before mempools and remove this late initialization:
if (mempool) {
mempool->ConnectManagers(dmnman.get(), llmq_ctx->isman.get());
}
so, mempool will be constructed strictly after dmnman and isman already alive so I hadn't created PR because I haven't done that part yet. Also so far as evodb and dmnman is not really part of chainstate it should not be initialized by chainstate.
| fs::PathToString(write_to)); | ||
| return false; | ||
| } | ||
| DirectoryCommit(*chaindir); |
There was a problem hiding this comment.
it looks a bit not reliable that DirectoryCommit is called directly in multiple occasions without any guards. Can it be forgotten at some important place?
| if (!rename_result) { | ||
| user_error = strprintf(Untranslated("%s\n%s"), user_error, util::ErrorString(rename_result)); | ||
| user_error += Untranslated("\n") + util::ErrorString(rename_result); | ||
| } else if (!m_ibd_chainstate->m_evoDb.DiscardSnapshotMarkers()) { |
There was a problem hiding this comment.
nit for "backport: adapt bitcoin#27862 for Dash crash recovery"
this commit should be squashed to 27862 backport, because
backport removes this code and after that it apparently re-appear. It looks like bug:
Diff of 27862-backport:
- m_snapshot_chainstate->InvalidateCoinsDBOnDisk();
- if (!m_ibd_chainstate->m_evoDb.DiscardSnapshotMarkers()) {
- LogPrintf("[snapshot] failed to remove invalid snapshot EvoDB markers\n");
+ auto rename_result = m_snapshot_chainstate->InvalidateCoinsDBOnDisk();
+ if (!rename_result) {
+ user_error = strprintf(Untranslated("%s\n%s"), user_error, util::ErrorString(rename_result));
} | Chainstate& validation_chainstate = *std::get<0>(chainstates); | ||
| ChainstateManager& chainman = *Assert(m_node.chainman); | ||
| SnapshotCompletionResult res; | ||
| auto mock_shutdown = [](bilingual_str msg) {}; |
There was a problem hiding this comment.
nit: the same for "test: adapt bitcoin#28050 coverage for Dash shutdown"
This commit should be squashed to backport.
Backport of 28050 does remove this code and it apparently appeared back.
| std::shared_ptr<CBlock> pblockone = std::make_shared<CBlock>(); | ||
| { | ||
| LOCK(::cs_main); | ||
| BOOST_REQUIRE(node::ReadBlockFromDisk(*pblockone, chainman.ActiveChain()[1], Params().GetConsensus())); |
There was a problem hiding this comment.
nit: instead should be used chainman.GetConsensus()
|
|
||
| // Set tip of the assume-valid-based chain to the assume-valid block | ||
| cs2.m_chain.SetTip(*assumed_base); | ||
|
|
There was a problem hiding this comment.
nit: unrelated change; empty line is added after call of cs2.m_chain.SetTip()
-| } | ||
|
|
||
| int nHeight = pindex->nHeight; | ||
| std::vector<CBlockIndex*> reconsidered_blocks; |
There was a problem hiding this comment.
could you clarify, why exactly setBlockIndexCandidates can not be used as it was used and reconsidered_blocks should be introduced?
Downside of it is refactoring is increasing diversification between dash core and bitcoin core implementation.
| !SerializedEqual(stored_commitment, std::make_pair(qc, blockHash))) { | ||
| // Preserve the existing duplicate-commitment result while allowing an | ||
| // exact block re-derivation to proceed through all validation below. | ||
| // Note: a commitment retained by UndoBlock for another chainstate's |
There was a problem hiding this comment.
nit: commit message is cut ugly in the middle of word
Proces…
…sCommitment
…abort GetSnapshotBaseBlock() bypassed Chainstate::SnapshotBase() so completion could observe a missing base and return BASE_BLOCKHASH_MISMATCH, but that branch was unreachable: LoadBlockIndex's candidate admission Asserts the base for the background chainstate before completion ever runs, so a missing base aborted the node anyway, and the bypass silently lost upstream's per-call caching (bitcoin d4a11ab). Restore the cached delegation, make SnapshotBase() non-asserting (synthetic unit fixtures activate a snapshot before its base is indexed), and detect the missing base explicitly in ChainstateManager::LoadBlockIndex() before any admission runs, failing with the standard reindex advice; -reindex already discards the snapshot chainstate and its EvoDB markers. Covered by a new test that wipes blocks/index under a persisted snapshot.
The crash-recovery state machine depends on every chainstate-directory rename and removal being followed by a DirectoryCommit of the parent, but the pattern was open-coded at six sites where the commit could silently be forgotten. Add RenameDurably/RemoveAllDurably next to DirectoryCommit and use them everywhere the snapshot lifecycle touches directories.
…idate admission The CheckBlockIndex doc comment referenced upstream's m_options.check_block_index, which Dash does not have; the gate is still the fCheckBlockIndex global. Also document why ResetBlockFailureFlags defers candidate admission to a pass over every usable chainstate instead of upstream's inline insert (review question in dashpay#7553).
… overlap EraseSnapshotMarkers' removal of the background MN-list hash key was untested, and the promote/discard overlap that RecoverSnapshotCleanup documents (invalid-rename crash with the SNAPSHOT marker at the background tip) relied on a comment alone. Pin both: the marker-rollback test now seeds and asserts the background key, and a new recovery test drives the overlap state through LoadVerifyActivateChainstate and asserts the promote branch lands in the same end state as a completed discard.
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/util/fs_helpers.cpp`:
- Around line 164-174: Update DirectoryCommit, RenameDurably, and
RemoveAllDurably in src/util/fs_helpers.cpp to detect directory-open and fsync
failures and propagate them as fs::filesystem_error instead of reporting
success; document this failure contract in src/util/fs_helpers.h for both
durable helper APIs.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d3b7c62-d750-4f21-9ff0-da542c4b1631
📒 Files selected for processing (9)
src/node/chainstate.cppsrc/test/evo_db_tests.cppsrc/test/util/setup_common.cppsrc/test/util/setup_common.hsrc/test/validation_chainstatemanager_tests.cppsrc/util/fs_helpers.cppsrc/util/fs_helpers.hsrc/validation.cppsrc/validation.h
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/evo_db_tests.cpp
- src/node/chainstate.cpp
- src/validation.cpp
| void RenameDurably(const fs::path& src, const fs::path& dest) | ||
| { | ||
| fs::rename(src, dest); | ||
| DirectoryCommit(dest.parent_path()); | ||
| } | ||
|
|
||
| void RemoveAllDurably(const fs::path& path) | ||
| { | ||
| fs::remove_all(path); | ||
| DirectoryCommit(path.parent_path()); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 'void DirectoryCommit|fsbridge::fopen|fsync\(|RenameDurably|RemoveAllDurably' \
src/util/fs_helpers.cpp src/util/fs_helpers.h
rg -n -C 5 'RenameDurably\(|RemoveAllDurably\(|catch \(const fs::filesystem_error' \
src/validation.cpp src/node/chainstate.cppRepository: dashpay/dash
Length of output: 11022
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- helper implementation and declarations ---'
ast-grep outline src/util/fs_helpers.cpp --match 'DirectoryCommit' --view expanded
cat -n src/util/fs_helpers.cpp | sed -n '120,185p'
cat -n src/util/fs_helpers.h | sed -n '1,55p'
echo '--- all helper call sites ---'
rg -n -C 8 '\b(RenameDurably|RemoveAllDurably|DirectoryCommit)\s*\(' --glob '!src/util/fs_helpers.cpp' --glob '!src/util/fs_helpers.h' .
echo '--- filesystem error and snapshot transition handling ---'
cat -n src/node/chainstate.cpp | sed -n '1,145p'
cat -n src/validation.cpp | sed -n '6370,6525p'
echo '--- relevant tests and recent file history ---'
rg -n -C 4 'DirectoryCommit|RenameDurably|RemoveAllDurably|snapshot.*rename|snapshot.*remove' src test doc
git diff --stat
git log -8 --oneline -- src/util/fs_helpers.cpp src/util/fs_helpers.h src/node/chainstate.cpp src/validation.cppRepository: dashpay/dash
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- working-tree scope ---'
git status --short
git diff --stat -- src/util/fs_helpers.cpp src/util/fs_helpers.h src/node/chainstate.cpp src/validation.cpp
echo '--- focused diff for the helpers and lifecycle callers ---'
git diff -- src/util/fs_helpers.cpp src/util/fs_helpers.h src/node/chainstate.cpp src/validation.cpp
echo '--- filesystem bridge and exception conventions ---'
rg -n -C 8 'FILE\* fopen|fsbridge::fopen|filesystem_error|system_error|SysErrorString' src/util src | \
rg -v 'src/(immer|leveldb|secp256k1|univalue)/' | head -n 500
echo '--- helper-related tests only ---'
rg -n -C 10 'RenameDurably|RemoveAllDurably|DirectoryCommit|FileCommit' src/test src/test/utilRepository: dashpay/dash
Length of output: 25580
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
impl = Path("src/util/fs_helpers.cpp").read_text()
header = Path("src/util/fs_helpers.h").read_text()
validation = Path("src/validation.cpp").read_text()
chainstate = Path("src/node/chainstate.cpp").read_text()
directory = re.search(r"void DirectoryCommit\(.*?\n\}", impl, re.S)
assert directory, "DirectoryCommit() definition not found"
body = directory.group(0)
assert "if (file)" in body, "directory-open result is not checked"
assert "fsync(fileno(file));" in body, "directory fsync call not found"
assert "if (fsync" not in body, "unexpected fsync error check"
assert "return" not in body, "DirectoryCommit() unexpectedly reports a result"
rename = re.search(r"void RenameDurably\(.*?\n\}", impl, re.S).group(0)
remove = re.search(r"void RemoveAllDurably\(.*?\n\}", impl, re.S).group(0)
assert "DirectoryCommit(dest.parent_path());" in rename
assert "DirectoryCommit(path.parent_path());" in remove
assert "PromoteSnapshotMarkers(snapshot_tip)" in validation
assert validation.index("RenameDurably(snapshot_chainstate_path, ibd_chainstate_path)") < validation.index("PromoteSnapshotMarkers(snapshot_tip)")
assert "catch (const fs::filesystem_error& e)" in validation
assert "catch (const fs::filesystem_error& e)" in chainstate
print("DirectoryCommit() discards open/fsync failures; durable helpers have no failure path; lifecycle callers already handle fs::filesystem_error.")
PY
echo '--- fsbridge::fopen definition ---'
rg -n -C 8 'FopenFn|fopen\(const fs::path|namespace fsbridge' src/util/fs.h src/util/fs.cpp
echo '--- concise recent history ---'
git log -5 --oneline -- src/util/fs_helpers.cpp src/util/fs_helpers.h src/node/chainstate.cpp src/validation.cppRepository: dashpay/dash
Length of output: 3067
Propagate directory synchronization failures.
DirectoryCommit() ignores directory-open and fsync() failures. Therefore, RenameDurably() and RemoveAllDurably() can report success without durable filesystem changes. Snapshot cleanup can then promote EvoDB markers after an uncommitted transition.
Make the helpers propagate these failures as fs::filesystem_error, which existing lifecycle callers already handle. Document the failure contract in src/util/fs_helpers.h.
📍 Affects 2 files
src/util/fs_helpers.cpp#L164-L174(this comment)src/util/fs_helpers.h#L28-L39
🤖 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/util/fs_helpers.cpp` around lines 164 - 174, Update DirectoryCommit,
RenameDurably, and RemoveAllDurably in src/util/fs_helpers.cpp to detect
directory-open and fsync failures and propagate them as fs::filesystem_error
instead of reporting success; document this failure contract in
src/util/fs_helpers.h for both durable helper APIs.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The snapshot lifecycle is generally coherent and well tested, but the new durable filesystem wrappers can silently ignore directory-open and synchronization failures before EvoDB marker promotion, so the crash-recovery ordering guarantee is not actually enforced. I also confirmed the carried documentation mismatch, duplicate auxiliary block-index entries after promotion, and two commit-history cleanup items.
Source: Codex general, dash-core-commit-history, and backport-reviewer — backend gpt-5.6-sol; final verifier — backend gpt-5.6-sol. Orchestration only, not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 3 suggestion(s)
2 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/util/fs_helpers.cpp`:
- [BLOCKING] src/util/fs_helpers.cpp:153-174: Propagate directory synchronization failures from durable snapshot operations
On POSIX, `DirectoryCommit()` silently ignores both a failed `fsbridge::fopen()` and the return value from `fsync()`. The new `RenameDurably()` and `RemoveAllDurably()` APIs consequently report success even when the directory update was not made durable. `ValidatedSnapshotCleanup()` relies on that success before calling `PromoteSnapshotMarkers()`, so an unsuccessful directory sync can be followed by a durable EvoDB marker transition and a crash can leave the marker state inconsistent with the directory layout. Make the checked directory synchronization used by these helpers propagate failures as `fs::filesystem_error`; the snapshot lifecycle callers already catch that exception. Document the complete failure contract for both APIs.
In `src/node/chainstate.cpp`:
- [SUGGESTION] src/node/chainstate.cpp:399-404: Clear append-only block indexes before the post-promotion reload
`CompleteChainstateInitialization()` invokes `ChainstateManager::LoadBlockIndex()` before snapshot cleanup and invokes it again after `ValidatedSnapshotCleanup()`. Each load appends every parent/child relationship to `BlockManager::m_prev_block_index` and appends unresolved entries to `m_blocks_unlinked`, while `ResetChainstates()` does not clear either map. A successful promotion therefore retains duplicate entries for the lifetime of the process, increasing memory usage and causing descendant scans to process each child twice. Clear both auxiliary maps alongside the candidate set before the second initialization reload.
In `<commit:19d2a0c>`:
- [SUGGESTION] <commit:19d2a0c>:1: Fold the missing-base correction into the bitcoin#27746 adaptation
Commit `70e41641ddb` replaces upstream's cached `SnapshotBase()` delegation with a direct lookup, but this does not make missing-base completion recoverable because candidate admission asserts the base before completion runs. Commit `19d2a0c5939` restores the cached delegation and adds the pre-admission startup check and regression test that make the adaptation correct. Fold the correction into `70e41641ddb` so the advertised bitcoin#27746 adaptation does not leave an avoidable bad bisect state.
In `<commit:6194f23>`:
- [SUGGESTION] <commit:6194f23>:1: Add a durable rationale to the main Dash adaptation commit
Commit `6194f2324f3` contains the central Dash-specific adaptation across 13 files, including EvoDB marker promotion and discard, MN-list lifecycle hashes, crash recovery, completion outcomes, documentation, and tests, but its message contains only a subject. Add a body explaining why Dash requires a separate EvoDB lifecycle, which state is checked at completion, and why cold-start activation cannot capture the MN-list marker. This consensus-adjacent rationale should remain available in normal history rather than only in the pull-request description.
In `doc/design/assumeutxo.md`:
- [SUGGESTION] doc/design/assumeutxo.md:114-118: Document that the base MN-list comparison is skipped after cold-start activation
(existing thread: https://github.com/dashpay/dash/pull/7553#discussion_r3739817889)
This paragraph describes the deterministic masternode-list comparison as an unconditional completion check. In the implementation, `PopulateAndValidateSnapshot()` writes the snapshot and background MN-list markers only when the IBD chainstate is already at the snapshot base block. Cold-start activation writes neither marker, and `MaybeCompleteSnapshotValidation()` explicitly skips the MN-list comparison when the snapshot marker is absent. Update the document to state that cold-start completion currently relies on the UTXO hash and EvoDB best-block checks, while the MN-list comparison becomes unconditional only after a later snapshot format supplies independent Dash state.
| #endif | ||
| } | ||
|
|
||
| void RenameDurably(const fs::path& src, const fs::path& dest) | ||
| { | ||
| fs::rename(src, dest); | ||
| DirectoryCommit(dest.parent_path()); | ||
| } | ||
|
|
||
| void RemoveAllDurably(const fs::path& path) | ||
| { | ||
| fs::remove_all(path); | ||
| DirectoryCommit(path.parent_path()); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Propagate directory synchronization failures from durable snapshot operations
On POSIX, DirectoryCommit() silently ignores both a failed fsbridge::fopen() and the return value from fsync(). The new RenameDurably() and RemoveAllDurably() APIs consequently report success even when the directory update was not made durable. ValidatedSnapshotCleanup() relies on that success before calling PromoteSnapshotMarkers(), so an unsuccessful directory sync can be followed by a durable EvoDB marker transition and a crash can leave the marker state inconsistent with the directory layout. Make the checked directory synchronization used by these helpers propagate failures as fs::filesystem_error; the snapshot lifecycle callers already catch that exception. Document the complete failure contract for both APIs.
source: ['coderabbit']
| // A reload of the block index is required to recompute setBlockIndexCandidates | ||
| // for the fully validated chainstate. | ||
| chainman.ActiveChainstate().ClearBlockIndexCandidates(); | ||
|
|
||
| std::tie(init_status, init_error) = CompleteChainstateInitialization(chainman, cache_sizes, options, *evodb, | ||
| dmnman, llmq_ctx, chain_helper); |
There was a problem hiding this comment.
🟡 Suggestion: Clear append-only block indexes before the post-promotion reload
CompleteChainstateInitialization() invokes ChainstateManager::LoadBlockIndex() before snapshot cleanup and invokes it again after ValidatedSnapshotCleanup(). Each load appends every parent/child relationship to BlockManager::m_prev_block_index and appends unresolved entries to m_blocks_unlinked, while ResetChainstates() does not clear either map. A successful promotion therefore retains duplicate entries for the lifetime of the process, increasing memory usage and causing descendant scans to process each child twice. Clear both auxiliary maps alongside the candidate set before the second initialization reload.
| // A reload of the block index is required to recompute setBlockIndexCandidates | |
| // for the fully validated chainstate. | |
| chainman.ActiveChainstate().ClearBlockIndexCandidates(); | |
| std::tie(init_status, init_error) = CompleteChainstateInitialization(chainman, cache_sizes, options, *evodb, | |
| dmnman, llmq_ctx, chain_helper); | |
| chainman.m_blockman.m_prev_block_index.clear(); | |
| chainman.m_blockman.m_blocks_unlinked.clear(); | |
| chainman.ActiveChainstate().ClearBlockIndexCandidates(); |
source: ['codex']
4 participants
Issue being fixed or feature implemented
M1 (#7451) added AssumeUTXO snapshot persistence and M2 (#7456) gave the snapshot and background chainstates independent EvoDB identities, markers, and chain-aware Dash validation. What was still missing is the end of the lifecycle: nothing ever completed background validation, so a snapshot-backed node stayed in the dual-chainstate state (with DKG participation and quorum signing disabled) forever.
This is milestone 3 of the AssumeUTXO series: background validation completion. When the background chainstate reaches the snapshot base block, the node now verifies the background-derived state against the snapshot, disables the background chainstate, and on the next restart promotes the snapshot chainstate (coins directory and EvoDB markers) to the normal single-chainstate layout.
What was done?
Upstream backports (kept 1:1 where practical, Dash adaptations in separate commits):
ChainstateManager::MaybeCompleteSnapshotValidation()(UTXO-set hash comparison againstm_assumeutxo_datawhen the background tip reaches the base block) andValidatedSnapshotCleanup()(restart-time promotion ofchainstate_snapshotoverchainstate), withSnapshotCompletionResultreporting and thechainstate_snapshot_INVALIDquarantine path.Chainstate::m_disabledreplaces ad-hoc usability checks,CompleteChainstateInitialization()split out ofLoadChainstate()so chainstates can be reinitialized after cleanup,LoadExternalBlockFilemoved toChainstateManager, per-blockfile undo tracking without the active-chain reference, andBLOCK_ASSUMED_VALIDdocumentation/semantics updates.Dash-specific completion path:
CEvoDB::PromoteSnapshotMarkers()atomically (single synced batch) moves the SNAPSHOT best-block marker to the legacy NORMAL key and removes all dual-chainstate metadata;DiscardSnapshotMarkers()does the same for a rejected snapshot while preserving NORMAL state. Both reset the transaction-less default identity to NORMAL, closing theTODO(assumeutxo)markers left in M2.EVODB_SNAPSHOT_MNLIST_HASH); the background chainstate independently records the list hash it derives when it connects the base block (EVODB_BACKGROUND_MNLIST_HASH). Completion compares them (in addition to the upstream UTXO-set hash) and fails withSnapshotCompletionResult::EVO_STATE_MISMATCHon divergence. This is the first installment of the holistic base-state comparison M2 deferred; extending it to the CbTxmerkleRootMNList/merkleRootQuorumsand credit-pool commitments is called out as a TODO for theloadtxoutsetmilestone, where the snapshot payload gains Dash state. Until then this comparison is a corruption tripwire, not an independent check: in the only case both markers exist (activation with the background tip already at the base) they are written from a single derivation, and a cold-start activation captures neither and skips the comparison. Divergence therefore only signals on-disk damage to the marker pair; the independent comparison arrives when the snapshot payload carries the base MN list.ValidatedSnapshotCleanup()performs two directory renames plus a marker promotion, each individually durable.RecoverSnapshotCleanup()(run at startup before chainstate detection) classifies every interruption point — first rename done, both renames done with markers pending, promotion durable but deletion pending, invalid-snapshot rename done with marker discard pending — and either rolls back, finishes the promotion, or fails with a precise error instead of the generic reindex advice.EraseSnapshotMarkers()(the abandoned-activation rollback from M2) now also erases the new MN-list-hash markers; snapshot activation moves the mempool to the snapshot chainstate and restart activation clears it from the background chainstate (the assumeutxo (2) bitcoin/bitcoin#27596 shape), so background block connects can no longer callremoveForBlock/removeExpiredAssetUnlockagainst mempool state built on the snapshot tip; the invalid-snapshot revert hands the mempool back.Review follow-ups from the M2 merge applied here:
BLOCK_DATA_UNAVAILABLE_SUFFIX) shared by every producer and the matcher.CQuorumBlockProcessor::ProcessCommitment.Review follow-ups from the #7553 review round:
ChainstateManager::LoadBlockIndex()as a normal startup failure (recoverable via the standard reindex advice), instead of aborting in candidate admission;GetSnapshotBaseBlock()regains upstream's cachedSnapshotBase()delegation (bitcoin d4a11ab) that the initial adaptation had dropped. Note the related deliberate deviation:MaybeCompleteSnapshotValidation()converts one upstream hard assert into aSKIPPEDreturn for synthetic in-memory unit fixtures, discriminated byCoinsDB().StoragePath()being empty.RenameDurably()/RemoveAllDurably()helpers (fs::rename/fs::remove_all+DirectoryCommit), so the crash-recovery invariant is enforced by the helper rather than by remembering a follow-up call at six sites.RecoverSnapshotCleanup(must land in the same end state as a completed discard), and background-MN-hash coverage in the marker rollback test.With completion wired, the M2 duty gate resolves end-to-end:
IsSnapshotActiveAndUnvalidated()becomes false at completion, so DKG participation and quorum signing re-enable without a restart, and themasternode statusclause clears.How Has This Been Tested?
ab65592f85d); every conflict was resolved against M2's final review round (thread-scoped EvoDB transactions,EraseSnapshotMarkers, reindex-time snapshot discard, fallibleDetectSnapshotChainstate, BLS scheme establishment). The merged M2 testchainstate_connectblock_bls_schemeis adapted in the Rework validation logic for assumeutxo bitcoin/bitcoin#27746 commit forAcceptBlockmoving toChainstateManager.--enable-debug), then the completetest_dashsuite passes ("No errors detected"), including targeted reruns ofevo_db_tests,validation_chainstatemanager_tests,validation_chainstate_tests,evo_deterministicmns_tests,evo_mnhf_tests,evo_assetlocks_tests,evo_cbtx_tests,blockmanager_tests,coinstatsindex_tests, andvalidation_block_tests.snapshot_marker_promotion_and_discard(promotion/discard idempotency across restarts), the extended abandoned-activation marker rollback test,chainstatemanager_snapshot_completionand_hash_mismatch(upstream-shaped), anEVO_STATE_MISMATCHcompletion case, four crash-recovery tests that each reproduce a distinctValidatedSnapshotCleanupinterruption point on disk and drive it throughLoadVerifyActivateChainstate(), and mempool-ownership assertions at both activation paths.ConnectTip→MaybeCompleteSnapshotValidationEvoDB transaction lifecycle (the scoped committer closes before completion runs, so the single-open-transaction invariant holds), BLS-scheme guard nesting across connect/disconnect, all four mempool handoff transitions, and the recovery state machine. Its two "correct but implicit" findings are addressed in the final commit (at-rest raw reads for the lifecycle markers; a comment documenting the deliberate promote/discard overlap inRecoverSnapshotCleanup).lint-circular-dependencies,lint-python, andgit diff --checkare clean.Breaking Changes
None released. The dual-chainstate on-disk state introduced in M2 (unreleased) gains two lifecycle marker keys (
b_dcs_mn,b_dcs_bg_mn); nodes that never load a snapshot never write any of them.ChainstateLoadStatus::FAILURE_FATALis a new internal failure class treated likeFAILURE_INCOMPATIBLE_DBat init.Checklist:
doc/design/assumeutxo.mdupdated for the implemented lifecycle; the user-facing AssumeUTXO documentation lands withloadtxoutset)