backport: assumeutxo M1 — snapshot persistence (bitcoin#25308, #24513, #25667, #26828)#7451
Conversation
|
⛔ Blockers found — Sonnet deferred (commit 912c108) |
There was a problem hiding this comment.
💡 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".
|
Caution Review failedThe pull request is closed. Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (66)
WalkthroughThis change renames the chainstate implementation to Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/dbwrapper.h (1)
338-345: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
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
📒 Files selected for processing (66)
ci/dash/lint-tidy.shdoc/design/assumeutxo.mddoc/developer-notes.mdsrc/Makefile.amsrc/bench/load_external.cppsrc/bitcoin-chainstate.cppsrc/chainlock/signing.cppsrc/chainlock/signing.hsrc/coinjoin/client.cppsrc/coinjoin/client.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/coinjoin.hsrc/coinjoin/walletman.cppsrc/coinjoin/walletman.hsrc/dbwrapper.cppsrc/dbwrapper.hsrc/index/base.cppsrc/index/base.hsrc/init.cppsrc/instantsend/net_instantsend.cppsrc/instantsend/net_instantsend.hsrc/instantsend/signing.cppsrc/instantsend/signing.hsrc/kernel/checks.cppsrc/kernel/checks.hsrc/kernel/coinstats.cppsrc/kernel/coinstats.hsrc/kernel/mempool_persist.cppsrc/kernel/mempool_persist.hsrc/llmq/blockprocessor.cppsrc/llmq/blockprocessor.hsrc/llmq/signing.hsrc/node/blockstorage.cppsrc/node/blockstorage.hsrc/node/chainstate.cppsrc/node/chainstate.hsrc/node/interfaces.cppsrc/node/miner.cppsrc/node/miner.hsrc/node/utxo_snapshot.cppsrc/node/utxo_snapshot.hsrc/rpc/blockchain.cppsrc/rpc/blockchain.hsrc/rpc/mempool.cppsrc/rpc/mining.cppsrc/rpc/rawtransaction.cppsrc/rpc/txoutproof.cppsrc/streams.hsrc/test/coinstatsindex_tests.cppsrc/test/fuzz/mempool_utils.hsrc/test/fuzz/tx_pool.cppsrc/test/net_tests.cppsrc/test/util/chainstate.hsrc/test/util/setup_common.cppsrc/test/util/setup_common.hsrc/test/util/validation.hsrc/test/validation_chainstate_tests.cppsrc/test/validation_chainstatemanager_tests.cppsrc/test/validation_flush_tests.cppsrc/txdb.hsrc/txmempool.hsrc/validation.cppsrc/validation.htest/functional/feature_init.pytest/lint/lint-circular-dependencies.pytest/sanitizer_suppressions/tsan
💤 Files with no reviewable changes (1)
- test/functional/feature_init.py
ff6b46b to
bb0378e
Compare
There was a problem hiding this comment.
💡 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".
bb0378e to
52ef36c
Compare
There was a problem hiding this comment.
💡 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".
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
There was a problem hiding this comment.
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 52ef36c — 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.
52ef36c to
ef11d90
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 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".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/dbwrapper.h (1)
338-344: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider marking
StoragePath()const.It's a pure accessor with no mutation, consistent with
Read/Exists/etc. which are alreadyconst.♻️ 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
📒 Files selected for processing (20)
doc/design/assumeutxo.mdsrc/Makefile.amsrc/dbwrapper.cppsrc/dbwrapper.hsrc/init.cppsrc/node/blockstorage.cppsrc/node/chainstate.cppsrc/node/utxo_snapshot.cppsrc/node/utxo_snapshot.hsrc/streams.hsrc/test/util/chainstate.hsrc/test/util/setup_common.cppsrc/test/util/setup_common.hsrc/test/validation_chainstate_tests.cppsrc/test/validation_chainstatemanager_tests.cppsrc/txdb.hsrc/validation.cppsrc/validation.htest/functional/feature_init.pytest/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
ef11d90 to
7d207dc
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 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".
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/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
📒 Files selected for processing (20)
doc/design/assumeutxo.mdsrc/Makefile.amsrc/dbwrapper.cppsrc/dbwrapper.hsrc/init.cppsrc/node/blockstorage.cppsrc/node/chainstate.cppsrc/node/utxo_snapshot.cppsrc/node/utxo_snapshot.hsrc/streams.hsrc/test/util/chainstate.hsrc/test/util/setup_common.cppsrc/test/util/setup_common.hsrc/test/validation_chainstate_tests.cppsrc/test/validation_chainstatemanager_tests.cppsrc/txdb.hsrc/validation.cppsrc/validation.htest/functional/feature_init.pytest/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
|
@coderabbitai review |
✅ Action performedReview finished.
|
7d207dc to
4a319cd
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d49910f32e
ℹ️ 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".
| if (!m_ibd_chainstate->FlushStateToDisk(rollback_state, FlushStateMode::ALWAYS)) { | ||
| AbortNode(strprintf("Failed to roll back snapshot block index metadata: %s", rollback_state.ToString())); |
There was a problem hiding this comment.
If the marker write fails after the prior FlushStateToDisk() has made the fake nTx/BLOCK_ASSUMED_VALID block-index metadata durable, and this rollback flush also fails (for example due to ENOSPC or an I/O error), AbortNode() only requests shutdown and execution continues into the cleanup block below, which deletes chainstate_snapshot. The next restart can then have blocks/index entries requiring an assumed-valid snapshot but no snapshot directory for recovery or -reindex-chainstate cleanup. Return before deleting the snapshot directory, or otherwise preserve a recoverable marker/dir when the rollback cannot be made durable.
Useful? React with 👍 / 👎.
✅ Action performedReview finished.
|
d49910f to
fe6da7c
Compare
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe6da7c9b7
ℹ️ 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".
| if (!in_memory) { | ||
| LOCK(::cs_main); | ||
| if (!node::WriteSnapshotBaseBlockhash(*snapshot_chainstate)) { |
There was a problem hiding this comment.
When loading a disk-backed snapshot, this publishes chainstate_snapshot/base_blockhash immediately after PopulateAndValidateSnapshot(), but that path only writes the snapshot coins through normal LevelDB batches and never forces the snapshot coins DB durable before the marker is written. After a crash or power loss where the marker reaches disk but the snapshot LevelDB rolls back or is empty, the next startup will detect the marker and activate an unusable snapshot chainstate instead of falling back to IBD. Please force/sync the snapshot coins DB before writing the marker, or defer publishing the marker until the snapshot DB durability is guaranteed.
Useful? React with 👍 / 👎.
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/validation.cpp`:
- Around line 5459-5469: Update the persistence condition in the snapshot
validation flow to require both snapshot_ok and !in_memory before calling
node::WriteSnapshotBaseBlockhash. Keep the existing failure handling that sets
snapshot_ok to false when the write fails, and ensure failed snapshot loads
never persist the base_blockhash marker.
🪄 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: 490be220-35a2-4b65-80e0-e04dd0b32db1
📒 Files selected for processing (66)
ci/dash/lint-tidy.shdoc/design/assumeutxo.mddoc/developer-notes.mdsrc/Makefile.amsrc/bench/load_external.cppsrc/bitcoin-chainstate.cppsrc/chainlock/signing.cppsrc/chainlock/signing.hsrc/coinjoin/client.cppsrc/coinjoin/client.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/coinjoin.hsrc/coinjoin/walletman.cppsrc/coinjoin/walletman.hsrc/dbwrapper.cppsrc/dbwrapper.hsrc/index/base.cppsrc/index/base.hsrc/init.cppsrc/instantsend/net_instantsend.cppsrc/instantsend/net_instantsend.hsrc/instantsend/signing.cppsrc/instantsend/signing.hsrc/kernel/checks.cppsrc/kernel/checks.hsrc/kernel/coinstats.cppsrc/kernel/coinstats.hsrc/kernel/mempool_persist.cppsrc/kernel/mempool_persist.hsrc/llmq/blockprocessor.cppsrc/llmq/blockprocessor.hsrc/llmq/signing.hsrc/node/blockstorage.cppsrc/node/blockstorage.hsrc/node/chainstate.cppsrc/node/chainstate.hsrc/node/interfaces.cppsrc/node/miner.cppsrc/node/miner.hsrc/node/utxo_snapshot.cppsrc/node/utxo_snapshot.hsrc/rpc/blockchain.cppsrc/rpc/blockchain.hsrc/rpc/mempool.cppsrc/rpc/mining.cppsrc/rpc/rawtransaction.cppsrc/rpc/txoutproof.cppsrc/streams.hsrc/test/coinstatsindex_tests.cppsrc/test/fuzz/mempool_utils.hsrc/test/fuzz/tx_pool.cppsrc/test/net_tests.cppsrc/test/util/chainstate.hsrc/test/util/setup_common.cppsrc/test/util/setup_common.hsrc/test/util/validation.hsrc/test/validation_chainstate_tests.cppsrc/test/validation_chainstatemanager_tests.cppsrc/test/validation_flush_tests.cppsrc/txdb.hsrc/txmempool.hsrc/validation.cppsrc/validation.htest/functional/feature_init.pytest/lint/lint-circular-dependencies.pytest/sanitizer_suppressions/tsan
💤 Files with no reviewable changes (1)
- test/functional/feature_init.py
🚧 Files skipped from review as they are similar to previous changes (49)
- src/test/util/validation.h
- test/lint/lint-circular-dependencies.py
- src/test/fuzz/mempool_utils.h
- src/test/validation_flush_tests.cpp
- ci/dash/lint-tidy.sh
- src/coinjoin/coinjoin.cpp
- src/bench/load_external.cpp
- src/instantsend/signing.cpp
- src/Makefile.am
- src/node/miner.h
- src/coinjoin/coinjoin.h
- src/node/interfaces.cpp
- src/node/blockstorage.h
- src/kernel/checks.h
- src/chainlock/signing.h
- src/chainlock/signing.cpp
- src/test/net_tests.cpp
- src/txdb.h
- src/rpc/rawtransaction.cpp
- src/llmq/blockprocessor.cpp
- src/rpc/mining.cpp
- src/instantsend/net_instantsend.h
- src/streams.h
- src/dbwrapper.cpp
- src/node/miner.cpp
- src/coinjoin/walletman.h
- src/rpc/txoutproof.cpp
- src/test/fuzz/tx_pool.cpp
- doc/developer-notes.md
- src/rpc/blockchain.h
- src/coinjoin/client.cpp
- src/coinjoin/walletman.cpp
- src/llmq/signing.h
- src/txmempool.h
- src/index/base.h
- src/coinjoin/client.h
- src/test/validation_chainstate_tests.cpp
- src/kernel/mempool_persist.h
- src/kernel/mempool_persist.cpp
- src/instantsend/net_instantsend.cpp
- src/instantsend/signing.h
- src/index/base.cpp
- src/test/coinstatsindex_tests.cpp
- src/node/chainstate.h
- src/rpc/mempool.cpp
- src/dbwrapper.h
- src/kernel/coinstats.h
- src/rpc/blockchain.cpp
- src/init.cpp
| bool snapshot_ok = this->PopulateAndValidateSnapshot( | ||
| *snapshot_chainstate, coins_file, metadata); | ||
|
|
||
| // If not in-memory, persist the base blockhash for use during subsequent | ||
| // initialization. | ||
| if (!in_memory) { | ||
| LOCK(::cs_main); | ||
| if (!node::WriteSnapshotBaseBlockhash(*snapshot_chainstate)) { | ||
| snapshot_ok = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and locate related symbols.
ast-grep outline src/validation.cpp --view expanded | sed -n '1,220p'
printf '\n--- matches ---\n'
rg -n "WriteSnapshotBaseBlockhash|DetectSnapshotChainstate|DeleteCoinsDBFromDisk|PopulateAndValidateSnapshot|base_blockhash|snapshot_ok" src/validation.cpp src -SRepository: dashpay/dash
Length of output: 18726
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section of validation.cpp.
nl -ba src/validation.cpp | sed -n '5435,5515p'
printf '\n--- write marker implementation ---\n'
rg -n "bool WriteSnapshotBaseBlockhash|WriteSnapshotBaseBlockhash\\(" src -S
printf '\n--- detect snapshot chainstate ---\n'
rg -n "DetectSnapshotChainstate\\(" src -SRepository: dashpay/dash
Length of output: 190
🌐 Web query:
site:github.com dash core validation.cpp DetectSnapshotChainstate WriteSnapshotBaseBlockhash base_blockhash snapshot chainstate
💡 Result:
In Dash Core and similar Bitcoin-derived codebases, the components DetectSnapshotChainstate, WriteSnapshotBaseBlockhash, and the base_blockhash itself are part of the assumeutxo (UTXO snapshot) infrastructure [1][2][3]. DetectSnapshotChainstate This is a function used during node startup to search the data directory for an existing chainstate directory derived from a UTXO snapshot [2][4]. If such a directory is found, the node loads it as the active chainstate [2][4]. This allows the node to begin operation from a pre-validated UTXO set rather than re-syncing the entire blockchain from genesis [2][3]. base_blockhash The base_blockhash represents the hash of the specific block that serves as the foundation for the snapshot [5][3]. When a snapshot is created, the resulting UTXO set reflects the state of the chain at that specific block [5]. This hash is critical for identifying the snapshot and ensuring that the node correctly validates subsequent blocks starting from that height [3]. WriteSnapshotBaseBlockhash This function is responsible for persisting the base_blockhash to a file named base_blockhash within the snapshot's chainstate directory [2][5][6]. During subsequent node restarts, the node reads this file to reconstruct the identity of the snapshot-based chainstate [5][3]. In summary, when a snapshot-based chainstate is initialized, it is stored in a specific directory [2][7]. The WriteSnapshotBaseBlockhash function saves the snapshot's base block hash into a file within that directory [3][6]. Upon restarting, the node uses DetectSnapshotChainstate to locate the directory and read this hash, ensuring the chainstate is correctly reinitialized and aligned with the intended block height [2][4][3].
Citations:
- 1: https://github.com/bitcoin/bitcoin/blob/eef19c4ce2f85118e439a0d3165f46075c20ca1f/src/validation.h
- 2: bitcoin-core/gui@6912a28
- 3: https://github.com/bitcoin/bitcoin/blob/a11585692e72cac468fb1496ea2c30e4c07f73e5/doc/design/assumeutxo.md
- 4: jonatack@252abd1
- 5: https://github.com/bitcoin/bitcoin/blob/d7ed2840/src/node/utxo_snapshot.h
- 6: https://github.com/bitcoin/bitcoin/blob/d7ed2840/src/node/utxo_snapshot.cpp
- 7: https://github.com/bitcoin/bitcoin/blob/45719390a1434ad7377a5ed05dcd73028130cf2d/doc/design/assumeutxo.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant region with line numbers.
sed -n '5448,5498p' src/validation.cpp | cat -n
printf '\n--- surrounding failure cleanup ---\n'
sed -n '5498,5575p' src/validation.cpp | cat -nRepository: dashpay/dash
Length of output: 6110
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- utxo_snapshot.cpp ---'
sed -n '1,140p' src/node/utxo_snapshot.cpp | cat -n
printf '\n%s\n' '--- DetectSnapshotChainstate ---'
sed -n '5830,5865p' src/validation.cpp | cat -n
printf '\n%s\n' '--- DeleteCoinsDBFromDisk ---'
sed -n '5358,5398p' src/validation.cpp | cat -nRepository: dashpay/dash
Length of output: 7117
Persist the base_blockhash marker only after snapshot load succeeds. Guard the write with snapshot_ok && !in_memory; otherwise a failed load can leave chainstate_snapshot/base_blockhash behind, and DetectSnapshotChainstate() will trust the leftover snapshot dir on the next startup if the process dies before cleanup.
🤖 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 5459 - 5469, Update the persistence
condition in the snapshot validation flow to require both snapshot_ok and
!in_memory before calling node::WriteSnapshotBaseBlockhash. Keep the existing
failure handling that sets snapshot_ok to false when the write fails, and ensure
failed snapshot loads never persist the base_blockhash marker.
✅ Action performedReview finished.
|
|
This pull request has conflicts, please rebase. |
✅ Action performedReview finished.
|
Upstream verification script: sed -i 's/CChainState/Chainstate/g' $(git grep -l CChainState ':(exclude)doc/release-notes*') Verification: PASS. Applying the replacement to the parent tree reproduces this commit exactly across all 51 affected files, with no missing, unexpected, or non-mechanical changes.
Upstream commits: d14bebf db: add StoragePath to CDBWrapper/CCoinsViewDB f9f1735 validation: rename snapshot chainstate dir 252abd1 init: add utxo snapshot detection 34d1590 add utilities for deleting on-disk leveldb data ad67ff3 validation: remove snapshot datadirs upon validation failure 8153bd9 blockmanager: avoid undefined behavior during FlushBlockFile 3a29dfb move-only: test: make snapshot chainstate setup reusable 00b357c validation: add ResetChainstates() 3c36139 test: add reset_chainstate parameter for snapshot unittests 51fc924 test: allow on-disk coins and block tree dbs in tests cced4e7 test: move-only-ish: factor out LoadVerifyActivateChainstate() e4d7995 test: add testcases for snapshot initialization bf95976 doc: add note about snapshot chainstate init
Thread on-disk database options through Dash's generalized TestChainSetup hierarchy, and make snapshot restart tests tear down and recreate Dash managers and txindex around ChainstateManager replacement.
…ad of two exist checks
fe6da7c to
912c108
Compare
|
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. |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
| SnapshotTestSetup() : TestChain100Setup{ | ||
| CBaseChainParams::REGTEST, | ||
| {}, | ||
| /*coins_db_in_memory=*/false, | ||
| /*block_tree_db_in_memory=*/false, | ||
| } |
There was a problem hiding this comment.
🔴 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']
| ChainstateManager& SimulateNodeRestart() | ||
| { | ||
| ChainstateManager& chainman = *Assert(m_node.chainman); | ||
|
|
||
| BOOST_TEST_MESSAGE("Simulating node restart"); | ||
| // TestChainSetup owns a txindex connected to the old chainstate. Stop it | ||
| // before destroying that chainstate, just as TestChainSetup's destructor does. | ||
| IndexWaitSynced(*g_txindex); | ||
| g_txindex->Interrupt(); | ||
| g_txindex->Stop(); | ||
| SyncWithValidationInterfaceQueue(); | ||
| g_txindex.reset(); | ||
| { | ||
| LOCK(::cs_main); | ||
| for (Chainstate* cs : chainman.GetAll()) { | ||
| cs->ForceFlushStateToDisk(); | ||
| } | ||
| // Tear down Dash managers connected to the mempool and old chainstate | ||
| // before LoadVerifyActivateChainstate() recreates them below. | ||
| m_node.govman.reset(); | ||
| DashChainstateSetupClose(m_node); | ||
| chainman.ResetChainstates(); | ||
| BOOST_CHECK_EQUAL(chainman.GetAll().size(), 0); | ||
| const ChainstateManager::Options chainman_opts{ | ||
| .chainparams = ::Params(), | ||
| .adjusted_time_callback = GetAdjustedTime, | ||
| }; | ||
| // For robustness, ensure the old manager is destroyed before creating a | ||
| // new one. | ||
| m_node.chainman.reset(); | ||
| m_node.chainman.reset(new ChainstateManager(chainman_opts)); | ||
| } | ||
| return *Assert(m_node.chainman); |
There was a problem hiding this comment.
🟡 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']
| ChainstateManager& SimulateNodeRestart() | ||
| { | ||
| ChainstateManager& chainman = *Assert(m_node.chainman); | ||
|
|
||
| BOOST_TEST_MESSAGE("Simulating node restart"); | ||
| // TestChainSetup owns a txindex connected to the old chainstate. Stop it | ||
| // before destroying that chainstate, just as TestChainSetup's destructor does. | ||
| IndexWaitSynced(*g_txindex); | ||
| g_txindex->Interrupt(); | ||
| g_txindex->Stop(); | ||
| SyncWithValidationInterfaceQueue(); | ||
| g_txindex.reset(); | ||
| { | ||
| LOCK(::cs_main); | ||
| for (Chainstate* cs : chainman.GetAll()) { | ||
| cs->ForceFlushStateToDisk(); | ||
| } | ||
| // Tear down Dash managers connected to the mempool and old chainstate | ||
| // before LoadVerifyActivateChainstate() recreates them below. | ||
| m_node.govman.reset(); | ||
| DashChainstateSetupClose(m_node); | ||
| chainman.ResetChainstates(); | ||
| BOOST_CHECK_EQUAL(chainman.GetAll().size(), 0); | ||
| const ChainstateManager::Options chainman_opts{ | ||
| .chainparams = ::Params(), | ||
| .adjusted_time_callback = GetAdjustedTime, | ||
| }; | ||
| // For robustness, ensure the old manager is destroyed before creating a | ||
| // new one. | ||
| m_node.chainman.reset(); | ||
| m_node.chainman.reset(new ChainstateManager(chainman_opts)); | ||
| } | ||
| return *Assert(m_node.chainman); |
There was a problem hiding this comment.
🟡 Suggestion: Latest rebase restores a dangling PeerManager reference
The 5686926..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']
…tence (bitcoin#25308, bitcoin#24513, bitcoin#25667, bitcoin#26828)" This reverts commit f153015, reversing changes made to f435ae5.
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#27596is intentionally deferred to M5, when snapshots become user-loadable through the completed assumeutxo flow. M1 does not exposeloadtxoutsetand does not include any partial snapshot-deletion/reindex lifecycle from that later upstream change.What was done?
bitcoin#25308, reducingLoadChainstateparameters and return values through the upstream options/status plumbing while retaining Dash-specific options and the merged-parblsclamp.bitcoin#24513, the mechanicalCChainStatetoChainstaterename across 51 files (204 additions and 204 deletions).bitcoin#25667snapshot-initialization implementation.bitcoin#25667: test-fixture/database-option plumbing, teardown/recreation of Dash managers and txindex aroundChainstateManagerreplacement, and removal of a redundantcs_maindeclaration already supplied byvalidation.h.bitcoin#26828, catching and logging snapshot-pathfs::removeerrors.This branch contains no partial
bitcoin#27596behavior, crash-recovery hardening, reindex cleanup, manager-rebinding workaround, or unrelated-parblsfix. M2-M7 were not restacked by this update.How Has This Been Tested?
make -C src -j"$(sysctl -n hw.ncpu)" test/test_dash../src/test/test_dash --run_test=validation_chainstatemanager_tests(5 cases)../src/test/test_dash --run_test=validation_flush_tests(1 case)../src/test/test_dash --run_test=coinstatsindex_tests(2 cases)../src/test/test_dash --run_test=dbwrapper_tests(9 cases)../src/test/test_dash --run_test=blockmanager_tests(2 cases)../src/test/test_dash --run_test=validation_chainstate_testsreproduces the samechainstate_update_tipabort on the exact previously approved head183f168ef9d32a809fed0f8b51002f01bfb2bb77; this reconstruction intentionally does not change inherited approved-baseline behavior.=patch identity; commit 1 differs only by preserving the merged-parblsclamp through theLoadChainstaterefactor, and commit 4 differs only by removing the redundant declaration. The sole rebase conflict was resolved transformation-faithfully insrc/init.cpp.Breaking Changes
None. This is unreleased assumeutxo groundwork;
loadtxoutsetremains unavailable in M1.Checklist: