backport: bitcoin/bitcoin#28609, #34215, #34226, #34370, partials #31423, #34156 (migratewallet fixes from 28.x)

src/wallet/bdb.cpp

@@ -13,6 +13,7 @@
 #include <util/strencodings.h>
 #include <util/translation.h>
 
+#include <set>
 #include <stdint.h>
 
 #include <db_cxx.h>
@@ -331,6 +332,53 @@ bool BerkeleyDatabase::Verify(bilingual_str& errorStr)
     return true;
 }
 
+std::vector<fs::path> BerkeleyDatabase::Files()
+{
+    std::vector<fs::path> files;
+    // If the wallet is the *only* file, clean up the entire BDB environment
+    constexpr auto build_files_list = [](std::vector<fs::path>& files, const std::shared_ptr<BerkeleyEnvironment>& env, const fs::path& filename) {
+        if (env->m_databases.size() != 1) return false;
+
+        const auto env_dir = env->Directory();
+        const auto db_subdir = env_dir / "database";
+        if (fs::exists(db_subdir)) {
+            if (!fs::is_directory(db_subdir)) return false;
+            for (const auto& entry : fs::directory_iterator(db_subdir)) {
+                const auto& path = entry.path().filename();
+                if (!fs::PathToString(path).starts_with("log.")) {
+                    return false;
+                }
+                files.emplace_back(entry.path());
+            }
+        }
+        const std::set<fs::path> allowed_paths = {
+            filename,
+            "db.log",
+            ".walletlock",
+            "database"
+        };
+        for (const auto& entry : fs::directory_iterator(env_dir)) {
+            const auto& path = entry.path().filename();
+            if (allowed_paths.contains(path)) {
+                files.emplace_back(entry.path());
+            } else if (fs::is_directory(entry.path())) {
+                // Subdirectories can't possibly be using this db env, and is expected if this is a non-directory wallet
+                // Do not include them in Files, but still allow the env cleanup
+            } else {
+                return false;
+            }
+        }
+        return true;
+    };
+    try {
+        if (build_files_list(files, env, m_filename)) return files;
+    } catch (...) {
+        // Give up building the comprehensive file list if any error occurs
+    }
+    // Otherwise, it's only really safe to delete the one wallet file
+    return {env->Directory() / m_filename};
+}
+
 void BerkeleyEnvironment::CheckpointLSN(const std::string& strFile)
 {
     dbenv->txn_checkpoint(0, 0, 0);

src/wallet/bdb.h

@@ -129,6 +129,8 @@ class BerkeleyDatabase : public WalletDatabase
     /** Return path to main database filename */
     std::string Filename() override { return fs::PathToString(env->Directory() / m_filename); }
 
+    std::vector<fs::path> Files() override;
+
     std::string Format() override { return "bdb"; }
     /**
      * Pointer to shared database environment.

src/wallet/db.h

@@ -162,6 +162,9 @@ class WalletDatabase
     /** Return path to main database file for logs and error messages. */
     virtual std::string Filename() = 0;
 
+    /** Return paths to all database created files */
+    virtual std::vector<fs::path> Files() = 0;
+
     virtual std::string Format() = 0;
 
     std::atomic<unsigned int> nUpdateCounter;

src/wallet/dump.cpp

@@ -280,11 +280,17 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs::
 
         dump_file.close();
     }
+    // On failure, gather the paths to remove
+    std::vector<fs::path> paths_to_remove = wallet->GetDatabase().Files();
+    if (!name.empty()) paths_to_remove.push_back(wallet_path);
+
     wallet.reset(); // The pointer deleter will close the wallet for us.
 
     // Remove the wallet dir if we have a failure
     if (!ret) {
-        fs::remove_all(wallet_path);
+        for (const auto& p : paths_to_remove) {
+            fs::remove(p);
+        }
     }
 
     return ret;

src/wallet/salvage.cpp

@@ -66,6 +66,7 @@ class DummyDatabase : public WalletDatabase
     void IncrementUpdateCounter() override { ++nUpdateCounter; }
     void ReloadDbEnv() override {}
     std::string Filename() override { return "dummy"; }
+    std::vector<fs::path> Files() override { return {}; }
     std::string Format() override { return "dummy"; }
     std::unique_ptr<DatabaseBatch> MakeBatch(bool flush_on_close = true) override { return std::make_unique<DummyBatch>(); }
 };

src/wallet/sqlite.cpp

@@ -109,12 +109,12 @@ Mutex SQLiteDatabase::g_sqlite_mutex;
 int SQLiteDatabase::g_sqlite_count = 0;
 
 SQLiteDatabase::SQLiteDatabase(const fs::path& dir_path, const fs::path& file_path, const DatabaseOptions& options, bool mock)
-    : WalletDatabase(), m_mock(mock), m_dir_path(fs::PathToString(dir_path)), m_file_path(fs::PathToString(file_path)), m_use_unsafe_sync(options.use_unsafe_sync)
+    : WalletDatabase(), m_mock(mock), m_dir_path(dir_path), m_file_path(fs::PathToString(file_path)), m_use_unsafe_sync(options.use_unsafe_sync)
 {
     {
         LOCK(g_sqlite_mutex);
         LogPrintf("Using SQLite Version %s\n", SQLiteDatabaseVersion());
-        LogPrintf("Using wallet %s\n", m_dir_path);
+        LogPrintf("Using wallet %s\n", fs::PathToString(m_dir_path));
 
         if (++g_sqlite_count == 1) {
             // Setup logging
@@ -250,7 +250,7 @@ void SQLiteDatabase::Open()
 
     if (m_db == nullptr) {
         if (!m_mock) {
-            TryCreateDirectories(fs::PathFromString(m_dir_path));
+            TryCreateDirectories(m_dir_path);
         }
         int ret = sqlite3_open_v2(m_file_path.c_str(), &m_db, flags, nullptr);
         if (ret != SQLITE_OK) {

src/wallet/sqlite.h

@@ -76,7 +76,7 @@ class SQLiteDatabase : public WalletDatabase
 private:
     const bool m_mock{false};
 
-    const std::string m_dir_path;
+    const fs::path m_dir_path;
 
     const std::string m_file_path;
 
@@ -133,6 +133,14 @@ class SQLiteDatabase : public WalletDatabase
     void IncrementUpdateCounter() override { ++nUpdateCounter; }
 
     std::string Filename() override { return m_file_path; }
+    /** Return paths to all database created files */
+    std::vector<fs::path> Files() override
+    {
+        std::vector<fs::path> files;
+        files.emplace_back(m_dir_path / fs::PathFromString(m_file_path));
+        files.emplace_back(m_dir_path / fs::PathFromString(m_file_path + "-journal"));
+        return files;
+    }
     std::string Format() override { return "sqlite"; }
 
     /** Make a SQLiteBatch connected to this database */

src/wallet/test/util.h

@@ -117,6 +117,7 @@ class MockableDatabase : public WalletDatabase
     void ReloadDbEnv() override {}
 
     std::string Filename() override { return "mockable"; }
+    std::vector<fs::path> Files() override { return {}; }
     std::string Format() override { return "mock"; }
     std::unique_ptr<DatabaseBatch> MakeBatch(bool flush_on_close = true) override { return std::make_unique<MockableBatch>(m_records, m_pass); }
 };

src/wallet/transaction.cpp

@@ -24,4 +24,9 @@ int64_t CWalletTx::GetTxTime() const
     int64_t n = nTimeSmart;
     return n ? n : nTimeReceived;
 }
+
+void CWalletTx::CopyFrom(const CWalletTx& _tx)
+{
+    *this = _tx;
+}
 } // namespace wallet

src/wallet/transaction.h

@@ -341,11 +341,15 @@ class CWalletTx
     bool IsCoinBase() const { return tx->IsCoinBase(); }
     bool IsPlatformTransfer() const { return tx->IsPlatformTransfer(); }
 
+private:
     // Disable copying of CWalletTx objects to prevent bugs where instances get
     // copied in and out of the mapWallet map, and fields are updated in the
     // wrong copy.
-    CWalletTx(CWalletTx const &) = delete;
-    void operator=(CWalletTx const &x) = delete;
+    CWalletTx(const CWalletTx&) = default;
+    CWalletTx& operator=(const CWalletTx&) = default;
+public:
+    // Instead have an explicit copy function
+    void CopyFrom(const CWalletTx&);
 };
 
 struct WalletTxOrderComparator {