Taproot Compiler #291

SarcasticNastik · 2022-01-16T16:21:33Z

This PR builds on top of #278.

This is one in a series of PRs aimed to implement a feature-complete Pay-to-Taproot P2Tr compiler.
Specifically, this introduces a basic compilation for a given policy by collecting the leaf nodes of the tree generated by considering root-level disjunctive ORand using this to generate a TapTree.

Assuming that duplicate keys are NOT possible even in different branches of the TapTree.

Follow Up PRs

Tr compiler v2 #342 - Uses heuristic for tree-generation/ merging algorithm while buillding TapTree to optimize over the expected average total cost.
Allow direct compilation of a Policy into a valid Descriptor #352
A future PR implementing enumerative strategies for optimizing thresh k-of-n and similar structures.

sanket1729

Did an initial review. I think we would need a few more iterations before we get this.

sanket1729 · 2022-01-16T23:38:01Z

src/policy/compiler.rs

                .collect();

-            if key_vec.len() == subs.len() && subs.len() <= MAX_PUBKEYS_PER_MULTISIG {
+            if Ctx::sig_type() == SigType::Schnorr {


nit: prefer explicit matching over all script arms instead of if else

Refactored in b36c116

sanket1729 · 2022-01-16T23:39:48Z

src/policy/concrete.rs

+#[cfg(feature = "compiler")]
 use Miniscript;
+#[cfg(feature = "compiler")]
+use Tap;


nit: You can import all of these in one line.

#[cfg(feature = "compiler")] use {}

Refactored in b36c116

sanket1729 · 2022-01-16T23:40:50Z

src/policy/concrete.rs

 impl<Pk: MiniscriptKey> Policy<Pk> {
+    /// Single-Node compilation
+    #[cfg(feature = "compiler")]
+    fn compile_huffman_taptree(policy: &Self) -> Result<TapTree<Pk>, Error> {


It is a convention to use &self instead of &Self. &self stands for self: &Self

This function is called compile_huffman. I expect in the next commit it would do something different

Right. The naming convention for the function was kept keeping future in mind. I'll rename it to compile_leaf_taptree for proper naming convention for this commit.

sanket1729 · 2022-01-16T23:44:34Z

src/policy/concrete.rs

+    }
+    /// Extract the Taproot internal_key from policy tree.
+    #[cfg(feature = "compiler")]
+    fn extract_key(policy: &Self, unspendable_key: Option<Pk>) -> Result<(Pk, &Policy<Pk>), Error> {


Some comment about &Self. This function should consume self. Functionally this should change the policy and extra the internal key out of it. I think this maybe done in the next commit

This would require us to make the compile_tr function consume self which doesn't align with the compile<Pk, Ctx>(&self) function. I will update this function as suggested and use a cloned copy for the same until we discuss further.

src/policy/concrete.rs

sanket1729 · 2022-01-16T23:53:20Z

src/policy/semantic.rs

-                1,
-                vec![Policy::KeyHash("".to_owned()), Policy::Older(1000),]
-            )
+            Policy::Threshold(1, vec![Policy::KeyHash("".to_owned()), Policy::Older(1000)])


These changes in semantic.rs apart from pub(crate) one seem unnecessary?

Yes. This code snippet was automatically formatted by cargo fmt (version: 1.58.0).

It was necessary to keep this change to pass the linter.

I see this is because you removed the "," at the end of all lines. This change is okay, but it is not a part of this commit. But I can live with this

i would recommend to bump 1.58.0 and then cargo fmt as one commit maybe?

src/policy/concrete.rs

sanket1729 · 2022-01-17T00:06:22Z

src/policy/concrete.rs

+
+        for key in concrete_keys.iter() {
+            if semantic_policy
+                .clone()


This algorithm is poor in memory performance because it calls clone for every single iteration. We can do a new tree traversal for every key we see. I think we will eventually need a better algorithm that avoids all these allocations.

We can first focus on the correctness/code hygine of the PR, then address these efficiency issues.

src/policy/concrete.rs

sanket1729

Very close. Last few comments.

src/policy/compiler.rs

src/policy/concrete.rs

sanket1729 · 2022-01-18T00:37:06Z

src/policy/concrete.rs

+            .pop()
+            .expect("huffman tree algorithm is broken")
+            .1;
+        Ok((*node).clone())


suggested how to fix it above.

sanket1729 · 2022-01-18T00:44:25Z

src/policy/semantic.rs

-                1,
-                vec![Policy::KeyHash("".to_owned()), Policy::Older(1000),]
-            )
+            Policy::Threshold(1, vec![Policy::KeyHash("".to_owned()), Policy::Older(1000)])


I see this is because you removed the "," at the end of all lines. This change is okay, but it is not a part of this commit. But I can live with this

SarcasticNastik · 2022-02-25T05:56:43Z

The Lint+Fuzz test fails here because of this issue. Locally running the scripts work just fine.
Edit: #300 fixes the issue. Cherry-picked the commit into this branch.

src/policy/compiler.rs

JeremyRubin · 2022-03-09T17:17:22Z

shouldn't this try to compile the common: And(Or(a,b), Or(c,d)) to: ac ad bc bd?

how do we judge if the increased number of nodes vs script length cutoff makes sense?

It seems like there is a script length beyond which it always makes sense, maybe like 32 byte script?

the issue is that many scripts it'd be exp to actually do this unrolling, but it's what i'd expect.

SarcasticNastik · 2022-03-16T20:05:36Z

@JeremyRubin You're right in the intuitive analysis of the method taproot compiler should ideally adopt. I plan on introducing this type to reduction/ enumeration of policy nodes in a follow-up PR. The reason to keep that on hold was to understand if doing so would turn out to be intractable. For example:- Consider a generic thresh(k, ...) k-of-n keys scenario. Enumerating this as separate nodes in disjunction results in nCk (exponential in k). This can very easily blow up. Any suggestions on this are welcome!
In addition, this PR just serves to provide one of the many compilation methods we plan to try/test upon. Another version of this compiler is underway which uses a heuristic to re-arrange the policy nodes and only compile the nodes (A and B) separately if resulting expected total cost (script_size + average satisfaction cost) of compilation into a taptree is lesser than compilation of a combination of the two (Or(A, B)).

dr-orlovsky · 2022-03-28T18:24:56Z

@sanket1729 is there plans of getting this into the release?

sanket1729 · 2022-03-28T18:29:06Z

@dr-orlovsky, no this won't go into the release. There is still some work to be done here, and the compiler implemented here is feature complete. We still have a few things we want before we support this.

Maybe we will have another release with support for partial descriptors, unspendable key support, musig descriptors etc... Possibly 1 month from now

SarcasticNastik · 2022-04-03T21:59:22Z

src/policy/concrete.rs

+            let mut prob = 0.;
+            let semantic_policy = self.lift()?;
+            let concrete_keys = self.keys();
+            let key_prob_map: HashMap<_, _> = self


The logic of internal-key extraction has been changed from selecting first-encountered key satisfying the policy to most-probable key satisfying the policy.

I see the ertract key is updated in next commit. No need to fix it here.

sanket1729 · 2022-04-30T02:26:10Z

Can you rebase on top of master.? And remove the last CI commit.

src/policy/concrete.rs

sanket1729 · 2022-04-30T02:29:22Z

src/policy/concrete.rs

+        f: T,
+    ) -> Result<TapTree<Pk>, Error>
+    where
+        T: Fn(OrdF64) -> OrdF64,


Let's not have this confusing argument for now. And hard-code whatever dummy metric we have

src/policy/concrete.rs

SarcasticNastik · 2022-05-02T15:41:08Z

Changes in the force update:

Updated documentation .
Rebased on top of master and removed CI commit.
Moved with_huffman_tree out of the impl block.
Seperate internal_key_extraction and taptree compilation into different commits.

sanket1729

Looks mostly good

src/policy/concrete.rs

sanket1729 · 2022-05-09T16:13:09Z

@SarcasticNastik can you also add in the description how all the PRs relate and what they do.

src/policy/concrete.rs

sanket1729 · 2022-05-10T20:07:35Z

Sorry, @SarcasticNastik this would require a rebase again :( .

@apoelstra requesting your review. This is a major contribution from a new contributor and I don't want to go ahead without your review. The commit messages and PR description are helpful in understanding how this is structured.

SarcasticNastik · 2022-05-10T20:41:49Z

@sanket1729 I've rebased the PR onto current master. @apoelstra A kind request for your review.

EDIT
Changes in the latest force-push 2b13694:

Minor changes in doc-comments .
Rebase on top of current master.

A new method `compile_tr` is introduced in policy for creating a Tr descriptor with TapTree containing single script child-node. An internal-key extraction method from the policy has also been implemented for the same.

The internal-key extraction method now selects the most-likely key (in the policy) as suggested by BIP341.

Introduce a `compiler_tr` API for compiling a policy to a Tr Descriptor.

Testing done.

apoelstra

ACK 2b13694

sanket1729

ACK 2b13694 . Let's push forward on this. In the interest of ACKs and multiple rebases already done here, I am merging this. We recently shifted to rust 2018. And this code has a bunch of warnings because of it.

62a42bc Fix compiler warnings in PR #291 (Aman Rojjha) Pull request description: ACKs for top commit: sanket1729: utACK 62a42bc Tree-SHA512: c64f51a5d9637bd8f511a200c0ef60cd66263e28b19951077ba43b4232a3fc7bc45381def7cea8386fc65adea150c99ea5d768599aa31b699c31e42c3f121fe2

ef4249d Add taproot compiler example usage (Aman Rojjha) cef2a5b Add Tr-compiler write-up and doc-comment (Aman Rojjha) 5233c66 Add Taproot compiler API (Aman Rojjha) Pull request description: This PR builds on top of #291. This aims to introduce an efficient version of the tapscript compiler by using a few heuristics to optimize over the expected average total cost for the TapTree. ## Strategy implemented - While merging TapTrees `A` and `B`, check whether the compilation of `Policy::Or(policy(A), policy(B))` is more efficient than a TapTree with the respective children `A` and `B`. **Note**: This doesn't include the `thresh(k, ...n..)` enumeration strategy. Planning on working on it separately. ACKs for top commit: sanket1729: ACK ef4249d Tree-SHA512: ae5949b5170ff4cc8202434655af41b4938e96801209896d117bf4e4bd85c0becc6fd0c7affb100c31655f0085c1ff5e7c19184db3433b2831f73db22d94348d

169d849 Add policy to descriptor target compilation method (Aman Rojjha) Pull request description: This PR works on top of #291 and #342. It introduces a new function `compile_to_descriptor` which requires a `DescriptorCtx` (suggestions for a better enum name are welcome!) allowing directly compilation of a policy into the specified descriptor type. ACKs for top commit: apoelstra: ACK 169d849 sanket1729: ACK 169d849 Tree-SHA512: 0f895f4774ea4f56db76280ac756df616a70e5d7d60cc128f9a050ef86658f786b2d45885d748dba51751146a6e93b3731f4ae193b7d80284ffaea20be1e8f92

2b1369409adb49c838253572a59d05b00060df23 Add non-trivial multi-node example (Aman Rojjha) 1c2a80e33cbc183e4db5ce74a4c40a11b0a1217e Add validity and malleability checks. (Aman Rojjha) 0866807031ade6879ca4ef7d6cfac6ac5f14140d Add P2Tr compiler (Aman Rojjha) 285207e26f25939e9108c810f573ca6d74d3f230 Internal-key extraction done (Aman Rojjha) 26fc574434e90117280c9579d28bbcccb1d300d5 Policy to single-leaf TapTree compilation done (Aman Rojjha) Pull request description: This PR builds on top of #278. This is one in a series of PRs aimed to implement a feature-complete *Pay-to-Taproot* **P2Tr** compiler. Specifically, this introduces a basic compilation for a given policy by collecting the leaf nodes of the *tree* generated by considering root-level disjunctive `OR`and using this to generate a `TapTree`. > Assuming that _duplicate keys_ are **NOT** possible even in different branches of the TapTree. # Follow Up PRs - #342 - Uses heuristic for tree-generation/ _merging_ algorithm while buillding `TapTree` to optimize over the *expected average total cost*. - #352 - A future PR implementing enumerative strategies for optimizing `thresh` `k`-of-`n` and similar structures. ACKs for top commit: apoelstra: ACK 2b1369409adb49c838253572a59d05b00060df23 sanket1729: ACK 2b1369409adb49c838253572a59d05b00060df23 . Let's push forward on this. In the interest of ACKs and multiple rebases already done here, I am merging this. We recently shifted to rust 2018. And this code has a bunch of warnings because of it. Tree-SHA512: 4ceca51a383f5d52b572a16937bbcc3a9c53f0247e4b6df57a6547fd0b1c7cc33ff04dd9a476914bcf6d0a09e255918b8f7ebfe176c839d6ae31c84613dce67f

sanket1729 reviewed Jan 17, 2022

View reviewed changes

SarcasticNastik force-pushed the tr-compiler branch from b10cb7a to e507066 Compare January 17, 2022 12:25

sanket1729 reviewed Jan 18, 2022

View reviewed changes

SarcasticNastik force-pushed the tr-compiler branch 3 times, most recently from 1a1d192 to 4358e88 Compare January 18, 2022 14:46

SarcasticNastik force-pushed the tr-compiler branch from 4358e88 to f47ba52 Compare February 25, 2022 05:48

SarcasticNastik marked this pull request as ready for review February 25, 2022 05:56

SarcasticNastik force-pushed the tr-compiler branch 3 times, most recently from 255fbed to 4b8c5fe Compare March 4, 2022 08:28

JeremyRubin reviewed Mar 9, 2022

View reviewed changes

src/policy/compiler.rs Outdated Show resolved Hide resolved

SarcasticNastik force-pushed the tr-compiler branch from 4b8c5fe to 96b4e66 Compare April 3, 2022 21:54

SarcasticNastik commented Apr 3, 2022

View reviewed changes

SarcasticNastik force-pushed the tr-compiler branch from 96b4e66 to 6e5a600 Compare April 6, 2022 22:32

This was referenced Apr 8, 2022

Tr compiler v2 #342

Merged

Update to bitcoin 0.28.0 release #337

Closed

SarcasticNastik mentioned this pull request Apr 19, 2022

Allow direct compilation of a Policy into a valid Descriptor #352

Merged

sanket1729 reviewed Apr 30, 2022

View reviewed changes

SarcasticNastik force-pushed the tr-compiler branch from 6e5a600 to ad6c02b Compare May 2, 2022 15:36

SarcasticNastik force-pushed the tr-compiler branch 2 times, most recently from 0ec0202 to 658a66b Compare May 4, 2022 13:39

sanket1729 reviewed May 9, 2022

View reviewed changes

src/policy/concrete.rs Outdated Show resolved Hide resolved

src/policy/concrete.rs Outdated Show resolved Hide resolved

SarcasticNastik force-pushed the tr-compiler branch 2 times, most recently from 3e9214c to 08c6530 Compare May 9, 2022 22:21

sanket1729 reviewed May 10, 2022

View reviewed changes

src/policy/concrete.rs Outdated Show resolved Hide resolved

SarcasticNastik force-pushed the tr-compiler branch from 08c6530 to e08486d Compare May 10, 2022 20:37

SarcasticNastik force-pushed the tr-compiler branch from e08486d to 4831134 Compare May 11, 2022 08:07

SarcasticNastik added 5 commits May 11, 2022 14:36

Policy to single-leaf TapTree compilation done

26fc574

A new method `compile_tr` is introduced in policy for creating a Tr descriptor with TapTree containing single script child-node. An internal-key extraction method from the policy has also been implemented for the same.

Internal-key extraction done

285207e

The internal-key extraction method now selects the most-likely key (in the policy) as suggested by BIP341.

Add P2Tr compiler

0866807

Introduce a `compiler_tr` API for compiling a policy to a Tr Descriptor.

Add validity and malleability checks.

1c2a80e

Testing done.

Add non-trivial multi-node example

2b13694

SarcasticNastik force-pushed the tr-compiler branch from 4831134 to 2b13694 Compare May 11, 2022 09:22

apoelstra approved these changes May 18, 2022

View reviewed changes

sanket1729 approved these changes May 19, 2022

View reviewed changes

sanket1729 merged commit 7efe5e0 into rust-bitcoin:master May 19, 2022

SarcasticNastik added a commit to SarcasticNastik/rust-miniscript that referenced this pull request May 19, 2022

Fix compiler warnings in PR rust-bitcoin#291

62a42bc

SarcasticNastik deleted the tr-compiler branch June 3, 2022 11:02

joemphilips pushed a commit to joemphilips/rust-miniscript that referenced this pull request Dec 9, 2022

Fix compiler warnings in PR rust-bitcoin#291

308d8b7

Taproot Compiler #291

Taproot Compiler #291

Conversation

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

Uh oh!

Follow Up PRs

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SarcasticNastik Jan 17, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SarcasticNastik commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

JeremyRubin commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

dr-orlovsky commented Mar 28, 2022

Uh oh!

sanket1729 commented Mar 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanket1729 commented Apr 30, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SarcasticNastik commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SarcasticNastik commented Jan 16, 2022 •

edited

Loading

SarcasticNastik Jan 17, 2022 •

edited

Loading

SarcasticNastik commented Feb 25, 2022 •

edited

Loading

JeremyRubin commented Mar 9, 2022 •

edited

Loading

SarcasticNastik commented Mar 16, 2022 •

edited

Loading

SarcasticNastik commented May 2, 2022 •

edited

Loading

SarcasticNastik commented May 10, 2022 •

edited

Loading

sanket1729 left a comment •

edited

Loading