wallet: remove most asserts of WALLET_FLAG_DESCRIPTORS flag #34502

1. 
   rkrux commented at 10:42 am on February 4, 2026: contributor

   Reasons:

   1. Few of the wallet methods are descriptors specific as suggested by their names, these assertions seems redundant within them.
   2. Since the legacy wallet can’t be loaded anymore, few of these assertions are outdated now.
   3. Seeing too many of these assertions in the CWallet class while reading the code is cognitively loaded, and after a while becomes annoying for the reader.

   Note: Such assertions in the migration specific functions have been left untouched because they seem necessary there.

2. DrahtBot added the label Wallet on Feb 4, 2026
3. 
   DrahtBot commented at 10:42 am on February 4, 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. A summary of reviews will appear here.

   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.

4. 
   rkrux commented at 11:49 am on February 4, 2026: contributor

   I had incorrectly run the functional tests in a different directory in local that led to all the functional tests pass. I can reproduce the test failure locally now, putting the PR in draft for now.
5. rkrux marked this as a draft on Feb 4, 2026
6. DrahtBot added the label CI failed on Feb 4, 2026
7. wallet: remove few asserts of `WALLET_FLAG_DESCRIPTORS` flag

   Reasons:
   1. Few of the wallet methods are descriptors specific as suggested
   by their names, these assertions seems redundant within them.
   2. Since the legacy wallet can't be loaded anymore, few of these
   assertions are outdated now.
   3. Seeing too many of these assertions in the `CWallet` class while
   reading the code is cognitively loaded, and after a while becomes
   annoying for the reader.
   
   Note: Such assertions in the migration specific functions have been
   left untouched because they seem necessary there.

   a3ff41a5a8
8. wallet: move out `UpgradeDescriptorCache()` wallet flag check to the callers

   The `UpgradeDescriptorCache` method ends up being called via the
   `CWallet::Unlock` and `WalletBatch::LoadWallet` methods, both of
   which can be called for the legacy wallets within the context of
   wallet migration. For the former caller, it is evident in the
   `test_encrypted` test of `wallet_migration` suite.
   
   The previous code where there is no explicit check in the callers
   makes the reader believe that the callers end up being called only
   for descriptor wallets, which is misleading. This move out of the
   check makes it explicit for the reader that this is not the only
   flow for these callers.

   77778e2559
9. rkrux force-pushed on Feb 4, 2026
10. rkrux renamed this:
    wallet: remove few asserts of `WALLET_FLAG_DESCRIPTORS` flag
    wallet: remove most asserts of `WALLET_FLAG_DESCRIPTORS` flag
    on Feb 4, 2026
11. DrahtBot removed the label CI failed on Feb 4, 2026
12. rkrux marked this as ready for review on Feb 5, 2026
13. 
    frankomosh commented at 11:29 am on February 9, 2026: contributor

    Few questions from my code review;

    Why remove the assertion from EncryptWallet()? I think it also does descriptor-specific work (iterates m_spk_managers, calls SetupDescriptorScriptPubKeyMans()).

    You said in the other commit that Unlock() and LoadWallet() can be called during migration for legacy wallets. I wonder if EncryptWallet() can face a similar situation.

Contributors
rkrux DrahtBot frankomosh

Labels
Wallet