wallet: Migrate legacy wallets to descriptor wallets#19602
wallet: Migrate legacy wallets to descriptor wallets#19602achow101 merged 11 commits intobitcoin:masterfrom
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. |
achow101
force-pushed
the
descriptor-wallet-migration
branch
3 times, most recently
from
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. |
achow101
force-pushed
the
descriptor-wallet-migration
branch
from
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. |
maflcko
left a comment
maflcko
left a comment
There was a problem hiding this comment.
Left a style-nit. Feel free to ignore.
|
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 |
Sjors
left a comment
Sjors
left a comment
There was a problem hiding this comment.
Thanks for the changes. Reviewed another commit, will continue later.
src/wallet/walletdb.cpp
Outdated
There was a problem hiding this comment.
c50135edb6414fa5fd598f65e13eaa0e639573b9: maybe add comment: // These keys are deleted by the migratewallet RPC to ensure we don't add more stuff to this list without making sure they get migrated.
Note to other reviewers (plus some open questions), these types are captured by the backup as follows:
CRYPTED_KEY: whenwalletdb.cpploads them, it puts them intomapCryptedKeysviaLoadCryptedKey().MigrateToDescriptor()iterates over that map.CSCRIPT: whenwalletdb.cpploads them, it puts them intomapScriptsviaLoadCScript().MigrateToDescriptor()iterates over any multisig scripts in that map- TODO: I'm unclear what happens to
mapScriptsentries that are not multisig (see comment on earlier commit) - caveat: old records with
redeemScript.size() > MAX_SCRIPT_ELEMENT_SIZEare lost in the upgrade (they're in the backup)
DEFAULTKEY: this is only used to detect corruption, and is not migrated, newer wallets don't have this keyHDCHAIN:m_hd_chainwhich we migrateKEYMETA: ends up inmapKeyMetadataviaLoadKeyMetadata, which we migrate (at least for every key we have).KEY: whenwalletdb.cpploads them, it puts them intomapKeysviaLoadKey().MigrateToDescriptor()iterates over that map.OLD_KEY: these can only be loaded with v0.18 and older. They are skipped during wallet load, so they are lost in the migration.POOL: these are ignored, we regenerate the keypool using descriptorsWATCHMETA: put inm_script_metadatabyLoadScriptMetadata, and indexed by theirCScriptID, TODO: same question as withCSCRIPTWATCHSput inmapWatchKeysviaLoadWatchOnly, and its keys are used to monitor PKH, etc viaImplicitlyLearnRelatedKeyScripts. We don't iterate overmapWatchKeysdirectly, so I'm a bit confused how these are migrated.
|
|
||
| std::vector<std::vector<unsigned char>> sols; | ||
| TxoutType type = Solver(script, sols); | ||
| if (type == TxoutType::MULTISIG) { |
There was a problem hiding this comment.
35f428f What happens to mapScripts entries that are not MULTISIG?
There was a problem hiding this comment.
They will be part of spks and added to the watchonly wallet. It is not possible to import other scripts without also watching them (and their P2(W)SH). Multisigs are special because we have addmultisigaddress which add the multisig but does not watch them. All of the import rpcs will watch.
src/wallet/scriptpubkeyman.cpp
Outdated
There was a problem hiding this comment.
c50135edb6414fa5fd598f65e13eaa0e639573b9 : DeleteLegacyRecords (since we keep plenty of other stuff)
There was a problem hiding this comment.
We delete all of the LegacyScriptPubKeyMan records (which this function is a member of). There are no remaining LegacySPKM records after this.
Sjors
left a comment
Sjors
left a comment
There was a problem hiding this comment.
Reviewed up to a6c0e2f844f589df4f2d19e38c2e350952e1b4bf. Mostly happy. I'm a bit confused about how legacy wallets deal with (unwatched) solvable scripts, so I'll review that aspect again once I wrap my head around it.
src/wallet/wallet.cpp
Outdated
There was a problem hiding this comment.
a6c0e2f844f589df4f2d19e38c2e350952e1b4bf: bit of an aesthetic point, but I don't think the MigrationData struct should have the CWallet pointers. Instead you could just pass watchonly_wallet and solvable_wallet into ApplyMigrationData. That also makes it more clear what the data is applied to.
There was a problem hiding this comment.
I don't think that's any clearer, and would like to keep the function signature small.
src/wallet/wallet.cpp
Outdated
There was a problem hiding this comment.
a6c0e2f844f589df4f2d19e38c2e350952e1b4bf: if this fails after the file deletion, can we try to move the backup back in place? If that fails too, have the RPC throw an error telling the user the name of the backup file and they need to copy / rename it.
Similarly the asserts inside MigrateToSQLite could return false instead, though in that case moving the backup file back might fail.
MigrateToSQLite could take a second argument bool& deleted so we know whether or not the backup file needs to be moved.
Or you could create a file with a different name, defer deleting all the way to the end, and then rename the new file to match the original name. But then the m_database hot swap stuff won't work.
There was a problem hiding this comment.
Since these are filesystem errors, I don't think restoring the backup would actually work. Furthermore, because m_database has been swapped out, and potentially doesn't have a backing database, I don't think it's safe to continue execution in that state as the CWallet would still exist, but nowhere to write data to.
src/wallet/wallet.cpp
Outdated
There was a problem hiding this comment.
a6c0e2f844f589df4f2d19e38c2e350952e1b4bf: maybe move the file creation to a common method, so that if we ever decide to use some new opts field, we don't forget to apply it here.
There was a problem hiding this comment.
Perhaps for a followup to do this everywhere we are making wallets.
src/wallet/wallet.cpp
Outdated
There was a problem hiding this comment.
a6c0e2f844f589df4f2d19e38c2e350952e1b4bf: I don't understand this comment.
Also, should we check db_status?
There was a problem hiding this comment.
No. it just contains more granular errors. There's no "success by with caveats" that would be relevant here.
src/wallet/wallet.cpp
Outdated
There was a problem hiding this comment.
a6c0e2f844f589df4f2d19e38c2e350952e1b4bf: maybe just explicitly check for a lock before calling MigrateToDescriptor, so we can allow that function to fail and return nothing in other situations.
There was a problem hiding this comment.
I don't understand what you're talking about.
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.
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.
There was a problem hiding this comment.
I don't think the message implies that the only reason it would return nullopt is because it is locked. However, it is likely that a failure would be due to being locked, hence the message. We do this in a couple of other places too where failure is often being locked, but could be some weird db condition that actually happened.
Refactors SetupDescSPKMs so that the DescSPKM loops are in their own function. This allows us to call it later during migration with a key that was already generated.
w0xlt
left a comment
w0xlt
left a comment
There was a problem hiding this comment.
ACK 3299574f5e27bdb6c12fffd541e7cbfd148ab883
|
I started the GUI, made some transactions and then called I was unable to do a clean shutdown with ctrl+c from the terminal (where I started QT from). A gentle I repeated the experiment calling from the GUI console. There was again a 9 minute gap after watch-only was created, and the console got stuck in Executing, except the GUI never times out. After migration the watch-only labels were missing. This also happened when I repeated the migration using the GUI console. I'll send you the wallet (maybe it's related to the stuff I added below).
Also, IIUC along with wrapped SegWit, multisig is the only I played around a bit with Interestingly, once we have Miniscript support, there could be cases where a (P2W)SH script goes from watch-only and non-solvable to spendable, assuming the migration script picks it up. |
maflcko
left a comment
maflcko
left a comment
There was a problem hiding this comment.
I think it is expected that you run into deadlocks, given that the thread sanitizer doesn't pass CI.
Also, the Windows compilation fails, see u8string suggestion.
test/functional/wallet_migration.py
Outdated
There was a problem hiding this comment.
fb5e0957e066bd0825947a399242242f1f1a2ae6: 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 |
|
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. |
src/wallet/wallet.cpp
Outdated
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 <[email protected]>
|
tACK 53e7ed0 I think this is good enough for an |
| 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)
17 participants
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.