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

davidgumberg commented at 0:37 am on May 29, 2025: contributor

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:

https://github.com/bitcoin/bitcoin/blob/370c59261269fd9043674e0f4fd782a89e724473/src/wallet/wallet.cpp#L2882-L2885

This caused issues like #32112 and #32111 (both of which are fixed by this PR) and likely other related misbehavior for any existing wallet 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.

scripted-diff: refactor: rename CWallet::LoadWallet

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-

f26bf5fd24

refactor: wallet: move error handling to PopulateWalletFromDB() e5634cd891

refactor: Move -walletbroadcast setting init

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.

f74bf3fcb9

refactor: Split out wallet argument loading

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.

56ca187abc

wallet: Move argument parsing to before DB load

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

f3e2400159

wallet: Remove redundant birth time update

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()`.

b9ac41e61f

refactor: Wallet stats logging in its own function

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

744ebdc185

wallet: Create separate function for wallet load

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

735100ed8b

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

test: wallet: Split create and load 1e66288eed

wallet: remove loading logic from CWallet::Create d5d55b3554

scripted-diff: refactor: CWallet::Create() -> CreateNew()

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-

0f82224d39

DrahtBot commented at 0:37 am on May 29, 2025: contributor

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:

#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 commented at 8:14 pm on May 29, 2025: contributor

Approach ACK

achow101 commented at 9:56 pm on May 29, 2025: member

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.

achow101 added the label Wallet on Jun 4, 2025

luke-jr commented at 9:05 pm on June 6, 2025: member

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 commented at 9:11 pm on June 6, 2025: member

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 commented at 0:47 am on June 15, 2025: member

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.

in src/wallet/wallet.cpp:3084 in 744ebdc185 outdated

3085-        LOCK(walletInstance->cs_wallet);
3086-        walletInstance->WalletLogPrintf("setKeyPool.size() = %u\n",      walletInstance->GetKeyPoolSize());
3087-        walletInstance->WalletLogPrintf("mapWallet.size() = %u\n",       walletInstance->mapWallet.size());
3088-        walletInstance->WalletLogPrintf("m_address_book.size() = %u\n",  walletInstance->m_address_book.size());
3089-    }
3090+    WITH_LOCK(walletInstance->cs_wallet, walletInstance->LogStats());

rkrux commented at 1:31 pm on June 16, 2025:

In 744ebdc18556223b8749a7fed420c9ae95be418e:

Sometime back I noted that during the creation of encrypted wallets, the wallet metrics are not correctly logged after the SPKMs have been created. This is unlike the creation of non-encrypted wallets where the correct metrics are logged because they are done so after the setup of SPKMS. PFB a sample from my node.

 0^[[C=2025-05-27T15:19:21Z Using SQLite Version 3.43.2
 12025-05-27T15:19:21Z Using wallet /Users/rkrux/Library/ApplicationSupport/Bitcoin/regtest/wallets/encryptedtest
 22025-05-27T15:19:21Z init message: Loading wallet…
 32025-05-27T15:19:21Z [encryptedtest] Legacy Wallet Keys: 0 plaintext, 0 encrypted, 0 w/ metadata, 0 total.
 42025-05-27T15:19:21Z [encryptedtest] Descriptors: 0, Descriptor Keys: 0 plaintext, 0 encrypted, 0 total.
 52025-05-27T15:19:21Z [encryptedtest] Setting minversion to 169900
 62025-05-27T15:19:21Z [encryptedtest] Wallet completed loading in              12ms
 72025-05-27T15:19:21Z [encryptedtest] setKeyPool.size() = 0
 82025-05-27T15:19:21Z [encryptedtest] mapWallet.size() = 0
 92025-05-27T15:19:21Z [encryptedtest] m_address_book.size() = 0
102025-05-27T15:19:22Z [encryptedtest] Encrypting Wallet with an nDeriveIterations of 251048
112025-05-27T15:19:22Z [encryptedtest] Setting spkMan to active: id = a8af620b202a2542a6b4575578133499397089fa67110b27dffced31b3e6ccbd, type = legacy, internal = false
122025-05-27T15:19:22Z [encryptedtest] Setting spkMan to active: id = b389c6313cd85b907990fd2c57a87db230fa0dad1c94c929dd0e010e036d802e, type = p2sh-segwit, internal = false
132025-05-27T15:19:22Z [encryptedtest] Setting spkMan to active: id = 727d3e57942f107c713932ccf8aec8f6968ee76b98f8787802afc62cbaa13d38, type = bech32, internal = false

I was not sure if a PR just for this would garner review but now that the code touching these metrics is being updated anyway, might as well fix this issue. I have not checked if this PR already fixes it, great if it does!

in src/wallet/wallet.cpp:3027 in d5d55b3554 outdated

3020@@ -3021,11 +3021,6 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
3021         return nullptr;
3022     }
3023 
3024-    // This wallet is in its first run if there are no ScriptPubKeyMans and it isn't blank or no privkeys
3025-    const bool fFirstRun = walletInstance->m_spk_managers.empty() &&
3026-                     !walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) &&
3027-                     !walletInstance->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);

rkrux commented at 1:54 pm on June 16, 2025:

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

Agree that this is not a good heuristic but the PR description doesn’t seem to explain or summarise why it’s a bad heuristic. Maybe add few points from the findings of the linked issues?

rkrux commented at 2:38 pm on June 16, 2025: contributor

Concept ACK 0f82224d3937db3f60cfaec249ac5fe3264dc3d5 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.

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.

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

Code Coverage & Benchmarks

Reviews

Conflicts

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