Skip to content

Conversation

@SarcasticNastik
Copy link
Contributor

@SarcasticNastik SarcasticNastik commented Apr 8, 2022

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.

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some partial review of the first few commits

},
)?;
Ok(tree)
self.is_valid()?; // Check for validity
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to re-engineer this to not disallow repeated keys in the taproot policy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: Following discussions in issue #338, we currently disallow repeated keys in taproot policy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking is as follows: we should allow repeated keys in the policy. While doing individual policy compilations to miniscript, we should check this. Not while compiling the entire tr policy. I want to make sure that we support policy compilations that allow same keys in different tree leaves.

As for this PR, this need not concern us.

@SarcasticNastik SarcasticNastik force-pushed the tr-compiler-v2 branch 3 times, most recently from 05f2431 to 219119d Compare April 19, 2022 21:17
@SarcasticNastik SarcasticNastik marked this pull request as ready for review April 19, 2022 23:25
@SarcasticNastik SarcasticNastik changed the title [DO NOT MERGE]Tr compiler v2 Tr compiler v2 Apr 19, 2022
@SarcasticNastik SarcasticNastik mentioned this pull request May 9, 2022
@SarcasticNastik SarcasticNastik force-pushed the tr-compiler-v2 branch 3 times, most recently from 01c3f71 to da16d05 Compare May 11, 2022 10:05
sanket1729 added a commit that referenced this pull request May 19, 2022
2b13694 Add non-trivial multi-node example (Aman Rojjha)
1c2a80e Add validity and malleability checks. (Aman Rojjha)
0866807 Add P2Tr compiler (Aman Rojjha)
285207e Internal-key extraction done (Aman Rojjha)
26fc574 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 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.

Tree-SHA512: 4ceca51a383f5d52b572a16937bbcc3a9c53f0247e4b6df57a6547fd0b1c7cc33ff04dd9a476914bcf6d0a09e255918b8f7ebfe176c839d6ae31c84613dce67f
@SarcasticNastik
Copy link
Contributor Author

SarcasticNastik commented May 24, 2022

@sanket1729 @apoelstra This PR is ready-for-review, given #291 has been merged now. Kindly take a look at it!
Changes in force-push:

  1. Rebase atop master

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approach and algorithm ACK. some code improvments to be done


/// [`Miniscript`] -> leaf probability in policy cache
#[cfg(feature = "compiler")]
type MsTapCache<Pk> = BTreeMap<TapTree<Pk>, f64>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when there is a collision? Making sure that we are dealing with those cases in a sane manner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure I'll update this! This was done considering that there were no duplicate keys in the initial policy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no duplicates, but still, there can be collisions in the compilation.

Copy link
Contributor Author

@SarcasticNastik SarcasticNastik Jun 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that makes sense! Thanks.

},
)?;
Ok(tree)
self.is_valid()?; // Check for validity
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking is as follows: we should allow repeated keys in the policy. While doing individual policy compilations to miniscript, we should check this. Not while compiling the entire tr policy. I want to make sure that we support policy compilations that allow same keys in different tree leaves.

As for this PR, this need not concern us.


fn main() {
let policies_str = [
"or(1@pk(A),1@pk(B))",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make this example as a tutorial.

  • Take a example policy,
  • explain what does
  • explain the compiled result.

Do this a couple of times for interesting results. Using unspendable internal key, useful internal key being inferred etc..

@SarcasticNastik
Copy link
Contributor Author

SarcasticNastik commented Jun 4, 2022

Changes in the force-push

  1. Addressing error lifting issues.
  2. Update taproot example as suggested.
  3. Handling collisions in cache and added comments for assumptions.
  4. Rebase on top of master.

@SarcasticNastik SarcasticNastik force-pushed the tr-compiler-v2 branch 4 times, most recently from 5b2f024 to 6ce9fb1 Compare June 4, 2022 13:11
@sanket1729
Copy link
Member

@SarcasticNastik, we recently merged support for no-std. This would require another rebase

@SarcasticNastik SarcasticNastik force-pushed the tr-compiler-v2 branch 2 times, most recently from 375b94b to 2940416 Compare June 6, 2022 06:07
@sanket1729
Copy link
Member

Hey @SarcasticNastik, looks are still some issues. Let me know if you are having difficulty in dealing with CI with no-std. The general thumb rules are 1) use core instead of std, 2) Common things should be available via use prelude::*

@SarcasticNastik
Copy link
Contributor Author

SarcasticNastik commented Jun 6, 2022

@sanket1729 Done :p! Thanks for the advice.

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last iteration(I think :P)

.compile_tr(Some(unspendable_key.clone()))
.unwrap();
let opt_expected_desc = Descriptor::Tr(
Tr::<String>::from_str("tr(A,{{pk(D),pk(C)},{pk(B),and_v(v:pk(E),pk(F))}})")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we succeed in finding any policy where these differs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did find policies where the TapTree leaf nodes re-arranged, but none yet where we noticed policy-merging. One of those were:
Policy: thresh(1,or(1@pk(A),1@pk(B)),or(1@pk(C),1@or(1@and(pk(E),pk(F)),1@pk(D))))
Private compilation: tr(A,{{and_v(v:pk(E),pk(F)),pk(D)},{pk(C),pk(B)}})#y7cmc3xl
Default compilation: tr(A,{{pk(D),pk(C)},{pk(B),and_v(v:pk(E),pk(F))}})#vv7casf3

@SarcasticNastik SarcasticNastik force-pushed the tr-compiler-v2 branch 2 times, most recently from d4f3245 to 5c456d7 Compare June 9, 2022 13:31
@SarcasticNastik
Copy link
Contributor Author

SarcasticNastik commented Jun 9, 2022

Changes in the force-push

  • Fix invalid parent compilation bug.
  • Use serialized keys for creating pubkeys.

@SarcasticNastik SarcasticNastik force-pushed the tr-compiler-v2 branch 3 times, most recently from 1c34908 to faa4e16 Compare June 17, 2022 11:58
@SarcasticNastik
Copy link
Contributor Author

SarcasticNastik commented Jun 17, 2022

@sanket1729 I've added the document explaining how the private compilation is efficient too and refactored the example.
I'd also appreciate comments on the doc and address the ambiguities and issues, if any.

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last few comments. Almost there

/// the respective miniscripts. A Huffman Tree is created from this vector which optimizes over
/// the probabilitity of satisfaction for the respective branch in the TapTree.
///
/// Refer to [doc/Tr compiler.pdf] in the root of the repository to understand why such
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, we also want the source code of this uploaded somewhere where I can copy/edit it. In case, we find some errors in the document, there would be no way to edit it.

Can you create a new repo with the code for this? And also link the source here

Copy link
Contributor Author

@SarcasticNastik SarcasticNastik Jun 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made the gist here and linked it in the docs.

This write-up is aimed to reason about how the private-compilation (as defined in doc) is also cost-efficient.
Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK ef4249d

@sanket1729 sanket1729 merged commit 0662d2e into rust-bitcoin:master Jun 17, 2022
@SarcasticNastik SarcasticNastik deleted the tr-compiler-v2 branch June 18, 2022 06:06
sanket1729 added a commit that referenced this pull request Jun 20, 2022
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
heap-coder added a commit to heap-coder/rust-miniscript that referenced this pull request Sep 27, 2025
ef4249d0989dac009c99fada4c5324c523019fd4 Add taproot compiler example usage (Aman Rojjha)
cef2a5b604de584518b3a9574a86fb1c449b65e7 Add Tr-compiler write-up and doc-comment (Aman Rojjha)
5233c6645581e6e5a5afee55946193210f16b7d4 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 ef4249d0989dac009c99fada4c5324c523019fd4

Tree-SHA512: ae5949b5170ff4cc8202434655af41b4938e96801209896d117bf4e4bd85c0becc6fd0c7affb100c31655f0085c1ff5e7c19184db3433b2831f73db22d94348d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants