Idea: add a PhantomData tag to differentiate `DescriptorPublicKey` before and after derivation

[afilini]

I am starting to realize that most operations one might want to do on a Descriptor are only meaningful either if the descriptor has all the keys in their "extended" form or if all of them are "derived", not both: generally speaking, all the operations that need to call to_public_key() somewhere want the derived keys (and there are actual assertions in the code to ensure that), while other functions that work on the full paths of the keys like matches() want the original descriptor with the wildcard keys.

I think it would be nice to have the compiler check that the right methods are called on the right descriptor, and one way to do that would be to add a PhatomData "tag" to the Pk type that basically carries this information. For instance, the MiniscriptKey trait could be changed to take a type argument which we could call, let's say, "KeyState" and there would be two possibile "states", one called "Extended" and one called "Derived".

For key types that cannot be derived further by definition, only the "Derived" type would be implemented (i.e. impl MiniscriptKey<Derived> for bitcoin::PublicKey), while for types that can be derived like DescriptorPublicKey both would be implemented with a generic. Then, ToPublicKey would only be implemented for the Derived version, which would ensure that all the methods that need to_public_key() cannot possibly be called on a non-derived key. Other methods like derive() and matches() would instead only be available for Descriptors where their key type is Extended, like: impl<Pk: MiniscriptKey<Extended>> Descriptor<Pk> ... . The derive() method would obviously be responsible for translating the keys from the Extended state to the Derived state.

I haven't actually tried implementing this concept (yet), so there's a good chance that this is either not useful at all, completely broken, or just not the best way to do it, but I'm curious to hear your feedback on this topic!

[sanket1729]

(and there are actual assertions in the code to ensure that)

Are there any assertions in the current master that could be causing panic? I couldn't find any in my first pass of the latest master. Or do you mean that you have some assertions downstream?. In any case, I like the overall idea on the surface to separate the operations on Descriptor before and after derivation.

Some discussion starter questions:

In this case, what are the advantages of having a generic instead of a separate return type, and are they worth the code complexity? derive() takes in an ExtendedDescriptorPubkey type and outputs DerviedDescriptorPubKey type. We implement MiniscriptKey for both and implement ToPublicKey for DerivedDescriptorPubkey.

Slightly (un)related, we changed the ToPublicKey API after #166, but that does not address this issue.

[afilini]

Are there any assertions in the current master that could be causing panic? I couldn't find any in my first pass of the latest master. Or do you mean that you have some assertions downstream?. In any case, I like the overall idea on the surface to separate the operations on Descriptor before and after derivation.

You are right, I was referring to this line but that's actually a "debug_assert", not a full assertion. I just wanted to say that it's clear that some functions are designed to operate only on derived keys and that the way the code is written right now highlights that.

rust-miniscript/src/descriptor/mod.rs

Line 664 in a87fd40

debug_assert!(!xpub.is_wildcard);

I guess the advantage would just be the fact that the code can be reused: both DerivedDescriptorPubKey and ExtendedDescriptorPubKey would be pretty much identical, except that the "derived" version doesn't have is_wildcard keys, which would also probably cause even more duplicated code since DescriptorXKey now always has the is_wildcard field and that would need to be removed for the derived version.

Plus, if done right, that would allow for types outside of this library to potentially be "derivable" themselves: somebody could implement MiniscriptKey<Extended> for their own custom type, which I guess is something that this library tires to support considering that it is all generalized around the Pk generic type.

With the way I have those changes in mind I would probably modify the DescriptorPublicKeyCtx type introduced in #166 by removing the child_number: the context would simply be the secp context, and since the methods that need to use to_public_key() would only be available on the already derived descriptor, the child number for the derivation wouldn't be needed anymore.

[sanket1729]

Agreed. I am convinced with the objective that we should have something in the type system to distinguish between DescriptorPublicKey before and after derivation.

I am still thinking about whether updating MiniscriptKey is the best way. Will we have similar issues with tapscript descriptors or MuSig descriptors? On paper, even there it sounds helpful to have a distinction based on KeyState for those descriptors too. @apoelstra, any thoughts?

[apoelstra]

I'm tempted to try a slightly different approach where we genericize over what the * wildcard means, so we could have separate notions of needing a childnum, needing a p2c tweak, or being "final" and not needing anything. These would be reflected in having different ToPubkeyCtx types.

I definitely would like to get avoid making the user provide a dummy childnum in any cases, provided we can do that without having too much of a messy API.

[apoelstra]

I guess we should be explicit about our goals:

1. Clean API with as much error-checking done at compile-time as we can manage
2. DescriptorPublicKey being a type which is 100% compatible with what Core does, so we can achieve the compatibilty goals of the descriptor project.

Then supporting pay-to-contract or other Elements stuff is basically just bonus, we shouldn't make concessions for that. AFAIK there are no plans to support p2c in Core so we should be careful about how first-class we make it in rust-miniscirpt.

[afilini]

This is probably unrelated, but just so we don't forget: for full compatibility with Bitcoin Core I guess we would have to also support the "hardened wildcard" derivation, which would probably involve taking a reference to KeyMap in the derive() function

[apoelstra]

So, we could genericize the * to achieve all these goals...

1. Have one type which de/serializes as /* and takes a childnum
2. one that de/serializes as /*' and takes a (childnum, keymap)
3. one that de/serializes as /## or whatever and takes a p2c tweak

We'd want to support different ones of these on different keys though within the same descriptor....so maybe type-level separation here is not the right approach.

[sanket1729]

We have cleanly separated these via separate types DefinateDescriptorKey and DescriptorPublicKey. Feel free to re-open if you have better ideas.