Define a new type for derived `DescriptorPublicKey`s #345

afilini · 2022-04-15T15:52:44Z

After a brief chat on IRC and in PR #339, here's a quick draft for a new type specific to derived DescriptorPublicKeys.

This is the same design we use in BDK, where we wrap a normal DescriptorPublicKey and a secp context in a new struct. This has the drawback that the struct carries a lifetime and generic for the ctx, but I couldn't think of any other way to get this to work unless we drop the implementation of ToPublicKey for DerivedDescriptorKey which in my opinion can be very useful.

sanket1729

concept ACK. I think we need to also revisit the APIs in impl Descriptor<DescriptorPublicKey> in descriptor/mod.rs to use this type.

src/descriptor/key.rs

sanket1729 · 2022-04-15T20:52:58Z

src/descriptor/key.rs

+
+impl<'secp, C: Verification> ToPublicKey for DerivedDescriptorKey<'secp, C> {
+    fn to_public_key(&self) -> bitcoin::PublicKey {
+        self.key.derive_public_key(&self.secp).unwrap()


Let us not derive ToPublicKey at all. Users can also use the

rust-miniscript/src/descriptor/mod.rs

Line 684 in d8cc633

pub fn derived_descriptor<C: secp256k1::Verification>(

to avoid the awkward translate_pk2 APIs.

derived_descriptor() is only available on Descriptor<DescriptorPublicKey>, so if I did a first derive() and now I have a Descriptor<DerivedDescriptorKey> then I can't use this directly and I have to go back to the "un-derived" descriptor.

Should I implement something similar for Descriptor<DerivedDescriptorKey>? Ideally with a different name, otherwise it can get confusing. Something that internally translates the keys

sanket1729 · 2022-04-15T21:00:17Z

ToPublicKey for DerivedDescriptorKey which in my opinion can be very useful.

Indeed this is super useful. After we have private key derivations, we can add support for this. But for now, this would have to do.

Also, creating new verification contexts is now cheap(not exactly free still takes 50 micros seconds) as these contexts are now static in the underlying secp library. We are waiting on some cleanups from upstream secp after which creating verification contexts would be free!

LLFourn · 2022-04-19T01:22:12Z

src/descriptor/key.rs

+/// A derived [`DescriptorPublicKey`]
+///
+/// Derived keys are guaranteed to never contain wildcards
+pub struct DerivedDescriptorKey<'secp, C: 'secp + Verification> {


I think a much better name here would be DefiniteDescriptorKey. "Derivation" is the process of going from an extended key to a child key not from going from a key with wildcards to a key at a particular index of a wildcard.

I am not thrilled with either names, but can't come up with a better name myself. I don't have any preference between both options, both are not ideal.

I think definite vs indefinite is the correct way of distinguishing between this property linguistically. For example in English it shows up as:

Definite: I'm taking my cat to the vet

Indefinite: I'm taking a cat to the vet (we don't know which cat)

In miniscript you have:

Definite: A descriptor where all derivation indexes are known

Indefinite: A descriptor that could be instantiated at a particular index (like "a cat" could be instantiated with any particular cat but "my cat" could not).

It might be useful to apply this naming logic to other parts of the code. There is a function named derive that doesn't actually derive but just sets the derivation index. Then there is a function that actually does derivation called derive_public_key.

I was confused by the current function naming you describe recently, +1 for 'definite'.

I think renaming derive to set_derivation_index would probably be better than make_definite or something. But I do think definite and indefinite in the type names make sense.

Maybe Concrete/Abstract?

I'll let you guys discuss this, let me know when you've settled with one and I'll update my PR.

I personally don't have any preference.

src/descriptor/key.rs

afilini · 2022-04-25T13:59:45Z

Addressed most comments

apoelstra · 2022-05-03T20:45:31Z

Needs rebase, and does not compile currently I think because of rust-bitcoin updates. but ACK e619be4 -- this is much smaller/cleaner than I feared.

afilini · 2022-05-04T16:06:58Z

Rebased and fixed everything!

apoelstra

Nice!
ACK 28615fd

sanket1729

ACK 28615fd

sanket1729

ACK 28615fd. Was going to merge this, but this would need to be rebased because of conflicts.
Might be good to address the nit as well

sanket1729 · 2022-05-09T23:41:34Z

src/descriptor/key.rs

+    }
+
+    /// Return the derivation index of this key
+    pub fn get_index(&self) -> u32 {


nit: We usually don't use the get prefix for accessor methods.

afilini · 2022-05-10T18:26:07Z

Rebased and fixed your last nit!

apoelstra

ACK 9cf065b

sanket1729

ACK 9cf065b

2ad6555 Rename "derive" things that are not doing derivation (LLFourn) 6dc6aca Make DerivedDescriptorKey first-class citizen (LLFourn) Pull request description: This PR further develops the idea introduced in #345. It has two commits both with relatively detailed commit messages that can be reviewed separately. In summary: 1. Develop the `DerivedDescriptorKey` (a key that is guaranteed not to have wildcard in it) idea more by adding missing functionality and refining the API. In addition, I removed the error cases from `ConversionError` which seems to have been omitted from #345. 2. Since the introduction of `DerivedDescriptorKey`, the overlapping usage of the term "derive" has become quite confusing. In addition to the usual definition of "derive" we have also used it to mean "replace a wildcard with a particular derivation index". I deprecated and renamed everything that uses the latter definition. ACKs for top commit: apoelstra: ACK 2ad6555 sanket1729: ACK 2ad6555. Thanks for the clean changes. Tree-SHA512: 0198404a1bfecab2324a8785117248fc566cfbb53decbd234928e378f102bdc5c5de6d31b437b8f1b0ba90ef524a362a46150028f80a4b029589406233abd2fe

… `DescriptorPublicKey`s 9cf065b7fafc41a0681374df8aa83baf0b9d3425 Define a new type for derived `DescriptorPublicKey`s (Alekos Filini) Pull request description: After a brief chat on IRC and in PR #339, here's a quick draft for a new type specific to derived `DescriptorPublicKey`s. This is the same design we use in BDK, where we wrap a normal `DescriptorPublicKey` and a secp context in a new struct. This has the drawback that the struct carries a lifetime and generic for the ctx, but I couldn't think of any other way to get this to work unless we drop the implementation of `ToPublicKey` for `DerivedDescriptorKey` which in my opinion can be very useful. ACKs for top commit: apoelstra: ACK 9cf065b7fafc41a0681374df8aa83baf0b9d3425 sanket1729: ACK 9cf065b7fafc41a0681374df8aa83baf0b9d3425 Tree-SHA512: 3dd486d52c589d104da9611c65229988825cc79115a1ff45d1a39aa63e49d2d2a76b40b21a278a58d0b8566b2b37682ffc5efbbb78779ef4e5dadeaf14bbcc6c

afilini force-pushed the derived-descriptor-key branch 2 times, most recently from 9f888ec to 878c000 Compare April 15, 2022 16:15

sanket1729 reviewed Apr 15, 2022

View reviewed changes

LLFourn reviewed Apr 19, 2022

View reviewed changes

src/descriptor/key.rs Outdated Show resolved Hide resolved

afilini force-pushed the derived-descriptor-key branch 2 times, most recently from e37a3cb to 90c1cdd Compare April 25, 2022 13:57

afilini marked this pull request as ready for review April 25, 2022 13:59

afilini force-pushed the derived-descriptor-key branch from 90c1cdd to e619be4 Compare April 25, 2022 14:12

afilini force-pushed the derived-descriptor-key branch 2 times, most recently from cbf5403 to 28615fd Compare May 4, 2022 16:05

apoelstra previously approved these changes May 4, 2022

View reviewed changes

sanket1729 previously approved these changes May 9, 2022

View reviewed changes

sanket1729 reviewed May 9, 2022

View reviewed changes

Define a new type for derived DescriptorPublicKeys

9cf065b

afilini dismissed stale reviews from sanket1729 and apoelstra via 9cf065b May 10, 2022 18:25

afilini force-pushed the derived-descriptor-key branch from 28615fd to 9cf065b Compare May 10, 2022 18:25

apoelstra approved these changes May 10, 2022

View reviewed changes

sanket1729 approved these changes May 10, 2022

View reviewed changes

sanket1729 merged commit 69440a4 into rust-bitcoin:master May 10, 2022

afilini deleted the derived-descriptor-key branch May 10, 2022 20:05

LLFourn mentioned this pull request Jul 14, 2022

DerivedDescriptor unfinished business #448

Merged

Define a new type for derived DescriptorPublicKeys #345

Define a new type for derived DescriptorPublicKeys #345

Uh oh!

Conversation

afilini commented Apr 15, 2022

Uh oh!

sanket1729 left a comment

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!

sanket1729 commented Apr 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanket1729 Apr 19, 2022 • 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!

afilini May 3, 2022 • 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!

afilini commented Apr 25, 2022

Uh oh!

apoelstra commented May 3, 2022

Uh oh!

afilini commented May 4, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afilini commented May 10, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Define a new type for derived `DescriptorPublicKey`s #345

Define a new type for derived `DescriptorPublicKey`s #345

sanket1729 Apr 19, 2022 •

edited

Loading

afilini May 3, 2022 •

edited

Loading