wallet: Follow-ups to create/load split (#32636)

davidgumberg commented at 6:02 am on February 3, 2026: contributor

This adds documentation for PopulateWalletFromDB() and improves documentation for LoadExisting() and CreateNew(), and makes WalletBatch::WriteVersion(int client_version) respect the argument that it’s passed, addressing issues reviewers pointed in #32636.

DrahtBot added the label Wallet on Feb 3, 2026

davidgumberg marked this as a draft on Feb 3, 2026

DrahtBot commented at 6:03 am on February 3, 2026: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	rkrux

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)
#32895 (wallet: Prepare for future upgrades by recording versions of last client to open and decrypt 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 typos and grammar issues:

std::nullptr -> nullptr [std::nullptr is not valid; the correct null pointer literal is nullptr. This appears in two added docs (“CreateNew” and “LoadExisting”).]

2026-02-04 21:40:44

DrahtBot added the label CI failed on Feb 3, 2026

davidgumberg force-pushed on Feb 3, 2026

DrahtBot removed the label CI failed on Feb 3, 2026

davidgumberg force-pushed on Feb 4, 2026

doc: wallet: Improve wallet creation function docs

This addresses review comments from #32636:

- https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2755916371
- https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2756024581,
- https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2756055872

880a800072

doc: wallet: clarify "create" meaning initialize

https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2756245482

3c5a228fcd

wallet: WalletBatch->WriteVersion respect argument.

Previously would use global `CLIENT_VERSION` no matter what, but this is
one sense a refactor since all of the places where WriteVersion is
called currently call it with `CLIENT_VERSION` anyways. The
`client_version` argument is kept since future test code may want to
write other versions.

Addresses a review comment from #32636:

https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2356299627

fedcd466f0

davidgumberg force-pushed on Feb 4, 2026

davidgumberg marked this as ready for review on Feb 4, 2026

davidgumberg commented at 9:42 pm on February 4, 2026: contributor

Now that #32636 is merged this can be reviewed.

(soft ping @murchandamus, @enirox001, and @rkrux whose review comments are addressed in this PR)

in src/wallet/wallet.h:803 in fedcd466f0

796@@ -797,6 +797,19 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
797     bool IsFromMe(const CTransaction& tx) const;
798     CAmount GetDebit(const CTransaction& tx) const;
799 
800+    /**
801+     * Pass-through to WalletBatch::LoadWallet() to initialize this CWallet from
802+     * the records stored in CWallet::m_database. Primarily used by
803+     * CWallet::LoadExisting.

murchandamus commented at 10:27 pm on February 4, 2026:

I feel that this doesn’t answer why the method exists. I think this decorates LoadExisting with error handling, maybe that could be mentioned.

in src/wallet/wallet.cpp:4307 in fedcd466f0

4303@@ -4304,7 +4304,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
4304         return util::Error{Untranslated("Wallet file verification failed.") + Untranslated(" ") + error};
4305     }
4306 
4307-    // Make the local wallet
4308+    // Initialize the CWallet for the local wallet.

murchandamus commented at 10:27 pm on February 4, 2026:

I think this comment is obsolete.

rkrux commented at 12:50 pm on February 5, 2026:

Is any comment required here now?

The older comment might have been needed when CWallet::Create was called wherein it was not obvious in the caller whether the wallet would be created or loaded.

Reading CWallet::LoadExisting below seems pretty self explanatory to me.

DrahtBot added the label CI failed on Feb 4, 2026

in src/wallet/walletdb.h:269 in fedcd466f0

264@@ -265,8 +265,14 @@ class WalletBatch
265 
266     DBErrors LoadWallet(CWallet* pwallet);
267 
268-    //! Write the given client_version.
269-    bool WriteVersion(int client_version) { return m_batch->Write(DBKeys::VERSION, CLIENT_VERSION); }
270+    /**
271+     * Write the given `client_version` to #m_batch, indicating the last version

rkrux commented at 1:07 pm on February 5, 2026:

#m_batch

Typo?

in src/wallet/wallet.h:890 in fedcd466f0

885@@ -873,10 +886,24 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
886 
887     static bool LoadWalletArgs(std::shared_ptr<CWallet> wallet, const WalletContext& context, bilingual_str& error, std::vector<bilingual_str>& warnings);
888 
889-    /* Initializes, creates and returns a new CWallet; returns a null pointer in case of an error */
890+    /**
891+     * Create a new wallet.

rkrux commented at 1:16 pm on February 5, 2026:

Nit: Can mention it writes stuff to the database.

rkrux commented at 1:17 pm on February 5, 2026: contributor

Concept ACK fedcd466f00d25cd9d5d160d536cf7ed2d78ccbe

DrahtBot removed the label CI failed on Feb 9, 2026

wallet: Follow-ups to create/load split (#32636) #34490

Reviews

Conflicts

LLM Linter (✨ experimental)