sami7777/triangles_v5
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

chaindb: W2 iterator-scoping + H4 marker-verify + W1 INADDR_ANY

Browse Source 
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.
This commit is contained in:
Krystie
2026-07-02 01:29:03 -07:00
parent f9d1723f6e
commit 6cadf7f496
3 changed files with 152 additions and 27 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/chaindb_migrate.cpp
+80 -24
View File
@@ -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();
src/init.cpp
+12 -3
View File
@@ -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);
}
src/test/chaindb_runtime_tests.cpp
+60
View File
@@ -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()