backport: assumeutxo M3 — background validation completion and snapshot promotion - #7553
backport: assumeutxo M3 — background validation completion and snapshot promotion#7553PastaPastaPasta wants to merge 12 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
🔍 Review in progress — actively reviewing now (commit e3a8989) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis change moves block acceptance, external block loading, candidate handling, and block-index checks to Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Node
participant ChainstateManager
participant BackgroundChainstate
participant CEvoDB
Node->>ChainstateManager: load chainstates
ChainstateManager->>BackgroundChainstate: validate snapshot
BackgroundChainstate->>ChainstateManager: reach validation tip
ChainstateManager->>CEvoDB: verify and promote markers
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
3 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.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.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)