Expose inner `DescriptorPublicKey` functionality by quad · Pull Request #570 · rust-bitcoin/rust-miniscript

quad · 2023-07-23T03:23:50Z

Hi friends, it's me again! 👋

In our use of miniscript, we find ourselves writing constantly writing this idiom:

let xpub = match dpub {
    DescriptorPublicKey::Single(_) => unimplemented!(),
    DescriptorPublicKey::XPub(xpub) => xpub,
}

I'd love to directly use the DescriptorXKey<bip32::ExtendedPubKey> type; but, its functionality is anemic when compared to all the useful functions on DescriptorPublicKey.

The closer I looked at DescriptorPublicKey, the more I realised it was mostly match blocks itself that delegated to different implementations based on the internal enum type. And, so, here's a PR that leans into that and reifies a trait (DescriptorInnerKey) that encapsulates all that behaviour and three types that implement it:

DescriptorSinglePublicKey (a type alias for SinglePub 🤒)
DescriptorExtendedPublicKey
DescriptorMultiExtendedPublicKey

This all said, I feel like this PR is still lacking. A few questions:

DescriptorPublicKey could implement the trait too; but, I suspect that be a breaking change? Moreover …
DescriptorInnerKey is a weird name, especially when contrasted against the already existing InnerXKey trait. I'm open to a better name!
DescriptorPublicKey::into_single_keys has been awkwardly left out of the extraction because it returns a vector of DescriptorPublicKeys. If I was going to do a breaking refactor, I'd probably move that method on to DescriptorMultiExtendedPublicKey itself. 🤷
What would going full breaking change look like if DescriptorPublicKey as a struct just disappeared … and instead there was only a DescriptorPublicKey trait?

quad · 2023-07-23T03:32:32Z

src/descriptor/key.rs

+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        maybe_fmt_master_id(f, &self.origin)?;
+        self.xkey.fmt(f)?;
+        fmt_derivation_path(f, &self.derivation_path)?;
+        match self.wildcard {
+            Wildcard::None => {}
+            Wildcard::Unhardened => write!(f, "/*")?,
+            Wildcard::Hardened => write!(f, "/*h")?,
+        }
+        Ok(())
+    }


Good candidate to extract to a fmt_xkey(origin, [paths]) function.

quad · 2023-07-23T03:37:04Z

src/descriptor/key.rs

+                    .ok_or(ConversionError::HardenedChild)?,
+            ),
+        };
+        let definite = DescriptorPublicKey::XPub(DescriptorXKey {


This is an awkward "upward" instantiation of the DescriptorPublicKey type.

An idea: DefiniteDescriptorKey::new could take a DescriptorInnerKey.

Maybe DescriptorInnerKey should have an Into<DescriptorPublicKey> bound?

Erm, that'll require ~~DescriptorInnerKey~~DescriptorKey to be made object-safe. That's beyond my ken.

I think it might be as simple as sticking a : Sized bound on the trait? What error do you get?

apoelstra · 2023-07-23T14:43:37Z

src/descriptor/key.rs

In e4d9af4:

Do you think we should add fmt::Display and std::str::FromStr bounds on this? How about Eq, Hash, etc.? (My vote is yes to all four, and maybe fmt::Debug too.)

cc @sanket1729 who may have opinions here.

I'm a big fan of that, since it was the lack of Display / FromStr that motivated this PR.

I added Clone + fmt::Debug + fmt::Display + Eq + FromStr + std::hash::Hash + Ord to match the existing derivations.

apoelstra · 2023-07-23T14:45:22Z

src/descriptor/key.rs


+pub trait DescriptorInnerKey {
+    /// The fingerprint of the master key associated with this key, `0x00000000` if none.
+    fn master_fingerprint(&self) -> bip32::Fingerprint;


In e4d9af4:

As a separate issue, I think we should return an option here and then upstream we should have a bip32::Fingerprint::from_opt function that would do the 0x00000000 thing. Though I'm unsure. Maybe this function is only ever used in contexts where the result needs to be serialized (PSBT, encoding xpubs) in which case users always want the existng behavior.

That's sure where I'm using it. 😅

apoelstra · 2023-07-23T14:52:09Z

src/descriptor/key.rs

In e4d9af4:

This can go in a separate PR, but like to have a type-level relationship between DescriptorPublicKey and DescriptorSecretKey. This would help me downstream in elements-miniscript where I want to define a "view key" for a confidential transaction descriptor, which has a private key in one slot and a public key in every other slot. I think it would also simplify a lot of usage of xprv descriptors, which currently require you maintain an xpub descriptor alongside a hashmap, though I haven't worked out the details.

One way we could do this is to add two associated types, like

type SecretKey: DescriptorInnerKey; type PublicKey: DescriptorInnerKey;

where almost always one of these would be Self.

I would also like first-class support for unspendable keys in taproot. I am imagining an UnspendableKey whose SecretKey type would be !, or something.

Anyway these are just half-baked ideas that don't belong in this PR. But I want to bring them up in case they could guide this API.

apoelstra · 2023-07-23T15:06:53Z

Overall 14799e2 looks good. It's a straightforward enough diff and I am confirming that all commits pass all tests (looking good so far).

DescriptorPublicKey could implement the trait too; but, I suspect that be a breaking change? Moreover …

I don't think it would be breaking, but I also don't know that we need to worry about breaking changes here. Especially if we can also figure out the "what is the right way to relate private and public keys" and "what is the right way to handle unspendable keys" API questions.

But yes, it did occur to me when reading the code that it might make sense to implement this on DescriptorPublicKey.

DescriptorInnerKey is a weird name, especially when contrasted against the already existing InnerXKey trait. I'm open to a better name!

Maybe just DescriptorKey? We could maybe have a nice API where users could have a Descriptor<Pk: MiniscriptKey> but then get a ton of extra functionality if they actually had a Descriptor<Pk: DescriptorKey>.

DescriptorPublicKey::into_single_keys has been awkwardly left out of the extraction because it returns a vector of DescriptorPublicKeys. If I was going to do a breaking refactor, I'd probably move that method on to DescriptorMultiExtendedPublicKey itself. shrug

Good to know. Let's revisit that if we decide to do a breaking change.

What would going full breaking change look like if DescriptorPublicKey as a struct just disappeared … and instead there was only a DescriptorPublicKey trait?

So, with Descriptor itself we actually went the opposite direction. We originally had a trait because we imagined that downstream users might define their own descriptor types (in particular we had CTV and Elements in mind). But then we realized that it almost never made sense to be generic over an arbitrary descriptor, and using a trait was extra noise. See #386. We also noticed that there actually isn't a lot of functionality that makes sense for all descriptor types, unless you want every method to return a Result.

But having said that, for keys we have continuously moved toward more trait usage, adding more traits and associated types etc.

…edPublicKey

…PublicKey

…xtendedPublicKey

quad · 2023-07-24T11:13:38Z

Maybe just DescriptorKey?

👍

But yes, it did occur to me when reading the code that it might make sense to implement this on DescriptorPublicKey.

I added a commit that does that. It's "breaking" insofar as consumers now need to import a trait. (e.g. see the change in src/descriptor/mod.rs)

sanket1729 · 2023-07-25T00:26:20Z

Hello, thanks for this PR.

Am I correct in summarizing that if we have a type XPubOnlyDesc = Descriptor<DescriptorExtendedPublicKey>, than it should satisfy your use-cases? Someone else also offered me the same feedback that most wallets today only use XPubs(without the multiX) and we should simplify our API to that end.

If this the case, we provide the same APIs for XPubOnlyDesc as we do Descriptor<DescriptorPubicKey>. Our code is mostly generic over MiniscriptKey, so there are only a few APIs that are specialized for DescriptorPublicKey.

We can then offer some granular types with some guidelines as

// lib.rs
pub type Descriptor = Descriptor<DescriptorPublicKey>; // Full support for all types of keys
pub type DescriptorExtendedKeysOnly = Descriptor<DescriptorExtendedPublicKey>; // Without single or multi-keys.

Tihs obviously implies that you cannot single keys in your descriptor. Curious what @apoelstra thinks about this.

Comment about the trait approach

I am not sure about adding a trait, 1) It requires users to import it and 2) we are not using it any bounds here. Are you planning it downstream? If so, can you share you want it to be used as a bound?
If it is just a way to bundle "similar looking" APIs together, I am not sure about it.

As Andrew mentioned in the #386, we found ourselves into a similar issue where we group things for DescriptorTrait that were almost similar, but not exactly the same. I think is also one of the cases where we are intentionally merging things. I think this is evident by the fact that we already have weird cases around multi, bip32 master fingerprint.

I do think the trait has value only if we go all the way so that Descriptor<DescriptorExtendedPublicKey> can be used directly. (Which would require implementing MiniscriptKey on DescriptorExtendedPublicKey). Otherwise, with respect to this PR as is, we can provide all the functionality without the trait. As in, SinglePub would have a method called master_fingerprint(), into_single_descriptors would only be for DescriptorMulti.. and so on for other methods that make sense for each type. The top level enum would have all the types with the correct Option, Result return types for respective APIs.

cc @tcharding worked on changing the previous long names that we had DescirptorSinglePublicKey to SinglePub, would like to get his input on naming here.

quad · 2023-07-25T02:54:23Z

Am I correct in summarizing that if we have a type XPubOnlyDesc = Descriptor<DescriptorExtendedPublicKey>, than it should satisfy your use-cases?

Maybe? Honestly, the type hierarchy is too complex for me to follow.

That said, there's a lot of useful functionality on DescriptorPublicKey. DescriptorPublicKey::from_str being paramount!

Someone else also offered me the same feedback that most wallets today only use XPubs(without the multiX) and we should simplify our API to that end.

I strongly agree with this sentiment.

quad · 2023-07-25T02:57:23Z

Are you planning it downstream? If so, can you share you want it to be used as a bound?

Our codebase has type DescriptorExtendedKey = DescriptorXKey<ExtendedPubKey> and almost entirely standardises on that type internally. We match / unwrap DescriptorPublicKey at the edges. I'd love to use DescriptorExtendedKey at our edges, but its too aenemic right now to justify it.

sanket1729 · 2023-07-27T21:57:00Z

Maybe? Honestly, the type hierarchy is too complex for me to follow.

If all functionality for DescriptorPublicKey is available in DescriptorExtendedKey, does that satisfy your use-cases?

tcharding · 2023-07-27T22:36:21Z

src/descriptor/key.rs

+type DescriptorSinglePublicKey = SinglePub;
+type DescriptorExtendedPublicKey = DescriptorXKey<bip32::ExtendedPubKey>;
+type DescriptorMultiExtendedPublicKey = DescriptorMultiXKey<bip32::ExtendedPubKey>;


IMO we should not start putting Extended back into the identifiers of keys. The only other place that appears is in bitcoin::bip32 which is not a module to copy :)

tcharding · 2023-07-27T22:37:09Z

cc @tcharding worked on changing the previous long names that we had DescirptorSinglePublicKey to SinglePub, would like to get his input on naming here.

In general I prefer shorter names if there is no ambiguity or loss of clarity. Thanks.

sanket1729 · 2023-07-31T09:10:54Z

@quad. I was suggesting something like: #586 without all the forced mixing of public traits involved here.

quad force-pushed the ssr/20230723/better-inner-dpub-types branch from a1dbaf5 to dcfcc25 Compare July 23, 2023 03:30

quad commented Jul 23, 2023

View reviewed changes

quad force-pushed the ssr/20230723/better-inner-dpub-types branch 9 times, most recently from a6f0354 to 14799e2 Compare July 23, 2023 05:25

apoelstra reviewed Jul 23, 2023

View reviewed changes

quad force-pushed the ssr/20230723/better-inner-dpub-types branch 2 times, most recently from 397d435 to ef617d5 Compare July 24, 2023 11:00

Scott Robinson added 6 commits July 24, 2023 21:11

DescriptorKey trait

ffe62ef

Extract specialised DescriptorKey functionality into DescriptorExtend…

bfa946e

…edPublicKey

Extract specialised DescriptorKey functionality into DescriptorSingle…

c430f70

…PublicKey

Extract specialised DescriptorKey functionality into DescriptorMultiE…

687fd4e

…xtendedPublicKey

Simplify DescriptorPublicKey::from_str

9c55ad3

Make DescriptorPublicKey a DescriptorKey

b18ba59

quad force-pushed the ssr/20230723/better-inner-dpub-types branch from ef617d5 to b18ba59 Compare July 24, 2023 11:11

tcharding reviewed Jul 27, 2023

View reviewed changes

Conversation

quad commented Jul 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quad Jul 23, 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!

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!

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Jul 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

quad commented Jul 24, 2023

Uh oh!

sanket1729 commented Jul 25, 2023

Comment about the trait approach

Uh oh!

quad commented Jul 25, 2023

Uh oh!

quad commented Jul 25, 2023

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding commented Jul 27, 2023

Uh oh!

sanket1729 commented Jul 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

quad commented Jul 23, 2023 •

edited

Loading

quad Jul 23, 2023 •

edited

Loading

apoelstra commented Jul 23, 2023 •

edited

Loading

sanket1729 commented Jul 27, 2023 •

edited

Loading