wallet: Make SQLiteDatabase::m_file_path an std::optional<fs::path> #35707

Follow-up to #35655. Implements the approach proposed by Sjors during review.

<details> <summary>Make the <code>SQLiteDatabase</code> internal file path optional so that subclasses — such as <code>InMemoryWalletDatabase</code> — with no on-disk persistence don't need to carry an unnecessary placeholder file path.</summary>

• Replace member of SQLiteDatabase const std::string m_file_path with const std::optional<fs::path> m_file_path, so subclasses with no on-disk persistence — such as InMemoryWalletDatabase — pass std::nullopt instead of storing the SQLite-internal magic string ":memory:" as a placeholder file path.

• An Assert in the protected constructor enforces the invariant that m_file_path is absent if and only if SQLITE_OPEN_MEMORY is set, catching any future misuse at startup.

• Add a private IsOnDisk() predicate to replace bare if (m_file_path) checks in Open(), Filename(), and Files(). Files() now returns an empty vector for in-memory databases through the base class implementation, so the redundant override in InMemoryWalletDatabase is removed. The Filename() override in MockableSQLiteDatabase is also removed, as the base class now correctly returns ":memory:" for in-memory databases.

</details>

<details> <summary>Add a test for <code>InMemoryWalletDatabase::Filename()</code> and <code>Files()</code>.</summary>

• Add a test case verifying that InMemoryWalletDatabase::Filename() returns ":memory:" and Files() returns an empty vector.

</details>

<ins>Note:</ins> InMemoryWalletDatabase intentionally inherits from SQLiteDatabase rather than WalletDatabase directly — it uses real SQLite semantics in memory, so all wallet code exercising it gets faithful SQLite behaviour without having to reimplement the full database layer. The optional m_file_path formalises the existing design gap rather than introducing one.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

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

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Concept ACK

-<ins>Updates</ins>:

• Addressed @Sjors's feedback, suggestion taken on simpler path construction in Files().
• Commit missed @Sjors co-authoring, updated it.
• Updated PR description collapsing technical details behind expandable sections and adding a note on addressing a pre-existing design gap in SQLiteDatabase.

Concept NACK

I don't think we should allow SQLiteDatabase's main constructor to optionally be nameless, even if we are going to assert the name later. If this is going to be done, then it should be a separate constructor. But I don't think we should even be doing that; the separate class for the in memory database is the correct approach IMO.

Specifically, database names have a meaning in sqlite, and there are other special names that could be passed that control different things about the database. There are other ways to give a name that specify an in memory database. Additionally, if there needed to be multiple connections to a single in memory database, that would also have to be specified through the name, which this PR prevents.

I'm failing to see what improvement this does for us at all. It adds more complication to the base SQLiteDatabase class which now has 2 different ways to determine in-memoryness. We have the separate class for in memory databases, that is sufficient.

Closing in light of @achow101's Concept NACK and the wider picture this discussion has surfaced. @achow101 is right that making SQLiteDatabase's constructor optionally nameless adds complexity to the base class, and that the separate class approach for in-memory databases is already the correct structural choice. However, working through the objections revealed that the design gap runs deeper than this PR addresses.

Two related findings:

1. Missing intermediate class.

   There is no class between WalletDatabase and SQLiteDatabase that holds the SQLite implementation without file path concerns. As a result, InMemoryWalletDatabase is forced to inherit from SQLiteDatabase and carry a placeholder path — which is what this PR was trying to clean up. The proper fix is to introduce a base class that holds the SQLite implementation (connection, batches, cursors, transactions), with SQLiteDatabase extending it for on-disk use and InMemoryWalletDatabase extending it directly with no path at all.

   <details> <summary>Naming considerations for the proposed hierarchy</summary> <br>

   SQLiteDatabase should keep its name — it's the primary public-facing class and renaming it would break callers. The new base needs a name that conveys "SQLite implementation without file path concerns." SQLiteCore is concise; SQLiteDatabaseBase is more self-documenting in a codebase where you might encounter it cold. The safest structure requiring no existing renames:

     SQLiteDatabaseBase          ← new: SQLite impl, no path
     ├── SQLiteDatabase          ← keeps name: on-disk
     └── InMemoryWalletDatabase  ← keeps name: in-memory, inherits base directly
         └── MockableSQLiteDatabase  ← keeps name
   

   <br>

   InMemoryWalletDatabase's DisplayName() would return something meaningful (e.g. the wallet name) since the base class would have no file path logic, while SQLiteDatabase::DisplayName() would return the actual filename.

   </details>

2. Filename() overloading.

   Filename() is currently used for two incompatible purposes: display/logging and file operations. For in-memory databases ":memory:" appears in log lines, which is not useful — and with multiple simultaneous in-memory wallets (e.g. two concurrent exportwatchonlywallet calls) they become indistinguishable in logs. @Sjors flagged this in his review, suggesting the two concerns be separated. Filename() (or a renamed DisplayName()) should serve display purposes only, with file operations relying solely on Files().

   <details> <summary>Current <code>Filename()</code> call sites by purpose</summary> <br>

   File operations (must be a real path):

   • wallet.cpp:3885 — fs::PathFromString(m_database->Filename()) then fs::remove()
   • wallet.cpp:3911 — fs::remove(m_database->Filename())
   • wallet.cpp:4420 — .parent_path() for directory cleanup <br>

   Display/logging (wallet name would be more useful for in-memory):

   • sqlite.cpp:55 — LogTrace("[%s] SQLite Statement", Filename())
   • sqlite.cpp:283 — LogWarning("Failed to enable SQL tracing for %s", Filename())
   • wallet.cpp:2386, 3105, 3160 — identification in messages

   </details>

Happy to open a tracking issue to coordinate the refactoring across a few focused PRs — if there's interest from reviewers for me to take this on.

<details> <summary>Proposed PR breakdown</summary>

</details>