Taproot descriptor by SarcasticNastik · Pull Request #267 · rust-bitcoin/rust-miniscript

SarcasticNastik · 2021-08-30T16:45:46Z

This defines tree structure for TapTree and implements parsing of Taproot Descriptors as a part of #255 . The purpose is to get basic functionalities for the same and integrate it with the Miniscript compiler.

Feedback about changes to tr.rs and expression.rs would be really appreciated.

Currently helper functions specific to Tr are required to parse taproot descriptor into Tree (as defined in expression.rs).

TODO

Docs for tr.rs
Pass fuzz tests
Include hex checksum for taproot descriptor.

Future Work

Implement functionalities required to integrate it with Descriptor (in mod.rs)

SarcasticNastik · 2021-08-31T17:52:26Z

@sanket1729 Kindly review this PR while I add the required docs for tr.rs.

sanket1729

This looks good. I just did a partial review and left some specific comments in the first commit.
Here are some general remarks:

In general, commits should be atomic (and they should just do one thing and the one thing completely in that commit) and they should do pass all tests/build.

A good way to test this is via -x option in git rebase.

git rebase -i HEAD~4 -x "cargo test --features=compiler". This will stop the code at the commit where the build/test fails, you can fix the changes there and git rebase --continue.

It might take a while to get familiar with advanced git techniques, but it is an important skill in the long run. Every project uses this method so that it is easier for tools like git bisect(essentially a binary search on commits) to figure out errors when something goes wrong and figure out which commit things went bad.

I think for starters, you can separate this into two commits. Adding the code and later another commit for adding the test.

sanket1729 · 2021-08-31T17:58:25Z

src/descriptor/mod.rs

 mod segwitv0;
 mod sh;
 mod sortedmulti;
+mod tr;


In 73dd23e,
You should be exposing public utilities from private module tr to the external user. In particular, I think you need to expose TapTree/Tr.

A good exercise might be to write an example (see /examples folder) to see how an external user might use the library.

This is still not addressed

We still need a statement with pub use self::tr::{TapTree} and all other utilities that need to be exposed.

sanket1729 · 2021-08-31T17:59:35Z

src/descriptor/tr.rs

@@ -0,0 +1,201 @@
+// Tapscript
+
+// use super::{


Unnecessary commented code here in 73dd23e

sanket1729 · 2021-08-31T18:00:20Z

src/descriptor/tr.rs

+use {miniscript::Miniscript, Error, MiniscriptKey};
+
+// TODO: Update this to infer version from descriptor.
+const VER: u8 = 0xc0;


In 73dd23e,
The above two line should be removed.

sanket1729 · 2021-08-31T18:01:56Z

src/descriptor/tr.rs

+#[derive(Clone, Ord, PartialOrd, Eq, PartialEq, Hash)]
+pub enum TapTree<Pk: MiniscriptKey> {
+    Tree(Arc<TapTree<Pk>>, Arc<TapTree<Pk>>),
+    Miniscript_(u8, Arc<Miniscript<Pk, Segwitv0>>),


In 73dd23e,
The leaf version should be removed from Miniscript, and I think we can name the enum variants as Miniscript_ as Leaf

sanket1729 · 2021-08-31T18:06:55Z

src/descriptor/tr.rs

+
+#[derive(Clone, Ord, PartialOrd, Eq, PartialEq, Hash)]
+pub struct Tr<Pk: MiniscriptKey> {
+    key_path: Pk,


rename as internal_key and tree? key_path and script_path represent the ways in which we spend scripts, not the structures.

sanket1729 · 2021-08-31T18:13:55Z

src/descriptor/tr.rs

+    Pk::Hash: FromStr,
+    <Pk as FromStr>::Err: ToString,
+    <<Pk as MiniscriptKey>::Hash as FromStr>::Err: ToString,
+{


Same comment about the bounds here.

sanket1729 · 2021-08-31T18:15:29Z

src/descriptor/tr.rs

+    Pk::Hash: FromStr,
+    <Pk as FromStr>::Err: ToString,
+    <<Pk as MiniscriptKey>::Hash as FromStr>::Err: ToString,
+{


Same comment about the bounds here. Try to separate out the functions that really need the bound into its own impl. For example, the new function Tr::new() should not need this bound.

Also make accessor function for fields in Tr as those are private

sanket1729 · 2021-08-31T18:26:02Z

src/descriptor/tr.rs

+    <Pk as FromStr>::Err: ToString,
+    <<Pk as MiniscriptKey>::Hash as FromStr>::Err: ToString,
+{
+    pub fn new(key_path: Pk, script_path: Option<TapTree<Pk>>) -> Result<Self, Error> {


Add this check and return an error when this does not hold.
https://github.com/bitcoin/bitcoin/blob/81f4a3e84d6f30e7b12a9605dabc3359f614da93/src/script/interpreter.h#L229.

Introduce this constant in limits.rs

Right now, you can implement a simple depth/height function on Taptree.

We can cache the height/depth in the future.

sanket1729 · 2021-08-31T18:28:24Z

src/expression.rs

 impl<'a> Tree<'a> {
-    fn from_slice(sl: &'a str) -> Result<(Tree<'a>, &'a str), Error> {
-        Self::from_slice_helper(sl, 0u32)
+    pub fn from_slice(sl: &'a str) -> Result<(Tree<'a>, &'a str), Error> {


In 73dd23e, the function signature should be changed in this commit itself.

sanket1729 · 2021-08-31T18:30:31Z

src/lib.rs

-#![deny(missing_docs)]
+// #![deny(dead_code)]
+// #![deny(unused_imports)]
+// #![deny(missing_docs)]


in 73dd23e, this is fine.

Adding this change to check this is re-enabled again.

sanket1729

Left some more remarks, we are getting closer :)

sanket1729 · 2021-09-12T08:53:36Z

src/descriptor/tr.rs

+        match tree {
+            TapTree::Tree(ref left_tree, ref right_tree) => {
+                1 + max(
+                    Self::taptree_height(&**left_tree),


more idiomatic to call left_tree.taptree_height()

and same for right tree below

sanket1729 · 2021-09-12T08:55:55Z

src/descriptor/tr.rs

+        };
+
+        if nodes > TAPROOT_MAX_NODE_COUNT {
+            Ok(Self { internal_key, tree })


If condition is reversed. It should be error if nodes > TAPROOT_MAX_NODE_COUNT

sanket1729 · 2021-09-12T08:56:52Z

src/descriptor/tr.rs

+        }
+    }
+
+    pub fn get_internal_key(&self) -> Result<Pk, Error> {


There is no need for this function to return a result, it should return &Pk.

In rust, typically the name get is not added for function names. See https://rust-lang.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter if you are interested for more guidelines.

sanket1729 · 2021-09-12T08:59:12Z

src/descriptor/tr.rs

+        Ok(self.internal_key.clone())
+    }
+
+    pub fn get_taptree(&self) -> Result<TapTree<Pk>, Error> {


This should option and reference to TapTree. In general, try to avoid cloning(extra allocation) as possible

sanket1729 · 2021-09-12T09:04:34Z

src/descriptor/tr.rs

+            Tree { name, args } if name.len() > 0 && args.len() == 0 => {
+                let script = name;
+                let script = Self::parse_miniscript(script)?;
+                let script = Arc::new(script);


nit: slight code preference for inlining some operations. You don't really need the function parse_miniscript. You can call

Miniscript::<Pk, Ctx>::from_str()

to get the miniscript directly. The function already does all the checks that you implement here. Also, the method parse_method does make sense itself inside an impl for TapTree

sanket1729 · 2021-09-12T09:06:46Z

src/descriptor/tr.rs

+                let left_tree = Self::tr_script_path(&left_branch)?;
+                let right_tree = Self::tr_script_path(&right_branch)?;
+                let left_ref = Arc::new(left_tree);
+                let right_ref = Arc::new(right_tree);


nit: Prefer inlining some code instead of 6 lines.

let left = Self::tr_script_path(&args[0])?; .. Ok(TapTree::Tree(Arc::new(left_ref), Arc::new(right_ref)))

sanket1729 · 2021-09-12T09:09:41Z

src/descriptor/tr.rs

+        }
+    }
+
+    return if s.len() > 3 && &s[..3] == "tr(" && s.as_bytes()[s.len() - 1] == b')' {


assign let ret = expr; and do a return statement in the end

sanket1729 · 2021-09-12T09:18:38Z

src/descriptor/tr.rs

+        if found == inp.len() - 1 {
+            Some((&inp[..], &""))
+        } else {
+            Some((&inp[..found], &inp[found + 1..]))


Add a comment that this deals with the case when the , is the last character.

SarcasticNastik · 2021-09-16T05:47:28Z

@sanket1729 I have made the suggested changes. Apart from that,

In the tr_roundtrip_tree test, using DummyKey caused AnalysisError::RepeatedPubKeys error (I think we have to turn that off while using DummyKey) so I changed the test to use PublicKey accordingly.

sanket1729

I think this is good overall. I can take it over from here and update the PR once #255 is merged.

sanket1729 · 2021-09-17T15:45:21Z

src/descriptor/mod.rs

 mod segwitv0;
 mod sh;
 mod sortedmulti;
+mod tr;


We still need a statement with pub use self::tr::{TapTree} and all other utilities that need to be exposed.

dr-orlovsky · 2021-11-14T08:43:31Z

src/descriptor/tr.rs

+#[derive(Clone, Ord, PartialOrd, Eq, PartialEq, Hash)]
+pub enum TapTree<Pk: MiniscriptKey> {
+    Tree(Arc<TapTree<Pk>>, Arc<TapTree<Pk>>),
+    Leaf(Arc<Miniscript<Pk, Segwitv0>>),


@sanket1729: I assume we can't use Segwitv0 here. Neither Tap, since tapscript is only valid for leafs with hash version 0xC0. No idea how to solve this problem without refactoring the whole context system

@dr-orlovsky. This is being done in #278 now

sanket1729 · 2022-03-09T14:15:41Z

This commit was merged as a part of #278.

SarcasticNastik force-pushed the taproot-descriptor branch from 98c970d to 6135e57 Compare August 31, 2021 16:26

SarcasticNastik marked this pull request as ready for review August 31, 2021 17:50

sanket1729 reviewed Aug 31, 2021

View reviewed changes

SarcasticNastik force-pushed the taproot-descriptor branch from c0930e6 to 73ad38d Compare September 7, 2021 16:28

sanket1729 reviewed Sep 12, 2021

View reviewed changes

SarcasticNastik force-pushed the taproot-descriptor branch 2 times, most recently from c75ffcc to ff365fd Compare September 16, 2021 05:22

SarcasticNastik added 2 commits September 16, 2021 11:51

taproot descriptor parsing done

943f9d6

taproot descriptor: add roundtrip tests

fd12b77

SarcasticNastik force-pushed the taproot-descriptor branch from ff365fd to fd12b77 Compare September 16, 2021 06:22

sanket1729 reviewed Sep 17, 2021

View reviewed changes

dunxen mentioned this pull request Oct 2, 2021

Add Taproot support bwt-dev/bwt#91

Open

notmandatory mentioned this pull request Nov 2, 2021

Taproot / Schnorr bitcoindevkit/bdk#63

Closed

8 tasks

dr-orlovsky reviewed Jan 10, 2022

View reviewed changes

sanket1729 mentioned this pull request Jan 21, 2022

7.0 RC1 release #292

Closed

sanket1729 closed this Mar 9, 2022

Comments

Conversation

SarcasticNastik commented Aug 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Future Work

Uh oh!

SarcasticNastik commented Aug 31, 2021

Uh oh!

sanket1729 left a comment

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!

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!

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!

sanket1729 left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

SarcasticNastik commented Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanket1729 left a comment

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!

sanket1729 commented Mar 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

SarcasticNastik commented Aug 30, 2021 •

edited

Loading

SarcasticNastik commented Sep 16, 2021 •

edited

Loading