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

davidgumberg · 2025-05-29T00:37:09Z

This PR is mostly a refactor which splits out logic used for creating wallets and for loading wallets, both of which are presently contained in CWallet::Create() into CWallet::CreateNew() and CWallet::LoadExisting()

The real win of this PR is that CWallet::Create() uses a very bad heuristic for trying to guess whether or not it is supposed to be creating a new wallet or loading an existing wallet:

bitcoin/src/wallet/wallet.cpp

Lines 2882 to 2885 in 370c592

    
           // This wallet is in its first run if there are no ScriptPubKeyMans and it isn't blank or no privkeys 
        
           const bool fFirstRun = walletInstance->m_spk_managers.empty() && 
        
                            !walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && 
        
                            !walletInstance->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);

This heuristic assumes that wallets with no ScriptPubKeyMans are being created, which sounds reasonable, but as demonstrated in #32112 and #32111, this can happen when the user tries to load a wallet file that is corrupted, both issues are fixed by this PR and any other misbehavior for wallet files which succeeded the broken heuristic's sniff test for new wallets.

It was already the case that every caller of CWallet::Create() knows whether it is creating a wallet or loading one, so we can avoid replacing this bad heuristic with another one, and just shift the burden to the caller.

DrahtBot · 2025-05-29T00:37:13Z

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/32636.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	pablomartin4btc, rkrux
Approach ACK	w0xlt

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33082 (wallet, refactor: Remove Legacy check and error by pablomartin4btc)
#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)
#32685 (wallet: Allow read-only database access for info and dump commands by PeterWrighten)
#32138 (wallet, rpc: remove settxfee and paytxfee by polespinasa)
#29278 (Wallet: Add maxfeerate wallet startup option by ismaelsadeeq)

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.

w0xlt

Approach ACK

achow101 · 2025-05-29T21:56:47Z

PopulateWalletFromDB inside of CreateNew should no longer be necessary.

It'd be nice if the load or create intention could be propagated down to the DB level as well. In SQLiteDatabase::Open, we pass the flags SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE which does both load and create. I think ideally it would be just SQLITE_OPEN_READWRITE for loading, and SQLITE_OPEN_CREATE for creating to further enforce that we expect a file to already exist for loading, and for no files to exist when creating. Similarly, we could avoid calling TryCreateDirectories when loading.

luke-jr · 2025-06-06T21:05:51Z

Any reason not to just pass a parameter to CWallet::Create, just to fix the bug in the simplest manner? Refactoring should ideally be separate from fixing.

achow101 · 2025-06-06T21:11:27Z

Any reason not to just pass a parameter to CWallet::Create, just to fix the bug in the simplest manner? Refactoring should ideally be separate from fixing.

Because doing the bare minimum to fix a bug that is unreachable in production is how we end up with technical debt.

IMO, this PR is primarily a refactor to split these 2 actions that should not be combined. The fact that it fixes those bugs, which are a result of the combining, is a bonus.

pablomartin4btc

Concept ACK

The current state of this code in master adds confusion to flows like the legacy to descriptor wallet migration that's why I think it's an important work to be done.

rkrux

Concept ACK 0f82224
I want to share my initial thoughts before I invest more time reviewing this.

I am in favour of such a refactor because from a reviewing POV I lose mental cycles every time I end up going through CWallet:Create function that requires me to keep a mental context of whether this function was called from the creation flow or the load flow. Also, I feel this is a good time to get this done now that legacy wallets are no longer there.

However, I am slightly less inclined to review this PR in its current format because there are a few moving pieces here, and I'm afraid reviewing it thoroughly might be more time consuming and would induce less confidence in me finally a-c-king it.

Moreover, I feel the benefits of such a refactor would be more highlighted and appreciated if this can be broken down into smaller PRs in addition to ensuring data-correctness. Couple reasons I'm suggesting a breakdown are:

I would like to give more attention to each of the commits by trying to understand their consequences on the wallet as a whole.
I don't suppose all the commits are required to be present sequentially in a single PR based on my initial look, please feel free to correct me if I'm mistaken about their ability to be present in parallel.

PR breakdown suggestion

The first 3 can be done in parallel & the last one might be dependent on the previous ones:

The PopulateWalletFromDB related changes can be 1 PR - the first 2 commits.
The fBroadcastTransactions change is independent, can be clubbed with the birth-time-update-removal optimisation - 3rd, 6th commits.
The argument parsing changes also seem independent to me and can be another PR - 4th, 5th commits.
No strong opinion on the wallet stats logging commit, can go anywhere.
The last 5 commits can be 1 PR that introduces and uses the LoadExisting & CreateNew functions.

Also, I believe this is a good suggestion that might give dividends in the future and breaking down into smaller PRs might make it easy to incorporate it.

Having suggested all this, if you think the reward for breaking down the PR is not enough because the smaller refactoring PRs might not attract reviewers or might add rebase churn, I would still review this PR later but some kind of breakdown would aid review if not the one that I suggested.

Sorry if this got too long.

src/wallet/wallet.cpp

src/qt/test/addressbooktests.cpp

src/wallet/wallet.cpp

pablomartin4btc · 2025-08-01T21:54:32Z

Shouldn't src/wallet/walletdb.cpp be part of this PR? DBErrors WalletBatch::LoadWallet(CWallet* pwallet) is being called during wallet creation I think. If so, please also consider the note at #33082's description.

DrahtBot · 2025-08-23T05:46:29Z

🚧 At least one of the CI tasks failed.
_{Task lint: https://github.com/bitcoin/bitcoin/runs/48721895167}
_{LLM reason (✨ experimental): The CI failure is caused by a linting error detected by ruff due to an unused import in the test file, preventing successful completion of the lint check.}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

Also fixes wallet stats being incorrectly logged. (bitcoin#32636 (comment))

davidgumberg · 2025-08-23T06:42:28Z

@achow101

PopulateWalletFromDB inside of CreateNew should no longer be necessary.

@pablomartin4btc

DBErrors WalletBatch::LoadWallet(CWallet* pwallet) is being called during wallet creation I think.

Thanks for catching this, fixed in acc0d3b

It'd be nice if the load or create intention could be propagated down to the DB level as well. In SQLiteDatabase::Open, we pass the flags SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE which does both load and create. I think ideally it would be just SQLITE_OPEN_READWRITE for loading, and SQLITE_OPEN_CREATE for creating to further enforce that we expect a file to already exist for loading, and for no files to exist when creating. Similarly, we could avoid calling TryCreateDirectories when loading.

Agreed, I think it might be better to fix this in a follow-up as this PR is already quite large.

@rkrux

I feel the benefits of such a refactor would be more highlighted and appreciated if this can be broken down into smaller PRs

Thanks for the suggestion, despite the large number of commits in this PR, I've tried my best to make each one atomic and readable. I would prefer to not split into multiple PR's since most of the commits here are refactors that are setting-up for splitting wallet creation and loading, the first PR(s) in the series would have very little motivation, but if you insist I am open to splitting it up. Is there a particular commit or group of commits that are difficult to review?

rkrux · 2025-08-25T08:19:14Z

I would prefer to not split into multiple PR's since most of the commits here are refactors that are setting-up for splitting wallet creation and loading, the first PR(s) in the series would have very little motivation, but if you insist I am open to splitting it up.

No insistence at the moment, I will do a full review of the PR soon.

src/wallet/wallet.cpp

Previously creating an encrypted wallet would result in the keypool size incorrectly being reported as 0. See: bitcoin#32636 (comment)

DrahtBot · 2025-08-28T00:41:49Z

🚧 At least one of the CI tasks failed.
_{Task TSan, depends, no gui: https://github.com/bitcoin/bitcoin/runs/49052123167}
_{LLM reason (✨ experimental): Compilation failed in wallet tests: LoadExisting is called with six arguments, but its declaration only takes five.}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

…FromDB There are too many functions in CWallet with names like "Load" and "Create", disambiguate what CWallet::LoadWallet does by renaming it to PopulateWalletFromDB. -BEGIN VERIFY SCRIPT- sed -i 's|\bLoadWallet()|PopulateWalletFromDB()|g' $(git grep -l 'LoadWallet()' -- ':(exclude)src/wallet/walletdb.cpp') -END VERIFY SCRIPT-

@w0xlt

Co-authored-by: @w0xlt <w0xlt@users.noreply.github.com>

Modifying `fBroadcastTransactions` does not require any locks, initialization of this wallet parameter can be relocated with all of the other argument parsing in this function.

This section is necessarily repetitive, makes CWallet::Create() easier to read, and splits out functionality that will be useful when wallet creation and loading are separated.

`m_keypool_size` must be set before `CWallet::PopulateWalletFromDB()`, in order to move parsing of `-keypool` into `CWallet::LoadWalletArgs`, `LoadWalletArgs()` invocation in `CWallet::Create()` must be moved before `PopulateWalletFromDB()` is called.

Checking every SPKM in `CWallet::Create()` is not necessary, since the only way presently for an SPKM to get added to `m_spk_managers` (the return value of `GetAllScriptPubKeyMans()`) is through `AddScriptPubKeyMan()`, which already invokes `MaybeUpdateBirthTime()`.

This will avoid repetition when wallet creation and loading are separated.

Splits out logic relevant only to existing wallets in `CWallet::Create()` into `CWallet::LoadExisting()`

Writing the wallet version needs to be done on wallet creation, but WalletBatch::LoadWallet() does not, so factor this functionality out.

Previously creating an encrypted wallet would result in the keypool size incorrectly being reported as 0. See: bitcoin#32636 (comment)

Aside from being more legible, changing the name of `CWallet::Create()` also validates that every instance where a new wallet is `Create()`'ed is handled in this branch. -BEGIN VERIFY SCRIPT- sed -i 's|\bCreate(|CreateNew(|g' src/wallet/wallet.cpp src/wallet/wallet.h src/wallet/test/util.cpp src/wallet/test/wallet_tests.cpp -END VERIFY SCRIPT-

This was referenced May 29, 2025

wallet: migratewallet crashes "Assertion `legacy_spkm' failed" #32112

Open

wallet: migratewallet crashes "Assertion `m_wallet_flags == 0' failed" #32111

Open

DrahtBot mentioned this pull request May 29, 2025

Wallet: Add maxfeerate wallet startup option #29278

Open

w0xlt reviewed May 29, 2025

View reviewed changes

achow101 added the Wallet label Jun 4, 2025

pablomartin4btc mentioned this pull request Jun 15, 2025

wallet, refactor: Remove Legacy wallet unused warnings and errors #32481

Merged

pablomartin4btc reviewed Jun 15, 2025

View reviewed changes

rkrux reviewed Jun 16, 2025

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

This was referenced Jul 11, 2025

wallet: Remove upgradewallet RPC #32944

Merged

wallet: Remove wallet version and several legacy related functions #32977

Merged

DrahtBot mentioned this pull request Jul 23, 2025

[POC] wallet: Enable non-electronic (paper-based) wallet backup with codex32 #33043

Draft

achow101 reviewed Jul 23, 2025

View reviewed changes

src/qt/test/addressbooktests.cpp Show resolved Hide resolved

w0xlt reviewed Jul 24, 2025

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Jul 25, 2025

pablomartin4btc mentioned this pull request Aug 1, 2025

wallet, refactor: Remove Legacy check and error #33082

Open

pablomartin4btc reviewed Aug 1, 2025

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

pablomartin4btc reviewed Aug 1, 2025

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

davidgumberg force-pushed the 5-27-2025-create-refactor branch from 0f82224 to 349c9bc Compare August 23, 2025 02:07

DrahtBot removed the Needs rebase label Aug 23, 2025

davidgumberg force-pushed the 5-27-2025-create-refactor branch from 349c9bc to 353cfdd Compare August 23, 2025 05:09

DrahtBot added the CI failed label Aug 23, 2025

davidgumberg added a commit to davidgumberg/bitcoin that referenced this pull request Aug 23, 2025

wallet: remove loading logic from CWallet::Create

c3e5001

Also fixes wallet stats being incorrectly logged. (bitcoin#32636 (comment))

davidgumberg force-pushed the 5-27-2025-create-refactor branch from 353cfdd to 05cbadf Compare August 23, 2025 06:01

davidgumberg added a commit to davidgumberg/bitcoin that referenced this pull request Aug 23, 2025

wallet: remove loading logic from CWallet::Create

2826ccd

Also fixes wallet stats being incorrectly logged. (bitcoin#32636 (comment))

davidgumberg force-pushed the 5-27-2025-create-refactor branch from 5c33e05 to a374f51 Compare August 23, 2025 06:31

DrahtBot removed the CI failed label Aug 23, 2025

This was referenced Aug 23, 2025

wallet: Store transactions in a separate sqlite table #33034

Draft

wallet: Prepare for future upgrades by recording versions of last client to open and decrypt #32895

Open

wallet: Allow read-only database access for info and dump commands #32685

Open

DrahtBot mentioned this pull request Aug 27, 2025

wallet, rpc: remove settxfee and paytxfee #32138

Open

achow101 reviewed Aug 27, 2025

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

davidgumberg added a commit to davidgumberg/bitcoin that referenced this pull request Aug 28, 2025

wallet: Correctly log stats for encrypted messages.

1eb926d

Previously creating an encrypted wallet would result in the keypool size incorrectly being reported as 0. See: bitcoin#32636 (comment)

davidgumberg force-pushed the 5-27-2025-create-refactor branch from a374f51 to 7b5fcb7 Compare August 28, 2025 00:18

DrahtBot added the CI failed label Aug 28, 2025

davidgumberg and others added 15 commits August 27, 2025 20:33

refactor: wallet: move error handling to PopulateWalletFromDB()

8b6a8a0

wallet: refactor: PopulateWalletFromDB use switch statement.

f6c4454

Co-authored-by: @w0xlt <w0xlt@users.noreply.github.com>

refactor: Move -walletbroadcast setting init

3a29186

Modifying `fBroadcastTransactions` does not require any locks, initialization of this wallet parameter can be relocated with all of the other argument parsing in this function.

refactor: Split out wallet argument loading

d9a3af9

This section is necessarily repetitive, makes CWallet::Create() easier to read, and splits out functionality that will be useful when wallet creation and loading are separated.

refactor: Wallet stats logging in its own function

cf59f05

This will avoid repetition when wallet creation and loading are separated.

wallet: Create separate function for wallet load

29c624a

Splits out logic relevant only to existing wallets in `CWallet::Create()` into `CWallet::LoadExisting()`

wallet: Use CWallet::LoadExisting() for loading existing wallets.

7f7dc32

test: wallet: Split create and load

dd720fe

refactor: wallet: Add WriteVersion()

62e0721

Writing the wallet version needs to be done on wallet creation, but WalletBatch::LoadWallet() does not, so factor this functionality out.

wallet: remove loading logic from CWallet::Create

8ef5c41

wallet: Correctly log stats for encrypted messages.

8022e04

Previously creating an encrypted wallet would result in the keypool size incorrectly being reported as 0. See: bitcoin#32636 (comment)

davidgumberg force-pushed the 5-27-2025-create-refactor branch from 7b5fcb7 to a6b43fa Compare August 28, 2025 03:42

DrahtBot removed the CI failed label Aug 28, 2025

	// This wallet is in its first run if there are no ScriptPubKeyMans and it isn't blank or no privkeys
	const bool fFirstRun = walletInstance->m_spk_managers.empty() &&
	!walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) &&
	!walletInstance->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);

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

Are you sure you want to change the base?

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

Conversation

davidgumberg commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented May 29, 2025

Uh oh!

luke-jr commented Jun 6, 2025

Uh oh!

achow101 commented Jun 6, 2025

Uh oh!

pablomartin4btc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkrux left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pablomartin4btc commented Aug 1, 2025

Uh oh!

DrahtBot commented Aug 23, 2025

Uh oh!

davidgumberg commented Aug 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkrux commented Aug 25, 2025

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Aug 28, 2025

Uh oh!

Uh oh!

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

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

davidgumberg commented May 29, 2025 •

edited

Loading

DrahtBot commented May 29, 2025 •

edited

Loading

pablomartin4btc left a comment •

edited

Loading

rkrux left a comment •

edited

Loading

davidgumberg commented Aug 23, 2025 •

edited

Loading