Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose inner DescriptorPublicKey functionality #570

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

quad
Copy link
Contributor

@quad quad commented Jul 23, 2023

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:

  1. DescriptorSinglePublicKey (a type alias for SinglePub 🤒)
  2. DescriptorExtendedPublicKey
  3. DescriptorMultiExtendedPublicKey

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

  1. DescriptorPublicKey could implement the trait too; but, I suspect that be a breaking change? Moreover …
  2. DescriptorInnerKey is a weird name, especially when contrasted against the already existing InnerXKey trait. I'm open to a better name!
  3. 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. 🤷
  4. What would going full breaking change look like if DescriptorPublicKey as a struct just disappeared … and instead there was only a DescriptorPublicKey trait?

@quad quad force-pushed the ssr/20230723/better-inner-dpub-types branch from a1dbaf5 to dcfcc25 Compare July 23, 2023 03:30
Comment on lines +302 to +311
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(())
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.ok_or(ConversionError::HardenedChild)?,
),
};
let definite = DescriptorPublicKey::XPub(DescriptorXKey {
Copy link
Contributor Author

@quad quad Jul 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe DescriptorInnerKey should have an Into<DescriptorPublicKey> bound?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧠 💡

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@quad 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
@@ -525,6 +525,53 @@ impl error::Error for ConversionError {
}
}

pub trait DescriptorInnerKey {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -525,6 +525,53 @@ impl error::Error for ConversionError {
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -525,6 +525,53 @@ impl error::Error for ConversionError {
}
}

pub trait DescriptorInnerKey {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

apoelstra commented Jul 23, 2023

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.

@quad 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
@quad quad force-pushed the ssr/20230723/better-inner-dpub-types branch from ef617d5 to b18ba59 Compare July 24, 2023 11:11
@quad
Copy link
Contributor Author

quad commented Jul 24, 2023

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
Copy link
Member

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
Copy link
Contributor Author

quad commented Jul 25, 2023

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
Copy link
Contributor Author

quad commented Jul 25, 2023

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
Copy link
Member

sanket1729 commented Jul 27, 2023

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?

Comment on lines +21 to +23
type DescriptorSinglePublicKey = SinglePub;
type DescriptorExtendedPublicKey = DescriptorXKey<bip32::ExtendedPubKey>;
type DescriptorMultiExtendedPublicKey = DescriptorMultiXKey<bip32::ExtendedPubKey>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants