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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32481.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
250 | -"Wallet loaded successfully. The legacy wallet type is being deprecated and " 251 | -"support for creating and opening legacy wallets will be removed in the " 252 | -"future. Legacy wallets can be migrated to a descriptor wallet with " 253 | -"migratewallet."), 254 | -QT_TRANSLATE_NOOP("bitcoin-core", "" 255 | "Warning: Dumpfile wallet format \"%s\" does not match command line specified "
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task lint: https://github.com/bitcoin/bitcoin/runs/42142854745</sub>
<sub>LLM reason (✨ experimental): The CI failure is due to trailing whitespace and spelling errors identified by the lint check.
</sub>
<details><summary>Hints</summary>
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.
</details>
376 | @@ -382,11 +377,6 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string& 377 | const SecureString& passphrase = options.create_passphrase; 378 | 379 | if (wallet_creation_flags & WALLET_FLAG_DESCRIPTORS) options.require_format = DatabaseFormat::SQLITE; 380 | - else {
In e66e9ee9f19413c93d8523afdb8f9f3e07b818ec "wallet, refactor: Remove Legacy warnings and errors"
I'd prefer to at least preserve enforcement that the wallet being created is a descriptor wallet. Perhaps we could add an Assert() or Assume() for the flag.
I did that earlier but got a failure and I thought it was related with migration and rolled it back without checking it properly. I'll add it back.
375 | @@ -381,12 +376,10 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string& 376 | uint64_t wallet_creation_flags = options.create_flags; 377 | const SecureString& passphrase = options.create_passphrase; 378 | 379 | + // Only descriptor wallets can be created 380 | + Assert(wallet_creation_flags & WALLET_FLAG_DESCRIPTORS); 381 | + 382 | if (wallet_creation_flags & WALLET_FLAG_DESCRIPTORS) options.require_format = DatabaseFormat::SQLITE;
In 5111176aec91396320c7a337c0613389c1a5f4c0 "wallet, refactor: Remove Legacy warnings and errors"
This if is not needed anymore with the above Assert.
maybe neither do this one:
(just to confirm and do both in 1 go)
Yes
Addressed feedback removing unnecessary descriptor wallet checks.
<ins>Updates</ins>:
383 | - status = DatabaseStatus::FAILED_CREATE; 384 | - return nullptr; 385 | - } 386 | + // Only descriptor wallets can be created 387 | + Assert(wallet_creation_flags & WALLET_FLAG_DESCRIPTORS); 388 | +
nit if retouched: extra blank line here
If was intentional but it makes sense. Done, thanks!
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?
382 | - error = Untranslated("Legacy wallets can no longer be created"); 383 | - status = DatabaseStatus::FAILED_CREATE; 384 | - return nullptr; 385 | - } 386 | + // Only descriptor wallets can be created 387 | + Assert(wallet_creation_flags & WALLET_FLAG_DESCRIPTORS);
there is now dead code in line 401: if ((wallet_creation_flags & WALLET_FLAG_EXTERNAL_SIGNER) && !(wallet_creation_flags & WALLET_FLAG_DESCRIPTORS)) {
Good catch! Done, thanks.
396 | @@ -405,7 +397,7 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string& 397 | } 398 | 399 | // Descriptor support must be enabled for an external signer wallet 400 | - if ((wallet_creation_flags & WALLET_FLAG_EXTERNAL_SIGNER) && !(wallet_creation_flags & WALLET_FLAG_DESCRIPTORS)) { 401 | + if (wallet_creation_flags & WALLET_FLAG_EXTERNAL_SIGNER) {
In 8fbb1c9657ef034aa179655a0869db45be2aaac8 "wallet, refactor: Remove Legacy warnings and errors"
That's wrong and causing a CI failure. I think this error condition is unneeded now.
Yeah, the simplification of a && !true is a && false is false, so dead code and not a.
Yeah, my bad, also de error description was clear. Just found another case, I'll fix this soon. Thanks!
Sorry, having said that, the error description needs to be updated mentioning that external signer should be enabled instead of descriptor wallet.
I think this error condition is unneeded now.
Yup, I'll remove it.
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.
The changes of CreateWallet and EncryptWallet in this file are fine because these functions are called only during new wallet creation and/or encryption.
There is another dead code in this file in AddWalletDescriptor function if you want to remove it as well in this diff: https://github.com/bitcoin/bitcoin/blob/3e816844269238ed1b4f32139bf37ca7a213a7af/src/wallet/wallet.cpp#L3718-L3720
This function seems to be called only while creating watch-only & solvables wallets during migration and the importdescriptors RPC.
Yup, I was already planning to do in a separate PR since I found an issue/ improvement in importdescriptors RPC.
2880 | - return nullptr; 2881 | - } 2882 | - } 2883 | - } 2884 | + walletInstance->SetupDescriptorScriptPubKeyMans(); 2885 | + // SetupDescriptorScriptPubKeyMans already calls SetupGeneration for us so we don't need to call SetupGeneration separately
These changes inside the common CWallet::Create function are fine because these are enclosed by the if (firstRun) check that should only happen when the wallet is run for the first time. Not applicable in the migration flow as well because only non empty wallets are migrated that would make them not fall under the firstRun check: https://github.com/bitcoin/bitcoin/blob/3e816844269238ed1b4f32139bf37ca7a213a7af/src/wallet/wallet.cpp#L4300-L4303
Also, there is an assert already in this block few lines above that adds more robustness.
287 | @@ -288,11 +288,6 @@ std::shared_ptr<CWallet> LoadWalletInternal(WalletContext& context, const std::s 288 | return nullptr; 289 | } 290 | 291 | - // Legacy wallets are being deprecated, warn if the loaded wallet is legacy 292 | - if (!wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { 293 | - warnings.emplace_back(_("Wallet loaded successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future. Legacy wallets can be migrated to a descriptor wallet with migratewallet.")); 294 | - }
This portion is not triggered in the current code coverage: https://maflcko.github.io/b-c-cov/total.coverage/src/wallet/wallet.cpp.gcov.html
I think there are couple cases where this can be triggered - when the legacy wallet was already loaded and migration was started but for some reason the backup and unlock failed: https://github.com/bitcoin/bitcoin/blob/3e816844269238ed1b4f32139bf37ca7a213a7af/src/wallet/wallet.cpp#L4269-L4283
But it seems fine to remove the warning because the user would already be aware of the need to migrate this legacy wallet, so this warning is not needed here.
Mmm... I doubt it... just double checked running WalletMigrationTest.test_encrypted() (which would produced the unlock failure, similar behaviour as the backup failure, since both would return util::Error) with the removed check (-291 to -294) and that didn't reach the un-removed lines.
I don't think the test would have covered those lines because the test coverage didn't show them covered. I found out that the reason test_encrypted didn't cover those lines is because the migration was done on the master node and the wallet to be migrated was not loaded on the master node. The wallet needs to be loaded for the above flows to be triggered.
Right, there are 2 different things (or flows within LoadWalletInternal). First what I tried to explain in my previous comment (at least in my head) was that the migration process won't reach those lines (inside the if condition) because when that flow goes thru LoadWalletInternal it does it with the purpose of creating the target descriptor wallet so at that check/ condition the wallet is always a descriptor already. You can execute the migration using the old node (self.old_node.migratewallet) and will be the same thing. The second point is the "loading" behaviour of LoadWalletInternal, in order to hit it you had to call loadwallet RPC for a legacy wallet using the old node (v28 node or earlier), a legacy can't be loaded on the master node anymore (a validation cuts the flow earlier), that's why it's dead code (you can see that the test validating that warning was removed from test/functional/wallet_backwards_compatibility.py in #31250). Maybe you were saying the same thing but just wanted to clarify this for other ppl that could ever read this. Having LoadWalletIternal these 2 different behaviours (loading and creating) seems incorrect and brings confusion, I see there's already some interest in order to fix this in #32636. Thanks for your review!
utACK ce90f0c99fded22dd24f08757d6f48b5c6b52990
Looks fine to me, I navigated through the codebase to verify the diff's correctness. Suggested one more dead code removal if makes sense.
ACK ce90f0c99fded22dd24f08757d6f48b5c6b52990