Remove the `DescriptorTrait` #386

tcharding · 2022-05-04T00:09:06Z

We never abstract over the DescriptorTrait so it is questionable if it should exist at all. Removing the DescriptorTrait allows us to also delete the pretaproot module because all the fallible vs infallible issues disolve.

Delete the DescriptorTrait, implement the trait methods directly on Descriptor (fallible methods). Implement infallible versions of the trait methods on each descriptor as required.

Done as individual patches to ease review. Although the work is only really meaningful as a block each patch is discreet enough to merge as is.

apoelstra · 2022-05-04T17:38:14Z

Now that the trait methods are infallible I wonder if we should just remove the specialized ones.

tcharding · 2022-05-04T23:36:26Z

That was one direction I thought would come out of review, I'll explore it.

tcharding · 2022-05-05T23:31:35Z

This PR has been totally re-written, description and title have been changed to reflect the new PR content.

tcharding · 2022-05-05T23:35:44Z

I was hoping to re-name the DescriptorTrait, the best I could come up with wasSatisfy but the three methods sanity_check, script_pubkey, and unsigned_script_sig don't fit into that name :(

src/descriptor/bare.rs

src/descriptor/segwitv0.rs

apoelstra · 2022-05-06T13:33:35Z

I don't know about this approach of pulling individual methods into infallible traits. It means that you can't write code which is generic over all descriptor types, and still be able to compute Addresses, unless you use the specific enum Descriptor type that we provide. It means that other implementors of DescriptorTrait that don't also implement ToAddress and ExplicitScript are second-class.

It also requires author of generic code to provide several more explicit trait bounds.

On the other hand, going forward we expect users to mostly use Taproot in which case they can ignore the ExplicitScript trait, and we don't expect any new ToAddress descriptor types to pop up do we?

@sanket1729 what do you think?

tcharding · 2022-05-08T02:13:06Z

I don't know about this approach of pulling individual methods into infallible traits. It means that you can't write code which is generic over all descriptor types, and still be able to compute Addresses, unless you use the specific enum Descriptor type that we provide. It means that other implementors of DescriptorTrait that don't also implement ToAddress and ExplicitScript are second-class.

hmmm, I'm not confident that I can mentally model consuming these generic APIs, I seem to be only thinking at the level of the code on the sceen in rust-minscript. @thomaseizinger has a pretty good brain for this sort of thing, do you have a few clock cycles to spare to weigh in on this please?

In a nutshell the [perceived] problem this PR is trying to solve is that pre-taproot and taproot descriptors are not being treated as equivalent as is apparent by the need for the pretaproot module. This is (in my opinion) just a code smell and the 'solution' I implemented just fell out while I was messing around.

It also requires author of generic code to provide several more explicit trait bounds.

I agree with you, this is definitely bad.

On the other hand, going forward we expect users to mostly use Taproot in which case they can ignore the ExplicitScript trait, and we don't expect any new ToAddress descriptor types to pop up do we?

I'm out of my depth here.

@sanket1729 what do you think?

Thanks in advance @sanket1729 :)

thomaseizinger

Thanks for tagging me @tcharding, I'll add my 2c :)

In general, smaller, focused traits are better because they encourage composability. i.e. I can write code that only abstracts over ToAddress and compose that with code that abstracts over ToAddress + ScriptCode. If I can only abstract over a single trait, the individual units of code tend to be bigger.
It is still possible to provide a general DescriptorTrait that forces to implement all other traits and provide a blanket impl to reduce noise in where clauses. I would consider this to be somewhat of an anti-pattern though.

I've skimmed the codebase and we don't seem to have a single piece of code that says:

fn generic_code<D: DescriptorTrait>(desc: D) { .. }

I'd like to challenge whether this trait should even exist if we never abstract over it?

In my opinion:

Trying to guess what other uses may want abstract over is a fallacy. They can create their own traits if they need to abstract over multiple descriptors.
Aligning APIs is not enough to justify a trait. Several, concrete descriptors can have the same API without forcing the user to import a trait to use them. This should resolve the fallible vs. infallible dilemma.
The main API seems to be the Descriptor enum. The functions on that one can just live within a regular impl block and delegate to all known variants. No trait needed for that.

thomaseizinger · 2022-05-09T06:56:22Z

src/descriptor/mod.rs

+}
+
+/// A trait for getting the `scriptCode` of a transaction output.
+pub trait ScriptCode<Pk: MiniscriptKey + ToPublicKey> {


I would avoid trait-bounds in trait definitions (and struct definitions as well). They don't add any value unless you need to access associated types. Keep trait bounds to where clauses in impl blocks where (pun intended) the trait functionality is actually needed.

That will result in the least amount of friction.

This definition also doesn't seem to even use Pk. Why is it there? I think you should be able to completely omit these Pk type parameters.

thomaseizinger · 2022-05-09T06:58:43Z

src/descriptor/mod.rs

+/// A trait for getting the "witness script" for a descriptor.
+pub trait ExplicitScript<Pk: MiniscriptKey + ToPublicKey> {
+    /// Computes the "witness script" of the descriptor, i.e. the underlying
+    /// script before any hashing is done. For `Bare`, `Pkh` and `Wpkh` this
+    /// is the scriptPubkey; for `ShWpkh` and `Sh` this is the redeemScript;
+    /// for the others it is the witness script.
+    fn explicit_script(&self) -> Script;
+}


You are talking a lot about "witness script" in the docs but the trait is named ExplicitScript. Sure that is the best name? :)

I think this is on purpose, FTR my new version does not use this short form of the comment.

Agreed, I think it's the comment that should be changed :)

Cheers, fixed. The code comment I left in may need further improvements.

thomaseizinger · 2022-05-09T07:41:02Z

Aligning APIs is not enough to justify a trait. Several, concrete descriptors can have the same API without forcing the user to import a trait to use them. This should resolve the fallible vs. infallible dilemma.

On this: The problem the pretaproot module seems to aim to solve is that some users may (statically) know that they don't have a taproot descriptor and thus can get access to infallible APIs. There are multiple ways of solving this, but essentially what you want is known as "dependent types". Rust doesn't have those but you can emulate them to some degree.

Instead of splitting the descriptors into two enums, we can use another type parameter to encode in the type system, which APIs are available. For example (names subject to bike-shedding):

/// Marker type to encode that we only support descriptors pre-taproot.
enum PreTaprootOnly { }

/// Marker type to encode that we support all kinds of descriptors, including taproot.
enum AllKinds { }

struct Descriptor<Pk, TaprootMarker> {
	pub fn parse_pre_taproot<Pk>(desc: &str) -> Result<Descriptor<Pk, PreTaprootOnly>, Error> {}
	pub fn parse<Pk>(desc: &str) -> Result<Descriptor<Pk, AllKinds>, Error> {}
}

Within parse_pre_taproot, we would fail in case someone gives us a taproot descriptor. Later in the process, we can then write:

struct Descriptor<Pk, PreTaprootOnly> {
   // all sorts of functions in an infallible-flavor because we are guaranteed to never have the `Tr` variant
}

struct Descriptor<Pk, AllKinds> {
   // all sorts of functions in a fallible-flavor because we may also hold a `Tr` variant
}

struct Descriptor<Pk, TaprootMarker> {
   // all functions which don't care about taproot
}

This design relies on the fact that nothing else other than the parse functions above can construct an instance of Descriptor. We would thus have to move away from a public enum and instead make the enum pub(crate) or private and hold it in an inner, private field.

Coming back to the initial goal: In case a user statically knows that they will never need taproot scripts, they can use the specialised constructor and gain access to a range of infallible APIs which would otherwise not exist. If that cannot be guaranteed statically, they need to use the other parse API and deal with potential failure cases. We can also offer a conversion function between two.

I am unsure whether this complexity is worth it though.

sanket1729 · 2022-05-09T23:11:29Z

I think before we tackle the reform of descriptor trait, we should
Overall descriptors overall serve three main purposes:

Create new addresses: Done with address API.
Watch funds already contained in some descriptors: Done with script_pubkey API. Note that this allows watching of descriptors that don't have a address defined for them.
Enable some form of semantic analysis from the descriptor: Possible with the lift trait. ExplicitScript also kind of falls into this category.

We also clubbed other minor functionality descriptor trait with signing-related utilities. For example:

In case the descriptor is wrapped segwit descriptor, we can figure out unsigned_script_sig
For segwit signers that need access to script code, we have the script_code API.
explicit_script offers a nice way to look at the internal policy without the script wrapping.

None of these require functions like sanity check that should be removed and not be a part of trait API. This should be enforced(I think it already is, but I will double-check) at descriptor creation time.

I tried to do document something similar a while ago: https://gist.github.com/sanket1729/b1ed8ed776c7ac798d5e33e40f5d996d.

Trying to guess what other uses may want abstract over is a fallacy. They can create their own traits if they need to abstract over multiple descriptors.
Aligning APIs is not enough to justify a trait. Several, concrete descriptors can have the same API without forcing the user to import a trait to use them. This should resolve the fallible vs. infallible dilemma.
The main API seems to be the Descriptor enum. The functions on that one can just live within a regular impl block and delegate to all known variants. No trait needed for that.

I agree with @thomaseizinger, I think this is the way to go. AFAIR, @afilini has a downstream implementation of DescriptorTrait. Would like see what he thinks of the idea of decomposing DescriptorTrait into multiple traits.

I don't know about this approach of pulling individual methods into infallible traits. It means that you can't write code which is generic over all descriptor types, and still be able to compute Addresses, unless you use the specific enum Descriptor type that we provide. It means that other implementors of DescriptorTrait that don't also implement ToAddress and ExplicitScript are second-class.

This is the major criticism of the idea. I agree that this is bad, but I think people implementing custom descriptor traits are already advanced users of the library that I don't mind making things hard of them. This simplifies the codebase for us and the majority of people whom we expect will use the Descriptor enum from this library. What do you think @apoelstra ?

sanket1729

Sorry for the delay. I was away last weekend, will catch up with notifications across the rust-bitcoin stack

sanket1729 · 2022-05-09T16:40:34Z

src/descriptor/bare.rs

@@ -20,7 +20,7 @@

 use std::{fmt, str::FromStr};

-use bitcoin::{self, blockdata::script, Script};
+use bitcoin::{self, blockdata::script, Address, Network, Script};


You might find this throughout the codebase. I think I (probably Andrew too), always used bitcoin::DataStructure throughout the codebase.
I have no opposition to changing this, just because I want to be consistent with the new code that I push going forward.

I was not sure what to do about this. I saw, as you say, this is done a bunch. I get it for PublicKey and PrivateKey but I don't get it for Network/Address etc. since this is a bitcoin library we all know what these things are. I was also unsure because of uniformity in this crate vs uniformity across the stack, I'm not exactly sure but I think it makes sense to try for uniformity across the stack?

Is there some perspective on using bitcoin::Address that I'm missing. Its not the first time I've debated removing paths from identifiers so I know I tend to err on the side of too terse :)

I do this because I work a lot on rust-elements (which has its own fork of miniscript which uses the new opcodes) so it's easy to distinguish elements::Address from bitcoin::Address. For Address in particular it also helps me distinguish a "bitcoin address" from a "network address" or "memory address" etc.

I agree that within rust-miniscript itself, this serves little purpose. I mildly prefer things the way they are but don't have strong feelings either way.

I'm not exactly sure but I think it makes sense to try for uniformity across the stack

Yep, I would like to be consistent throughout the stack. I think with all the new contributors and throughout the lifetime of the project, this seems difficult to enforce/guarantee.

I agree that using Address makes sense (although there is also IP address in the network module). I always have had the personal convention to external_crate::DataStructure or have exactly one thing before the :: in the path. I don't have a strong rationte to do this, just a habit I subconsciously formed.

I do this because I work a lot on rust-elements (which has its own fork of miniscript which uses the new opcodes) so it's easy to distinguish elements::Address from bitcoin::Address

For the purposes of having minimal conflicts when porting to elements-miniscript, it might be better to Addresss instead of bitcoin::Address. Considering this, if you don't have any preference between the two I would rather it be Address than bitcoin::Address as it would save me some time when porting changes from here.

Ok cool, good feedback. Let's go with Address (and Network) then.

tcharding · 2022-05-10T00:58:51Z

Sorry for the delay. I was away last weekend

No sweat, good to have you back :)

tcharding · 2022-05-10T01:21:08Z

Thanks for your input @thomaseizinger, I appreciate the effort you went to.

apoelstra · 2022-05-10T18:16:02Z

I agree with @thomaseizinger and @sanket1729. Lets just drop our own DescriptorTrait (splitting it into a set of smaller traits where that makes sense) and move our "descriptor-generic" code into direct impls on the Descriptor enum.

People who want their own "descriptor trait" are doing advanced and probably-custom things and can roll their own traits.

Thanks for your detailed analysis, and for reverse-engineering so much of our API @thomaseizinger !!

thomaseizinger

Skimmed the PR again and made a few comments, looks very good overall :)

src/descriptor/bare.rs

src/descriptor/mod.rs

src/descriptor/sh.rs

tcharding · 2022-05-11T02:09:01Z

Implemented suggestions from @thomaseizinger, squashed it all down to two patches, rebased, and re-wrote the PR description.

afilini · 2022-05-12T08:22:19Z

AFAIR, @afilini has a downstream implementation of DescriptorTrait. Would like see what he thinks of the idea of decomposing DescriptorTrait into multiple traits.

Yeah kinda, we have some traits that we implement on Descriptor that are in a way an extension to DescriptorTrait, to provide other useful stuff quickly.

I haven't read the code or anything, just wanted to say that we were planning to abstract over generic DescriptorTrait types in BDK to support custom scripts that can't be expressed as a descriptor. So the idea was to store a generic type inside the wallet, and if you want you can implement DescriptorTraiton your custom type and BDK would work with it automatically.

That said, it's true that we already have our own custom traits that you probably would have to implement as well in the scenario I described, so it's not the end of the world to reimplement in BDK the equivalent of DescriptorTrait if you remove it from here.

apoelstra

ACK dbdcc81

It's a big diff and I'm not sure I fully grokked all the API changes. But I tried to read it carefully and this all looks good to me. One minor nit is that I'd prefer the term "ec-signature" in the comments to be changed to "ECDSA siganture".

thomaseizinger · 2022-05-14T03:06:40Z

This sentence in the 2nd commit could be worded better IMO:

This makes using the type for ergonomic also since users no longer need to import the trait to use a descriptor.

Maybe say: This makes usage of the Descriptor type more ergonomic because users no longer need to import a trait.

tcharding · 2022-05-15T22:32:45Z

I converted to draft. Now we have some agreement on the way forward I'll put some effort into making the patches more reviewable. Probably should have been draft this whole time, my bad.

Using the `bitcoin::` path is necessary for keys but for other well known types, like `Address`, using the path does not add to the readability of the code. (This is subjective.)

In preparation for removing the `DescriptorTrait` remove the pretaproot module. This is possible because removing the trait will resolve the error return issues that the taproot module exists to solve.

In preparation for removing the `DescriptorTrait` move the `sanity_check` trait methods onto each individual descriptor.

In preparation for removing the `DescriptorTrait` move the `address` trait methods onto each individual descriptor. Replaces the `addr` method.

In preparation for removing the `DescriptorTrait` move the `script_pubkey` trait methods onto each individual descriptor. Replaces the `spk` method. Note that once upon a time `spk` was an infallible version of `script_pubkey` but now `script_pubkey` is itself infallible so this patch just renames the `spk` method and removes the trait method.

In preparation for removing the `DescriptorTrait` move the `unsigned_script_sig` trait methods onto each individual descriptor as required (`Sh` only). All the other trait method implementations just return `Script::new()`, just call `new` directly from the implantation of `unsigned_script_sig` on `Descriptor`.

In preparation for removing the `DescriptorTrait` remove the `explicit_script` trait method. Implement the same logic on `Descriptor` directly but call through to `inner_script` or `script_pubkey` for each descriptor as required.

In preparation for removing the `DescriptorTrait` move the `script_code` trait methods onto each individual descriptor. Fix docs on related methods also (e.g. `inner_scipt`). Implement `script_code` directly on `Descriptor`.

We never abstract over the `DescriptorTrait`, it is therefore questionable for it to exist. We have recently removed a number of trait methods out of the trait and implemented them directly on the respective descriptors. We can now do the same with the remaining 'satisfaction' trait methods. Implement the various satisfaction methods on each descriptor as required. Implement the methods directly on `Descriptor`. Delete the `DescriptorTrait`.

tcharding · 2022-05-17T02:17:06Z

Split up into smaller patches, re-wrote PR description. Its still a bit to review, thanks in advance for the time!

apoelstra

ACK 32a74af

It is much easier to read this way, thanks!

thomaseizinger

Nice work.

ACK 32a74af

thomaseizinger · 2022-05-19T09:24:39Z

src/descriptor/mod.rs

-        Pk: ToPublicKey,
-    {
+    ///
+    /// Some descriptors like pk() don't have any address.


Nit:

Suggested change

/// Some descriptors like pk() don't have any address.

/// Some descriptors like pk() don't have an address.

Agree, this is a silght improvement

sanket1729

ACK 32a74af. This is a very good improvement.

sanket1729 · 2022-05-19T17:18:58Z

src/lib.rs

@@ -706,7 +705,7 @@ impl fmt::Display for Error {
                write!(f, "MultiA too many keys {}", k)
            }
            Error::TaprootSpendInfoUnavialable => {
-                write!(f, "Taproot Spend Info not computed. Hint: Did you call `compute_spend_info` before calling methods from DescriptorTrait")


Thanks for catching this.

a84a238 Fix grammar 'any' -> 'an' (Tobin C. Harding) Pull request description: Do minor grammar improvement. This is a nit from review of #386 (review) ACKs for top commit: sanket1729: ACK a84a238 Tree-SHA512: a9b08fb7a28a10958b8d2ecdf103fa4e7e6240d9217b3b5468aff423f7d6e6cae7e2980469f206f4316a91b29d1cfc8282d8dc563acce299b3925f3f2296a5c7

tcharding changed the title ~~Descriptor spk~~ Refactor and document code around the spk method May 4, 2022

tcharding marked this pull request as draft May 4, 2022 00:19

tcharding force-pushed the descriptor-spk branch from 4a234e2 to a680ec7 Compare May 4, 2022 00:35

tcharding changed the title ~~Refactor and document code around the spk method~~ Attempt to untangle code/docs around the spk method May 4, 2022

tcharding force-pushed the descriptor-spk branch 2 times, most recently from 13e3892 to 9dd0371 Compare May 4, 2022 00:51

tcharding marked this pull request as ready for review May 4, 2022 01:49

tcharding force-pushed the descriptor-spk branch from 9dd0371 to dcfa174 Compare May 5, 2022 23:30

tcharding changed the title ~~Attempt to untangle code/docs around the spk method~~ Extract new traits out of DescriptorTrait May 5, 2022

tcharding force-pushed the descriptor-spk branch from dcfa174 to a0c7b0c Compare May 5, 2022 23:50

apoelstra reviewed May 6, 2022

View reviewed changes

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

apoelstra reviewed May 6, 2022

View reviewed changes

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

thomaseizinger reviewed May 9, 2022

View reviewed changes

sanket1729 reviewed May 9, 2022

View reviewed changes

tcharding force-pushed the descriptor-spk branch from a0c7b0c to 7e5bd23 Compare May 10, 2022 01:16

tcharding marked this pull request as draft May 10, 2022 01:16

thomaseizinger reviewed May 11, 2022

View reviewed changes

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

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

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

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

tcharding force-pushed the descriptor-spk branch from cc6e055 to dbdcc81 Compare May 11, 2022 02:06

tcharding marked this pull request as ready for review May 11, 2022 02:07

apoelstra previously approved these changes May 13, 2022

View reviewed changes

tcharding changed the title ~~Extract new traits out of DescriptorTrait~~ Remove the DescriptorTrait May 15, 2022

tcharding marked this pull request as draft May 15, 2022 22:31

tcharding added 9 commits May 17, 2022 09:53

Remove unnecessary path

03d4bd6

Using the `bitcoin::` path is necessary for keys but for other well known types, like `Address`, using the path does not add to the readability of the code. (This is subjective.)

Delete the pretaproot module

4980604

In preparation for removing the `DescriptorTrait` remove the pretaproot module. This is possible because removing the trait will resolve the error return issues that the taproot module exists to solve.

Remove sanity_check from DescriptorTrait

4a9e20f

In preparation for removing the `DescriptorTrait` move the `sanity_check` trait methods onto each individual descriptor.

Remove address from DescriptorTrait

3d0b112

In preparation for removing the `DescriptorTrait` move the `address` trait methods onto each individual descriptor. Replaces the `addr` method.

Remove explicit_script from DescriptorTrait

c6884fc

In preparation for removing the `DescriptorTrait` remove the `explicit_script` trait method. Implement the same logic on `Descriptor` directly but call through to `inner_script` or `script_pubkey` for each descriptor as required.

Remove script_code from DescriptorTrait

52a480d

In preparation for removing the `DescriptorTrait` move the `script_code` trait methods onto each individual descriptor. Fix docs on related methods also (e.g. `inner_scipt`). Implement `script_code` directly on `Descriptor`.

tcharding dismissed apoelstra’s stale review via 32a74af May 17, 2022 02:03

tcharding force-pushed the descriptor-spk branch from dbdcc81 to 32a74af Compare May 17, 2022 02:03

tcharding marked this pull request as ready for review May 17, 2022 02:16

apoelstra approved these changes May 18, 2022

View reviewed changes

thomaseizinger approved these changes May 19, 2022

View reviewed changes

sanket1729 approved these changes May 19, 2022

View reviewed changes

sanket1729 merged commit 511e66b into rust-bitcoin:master May 19, 2022

tcharding mentioned this pull request May 19, 2022

Fix grammar 'any' -> 'an' #416

Merged

apoelstra mentioned this pull request Jul 23, 2023

Expose inner DescriptorPublicKey functionality #570

Open

	/// Some descriptors like pk() don't have any address.
	/// Some descriptors like pk() don't have an address.

Remove the DescriptorTrait #386

Remove the DescriptorTrait #386

Conversation

tcharding commented May 4, 2022 • edited Loading

apoelstra commented May 4, 2022

tcharding commented May 4, 2022

tcharding commented May 5, 2022

tcharding commented May 5, 2022

apoelstra commented May 6, 2022

tcharding commented May 8, 2022

thomaseizinger left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger commented May 9, 2022

sanket1729 commented May 9, 2022 • edited Loading

sanket1729 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented May 10, 2022

tcharding commented May 10, 2022

apoelstra commented May 10, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

tcharding commented May 11, 2022

afilini commented May 12, 2022

apoelstra left a comment

Choose a reason for hiding this comment

thomaseizinger commented May 14, 2022

tcharding commented May 15, 2022 • edited Loading

tcharding commented May 17, 2022

apoelstra left a comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanket1729 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remove the `DescriptorTrait` #386

Remove the `DescriptorTrait` #386

tcharding commented May 4, 2022 •

edited

Loading

thomaseizinger left a comment •

edited

Loading

sanket1729 commented May 9, 2022 •

edited

Loading

tcharding commented May 15, 2022 •

edited

Loading