backport: assumeutxo M1 — snapshot persistence (bitcoin#25308, #24513, #25667, #26828)

[thepastaclaw]

Issue being fixed or feature implemented

This is milestone 1 of 7 in the stacked assumeutxo backport series. It restores the previously approved minimal snapshot-initialization groundwork needed by later milestones while preserving Dash-specific chainstate manager behavior.

bitcoin#27596 is intentionally deferred to M5, when snapshots become user-loadable through the completed assumeutxo flow. M1 does not expose loadtxoutset and does not include any partial snapshot-deletion/reindex lifecycle from that later upstream change.

What was done?

• Backported bitcoin#25308, reducing LoadChainstate parameters and return values through the upstream options/status plumbing while retaining Dash-specific options and the merged -parbls clamp.
• Backported bitcoin#24513, the mechanical CChainState to Chainstate rename across 51 files (204 additions and 204 deletions).
• Backported the historical bitcoin#25667 snapshot-initialization implementation.
• Retained the previously approved minimal Dash adaptation for bitcoin#25667: test-fixture/database-option plumbing, teardown/recreation of Dash managers and txindex around ChainstateManager replacement, and removal of a redundant cs_main declaration already supplied by validation.h.
• Backported bitcoin#26828, catching and logging snapshot-path fs::remove errors.

This branch contains no partial bitcoin#27596 behavior, crash-recovery hardening, reindex cleanup, manager-rebinding workaround, or unrelated -parbls fix. M2-M7 were not restacked by this update.

How Has This Been Tested?

• Built the unit test binary with make -C src -j"$(sysctl -n hw.ncpu)" test/test_dash.
• Passed ./src/test/test_dash --run_test=validation_chainstatemanager_tests (5 cases).
• Passed ./src/test/test_dash --run_test=validation_flush_tests (1 case).
• Passed ./src/test/test_dash --run_test=coinstatsindex_tests (2 cases).
• Passed ./src/test/test_dash --run_test=dbwrapper_tests (9 cases).
• Passed ./src/test/test_dash --run_test=blockmanager_tests (2 cases).
• ./src/test/test_dash --run_test=validation_chainstate_tests reproduces the same chainstate_update_tip abort on the exact previously approved head 183f168ef9d32a809fed0f8b51002f01bfb2bb77; this reconstruction intentionally does not change inherited approved-baseline behavior.
• The range-diff maps all five logical commits one-for-one. Commits 2, 3, and 5 retain = patch identity; commit 1 differs only by preserving the merged -parbls clamp through the LoadChainstate refactor, and commit 4 differs only by removing the redundant declaration. The sole rebase conflict was resolved transformation-faithfully in src/init.cpp.

Breaking Changes

None. This is unreleased assumeutxo groundwork; loadtxoutset remains unavailable in M1.

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone

[thepastaclaw]

⛔ Blockers found — Sonnet deferred (commit 912c108)
Canonical validated blockers: 1

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 183f168ef9

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[coderabbitai]

Caution

Review failed

The pull request is closed.

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: a256d3bb-057d-4f3c-98b1-f736915e3f48

📥 Commits

Reviewing files that changed from the base of the PR and between fe6da7c9b7a2a9085518afc3856b4f8f2f219452 and 912c108.

📒 Files selected for processing (66)

• ci/dash/lint-tidy.sh
• doc/design/assumeutxo.md
• doc/developer-notes.md
• src/Makefile.am
• src/bench/load_external.cpp
• src/bitcoin-chainstate.cpp
• src/chainlock/signing.cpp
• src/chainlock/signing.h
• src/coinjoin/client.cpp
• src/coinjoin/client.h
• src/coinjoin/coinjoin.cpp
• src/coinjoin/coinjoin.h
• src/coinjoin/walletman.cpp
• src/coinjoin/walletman.h
• src/dbwrapper.cpp
• src/dbwrapper.h
• src/index/base.cpp
• src/index/base.h
• src/init.cpp
• src/instantsend/net_instantsend.cpp
• src/instantsend/net_instantsend.h
• src/instantsend/signing.cpp
• src/instantsend/signing.h
• src/kernel/checks.cpp
• src/kernel/checks.h
• src/kernel/coinstats.cpp
• src/kernel/coinstats.h
• src/kernel/mempool_persist.cpp
• src/kernel/mempool_persist.h
• src/llmq/blockprocessor.cpp
• src/llmq/blockprocessor.h
• src/llmq/signing.h
• src/node/blockstorage.cpp
• src/node/blockstorage.h
• src/node/chainstate.cpp
• src/node/chainstate.h
• src/node/interfaces.cpp
• src/node/miner.cpp
• src/node/miner.h
• src/node/utxo_snapshot.cpp
• src/node/utxo_snapshot.h
• src/rpc/blockchain.cpp
• src/rpc/blockchain.h
• src/rpc/mempool.cpp
• src/rpc/mining.cpp
• src/rpc/rawtransaction.cpp
• src/rpc/txoutproof.cpp
• src/streams.h
• src/test/coinstatsindex_tests.cpp
• src/test/fuzz/mempool_utils.h
• src/test/fuzz/tx_pool.cpp
• src/test/net_tests.cpp
• src/test/util/chainstate.h
• src/test/util/setup_common.cpp
• src/test/util/setup_common.h
• src/test/util/validation.h
• src/test/validation_chainstate_tests.cpp
• src/test/validation_chainstatemanager_tests.cpp
• src/test/validation_flush_tests.cpp
• src/txdb.h
• src/txmempool.h
• src/validation.cpp
• src/validation.h
• test/functional/feature_init.py
• test/lint/lint-circular-dependencies.py
• test/sanitizer_suppressions/tsan

---

Walkthrough

This change renames the chainstate implementation to Chainstate, refactors chainstate loading and verification around shared option/result types, adds persistent snapshot metadata and discovery helpers, and updates snapshot activation and cleanup. It propagates the API changes through validation, node services, RPCs, integrations, tests, documentation, build configuration, and tooling. Snapshot tests cover activation, persistence, restart handling, marker failures, cleanup, and initialization.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

• dashpay/dash#7330: Modifies overlapping LoadChainstate and VerifyLoadedChainstate plumbing.
• dashpay/dash#7361: Updates IWYU source selection involving kernel and mempool persistence files.

Suggested reviewers: pastapastapasta, udjinm6, knst

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 21.30% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title is concise and accurately summarizes the main change: M1 backport work for assumeutxo snapshot persistence.
Description check	✅ Passed	The description is clearly aligned with the changeset, covering the backported assumeutxo groundwork, snapshot persistence, tests, and excluded later milestones.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/dbwrapper.h (1)

338-345: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Make StoragePath() const.

This read-only accessor should be callable through const CDBWrapper&; otherwise const database/view wrappers cannot expose the storage path without changing their own constness.

Proposed fix

-    std::optional<fs::path> StoragePath() {
+    std::optional<fs::path> StoragePath() const {

🤖 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/dbwrapper.h` around lines 338 - 345, Update the StoragePath() accessor in
CDBWrapper to be const-qualified, preserving its existing read-only behavior and
return values so it can be called through a const CDBWrapper reference.

🤖 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/init.cpp`:
- Around line 2031-2037: Update the -parbls calculation in the bls_threads
initializer to use int for the threads intermediate, including the core-count
adjustment and clamp input. Clamp the value in int, then cast the final clamped
result to int8_t when returning, preserving the existing bounds and behavior.

---

Nitpick comments:
In `@src/dbwrapper.h`:
- Around line 338-345: Update the StoragePath() accessor in CDBWrapper to be
const-qualified, preserving its existing read-only behavior and return values so
it can be called through a const CDBWrapper reference.

🪄 Autofix (Beta)

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

Run ID: bbd6e530-cc5f-4077-98be-a3ee5b67e71f

📥 Commits

Reviewing files that changed from the base of the PR and between 13b9071 and 183f168.

📒 Files selected for processing (66)

• ci/dash/lint-tidy.sh
• doc/design/assumeutxo.md
• doc/developer-notes.md
• src/Makefile.am
• src/bench/load_external.cpp
• src/bitcoin-chainstate.cpp
• src/chainlock/signing.cpp
• src/chainlock/signing.h
• src/coinjoin/client.cpp
• src/coinjoin/client.h
• src/coinjoin/coinjoin.cpp
• src/coinjoin/coinjoin.h
• src/coinjoin/walletman.cpp
• src/coinjoin/walletman.h
• src/dbwrapper.cpp
• src/dbwrapper.h
• src/index/base.cpp
• src/index/base.h
• src/init.cpp
• src/instantsend/net_instantsend.cpp
• src/instantsend/net_instantsend.h
• src/instantsend/signing.cpp
• src/instantsend/signing.h
• src/kernel/checks.cpp
• src/kernel/checks.h
• src/kernel/coinstats.cpp
• src/kernel/coinstats.h
• src/kernel/mempool_persist.cpp
• src/kernel/mempool_persist.h
• src/llmq/blockprocessor.cpp
• src/llmq/blockprocessor.h
• src/llmq/signing.h
• src/node/blockstorage.cpp
• src/node/blockstorage.h
• src/node/chainstate.cpp
• src/node/chainstate.h
• src/node/interfaces.cpp
• src/node/miner.cpp
• src/node/miner.h
• src/node/utxo_snapshot.cpp
• src/node/utxo_snapshot.h
• src/rpc/blockchain.cpp
• src/rpc/blockchain.h
• src/rpc/mempool.cpp
• src/rpc/mining.cpp
• src/rpc/rawtransaction.cpp
• src/rpc/txoutproof.cpp
• src/streams.h
• src/test/coinstatsindex_tests.cpp
• src/test/fuzz/mempool_utils.h
• src/test/fuzz/tx_pool.cpp
• src/test/net_tests.cpp
• src/test/util/chainstate.h
• src/test/util/setup_common.cpp
• src/test/util/setup_common.h
• src/test/util/validation.h
• src/test/validation_chainstate_tests.cpp
• src/test/validation_chainstatemanager_tests.cpp
• src/test/validation_flush_tests.cpp
• src/txdb.h
• src/txmempool.h
• src/validation.cpp
• src/validation.h
• test/functional/feature_init.py
• test/lint/lint-circular-dependencies.py
• test/sanitizer_suppressions/tsan

💤 Files with no reviewable changes (1)

• test/functional/feature_init.py

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bb0378e607

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 52ef36c57a

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[thepastaclaw]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[thepastaclaw]

Code Review

Policy gate: an agent-reported missing upstream prerequisite was restored as a blocking finding. This PR is a full Bitcoin backport, and omitted upstream hunks require their prerequisite PRs unless the finding is explicitly allowlisted (intentional_exclusion / policy_override). The agent's original evidence is preserved in the finding(s) below.

Prior verifier summary (overridden by policy gate): PR #7451 (assumeutxo M1/7 backport: bitcoin#25308, bitcoin#24513, bitcoin#25667, bitcoin#26828) is functionally sound at head 52ef36c57ab — no consensus-affecting defect was found. Six non-blocking issues survive independent verification against the live source and PR body: a standalone-build hazard in the squashed bitcoin#25667 commit, a latent dangling-reference bug in a test helper, two PR-body inaccuracies, an unrelated scope-creep commit, and a narrow soft-prerequisite gap around reindex-chainstate error handling. The prior bitcoin#21009/NeedsRedownload claim was refuted from first principles as SegWit-specific and architecturally inapplicable to Dash.

Source: reviewers gpt-5.6-sol and claude-sonnet-5 (general, dash-core-commit-history, backport-reviewer); verifier claude-sonnet-5.

Note: GitHub does not allow PastaClaw to submit REQUEST_CHANGES on its own PR, so the canonical blocking verdict is posted as a COMMENT.

🔴 9 blocking | 🟡 3 suggestion(s) | 💬 3 nitpick(s)

5 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/test/util/setup_common.h`:
- [SUGGESTION] src/test/util/setup_common.h:221-224: Squashed bitcoin#25667 merge commit (3ed61a861ad) does not build standalone
  Verified directly: at commit 3ed61a861ad, src/test/validation_chainstatemanager_tests.cpp's SnapshotTestSetup constructor calls `TestChain100Setup{{}, {}, /*coins_db_in_memory=*/false, /*block_tree_db_in_memory=*/false}` (4 arguments), but that exact commit's setup_common.h:222 declares only the 2-argument `TestChain100Setup(chain_name, extra_args)`. Checking out 3ed61a861ad alone is a compile error — the following commit, cc49052fb15, is required to widen the constructor. This breaks `git bisect` across that boundary and violates the project's own commit-atomicity convention (CLAUDE.md: 'Each commit should build and make sense on its own').
- [BLOCKING] src/test/util/setup_common.h:221-224: [Carried forward from p2, STILL VALID under new SHA] 3ed61a861ad (bitcoin#25667 merge) still does not build standalone; cc49052fb15 still exists only to repair it
  The branch was rebased since the last review — the prior-reported commit 19c3ee07885 no longer exists in this branch's ancestry, replaced by 3ed61a861ad (content-diffed and confirmed near-identical plus the new review-follow-up hunks described in the finding above). The underlying defect is unchanged: at 3ed61a861ad, src/test/validation_chainstatemanager_tests.cpp's `SnapshotTestSetup() : TestChain100Setup{{}, {}, /*coins_db_in_memory=*/false, /*block_tree_db_in_memory=*/false}` calls a 4-argument constructor, but that same commit's src/test/util/setup_common.h:222 still declares only `TestChain100Setup(const std::string& chain_name = ..., const std::vector<const char*>& extra_args = {})` (2 parameters) — a compile error if checked out alone. The next commit, cc49052fb15 ('backport: adapt Dash chainstate init and tests for bitcoin#25667', the rebased successor of ea68d3b2ed0), widens the constructor to 4 parameters. Squash the constructor-signature widening into 3ed61a861ad so every commit in the stack builds independently and git bisect remains usable.

  ---
  **Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.

In `src/test/validation_chainstatemanager_tests.cpp`:
- [SUGGESTION] src/test/validation_chainstatemanager_tests.cpp:387-421: SimulateNodeRestart() leaves clhandler and peerman holding dangling ChainstateManager references
  Verified directly: SimulateNodeRestart() calls DashChainstateSetupClose(m_node) (src/node/chainstate.cpp:233-245), which only resets chain_helper, llmq_ctx, and dmnman, then replaces m_node.chainman with a new ChainstateManager (line 419). It never touches m_node.clhandler or m_node.peerman. Both chainlock::ChainlockHandler::m_chainman (src/chainlock/handler.h:47) and PeerManagerImpl::m_chainman (src/net_processing.cpp:821) are ChainstateManager& reference members bound once at fixture construction — after restart, both still reference the destroyed manager. Currently latent (neither component is exercised through this path in the test), but a future test invoking clhandler/peerman after SimulateNodeRestart() would use freed memory. Recreate or reset both alongside the chainman replacement.
- [BLOCKING] src/test/validation_chainstatemanager_tests.cpp:387-421: Restart adaptation retains references to the destroyed ChainstateManager
  STILL VALID; carried forward from p2. SimulateNodeRestart() stops the txindex, resets governance and the Dash chainstate helper hierarchy, then destroys and replaces m_node.chainman. DashChainstateSetupClose() resets chain_helper, llmq_ctx, and dmnman, but does not reset m_node.clhandler or m_node.peerman. Those objects retain ChainstateManager references established against the destroyed instance. The latest delta expands restart coverage but does not repair these references. Recreate or clear both manager-dependent objects around the replacement.

  ---
  **Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.

In `src/node/chainstate.cpp`:
- [SUGGESTION] src/node/chainstate.cpp:64-69: Reindex-time snapshot-deletion failure returns generic FAILURE instead of upstream's FAILURE_FATAL, with a narrow user-facing quirk for -reindex-chainstate-only runs
  Dash's ChainstateLoadStatus enum (src/node/chainstate.h:56) has no FAILURE_FATAL value — that's added upstream by bitcoin#27862, which this PR correctly defers to M3 (it depends on bitcoin#25740's SnapshotCompletionResult). The adapted code at chainstate.cpp:69 returns plain FAILURE when snapshot deletion fails during a reindex. I verified the actual behavioral impact in src/init.cpp:2076-2112: when `-reindex` is set, FAILURE and FAILURE_INCOMPATIBLE_DB both short-circuit to InitError() with no difference in practice, so the originally-claimed 'prompts to rebuild while already reindexing' UX bug does NOT occur for plain -reindex. It does occur, narrowly, when only `-reindex-chainstate` (not `-reindex`) is passed and snapshot deletion fails: `!options.reindex` is then true, so the code prompts 'restart with -reindex or -reindex-chainstate to recover' even though -reindex-chainstate was already what triggered the failure. This is a real but narrow, non-crashing edge case tied to the already-acknowledged M3 deferral, not a new defect introduced by this PR's own code.
- [BLOCKING] src/node/chainstate.cpp:173-180: Missing prerequisite: bitcoin#21009
  STILL VALID; consolidates p5 and p7. Upstream bitcoin#25308 starts from a LoadChainstate path containing Chainstate::NeedsRedownload() and the witness-insufficiently-validated result introduced by bitcoin#21009 (merge 19a56d1519f, implementation commit d831e711cab). bitcoin#25308 transforms that check into the new ChainstateLoadResult form. Dash's pre-PR and current paths instead leave the chainstate-loading loop at lines 173-180 and proceed directly to Dash database migrations, with no NeedsRedownload check or explicit PR-description exclusion for bitcoin#21009. Dash lacks SegWit machinery, but the authoritative classification rule says an absent subsystem is not by itself evidence of intentional divergence when a PR claims the complete upstream PR. Backport bitcoin#21009 or explicitly document this permanent Dash-specific exclusion.

  ---
  **Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
- [BLOCKING] src/node/chainstate.cpp:64-69: New in this delta: reindex-time snapshot deletion (adapted from bitcoin#27596/c711ca186f8) silently downgrades FAILURE_FATAL to FAILURE, a soft prerequisite gap from bitcoin#27862
  This code block is new since the last review (it does not exist in the prior-reviewed commit; independently confirmed via `diff <(git show 3ed61a861ad) <(git show 19c3ee07885)`, which shows it introduced under a 'Review follow-ups' note in the #25667 merge commit message: 'Adapt c711ca186f8 from bitcoin#27596 so reindex modes delete a persisted snapshot before database initialization'). Upstream's actual c711ca186f8 (bitcoin#27596, 'assumeutxo: remove snapshot during -reindex{-chainstate}') returns `ChainstateLoadStatus::FAILURE_FATAL` on deletion failure: `return {ChainstateLoadStatus::FAILURE_FATAL, Untranslated("Couldn't remove snapshot chainstate.")};`. Dash's adaptation at line 69 instead returns `ChainstateLoadStatus::FAILURE` because Dash's `enum class ChainstateLoadStatus` (src/node/chainstate.h) only has `{SUCCESS, FAILURE, FAILURE_INCOMPATIBLE_DB, INTERRUPTED}` — no `FAILURE_FATAL`. Traced the origin: `FAILURE_FATAL` is added to the enum by bitcoin#27862 commit 9047337d369d800e6eca4d3b686139073a8e8905 ('validation: Stricter assumeutxo error handling in LoadChainstate'), confirmed via `--follow` history pickaxe on src/node/chainstate.h. That same commit references `SnapshotCompletionResult::SUCCESS` (from bitcoin#25740), so bitcoin#27862 genuinely cannot be cleanly backported without #25740 — consistent with, and validating, this PR's own stated reason for deferring #27862 to M3. The practical effect on Dash's current code: in src/init.cpp:2089-2097, `FAILURE_INCOMPATIBLE_DB` is special-cased to abort immediately, but plain `FAILURE` (which this new reindex-deletion failure now returns) falls through to the generic path that prompts the user 'Do you want to rebuild the block database now? ... restart with -reindex or -reindex-chainstate to recover' — even though the failure occurred while a `-reindex`/`-reindex-chainstate` request was already being processed (retrying the same flag cannot fix a stuck/locked snapshot-directory deletion). This is a real, if narrow and non-crashing, UX/behavior degradation traceable to a missing prerequisite (bitcoin#27862), not a Dash-specific design choice. Given #25740/#27862 are correctly out of M1 scope, the pragmatic fix is not to backport them here, but to document this specific known limitation (either in the PR body's existing #27862 exclusion note, or as a code comment at line 69) so it isn't mistaken for an oversight in a later milestone.

  ---
  **Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.

In `(PR description)`:
- [BLOCKING] (PR description): Backport-prerequisite items (bitcoin#21009/NeedsRedownload, bitcoin#21110 chrono check) — out of this role's scope entirely
  Per review.md, prerequisite analysis is owned exclusively by the backport-reviewer specialist. Noting only that the PR body now contains new audit text ('bitcoin#21585 was audited and found already present...', 'bitcoin#27862 ... moved to M3', and the existing '### Intentional exclusion: bitcoin#21110' section) that appears responsive to some of the prior policy-restored prerequisite findings; the backport-reviewer lane should make the actual determination at this exact head.

  ---
  **Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
- [BLOCKING] (PR description): Snapshot persistence remains attributed to the wrong upstream PR
  STILL VALID; consolidates p3, p6, and p9. The supplied PR context continues to attribute the chainstate_snapshot directory and persistence to bitcoin#25308. Upstream bitcoin#25308 only introduces the LoadChainstate options, status, and result refactor. The directory rename and persisted base_blockhash come from bitcoin#25667, including f9f1735f139. The implementation is present, so this is a metadata issue rather than a missing code prerequisite; move the persistence wording to the bitcoin#25667 entry.

  ---
  **Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.

In `src/validation.cpp`:
- [BLOCKING] src/validation.cpp:5822-5829: Missing prerequisite: bitcoin#25740 before the bitcoin#27596 follow-up
  NEW IN THE LATEST DELTA. Commit 3ed61a861ad explicitly imports c711ca186f8 from bitcoin#27596. That upstream commit starts after bitcoin#25740 (merge d5e4f9a4395), which factors chainstate initialization and adds background-validation completion state. Dash has not backported that starting state, so DeleteSnapshotChainstate is rewritten to discover the directory directly because the snapshot has no initialized CoinsViews. The current comment states this difference explicitly, making it a workaround for not-yet-backported upstream content rather than a Dash architectural divergence. The adaptation is coherent and tested, so this is a soft, non-blocking per the agent (restored to blocking by policy) prerequisite gap; document the partial bitcoin#27596 backport and its bitcoin#25740 dependency, or stage it after bitcoin#25740.

  ---
  **Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.

In `<commit:3ed61a861ad>`:
- [BLOCKING] <commit:3ed61a861ad>:1: The bitcoin#25667 commit is not independently buildable
  STILL VALID; carried forward from p1 with rewritten commit hashes. At 3ed61a861ad, SnapshotTestSetup calls TestChain100Setup with four arguments, while that exact commit's src/test/util/setup_common.h still declares only the two-argument constructor. The following Dash adaptation commit cc49052fb15 adds the database-option parameters. The final head contains the repair, but the upstream backport commit itself cannot compile. Move the required constructor adaptation into 3ed61a861ad or otherwise restructure the pair so every permanent commit builds independently.

  ---
  **Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.

In `src/init.cpp`:
- [BLOCKING] src/init.cpp:2031-2037: The unrelated parbls fix remains outside the advertised backport scope
  STILL VALID; carried forward from p4. The tail commit 52ef36c57ab fixes pre-existing -parbls truncation but is not part of bitcoin#25308, #24513, #25667, or #26828. The fix itself introduces no prerequisite gap. Mention it in the PR description or move it to a separate change so the advertised backport ledger matches the reachable commit stack.

  ---
  **Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.

[thepastaclaw]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ef11d9069a

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[coderabbitai]

🧹 Nitpick comments (1)

src/dbwrapper.h (1)

338-344: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider marking StoragePath() const.

It's a pure accessor with no mutation, consistent with Read/Exists/etc. which are already const.

♻️ Proposed fix

-    std::optional<fs::path> StoragePath() {
+    std::optional<fs::path> StoragePath() const {

🤖 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/dbwrapper.h` around lines 338 - 344, Mark the StoragePath() accessor as
const, matching the existing const-qualified read-only methods such as Read and
Exists, without changing its return behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/dbwrapper.h`:
- Around line 338-344: Mark the StoragePath() accessor as const, matching the
existing const-qualified read-only methods such as Read and Exists, without
changing its return behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: fdfa8010-0fea-412f-b735-5423f14bf082

📥 Commits

Reviewing files that changed from the base of the PR and between 52ef36c57ab4ad043ed055a9fd450a7729296145 and ef11d9069a14115ec39a9814d5ef792c60a8cbe8.

📒 Files selected for processing (20)

• doc/design/assumeutxo.md
• src/Makefile.am
• src/dbwrapper.cpp
• src/dbwrapper.h
• src/init.cpp
• src/node/blockstorage.cpp
• src/node/chainstate.cpp
• src/node/utxo_snapshot.cpp
• src/node/utxo_snapshot.h
• src/streams.h
• src/test/util/chainstate.h
• src/test/util/setup_common.cpp
• src/test/util/setup_common.h
• src/test/validation_chainstate_tests.cpp
• src/test/validation_chainstatemanager_tests.cpp
• src/txdb.h
• src/validation.cpp
• src/validation.h
• test/functional/feature_init.py
• test/lint/lint-circular-dependencies.py

💤 Files with no reviewable changes (1)

• test/functional/feature_init.py

🚧 Files skipped from review as they are similar to previous changes (11)

• src/dbwrapper.cpp
• test/lint/lint-circular-dependencies.py
• src/txdb.h
• doc/design/assumeutxo.md
• src/node/blockstorage.cpp
• src/streams.h
• src/node/utxo_snapshot.h
• src/node/chainstate.cpp
• src/test/util/chainstate.h
• src/validation.h
• src/validation.cpp

[thepastaclaw]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7d207dc5f4

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[coderabbitai]

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/test/validation_chainstatemanager_tests.cpp`:
- Around line 400-435: Update SimulateNodeRestart to reset m_node.peerman before
destroying and recreating m_node.chainman. Ensure the peer manager is torn down
while the existing ChainstateManager is still valid, before
m_node.chainman.reset(new ChainstateManager(...)), so no validation-interface
callbacks can use a dangling ChainstateManager reference.

🪄 Autofix (Beta)

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

Run ID: 0ecc9e85-2c30-4cb8-b41b-e71425f1b06f

📥 Commits

Reviewing files that changed from the base of the PR and between ef11d9069a14115ec39a9814d5ef792c60a8cbe8 and 7d207dc5f42a23abd76d26cadee58d375efb8f60.

📒 Files selected for processing (20)

• doc/design/assumeutxo.md
• src/Makefile.am
• src/dbwrapper.cpp
• src/dbwrapper.h
• src/init.cpp
• src/node/blockstorage.cpp
• src/node/chainstate.cpp
• src/node/utxo_snapshot.cpp
• src/node/utxo_snapshot.h
• src/streams.h
• src/test/util/chainstate.h
• src/test/util/setup_common.cpp
• src/test/util/setup_common.h
• src/test/validation_chainstate_tests.cpp
• src/test/validation_chainstatemanager_tests.cpp
• src/txdb.h
• src/validation.cpp
• src/validation.h
• test/functional/feature_init.py
• test/lint/lint-circular-dependencies.py

💤 Files with no reviewable changes (1)

• test/functional/feature_init.py

🚧 Files skipped from review as they are similar to previous changes (11)

• src/Makefile.am
• src/streams.h
• src/txdb.h
• doc/design/assumeutxo.md
• src/dbwrapper.cpp
• src/node/blockstorage.cpp
• src/node/utxo_snapshot.h
• src/test/validation_chainstate_tests.cpp
• src/node/chainstate.cpp
• src/init.cpp
• src/validation.cpp

[thepastaclaw]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[thepastaclaw]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[github-actions]

This pull request has conflicts, please rebase.

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[PastaPastaPasta]

utACK 912c108

[PastaPastaPasta]

I believe some of the automated code review from rabbit / openai are flagging issues bitcoin fixes later; and we fix in later prs in the stacked series.

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[thepastaclaw]

Preliminary review — Codex only

At exact HEAD 912c108, one blocking commit-history defect remains: cc4903f uses constructor parameters added only by a1a2448. SimulateNodeRestart() also leaves ChainlockHandler and PeerManager bound to the destroyed ChainstateManager; the PeerManager regression was introduced by the latest rebase. The historical review artifact was read, the checkout and commit boundaries were verified, and git diff --check passed. No local test binary was available.

Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (failed), gpt-5.6-sol — dash-core-commit-history (failed), gpt-5.6-sol — backport-reviewer (failed), 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 | 🟡 2 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 `src/test/validation_chainstatemanager_tests.cpp`:
- [BLOCKING] src/test/validation_chainstatemanager_tests.cpp:189-194: bitcoin#25667 cherry-pick is not independently buildable
  Commit cc4903f2b85 constructs SnapshotTestSetup with four TestChain100Setup arguments, but that commit's setup_common.h declares only the two-argument constructor. The required coins_db_in_memory and block_tree_db_in_memory plumbing first appears in a1a24480661. Upstream merge 6912a28f08c included the fixture call and constructor widening in one transformation, so checking out or bisecting to the permanent bitcoin#25667 import produces a compile error. Move the constructor-plumbing hunks into cc4903f2b85 or place them in a coherent preparatory commit before it.
- [SUGGESTION] src/test/validation_chainstatemanager_tests.cpp:387-419: Dash restart adaptation leaves ChainlockHandler bound to the destroyed manager
  SimulateNodeRestart() destroys m_node.chainman without destroying or recreating m_node.clhandler. ChainTestingSetup constructs that handler with the original ChainstateManager&, ChainlockHandler stores it as the direct reference m_chainman, and DashChainstateSetupClose() does not own the handler. The current test does not exercise a chainstate-dependent ChainLocks operation after restart, but the fixture retains a dangling reference. Stop and reset the handler before replacing the manager, then recreate it against the replacement if the restarted fixture needs ChainLocks.
- [SUGGESTION] src/test/validation_chainstatemanager_tests.cpp:387-419: Latest rebase restores a dangling PeerManager reference
  The 5686926e92e..912c10871b1 delta removes m_node.peerman.reset() before m_node.chainman is destroyed and replaced. PeerManagerImpl stores the original manager as the direct reference m_chainman, and LoadVerifyActivateChainstate() does not recreate PeerManager. The current test does not invoke peer processing after restart, but the fixture leaves PeerManager attached to freed state. Reset PeerManager while the old manager is alive and recreate it, including the connection-manager message-processor binding, if later fixture operations require peer processing.

GitHub does not permit an author to request changes on its own PR. This review is posted as a comment; the preliminary gate remains blocked by the validated blocker above.

[thepastaclaw]

🔴 Blocking: bitcoin#25667 cherry-pick is not independently buildable

Commit cc4903f constructs SnapshotTestSetup with four TestChain100Setup arguments, but that commit's setup_common.h declares only the two-argument constructor. The required coins_db_in_memory and block_tree_db_in_memory plumbing first appears in a1a2448. Upstream merge 6912a28 included the fixture call and constructor widening in one transformation, so checking out or bisecting to the permanent bitcoin#25667 import produces a compile error. Move the constructor-plumbing hunks into cc4903f or place them in a coherent preparatory commit before it.

_{source: ['codex']}

[thepastaclaw]

🟡 Suggestion: Dash restart adaptation leaves ChainlockHandler bound to the destroyed manager

SimulateNodeRestart() destroys m_node.chainman without destroying or recreating m_node.clhandler. ChainTestingSetup constructs that handler with the original ChainstateManager&, ChainlockHandler stores it as the direct reference m_chainman, and DashChainstateSetupClose() does not own the handler. The current test does not exercise a chainstate-dependent ChainLocks operation after restart, but the fixture retains a dangling reference. Stop and reset the handler before replacing the manager, then recreate it against the replacement if the restarted fixture needs ChainLocks.

_{source: ['codex']}

[thepastaclaw]

🟡 Suggestion: Latest rebase restores a dangling PeerManager reference

The 5686926e92e..912c108 delta removes m_node.peerman.reset() before m_node.chainman is destroyed and replaced. PeerManagerImpl stores the original manager as the direct reference m_chainman, and LoadVerifyActivateChainstate() does not recreate PeerManager. The current test does not invoke peer processing after restart, but the fixture leaves PeerManager attached to freed state. Reset PeerManager while the old manager is alive and recreate it, including the connection-manager message-processor binding, if later fixture operations require peer processing.

_{source: ['codex']}

[knst]

post ACK 912c108, follow-up PR with fixes created: #7471