wallet: Have the wallet store the key for automatically generated descriptors #26728

achow101 · 2022-12-19T22:31:05Z

The wallet currently does not know the master key that was used to generate the automatically generated descriptors. This makes it difficult to add new automatically generated descriptors when new ones are introduced. So instead of losing this information after the descriptors are created, have CWallet store it. The xpub will be stored in a new activehdkey field. The private key must be one of the keys that is used by the descriptors, and will be extracted upon loading.

As this is a new field, wallets will be automatically upgraded upon loading. This loading is backwards compatible and uses a new non-required flag WALLET_FLAG_GLOBAL_HD_KEY to signal that the upgrade completed. The upgrade will search for an xpub that is shared by pkh(), wpkh(), and sh(wpkh() descriptors with the derivation pattern that we use. For new wallets, the xpub will be set during descriptor creation rather than trying to reverse engineer it. The flag will be set for all wallets, regardless of whether such an xpub was found or can even exist, in order to indicate that the upgrade will not need to be run in the future.

This allows us to have a gethdkey command which is useful for those who need a simple way to get an xpub from a wallet. gethdkey can also take a boolean parameter to indicate whether it should also output the corresponding xprv.

Supercedes #23417

DrahtBot · 2022-12-19T22:31:08Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	RandyMcMillan, ryanofsky
Approach ACK	w0xlt
Stale ACK	furszy, Sjors, S3RK

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:

#29136 (wallet: addhdkey RPC to add just keys to wallets via new void(KEY) descriptor by achow101)
#29130 (wallet: Add createwalletdescriptor and gethdkeys RPCs for adding new automatically generated descriptors by achow101)
#29124 (wallet: Automatically repair corrupted metadata with doubled derivation path by achow101)
#29054 (wallet: reenable sethdseed for descriptor wallets by achow101)
#29016 (RPC: add new listmempooltransactions by niftynei)
#28724 (wallet: Cleanup accidental encryption keys in watchonly wallets by achow101)
#28574 (wallet: optimize migration process, batch db transactions by furszy)
#28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)
#28142 (wallet: Allow users to create a wallet that encrypts all database records by achow101)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#27286 (wallet: Keep track of the wallet's own transaction outputs in memory by achow101)
#26008 (wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets by achow101)
#25907 (wallet: rpc to add automatically generated descriptors by achow101)
#24748 (test/BIP324: functional tests for v2 P2P encryption by stratospher)
#22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)
#22341 (rpc: add path to gethdkey by Sjors)

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.

src/script/descriptor.cpp

src/key.cpp

src/wallet/walletdb.cpp

w0xlt

Approach ACK

S3RK

Thank you for taking your time to rework #23417. It looks like a great improvement
I left some high-level comments inline and will continue with more in depth review.

S3RK · 2023-01-18T08:47:57Z

src/wallet/wallet.h

+    //! The extended public key to be used for new automatically generated descriptors
+    CExtPubKey m_xpub GUARDED_BY(cs_wallet);
+    //! The previous and current extended keys that are used for automatically generated descriptors
+    std::map<CExtPubKey, CKey> m_hd_keys GUARDED_BY(cs_wallet);


This presupposes that we will and should allow changing active HD key for a given wallet. I'd like to better understand for what use-cases this is needed.

abac3e3 There's also the scenario where we generate a new HD key and mark the existing one inactive. We could at that point delete the old HD(C)KEY record, in which case m_hd_key could just hold one key. Not sure if that's much better.

This is exactly what I'm curious about. Should we allow to generate a new HD key for existing wallet? I'm trying to understand why someone would like to generate new HD key for existing wallet instead of just creating a new wallet?

Or should we make a rule 1 wallet = 1 HD key. Which is much simpler to understand and manage.

I'm not entirely sure of the reasoning, but it's been requested as required feature.

S3RK · 2023-01-18T08:50:32Z

src/wallet/walletdb.cpp

+            }
+
+            CExtKey extkey(it->second, key);
+            pwallet->AddHDKey(extkey);


Probably I don't follow, but it seems we don't need to add all descriptor keys as HD keys to the wallet

Duplicates are skipped (see 8b0af8a).

9a17347: this function can fail, and SetActiveHDKey should probably called last.

I meant, that we can write only the key for best_xpub. Why do we need to write all the candidates as well?

Sjors

Concept ACK. Here's my initial code review (b756f22).

Sjors · 2023-01-19T13:44:44Z

src/wallet/wallet.h

+    //! The extended public key to be used for new automatically generated descriptors
+    CExtPubKey m_xpub GUARDED_BY(cs_wallet);
+    //! The previous and current extended keys that are used for automatically generated descriptors
+    std::map<CExtPubKey, CKey> m_hd_keys GUARDED_BY(cs_wallet);


abac3e3 There's also the scenario where we generate a new HD key and mark the existing one inactive. We could at that point delete the old HD(C)KEY record, in which case m_hd_key could just hold one key. Not sure if that's much better.

src/wallet/walletutil.h

test/functional/wallet_backwards_compatibility.py

src/wallet/walletdb.cpp

src/script/descriptor.h

Sjors · 2023-01-19T17:50:11Z

src/wallet/walletdb.cpp

+            }
+
+            CExtKey extkey(it->second, key);
+            pwallet->AddHDKey(extkey);


9a17347: this function can fail, and SetActiveHDKey should probably called last.

src/wallet/walletdb.cpp

Sjors · 2023-01-19T17:59:24Z

src/wallet/walletdb.cpp

+        std::map<std::string, int> tmpl = {{"pkh(", 0}, {"sh(wpkh(", 0}, {"wpkh(", 0}};
+
+        // Find root xpubs used in pkh(), sh(wpkh()), and wpkh() descriptors
+        for (const auto& spkm : pwallet->GetAllScriptPubKeyMans()) {


9a17347: Why not limit ourselves to GetActiveScriptPubKeyMans? Not sure how useful it is to store older HD keys, and it does seem to add some complexity.

The user may have imported other keys for some active spkms, so we'd be unable to determine the master if we only looked at the active ones.

Sjors

Changes look good, but I still have to study 8f9c7b9 and 9ec6941 more. Thanks for the clarification via IRC.

Sjors · 2023-01-27T16:18:41Z

src/wallet/walletdb.cpp

+        std::map<CExtPubKey, std::pair<std::map<OutputType, int>, uint64_t>> descs_keys;
+        std::map<OutputType, int> tmpl = {{OutputType::LEGACY, 0}, {OutputType::P2SH_SEGWIT, 0}, {OutputType::BECH32, 0}, {OutputType::BECH32M, 0}};
+
+        // Find root xpubs used in pkh(), sh(wpkh()), and wpkh() descriptors


6bb3bee nit: and tr()

ryanofsky

re: #26728 (comment)
re: #26728 (review)

This is essentially what we do today, albeit with hd keys rather than hd seeds.

Right. It seems like we can add the features we want without making the wallet data model more complicated, so I think we should do that. If there are features that can't work under the current model, or would work worse under the current model, that would change things, but in all of the cases I've read about it actually looks like the features would work better and be less confusing in the current model compared to the new model.

I think it is useful to have a gethdkey method similar to the one added here that would return the hd key associated with various descriptors. But that could be implemented more simply without changing the database format, or adding a flag, or bringing in other complexity introduced in this PR.

What would the API for that look like?

Instead of returning an object with "xpub" and "xpriv" fields, it would return a list of objects with "xpub", "xpriv", and "descriptors" fields. The "descriptors" field would contain a list of descriptors using the hd key.

It would also be good for the method to accept an "active_only" option, and only return hd keys associated with active descriptors by default.

This would make it easy to get the hd key, and easy to understand how the hd key is referenced by descriptors. In the typical case it would only return 1 hd key, and in more complicated cases it would be easier to understand than the gethdmethod in this PR, because it would give you all the information about hd keys, instead of just choosing one hd key and not telling you which descriptors it is associated with.

Ultimately, the work in this PR is the product of different iterations of how to implement createwalletdescriptor. The main thing blocking that is the question of which key should be used in the new descriptors. I've tried a couple of different approaches, and they all have drawbacks in some form: [...]

Thanks for describing all those approaches. It clarifies a lot, and I agree with all the drawbacks you mentioned for them. The approach I would suggest is making the hd key an optional parameter to createwalletdescriptor, and if it is not specified, using the same hd key as other active descriptors. Unless I am misunderstanding something, in normal cases we would expect there to be exactly one hd key. But in cases where there is more than one key, we could choose the active descriptor for the default output type (like getnewaddress), or be more conservative and raise an exception asking for an hd key to be specified to disambiguate.

For cases where the hd key is specified to createwalletdescriptor, I think we would want it to be flexible and accept multiple formats, but accepting "xpub" value from gethdkey would be an obvious start. It could also accept the xpriv value, or even the seed keys or bytes. If it accepted private keys or seeds, we could treat it as a generalized alternative to sethdseed, which would be great because it would simplify usage, and let us deprecate the sethdseed method instead of keeping both methods.

The goal of this PR is to provide an answer for this question. Whenever there is more than one hd key, there is a UX problem in how the user is supposed to specify it. So this PR resolves that by picking only one key. Then there is the problem with upgrading. This PR solves that by being consistent with new wallets - new wallets have a master key that is automatically generated and used in all the automatically generated descriptors, so it tries to reverse engineer the hd key from the existing descriptors. Using the automatically generated key means that all wallets, both old and new, should behave consistently.

Yeah I appreciate the cleverness of the implementation, and the fact that you actually found a way to add a new field to the database that depends on existing fields, and still allow the wallet to be opened by older software, and magically get the new software to detect whether the existing fields changed to update the new field.

But unless I'm missing something (which is possible and maybe likely), I don't actually see how this magic improves the UX. It seems like it only returns less information to the user and makes wallet behavior more opaque in the use-cases I've seen so far.

does it make sense for getwalletinfo to start returning an hdseedid value?

We can't do that since we're dealing with hd keys rather than hd seeds. The seed is hashed in order to become a master key, and descriptor wallets always throw that away once the master key is computed as it is no longer needed. We wouldn't be able to compute this for old wallets.

Wow yeah, I didn't realize descriptor wallets actually generate the seed but then throw it away only keeping the master key. I guess there are pros and cons to that, but it's another topic and mostly unrelated.

src/wallet/walletdb.cpp

test/functional/wallet_backwards_compatibility.py

achow101 · 2023-12-21T03:56:24Z

Instead of returning an object with "xpub" and "xpriv" fields, it would return a list of objects with "xpub", "xpriv", and "descriptors" fields. The "descriptors" field would contain a list of descriptors using the hd key.

It would also be good for the method to accept an "active_only" option, and only return hd keys associated with active descriptors by default.

This would make it easy to get the hd key, and easy to understand how the hd key is referenced by descriptors. In the typical case it would only return 1 hd key, and in more complicated cases it would be easier to understand than the gethdmethod in this PR, because it would give you all the information about hd keys, instead of just choosing one hd key and not telling you which descriptors it is associated with.

The approach I would suggest is making the hd key an optional parameter to createwalletdescriptor, and if it is not specified, using the same hd key as other active descriptors. Unless I am misunderstanding something, in normal cases we would expect there to be exactly one hd key. But in cases where there is more than one key, we could choose the active descriptor for the default output type (like getnewaddress), or be more conservative and raise an exception asking for an hd key to be specified to disambiguate.

For cases where the hd key is specified to createwalletdescriptor, I think we would want it to be flexible and accept multiple formats, but accepting "xpub" value from gethdkey would be an obvious start. It could also accept the xpriv value, or even the seed keys or bytes. If it accepted private keys or seeds, we could treat it as a generalized alternative to sethdseed, which would be great because it would simplify usage, and let us deprecate the sethdseed method instead of keeping both methods.

Hmm. I suppose that could be a viable approach. I will experiment with it.

I know that @Sjors has some ideas for gethdkey/getxpub but I'm not sure if those are compatible with this proposed approach. Otherwise, it does seem like it would cover the use cases targeted by this PR and it's descendant.

S3RK · 2023-12-21T07:58:23Z

IIUC the approach proposed by @ryanofsky is to basically figure out the key on demand instead of storing it in the DB.

So this works for generating tr() descriptors for existing wallets, but it doesn't work that well for #24861. The desired flow is to have a wallet with HD key but without descriptors and then retrieve an xpub to construct multisig descriptor. How would it work if we don't have an HD key associated with the wallet?

In my opinion, approach proposed in this PR resolves a regression we had with transition to descriptor wallets. In legacy we actually had a proper HD key residing in a wallet. And this is the same thing we want to bring back to descriptors. 1 wallet = 1 HD key. Yes, the matter is complicated by the fact that users can add any other keys they wish, but the model and the default flow are actually pretty straightforward.

Sjors · 2023-12-21T09:15:49Z

@achow101 wrote:

I've tried a couple of different approaches, and they all have drawbacks in some form:

Generate a new hd key and use that for the descriptor.

Another drawback of that approach is that it requires a new backup, e.g. for anyone who just wants to add taproot support to a wallet that doesn't have it yet.

Something I still need to do, but waiting for this PR to land, is to enable adding a tr() to a external signer wallet. Hardware wallets would typically derive those from the same master seed, though from our end that ~~doesn't~~ shouldn't matter; external signer wallets are watch-only. Well, except that we identify the physical device by the fingerprint of its master key.

@S3RK wrote:

The desired flow is to have a wallet with HD key but without descriptors and then retrieve an xpub to construct multisig descriptor. How would it work if we don't have an HD key associated with the wallet?

Indeed. There's different proposals out there of how to derive keys for a multisig wallet, but I'm mainly thinking of two approaches:

Take an existing wallet with the usual descriptors, pick an unused derivation path (e.g. m/48'/..., based on BIP 48) and share that with the other participants. You then import the resulting multisig descriptor.
Create a fresh wallet with only a seed / master key, and no descriptors. Pick any derivation... (same as 1)

I prefer (2) because I think you shouldn't co-mingle multisig and single sig funds in the same wallet, for privacy reason and because it's potentially confusing. Multisig setups always require a new backup, so if we need to generate a fresh master key that's not a big deal to me.

Best reviewed with `git show -w`

…ster The test creates a wallet on master, downgrades and encrypts the wallet. Then, it tries to open it again on master.

ryanofsky · 2023-12-21T16:50:39Z

@Sjors and @S3RK, can you help me out by being specific about the flows you have in mind? Like in terms of what RPC calls users will make to do a particular operation? I'm trying to fill in the gaps myself, but in the cases I can think of, it seems like adding the concept of a sticky master key doesn't actually make things simpler, just more opaque and confusing.

I understand that if you were designing a new wallet model and API, you would both prefer an approach that required all active descriptors to have a single master key, and would take advantage of having that master key to simplify certain things.

But I see real drawbacks to usability and code complexity if we are going to try to pretend that the wallet only has an single active hd key while not actually enforcing this. I think you if compare the API implemented in this PR and its followups to the API proposed in #26728 (review) I think you would see the same problems and hopefully agree. To be specific:

The gethdkey method implemented in this PR currently picks one master key used by descriptors, and ignores others that may be present. The proposed gethdkey described in #26728 (review) would return any hd key that is actively being used to generate addresses in the wallet by default (and would return inactive hd keys with an option) and also list descriptors associated with hd keys. This should be better and less confusing in every case compared to the implementation in this PR which just returns a single hd key with no descriptor information, with the key chosen in a complicated way that is sticky and can't be controlled by the user, even if they import new descriptors and set them to be active.
The createwalletdescriptor method implemented in #25907 and sethdseed method added for descriptor wallets in #29054 have overlapping functionality and confusing limitations that would be avoided by dropping sethdseed and making createwalletdescriptor more flexible as described #26728 (review) .
IIUC, the importdescriptor extension from #23544 to allow importing descriptors with known xpubs seems like it would be more robust and less confusing if it just worked with any xpub in the wallet or any active xpub, instead of only working for the sticky master key and failing with an error about not having private keys which are actually present.
IIUC, the importdescriptor extension from #27351 would also seem to work more smoothly with the suggested approach, because there would be no need for a separate sethdseed call, and the descriptor and master key would show up together in gethdkey output.

For external signing and multisig, I could believe in theory that there are cases that may be more complicated by not having a sticky hd key. But I haven't seen examples of that yet, and don't think it should be hard to come up with them if they exist. Also I wouldn't want to make the wallet model more complicated prematurely if it seems like all the improvements we want to make are possible without this.

re: #26728 (comment)

it doesn't work that well for #24861. The desired flow is to have a wallet with HD key but without descriptors and then retrieve an xpub to construct multisig descriptor. How would it work if we don't have an HD key associated with the wallet?

This flow actually sounds more complicated than what I would expect. If you have a blank wallet, why would you want creating a multisig desriptor and creating the hd key to be two separate steps, and require a separate sethdseed call before whatever call is used to add the multisig descriptor? Wouldn't the flow be simpler if you just made one call to createwalletdescriptor or importdescriptor and it did both things? (EDIT: Actually rereading this, I can see why this wouldn't work because when you have multiple wallets you need to generate all the seeds first before you can generate the descriptor. I'm still not sure that requires all the complexity in this PR, but it does suggest why it is there.) In the case of a non-blank wallet, I also think not having a sticky hd key would be more transparent. Creating a multisig desciptor in that case could use the wallet's one hd key if there is one or require one to be specified if there are multiple.

In my opinion, approach proposed in this PR resolves a regression we had with transition to descriptor wallets. In legacy we actually had a proper HD key residing in a wallet.

Yes I can see the aspiration to do this, and I could imagine a different wallet model with a less flexible importdescriptor RPC that required all descriptor to have the same hd key, or required any descriptors with different hd keys to be inactive. But this is not the current wallet model we have, and it seems like at this point trying to bolt on the concept of a having one hd key when wallet doesn't actually enforce it just seems messy and possibly the worst of both worlds.

re: #26728 (comment)

Something I still need to do, but waiting for this PR to land, is to enable adding a tr() to a external signer wallet. Hardware wallets would typically derive those from the same master seed, though from our end that doesn't shouldn't matter; external signer wallets are watch-only. Well, except that we identify the physical device by the fingerprint of its master key.

I think I am probably missing the point and don't have enough context, but I don't see how the current PR helps with this. I don't see how the wallet having a sticky hd key would help working with an external signer or creating a tr() descriptor. Like in general I could see how the wallet storing more information could help it work with external signers, but I don't understand how storing this particular bit of information would help.

Indeed. There's different proposals out there of how to derive keys for a multisig wallet, but I'm mainly thinking of two approaches [...] I prefer (2)

I see, but that suggests this PR is good because the limitations it adds will force users to choose approach (2). So if you have a wallet with default descriptors you can't import a multisig using the same hd key, and if you have a wallet with a multisig descriptor you can't create default descriptors with the same hd key. But it seems like the approach in this PR would only be enforcing that selectively, and the resulting behavior would probably just be confusing. Wouldn't it be simpler and clearer to just have explicit checks when adding descriptors for the combinations of descriptors you don't want to allow? Again here I think it would be helpful to be more concrete and think about the specific RPC methods that would be used for these flows.

I definitely don't understand everything and there could be real reasons why the approach in this PR makes sense. Just so far I haven't seen what the benefits are, and the drawbacks seem unpleasant.

ryanofsky · 2023-12-21T18:40:07Z

re: #26728 (comment)

If you have a blank wallet, why would you want creating a multisig desriptor and creating the hd key to be two separate steps, and require a separate sethdseed call before whatever call is used to add the multisig descriptor? Wouldn't the flow be simpler if you just made one call to createwalletdescriptor or importdescriptor and it did both things? (EDIT: Actually rereading this, I can see why this wouldn't work because when you have multiple wallets you need to generate all the seeds first before you can generate the descriptor. I'm still not sure that requires all the complexity in this PR, but it does suggest why it is there.)

Replying to my own comment, since I had a realization after I posted it and made the edit above.

I think neither the approach in this PR nor the suggested approach in #26728 (review) will support multisig well by themselves. But I think this PR's approach of deriving a master key from existing (non-multisig) descriptors, storing it separately, and adding new wallet code in followup PRs to rely on it puts us in a worse starting place for providing a good multisig UX than the suggested approach. Mostly because of the complexity it adds, but also because it does not assign a specific purpose to the HD key, so it could encourages uses that "co-mingle multisig and single sig funds in the same wallet" like Sjors was trying to avoid.

I don't think the flow for how we support multisig has to be decided now, since it's beyond the scope of this PR. But I do want to spell out how I think things could work more simply without this PR. I think to support multisig and other more complicated cases well we will want to give the wallet the ability to store an HD master key not associated with any descriptor, and which can be used later to create complicated descriptors. But this could be supported with a new wallet field holding a CExtPubKey that is only set temporarily between creating the master key and creating a descriptor that uses it. It would need to be accompanied by a wallet flag preventing older wallets from using the wallet while the field exists, but as soon as the first descriptor is created using the master key, the flag and temporary field could be deleted, and the wallet would be backward compatible again and stay within our current data model.

This would allow a simpler flow for creating multisig wallets without adding a permanent burden and increase in complexity after they are created.

ryanofsky · 2023-12-21T21:36:09Z

achow101, S3RK, Sjors, and me talked about this on a call. I think the plan for now is that achow will make 2 independent PRs targeted at different use-cases:

First PR would add a gethdkey method like the one in this PR and a createwalletdescriptor method like the one in #25907 that should both make reusing existing hd keys easier. Specifically these methods should be useful for adding new types of descriptors to existing wallets and upgrading existing wallets. Unlike the current PRs, this new PR would be fully backwards compatible and not change the database format.
Second PR would implement a sethdseed implementation for descriptor wallets like #29054 or a similar method that generates standalone hd keys not (yet) associated with any descriptor. This would be useful for creating multisig descriptors. The new keys would need to be stored in a new database field, or possibly multiple fields, so this would change the database format, but in a simple way that should avoid upgrade/downgrade pitfalls encountered in previous iterations of this PR with DBKeys::ACTIVEHDKEY. If we are concerned with having more than one HD key per wallet, this could be conservative about when it allows creating standalone HD keys.

~~S3RK~~ achow made a wiki page https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Use-Cases where we can start to look at different use cases, and what RPC calls would look like under different flows and models. On the call S3RK made the point that a model where wallets have exactly one active hd key at a time would be more consistent with other wallet software and more naturally align with the BIPs. I think this is definitely true, and I think we should optimize wallet UX for the case where there is only one active hd key, but also not be broken or confusing in cases where there is more than one hd key. So far, I haven't seen specific cases where supporting more than one hd key seems to hurt the UX or implementation, so I'm still skeptical of the idea that there is something wrong or bad about our current wallet model, even if it is different. But I think things should become clearer as we add features, and I think the two PR's above will keep our options open.

Update: replacement for this PR which adds gethdkey and #25907 which adds createwalletdescriptor is #29130 which adds both methods. Replacement for #29054 which adds sethdseed for descriptor wallets is #29136 which introduces an addhdkey method.

ryanofsky · 2023-12-22T14:01:12Z

re: #26728 (comment)

The new keys would need to be stored in a new database field

This is not actually true. Thinking about this more, I wonder if a more natural way of representing standalone keys in the wallet is just to treat them as a new descriptor type, like void(KEY), where the void descriptor would hold the key, but instead of expanding to a scriptPubKey or set of scriptPubKeys like other descriptors it would expand to the empty set.

We could treat this as an internal implementation detail, just using void(xpub....) as a way for the wallet to reference unused hd keys. But there could be benefits to using the type externally. For example, if we included these in listdescriptors output, users could continue using listdescriptors as a form of wallet backup, or way to see all the wallet keys a wallet contains, without having to call a new RPC method like gethdkey.

But I think the main advantage of this approach would just be fitting better into the existing database format, keeping it clean and easy to reason about, and also having more backwards compatibility.

The idea of naming the descriptor void(KEY) just comes the void operator in javascript, and void type in c/c++/java
which consume values without returning returning or signify functions that don't return values.

Sjors · 2023-12-22T14:32:30Z

@ryanofsky interesting idea, I especially like the backup implication, because descriptors can be used for paper backups.

ryanofsky · 2023-12-22T16:31:05Z

Thanks, I was also trying to think of names for these other than void descriptors. For example maybe they could be called data descriptors and written like data(xpub...) to convey the idea that these descriptors are just used to hold data and don't produce scriptpubkeys.

achow101 · 2023-12-22T16:49:44Z

This is not actually true. Thinking about this more, I wonder if a more natural way of representing standalone keys in the wallet is just to treat them as a new descriptor type, like void(KEY), where the void descriptor would hold the key, but instead of expanding to a scriptPubKey or set of scriptPubKeys like other descriptors it would expand to the empty set.

Nice idea, I think that could work. These could be imported with importdescriptors. If the user wants their wallet to generate a key automatically though, we'll still need a sethdseed equivalent (probably name it something else since it's not the same as setting a hd seed).

achow101 · 2024-01-06T00:05:58Z

Superseded by #29130

I don't think there's a reason to come back to this design, but the branch won't be deleted so we can reopen if desired.

…Cs for adding new automatically generated descriptors 746b6d8 test: Add test for createwalletdescriptor (Ava Chow) 2402b63 wallet: Test upgrade of pre-taproot wallet to have tr() descriptors (Ava Chow) 460ae1b wallet, rpc: Add createwalletdescriptor RPC (Ava Chow) 8e1a475 wallet: Be able to retrieve single key from descriptors (Ava Chow) 85b1fb1 wallet: Add GetActiveHDPubKeys to retrieve xpubs from active descriptors (Ava Chow) 73926f2 wallet, descspkm: Refactor wallet descriptor generation to standalone func (Andrew Chow) 54e74f4 wallet: Refactor function for single DescSPKM setup (Andrew Chow) 3b09d0e tests: Test for gethdkeys (Ava Chow) 5febe28 wallet, rpc: Add gethdkeys RPC (Ava Chow) 66632e5 wallet: Add IsActiveScriptPubKeyMan (Ava Chow) fa6a259 desc spkm: Add functions to retrieve specific private keys (Ava Chow) fe67841 descriptor: Be able to get the pubkeys involved in a descriptor (Ava Chow) ef67458 key: Add constructor for CExtKey that takes CExtPubKey and CKey (Ava Chow) Pull request description: This PR adds a `createwalletdescriptor` RPC which allows users to add new automatically generated descriptors to their wallet, e.g. to upgrade a 0.21.x wallet to contain a taproot descriptor. This RPC takes 3 arguments: the output type to create a descriptor for, whether the descriptor will be internal or external, and the HD key to use if the user wishes to use a specific key. The HD key is an optional parameter. If it is not specified, the wallet will use the key shared by the active descriptors, if they are all single key. For most users in the expected upgrade scenario, this should be sufficient. In more advanced cases, the user must specify the HD key to use. Currently, specified HD keys must already exist in the wallet. To make it easier for the user to know, `gethdkeys` is also added to list out the HD keys in use by all of the descriptors in the wallet. This will include all HD keys, whether we have the private key, for it, which descriptors use it and their activeness, and optionally the extended private key. In this way, users with more complex wallets will be still be able to get HD keys from their wallet for use in other scenarios, and if they want to use `createwalletdescriptor`, they can easily get the keys that they can specify to it. See also #26728 (comment) ACKs for top commit: Sjors: re-utACK 746b6d8 furszy: ACK 746b6d8 ryanofsky: Code review ACK 746b6d8, and this looks ready to merge. There were various suggested changes since last review where main change seems to be switching `gethdkeys` output to use normalized descriptors (removing hardened path components). Tree-SHA512: f2849101e6fbf1f59cb031eaaaee97af5b1ae92aaab54c5716940d210f08ab4fc952df2725b636596cd5747b8f5beb1a7a533425bc10d09da02659473516fbda

DrahtBot added the Wallet label Dec 19, 2022

achow101 marked this pull request as ready for review December 19, 2022 22:31

achow101 requested a review from S3RK December 19, 2022 22:31

This was referenced Dec 19, 2022

wallet, spkm: Move key management from DescriptorScriptPubKeyMan to wallet level KeyManager #23417

Closed

wallet: rpc to add automatically generated descriptors #25907

Closed

achow101 force-pushed the wallet-knows-master-key branch from 27a4176 to 4b9ac3e Compare December 19, 2022 22:41

This was referenced Dec 20, 2022

wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets #26008

Merged

wallet: Include a signature with encrypted keys to mitigate a wallet scam #25766

Closed

aureleoules reviewed Dec 20, 2022

View reviewed changes

src/script/descriptor.cpp Outdated Show resolved Hide resolved

src/key.cpp Outdated Show resolved Hide resolved

src/wallet/walletdb.cpp Outdated Show resolved Hide resolved

achow101 force-pushed the wallet-knows-master-key branch from 4b9ac3e to b756f22 Compare December 20, 2022 19:35

DrahtBot mentioned this pull request Jan 7, 2023

[Draft / POC] Silent Payments #24897

Closed

w0xlt reviewed Jan 10, 2023

View reviewed changes

This was referenced Jan 10, 2023

wallet: Use defined purposes instead of inlining #26858

Closed

BIP324: Enable v2 P2P encrypted transport #24545

Closed

S3RK reviewed Jan 18, 2023

View reviewed changes

Sjors reviewed Jan 19, 2023

View reviewed changes

DrahtBot mentioned this pull request Jan 21, 2023

refactor: Use util::Result class for wallet loading #25722

Draft

achow101 force-pushed the wallet-knows-master-key branch from b756f22 to e1f97a3 Compare January 24, 2023 22:02

DrahtBot mentioned this pull request Jan 25, 2023

wallet: group independent db writes on single batched db transaction #25297

Closed

achow101 force-pushed the wallet-knows-master-key branch from e1f97a3 to 5cd42d2 Compare January 25, 2023 17:37

DrahtBot mentioned this pull request Jan 26, 2023

wallet: improve rescan performance 640% #26951

Closed

achow101 force-pushed the wallet-knows-master-key branch from 5cd42d2 to 0c4b2bc Compare January 26, 2023 19:13

Sjors reviewed Jan 27, 2023

View reviewed changes

DrahtBot removed the CI failed label Dec 21, 2023

DrahtBot mentioned this pull request Dec 21, 2023

test: Test that migration automatically repairs corrupted metadata with doubled derivation path #29124

Merged

ryanofsky reviewed Dec 21, 2023

View reviewed changes

src/wallet/walletdb.cpp Outdated Show resolved Hide resolved

test/functional/wallet_backwards_compatibility.py Show resolved Hide resolved

achow101 force-pushed the wallet-knows-master-key branch from 98b6fad to ea6a87a Compare December 21, 2023 03:45

achow101 and others added 8 commits December 21, 2023 11:29

wallet: Automatically upgrade a wallet to have global hd key

954bfa6

tests: Test for gethdkey

31b43b6

test: Test automatic upgrade of descriptor wallets to have hd key

09849f9

wallet: Set global hd key for migrated wallets

5a5b351

test: Also do backwards compat test with encrypted wallets

fccde79

Best reviewed with `git show -w`

walletdb: Check that unencrypted and crypted keys are mutually exclusive

d2ecd4f

test: Check that no unencrypted records persist after encrypting

8a54bfe

test: add coverage for re-opening a downgraded encrypted wallet on ma…

d1caefc

…ster The test creates a wallet on master, downgrades and encrypts the wallet. Then, it tries to open it again on master.

achow101 force-pushed the wallet-knows-master-key branch from ea6a87a to d1caefc Compare December 21, 2023 16:29

achow101 mentioned this pull request Dec 22, 2023

wallet: Add createwalletdescriptor and gethdkeys RPCs for adding new automatically generated descriptors #29130

Merged

DrahtBot mentioned this pull request Dec 22, 2023

wallet: Cleanup accidental encryption keys in watchonly wallets #28724

Merged

achow101 mentioned this pull request Dec 22, 2023

wallet: addhdkey RPC to add just keys to wallets via new unused(KEY) descriptor #29136

Open

achow101 closed this Jan 6, 2024

bitcoin locked and limited conversation to collaborators Jan 9, 2025

wallet: Have the wallet store the key for automatically generated descriptors #26728

wallet: Have the wallet store the key for automatically generated descriptors #26728

Uh oh!

Conversation

achow101 commented Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

S3RK left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors Jan 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

achow101 commented Dec 21, 2023

Uh oh!

S3RK commented Dec 21, 2023

Uh oh!

Sjors commented Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Dec 21, 2023

achow101 commented Dec 19, 2022 •

edited

Loading

DrahtBot commented Dec 19, 2022 •

edited

Loading

Sjors Jan 27, 2023 •

edited

Loading

ryanofsky left a comment •

edited

Loading

Sjors commented Dec 21, 2023 •

edited

Loading

ryanofsky commented Dec 21, 2023 •

edited

Loading

ryanofsky commented Dec 21, 2023 •

edited

Loading