From 70b35ac11ab00a001b37cd7df55b405104070916 Mon Sep 17 00:00:00 2001 From: DanS Date: Fri, 16 Oct 2026 21:22:16 -0500 Subject: [PATCH] wallet/db: keep the BDB logs when the shutdown flush fails CDBEnv::Flush(true) removed database/ (log_archive DB_ARCH_REMOVE, then remove_all) once mapFileUseCount was empty. It did that even when the checkpoint or lsn_reset before it had failed, or the environment had panicked (BDB0060). In those states wallet.dat can be missing committed changes that only the logs hold: - a page write failed (EBADF/ESPIPE through an fd closed behind BDB's back, the wolfSSL CloseSocket bug), so the page stayed dirty in the mpool cache and the checkpoint never completed; - pages written after the last lsn_reset carry real LSNs that point into those logs. Deleting the logs turned a wallet that the next start's DB_RECOVER would repair into one that silently lost those changes. A std::terminate kept the logs; the clean shutdown that ThreadNotifyWallets now does on a wallet DB error deleted them. Remember a failed txn_checkpoint or lsn_reset (in Flush and in CheckpointLSN, which the periodic flush and backupwallet use). Before removing anything, also check the environment for a panic. get_flags reports it, and itself fails with DB_RUNRECOVERY once the environment has panicked. In any of those cases keep database/ and log why. If the kept logs are unusable, the next start already moves database/ aside and retries (CWallet::Verify). WalletDbEnv tests: - a clean shutdown still removes the logs; - a panicked environment (DB_PANIC_ENVIRONMENT) keeps them; - a failed lsn_reset keeps them. With the old db.cpp only the first passes. Co-Authored-By: Claude Opus 5.5 --- src/Makefile.gtest.include | 3 +- src/gtest/test_wallet_dbenv.cpp | 58 +++++++++++++++++++++++++++++++++ src/wallet/db.cpp | 41 ++++++++++++++++++----- src/wallet/db.h | 4 +++ 4 files changed, 96 insertions(+), 10 deletions(-) create mode 100644 src/gtest/test_wallet_dbenv.cpp diff --git a/src/Makefile.gtest.include b/src/Makefile.gtest.include index 35042a0b7..0d474b5c4 100644 --- a/src/Makefile.gtest.include +++ b/src/Makefile.gtest.include @@ -17,7 +17,8 @@ hush_gtest_SOURCES = \ gtest/test_stratum_jobid.cpp \ gtest/test_wallet_notedata.cpp \ gtest/test_net_closesocket.cpp \ - gtest/test_fd_defense.cpp + gtest/test_fd_defense.cpp \ + gtest/test_wallet_dbenv.cpp hush_gtest_CPPFLAGS = $(AM_CPPFLAGS) -DMULTICORE -fopenmp -DBINARY_OUTPUT -DCURVE_ALT_BN128 -DSTATIC $(BITCOIN_INCLUDES) hush_gtest_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) diff --git a/src/gtest/test_wallet_dbenv.cpp b/src/gtest/test_wallet_dbenv.cpp new file mode 100644 index 000000000..9cb0ce327 --- /dev/null +++ b/src/gtest/test_wallet_dbenv.cpp @@ -0,0 +1,58 @@ +// Copyright (c) 2016-2024 The Hush developers +// Distributed under the GPLv3 software license, see the accompanying +// file COPYING or https://www.gnu.org/licenses/gpl-3.0.en.html + +#include + +#include + +#include "wallet/db.h" + +namespace { + +// A fresh environment in a temp dir with one committed record in "wallet.dat", so database/ holds a log file. +class WalletDbEnv : public ::testing::Test { +protected: + boost::filesystem::path dir; + CDBEnv env; + + void SetUp() override { + dir = boost::filesystem::temp_directory_path() / boost::filesystem::unique_path("dbenv-%%%%%%%%"); + boost::filesystem::create_directories(dir); + ASSERT_TRUE(env.Open(dir)); + Db db(env.dbenv, DB_CXX_NO_EXCEPTIONS); + ASSERT_EQ(0, db.open(NULL, "wallet.dat", "main", DB_BTREE, DB_CREATE | DB_AUTO_COMMIT, 0)); + char k[] = "key", v[] = "value"; + Dbt key(k, sizeof k), value(v, sizeof v); + ASSERT_EQ(0, db.put(NULL, &key, &value, 0)); + ASSERT_EQ(0, db.close(0)); + ASSERT_TRUE(boost::filesystem::exists(Log())); + } + void TearDown() override { + env.Close(); + boost::system::error_code ec; + boost::filesystem::remove_all(dir, ec); + } + boost::filesystem::path Log() const { return dir / "database" / "log.0000000001"; } +}; + +} // namespace + +TEST_F(WalletDbEnv, CleanShutdownRemovesTheLogs) { + env.Flush(true); + EXPECT_FALSE(boost::filesystem::exists(dir / "database")); +} + +// BDB0060 PANIC (a page write failed, e.g. through an fd closed behind BDB's back): the logs may hold committed +// changes wallet.dat never got, and the next start's DB_RECOVER needs them. +TEST_F(WalletDbEnv, PanickedEnvironmentKeepsTheLogs) { + ASSERT_EQ(0, env.dbenv->set_flags(DB_PANIC_ENVIRONMENT, 1)); + env.Flush(true); + EXPECT_TRUE(boost::filesystem::exists(Log())); +} + +TEST_F(WalletDbEnv, FailedCheckpointKeepsTheLogs) { + env.CheckpointLSN("no-such-file.dat"); // lsn_reset fails + env.Flush(true); + EXPECT_TRUE(boost::filesystem::exists(Log())); +} diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index c924628ad..5f2df38a3 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -61,6 +61,7 @@ void CDBEnv::Reset() dbenv = new DbEnv(DB_CXX_NO_EXCEPTIONS); fDbEnvInit = false; fMockDb = false; + fKeepLogs = false; } CDBEnv::CDBEnv() : dbenv(NULL) @@ -276,10 +277,12 @@ bool CDBEnv::Salvage(const std::string& strFile, bool fAggressive, std::vectortxn_checkpoint(0, 0, 0); + if (dbenv->txn_checkpoint(0, 0, 0) != 0) + fKeepLogs = true; if (fMockDb) return; - dbenv->lsn_reset(strFile.c_str(), 0); + if (dbenv->lsn_reset(strFile.c_str(), 0) != 0) + fKeepLogs = true; } @@ -498,10 +501,16 @@ void CDBEnv::Flush(bool fShutdown) // Move log data to the dat file CloseDb(strFile); LogPrint("db", "CDBEnv::Flush: %s checkpoint\n", strFile); - dbenv->txn_checkpoint(0, 0, 0); + int ret = dbenv->txn_checkpoint(0, 0, 0); + if (ret != 0) { + LogPrintf("CDBEnv::Flush: %s checkpoint failed: %s\n", strFile, DbEnv::strerror(ret)); + fKeepLogs = true; + } LogPrint("db", "CDBEnv::Flush: %s detach\n", strFile); - if (!fMockDb) - dbenv->lsn_reset(strFile.c_str(), 0); + if (!fMockDb && (ret = dbenv->lsn_reset(strFile.c_str(), 0)) != 0) { + LogPrintf("CDBEnv::Flush: %s lsn_reset failed: %s\n", strFile, DbEnv::strerror(ret)); + fKeepLogs = true; + } LogPrint("db", "CDBEnv::Flush: %s closed\n", strFile); mapFileUseCount.erase(mi++); } else @@ -511,10 +520,24 @@ void CDBEnv::Flush(bool fShutdown) if (fShutdown) { char** listp; if (mapFileUseCount.empty()) { - dbenv->log_archive(&listp, DB_ARCH_REMOVE); - Close(); - if (!fMockDb) - boost::filesystem::remove_all(boost::filesystem::path(strPath) / "database"); + // After a failed flush, or once the environment has panicked (BDB0060, e.g. after a page + // write failed), wallet.dat can be missing committed changes that only the logs hold. + // Removing them would turn a wallet the next start recovers (DB_RECOVER) into one that + // silently lost them. If the logs are unusable, the next start moves database/ aside. + // (get_flags itself fails with DB_RUNRECOVERY once the environment has panicked.) + u_int32_t envFlags = 0; + if (dbenv->get_flags(&envFlags) != 0 || (envFlags & DB_PANIC_ENVIRONMENT)) + fKeepLogs = true; + if (fKeepLogs && !fMockDb) { + LogPrintf("CDBEnv::Flush: the wallet database was not flushed cleanly; keeping %s for recovery at the next start\n", + (boost::filesystem::path(strPath) / "database").string()); + Close(); + } else { + dbenv->log_archive(&listp, DB_ARCH_REMOVE); + Close(); + if (!fMockDb) + boost::filesystem::remove_all(boost::filesystem::path(strPath) / "database"); + } } } } diff --git a/src/wallet/db.h b/src/wallet/db.h index 1e4de9d78..73ea50e43 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -28,6 +28,7 @@ #include "sync.h" #include "version.h" +#include #include #include #include @@ -53,6 +54,9 @@ class CDBEnv private: bool fDbEnvInit; bool fMockDb; + // A checkpoint or lsn_reset failed: the logs in database/ may hold the only copy of committed + // wallet changes, so the shutdown flush must leave them for DB_RECOVER at the next start. + std::atomic fKeepLogs; // Don't change into boost::filesystem::path, as that can result in // shutdown problems/crashes caused by a static initialized internal pointer. std::string strPath;