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);
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");
+20 -3
View File
@@ -1279,20 +1279,37 @@ bool AppInit2()
{
bool fExplicit = GetBoolArg("-migratechaindb", 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() &&
fs::exists(GetDataDir() / "txleveldb") &&
!fs::exists(GetDataDir() / "rocksdb");
(!fs::exists(GetDataDir() / "rocksdb") || fCrashedMigration);
if (fExplicit || fAuto)
{
uiInterface.InitMessage(_("Migrating chain database to RocksDB..."));
if (fAuto && !fExplicit)
printf("ChainDB: RocksDB backend active with a legacy LevelDB present; "
"migrating automatically.\n");
printf("ChainDB: RocksDB backend active with a legacy LevelDB present%s; "
"migrating automatically.\n",
fCrashedMigration ? " and a previous migration was interrupted" : "");
std::string strMigrateError;
bool fForce = GetBoolArg("-migratechaindbforce", false);
if (!MaybeMigrateLevelDbToRocksDb(fForce, strMigrateError))
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
+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)
{
// 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_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");
BOOST_CHECK_EQUAL(IsRocksDbChainBackend(), false);
BOOST_CHECK_EQUAL(GetChainDataDir().filename().string(), "txleveldb");
BOOST_CHECK_EQUAL(IsRocksDbChainBackend(), true);
BOOST_CHECK_EQUAL(GetChainDataDir().filename().string(), "rocksdb");
}
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");
}
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
// handle which creates the txleveldb/ directory on disk. The wipe test
// just verifies that directory exists pre-wipe and is gone post-wipe.
mapArgs.erase("-chaindb");
// With -chaindb=leveldb, MakeChainDB("cr+") opens the LevelDB handle which
// creates the txleveldb/ directory on disk. The wipe test just verifies
// that directory exists pre-wipe and is gone post-wipe. (RocksDB is the
// default now, so LevelDB must be requested explicitly.)
mapArgs["-chaindb"] = "leveldb";
{
auto base = MakeChainDB("cr+");
BOOST_REQUIRE(base != nullptr);
@@ -467,6 +471,7 @@ BOOST_AUTO_TEST_CASE(wipe_removes_txleveldb_dir_by_default)
WipeChainDataDir();
BOOST_CHECK(!fs::exists(dir));
mapArgs.erase("-chaindb");
}
BOOST_AUTO_TEST_SUITE_END()
+4 -1
View File
@@ -23,7 +23,10 @@ enum class ChainDbKind { LevelDB, RocksDB };
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));
if (s == "leveldb")
+9 -2
View File
@@ -274,8 +274,15 @@ bool CTxDB::ExistsRaw(const std::string& key) const
if (activeBatch) {
bool deleted = false;
if (ScanBatch(key, &unused, &deleted) && !deleted)
return true;
if (ScanBatch(key, &unused, &deleted)) {
// 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);
+75 -65
View File
@@ -31,8 +31,26 @@ namespace fs = std::filesystem;
// Global pointer for the RocksDB instance, shared across CRocksTxDB instances
// the same way the LevelDB backend shares its txdb singleton.
static rocksdb::DB* g_rocksdb = nullptr;
static rocksdb::ColumnFamilyHandle* g_cf_handles[5] = {}; // indexed by CF_ enum
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;
// crash recovery replays from block files anyway. Default WriteOptions may
@@ -130,27 +148,8 @@ static rocksdb::Options GetRocksOptions()
return opts;
}
// ─── Column family names ───────────────────────────────────────────────────
static const std::string CF_NAMES[] = {
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
};
// Column-family partitioning is disabled (see CRocksTxDB::GetCF). All keys live
// in the default column family, mirroring the single-keyspace LevelDB backend.
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);
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;
rocksdb::Options listOpts = options;
listOpts.create_if_missing = false;
rocksdb::DB::ListColumnFamilies(listOpts, directory.string(), &existingCFs);
bool needsCreate = (existingCFs.size() <= 1); // Only "default" or empty
std::vector<rocksdb::ColumnFamilyDescriptor> cfDescs;
for (int i = 0; i < CF_COUNT; i++) {
// Include this CF if it already exists OR if we're creating new
bool exists = false;
for (auto& name : existingCFs)
if (name == CF_NAMES[i]) { exists = true; break; }
if (exists || needsCreate) {
rocksdb::ColumnFamilyOptions cfOpts = 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));
}
cfDescs.push_back(rocksdb::ColumnFamilyDescriptor(
rocksdb::kDefaultColumnFamilyName, rocksdb::ColumnFamilyOptions(options)));
for (const auto& name : existingCFs) {
if (name == rocksdb::kDefaultColumnFamilyName)
continue; // default already added above
cfDescs.push_back(rocksdb::ColumnFamilyDescriptor(
name, rocksdb::ColumnFamilyOptions(options)));
}
std::vector<rocksdb::ColumnFamilyHandle*> handles;
rocksdb::Status status = OpenRocksDBCF(options, directory.string(),
cfDescs, &handles, &g_rocksdb);
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());
status = OpenRocksDB(options, directory.string(), &g_rocksdb);
if (!status.ok()) {
throw runtime_error(strprintf("open_rocksdb(): error opening database: %s",
status.ToString().c_str()));
}
g_cf_handles.clear(); // plain Open returns no handles to manage
return;
}
// Store handles in the global array (CF names map directly to indices)
for (size_t i = 0; i < handles.size() && i < CF_COUNT; i++) {
// Match handle to our index by name
std::string hname = handles[i]->GetName();
for (int j = 0; j < CF_COUNT; j++) {
if (hname == CF_NAMES[j]) {
g_cf_handles[j] = handles[i];
break;
}
}
}
g_cf_enabled = true;
// We only ever route to the default CF, so keep CF routing off. Any extra
// handles opened above for legacy-database compatibility are unused for
// routing but MUST be retained so close_rocksdb() can destroy them before
// the DB is deleted (RocksDB API requirement).
g_cf_handles = handles;
g_cf_enabled = false;
}
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",
DATABASE_VERSION);
delete g_rocksdb;
g_rocksdb = pdb = nullptr;
close_rocksdb();
pdb = nullptr;
delete activeBatch;
activeBatch = nullptr;
@@ -277,8 +272,8 @@ CRocksTxDB::~CRocksTxDB()
void CRocksTxDB::Close()
{
delete g_rocksdb;
g_rocksdb = pdb = nullptr;
close_rocksdb();
pdb = nullptr;
delete activeBatch;
activeBatch = nullptr;
}
@@ -351,15 +346,30 @@ bool CRocksTxDB::ScanBatch(const std::string& key, std::string* value, bool* del
}
// ─── 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; // 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
return nullptr; // single keyspace: always the default column family
}
bool CRocksTxDB::ReadRaw(const std::string& key, std::string& value) const