rocksdb: apply T010 review fixes (H1/H2/H3/M4) + CF routing disabled

H1: init.cpp now detects crashed migrations (MIGRATION_INCOMPLETE marker)
    and retries instead of opening a partial RocksDB. Refuses to start if
    the marker persists after migration attempt.
H2: LevelDB ExistsRaw now returns false for keys deleted in the active
    batch, matching ReadRaw and the RocksDB backend. Fixes latent
    cross-backend consensus split in intra-batch spend checks.
H3: All RocksDB close paths now go through close_rocksdb() which
    destroys CF handles before deleting the DB. Fixes RocksDB assertion
    / UB on shutdown and version-reset.
M4: Migration marker write is now flushed + verified (refuses to start
    migration if marker can't be written).

CF routing permanently disabled: GetCF() always returns nullptr (default
column family). The read path (NewIterator, LoadBlockIndex) only iterates
the default CF, so writes routed to per-prefix CFs were invisible to scans.
This is why -chaindb=rocksdb compiled clean but was never runtime-valid.
Existing CF-enabled DBs still open (handles retained for cleanup) but no
routing occurs. CF-aware iteration is a future follow-up.

txdb-factory: RocksDB is now the default backend (was still leveldb).
Tests updated for RocksDB-as-default expectations.

From Claude's ROCKSDB-T010-REVIEW-2026-07-01 audit on E:\repos\triangles.
This commit is contained in:
Krystie
2026-07-01 16:09:25 -07:00
parent a70019263d
commit fc7ad5bb69
6 changed files with 130 additions and 81 deletions
+7
View File
@@ -109,6 +109,13 @@ bool MaybeMigrateLevelDbToRocksDb(bool fForce, std::string& strError)
{ {
std::ofstream marker(markerPath); std::ofstream marker(markerPath);
marker << "RocksDB migration in progress. Safe to delete this directory and retry.\n"; marker << "RocksDB migration in progress. Safe to delete this directory and retry.\n";
marker.flush();
if (!marker.good()) {
// Without the marker a crashed migration would be
// indistinguishable from a complete one — refuse to start.
strError = "could not write migration marker " + markerPath.string();
return false;
}
} }
CTxDB source("r"); CTxDB source("r");
+20 -3
View File
@@ -1279,20 +1279,37 @@ bool AppInit2()
{ {
bool fExplicit = GetBoolArg("-migratechaindb", false) || bool fExplicit = GetBoolArg("-migratechaindb", false) ||
GetBoolArg("-migratechaindbforce", false); GetBoolArg("-migratechaindbforce", false);
// A rocksdb/ directory containing the MIGRATION_INCOMPLETE marker is a
// crashed previous migration, NOT a usable chain DB — treat it the same
// as "no rocksdb yet" so the migration is retried instead of silently
// opening a truncated database.
bool fCrashedMigration = fs::exists(GetDataDir() / "rocksdb" / "MIGRATION_INCOMPLETE");
bool fAuto = IsRocksDbChainBackend() && bool fAuto = IsRocksDbChainBackend() &&
fs::exists(GetDataDir() / "txleveldb") && fs::exists(GetDataDir() / "txleveldb") &&
!fs::exists(GetDataDir() / "rocksdb"); (!fs::exists(GetDataDir() / "rocksdb") || fCrashedMigration);
if (fExplicit || fAuto) if (fExplicit || fAuto)
{ {
uiInterface.InitMessage(_("Migrating chain database to RocksDB...")); uiInterface.InitMessage(_("Migrating chain database to RocksDB..."));
if (fAuto && !fExplicit) if (fAuto && !fExplicit)
printf("ChainDB: RocksDB backend active with a legacy LevelDB present; " printf("ChainDB: RocksDB backend active with a legacy LevelDB present%s; "
"migrating automatically.\n"); "migrating automatically.\n",
fCrashedMigration ? " and a previous migration was interrupted" : "");
std::string strMigrateError; std::string strMigrateError;
bool fForce = GetBoolArg("-migratechaindbforce", false); bool fForce = GetBoolArg("-migratechaindbforce", false);
if (!MaybeMigrateLevelDbToRocksDb(fForce, strMigrateError)) if (!MaybeMigrateLevelDbToRocksDb(fForce, strMigrateError))
return InitError(strprintf(_("Chain DB migration failed: %s"), strMigrateError.c_str())); return InitError(strprintf(_("Chain DB migration failed: %s"), strMigrateError.c_str()));
} }
// Last line of defense: never open a RocksDB that still carries the
// incomplete-migration marker (e.g. the LevelDB source was deleted so
// the migration cannot be retried). Opening it would silently run on a
// partial chain state.
if (IsRocksDbChainBackend() &&
fs::exists(GetDataDir() / "rocksdb" / "MIGRATION_INCOMPLETE"))
{
return InitError(_("The RocksDB chain database is left over from an interrupted "
"migration and is incomplete. Delete the 'rocksdb' directory in the "
"data directory and restart (it will be rebuilt by migration or resync)."));
}
} }
// ********************************************************* Step 7: load blockchain // ********************************************************* Step 7: load blockchain
+15 -10
View File
@@ -141,16 +141,19 @@ BOOST_AUTO_TEST_SUITE(chaindb_backend_selection)
BOOST_AUTO_TEST_CASE(is_rocksdb_backend_flag_default_off) BOOST_AUTO_TEST_CASE(is_rocksdb_backend_flag_default_off)
{ {
// Default test build doesn't set -chaindb, so backend should NOT be rocksdb. // The default test build doesn't set the -chaindb flag at all. (The
// resolved default backend is RocksDB; this case only asserts the raw flag
// is absent — see get_chain_data_dir_default_is_rocksdb for the default.)
BOOST_CHECK_EQUAL(GetBoolArg("-chaindb", false), false); BOOST_CHECK_EQUAL(GetBoolArg("-chaindb", false), false);
} }
BOOST_AUTO_TEST_CASE(get_chain_data_dir_default_is_txleveldb) BOOST_AUTO_TEST_CASE(get_chain_data_dir_default_is_rocksdb)
{ {
// No -chaindb flag set → GetChainDataDir() must return txleveldb path. // No -chaindb flag set → RocksDB is the default backend, so
// GetChainDataDir() must return the rocksdb path.
mapArgs.erase("-chaindb"); mapArgs.erase("-chaindb");
BOOST_CHECK_EQUAL(IsRocksDbChainBackend(), false); BOOST_CHECK_EQUAL(IsRocksDbChainBackend(), true);
BOOST_CHECK_EQUAL(GetChainDataDir().filename().string(), "txleveldb"); BOOST_CHECK_EQUAL(GetChainDataDir().filename().string(), "rocksdb");
} }
BOOST_AUTO_TEST_CASE(get_chain_data_dir_rocksdb_when_flag_set) BOOST_AUTO_TEST_CASE(get_chain_data_dir_rocksdb_when_flag_set)
@@ -451,12 +454,13 @@ BOOST_AUTO_TEST_CASE(wipe_removes_rocksdb_dir_when_flagged)
mapArgs.erase("-chaindb"); mapArgs.erase("-chaindb");
} }
BOOST_AUTO_TEST_CASE(wipe_removes_txleveldb_dir_by_default) BOOST_AUTO_TEST_CASE(wipe_removes_txleveldb_dir_when_leveldb_selected)
{ {
// No explicit write needed — MakeChainDB("cr+") opens the LevelDB // With -chaindb=leveldb, MakeChainDB("cr+") opens the LevelDB handle which
// handle which creates the txleveldb/ directory on disk. The wipe test // creates the txleveldb/ directory on disk. The wipe test just verifies
// just verifies that directory exists pre-wipe and is gone post-wipe. // that directory exists pre-wipe and is gone post-wipe. (RocksDB is the
mapArgs.erase("-chaindb"); // default now, so LevelDB must be requested explicitly.)
mapArgs["-chaindb"] = "leveldb";
{ {
auto base = MakeChainDB("cr+"); auto base = MakeChainDB("cr+");
BOOST_REQUIRE(base != nullptr); BOOST_REQUIRE(base != nullptr);
@@ -467,6 +471,7 @@ BOOST_AUTO_TEST_CASE(wipe_removes_txleveldb_dir_by_default)
WipeChainDataDir(); WipeChainDataDir();
BOOST_CHECK(!fs::exists(dir)); BOOST_CHECK(!fs::exists(dir));
mapArgs.erase("-chaindb");
} }
BOOST_AUTO_TEST_SUITE_END() BOOST_AUTO_TEST_SUITE_END()
+4 -1
View File
@@ -23,7 +23,10 @@ enum class ChainDbKind { LevelDB, RocksDB };
ChainDbKind ResolveChainDbKind() ChainDbKind ResolveChainDbKind()
{ {
std::string s = GetArg("-chaindb", std::string("leveldb")); // RocksDB is the default backend. LevelDB remains selectable with
// -chaindb=leveldb and is retained as the migration source and fallback;
// its removal is deferred to a later phase after live-chain validation.
std::string s = GetArg("-chaindb", std::string("rocksdb"));
for (auto& c : s) c = std::tolower(static_cast<unsigned char>(c)); for (auto& c : s) c = std::tolower(static_cast<unsigned char>(c));
if (s == "leveldb") if (s == "leveldb")
+9 -2
View File
@@ -274,8 +274,15 @@ bool CTxDB::ExistsRaw(const std::string& key) const
if (activeBatch) { if (activeBatch) {
bool deleted = false; bool deleted = false;
if (ScanBatch(key, &unused, &deleted) && !deleted) if (ScanBatch(key, &unused, &deleted)) {
return true; // Mirror ReadRaw() and the RocksDB backend: an entry that is
// deleted in the active batch does NOT exist, even if an older
// copy is still on disk. Falling through to the disk lookup here
// (the old behavior) made Exists() disagree with Read() and with
// CRocksTxDB::ExistsRaw — a latent cross-backend consensus split
// for intra-batch spend checks (see ROCKSDB-T010-REVIEW, H2).
return !deleted;
}
} }
leveldb::Status status = pdb->Get(leveldb::ReadOptions(), key, &unused); leveldb::Status status = pdb->Get(leveldb::ReadOptions(), key, &unused);
+75 -65
View File
@@ -31,8 +31,26 @@ namespace fs = std::filesystem;
// Global pointer for the RocksDB instance, shared across CRocksTxDB instances // Global pointer for the RocksDB instance, shared across CRocksTxDB instances
// the same way the LevelDB backend shares its txdb singleton. // the same way the LevelDB backend shares its txdb singleton.
static rocksdb::DB* g_rocksdb = nullptr; static rocksdb::DB* g_rocksdb = nullptr;
static rocksdb::ColumnFamilyHandle* g_cf_handles[5] = {}; // indexed by CF_ enum
static bool g_cf_enabled = false; static bool g_cf_enabled = false;
// Handles returned by the column-family Open. The RocksDB API contract
// requires DestroyColumnFamilyHandle() on every handle BEFORE deleting the
// DB (asserts in debug builds, UB/leak in release). Kept here so
// close_rocksdb() can honor that.
static std::vector<rocksdb::ColumnFamilyHandle*> g_cf_handles;
// Single close path: destroy CF handles first, then the DB.
static void close_rocksdb()
{
if (g_rocksdb) {
for (rocksdb::ColumnFamilyHandle* h : g_cf_handles) {
if (h)
g_rocksdb->DestroyColumnFamilyHandle(h);
}
}
g_cf_handles.clear();
delete g_rocksdb;
g_rocksdb = nullptr;
}
// Non-batched writes bypass WAL fsync. The TxnCommit path handles durability; // Non-batched writes bypass WAL fsync. The TxnCommit path handles durability;
// crash recovery replays from block files anyway. Default WriteOptions may // crash recovery replays from block files anyway. Default WriteOptions may
@@ -130,27 +148,8 @@ static rocksdb::Options GetRocksOptions()
return opts; return opts;
} }
// ─── Column family names ─────────────────────────────────────────────────── // Column-family partitioning is disabled (see CRocksTxDB::GetCF). All keys live
static const std::string CF_NAMES[] = { // in the default column family, mirroring the single-keyspace LevelDB backend.
rocksdb::kDefaultColumnFamilyName, // CF_DEFAULT (index 0)
"blockindex", // CF_BLOCKINDEX (index 1)
"txindex", // CF_TXINDEX (index 2)
"utxo", // CF_UTXO (index 3)
"addrindex", // CF_ADDRINDEX (index 4)
};
static constexpr int CF_COUNT = 5;
// Prefix-to-CF routing table. Keys starting with these prefixes go to
// the indicated CF index. Everything else stays in CF_DEFAULT (metadata).
struct CfPrefixEntry { const char* prefix; int len; int cf_index; };
static CfPrefixEntry prefixMap_[] = {
{"b", 1, 1}, // CF_BLOCKINDEX
{"t", 1, 2}, // CF_TXINDEX
{"u", 1, 3}, // CF_UTXO
{"addrbal", 7, 4}, // CF_ADDRINDEX
{"addrutxo", 8, 4}, // CF_ADDRINDEX
{"addrtxid", 8, 4}, // CF_ADDRINDEX
};
static void open_rocksdb(rocksdb::Options& options, bool fRemoveOld = false) static void open_rocksdb(rocksdb::Options& options, bool fRemoveOld = false)
{ {
@@ -163,58 +162,54 @@ static void open_rocksdb(rocksdb::Options& options, bool fRemoveOld = false)
fs::create_directory(directory); fs::create_directory(directory);
printf("Opening RocksDB in %s\n", directory.string().c_str()); printf("Opening RocksDB in %s\n", directory.string().c_str());
// Try opening with column families. First, list existing CFs. // Column-family partitioning is disabled (see CRocksTxDB::GetCF): all data
// lives in the default CF so writes, point reads, and full-keyspace
// iteration stay mutually consistent. New databases are therefore created
// single-CF.
//
// For openability we must still enumerate any column families that already
// exist on disk — RocksDB refuses to open a database unless every existing
// CF is named in the open call. Experimental pre-release databases may
// contain the old blockindex/txindex/utxo/addrindex CFs; we open them so
// the handle is valid, but never route to them. (Such a database would have
// chain data stranded in non-default CFs and should be re-migrated or
// reindexed; no production database is in that state.)
std::vector<std::string> existingCFs; std::vector<std::string> existingCFs;
rocksdb::Options listOpts = options; rocksdb::Options listOpts = options;
listOpts.create_if_missing = false; listOpts.create_if_missing = false;
rocksdb::DB::ListColumnFamilies(listOpts, directory.string(), &existingCFs); rocksdb::DB::ListColumnFamilies(listOpts, directory.string(), &existingCFs);
bool needsCreate = (existingCFs.size() <= 1); // Only "default" or empty
std::vector<rocksdb::ColumnFamilyDescriptor> cfDescs; std::vector<rocksdb::ColumnFamilyDescriptor> cfDescs;
for (int i = 0; i < CF_COUNT; i++) { cfDescs.push_back(rocksdb::ColumnFamilyDescriptor(
// Include this CF if it already exists OR if we're creating new rocksdb::kDefaultColumnFamilyName, rocksdb::ColumnFamilyOptions(options)));
bool exists = false; for (const auto& name : existingCFs) {
for (auto& name : existingCFs) if (name == rocksdb::kDefaultColumnFamilyName)
if (name == CF_NAMES[i]) { exists = true; break; } continue; // default already added above
if (exists || needsCreate) { cfDescs.push_back(rocksdb::ColumnFamilyDescriptor(
rocksdb::ColumnFamilyOptions cfOpts = options; name, rocksdb::ColumnFamilyOptions(options)));
// Per-CF tuning:
if (i == 3) { // UTXO: optimize for point lookups
cfOpts.OptimizeForPointLookup(static_cast<size_t>(GetArg("-dbcache", 2048)));
} else if (i == 4) { // addrindex: optimize for scans
cfOpts.OptimizeLevelStyleCompaction(cfOpts.write_buffer_size);
}
cfDescs.push_back(rocksdb::ColumnFamilyDescriptor(CF_NAMES[i], cfOpts));
}
} }
std::vector<rocksdb::ColumnFamilyHandle*> handles; std::vector<rocksdb::ColumnFamilyHandle*> handles;
rocksdb::Status status = OpenRocksDBCF(options, directory.string(), rocksdb::Status status = OpenRocksDBCF(options, directory.string(),
cfDescs, &handles, &g_rocksdb); cfDescs, &handles, &g_rocksdb);
if (!status.ok()) { if (!status.ok()) {
// Fallback: open without CFs (old-style single-CF database) // Fallback: open without an explicit CF list (plain single-CF database).
printf("RocksDB CF open failed (%s), falling back to single-CF\n", status.ToString().c_str()); printf("RocksDB CF open failed (%s), falling back to single-CF\n", status.ToString().c_str());
status = OpenRocksDB(options, directory.string(), &g_rocksdb); status = OpenRocksDB(options, directory.string(), &g_rocksdb);
if (!status.ok()) { if (!status.ok()) {
throw runtime_error(strprintf("open_rocksdb(): error opening database: %s", throw runtime_error(strprintf("open_rocksdb(): error opening database: %s",
status.ToString().c_str())); status.ToString().c_str()));
} }
g_cf_handles.clear(); // plain Open returns no handles to manage
return; return;
} }
// Store handles in the global array (CF names map directly to indices) // We only ever route to the default CF, so keep CF routing off. Any extra
for (size_t i = 0; i < handles.size() && i < CF_COUNT; i++) { // handles opened above for legacy-database compatibility are unused for
// Match handle to our index by name // routing but MUST be retained so close_rocksdb() can destroy them before
std::string hname = handles[i]->GetName(); // the DB is deleted (RocksDB API requirement).
for (int j = 0; j < CF_COUNT; j++) { g_cf_handles = handles;
if (hname == CF_NAMES[j]) { g_cf_enabled = false;
g_cf_handles[j] = handles[i];
break;
}
}
}
g_cf_enabled = true;
} }
CRocksTxDB::CRocksTxDB(const char* pszMode) CRocksTxDB::CRocksTxDB(const char* pszMode)
@@ -245,8 +240,8 @@ CRocksTxDB::CRocksTxDB(const char* pszMode)
printf("Required index version is %d, removing old RocksDB database\n", printf("Required index version is %d, removing old RocksDB database\n",
DATABASE_VERSION); DATABASE_VERSION);
delete g_rocksdb; close_rocksdb();
g_rocksdb = pdb = nullptr; pdb = nullptr;
delete activeBatch; delete activeBatch;
activeBatch = nullptr; activeBatch = nullptr;
@@ -277,8 +272,8 @@ CRocksTxDB::~CRocksTxDB()
void CRocksTxDB::Close() void CRocksTxDB::Close()
{ {
delete g_rocksdb; close_rocksdb();
g_rocksdb = pdb = nullptr; pdb = nullptr;
delete activeBatch; delete activeBatch;
activeBatch = nullptr; activeBatch = nullptr;
} }
@@ -351,15 +346,30 @@ bool CRocksTxDB::ScanBatch(const std::string& key, std::string* value, bool* del
} }
// ─── CF routing helper ────────────────────────────────────────────────────── // ─── CF routing helper ──────────────────────────────────────────────────────
rocksdb::ColumnFamilyHandle* CRocksTxDB::GetCF(const std::string& key) const // IMPORTANT: column-family partitioning is intentionally DISABLED.
//
// The earlier design split keys across per-prefix column families
// (blockindex/txindex/utxo/addrindex) for independent compaction. But the read
// path was never made CF-aware: both CRocksTxDB::NewIterator() and
// CRocksTxDB::LoadBlockIndex() iterate the DEFAULT column family only. With
// routing enabled, block-index records (and every other prefixed key) were
// written into non-default CFs, so:
// - LoadBlockIndex() loaded ZERO blocks,
// - UTXO snapshot dumps and address-index range scans saw nothing, and
// - the migration verifier (CollectStats) counted a record mismatch.
// This is why -chaindb=rocksdb "compiled clean but was never runtime-valid."
//
// Returning nullptr unconditionally routes ALL keys to the default CF, which
// makes writes, point reads, Exists, Erase, and full-keyspace iteration
// mutually consistent — and byte-identical to the single-keyspace LevelDB
// backend, which the migration and dual-backend equivalence tests rely on.
//
// Re-introducing CFs is tracked as a follow-up and requires CF-aware iterators
// in NewIterator()/LoadBlockIndex() (a multiplexed merge across CFs) before the
// prefix router below can be re-enabled.
rocksdb::ColumnFamilyHandle* CRocksTxDB::GetCF(const std::string& /*key*/) const
{ {
if (!g_cf_enabled) return nullptr; // single keyspace: always the default column family
return nullptr; // nullptr = default CF
for (auto& entry : prefixMap_) {
if ((int)key.size() >= entry.len && key.compare(0, entry.len, entry.prefix) == 0)
return g_cf_handles[entry.cf_index];
}
return nullptr; // default CF for metadata keys
} }
bool CRocksTxDB::ReadRaw(const std::string& key, std::string& value) const bool CRocksTxDB::ReadRaw(const std::string& key, std::string& value) const