Migrate legacy wallets to descriptor wallets (wallet , rpc/rest/zmq )
Mar 9, 2022
https://github.com/bitcoin/bitcoin/pull/19602
Host:
achow101
PR author: achow101
The PR branch HEAD was df1dca43c277704d3959253037cba651ba2c0b46 at the time of this review club meeting.
Notes
PR #17261 introduced the ScriptPubKeyMan
interface for classes for managing keys and scripts. All of the key and script management
code for the Legacy wallet was moved into a LegacyScriptPubKeyMan.
PR #16528 introduced Descriptor wallets, an
entirely new type of wallet which uses output script descriptors
to manage keys and scripts in a wallet. This was implemented by introducing a
DescriptorScriptPubKeyMan.
The old type of wallets are referred to as Legacy wallets and they are slowly being deprecated
and removed. However there needs to be a way to turn a Legacy wallet into a Descriptor wallet
so that users do not find themselves unable to use their wallet.
In addition to changing how keys and scripts are tracked, Descriptor wallets also redefine the
concept (and implementation) of IsMine. Instead of allowing both private keys and watch-only addresses in a single
wallet, Descriptor wallets can either always have private keys, or never have private keys.
PR #19602 implements an RPC which migrates a Legacy wallet into a descriptor wallet by computing
descriptors for everything the legacy wallet would have considered to belong to it. These
descriptors are subsequently added to a newly created descriptor wallet with the same name as
the original Legacy wallet.
Legacy wallets determine whether a script belongs to it by checking to see if it would be able
to sign for the script, or the script is explicitly being watched. Descriptor wallets determine
this by comparing the script to the set of scripts computed from the stored descriptors.
This PR is a step in the roadmap for the
eventual removal of the Legacy wallet and the Berkeley DB dependency.
Questions
Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK ?
Try creating a Legacy wallet and then migrating it using migratewallet. How can you tell if the migration was successful?
A Legacy wallet can be created with bitcoin-cli -named createwallet wallet_name=<name> descriptors=false
In your own words, briefly describe the migration process.
What kind of scripts will a Legacy wallet consider to belong to it for a single key
(Hint: Look at LegacyScriptPubKeyMan::IsMine
Under what circumstances will a Legacy wallet watch for a multisig?
Why is LegacyScriptPubKeyMan::GetAllScriptPubKeys() needed
(Hint: Consider how DescriptorScriptPubKeyMan::IsMine
Why isn’t the HD seed ignored when looking at all of the keys in a Legacy wallet?
What happens to watch-only addresses, scripts, and pubkeys that were in the Legacy wallet?
What happens if the migration fails for some reason? How does the user recover their wallet?
Meeting Log
1 17:00 <achow101> #startmeeting
3 17:01 <achow101> Welcome to PR review club. Today we're looking at #19602 wallet: Migrate legacy wallets to descriptor wallets
6 17:01 <effexzi> Hey every1
7 17:02 <achow101> did everyone get a chance to review the PR and the notes? y/n
8 17:02 <svav> Read the notes
9 17:02 <michaelfolkson> hi
11 17:03 <hernanmarino> hi
13 17:03 <B_1o1> y/y, basic test of new wallet migration
14 17:03 <Kaizen_Kintsugi_> hi
15 17:03 <Kaizen_Kintsugi_> n, just gonna lurk
16 17:03 <michaelfolkson> y
17 17:03 <glozow> read notes, still reviewing
18 17:04 <hernanmarino> read notes only
19 17:04 <michaelfolkson> B_1o1: You generated a legacy-bdb wallet, sent funds to it and then used the migratewallet RPC?
20 17:05 <svav> Can someone describe the nature of the Berkeley DB dependency?
21 17:05 <svav> *briefly*Â XD
22 17:05 <B_1o1> michaelfolkson: just generated the legacy and migrated, didn't sent funds
23 17:06 <glozow> it's an evil, evil dependency that we must dispel
24 17:06 <achow101> svav: It is used as the database system for the legacy wallet.
25 17:06 <achow101> it's also unmaintained
26 17:06 <brunoerg> did I miss it?
27 17:06 <svav> So what would it be replaced by?
28 17:06 <achow101> sqlite
30 17:07 <achow101> did anyone try to migrate a legacy wallet?
32 17:08 <michaelfolkson> B_1o1: Cool. I thought to test the migration you'd need to have some funds (testnet/signet maybe) at an address. Otherwise you are migrating an empty wallet to a different empty wallet
33 17:08 <B_1o1> yeah I migh
34 17:08 <B_1o1> might try later
35 17:09 <glozow> or use regtest and generate to an address owned by the wallet
36 17:09 <michaelfolkson> Is that right achow101? Is there value in testing migration without any funds in that wallet?
37 17:10 <achow101> michaelfolkson: certainly. the migration deals with keys and scripts, not really transactions. Even without funds you can check that the keys and scripts were migrated correctly
38 17:11 <michaelfolkson> But keys with no value attributed to them? They were just previously generated but no Bitcoin sent to them?
39 17:11 <achow101> we must still watch for funds sent to them all the same
40 17:11 <hernanmarino> you might receive funds in the future, i guess
41 17:11 <michaelfolkson> Ok cool
42 17:12 <michaelfolkson> B_1o1: You should definitely comment on the PR with what you tested then :)
43 17:12 <B_1o1> michaelfolkson: +1
44 17:13 <achow101> could someone briefly describe the migration process?
45 17:13 <glozow> Basically pack everything into a `MigrationData` struct - keys, keyids, hd seeds, key origins, scripts, etc
46 17:14 <svav> Â descriptors are computed for everything the legacy wallet would have considered to belong to it
47 17:14 <svav> These descriptors are subsequently added to a newly created descriptor wallet with the same name as the original Legacy wallet
48 17:15 <glozow> and then create a descriptor wallet, generate descriptors using all this stuff
49 17:16 <glozow> also make a sqlite db and write everything
50 17:16 <achow101> svav: indeed!
51 17:16 <achow101> how do we compute everything the legacy wallet would have considered to belong to it?
52 17:17 <Kaizen_Kintsugi_> GetLegacyScriptPubKeyMan?
53 17:18 <achow101> Kaizen_Kintsugi_: not quite. That just retrieves the ScriptPubKeyMan object for all the legacy wallet logic
54 17:18 <B_1o1> if it's able to sign for the script or if the Script is being watched?
55 17:19 <Kaizen_Kintsugi_> Hmmm m_database->makeBatch? this seems to scoop up all the records in the database
56 17:19 <larryruane> glozow: "and then write everything" ... If the node crashes at a random time during the migration, are we guaranteed to be okay? Start up the node again and it finishes the migration?
57 17:19 <glozow> is it everything that isn't ISMINE_NO?
58 17:19 <achow101> B_1o1: that's part of it
59 17:20 <achow101> glozow: and how do we determine that?
60 17:21 <Kaizen_Kintsugi_> ISMINE_No determines if it is a watchonly?
61 17:22 <glozow> should i be looking at `IsMineInner()` ?
63 17:23 <achow101> Kaizen_Kintsugi_: ISMINE_NO means that the script does not belong to the wallet. The trick is finding the finite set of scripts that are not ISMINE_NO, as there is an inifinite number of scripts that are.
64 17:24 <glozow> aha. we look at the spk, and depending on the type, see if we have the key/script for it in our `keystore`
65 17:24 <Kaizen_Kintsugi_> good to know
66 17:24 <achow101> glozow: right! and what about multisigs?
67 17:24 <glozow> we need to own all of the keys involved
69 17:26 <achow101> indeed. so how do we go from IsMineInner to a set of scriptPubKeys?
70 17:26 <achow101> or rather a set of descriptors
71 17:28 <Kaizen_Kintsugi_> Is it in APplyMigrationData?
73 17:28 <Kaizen_Kintsugi_> MigrateToDescriptor?
74 17:29 <achow101> it's in GetAllScriptPubKeys
75 17:29 <achow101> sorry, GetScriptPubKeys
76 17:30 <michaelfolkson> Ha was gonna say, that isn't in scriptpubkeyman.cpp
77 17:32 <svav> Get the DescriptScriptPubKeyMans that have the same scriptPubKeys as this LegacyScriptPubKeyMan ??
78 17:33 <glozow> ok so here we're going through `mapKeys`, `mapCryptedKeys`, and `mapScripts`, and adding them to spks if `IsMine()`
79 17:34 <glozow> er - we only call `IsMine()` for the scripts
80 17:35 <glozow> and then in `MigrateToDescriptor()`, after we gather this set of spks, we create `desc_spkms` etc
81 17:36 <achow101> so a related question is why we have to do this at all? what is the difference between LegacyScriptPubKeyMan::IsMine and DescriptorScriptPubKeyMan::IsMine?
82 17:37 <Kaizen_Kintsugi_> DescriptorScripts seem like they are only spendable?
83 17:37 <glozow> `DescriptorScriptPubKeyMan` doesn't include watchonly 🤔
84 17:38 <Kaizen_Kintsugi_> +1 glozow
85 17:39 <glozow> so the watchonly stuff is put in a different `CWallet`?
86 17:39 <ls55> I think `DescriptorScriptPubKeyMan` also doesn't include `FillableSigningProvider::mapKeys`.
87 17:39 <glozow> std::shared_ptr<CWallet> watchonly_wallet{nullptr};
88 17:39 <glozow> std::shared_ptr<CWallet> solvable_wallet{nullptr};
89 17:39 <achow101> Kaizen_Kintsugi_: kind of, it depends on the wallet type, but Descriptor wallets get rid of the concept of mixed watchonly and spendable. Of course we have to deal with this somehow during migration
90 17:40 <svav> Descriptor wallets can either have private keys or be watch only, not both ??
91 17:40 <achow101> glozow: right
92 17:40 <achow101> svav: yes
93 17:40 <michaelfolkson> Why (get rid of this concept)?
94 17:40 <Kaizen_Kintsugi_> single responsibility pricipal?
95 17:40 <michaelfolkson> Just cleaner? It isn't descriptor related...
96 17:41 <glozow> definitely think this is simpler. why were watchonly and owned mixed together before?
97 17:41 <achow101> michaelfolkson: it really simplifies IsMine. Also separation of funds/duties/etc.
98 17:41 <Kaizen_Kintsugi_> watchonly wallets wouldn't need to store scripts?
99 17:41 <achow101> glozow: because it used to not be possible to have different wallet files for different purposes
100 17:42 <achow101> multiwallet is relatively recent
101 17:43 <Kaizen_Kintsugi_> This is to help organizational purposes I assume?
103 17:44 <glozow> are there other multiple wallets to separate beyond watchonly and owned?
104 17:45 <ls55> External Signer ?
105 17:45 <achow101> glozow: yes. you pointed out watchonly_wallet and solvable_wallet, but solvable_wallet is not the same as the spendable wallet. it's watchonly, but also slightly different
106 17:46 <Kaizen_Kintsugi_> o rly? I thought solvable and spendable were sonoymous
107 17:46 <Kaizen_Kintsugi_> I guess that has to do with multisig? where I would have 1 of N keys? I can partially solve but can't spend?
109 17:47 <achow101> Kaizen_Kintsugi_: solvable means that we would be able to sign if we knew the keys
110 17:47 <glozow> is spendable a subset of solvable?
111 17:47 <achow101> but the split between watchonly and solvable actually goes back to LegacyScriptPubKeyMan::IsMine
112 17:47 <achow101> there's actually a solvable, but not watchonly set of scripts
114 17:48 <achow101> could anyone describe how that might happen?
115 17:48 <Kaizen_Kintsugi_> missing script?
116 17:49 <achow101> Kaizen_Kintsugi_: missing the script would mean it is not solvable
117 17:50 <achow101> (this requires looking at SignStep in src/script/sign.cpp)
118 17:50 <michaelfolkson> glozow: That's my understanding. You can't spend if not solvable. But it can be solvable without you being able to spend
119 17:51 <ls55> `if scriptPubKey could not be completely satisfied.` ?
120 17:51 <glozow> multisig but don't know all pubkeys?
121 17:51 <glozow> wait is that a thing
122 17:52 <vnprc> does it have to do with standard script types?
124 17:52 <ls55> case TxoutType::NONSTANDARD:
125 17:52 <ls55> case TxoutType::NULL_DATA:
126 17:52 <ls55> case TxoutType::WITNESS_UNKNOWN:
128 17:52 <Murch> Yeah, achow101: is there a way for an output to be spendable but not solvable?
130 17:52 <achow101> It is not possible for a script to be spendable but not solvable
131 17:53 <achow101> glozow: I think that might be true
132 17:53 <michaelfolkson> glozow: As a general concept you do need to know the pubkeys of the multisig to spend from it
133 17:54 <achow101> we could have a multisig script, but not all the keys for it, and also not necessarily be watching the spk
134 17:54 <michaelfolkson> At least with CHECKMULTISIG, with CHECKSIGADD....same deal?!
135 17:54 <Kaizen_Kintsugi_> makes sense
136 17:55 <achow101> michaelfolkson: no, that's taproot so descriptors only
137 17:55 <michaelfolkson> Indeed, thanks
138 17:55 <glozow> okay yeah so if it was a 1 of 2 multisig script, you have a key and it's solvable, but you're not watching the script
139 17:55 <ls55> Isn't legacy wallet compatible with Taproot ? why ?
140 17:56 <michaelfolkson> Taproot is only descriptors (as Andrew said). Just design choice
141 17:56 <achow101> ls55: the legacy wallet does not support taproot. we decided not to implement taproot for it because it will be going away soon(tm)
142 17:56 <ls55> Ok. Thanks.
143 17:57 <achow101> related to weird IsMine things, can anyone say what we do with the key for the HD seed?
144 17:57 <ls55> Will `LegacyScriptPubKeyMan` be removed from codebase ?
145 17:57 <achow101> ls55: that's the plan
146 17:57 <svav> What is an HD seed?
147 17:57 <ls55> Very good. It simplifies the code.
148 17:58 <hernanmarino> achow101: it gets migrated ?
149 17:58 <Kaizen_Kintsugi_> seed of an HD wallet
150 17:58 <ls55> I think not
151 17:58 <hernanmarino> that was an affirmation not a question .
152 17:58 <ls55> `Note that we do not ignore the seeds themselves because they are considered IsMine!`
153 17:58 <Murch> svav: The main secret of a wallet using hierarchical deterministic derivation
154 17:58 <Murch> See BIP32
155 17:59 <achow101> ls55: right! what's the implication of that?
156 17:59 <svav> Thanks Murch:
157 17:59 <hernanmarino> it is a valid key
158 17:59 <ls55> A new seed is created in the new descriptor wallet ?
159 18:00 <achow101> hernanmarino: yes, and as a valid key, you could receive Bitcoin to it even though it's corresponding addresses would never be given out
160 18:00 <hernanmarino> exactly :)
161 18:01 <Murch> So, we generate one wallet from the seed and another from the descriptor that replaces the derived addresses of the seed?
162 18:01 <achow101> Murch: the same wallet with multiple descriptors
163 18:02 <achow101> we're at the top of the hour, so thank you all for coming and for reviewing the pr.
164 18:02 <achow101> #endmeeting