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

pull davidgumberg wants to merge 3 commits into bitcoin:master from davidgumberg:2026-02-02-create-load-refactor-followup changing 3 files +38 −5

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, w0xlt

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:
- #34909 (wallet, refactor: modularise wallet by extracting out legacy wallet migration by rkrux)
- #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)
- #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
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").]
<sup>2026-02-04 21:40:44</sup>
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

Type	Reviewers
Concept ACK	rkrux, w0xlt

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.

davidgumberg commented at 2:16 AM on March 11, 2026:

Not sure exactly what you think would be helpful here, could you be more specific?
rkrux commented at 1:13 PM on March 11, 2026:

- * Create a new wallet. + * Create a new wallet and persist in the database.

So that the reader doesn't assume that the wallet is created only in memory.

I guess the function pre-split influence was still present at the time I mentioned it because for the longest time the reader was not sure if the code was creating (writing to the disk) or loading (reading from the disk) the wallet.

This suggestion can be ignored if the presence of CreateNew name makes it obvious for everyone else.
rkrux commented at 1:17 PM on February 5, 2026: contributor

Concept ACK fedcd466f00d25cd9d5d160d536cf7ed2d78ccbe
DrahtBot removed the label CI failed on Feb 9, 2026
w0xlt commented at 12:54 AM on February 19, 2026: contributor

Concept ACK

Contributors

Labels

Linked (view graph)

#32636 Split `CWallet::Create()` into `CreateNew` and `LoadExisting`

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

Reviews

Conflicts

LLM Linter (✨ experimental)