wallet: Migrate legacy wallets to descriptor wallets #19602
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
5bb71b2 to
ac5549a
Compare
|
Why doesn't this extend |
It didn't feel like it fit This is also pretty invasive and fundamentally changes how the wallet is working, so I wanted to keep it separate from something that people might still want to use on legacy wallets. |
ac5549a to
97e34cd
Compare
|
I think we should only add this functionality after |
I think there's a question of whether we want to keep sqlite separate from descriptors. We might want to allow legacy wallets to migrate to sqlite without migrating to descriptors since they are orthogonal. |
|
Do you know what the performance of a wallet would be like, in the case of a non-HD wallet being migrated? |
Probably worse. |
Almost certainly, that's why I'm asking :) It would be nice to know how much worse it would be |
test/functional/wallet_migration.py
Outdated
|
|
||
| addr = ms_info["address"] | ||
| txid = default.sendtoaddress(addr, 10) | ||
| multisig1.importaddress(addr) |
There was a problem hiding this comment.
fb5e095: why do you need to call importaddress after addmultisigaddress? Is that makes it marked as watch-only (as oppose to the second address)? Why?
(update: see above, but I still don't understand why)
There was a problem hiding this comment.
So that this particular address is being watched and will end up in the _watchonly wallet.
|
I was confused about the difference between solvable and watch-only.
A solvable address is not automatically watch-only, but all import methods except
|
|
I also noticed the label given by We should consider making the |
11f19f9 to
a2278c2
Compare
|
The issues with locks and hanging should be resolved now. I've changed it to just unload a wallet before migrating, then loading it but without connecting all of the signals, etc. for the migration itself. This should fix the issues with locks and hanging (which was caused by waiting for the shared_ptr to be released by the GUI).
I don't think we should. The point is to make it so that migrated wallets do not have any dependency on legacy stuff at all so that we can remove the legacy wallet. I agree that it can be confusing, but I don't think it is helpful or less confusing to make a new legacy wallet when the user is expecting everything to go to descriptors. |
|
The problem is that the solvables wallet is completely useless. It can't sign things the legacy wallet could sign. You might as well put these things in the watch-only wallet, unless I'm missing something. Another approach could be to handle the special (and only?) case of a multisig wallet more gracefully. The best substitute we can make for how that worked before, is to create a blank descriptor wallet and then import the (inferred) descriptor with the private keys that we have. They'll lose the "feature" of not seeing transactions, but they'll retain the ability to sign, which is more important. The locking issue seems resolved. It's also no longer freezing the UI while the migration is in progress. Labels It's quite a slow call (several minutes), so a nice followup improvement would be a way to abort it and maybe a progress bar. It's a one-off task, but probably one that makes users very nervous. Notice one glitch: if you call this from GUI console, since it closes the wallet, you'll suddenly see a different wallet selected in the console, even though the command is still in progress. That's confusing, though maybe it's enough for now to just to warn the user to ignore that. It also crashes if you touch the dropdown though. |
The solvables wallet can update a PSBT with the UTXO, scripts, and pubkeys, for inputs that spend the any of the scripts it contains. Because the user had not watched those scripts when the wallet is a legacy wallet, it does not make sense to have them be in the watchonly wallet. |
|
The slowness seems to be macOS thing, or even particular to one of my machines. The exact same wallet takes ~9 minutes to upgrade on one on my macOS machines, and less than a minute on the other and on Ubuntu. When the upgrade is fast, the labels are preserved fine too. It would be useful to know how people use(d) legacy multisig wallets, especially before PSBT. It's not clear to me with which wallet they would craft the transaction. A solveable-only legacy wallet can sign them and add more metadata, but because it lacks transaction history, they can't craft them. Presumably the user would have different wallet for that purpose, which they may or may not be upgrading separately. As long as that other wallet can produce a PSBT, then our upgraded main wallet can sign it just fine. The _solvable wallet's only purpose then is to backup the redeemscript. They should never need it. If their other wallet can't produce a psbt, then the upgrade process breaks their ability to sign, since the private keys are now no longer in the same wallet as the redeemscript. So we should warn about that. And we could a utility that takes a raw unsigned transaction and transform it into a PSBT to address that potential problem. Knowing these use cases would also help us write upgrade documentation for them, to explain what new workflows they should consider. |
| } | ||
|
|
||
| std::optional<MigrationData> res = legacy_spkm->MigrateToDescriptor(); | ||
| if (res == std::nullopt) { |
There was a problem hiding this comment.
You're assuming here that the reason std::nullopt is returned is that the wallet is locked, rather than some other failure.
The RPC handler is ensuring that the wallet is unlocked (third line there). This check is redundant for now, and might be useful when a migration button is implemented on the GUI.
They don't have any private data and they can't be nested so they should return false for ToPrivateString.
Co-Authored-By: furszy <matiasfurszyfer@protonmail.com>
a2278c2 to
53e7ed0
Compare
|
tACK 53e7ed0 I think this is good enough for an |
6725030 qt: Update translation source file for string freeze (round 2) (Hennadii Stepanov) Pull request description: On the day of [translation string freeze](bitcoin/bitcoin#24987 (comment)), it happened that #660 did not include new strings from bitcoin/bitcoin#19602. This PR includes all recent updates. As a Transifex translator, I believe it is enough time for all translators to handle a few new strings by a release date. Also a Transifex check failure has been [fixed](/pull/664). ACKs for top commit: jarolrod: ACK 6725030 Tree-SHA512: d57b841e87e389d31ec4ae9067b83f7f209e168399bc088c3234c2c66b34772739cb801f04b5038d55de115083d022d603bc976374bfd537b8ea10c10a545183
| for (const auto& addr_pair : m_address_book) { | ||
| // Labels applied to receiving addresses should go based on IsMine | ||
| if (addr_pair.second.purpose == "receive") { | ||
| if (!IsMine(addr_pair.first)) { |
There was a problem hiding this comment.
In commit "Implement MigrateLegacyToDescriptor" (0bf7b38)
It seems like this should say IsMine not !IsMine. Also I'm not sure why addr_pair.second.purpose == "receive" check is necessary. It seems like just checking IsMine should be enough, and it would be less robust to rely on purpose field being present.
There was a problem hiding this comment.
It's because the migration process divides data between different wallets.
It first deletes and unloads the legacy spkm from the main wallet (check the beginning of the function), and setup new descriptors. Then moves the data to two possible wallets; a solvable and a watch-only wallet.
So the !IsMine means that the record in the addressbook is from the legacy spkm (not loaded anymore), so it needs migration to the new wallets.
It also took me a while to get it while was rewriting it for #26836.
(sorry for the double PR mention, saw late your response in the other PR)
This PR adds a new
migratewalletRPC which migrates a legacy wallet to a descriptor wallet. Migrated wallets will need a new backup. If a wallet has watchonly stuff in it, a new watchonly descriptor wallet will be created containing those watchonly things. The related transactions, labels, and descriptors for those watchonly things will be removed from the original wallet. Migrated wallets will not have any of the legacy things be available for fetching fromgetnewaddressorgetrawchangeaddress. Wallets that have private keys enabled will have newly generated descriptors. Wallets with private keys disabled will not have any activeScriptPubKeyMans.For the basic HD wallet case of just generated keys, in addition to the standard descriptor wallet descriptors using the master key derived from the pre-existing hd seed, the migration will also create 3 descriptors for each HD chain in: a ranged combo external, a ranged combo internal, and a single key combo for the seed (the seed is a valid key that we can receive coins at!). The migrated wallet will then have newly generated descriptors as the active
ScriptPubKeyMans. This is equivalent to creating a new descriptor wallet and importing the 3 descriptors for each HD chain. For wallets containing non-HD keys, each key will have its own combo descriptor.There are also tests.