descriptors: Use xpub at last hardened step if possible

achow101 commented at 10:16 pm on February 16, 2020: member

When a descriptor contains a private key and has hardened derivation steps, derive the xpub at the last hardened step and convert the derivation into origin info. So the result is that a BIP32PubkeyProvider containing a xprv with hardened derivation steps and ending with unhardened derivation becomes a OriginPubkeyProvider with those hardened derivation steps as the origin info and a new BIP32PubkeyProvider containing the xpub and the unhardened derivation steps.

For example: wpkh(xprv..../0'/0'/*) becomes wpkh([d34db33f/0'/0']xpub.../*)

If the descriptor has only hardened steps, it is not modified.

This change allows descriptor wallets to derive from such descriptors without needing to be unlocked.

fanquake added the label Descriptors on Feb 16, 2020

DrahtBot commented at 3:53 am on February 17, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18204 (descriptors: improve descriptor cache and cache xpubs by achow101)
#16549 ([WIP] UI external signer support (e.g. hardware wallet) by Sjors)
#16546 ([WIP] External signer support - Wallet Box edition 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.

in src/script/descriptor.cpp:726 in c1068140ed outdated

721+            KeyOriginInfo last_hardened_info;
722+            CKeyID extpubkeyid = extpubkey.pubkey.GetID();
723+            std::copy(extpubkeyid.begin(), extpubkeyid.begin() + 4, last_hardened_info.fingerprint);
724+
725+            KeyPath::iterator it = path.end() - 1;
726+            for (; it >= path.begin(); it--) {

Sjors commented at 10:37 am on February 19, 2020:

Travis doesn’t like this: https://travis-ci.org/bitcoin/bitcoin/jobs/651243106#L4064

0Error: attempt to decrement a dereferenceable (start-of-sequence) iterator.

It sounds like a job for std::find_end with a binary predicate, since you’re looking for a hardened path element by an unhardened one.

achow101 commented at 11:06 pm on February 19, 2020:

I was able to use std::find_if for this.

Sjors commented at 10:38 am on February 19, 2020: member

Concept ACK

instagibbs commented at 1:56 pm on February 19, 2020: member

Is there a practical consideration for this change? It seems to make sense, just wondering if downstream applications are made easier by this or something.

achow101 commented at 5:22 pm on February 19, 2020: member

Is there a practical consideration for this change?

It would allow for descriptor wallets that import a descriptor with a private key and hardened steps to be able to derive scriptPubKeys without needing to be unlocked.

instagibbs commented at 5:26 pm on February 19, 2020: member

Adding that to OP would be OP

concept ACK!

achow101 force-pushed on Feb 19, 2020

in src/script/descriptor.cpp:779 in 0e54ba78ea outdated

733+
734+            last_hardened_info.path.insert(last_hardened_info.path.end(), path.begin(), last_hard_it);
735+
736+            // Only continue with this last hardened stuff if there are actually hardened steps in path
737+            if (!last_hardened_info.path.empty()) {
738+                // Derive that last hardened key

Sjors commented at 9:14 am on February 20, 2020:

nit: add helper function in key.h: bool DerivePath(CExtKey& out, KeyPath& path, bool neutered = false) const;

achow101 commented at 5:55 pm on February 21, 2020:

Done

in src/script/descriptor.cpp:749 in 0e54ba78ea outdated

744+                extpubkey = extkey.Neuter();
745+                out.keys.emplace(extpubkey.pubkey.GetID(), extkey.key);
746+
747+                // Shorten path to the unhardened stuff
748+                if (last_hardened_info.path.size() < path.size()) {
749+                    // At least on step is unhardened

Sjors commented at 9:45 am on February 20, 2020:

nit: “one”

achow101 commented at 5:56 pm on February 21, 2020:

Fixed

in src/script/descriptor.cpp:759 in 0e54ba78ea outdated

754+                    // All steps are hardened
755+                    path.clear();
756+                }
757+
758+                // Make a BIP32PubkeyProvider for the unhardened derivation
759+                extpubkey = extkey.Neuter();

Sjors commented at 9:49 am on February 20, 2020:

Duplicate line

achow101 commented at 5:56 pm on February 21, 2020:

Removed

Sjors commented at 10:18 am on February 20, 2020: member

0e54ba78ea7e7468ce5a185c4195b0dd58f3f82d looks good, expect the duplicate line.

That said, I feel a bit ambivalent about adding origin info to descriptors that don’t already have it. For example pkh(xprv9...C2U/0'/0'/0) becomes pkh([bd16bee5/0'/0']xpub6AVD...pQKh/0). Perhaps normalisation should only happen for descriptors with origin info. Also, an xpub encodes its depth, so if in the example the original xpriv depth wasn’t 0 it would be weird (or maybe not; depends on if we interpret the fingerprint as depth 0).

I personally haven’t seen real world descriptors without origin info yet, so I’m sure how they should be handled.

Also, what would be the next step to have InferDescriptor benefit from this change? It would be nice to have getaddressinfo in return an xpub, instead of individual public key, for BIP44/49/84 style watch-only (legacy) wallets.

achow101 commented at 4:25 pm on February 21, 2020: member

That said, I feel a bit ambivalent about adding origin info to descriptors that don’t already have it. For example pkh(xprv9...C2U/0'/0'/0) becomes pkh([bd16bee5/0'/0']xpub6AVD...pQKh/0). Perhaps normalisation should only happen for descriptors with origin info.

Then we wouldn’t achieve the primary goal of this PR. People could import xprvs without origin info that have some hardened derivation steps and we wouldn’t be able to derive the public keys from them.

Also, an xpub encodes its depth, so if in the example the original xpriv depth wasn’t 0 it would be weird (or maybe not; depends on if we interpret the fingerprint as depth 0).

The xpub at the end would still contain that depth information. Even without an origin info, an xprv could have a non-zero depth but that doesn’t bother us.

Also, what would be the next step to have InferDescriptor benefit from this change? It would be nice to have getaddressinfo in return an xpub, instead of individual public key, for BIP44/49/84 style watch-only (legacy) wallets.

I think that requires changes to SigningProvider.

achow101 force-pushed on Feb 21, 2020

Sjors commented at 6:42 pm on February 21, 2020: member

ACK b674297c7c491ea09cef171ed083533f23c9ffa9

I’ve never imported keys without origin info, but if you find it useful, I’m fine with supporting it.

Add DerivePath to CExtKey

Utility function to derive at a path from an CExtKey instead
of implementing this externally every time we want to derive at
a path.

136d8f826e

descriptors: Use xpub at last hardened step if possible f8274751f6

achow101 force-pushed on Feb 21, 2020

achow101 commented at 7:50 pm on February 21, 2020: member

Had to rebase

Sjors commented at 9:43 pm on February 21, 2020: member

Code review re-ACK f8274751f6eb7dbd2207ff4232131dfc0f9ff978 (rebased on #18034)

sipa commented at 5:53 pm on February 22, 2020: member

I discussed this with @achow101 IRL.

One alternative I’d like to see explored is instead of rewriting the descriptor itself, having an “xpub cache” added to the descriptor interface (and later the wallet itself), similar to the existing pubkey cache, but shared across indexes. This would have all the availability benefits of a change like this, but also speed up operations on descriptors that include several non-hardened steps in xpubs.

Sjors commented at 6:57 pm on February 22, 2020: member

That would take case of this TODO, right?

https://github.com/bitcoin/bitcoin/blob/ab9de435880c9d77e4137b65050591ef2d14f809/src/script/descriptor.cpp#L286-L291

Rewriting the descriptor has merit in any case; a descriptor like wpkh([d34db33f/xpub /0'/0'].../*) makes no sense, because you can’t actually derive any addresses without also knowing the xpriv. Whereas with wpkh([d34db33f/0'/0']xpub.../*) you can. Normalising makes them more straight-forward to share.

For BIP44/49/84 style descriptors (wpkh([d34db33f/84'/0'/0']xpub.../0/*)) it would indeed make sense to cache an xpub for the 0 after the visible xpub. But I think that can be hidden as an ephemeral implementation detail; I don’t think it needs to be serialised into the wallet payload. Unlike the individual key cache, the savings don’t scale with wallet size.

achow101 closed this on Feb 26, 2020

laanwj referenced this in commit 7f8176a1eb on Mar 13, 2020

sidhujag referenced this in commit 9ac84dd1a1 on Mar 14, 2020

instagibbs commented at 3:44 pm on April 8, 2020: member

something “like” this is probably nice to have for multisig setups. I’m looking into it for a dumpdescriptors RPC command for that type of setup

sidhujag referenced this in commit 2f115a38a8 on Nov 10, 2020

DrahtBot locked this on Feb 15, 2022

descriptors: Use xpub at last hardened step if possible #18163

Conflicts