Skip to content

refactor: assumeutxo M3 review follow-ups - #7585

Merged
PastaPastaPasta merged 3 commits into
dashpay:developfrom
PastaPastaPasta:assumeutxo/m3-followups
Aug 12, 2026
Merged

refactor: assumeutxo M3 review follow-ups#7585
PastaPastaPasta merged 3 commits into
dashpay:developfrom
PastaPastaPasta:assumeutxo/m3-followups

Conversation

@PastaPastaPasta

@PastaPastaPasta PastaPastaPasta commented Aug 12, 2026

Copy link
Copy Markdown
Member

Issue being fixed or feature implemented

Review of the assumeutxo M3 PR (#7553, now merged) surfaced three small non-blocking cleanups. They were split out so #7553 could merge as-is:

  1. RecoverSnapshotCleanup() re-spelled all four snapshot-lifecycle directory names as string literals while the rename sites derived them from SNAPSHOT_CHAINSTATE_SUFFIX or inlined them. A rename of any suffix would compile cleanly while crash recovery silently stopped matching the on-disk layout.
  2. The candidate-admission pass in ResetBlockFailureFlags() called ChainstateManager::GetAll() inside the per-block loop, re-acquiring cs_main recursively and heap-allocating a vector per reconsidered block even though the chainstate set cannot change while cs_main is held for the whole function.
  3. MaybeCompleteSnapshotValidation() carried assert(snapshot_base_height == index_new.nHeight) four lines after asserting the same equality.

What was done?

  • Added SNAPSHOT_INVALID_SUFFIX and SNAPSHOT_TODELETE_SUFFIX next to SNAPSHOT_CHAINSTATE_SUFFIX in node/utxo_snapshot.h and consume them in RecoverSnapshotCleanup(), InvalidateCoinsDBOnDisk(), and ValidatedSnapshotCleanup(). Test files intentionally keep spelled-out literals so they would catch an accidental rename of the on-disk names.
  • Hoisted the GetAll() call above the admission loop.
  • Dropped the duplicated assert.

How Has This Been Tested?

Both touched translation units compile with --enable-werror. No behavior change is intended; the existing validation_chainstatemanager_tests snapshot-recovery cases cover the renamed-directory handling.

Breaking Changes

None.

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 (for repository code-owners and collaborators only)

@PastaPastaPasta
PastaPastaPasta force-pushed the assumeutxo/m3-followups branch from 7959fa8 to 9190215 Compare August 12, 2026 16:21
@PastaPastaPasta
PastaPastaPasta marked this pull request as ready for review August 12, 2026 16:21
@thepastaclaw

thepastaclaw commented Aug 12, 2026

Copy link
Copy Markdown

🕓 Ready for review — 4 ahead in queue (commit d4851d8)
Queue position: 5/5 · 2 reviews active
ETA: start ~17:19 UTC · complete ~17:36 UTC (median 17m across 30 recent reviews; 2 slots)
Queued 6m ago · Last checked: 2026-08-12 16:40 UTC

@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e06adbf3-223b-4a52-9965-19276ac94e34

📥 Commits

Reviewing files that changed from the base of the PR and between 9190215 and d4851d8.

📒 Files selected for processing (1)
  • src/node/chainstate.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/node/chainstate.cpp

Walkthrough

The changes add shared suffix constants for snapshot-related directories. Chainstate recovery, invalidation, and cleanup now derive paths from the base chainstate path. Snapshot completion verifies the background chainstate height against snapshot_base_height. ResetBlockFailureFlags caches the chainstate collection before restoring reconsidered block candidates.

Estimated code review effort: 2 (Simple) | ~15 minutes

Possibly related PRs

  • dashpay/dash#7456: Modifies snapshot chainstate handling, cleanup, and suffix-based invalidation paths.
  • dashpay/dash#7553: Introduces snapshot cleanup and validation logic refined by this change.
  • dashpay/dash#7579: Updates snapshot cleanup, recovery, and shared directory suffix handling.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the assumeutxo refactoring and review follow-up changes.
Description check ✅ Passed The description directly explains all three cleanup changes, testing, and the absence of behavior or breaking changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

PastaPastaPasta and others added 3 commits August 12, 2026 11:30
…ecovery

RecoverSnapshotCleanup re-spelled the snapshot, todelete and INVALID directory names as string literals while the rename sites derived them from SNAPSHOT_CHAINSTATE_SUFFIX or inlined them. A rename of any suffix would compile cleanly while crash recovery silently stopped matching the on-disk layout. Define the INVALID and todelete suffixes next to SNAPSHOT_CHAINSTATE_SUFFIX and consume them at every producer and consumer.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n loop

The candidate-admission pass in ResetBlockFailureFlags called ChainstateManager::GetAll() for every reconsidered block, re-acquiring cs_main recursively and heap-allocating a vector per iteration even though the chainstate set cannot change while cs_main is held for the whole function. Fetch the set once before the loop.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
assert(snapshot_base_height == index_new.nHeight) restated assert(index_new.nHeight == snapshot_base_height) from four lines earlier; upstream carries only the first assert plus the curr_height check.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@PastaPastaPasta
PastaPastaPasta force-pushed the assumeutxo/m3-followups branch from 9190215 to d4851d8 Compare August 12, 2026 16:31
@PastaPastaPasta
PastaPastaPasta merged commit 3e6616c into dashpay:develop Aug 12, 2026
11 checks passed
@UdjinM6 UdjinM6 added this to the 24 milestone Aug 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants