wallet, refactor: Remove Legacy check and error #33082

1. 
   pablomartin4btc commented at 6:36 pm on July 28, 2025: member

   Remove dead code due to legacy wallet removal.

   Leftovers from previous #32481.

   ---

   Note:

   While attempting to remove the legacy check in CWallet::UpgradeDescriptorCache() (which is called from DBErrors WalletBatch::LoadWallet(CWallet* pwallet)), I once again ran into the fact that LoadWallet() is used in two distinct scenarios — something I was already aware of:

   • Wallet creation – the upgrade is ignored here because no wallet flags are yet set; attempting to set a flag (ie WALLET_FLAG_LAST_HARDENED_XPUB_CACHED at the end of the upgrade function, if the legacy check is removed) would produce a failure (DBErrors CWallet::LoadWallet() -> Assert(m_wallet_flags == 0)).
   • Wallet loading – the upgrade proceeds correctly and the flag WALLET_FLAG_LAST_HARDENED_XPUB_CACHED is set.

   While revisiting this, I also noticed that some LoadWallet() calls in the wallet tests are unnecessary and I’ve removed them in the first commit.

   The following change in UpgradeDescriptorCache() could be done in PR #32636 as part of the separation between wallet loading and creation responsibilities.

   0 
   1 void CWallet::UpgradeDescriptorCache()
   2 {
   3+    // Only descriptor wallets can upgrade descriptor cache
   4+    Assert(IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS));
   5+
   6-    if (!IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS) || IsLocked() || IsWalletFlagSet(WALLET_FLAG_LAST_HARDENED_XPUB_CACHED)) {
   7+    if (IsLocked() || IsWalletFlagSet(WALLET_FLAG_LAST_HARDENED_XPUB_CACHED)) {
   8         return;
   9     }
   

2. 
   DrahtBot commented at 6:36 pm on July 28, 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/33082.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

3. pablomartin4btc marked this as a draft on Jul 28, 2025
4. DrahtBot added the label CI failed on Jul 28, 2025
5. 
   DrahtBot commented at 11:35 pm on July 28, 2025: contributor

   🚧 At least one of the CI tasks failed. Task MSan, depends: https://github.com/bitcoin/bitcoin/runs/46884702924 LLM reason (✨ experimental): The CI failure is caused by an assertion failure in wallet/wallet.cpp:521 due to IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS), leading to subprocess abortion.

   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.

6. 
   fanquake commented at 9:35 am on July 29, 2025: member

   https://github.com/bitcoin/bitcoin/actions/runs/16577317093/job/46884696846?pr=33082#step:7:2967:

   0 131/145 Test   [#7](/bitcoin-bitcoin/7/): bench_sanity_check ...................Subprocess aborted***Exception:  30.55 sec
   1Running with -sanity-check option, output is being suppressed as benchmark results will be useless.
   2wallet/wallet.cpp:521 UpgradeDescriptorCache: Assertion `IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)' failed.
   

7. 
   pablomartin4btc commented at 2:12 pm on July 29, 2025: member

   131/145 Test # 7: bench_sanity_check ……………….Subprocess aborted***Exception: 30.55 sec wallet/wallet.cpp:521 UpgradeDescriptorCache: Assertion `IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)’ failed.

   Yeah, I’m on it, that’s why I put it on draft yesterday. Thanks!

8. pablomartin4btc force-pushed on Jul 31, 2025
9. pablomartin4btc force-pushed on Jul 31, 2025
10. test: Remove unnecessary LoadWallet() calls 30c6f64eed
11. wallet, refactor: Remove Legacy check and error

    Remove dead code due to legacy wallet removal.

    3d8a1895fc
12. pablomartin4btc force-pushed on Aug 1, 2025
13. 
    pablomartin4btc commented at 1:19 am on August 1, 2025: member

    Updates

    • Reverted a legacy wallet check and removed some unnecessary LoadWallet() calls from a few wallet tests in the first commit.
14. pablomartin4btc marked this as ready for review on Aug 1, 2025
15. DrahtBot removed the label CI failed on Aug 1, 2025

Contributors
pablomartin4btc DrahtBot fanquake