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

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

2. 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
3. refactor: wallet: move error handling to PopulateWalletFromDB() e5634cd891
4. 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
5. 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
6. 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
7. 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
8. refactor: Wallet stats logging in its own function

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

   744ebdc185
9. wallet: Create separate function for wallet load

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

   735100ed8b
10. wallet: Use CWallet::LoadExisting() for loading existing wallets. 26f5ece48b
11. test: wallet: Split create and load 1e66288eed
12. wallet: remove loading logic from CWallet::Create d5d55b3554
13. 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
14. 
    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
    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.

15. 
    w0xlt commented at 8:14 pm on May 29, 2025: contributor

    Approach ACK
16. 
    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.

Contributors
davidgumberg DrahtBot w0xlt achow101