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

pablomartin4btc commented at 2:25 AM on July 4, 2026: member

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).

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.

DrahtBot added the label Wallet on Jul 4, 2026

DrahtBot commented at 2:25 AM on July 4, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35655.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	janb84
Stale ACK	Sjors

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33034 (wallet: Store transactions in a separate sqlite table by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible places where comparison-specific test macros should replace generic comparisons:

src/wallet/test/db_tests.cpp BOOST_CHECK_THROW(database.Open(), std::runtime_error); -> consider BOOST_CHECK_EXCEPTION with a message matcher so the test verifies the specific failure reason, not just the exception type.

in test/functional/wallet_exported_watchonly.py:281 in ebfee857ad

 277 | @@ -278,6 +278,22 @@ def test_export_blank_wallet(self):
 278 |          assert_raises_rpc_error(-4, "Wallet has no descriptors to export", no_keys_wallet.exportwatchonlywallet, export_path)
 279 |          no_keys_wallet.unloadwallet()
 280 |  
 281 | +    def test_no_temp_wallet_on_disk(self):

Sjors commented at 9:32 AM on July 4, 2026:

I don't think this test is useful.

pablomartin4btc commented at 11:33 AM on July 4, 2026:

Fair point, I'll drop it. The existing tests already cover the exported wallet's correctness, and the test was checking a log string as a proxy for the internal implementation rather than any observable behavior.

pablomartin4btc force-pushed on Jul 4, 2026

Sjors commented at 2:56 PM on July 4, 2026: member

Concept ACK

Code review 532c677bf9828d483fc3b39ba18590b2ebec6f18.

My LLM friend found a potential issue: when TxnAbort() fails, SQLiteBatch::Close() tries to reopen the database, but it forgot about the SQLITE_OPEN_MEMORY flag. This is a problem in general, and can cause a crash here. It's worth fixing in a separate commit. E.g. you could have the SQLiteDatabase constructor store m_additional_flags.

Further more, consider the following patch:

consistently use <in-memory> in log messages
avoid using the display-only Filename() for file operations
- maybe rename it to DisplayFileName()
improves migration code to also delete journal files before hitting assert(false)

diff --git a/src/wallet/sqlite.h b/src/wallet/sqlite.h
index 33c9628c49..4709967d39 100644
--- a/src/wallet/sqlite.h
+++ b/src/wallet/sqlite.h
@@ -178,5 +178,5 @@ class InMemoryWalletDatabase : public SQLiteDatabase
 public:
     InMemoryWalletDatabase();
-    std::string Filename() override { return "memory"; }
+    std::string Filename() override { return "<in-memory>"; }
     std::vector<fs::path> Files() override { return {}; }
 };
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index be618efbd6..6714b4ad95 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3882,8 +3882,8 @@ bool CWallet::MigrateToSQLite(bilingual_str& error)
     }

-    // Close this database and delete the file
-    fs::path db_path = fs::PathFromString(m_database->Filename());
+    // Close this database and delete its file(s) - just one for BerkeleyRO
+    const std::vector<fs::path> old_files = m_database->Files();
     m_database->Close();
-    fs::remove(db_path);
+    for (const fs::path& file : old_files) fs::remove(file);

     // Generate the path for the location of the migrated wallet
@@ -3909,5 +3909,5 @@ bool CWallet::MigrateToSQLite(bilingual_str& error)
             batch->TxnAbort();
             m_database->Close();
-            fs::remove(m_database->Filename());
+            for (const fs::path& file : m_database->Files()) fs::remove(file);
             assert(false); // This is a critical error, the new db could not be written to. The original db exists as a backup, but we should not continue execution.
         }
@@ -4418,5 +4418,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
             // Wallets stored directly as files in the top-level directory
             // (e.g. default unnamed wallets) don’t have a removable parent directory.
-            wallet_empty_dirs_to_remove.insert(fs::PathFromString(wallet.GetDatabase().Filename()).parent_path());
+            if (!files.empty()) {
+                wallet_empty_dirs_to_remove.insert(files.front().parent_path());
+            }
         }
     };

</details>

pablomartin4btc commented at 6:36 PM on July 4, 2026: member

... a potential issue: when TxnAbort() fails, SQLiteBatch::Close() tries to reopen the database, but it forgot about the SQLITE_OPEN_MEMORY flag. This is a problem in general, and can cause a crash here. It's worth fixing in a separate commit. E.g. you could have the SQLiteDatabase constructor store m_additional_flags.

I'll add a fix for that on a separate commit (first/ base) as suggested, thanks!

Further more, consider the following patch:

consistently use <in-memory> in log messages

I agree, will change it.

avoid using the display-only Filename() for file operations

maybe rename it to DisplayFileName()

I agree with this also, but I'll do on a separate PR because I found that there are even more places to change (everything is within src/wallet/):

1 pure virtual declaration: db.h:156
3 overrides: sqlite.h:153, test/util.h:66, migrate.h:52
5 display callers:
- sqlite.cpp:56 — LogTrace log message
- sqlite.cpp:278 — LogWarning log message
- wallet.cpp:2386 — used in error/log messages after load
- wallet.cpp:3105 (CreateNew) — logs "Wallet completed creation in %dms at %s"
- wallet.cpp:3160 (LoadExisting) — same pattern
4 file op callers: wallet.cpp:3885, wallet.cpp:3911, wallet.cpp:4420, export.cpp:93 - the last one disappears with this PR, the remaining three are fixed in the commit below.

improves migration code to also delete journal files before hitting assert(false)

On the migration fixes (wallet.cpp:3885, 3911, 4420) — these are still there and are genuine pre-existing issues:

Line 3885/3887: closes the old BerkeleyDB and removes only the main file, potentially missing its journal;
Line 3911: closes the new SQLite db and removes only the main file, potentially leaving wallet.dat-journal behind before hitting assert(false).

Although they're independent of this PR, I think these are worth fixing and I'll add a 3rd commit with it. Thank you!

pablomartin4btc force-pushed on Jul 4, 2026

pablomartin4btc commented at 7:18 PM on July 4, 2026: member

-<ins>Updates</ins>:

Addressed @Sjors's feedback:
- Added a fix for a potential issue during TxnAbort() failure (1st commit);
- Updated InMemoryWalletDatabase()::Filename() to return <in-memory>;
- Improved migration code to also delete journal files (3rd commit).
Updated PR's description to reflect the above.

pablomartin4btc referenced this in commit ef2eeb8de1 on Jul 5, 2026

pablomartin4btc commented at 3:43 PM on July 5, 2026: member

-<ins>Updates</ins>:

After reflection, I folded the Filename() → DisplayFileName() rename (originally planned as a separate PR) into this one as a 4th commit. It lands more cleanly here immediately after the Files() fix — at that point all remaining callers are display-only, so the rename needs no caveats.
Updated PR's description accordingly.

in src/wallet/sqlite.cpp:142 in 7b6136e660

Sjors commented at 8:08 AM on July 6, 2026:

7b6136e660740b59db818b48576e311c311dfd0c nit: maybe use m_additional_flags here to be consistent with Open(), and to be safe if we ever modify flags here in the constructor.

pablomartin4btc commented at 2:42 AM on July 7, 2026:

Done. Thanks!

in src/wallet/wallet.cpp:4421 in 0c276add2b

4416 | @@ -4417,7 +4417,9 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
4417 |              // This applies to the watch-only and solvable wallets.
4418 |              // Wallets stored directly as files in the top-level directory
4419 |              // (e.g. default unnamed wallets) don’t have a removable parent directory.
4420 | -            wallet_empty_dirs_to_remove.insert(fs::PathFromString(wallet.GetDatabase().Filename()).parent_path());
4421 | +            if (!files.empty()) {
4422 | +                wallet_empty_dirs_to_remove.insert(files.front().parent_path());

Sjors commented at 8:24 AM on July 6, 2026:

In 0c276add2b76bdb690905ad899d5fb2fabc50443 wallet: use Files() instead of Filename() for file removal in migration: could add a comment here that all files are in the same path, so getting the parent path from any file will do.

achow101 commented at 7:35 PM on July 6, 2026:

In 0c276add2b76bdb690905ad899d5fb2fabc50443 "wallet: use Files() instead of Filename() for file removal in migration"

I think this is unsafe. The wallet database may (be changed to) store additional files in sub directories. Files() does not guarantee the order in which the files are returned an callers should not assume any order. This should continue to use Filename() as that is guaranteed to be the primary data file stored at the root of the wallet directory.

pablomartin4btc commented at 2:43 AM on July 7, 2026:

Dropping commit 3 entirely per @achow101's feedback on that hunk — files.front().parent_path() was unsafe and the BerkeleyRO change was unnecessary. Reverted to original.

pablomartin4btc commented at 3:31 AM on July 7, 2026:

Agreed, dropping commit 3. The DisplayFileName rename (commit 4) is also dropped.

Sjors commented at 11:32 AM on July 6, 2026: member

Code review b67d6b592d0c89189a938e293eb859a739756da8

I think MockableSQLiteDatabase should inherit from InMemoryWalletDatabase. That should also let you introduce the new subclass in its own commit, before _wallet: use in-memory SQLite for temporary wallet in exportwatchonlywallet_.

In 7b6136e660740b59db818b48576e311c311dfd0c wallet: store m_additional_flags in SQLiteDatabase to fix reopen path: there's a second problem that we should fix: it makes no sense to Close() and Open() an in-memory database, that's just going to wipe everything. It could throw() instead.

In b67d6b592d0c89189a938e293eb859a739756da8 wallet, refactor: rename Filename() to DisplayFileName() in WalletDatabase: I think we should go one step further and set m_display_file_name at construction time. That's safer because the SQLiteDatabase constructor logs things, so we shouldn't rely on an override DisplayFileName(). It also ties into the next suggestions.

Note that the Files() helper, introduced as part of #31423, is just guessing which files exist. You could make that a bit more robust by using sqlite3_filename_journal in modern sqlite3 versions.

We also rely on InMemoryWalletDatabase to override Files() with an empty list. Instead we could make m_file_path and m_journal_file_path optional, and not set them for an in-memory wallet. I have a commit that does this, along with some constructor ergonomics.

Here's a branch that does all the above (except for splitting b67d6b592d0c89189a938e293eb859a739756da8).

in src/wallet/sqlite.cpp:250 in b67d6b592d outdated

 246 | @@ -247,7 +247,7 @@ bool SQLiteDatabase::Verify(bilingual_str& error)
 247 |  
 248 |  void SQLiteDatabase::Open()
 249 |  {
 250 | -    Open(/*additional_flags*/0);
 251 | +    Open(m_additional_flags);

janb84 commented at 11:36 AM on July 6, 2026:

Not sure if the commit message is clear enough on the point that code change is to prevent a crash not to preserves data across refresh. A Close() + Open() will create a fresh in-memory DB, any changes will be losts.

Sjors commented at 11:40 AM on July 6, 2026:

We posted around the same time, see #35655#pullrequestreview-4633967855

it makes no sense to Close() and Open() an in-memory database, that's just going to wipe everything. It could throw() instead.

So we should probably not prevent a crash.

pablomartin4btc commented at 3:24 AM on July 7, 2026:

Done! — Open() now throws std::runtime_error for in-memory databases (both the force_conn_refresh path in SQLiteBatch::Close() and the public Open() override). A regression test is included in a separate commit, co-authored by @janb84.

in src/wallet/wallet.cpp:3911 in b67d6b592d

3907 | @@ -3908,7 +3908,7 @@ bool CWallet::MigrateToSQLite(bilingual_str& error)
3908 |          if (!batch->Write(std::span{key}, std::span{value})) {
3909 |              batch->TxnAbort();
3910 |              m_database->Close();
3911 | -            fs::remove(m_database->Filename());
3912 | +            for (const fs::path& file : m_database->Files()) fs::remove(file);

janb84 commented at 11:39 AM on July 6, 2026:

On line 3885 the pattern is to do a snapshot of the files, close database, delete files. On this line the pattern is different close database, snapshot files and delete files. The latter only works because of the const files.

NIT: pick one style.

pablomartin4btc commented at 3:28 AM on July 7, 2026:

Dropping commit 3 entirely per @achow101's feedback, so this is not longer applicable. Good catch regardless.

janb84 commented at 11:42 AM on July 6, 2026: contributor

Concept ACK b67d6b592d0c89189a938e293eb859a739756da8

PR Uses the functionality of SQLite to create a in-memory database to use as the temporary storage func. of the WatchOnlyWallet. This has the advantage that it's really temporary and removes a lot of removal logic from the code. Which I think is an improvement.

NIT: The changed open function with stored additional flags can use a regression test (imho)

eg:

something like this, as additional test in db_tests.cpp:

BOOST_AUTO_TEST_CASE(in_memory_connection_refresh)
{
    // Regression test for the reopen path of an in-memory database.
    InMemoryWalletDatabase database;

    std::string key = "key";
    std::string value = "value";

    // Reopening an in-memory database must not attempt any on-disk access.
    database.Close();
    BOOST_CHECK_NO_THROW(database.Open());

    // The refreshed in-memory connection must remain usable for reads and writes.
    SQLiteBatch batch{database};
    BOOST_CHECK(batch.Write(key, value));
    std::string read_value;
    BOOST_CHECK(batch.Read(key, read_value));
    BOOST_CHECK_EQUAL(read_value, value);
}

</details>

EDIT: i Agree with Sjors that throwing would be a better solution that recovering to a empty state.

in src/wallet/export.cpp:12 in b67d6b592d outdated

   8 | @@ -9,6 +9,7 @@
   9 |  #include <util/expected.h>
  10 |  #include <wallet/scriptpubkeyman.h>
  11 |  #include <wallet/context.h>
  12 | +#include <wallet/sqlite.h>

b-l-u-e commented at 4:10 PM on July 6, 2026:

nit: maybe consider declaring MakeInMemoryWalletDatabase() in db.h , so as export.cpp doesn’t need #include <wallet/sqlite.h>.

git diff src/wallet/db.h
diff --git a/src/wallet/db.h b/src/wallet/db.h
index eedfc5a0c4..adb4a0cdaa 100644
--- a/src/wallet/db.h
+++ b/src/wallet/db.h
@@ -204,6 +204,7 @@ std::vector<std::pair<fs::path, std::string>> ListDatabases(const fs::path& path
 
 void ReadDatabaseArgs(const ArgsManager& args, DatabaseOptions& options);
 std::unique_ptr<WalletDatabase> MakeDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error);
+std::unique_ptr<WalletDatabase> MakeInMemoryWalletDatabase();
 
 fs::path BDBDataFile(const fs::path& path);
 fs::path SQLiteDataFile(const fs::path& path);

git diff src/wallet/sqlite.h
diff --git a/src/wallet/sqlite.h b/src/wallet/sqlite.h
index 5b293587f5..fcea3a51f1 100644
--- a/src/wallet/sqlite.h
+++ b/src/wallet/sqlite.h
@@ -185,8 +185,6 @@ public:

 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

 git diff src/wallet/export.cpp
diff --git a/src/wallet/export.cpp b/src/wallet/export.cpp
index e9864f0374..34b31fdc45 100644
--- a/src/wallet/export.cpp
+++ b/src/wallet/export.cpp
@@ -9,7 +9,6 @@
 #include <util/expected.h>
 #include <wallet/scriptpubkeyman.h>
 #include <wallet/context.h>
-#include <wallet/sqlite.h>
 #include <wallet/wallet.h>
 
 #include <fstream>

achow101 commented at 7:45 PM on July 6, 2026:

No, don't do that. The implementation of a function should be in the .cpp file that corresponds to the .h file that the declaration is in. This breaks that pattern. If the pattern were preserved, it would then introduce a circular dependency. sqlite.{h/cpp} is the correct place.

pablomartin4btc commented at 3:29 AM on July 7, 2026:

Thanks. Keeping it in sqlite.h as-is.

in src/wallet/wallet.cpp:3887 in 0c276add2b

3885 | -    fs::path db_path = fs::PathFromString(m_database->Filename());
3886 | +    // Close this database and delete its file(s)
3887 | +    const std::vector<fs::path> old_files = m_database->Files();
3888 |      m_database->Close();
3889 | -    fs::remove(db_path);
3890 | +    for (const fs::path& file : old_files) fs::remove(file);

achow101 commented at 7:30 PM on July 6, 2026:

In 0c276add2b76bdb690905ad899d5fb2fabc50443 "wallet: use Files() instead of Filename() for file removal in migration"

This change is unnecessary, BerkeleyRODatabase returns exactly the same thing in Files() as is returned by Filename(). There is, and can only be, a single file path returned by either of these functions within BerkeleyRODatabase.

pablomartin4btc commented at 3:30 AM on July 7, 2026:

Agreed, dropping commit 3.

achow101 commented at 7:36 PM on July 6, 2026: member

Filename() is documented as returning a path for display/logging only, but the file-op callers above made its misuse invisible. After switching those to Files(), Filename() is renamed to DisplayFileName() to enforce the display-only intent at every remaining call site.

I think this is a mischaracterization of Filename(). When the documentation was originally written, it's only use was for logging, where we want to log the full path to the data file. Just because the documentation still says that does not mean that it is exclusively to be used for logging, nor that using it outside of logging is misuse. Filename() must point to the primary data file and callers can assume that it will be a full path to that file. I don't think this should be changed at all.

b67d6b592d0c89189a938e293eb859a739756da8 "wallet, refactor: rename Filename() to DisplayFileName() in WalletDatabase" Should be dropped. I'm inclined to say that 0c276add2b76bdb690905ad899d5fb2fabc50443 "wallet: use Files() instead of Filename() for file removal in migration" should be dropped as well. Both of these are also mostly unrelated to the main change in this PR.

achow101 commented at 7:47 PM on July 6, 2026: member

We also rely on InMemoryWalletDatabase to override Files() with an empty list. Instead we could make m_file_path and m_journal_file_path optional, and not set them for an in-memory wallet. I have a commit that does this, along with some constructor ergonomics.

No, every caller needs to intentionally choose what to put for the paths and names as these are relevant to sqlite's usage. We should not be defaulting names or paths as they may result in the wrong database files being opened.

pablomartin4btc force-pushed on Jul 7, 2026

pablomartin4btc commented at 2:42 AM on July 7, 2026: member

Code review b67d6b5

Done the first two — MockableSQLiteDatabase now derives from InMemoryWalletDatabase (removing its redundant Files() override), and the force_conn_refresh path now throws for in-memory connections.

On m_display_file_name and optional m_file_path: dropping commits 3 and 4 (per @achow101 's feedback) makes these unnecessary for this PR. Keeping Filename() as-is with the in-memory override, and keeping the explicit Files() override in InMemoryWalletDatabase.

pablomartin4btc force-pushed on Jul 7, 2026

pablomartin4btc commented at 3:23 AM on July 7, 2026: member

Concept ACK b67d6b5

Thanks for the ACK and the regression test suggestion! Added it in a separate commit, co-authored by you. Since the behaviour changed from silently reopening to throwing, the assertion is inverted: BOOST_CHECK_THROW(database.Open(), std::runtime_error) instead of BOOST_CHECK_NO_THROW.

DrahtBot added the label CI failed on Jul 7, 2026

pablomartin4btc commented at 3:33 AM on July 7, 2026: member

We also rely on InMemoryWalletDatabase to override Files() with an empty list. Instead we could make m_file_path and m_journal_file_path optional, and not set them for an in-memory wallet. I have a commit that does this, along with some constructor ergonomics.

No, every caller needs to intentionally choose what to put for the paths and names as these are relevant to sqlite's usage. We should not be defaulting names or paths as they may result in the wrong database files being opened.

Agreed, keeping the explicit Files() override in InMemoryWalletDatabase.

pablomartin4btc commented at 3:34 AM on July 7, 2026: member

b67d6b5 "wallet, refactor: rename Filename() to DisplayFileName() in WalletDatabase" Should be dropped. I'm inclined to say that 0c276ad "wallet: use Files() instead of Filename() for file removal in migration" should be dropped as well.

Both dropped, thanks.

pablomartin4btc commented at 3:45 AM on July 7, 2026: member

-<ins>Updates</ins>:

Addressed @achow101's feedback: dropped old commits 3 and 4 (Files() migration fix and DisplayFileName() rename) — both unrelated to the main change.
Addressed @Sjors's feedback: m_additional_flags stored as member in commit 1; force_conn_refresh path and public Open() now throw for in-memory databases; added friend class SQLiteBatch; MockableSQLiteDatabase now derives from InMemoryWalletDatabase in commit 2, removing its redundant Files() override.
Addressed @janb84's feedback: regression test added as a separate commit 3.
Updated PR description accordingly.

DrahtBot removed the label CI failed on Jul 7, 2026

Sjors commented at 7:00 AM on July 7, 2026: member

@achow101 wrote:

Filename() must point to the primary data file and callers can assume that it will be a full path to that file. I don't think this should be changed at all.

Then let's actually document the function to say so. But it's inconsistent with then setting it to "<in-memory>" in a subclass. So perhaps it still makes sense to make it an optional, something along the lines of what I did in https://github.com/Sjors/bitcoin/commit/69b5f28e369219b0363d8327e2fa58664375dedd, but leaving Files() alone.

We also rely on InMemoryWalletDatabase to override Files() with an empty list. Instead we could make m_file_path and m_journal_file_path optional, and not set them for an in-memory wallet. I have a commit that does this, along with some constructor ergonomics.

No, every caller needs to intentionally choose what to put for the paths and names as these are relevant to sqlite's usage. We should not be defaulting names or paths as they may result in the wrong database files being opened.

What do you mean by this? m_file_path is set by the constructor, https://github.com/Sjors/bitcoin/commit/69b5f28e369219b0363d8327e2fa58664375dedd doesn't change that. It just avoids setting a fake filename for an in-memory wallet.

achow101 commented at 5:27 PM on July 7, 2026: member

No, every caller needs to intentionally choose what to put for the paths and names as these are relevant to sqlite's usage. We should not be defaulting names or paths as they may result in the wrong database files being opened.

What do you mean by this? m_file_path is set by the constructor, Sjors@69b5f28 doesn't change that. It just avoids setting a fake filename for an in-memory wallet.

You're literally allowing file path to be nullopt, and then choosing an default when it is, and passing that default in to sqlite_open_v2. That is unsafe, as it implies that callers do not need to pass a file path, when in fact they must in order to use sqlite_open_v2 correctly. The path given has a meaning to sqlite, callers must know that it does and we should not be choosing a default value because that is how we end up accidentally using the wrong database.

If a subclass wants to use a bogus file path, it must do so explicitly by choice. SQLiteDatabase must not be choosing a default for it because that hides what the file path really is.

in src/wallet/export.cpp:81 in 599a46f2dc

 104 | -    });
 105 | -
 106 |      WalletContext empty_context;
 107 |      empty_context.args = context.args;
 108 | -    watchonly_wallet = CWallet::CreateNew(empty_context, wallet_name, std::move(database), options.create_flags, /*born_encrypted=*/false, error, warnings);
 109 | +    std::shared_ptr<CWallet> watchonly_wallet = CWallet::CreateNew(empty_context, /*name=*/"exportwatchonly_temp", MakeInMemoryWalletDatabase(), create_flags, /*born_encrypted=*/false, error, warnings);

achow101 commented at 6:13 PM on July 7, 2026:

In 599a46f2dc6cb9b411cecbcacc497b2408370352 "wallet: use in-memory SQLite for temporary wallet in exportwatchonlywallet"

I would prefer if the name was still based on the wallet's name. While I don't think it's an issue because we use a new WalletContext, having unique wallet names avoids potential issues with naming conflicts, e.g. from doing exportwatchonly on 2 different wallets simultaneously.

pablomartin4btc commented at 9:22 PM on July 7, 2026:

Ok, I do agree.

in src/wallet/sqlite.cpp:726 in 599a46f2dc

 721 | @@ -722,6 +722,15 @@ std::unique_ptr<SQLiteDatabase> MakeSQLiteDatabase(const fs::path& path, const D
 722 |      }
 723 |  }
 724 |  
 725 | +InMemoryWalletDatabase::InMemoryWalletDatabase()
 726 | +    : SQLiteDatabase(fs::path{}, fs::path{}, DatabaseOptions(), SQLITE_OPEN_MEMORY)

achow101 commented at 6:15 PM on July 7, 2026:

In 599a46f2dc6cb9b411cecbcacc497b2408370352 "wallet: use in-memory SQLite for temporary wallet in exportwatchonlywallet"

I think file_path should use the special name :memory: as that is also a signal to SQLite to use an in memory database. This would also remove the Filename() override. Note that an empty string has a different special meaning to create a temporary database file on disk.

See https://sqlite.org/inmemorydb.html

pablomartin4btc commented at 9:08 PM on July 7, 2026:

I'll fix that. Thanks for the sqlite ref link!

in src/wallet/sqlite.cpp:456 in 135e68aa54 outdated

 450 | @@ -448,6 +451,9 @@ void SQLiteBatch::Close()
 451 |      }
 452 |  
 453 |      if (force_conn_refresh) {
 454 | +        if (m_database.m_additional_flags & SQLITE_OPEN_MEMORY) {
 455 | +            throw std::runtime_error("SQLiteDatabase: Cannot recover in-memory database connection");
 456 | +        }

b-l-u-e commented at 8:54 PM on July 7, 2026:

I think here introduces a destructor failure-path issue.

since SQLiteBatch::~SQLiteBatch() already calls Close()

\$ sed -n '418,468p' src/wallet/sqlite.cpp 
void SQLiteBatch::Close()
{
    bool force_conn_refresh = false;

    // If we began a transaction, and it wasn't committed, abort the transaction in progress
    if (m_txn) {
        if (TxnAbort()) {
            LogWarning("SQLiteBatch: Batch closed unexpectedly without the transaction being explicitly committed or aborted");
        } else {
            // If transaction cannot be aborted, it means there is a bug or there has been data corruption. Try to recover in this case
            // by closing and reopening the database. Closing the database should also ensure that any changes made since the transaction
            // was opened will be rolled back and future transactions can succeed without committing old data.
            force_conn_refresh = true;
            LogWarning("SQLiteBatch: Batch closed and failed to abort transaction, resetting db connection..");
        }
    }

    // Free all of the prepared statements
    const std::vector<std::pair<sqlite3_stmt**, const char*>> statements{
        {&m_read_stmt, "read"},
        {&m_insert_stmt, "insert"},
        {&m_overwrite_stmt, "overwrite"},
        {&m_delete_stmt, "delete"},
        {&m_delete_prefix_stmt, "delete prefix"},
    };

    for (const auto& [stmt_prepared, stmt_description] : statements) {
        int res = sqlite3_finalize(*stmt_prepared);
        if (res != SQLITE_OK) {
            LogWarning("SQLiteBatch: Batch closed but could not finalize %s statement: %s",
                      stmt_description, sqlite3_errstr(res));
        }
        *stmt_prepared = nullptr;
    }

    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();
            // If TxnAbort failed and we refreshed the connection, the semaphorewas not released, so release it here to avoid deadlocks on future writes.
            m_database.m_write_semaphore.release();
        } catch (const std::runtime_error&) {
            // If open fails, cleanup this object and rethrow the exception
            m_database.Close();
            throw;
        }
    }
}

</details>

I reproduced locally with a temporary test in `src/wallet/test/db_tests.cpp`:

BOOST_AUTO_TEST_CASE(in_memory_failed_rollback_terminates)
{
    InMemoryWalletDatabase database;
    {
        SQLiteBatch batch(database);
        BOOST_REQUIRE(batch.TxnBegin());
        BOOST_REQUIRE(batch.Write(std::string{"key"}, std::string{"value"}));
        batch.SetExecHandler(std::make_unique<DbExecBlocker>(
            std::set<std::string>{"ROLLBACK TRANSACTION"}));
    }
}

Run command:

./build/bin/test_bitcoin --run_test=db_tests/in_memory_failed_rollback_terminates --catch_system_error=no --log_level=all -- -printtoconsole=1

output

SQLiteBatch: Failed to abort the transaction
SQLiteBatch: Batch closed and failed to abort transaction, resetting db connection..
terminate called after throwing an instance of 'std::runtime_error'
  what():  SQLiteDatabase: Cannot recover in-memory database connection

achow101 commented at 9:27 PM on July 7, 2026:

Terminating here is ok as this is part of a class of errors where writes failed. If the database is having issues at this point, corruption is very likely to be present and we should prefer to crash to avoid making anything worse.

b-l-u-e commented at 9:31 PM on July 7, 2026:

ohh i see will it not be better to make termination explicit

pablomartin4btc commented at 11:31 PM on July 7, 2026:

Agreed on making it explicit. There's actually a LogWarning already at this point ("Batch closed and failed to abort transaction, resetting db connection.") but it's misleading for the in-memory case since we're not resetting anything — we're about to terminate. And the throw message itself ("SQLiteDatabase: Cannot recover in-memory database connection") is swallowed by std::terminate() and never reaches the log or the user.

That said, I'd raise a broader concern: @achow101 noted here that termination is intentional when writes fail. That reasoning makes sense for on-disk databases where corruption is a real risk — but for an in-memory database there's no persistent data to corrupt, so terminating the whole node feels disproportionate. The challenge is that since we're in a destructor, we can't propagate the error back to the RPC caller cleanly. A LogError + explicit std::terminate() would at least make the intent clear and fix the misleading warning — but LogError + return might actually be the right call here. @achow101 — does the same "prefer crash" reasoning apply to the in-memory case?

Sjors commented at 10:14 AM on July 8, 2026:

I tend to agree that we should just consider this a failed export and return, if possible.

pablomartin4btc force-pushed on Jul 8, 2026

DrahtBot added the label CI failed on Jul 8, 2026

pablomartin4btc commented at 12:23 AM on July 8, 2026: member

-<ins>Updates</ins>:

Wallet temp name is now "<source_wallet>_watchonly_temp" (derived from the source wallet) rather than the fixed "exportwatchonly_temp", to avoid naming conflicts when exporting multiple wallets concurrently and to keep log lines traceable to the source — per @achow101.
InMemoryWalletDatabase now passes ":memory:" as file_path to sqlite3_open_v2 (the canonical SQLite name for in-memory databases) rather than an empty string, which has a different meaning in SQLite (temporary file on disk). The Filename() override is removed — the base class now returns ":memory:" directly — per @achow101.

DrahtBot removed the label CI failed on Jul 8, 2026

Sjors commented at 8:58 AM on July 8, 2026: member

utACK 3165ed8630ee685b39dddb13bf4e950309d147cf @achow101 thanks for the clarification.

What do you mean by this? m_file_path is set by the constructor, Sjors@69b5f28 doesn't change that. It just avoids setting a fake filename for an in-memory wallet.

You're literally allowing file path to be nullopt, and then choosing an default when it is, and passing that default in to sqlite_open_v2

The default should have been the magic string :memory: as you pointed out here: #35655 (review)

The path given has a meaning to sqlite, callers must know that it does and we should not be choosing a default value because that is how we end up accidentally using the wrong database.

The commit enforces Assert(m_file_path.has_value() != bool(m_additional_flags & SQLITE_OPEN_MEMORY));. So when the database has a real path the caller must provide it.

The fact that sqlite_open_v2 uses a magic file name doesn't need to bleed through to higher abstractions, especially since we need to decide whether to delete files.

Latest version of what I had in mind: https://github.com/Sjors/bitcoin/commit/7c6026ce7b686840313fc087b00626a07b0f3b6d

That said, I think the Files() override { return {} } hack is fine for now.

DrahtBot requested review from janb84 on Jul 8, 2026

pablomartin4btc commented at 3:20 PM on July 8, 2026: member

Latest version of what I had in mind: Sjors@7c6026c

Thanks for working on that idea, I find it interesting and I like the approach. I noticed some details (e.g. magic string I think should live and belongs to InMemoryWalletDatabase class)) I'd prefer to review more carefully in a follow-up rather than bundling here. Happy to revisit in a follow-up if you want to explore it further, but I'd prefer to keep SQLiteDatabase core unchanged for now.

wallet: store m_additional_flags in SQLiteDatabase to fix reopen path

SQLiteDatabase::Open() (the public override) always reopens the database
with no additional flags. If SQLiteBatch::Close() triggers the
force_conn_refresh path (TxnAbort failed), it calls Open() which drops
the original additional_flags, causing in-memory databases to be reopened
as on-disk instead.

Store additional_flags as a member and use it in Open() so the reconnect
preserves the original flags. For in-memory databases, connection recovery
makes no sense as all data would be lost; both the force_conn_refresh path
and the public Open() now throw instead.

Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>

ee43743f12

in src/wallet/sqlite.cpp:431 in 3165ed8630 outdated

 427 | @@ -425,7 +428,7 @@ void SQLiteBatch::Close()
 428 |              // by closing and reopening the database. Closing the database should also ensure that any changes made since the transaction
 429 |              // was opened will be rolled back and future transactions can succeed without committing old data.
 430 |              force_conn_refresh = true;
 431 | -            LogWarning("SQLiteBatch: Batch closed and failed to abort transaction, resetting db connection..");
 432 | +            LogWarning("SQLiteBatch:  transaction, resetting db connection..");

b-l-u-e commented at 9:40 PM on July 8, 2026:

Was this change intentional? Seems incomplete

pablomartin4btc commented at 9:58 PM on July 8, 2026:

That's wrong, I'll fix it. Thanks for letting me know!

pablomartin4btc force-pushed on Jul 8, 2026

wallet: use in-memory SQLite for temporary wallet in exportwatchonlywallet

The intermediate watchonly wallet created during exportwatchonlywallet is
a pure build artifact — it is always discarded once BackupWallet() copies
it to the destination. Creating it as an in-memory SQLiteDatabase
(SQLITE_OPEN_MEMORY) removes the need to write files to the wallets
directory and eliminates the cleanup handler that deleted those files on
both success and failure paths.

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

The wallet is named after the source wallet ("<name>_watchonly_temp") so
concurrent exports of different wallets use distinct names and log lines
remain traceable to the source wallet.

d1e7f8c986

test: add regression test for in-memory SQLiteDatabase reopen

InMemoryWalletDatabase::Open() now throws to prevent silently returning
a fresh empty connection after close, which would discard all data. Add a
test to pin this behaviour.

Co-authored-by: Jan B <608446+janb84@users.noreply.github.com>

777d23f25c

pablomartin4btc force-pushed on Jul 8, 2026

DrahtBot added the label CI failed on Jul 8, 2026

DrahtBot removed the label CI failed on Jul 8, 2026

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

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)

I reproduced locally with a temporary test in src/wallet/test/db_tests.cpp:

I reproduced locally with a temporary test in `src/wallet/test/db_tests.cpp`: