Skip to content

fix: remove every snapshot lifecycle directory on explicit reindex #7588

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
PastaPastaPasta wants to merge 1 commit into from
+47 −32
Closed

fix: remove every snapshot lifecycle directory on explicit reindex #7588

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 36 additions & 9 deletions src/node/chainstate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,35 @@
#include <vector>

namespace node {
static bool RemoveSnapshotChainstateArtifacts(const fs::path& data_dir, bilingual_str& error)
{
// Explicit reindexing discards both coins databases and EvoDB, so remove
// every snapshot lifecycle directory in the same stroke. A directory must
// not outlive the markers that describe it: a chainstate_snapshot dir whose
// EvoDB markers were just wiped can no longer be revived by
// ActivateExistingSnapshot(), and a reindex is also the user's request to
// discard the _INVALID forensics directory and any interrupted-swap
// remnant, neither of which the (skipped) recovery pass will see.
const fs::path normal{data_dir / "chainstate"};
fs::path snapshot{normal};
snapshot += SNAPSHOT_CHAINSTATE_SUFFIX;
fs::path invalid{snapshot};
invalid += SNAPSHOT_INVALID_SUFFIX;
fs::path to_delete{normal};
to_delete += SNAPSHOT_TODELETE_SUFFIX;
for (const auto& path : {snapshot, invalid, to_delete}) {
if (!fs::exists(path)) continue;

@coderabbitai coderabbitai Bot Aug 13, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n -C 5 'RemoveSnapshotChainstateArtifacts|fs::exists\(path\)|filesystem_error' src/node/chainstate.cpp
rg -n -C 3 --glob '*.{cpp,h}' 'namespace fs|using.*filesystem' .

Repository: dashpay/dash

Length of output: 5167

🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '1,75p' src/node/chainstate.cpp
sed -n '1,230p' src/util/fs.h
rg -n -C 4 'RemoveSnapshotChainstateArtifacts|ChainstateLoadStatus::FAILURE|cleanup_error' --glob '*.{cpp,h}'
rg -n -C 4 'bool exists|exists\\(' src/util src/node src/test --glob '*.{cpp,h}'

Repository: dashpay/dash

Length of output: 11074

🌐 Web query:

C++ std::filesystem::exists(const path&) throws filesystem_error status error specification

💡 Result:

The C++ standard library function std::filesystem::exists provides both throwing and non-throwing overloads to handle file system operations [1][2]. When using the overload that takes only a path (std::filesystem::path), the function may throw a std::filesystem::filesystem_error if an underlying operating system API call fails [1][3]. It is important to note that this exception is thrown when an error occurs during the attribute determination process (such as permission issues or invalid path formats that the OS cannot resolve), but it is explicitly NOT thrown simply because a file does not exist [1][2]. If the path is known to not exist, the function returns false without throwing an exception [1][2]. The throwing behavior is specified as follows: - The function internally calls std::filesystem::status [1][2]. - If that call encounters an OS-level error that prevents it from determining the file status (e.g., an I/O error or restricted access that isn't classified as 'not found'), it throws std::filesystem::filesystem_error [1][4]. - Any overload not marked noexcept may also throw std::bad_alloc if memory allocation fails [1][2]. To avoid exceptions, you should use the non-throwing overload that accepts a std::error_code parameter [1][2]: bool exists(const std::filesystem::path& p, std::error_code& ec) noexcept; In this version, if an error occurs, the function does not throw an exception; instead, it sets the provided std::error_code object to the specific OS error code, allowing you to handle the failure programmatically [1][3]. If the file does not exist, the function simply returns false and the error code is cleared or indicates success, depending on the implementation state [1][2].

Citations:

Catch fs::exists() failures.

fs::exists(const fs::path&) forwards to throwing std::filesystem::exists. A status error can throw before try, bypassing cleanup_error and the defined ChainstateLoadStatus::FAILURE return. Move the check inside try.

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

In `@src/node/chainstate.cpp` at line 57, Move the fs::exists(path) check in the
chainstate loading flow inside the existing try block so filesystem status
exceptions are caught. Preserve the continue behavior for nonexistent paths and
ensure failures still reach cleanup_error and return
ChainstateLoadStatus::FAILURE.

@PastaPastaPasta PastaPastaPasta Aug 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid — fixed in 2c3e615, though with a different mechanism than suggested: rather than changing DirectoryCommit()'s documented ignore-errors contract (other callers rely on it), the removal now goes through RemoveAllDurably(), the existing durable-deletion helper from the snapshot lifecycle work, whose SyncDirectoryOrThrow propagates open/fsync failures as the fs::filesystem_error this function's catch already converts into a startup failure.

🤖 Posted autonomously by Claude on behalf of pasta.

@coderabbitai coderabbitai Bot Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PastaPastaPasta, RemoveAllDurably() correctly propagates deletion and directory-sync failures.

However, the original failure path remains. fs::exists(path) is still outside the try block in RemoveSnapshotChainstateArtifacts(). A filesystem status error from that call can still throw before the function sets cleanup_error and returns ChainstateLoadStatus::FAILURE.

Please move the existence check into the try block, or use the std::error_code overload and convert its error to cleanup_error.

✏️ Learnings added 
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 7588
File: src/node/chainstate.cpp:57-57
Timestamp: 2026-08-13T03:19:33.580Z
Learning: In `src/node/chainstate.cpp`, `DirectoryCommit()` has a documented ignore-errors contract that other callers rely on. Use `RemoveAllDurably()` for snapshot lifecycle deletion when durable directory synchronization errors must propagate through `fs::filesystem_error`.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

You are interacting with an AI system.

try {
RemoveAllDurably(path);
} catch (const fs::filesystem_error& e) {
Comment on lines +57 to +60

@thepastaclaw thepastaclaw Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Catch failures from the snapshot artifact existence check

fs::exists(path) calls the throwing std::filesystem::exists overload before entering the try block. A filesystem status error therefore bypasses this helper's artifact-specific cleanup_error and false result. The main initialization path catches the escaped exception and still stops before EvoDB is wiped, so this does not violate the durability invariant, but it produces the generic block-database error instead of the intended cleanup failure and leaves direct LoadChainstate() callers with an unexpected exception path. Move the existence check into the existing try block.

Suggested change
if (!fs::exists(path)) continue;
try {
RemoveAllDurably(path);
} catch (const fs::filesystem_error& e) {
try {
if (!fs::exists(path)) continue;
RemoveAllDurably(path);
} catch (const fs::filesystem_error& e) {

source: ['coderabbit']

error = strprintf(_("Failed to remove snapshot chainstate artifact %s for reindex: %s"),
fs::PathToString(path), e.what());
return false;
}
}
return true;
}

static bool RecoverSnapshotCleanup(CEvoDB& evodb, const fs::path& data_dir, bilingual_str& error)
{
const fs::path normal{data_dir / "chainstate"};
Expand Down Expand Up @@ -326,6 +355,13 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize

LOCK(cs_main);

if (options.reindex || options.reindex_chainstate) {
bilingual_str cleanup_error;
if (!RemoveSnapshotChainstateArtifacts(options.data_dir, cleanup_error)) {
return {ChainstateLoadStatus::FAILURE, cleanup_error};
}
}

evodb.reset();
// TODO: pass DbWrapperParams as options instead multiple params
evodb = std::make_unique<CEvoDB>(util::DbWrapperParams{
Expand All @@ -344,15 +380,6 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
// Load the fully validated chainstate.
chainman.InitializeChainstate(options.mempool, *evodb, chain_helper);

// Wiping the shared EvoDB above erased the SNAPSHOT best-block marker that
// ActivateExistingSnapshot() requires, so a persisted snapshot chainstate can
// no longer be revived. Discard it here rather than letting startup fail with
// advice ("reindex") the user has just followed, which would never recover.
if ((options.reindex || options.reindex_chainstate) && !DeleteSnapshotChainstateFromDisk()) {
return {ChainstateLoadStatus::FAILURE,
_("Failed to remove the snapshot chainstate directory. Remove it manually before restarting.")};
}

// Load a chain created from a UTXO snapshot, if any exist.
bilingual_str snapshot_error;
if (!chainman.DetectSnapshotChainstate(options.mempool, snapshot_error)) {
Expand Down
13 changes: 0 additions & 13 deletions src/validation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5570,19 +5570,6 @@ static bool DeleteCoinsDBFromDisk(const fs::path db_path, bool is_snapshot)
return destroyed && !fs::exists(db_path);
}

bool DeleteSnapshotChainstateFromDisk()
{
AssertLockHeld(::cs_main);

auto snapshot_datadir = node::FindSnapshotChainstateDir();
if (!snapshot_datadir) {
return true;
}
LogPrintf("[snapshot] discarding persisted snapshot chainstate at %s\n",
fs::PathToString(*snapshot_datadir));
return DeleteCoinsDBFromDisk(*snapshot_datadir, /*is_snapshot=*/true);
}

bool ChainstateManager::ActivateSnapshot(
AutoFile& coins_file,
const SnapshotMetadata& metadata,
Expand Down
10 changes: 0 additions & 10 deletions src/validation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1294,16 +1294,6 @@ MnRewardEra GetMnRewardEraAfter(const CBlockIndex* pindexPrev, const ChainstateM
*/
const AssumeutxoData* ExpectedAssumeutxo(const int height, const CChainParams& params);

/**
* Remove a persisted snapshot chainstate's on-disk artifacts: its coins database
* and the base-blockhash file identifying it. Only valid while no snapshot
* Chainstate object exists, i.e. at startup before DetectSnapshotChainstate().
*
* @returns false only if a snapshot chainstate was found but could not be fully
* removed; true when there was nothing to remove.
*/
bool DeleteSnapshotChainstateFromDisk() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);

/** Identifies blocks that overwrote an existing coinbase output in the UTXO set (see BIP30) */
bool IsBIP30Repeat(const CBlockIndex& block_index);

Expand Down
11 changes: 11 additions & 0 deletions test/functional/feature_reindex.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
"""

import os
from pathlib import Path
from test_framework.test_framework import BitcoinTestFramework
from test_framework.messages import MAGIC_BYTES
from test_framework.util import assert_equal
Expand All @@ -25,9 +26,19 @@ def reindex(self, justchainstate=False, txindex=0):
self.generatetoaddress(self.nodes[0], 3, self.nodes[0].get_deterministic_priv_key().address)
blockcount = self.nodes[0].getblockcount()
self.stop_nodes()
chain_dir = Path(self.nodes[0].datadir) / self.nodes[0].chain
snapshot_artifacts = [
chain_dir / "chainstate_snapshot",
chain_dir / "chainstate_snapshot_INVALID",
chain_dir / "chainstate_todelete",
]
for artifact in snapshot_artifacts:
artifact.mkdir()
(artifact / "stale").touch()
extra_args = [["-reindex-chainstate", "-txindex=0"]] if justchainstate else [["-reindex", f"-txindex={txindex}"]]
self.start_nodes(extra_args)
assert_equal(self.nodes[0].getblockcount(), blockcount) # start_node is blocking on reindex
assert all(not artifact.exists() for artifact in snapshot_artifacts)
self.log.info("Success")

# Check that blocks can be processed out of order
Expand Down
Loading