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.

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> 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.

^{2026-07-08 22:04:25}

Concept ACK

Code review 532c677.

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());
+            }
         }
     };

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

-Updates:

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

-Updates:

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

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.

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.

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.

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.

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.

-Updates:

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

@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 Sjors@69b5f28, 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, Sjors@69b5f28 doesn't change that. It just avoids setting a fake filename for an in-memory wallet.

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.

-Updates:

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

utACK 3165ed8

@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 (comment)

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: Sjors@7c6026c

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

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.

re-utACK 777d23f

Just a text change since my previous review.

I'd prefer to keep SQLiteDatabase core unchanged for now.

Yes, that's fine.

ACK 777d23f