From fc7ad5bb6911678dd9a9e0d6b520f4b661fad7b6 Mon Sep 17 00:00:00 2001 From: Krystie Date: Wed, 1 Jul 2026 16:09:25 -0700 Subject: [PATCH] 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. --- src/chaindb_migrate.cpp | 7 ++ src/init.cpp | 23 ++++- src/test/chaindb_runtime_tests.cpp | 25 +++--- src/txdb-factory.cpp | 5 +- src/txdb-leveldb.cpp | 11 ++- src/txdb-rocksdb.cpp | 140 +++++++++++++++-------------- 6 files changed, 130 insertions(+), 81 deletions(-) diff --git a/src/chaindb_migrate.cpp b/src/chaindb_migrate.cpp index 3088de1..1878998 100644 --- a/src/chaindb_migrate.cpp +++ b/src/chaindb_migrate.cpp @@ -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"); diff --git a/src/init.cpp b/src/init.cpp index 566133e..23ad4f4 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -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 diff --git a/src/test/chaindb_runtime_tests.cpp b/src/test/chaindb_runtime_tests.cpp index 1b1cfdf..307b2d7 100644 --- a/src/test/chaindb_runtime_tests.cpp +++ b/src/test/chaindb_runtime_tests.cpp @@ -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() diff --git a/src/txdb-factory.cpp b/src/txdb-factory.cpp index 62b287c..bc4a32f 100644 --- a/src/txdb-factory.cpp +++ b/src/txdb-factory.cpp @@ -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(c)); if (s == "leveldb") diff --git a/src/txdb-leveldb.cpp b/src/txdb-leveldb.cpp index b88fab0..4b9c00d 100644 --- a/src/txdb-leveldb.cpp +++ b/src/txdb-leveldb.cpp @@ -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); diff --git a/src/txdb-rocksdb.cpp b/src/txdb-rocksdb.cpp index a13de40..331d32f 100644 --- a/src/txdb-rocksdb.cpp +++ b/src/txdb-rocksdb.cpp @@ -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 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 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 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(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 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