Tr compiler v3 - Incremental Enumerative Compiler by SarcasticNastik · Pull Request #418 · rust-bitcoin/rust-miniscript

SarcasticNastik · 2022-05-25T01:03:31Z

This PR is a follow-up in the Tr-compiler series. This introduces splitting of a thresh(k, ...n...) policy into different Tapleaves.

For now, kindly refer to the code documentation and comments for the strategy implemented

sanket1729

Did some partial review.

sanket1729 · 2022-06-26T19:22:18Z

src/policy/concrete.rs

            )),
            _ => {
                let (internal_key, policy) = self.clone().extract_key(unspendable_key)?;
+                policy.check_tapleaf()?;


Should update the name of the function. This implies that we can checking something on tapleafs. Perhaps, check_num_tapleaves?
I would prefer to inline the check_num_tapleaves here, but up to you.

Yes, the function name is a misnomer. Will change the name as suggested.

sanket1729 · 2022-06-26T19:22:41Z

src/policy/concrete.rs

+    /// Check on the number of TapLeaves
+    #[cfg(feature = "compiler")]
+     fn check_tapleaf(&self) -> Result<(), Error> {
+        if self.num_tap_leaves() > 100_000 {


Make this is local constant MAX_COMPILATION_TAP_LEAVES

sanket1729 · 2022-06-26T19:23:16Z

src/policy/concrete.rs

+    #[cfg(feature = "compiler")]
+     fn check_tapleaf(&self) -> Result<(), Error> {
+        if self.num_tap_leaves() > 100_000 {
+            return Err(errstr("Too many Tapleaves"));


Let's use a dedicated error variant instead of errstr

This is still not addressed.

sanket1729 · 2022-06-26T19:23:48Z

src/policy/concrete.rs

-    Ok(node)
+
+#[cfg(feature = "compiler")]
+fn combination(n: usize, k: usize) -> usize {


Whenever we call this, we need to make sure that we are having an overflow.

This function isn't supposed to be there. Seems I messed up while squashing the commits, my bad!

sanket1729 · 2022-06-26T19:24:50Z

src/policy/concrete.rs

+    fn num_tap_leaves(&self) -> usize {
+        match self {
+            Policy::Or(subs) => subs.iter().map(|(_prob, pol)| pol.num_tap_leaves()).sum(),
+            Policy::Threshold(k, subs) if *k == 1 => combination(subs.len(), *k),


What happens when there is overflow here? We should use all the checked_sum, checked_mul variants here to check for this. Return Option where we return None when there is overflow

sanket1729 · 2022-06-26T19:28:53Z

src/policy/concrete.rs

-        match *self {
+        match self {
            Policy::Or(ref subs) => {
                let total_odds: usize = subs.iter().map(|(ref k, _)| k).sum();


remove the ref k here.

As in, remove the ref . It should clean up * (*k) code in the next line

sanket1729 · 2022-06-26T19:33:01Z

src/policy/concrete.rs

                    .collect::<Vec<_>>()
            }
-            ref x => vec![(prob, x.clone())],
+            // Instead of the fixed-point algorithm, consider direct k-choose-n split if limits


I don't see why this comment is relevant here. This function is only splitting ors AFAIU.

sanket1729 · 2022-06-26T19:33:35Z

src/policy/concrete.rs

+    /// Given a [`Policy`], return a vector of policies whose disjunction is isomorphic to the initial one.
+    /// This function is supposed to incrementally expand i.e. represent the policy as disjunction over
+    /// sub-policies output by it. The probability calculations are similar as
+    /// [to_tapleaf_prob_vec][`Policy::to_tapleaf_prob_vec`]


Why do you have the link twice?

sanket1729 · 2022-06-26T19:36:26Z

src/policy/concrete.rs

+            Policy::Or(subs) => {
+                let total_odds = subs.iter().fold(0, |acc, x| acc + x.0);
+                subs.iter()
+                    .map(|(odds, pol)| (prob * *odds as f64 / total_odds as f64, pol.clone()))


Should this not be recursive?

sanket1729 · 2022-06-26T19:36:39Z

src/policy/concrete.rs

+            Policy::Threshold(k, subs) if *k == 1 => {
+                let total_odds = subs.len();
+                subs.iter()
+                    .map(|pol| (prob / total_odds as f64, pol.clone()))


Same comment here

sanket1729

code logic ACK, but would need some re-factors that will make it faster

sanket1729 · 2022-07-08T20:59:16Z

src/miniscript/limits.rs

 pub const MAX_PUBKEYS_PER_MULTISIG: usize = 20;
+
+/// Maximum TapLeafs allowed in a compiled TapTree
+pub(crate) const MAX_COMPILATION_LEAVES: usize = 10_000;


These constants are constants related to consensus standardness limits of bitcoin and are not specific to rust-miniscript. We should move MAX_COMPILATION_LEAVES to compiler.rs

Should we move it to concrete.rs since that's where the tr compiler is?

sanket1729 · 2022-07-08T21:28:59Z

src/policy/concrete.rs

+/// any one of the conditions exclusively.
+#[cfg(feature = "compiler")]
+fn generate_combination<Pk: MiniscriptKey>(
+    policy_vec: &Vec<Policy<Pk>>,


We should use Rc instead of cloning it here multiple times. This should help in space and time complexity

We would need a new data structure with Arc's here to store these instead of Policy. But we do not want so many clones here. This might be why everything is slow.

You can create something similar to Policy but with Arcs instead of Owned sub-policies.

SarcasticNastik · 2022-08-06T14:37:43Z

@sanket1729 I've added a new structure PolicyArc<Pk> according to your suggestions. But I didn't find any significant improvements for the compilation time.

sanket1729

mostly ACK. found a bug when dealing with more than MAX_COMPILATION_LEAVES part.

See the last commit here https://github.com/sanket1729/rust-miniscript/tree/pr418.

We are really close to merging this. I think you would run in --release mode to see the performance difference.

SarcasticNastik · 2022-08-11T12:50:29Z

src/policy/concrete.rs

+            }
+
+            // --- Sanity Checks ---
+            if enum_len > MAX_COMPILATION_LEAVES || *curr_policy == PolicyArc::Unsatisfiable {


We do not enumerate the sub-policy under consideration since we either exceed MAX_COMPILATION_LEAVES or the policy itself compiled to Unsatisfiable.

This was causing bugs because the enumeration was incorrect. I think the logic here would need to be improved. If you replace thresh(51,...) with thresh(49) you would see a crash

This was a major bug. I've fixed it in the 9c8a1c3.

sanket1729

Last few comments.

sanket1729 · 2022-08-14T06:15:16Z

src/policy/concrete.rs

+    #[cfg(feature = "compiler")]
+     fn check_tapleaf(&self) -> Result<(), Error> {
+        if self.num_tap_leaves() > 100_000 {
+            return Err(errstr("Too many Tapleaves"));


This is still not addressed.

sanket1729 · 2022-08-14T06:17:21Z

src/policy/concrete.rs

+            if enum_len > MAX_COMPILATION_LEAVES || *curr_policy == PolicyArc::Unsatisfiable {
+                // Root-level enumeration exceeds the limit, so push back the initial policy
+                if ret.is_empty() {
+                    ret.push((1.0, arc_self));


This is not the expected behavior. We need to split as much as we can until we hit the limit.

The variable enum_len stores the vector length if we consider the enumeration of plausible node. If this exceeds limit, we don't replace the initial plausible sub-policy by its enumerations (which is done after the check). I just handled the base-case where the root-level enumeration itself exceeds the limit, where we just return the policy itself.

sanket1729

ACK f9901ab. Can you also rebase to latest master? I think we are good to go here

sanket1729 · 2022-09-16T12:24:07Z

Unless @kanishk779 found something else in review here.

sanket1729 · 2022-09-17T04:57:15Z

I will wait a couple of days for @kanishk779 to respond. Will merge this later

sanket1729 · 2022-09-17T05:54:16Z

@SarcasticNastik , can you also update the PR description?

sanket1729

ACK 265b063

sanket1729 · 2022-09-20T10:24:15Z

@SarcasticNastik , sorry for this. But I promise, this would be last rebase

sanket1729

ACK 97de242

3f451db Release 8.0.0 (sanket1729) Pull request description: Draft release PR. Still blocked on reviews of 1) #418 2) #450 3) #461 ACKs for top commit: apoelstra: ACK 3f451db Tree-SHA512: aa455fd57a894b1edb49b6273126a05c9d0897ca50ee572fa8a59b4d2a96e2013f2ed9ec4b96cd5e449ba5b5ee6bd32aa57da110df69bd35b8a970ccb1440002

…022_10 51d66d1 Merge rust-bitcoin/rust-miniscript#418: Tr compiler v3 - Incremental Enumerative Compiler

…Enumerative Compiler 97de242709178f2c75c8bffbe44718356aae1e71 Implement tr-compiler-v3 enumeration compiler (Aman Rojjha) 7d5374f223687d01b82412f0da80ef407c0ad745 Limit the number of tap-leaves output by the `tr_compiler` (Aman Rojjha) Pull request description: This PR is a follow-up in the Tr-compiler series. This introduces splitting of a `thresh(k, ...n...)` policy into different Tapleaves. > For now, kindly refer to the code documentation and comments for the strategy implemented ACKs for top commit: sanket1729: ACK 97de242709178f2c75c8bffbe44718356aae1e71 Tree-SHA512: dfa4517bb211e329e57dd237998e8df219cdf6208f8ec2cb3f2f4f9cf8934f272fb611f3b52dc1ad080918ddbc4fbd3ffb506812f02fa536239402a62683552d

SarcasticNastik changed the title ~~Tr compiler - Private thresh-enumeration~~ [DO NOT MERGE]Tr compiler - Private thresh-enumeration May 25, 2022

SarcasticNastik marked this pull request as draft May 25, 2022 03:25

SarcasticNastik force-pushed the draft/tr-compiler-v3 branch 3 times, most recently from cb5cf91 to 3d473c9 Compare June 21, 2022 18:59

sanket1729 reviewed Jun 26, 2022

View reviewed changes

SarcasticNastik force-pushed the draft/tr-compiler-v3 branch from 3d473c9 to 788a03b Compare June 27, 2022 18:28

sanket1729 reviewed Jul 8, 2022

View reviewed changes

SarcasticNastik force-pushed the draft/tr-compiler-v3 branch 2 times, most recently from 1355757 to fee0f7c Compare August 6, 2022 14:36

sanket1729 reviewed Aug 11, 2022

View reviewed changes

SarcasticNastik force-pushed the draft/tr-compiler-v3 branch from fee0f7c to a4cf8b5 Compare August 11, 2022 12:48

SarcasticNastik commented Aug 11, 2022

View reviewed changes

SarcasticNastik changed the title ~~[DO NOT MERGE]Tr compiler - Private thresh-enumeration~~ Tr compiler v3 - Incremental Enumerative Compiler Aug 11, 2022

SarcasticNastik marked this pull request as ready for review August 11, 2022 12:51

SarcasticNastik force-pushed the draft/tr-compiler-v3 branch from a4cf8b5 to 2186b09 Compare August 13, 2022 10:16

sanket1729 reviewed Aug 14, 2022

View reviewed changes

SarcasticNastik force-pushed the draft/tr-compiler-v3 branch 6 times, most recently from a7b1e82 to 71e4f81 Compare August 20, 2022 19:30

SarcasticNastik force-pushed the draft/tr-compiler-v3 branch from 71e4f81 to f9901ab Compare August 21, 2022 23:46

sanket1729 reviewed Sep 16, 2022

View reviewed changes

SarcasticNastik force-pushed the draft/tr-compiler-v3 branch from f9901ab to 265b063 Compare September 16, 2022 13:44

sanket1729 previously approved these changes Sep 19, 2022

View reviewed changes

sanket1729 added this to the 8.0.0 milestone Sep 19, 2022

sanket1729 mentioned this pull request Sep 19, 2022

Release 8.0.0 #462

Merged

Limit the number of tap-leaves output by the tr_compiler

7d5374f

SarcasticNastik dismissed sanket1729’s stale review via 55b48fc September 27, 2022 04:50

SarcasticNastik force-pushed the draft/tr-compiler-v3 branch from 265b063 to 55b48fc Compare September 27, 2022 04:50

Implement tr-compiler-v3 enumeration compiler

97de242

SarcasticNastik force-pushed the draft/tr-compiler-v3 branch from 55b48fc to 97de242 Compare September 27, 2022 08:34

sanket1729 approved these changes Sep 28, 2022

View reviewed changes

sanket1729 merged commit 51d66d1 into rust-bitcoin:master Sep 28, 2022

Conversation

SarcasticNastik commented May 25, 2022 • 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!

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!

SarcasticNastik commented Aug 6, 2022

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 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 left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

sanket1729 commented Sep 17, 2022

Uh oh!

sanket1729 commented Sep 17, 2022

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 commented Sep 20, 2022

SarcasticNastik commented May 25, 2022 •

edited

Loading

sanket1729 commented Sep 16, 2022 •

edited

Loading