wallet: remove dead code in legacy wallet migration

[rkrux]

A discussion on a previous PR 32481 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.

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	furszy, achow101
Stale ACK	pablomartin4btc

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:

• #32273 (wallet: Fix relative path backup during migration. 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.

previous releases, depends DEBUGFailing after 4m — Task Summary

This seems unrelated as the error is 0curl: (6) Could not resolve host: github.com in the CI run.

Code review ACK

I've left some comments.

I think now the reload_wallet helper could be moved into the if (success) { condition:
https://github.com/bitcoin/bitcoin/blob/67845e0411241d83c770fafa933b11cf6b8b27b8/src/wallet/wallet.cpp#L4202-L4209

This could be removed entirely if MigrateLegacyToDescriptor(std::shared_ptr<CWallet> local_wallet, ... was only called from MigrateLegacyToDescriptor(const std::string& wallet_name, ... but currently this function is also being called from the bench wallet_migration.cpp.

I think now the reload_wallet helper could be moved into the if (success) { condition:

Makes sense, I have done this. Also, ended up doing few related changes in the subsequent commits.

re-ACK 0635f107c4e1442ce6b953308723b58044c558df

Concept ACK

I think the second commit would look nicer and simpler after something like e86d71b (which is part of #31423). We’d be able to inline the function at that point.

Thanks for highlighting.
I'll take a look at #31423 soon, it had fallen off my radar for some reason.

ACK 0635f107c4e1442ce6b953308723b58044c558df

Rebased over master to incorporate changes from #31423.

code reviewed, looks good. Just left a comment about the last commit.

ACK 150b5c9

ACK 150b5c9