Skip to content

wallet, refactor: Remove Legacy check and error #33082

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

wallet, refactor: Remove Legacy check and error #33082

wants to merge 2 commits into from
+1 −6

Conversation

pablomartin4btc
Copy link
Member

@pablomartin4btc pablomartin4btc commented Jul 28, 2025 •
edited
Loading

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.

 
 void CWallet::UpgradeDescriptorCache()
 {
+    // Only descriptor wallets can upgrade descriptor cache
+    Assert(IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS));
+
-    if (!IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS) || IsLocked() || IsWalletFlagSet(WALLET_FLAG_LAST_HARDENED_XPUB_CACHED)) {
+    if (IsLocked() || IsWalletFlagSet(WALLET_FLAG_LAST_HARDENED_XPUB_CACHED)) {
         return;
     }

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 28, 2025 •
edited
Loading

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.

Type Reviewers
ACK davidgumberg
Stale ACK brunoerg

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:

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

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.

@pablomartin4btc pablomartin4btc marked this pull request as draft July 28, 2025 18:57
@DrahtBot
Copy link
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.

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.

@fanquake
Copy link
Member

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

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

@pablomartin4btc
Copy link
Member Author

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!

@pablomartin4btc pablomartin4btc force-pushed the wallet-refactor-remove-legacy-checks-and-errors branch 2 times, most recently from d6fec28 to ffbddde Compare July 31, 2025 23:50
@pablomartin4btc pablomartin4btc force-pushed the wallet-refactor-remove-legacy-checks-and-errors branch from ffbddde to 3d8a189 Compare August 1, 2025 00:21
@pablomartin4btc
Copy link
Member Author

Updates

  • Reverted a legacy wallet check and removed some unnecessary LoadWallet() calls from a few wallet tests in the first commit.

@pablomartin4btc pablomartin4btc marked this pull request as ready for review August 1, 2025 01:19
@DrahtBot DrahtBot removed the CI failed label Aug 1, 2025
@pablomartin4btc pablomartin4btc mentioned this pull request Aug 1, 2025
Open
brunoerg
brunoerg reviewed Aug 21, 2025
View reviewed changes
brunoerg
brunoerg reviewed Aug 21, 2025
View reviewed changes
Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code review ACK 3d8a189

  • Checked that the unecessary calls to LoadWallet were removed from the tests - I didn't find any other occurance apart of TestLoadWallet.

@pablomartin4btc
wallet, refactor: Remove Legacy check and error
d3c5e47 
Remove dead code due to legacy wallet removal.
@pablomartin4btc pablomartin4btc force-pushed the wallet-refactor-remove-legacy-checks-and-errors branch from 3d8a189 to d3c5e47 Compare August 22, 2025 19:49
@pablomartin4btc
Copy link
Member Author

Updates

  • Addressed @brunoerg's feedback; removed redundant comment, as the added assertion makes the message clear enough.

davidgumberg added a commit to davidgumberg/bitcoin that referenced this pull request Aug 23, 2025
@davidgumberg @pablomartin4btc
wallet: UpgradeDescCache unreachable for legacy.
357b1b8 
Now that wallet loading doesn't call PopulateWalletFromDB,
UpgradeDescriptorCache is not reachable for legacy wallets, since they
are only loaded, never created.

Suggested here: bitcoin#33082 (comment)

Co-authored-by: pablomartin4btc <pablomartin4btc@gmail.com>
@davidgumberg
Copy link
Contributor

crACK d3c5e47

Removes unuseful legacy wallet code, also suggested here: #32636 (comment)

The suggestion in the PR description won't work without 30c6f64, if this gets merged first I'll add it to #32636.

@DrahtBot DrahtBot requested a review from brunoerg August 23, 2025 06:50
@maflcko maflcko mentioned this pull request Sep 3, 2025
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brunoerg brunoerg Awaiting requested review from brunoerg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pablomartin4btc @DrahtBot @fanquake @davidgumberg @brunoerg