descriptor: introduce several Taproot accessors #751

apoelstra · 2024-10-10T13:27:34Z

When working with Taproot descriptors you typically need to do an annoying (and hard to discover) match statement to get the Tr out of the descriptor, and then call accessors on that to get the actual data out.

Add two new methods to Descriptor that directly access the internal key and the taptree. Document that the actual leaves can be obtained by calling .iter on the taptree.

Next, when a user is trying to sign a Taproot branch, they need to obtain a TapLeafHash. We have internal code which does this (which I have pulled into a helper function since there is some room to optimize it there..) but no exposed code, forcing the user to go digging through the rust-bitcoin docs to figure it out (including knowing the standard Taproot leaf version, which is an arcane detail of the sort that Miniscript otherwise hides).

Add a new method leaf_hash on Taproot miniscripts, so that the user can directly obtain the leaf hashes.

Now you can write e.g.

for script in trdesc.tap_tree_iter() {
    let leaf_hash = script.leaf_hash();
    // Do whatever you want...
}

vs the previous code which was roughly

let tr = match trdesc {
    Descriptor::Tr(ref tr) => tr,
    _ => unreachable!("I know this is a Taproot descriptor"),
};
// Or tr.tap_tree().unwrap().iter() in case you miss the weirdly-named // Tr::iter_scripts
for script in tr.iter_scripts() {
    // Hope you know your rust-bitcoin docs by heart, and also that
    // .encode is the way to convert a Miniscript to a Script!
    let leaf_hash = TapLeafHash::from_script(
        LeafVersion::TapScript,
        script.encode(),
    );
}

When working with Taproot descriptors you typically need to do an annoying (and hard to discover) `match` statement to get the `Tr` out of the descriptor, and then call accessors on that to get the actual data out. Add two new methods to `Descriptor` that directly access the internal key and the taptree. Document that the actual leaves can be obtained by calling `.iter` on the taptree. Next, when a user is trying to sign a Taproot branch, they need to obtain a TapLeafHash. We have internal code which does this (which I have pulled into a helper function since there is some room to optimize it there..) but no exposed code, forcing the user to go digging through the rust-bitcoin docs to figure it out (including knowing the standard Taproot leaf version, which is an arcane detail of the sort that Miniscript otherwise hides). Add a new method `leaf_hash` on Taproot miniscripts, so that the user can directly obtain the leaf hashes. Now you can write e.g. for script in trdesc.tap_tree_iter() { let leaf_hash = script.leaf_hash(); // Do whatever you want... } vs the previous code which was roughly let tr = match trdesc { Descriptor::Tr(ref tr) => tr, _ => unreachable!("I know this is a Taproot descriptor"), }; // Or tr.tap_tree().unwrap().iter() in case you miss the weirdly-named // Tr::iter_scripts for script in tr.iter_scripts() { // Hope you know your rust-bitcoin docs by heart, and also that // .encode is the way to convert a Miniscript to a Script! let leaf_hash = TapLeafHash::from_script( LeafVersion::TapScript, script.encode(), ); }

apoelstra · 2024-10-10T13:28:17Z

cc @miketwenty1 this should make your life much easier.

Also, we should forward-port this to elements-miniscript (where there is a bit more work to do).

apoelstra · 2024-10-10T13:45:08Z

One still-missing piece of API surface is that there is no accessor for the BIP340 "standard unspendable key".

This is difficult to implement because there are currently no const constructors for XOnlyPublicKey, so I didn't include it here. Probably it will depend on updates to rust-secp to do key-parsing in pure Rust.

sanket1729 · 2024-10-10T22:32:16Z

Nice, on my reviewlist for this weekend.

shesek · 2024-10-14T02:51:42Z

How about a Descriptor::tap() -> Option<&Tr> method? Personally I would prefer using an explicit .tap() and chaining methods on that over the shortcut methods.

shesek · 2024-10-14T02:53:09Z

src/descriptor/mod.rs

@@ -241,6 +241,40 @@ impl<Pk: MiniscriptKey> Descriptor<Pk> {
        Ok(Descriptor::Tr(Tr::new(key, script)?))
    }

+    /// For a Taproot descriptor, returns the internal key.
+    pub fn internal_key(&self) -> Option<&Pk> {


Perhaps name it tap_internal_key() to keep it consistent with the other methods and to make it clearer that its taproot-related and taproot-only?

The other methods yield a TapTree and are named as such, with no prefix. It's just more obvious that a "taptree" is a taproot object while an "internal key" is less obvious.

I can stick a taproot_ prefix onto here if you want, though that makes the method name longer and won't make it look consistent with the other methods. Since Taproot is the "default" output type for the forseeable future, I would almost rather stick non_taproot_ onto everything else..

If Taproot is considered the 'default' then not having a prefix definitely makes sense. I didn't think of it as such.

Heh, well, today it's definitely not the default (as evidenced by you, and me, and miketwenty1, running into a tons of API issues just in the last couple months). But things are trending that way and all the extensions to Bitcoin I see on the horizon work within Taproot, so I think we should assume (for API purposes) that this is the way things are going.

Having said that, maybe the way to do this is to foreground Tr, as you say, and let Taproot-only users use that in place of Descriptor? Not sure.

I can add a taproot_ prefix here if you really want. But I weakly prefer not to.

shesek · 2024-10-14T02:59:22Z

src/descriptor/mod.rs

+
+    /// For a Taproot descriptor, returns an iterator over the scripts in the Taptree.
+    ///
+    /// If the descriptor is not a Taproot descriptor, **or** if the descriptor is a


I find this kind of strange, I would prefer to differentiate these two cases so I can handle the 'not Taproot descriptor' as an error case if I was expecting it to be a Taproot.

But I guess that if the distinction is important, one could also explicitly use (my suggested) tap(), handle the None case, then iter.

You can differentiate them by calling .tap_tree(), dealing with the Option, then calling iter_scripts. I added this method specifically to get rid of the extra unwrap that would require.

Ah, actually to distinguish a non-taproot descriptor from a keyspend-only descriptor, but without matching and without being able to distinguish further, you would need to call .internal_key().is_some().

I added this method specifically to get rid of the extra unwrap that would require.

In what scenarios would it make sense to have a code path that handles both taproot and non-taproot descriptors in that way? It seems to me that using iter_tap_tree on non-taproot descriptors would typically be a coding mistake, can't really think of a case where I'd want to do it and have it return a empty iterator

Ah, I see from your other comment that you were thinking about already known-taproot descriptors, say if the same code initializes the tr descriptor and calls methods on it. It can make sense for such cases.

Also, if you are trying to "just sign a transaction" you might take an arbitrary descriptor, grab its scriptcode to produce a pre-Taproot sighash, then also iterate through all the Taproot branches (if any) to produce Taproot sighashes. In this case you could use the same code for both Taproot and non-Taproot descriptors.

shesek · 2024-10-14T02:59:57Z

src/descriptor/mod.rs

+    /// Taproot descriptor containing only a keyspend, returns an empty iterator.
+    pub fn tap_tree_iter(&self) -> tr::TapTreeIter<Pk> {
+        if let Descriptor::Tr(ref tr) = self {
+            if let Some(ref tree) = tr.tap_tree() {


Why not use tr.iter_scripts()?

apoelstra · 2024-10-14T12:41:21Z

@shesek I am not going to add a .tap() method that returns a Tap. Users have no idea what a Tap is because it's an awkwardly named struct that exists in the API purely to hide some implementation details of the Descriptor::Tr variant. From a user POV it consists of exactly two things: an internal key and an (optional) taptree. I have added accessors for both of those.

shesek · 2024-10-14T15:23:22Z

From a user POV it consists of exactly two things: an internal key and an (optional) taptree.

I found myself also needing to access the inner Tr to convert it into a TaprootSpendInfo via spend_into(), and to access the merkle root. Another thing that may be useful is the output key (although typically the scriptPubKey containing it is what's needed).

apoelstra · 2024-10-14T15:39:03Z

Ah, good point. Also, I realize that the structure we're talking about is Tr, not Tap. And I like the name Tr much better since it reflects "the data of a tr descriptor". Ok, I will add a tr accessor. But I would still like to keep the set of accessors that I've added here, since they let you do common things on known-tr descriptors without needing unwraps.

I also see that there isn't a direct accessor for the external key. To get this from a descriptor you need to call descriptor.tr().unwrap().spend_info().output_key() which is pretty hard to discover.

Will you be in Atlanta for TABConf? Maybe we should try to whiteboard out all the Taproot data structures and try to come up with a sensible API to organize them.

shesek · 2024-10-14T16:12:30Z

I also see that there isn't a direct accessor for the external key.

Same is also true for the merkle root.

Will you be in Atlanta for TABConf? Maybe we should try to whiteboard out all the Taproot data structures and try to come up with a sensible API to organize them.

Was not planning to... but maybe 👀

I do have some ideas and some taproot stuff I implemented as extension traits that would be nice to have as part of rust-{miniscript,bitcoin}. We could also discuss it online (IRC?)

apoelstra · 2024-10-14T16:43:29Z

Yep, I'm on IRC. Let's move there ##miniscript or #rust-bitcoin on Libera to discuss the bigger picture.

sanket1729

ACK 86305f0

apoelstra · 2024-10-23T22:53:15Z

I'm going to merge this because I think it's a strict improvement. Will try to meet with @sanket1729 this week at TABConf and we can brainstorm the Taproot API.

apoelstra · 2024-10-23T23:32:35Z

But for the record -- @shesek would like an accessor for the Tr object that doesn't require the user match themselves, and this PR does not provide that.

apoelstra · 2024-10-27T22:45:14Z

At TABConf @sanket1729 and I worked out a new API which has all of the functionality of the existing Tr struct, its iterator, and the ``SpendInfostructure, while being flatter and more discoverable. We also think it will be more efficient since theSpendInfo` structure has a weird map that doesn't align well with Miniscript but is costly to instantiate.

I will open a new PR this week with a detailed description and implementation.

apoelstra · 2024-10-27T22:45:27Z

apoelstra added 2 commits October 10, 2024 13:39

ci: fix new clippy lint related to doccomments

dd3396e

ci: clippy: remove a ton of now-elidable lifetimes

86305f0

shesek reviewed Oct 14, 2024

View reviewed changes

sanket1729 approved these changes Oct 23, 2024

View reviewed changes

apoelstra merged commit 3a85952 into rust-bitcoin:master Oct 23, 2024
30 checks passed

apoelstra deleted the 2024-10--taproot-improvements branch October 23, 2024 23:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

descriptor: introduce several Taproot accessors #751

descriptor: introduce several Taproot accessors #751

apoelstra commented Oct 10, 2024

apoelstra commented Oct 10, 2024

apoelstra commented Oct 10, 2024

sanket1729 commented Oct 10, 2024

shesek commented Oct 14, 2024 •

edited

Loading

shesek Oct 14, 2024

apoelstra Oct 14, 2024

shesek Oct 14, 2024

apoelstra Oct 14, 2024

shesek Oct 14, 2024

apoelstra Oct 14, 2024

apoelstra Oct 14, 2024

shesek Oct 14, 2024

shesek Oct 14, 2024

apoelstra Oct 14, 2024

shesek Oct 14, 2024

apoelstra commented Oct 14, 2024

shesek commented Oct 14, 2024

apoelstra commented Oct 14, 2024

shesek commented Oct 14, 2024

apoelstra commented Oct 14, 2024

sanket1729 left a comment

apoelstra commented Oct 23, 2024

apoelstra commented Oct 23, 2024

apoelstra commented Oct 27, 2024

apoelstra commented Oct 27, 2024

descriptor: introduce several Taproot accessors #751

descriptor: introduce several Taproot accessors #751

Conversation

apoelstra commented Oct 10, 2024

apoelstra commented Oct 10, 2024

apoelstra commented Oct 10, 2024

sanket1729 commented Oct 10, 2024

shesek commented Oct 14, 2024 • 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

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

apoelstra commented Oct 14, 2024

shesek commented Oct 14, 2024

apoelstra commented Oct 14, 2024

shesek commented Oct 14, 2024

apoelstra commented Oct 14, 2024

sanket1729 left a comment

Choose a reason for hiding this comment

apoelstra commented Oct 23, 2024

apoelstra commented Oct 23, 2024

apoelstra commented Oct 27, 2024

apoelstra commented Oct 27, 2024

shesek commented Oct 14, 2024 •

edited

Loading