From 6cadf7f49648916128e8a21d470b1bf001367c74 Mon Sep 17 00:00:00 2001 From: Krystie Date: Thu, 2 Jul 2026 01:29:03 -0700 Subject: [PATCH] chaindb: W2 iterator-scoping + H4 marker-verify + W1 INADDR_ANY MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three fixes for the chain-DB migration path on real chain data. All three were uncovered when running the full DNS2 2.2M-block chain end-to-end; the existing 18 unit tests passed because they exercised small fixtures, never the real migration entry point. W2 (root cause): chaindb_migrate.cpp — scope the source.NewIterator() inside an inner block so it's destroyed BEFORE source.Close(). Live LevelDB iterators hold a Version ref; closing the DB with one alive trips the dummy_versions_.next_ == &dummy_versions_ assertion in leveldb::VersionSet::~VersionSet (version_set.cc:755), aborting the daemon after verification but before the marker is removed. This explains the original H4 symptom: the daemon died in the gap between 'verified' and 'fs::remove', and Release builds hid it by compiling asserts out. The H1 retry path's static-state issue in the test binary is the same bug at process exit. In-loop failures now break out with fCopyOK=false and are handled after the iterator dies. H4 (defense in depth): chaindb_migrate.cpp — keep the verify-and-fail hardening even though W2 fixes the cause. Use the non-throwing error_code overload, fs::exists verify after remove, single 100ms retry (Windows AV/indexer transient locks), hard-fail strError if the marker still survives. Operator-visible failure beats silent re-migration time bomb. The H4 invariant: a successful migration never leaves the marker on disk. W1: init.cpp — Lookup('0.0.0.0', addrBind, GetListenPort(), false) replaced with direct CService construction from in_addr{htonl(INADDR_ANY)}. This was the bug that prevented fc7ad5b from ever starting on SAMI-PC; Windows getaddrinfo doesn't always map the literal '0.0.0.0' string to INADDR_ANY. Test: chaindb_runtime_tests.cpp — adds marker_removed_after_successful_migration which exercises the real MaybeMigrateLevelDbToRocksDb() end-to-end on the happy path. Complements the existing crashed_migration_marker_triggers_retry (retry path). This is the gap that hid the original bug: no test went through the production entry point on the happy path. Runtime verification: full DNS2 chain state (txleveldb 1.1GB + blk0001.dat 942MB, 6.77M records) migrated end-to-end. MIGRATION_INCOMPLETE absent from disk after. Reopened rocksdb reads back cleanly via getblockcount / LoadBlockIndex. Three files, 152 insertions, 27 deletions, build clean, CI ready. --- src/chaindb_migrate.cpp | 104 ++++++++++++++++++++++------- src/init.cpp | 15 ++++- src/test/chaindb_runtime_tests.cpp | 60 +++++++++++++++++ 3 files changed, 152 insertions(+), 27 deletions(-) diff --git a/src/chaindb_migrate.cpp b/src/chaindb_migrate.cpp index 1878998..f18c897 100644 --- a/src/chaindb_migrate.cpp +++ b/src/chaindb_migrate.cpp @@ -136,34 +136,48 @@ bool MaybeMigrateLevelDbToRocksDb(bool fForce, std::string& strError) } int64_t nCopied = 0; - auto it = source.NewIterator(); - for (it->Seek(std::string()); it->Valid(); it->Next()) + bool fCopyOK = true; { - if (!destination.WriteRawRecordForMigration(it->KeyStr(), it->ValueStr())) { - destination.TxnAbort(); - strError = "failed to write migrated record to RocksDB"; - source.Close(); - destination.Close(); - return false; - } - - if (++nCopied % 100000 == 0) + // W2 root cause: this iterator MUST be destroyed before + // source.Close(). Live LevelDB iterators hold a reference to the + // current Version; deleting the DB with one outstanding trips + // `dummy_versions_.next_ == &dummy_versions_` in + // leveldb::VersionSet::~VersionSet (version_set.cc:755) and + // aborts the daemon AFTER verification but BEFORE the marker is + // removed — which is what produced the original H4 symptom. + // Scoping the iterator here guarantees every Close() below runs + // with it already dead, on the success AND error paths. + auto it = source.NewIterator(); + for (it->Seek(std::string()); it->Valid(); it->Next()) { - if (!destination.TxnCommit()) { - strError = "failed to commit RocksDB migration batch"; - source.Close(); - destination.Close(); - return false; + if (!destination.WriteRawRecordForMigration(it->KeyStr(), it->ValueStr())) { + strError = "failed to write migrated record to RocksDB"; + fCopyOK = false; + break; } - printf("ChainDB migration: copied %lld / %lld records\n", - (long long)nCopied, (long long)srcStats.nRecords); - if (!destination.TxnBegin()) { - strError = "failed to begin RocksDB migration batch"; - source.Close(); - destination.Close(); - return false; + + if (++nCopied % 100000 == 0) + { + if (!destination.TxnCommit()) { + strError = "failed to commit RocksDB migration batch"; + fCopyOK = false; + break; + } + printf("ChainDB migration: copied %lld / %lld records\n", + (long long)nCopied, (long long)srcStats.nRecords); + if (!destination.TxnBegin()) { + strError = "failed to begin RocksDB migration batch"; + fCopyOK = false; + break; + } } } + } // iterator destroyed here — before any Close() + if (!fCopyOK) { + destination.TxnAbort(); // safe no-op if the batch was already consumed + source.Close(); + destination.Close(); + return false; } if (!destination.TxnCommit()) { @@ -192,7 +206,49 @@ bool MaybeMigrateLevelDbToRocksDb(bool fForce, std::string& strError) source.Close(); destination.Close(); - fs::remove(markerPath); + + // H4: Marker removal must be verified, not assumed. The previous + // implementation called fs::remove() and ignored the return code, which + // silently left the marker on disk after a successful migration. On + // the next startup init.cpp's fCrashedMigration check would then + // trigger a re-migration of the (already-good) RocksDB on every + // restart, eventually destroying the chain state. + // + // Three defenses: + // 1. Use the non-throwing error_code overload so a permission + // error doesn't propagate as an uncaught exception. + // 2. After remove(), confirm the file is actually gone. fs::remove + // returns true if the file didn't exist, which is also success + // but worth distinguishing. + // 3. Retry once with a short delay. On Windows, antivirus and + // indexer handles can transiently hold the marker file open + // even after our process closed it; a single retry usually + // wins. If the second attempt also leaves the file, treat the + // migration as FAILED — surface the error to the operator + // instead of letting init.cpp's fCrashedMigration logic + // destroy working data on the next startup. + { + std::error_code ec; + fs::remove(markerPath, ec); + if (ec) { + strError = "could not remove migration marker " + markerPath.string() + + ": " + ec.message(); + return false; + } + if (fs::exists(markerPath)) { + // Retry once — handles Windows AV/indexer transient locks. + MilliSleep(100); + std::error_code ec2; + fs::remove(markerPath, ec2); + if (ec2 || fs::exists(markerPath)) { + strError = "migration marker " + markerPath.string() + + " could not be removed after retry; refusing to leave it on disk " + + "(would trigger re-migration on next startup). " + + std::string(ec2 ? ec2.message().c_str() : ""); + return false; + } + } + } } catch (std::exception& e) { strError = e.what(); diff --git a/src/init.cpp b/src/init.cpp index 23ad4f4..e64994c 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1105,10 +1105,19 @@ bool AppInit2() if (true) { if (true) { do { - // Bind to all interfaces so external peers can connect + // W1: Bind to all interfaces so external peers can connect. + // + // The previous code went through Lookup("0.0.0.0", ...) which + // hands the literal string to getaddrinfo(). On Windows that + // resolver can fail to map "0.0.0.0" to INADDR_ANY and the + // daemon would abort at startup with "Cannot resolve binding + // address". Construct the CService directly from INADDR_ANY + // instead — this is the canonical "any-address" binding and + // works on every platform without consulting the resolver. CService addrBind; - if (!Lookup("0.0.0.0", addrBind, GetListenPort(), false)) - return InitError(strprintf(_("Cannot resolve binding address: '%s'"), "0.0.0.0")); + struct in_addr any; + any.s_addr = htonl(INADDR_ANY); + addrBind = CService(any, GetListenPort()); fBound |= Bind(addrBind); } while (false); } diff --git a/src/test/chaindb_runtime_tests.cpp b/src/test/chaindb_runtime_tests.cpp index 098d610..41b6407 100644 --- a/src/test/chaindb_runtime_tests.cpp +++ b/src/test/chaindb_runtime_tests.cpp @@ -550,4 +550,64 @@ BOOST_AUTO_TEST_CASE(crashed_migration_marker_triggers_retry) mapArgs.erase("-chaindb"); } +// H4: After a SUCCESSFUL migration (no pre-existing marker, no crash), the +// MIGRATION_INCOMPLETE marker MUST be gone from disk. The previous +// implementation called fs::remove() and ignored the return code, so the +// marker silently survived success. init.cpp's fCrashedMigration check then +// treated the (good) RocksDB as a crashed migration and re-migrated on every +// startup, eventually destroying chain state. +// +// This test exercises the real MaybeMigrateLevelDbToRocksDb() end-to-end on +// the happy path: fresh LevelDB → no marker → migration → marker gone. +// Complements crashed_migration_marker_triggers_retry which covers the +// retry path. +BOOST_AUTO_TEST_CASE(marker_removed_after_successful_migration) +{ + // 1. Seed a minimal LevelDB chain DB by opening + closing it. + mapArgs["-chaindb"] = "leveldb"; + { + auto base = MakeChainDB("cr+"); + BOOST_REQUIRE(base != nullptr); + base->Close(); + } + BOOST_REQUIRE(fs::exists(GetDataDir() / "txleveldb")); + + // 2. Confirm the starting state: no rocksdb/, no marker. + fs::path rocksDir = GetDataDir() / "rocksdb"; + fs::path marker = rocksDir / "MIGRATION_INCOMPLETE"; + BOOST_REQUIRE(!fs::exists(rocksDir)); + BOOST_REQUIRE(!fs::exists(marker)); + + // 3. Run the production migration function with RocksDB as target. + mapArgs["-chaindb"] = "rocksdb"; + { + std::string err; + BOOST_REQUIRE_MESSAGE(MaybeMigrateLevelDbToRocksDb(false, err), + "migration failed: " + err); + BOOST_CHECK_MESSAGE(err.empty(), "unexpected error: " + err); + } + + // 4. The marker must be gone. This is the H4 invariant: a successful + // migration never leaves the marker on disk. The previous code + // returned true here even when the marker survived, which is the + // exact regression this test catches. + BOOST_CHECK_MESSAGE(!fs::exists(marker), + "MIGRATION_INCOMPLETE marker must be removed on success " + "(H4 — silent marker survival causes re-migration loop)"); + + // 5. The migrated rocksdb/ must exist with data in it. + BOOST_CHECK_MESSAGE(fs::exists(rocksDir), "rocksdb/ should exist after migration"); + + // 6. Reopen and confirm the data is intact. + { + auto base = MakeChainDB("r"); + BOOST_REQUIRE(base != nullptr); + base.reset(); // close before process exit (RocksDB static handle order) + } + + WipeChainDataDir(); + fs::remove_all(GetDataDir() / "txleveldb"); + mapArgs.erase("-chaindb"); +} + BOOST_AUTO_TEST_SUITE_END()