wallet, refactor: Remove Legacy wallet unused warnings and errors #32481

pablomartin4btc · 2025-05-13T14:44:33Z

Remove dead code due to legacy wallet support removal.

These changes have no impact on functionality. They are transparent to the end user, as legacy wallets can't be created or loaded anymore, so these checks are no longer reached. The legacy-to-descriptor wallet migration flow is not affected either, as these removals are not part of its process.

DrahtBot · 2025-05-13T14:44:38Z

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/32481.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux, achow101

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:

#28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively 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.

src/qt/bitcoinstrings.cpp

DrahtBot · 2025-05-13T14:52:53Z

🚧 At least one of the CI tasks failed.
_{Task lint: https://github.com/bitcoin/bitcoin/runs/42142854745}
_{LLM reason (✨ experimental): The CI failure is due to trailing whitespace and spelling errors identified by the lint check.}

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.

src/wallet/wallet.cpp

pablomartin4btc · 2025-05-13T20:03:02Z

Updates:

Addressed @achow101's feedback by adding an Assert() on CreateWallet() enforcing only descriptor wallets can be created.

src/wallet/wallet.cpp

pablomartin4btc · 2025-05-16T18:25:15Z

Addressed feedback removing unnecessary descriptor wallet checks.

pablomartin4btc · 2025-06-09T23:42:25Z

Updates:

Removed "non-descriptor" checks and error instances in RPC (noticed while reviewing rpc, doc: update listdescriptors RCP help #32708).

rkrux

Looking good based on a cursory glance, will take a deeper look soon.
Maybe mention a note in the PR description about wallet migration and how removing these checks don't affect that flow?

src/wallet/wallet.cpp

Remove dead code due to legacy wallet removal.

SetupGeneration was supposed to be the function that all SPKMs used to setup automatic generation, but it didn't work out that way and ended up being legacy only. It should be deleted at this point.

It is not possible to load a legacy/ non-descriptor wallet anymore so no need to check for WALLET_FLAG_DESCRIPTORS in RPC calls, even when passing -rpcwallet/ JSON `/wallet/<walletname>/` endpoint, that searches for the wallets loaded already in the context.

pablomartin4btc · 2025-06-10T19:57:42Z

Updates:

Addressed feedback from @maflcko and @achow101.
Removed also wallet descriptor check from CWallet::EncryptWallet() and CWallet::Create().

rkrux

utACK ce90f0c

Looks fine to me, I navigated through the codebase to verify the diff's correctness. Suggested one more dead code removal if makes sense.

src/wallet/wallet.cpp

achow101 · 2025-06-14T00:10:20Z

ACK ce90f0c

150b5c9 wallet: replace `reload_wallet` with inline functionality (rkrux) 0f86da3 wallet: remove dead code in legacy wallet migration (rkrux) Pull request description: A discussion on a previous [PR 32481](#32481 (comment)) related to legacy wallet dead code removal made me realize that checking if the legacy wallet was loaded prior to the start of the migration is not required ever since legacy wallets can't be loaded in the first place. I also verified that the `load_on_start` persistent setting can also not cause the legacy wallets to be loaded, which further makes the case for removal of the above mentioned checks during migration. The current test coverage also shows these lines uncovered. ACKs for top commit: achow101: ACK 150b5c9 furszy: ACK 150b5c9 Tree-SHA512: 9bc7043cac1f4051228557208895e43648de3c7ffae6860c0676d1aa2db3a8ed3a09d1f9defacd96ca50bbb9699ba86652ccb0c5e55cc88be248a1fe727c13d9

fanquake reviewed May 13, 2025

View reviewed changes

src/qt/bitcoinstrings.cpp Outdated Show resolved Hide resolved

DrahtBot added the CI failed label May 13, 2025

pablomartin4btc force-pushed the wallet-remove-legacy-warnings-and-errors branch 2 times, most recently from a483558 to e66e9ee Compare May 13, 2025 14:59

achow101 reviewed May 13, 2025

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

pablomartin4btc force-pushed the wallet-remove-legacy-warnings-and-errors branch 2 times, most recently from 1c0ab13 to 3909051 Compare May 13, 2025 19:43

DrahtBot removed the CI failed label May 14, 2025

This was referenced May 14, 2025

wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively #28333

Open

Menu action to export a watchonly wallet bitcoin-core/gui#872

Draft

wallet: Add exportwatchonlywallet RPC to export a watchonly version of a wallet #32489

Draft

achow101 reviewed May 14, 2025

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

pablomartin4btc force-pushed the wallet-remove-legacy-warnings-and-errors branch from 3909051 to dbc8427 Compare May 16, 2025 18:21

DrahtBot added CI failed and removed CI failed labels May 16, 2025

pablomartin4btc force-pushed the wallet-remove-legacy-warnings-and-errors branch from dbc8427 to 5466936 Compare June 9, 2025 23:33

rkrux reviewed Jun 10, 2025

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

maflcko reviewed Jun 10, 2025

View reviewed changes

src/wallet/wallet.cpp Show resolved Hide resolved

pablomartin4btc force-pushed the wallet-remove-legacy-warnings-and-errors branch from 5466936 to c89e583 Compare June 10, 2025 17:20

achow101 reviewed Jun 10, 2025

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

pablomartin4btc added 3 commits June 10, 2025 16:27

wallet, refactor: Remove Legacy warnings and errors

5431f2d

Remove dead code due to legacy wallet removal.

wallet, refactor: Remove unused SetupGeneration

573bcd7

SetupGeneration was supposed to be the function that all SPKMs used to setup automatic generation, but it didn't work out that way and ended up being legacy only. It should be deleted at this point.

pablomartin4btc force-pushed the wallet-remove-legacy-warnings-and-errors branch from c89e583 to ce90f0c Compare June 10, 2025 19:34

DrahtBot added the CI failed label Jun 10, 2025

DrahtBot removed the CI failed label Jun 10, 2025

rkrux reviewed Jun 13, 2025

View reviewed changes

src/wallet/wallet.cpp Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Show resolved Hide resolved

src/wallet/wallet.cpp Show resolved Hide resolved

achow101 merged commit 2def858 into bitcoin:master Jun 14, 2025
18 of 19 checks passed

pablomartin4btc mentioned this pull request Jun 15, 2025

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

Open

rkrux mentioned this pull request Jun 16, 2025

wallet: remove dead code in legacy wallet migration #32758

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

wallet, refactor: Remove Legacy wallet unused warnings and errors #32481

wallet, refactor: Remove Legacy wallet unused warnings and errors #32481

Uh oh!

pablomartin4btc commented May 13, 2025 •

edited

Loading

Uh oh!

DrahtBot commented May 13, 2025 •

edited

Loading

Uh oh!

Uh oh!

DrahtBot commented May 13, 2025

Uh oh!

Uh oh!

pablomartin4btc commented May 13, 2025

Uh oh!

Uh oh!

pablomartin4btc commented May 16, 2025

Uh oh!

pablomartin4btc commented Jun 9, 2025

Uh oh!

rkrux left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pablomartin4btc commented Jun 10, 2025

Uh oh!

rkrux left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

achow101 commented Jun 14, 2025

Uh oh!

Uh oh!

Uh oh!

wallet, refactor: Remove Legacy wallet unused warnings and errors #32481

wallet, refactor: Remove Legacy wallet unused warnings and errors #32481

Uh oh!

Conversation

pablomartin4btc commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

Uh oh!

DrahtBot commented May 13, 2025

Uh oh!

Uh oh!

pablomartin4btc commented May 13, 2025

Uh oh!

Uh oh!

pablomartin4btc commented May 16, 2025

Uh oh!

pablomartin4btc commented Jun 9, 2025

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pablomartin4btc commented Jun 10, 2025

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

achow101 commented Jun 14, 2025

Uh oh!

Uh oh!

Uh oh!

pablomartin4btc commented May 13, 2025 •

edited

Loading

DrahtBot commented May 13, 2025 •

edited

Loading