Merge bitcoin/bitcoin#35655: wallet: Use in-memory SQLite for temporary wallet in exportwatchonlywallet

777d23f25c test: add regression test for in-memory SQLiteDatabase reopen (Pablo Martin)
d1e7f8c986 wallet: use in-memory SQLite for temporary wallet in exportwatchonlywallet (Pablo Martin)
ee43743f12 wallet: store m_additional_flags in SQLiteDatabase to fix reopen path (Pablo Martin)

Pull request description:

  Since #33032 landed (in-memory `SQLiteDatabase` via `SQLITE_OPEN_MEMORY`), the intermediate wallet built during `exportwatchonlywallet` can live entirely in memory instead of being written to the wallets directory as a temporary file.

  The temp wallet is a pure build artifact: it is populated with descriptors, transactions, and address book data, then immediately discarded once `BackupWallet()` copies its contents to the destination file. Making it in-memory removes all on-disk footprint and eliminates the `cleanup_watchonly_wallet` RAII handler — along with the `wallet_path` and `cleanup_files` variables it needed — which previously ensured the temp files were deleted on both success and failure paths.

  This PR introduces `InMemoryWalletDatabase` (a minimal `SQLiteDatabase` subclass) and `MakeInMemoryWalletDatabase()` factory in `sqlite.h/cpp`, following the same pattern as `MockableSQLiteDatabase` / `CreateMockableWalletDatabase()`. `MockableSQLiteDatabase` now derives from `InMemoryWalletDatabase`, removing its redundant `Files()` override.

  Suggested by Sjors in #32489 ([comment](https://github.com/bitcoin/bitcoin/pull/32489#issuecomment-4874894955)).

  ---
  Also fixes a related issue found (by Sjors) during review:

  - `SQLiteDatabase::Open()` (the no-arg public override) hardcoded 0 as `additional_flags` when reopening after a failed `TxnAbort()`, which would reopen an in-memory database as on-disk. Fixed by storing `m_additional_flags` in the constructor and using it in the reopen path. For in-memory databases, both the `force_conn_refresh` path and the public `Open()` now throw instead of silently creating a fresh empty connection. A regression test for the `Open()` throw is included in a separate commit.

  ---
  As a follow-up, `InMemoryWalletDatabase` could replace `MockableSQLiteDatabase` in `src/bench/` (5 files, 6 call sites), since benchmarks don't need mock-specific behaviour and benefit from using the same in-memory path as production code.

ACKs for top commit:
  Sjors:
    re-utACK 777d23f25c
  achow101:
    ACK 777d23f25c
  janb84:
    ACK 777d23f25c

Tree-SHA512: 71178ce99c7ebc0fc5ba17956c27d37f90e3d36cefbdc0d15d1424b7a70bf15f05a13cb03c268d885678aba1fd3c90567d8389d3680650c8ae46f4cb4b10b28a

src/wallet/export.cpp

@@ -9,6 +9,7 @@
 #include <util/expected.h>
 #include <wallet/scriptpubkeyman.h>
 #include <wallet/context.h>
+#include <wallet/sqlite.h>
 #include <wallet/wallet.h>
 
 #include <fstream>
@@ -69,40 +70,15 @@ util::Result<std::string> ExportWatchOnlyWallet(const CWallet& wallet, const fs:
         return util::Error{_("Error: Wallet has no descriptors to export")};
     }
 
-    // Setup DatabaseOptions to create a new sqlite database
-    DatabaseOptions options;
-    options.require_existing = false;
-    options.require_create = true;
-    options.require_format = DatabaseFormat::SQLITE;
-
     // Make the wallet with the same flags as this wallet, but without private keys
-    options.create_flags = wallet.GetWalletFlags() | WALLET_FLAG_DISABLE_PRIVATE_KEYS;
+    const uint64_t create_flags = wallet.GetWalletFlags() | WALLET_FLAG_DISABLE_PRIVATE_KEYS;
 
-    // Make the watchonly wallet
-    DatabaseStatus status;
+    // Create the temporary watchonly wallet in memory to avoid leaving files on disk
     std::vector<bilingual_str> warnings;
-    std::string wallet_name = wallet.GetName() + "_watchonly_temp";
     bilingual_str error;
-    std::unique_ptr<WalletDatabase> database = MakeWalletDatabase(wallet_name, options, status, error);
-    if (!database) {
-        return util::Error{strprintf(_("Wallet file creation failed: %s"), error)};
-    }
-
-    // Always remove the temporary wallet files, even when returning early on error.
-    std::shared_ptr<CWallet> watchonly_wallet;
-    fs::path wallet_path = fs::PathFromString(database->Filename()).parent_path();
-    std::vector<fs::path> cleanup_files = database->Files();
-    auto cleanup_watchonly_wallet = interfaces::MakeCleanupHandler([&watchonly_wallet, &wallet_path, &cleanup_files] {
-        if (watchonly_wallet) watchonly_wallet.reset();
-        for (const auto& file : cleanup_files) {
-            fs::remove(file);
-        }
-        fs::remove(wallet_path);
-    });
-
     WalletContext empty_context;
     empty_context.args = context.args;
-    watchonly_wallet = CWallet::CreateNew(empty_context, wallet_name, std::move(database), options.create_flags, /*born_encrypted=*/false, error, warnings);
+    std::shared_ptr<CWallet> watchonly_wallet = CWallet::CreateNew(empty_context, /*name=*/wallet.GetName() + "_watchonly_temp", MakeInMemoryWalletDatabase(), create_flags, /*born_encrypted=*/false, error, warnings);
     if (!watchonly_wallet) {
         return util::Error{strprintf(_("Error: Failed to create new watchonly wallet. %s"), error)};
     }

src/wallet/sqlite.cpp

@@ -116,7 +116,7 @@ SQLiteDatabase::SQLiteDatabase(const fs::path& dir_path, const fs::path& file_pa
 {}
 
 SQLiteDatabase::SQLiteDatabase(const fs::path& dir_path, const fs::path& file_path, const DatabaseOptions& options, int additional_flags)
-    : WalletDatabase(), m_dir_path(dir_path), m_file_path(fs::PathToString(file_path)), m_write_semaphore(1), m_use_unsafe_sync(options.use_unsafe_sync)
+    : WalletDatabase(), m_dir_path(dir_path), m_file_path(fs::PathToString(file_path)), m_additional_flags(additional_flags), m_write_semaphore(1), m_use_unsafe_sync(options.use_unsafe_sync)
 {
     {
         LOCK(g_sqlite_mutex);
@@ -139,7 +139,7 @@ SQLiteDatabase::SQLiteDatabase(const fs::path& dir_path, const fs::path& file_pa
     }
 
     try {
-        Open(additional_flags);
+        Open(m_additional_flags);
     } catch (const std::runtime_error&) {
         // If open fails, cleanup this object and rethrow the exception
         Cleanup();
@@ -247,7 +247,10 @@ bool SQLiteDatabase::Verify(bilingual_str& error)
 
 void SQLiteDatabase::Open()
 {
-    Open(/*additional_flags*/0);
+    if (m_additional_flags & SQLITE_OPEN_MEMORY) {
+        throw std::runtime_error("SQLiteDatabase: Cannot reopen an in-memory database");
+    }
+    Open(m_additional_flags);
 }
 
 void SQLiteDatabase::Open(int additional_flags)
@@ -448,6 +451,9 @@ void SQLiteBatch::Close()
     }
 
     if (force_conn_refresh) {
+        if (m_database.m_additional_flags & SQLITE_OPEN_MEMORY) {
+            throw std::runtime_error("SQLiteDatabase: Cannot recover in-memory database connection");
+        }
         m_database.Close();
         try {
             m_database.Open();
@@ -716,6 +722,15 @@ std::unique_ptr<SQLiteDatabase> MakeSQLiteDatabase(const fs::path& path, const D
     }
 }
 
+InMemoryWalletDatabase::InMemoryWalletDatabase()
+    : SQLiteDatabase(fs::path{}, fs::path{":memory:"}, DatabaseOptions(), SQLITE_OPEN_MEMORY)
+{}
+
+std::unique_ptr<WalletDatabase> MakeInMemoryWalletDatabase()
+{
+    return std::make_unique<InMemoryWalletDatabase>();
+}
+
 std::string SQLiteDatabaseVersion()
 {
     return std::string(sqlite3_libversion());

src/wallet/sqlite.h

@@ -103,10 +103,14 @@ public:
 class SQLiteDatabase : public WalletDatabase
 {
 private:
+    friend class SQLiteBatch;
+
     const fs::path m_dir_path;
 
     const std::string m_file_path;
 
+    const int m_additional_flags;
+
     /**
      * This mutex protects SQLite initialization and shutdown.
      * sqlite3_config() and sqlite3_shutdown() are not thread-safe (sqlite3_initialize() is).
@@ -171,8 +175,19 @@ public:
     bool m_use_unsafe_sync;
 };
 
+/** An in-memory SQLiteDatabase. Used as a temporary build artifact where no
+ *  on-disk persistence is needed. */
+class InMemoryWalletDatabase : public SQLiteDatabase
+{
+public:
+    InMemoryWalletDatabase();
+    std::vector<fs::path> Files() override { return {}; }
+};
+
 std::unique_ptr<SQLiteDatabase> MakeSQLiteDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error);
 
+std::unique_ptr<WalletDatabase> MakeInMemoryWalletDatabase();
+
 std::string SQLiteDatabaseVersion();
 } // namespace wallet
 

src/wallet/test/db_tests.cpp

@@ -297,5 +297,14 @@ BOOST_AUTO_TEST_CASE(concurrent_txn_dont_interfere)
     BOOST_CHECK_EQUAL(read_value, value2);
 }
 
+BOOST_AUTO_TEST_CASE(in_memory_database_cannot_reopen)
+{
+    // Reopening an in-memory database would create a fresh empty connection,
+    // silently losing all data. Open() must throw instead.
+    InMemoryWalletDatabase database;
+    database.Close();
+    BOOST_CHECK_THROW(database.Open(), std::runtime_error);
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 } // namespace wallet

src/wallet/test/util.cpp

@@ -116,7 +116,7 @@ CTxDestination getNewDestination(CWallet& w, OutputType output_type)
 }
 
 MockableSQLiteDatabase::MockableSQLiteDatabase()
-    : SQLiteDatabase(fs::PathFromString("mock/"), fs::PathFromString("mock/wallet.dat"), DatabaseOptions(), SQLITE_OPEN_MEMORY)
+    : InMemoryWalletDatabase()
 {}
 
 std::unique_ptr<WalletDatabase> CreateMockableWalletDatabase()

src/wallet/test/util.h

@@ -56,7 +56,7 @@ public:
 
 /** A WalletDatabase whose contents and return values can be modified as needed for testing
  **/
-class MockableSQLiteDatabase : public SQLiteDatabase
+class MockableSQLiteDatabase : public InMemoryWalletDatabase
 {
 public:
     MockableSQLiteDatabase();
@@ -64,7 +64,6 @@ public:
     bool Backup(const std::string& strDest) const override { return true; }
 
     std::string Filename() override { return "mockable"; }
-    std::vector<fs::path> Files() override { return {}; }
     std::string Format() override { return "sqlite-mock"; }
     std::unique_ptr<DatabaseBatch> MakeBatch() override { return std::make_unique<MockableSQLiteBatch>(*this); }
 };