Merge rust-bitcoin/rust-miniscript#594: Implement TreeLike for policy::Concrete

1efc7a3d3a50ea7ea775a15ba486cfb7b8702623 Refactor check_timelocks helper (Tobin C. Harding)
4749931b389a7393b1bdf66770d52db7e5eae816 Use TreeLike to implement check_timelocks_helper (Tobin C. Harding)
1cbc018bd067739962fff4f72af2892c48348c8b Add unit test for check_timelocks API (Tobin C. Harding)
64488d6f9b2e3a0ff4a4afad78eb722c17f82bca Use TreeLike to implement translate_pk (Tobin C. Harding)
3282193c74b0b4297fee9390dccafd59a2851e67 Add basic unit test for translate_pk (Tobin C. Harding)
c81389f1b447cf589c84f1aa3dd1e24a549ea262 Use TreeLike to implement for_each_key (Tobin C. Harding)
b3c0955eed3f905d6739c355bfdce84bdcc1c5b6 Add unit test coverage for_each_key (Tobin C. Harding)
9280db237ea500b6a196190a05409989985c5580 Use TreeLike to implement num_tap_leaves (Tobin C. Harding)
3fc604af009265c3b03d273c4426539ce2017c02 Add basic unit test for num_tap_leaves (Tobin C. Harding)
b7f893911963848a2e164cf170d62e97ab5fc427 Use TreeLike to implement keys (Tobin C. Harding)
984576c3249b4b2fbfbe67d217a102ad71e68d42 Add basic unit test for keys (Tobin C. Harding)
2b11f64cd638b24117f4c918f781d9853d43e635 Use TreeLike to implement translate_unsatisfiable_pk (Tobin C. Harding)
4d22908d8b989641377997b5809372ae4c4653f2 Add basic unit test for translate_unsatisfiable_pk (Tobin C. Harding)
578dc6992798833dfdf503c11e9694fffaa81286 Remove code comment from soon-to-be duplicated pattern (Tobin C. Harding)
277b58777b2659cddcbcf464d19ebd0e2590cc20 Implement TreeLike for policy::Concrete (Tobin C. Harding)
82c0eee458dcee1e98482d2376240b4470f12e25 Remove PolicyArc (Tobin C. Harding)

Pull request description:

  Make a start on removing recursion in the `policy::concrete` module.

  - Patch 1: Remove`PolicyArc`
  - Patch2: Implement `TreeLike` for `concrete::Policy` (does not use it)
  - Patch 3: Trivial code comment deletion
  - Subsequent patches are pairs
     - Add a basic unit test for the function we are about to patch
     - Remove recursion from a function (the one we just tested)
  - Final patch is a refactoring

  There is still some to go but this is already a pretty heavy review burden.

ACKs for top commit:
  apoelstra:
    ACK 1efc7a3d3a50ea7ea775a15ba486cfb7b8702623
  sanket1729:
    ACK 1efc7a3d3a50ea7ea775a15ba486cfb7b8702623

Tree-SHA512: 529fcf60adda1ff43a95ee896dccac746870b527fec5a638e2377206220aa87369132d037a44a048ca25fc67df1b6309d53d8fdfb7134299421396538ec043d4

Author: heap-coder

src/iter/mod.rs

@@ -15,7 +15,7 @@ pub use tree::{
 };
 
 use crate::sync::Arc;
-use crate::{Miniscript, MiniscriptKey, ScriptContext, Terminal};
+use crate::{policy, Miniscript, MiniscriptKey, ScriptContext, Terminal};
 
 impl<'a, Pk: MiniscriptKey, Ctx: ScriptContext> TreeLike for &'a Miniscript<Pk, Ctx> {
     fn as_node(&self) -> Tree<Self> {
@@ -68,3 +68,29 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> TreeLike for Arc<Miniscript<Pk, Ctx>
         }
     }
 }
+
+impl<'a, Pk: MiniscriptKey> TreeLike for &'a policy::Concrete<Pk> {
+    fn as_node(&self) -> Tree<Self> {
+        use policy::Concrete::*;
+        match *self {
+            Unsatisfiable | Trivial | Key(_) | After(_) | Older(_) | Sha256(_) | Hash256(_)
+            | Ripemd160(_) | Hash160(_) => Tree::Nullary,
+            And(ref subs) => Tree::Nary(subs.iter().map(Arc::as_ref).collect()),
+            Or(ref v) => Tree::Nary(v.iter().map(|(_, p)| p.as_ref()).collect()),
+            Threshold(_, ref subs) => Tree::Nary(subs.iter().map(Arc::as_ref).collect()),
+        }
+    }
+}
+
+impl<'a, Pk: MiniscriptKey> TreeLike for Arc<policy::Concrete<Pk>> {
+    fn as_node(&self) -> Tree<Self> {
+        use policy::Concrete::*;
+        match self.as_ref() {
+            Unsatisfiable | Trivial | Key(_) | After(_) | Older(_) | Sha256(_) | Hash256(_)
+            | Ripemd160(_) | Hash160(_) => Tree::Nullary,
+            And(ref subs) => Tree::Nary(subs.iter().map(Arc::clone).collect()),
+            Or(ref v) => Tree::Nary(v.iter().map(|(_, p)| Arc::clone(p)).collect()),
+            Threshold(_, ref subs) => Tree::Nary(subs.iter().map(Arc::clone).collect()),
+        }
+    }
+}

src/miniscript/mod.rs

@@ -441,7 +441,6 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Miniscript<Pk, Ctx> {
     {
         let mut translated = vec![];
         for data in Arc::new(self.clone()).post_order_iter() {
-            // convenience method to reduce typing
             let child_n = |n| Arc::clone(&translated[data.child_indices[n]]);
 
             let new_term = match data.node.node {

src/policy/compiler.rs

@@ -786,10 +786,14 @@ where
         }
         Concrete::And(ref subs) => {
             assert_eq!(subs.len(), 2, "and takes 2 args");
-            let mut left = best_compilations(policy_cache, &subs[0], sat_prob, dissat_prob)?;
-            let mut right = best_compilations(policy_cache, &subs[1], sat_prob, dissat_prob)?;
-            let mut q_zero_right = best_compilations(policy_cache, &subs[1], sat_prob, None)?;
-            let mut q_zero_left = best_compilations(policy_cache, &subs[0], sat_prob, None)?;
+            let mut left =
+                best_compilations(policy_cache, subs[0].as_ref(), sat_prob, dissat_prob)?;
+            let mut right =
+                best_compilations(policy_cache, subs[1].as_ref(), sat_prob, dissat_prob)?;
+            let mut q_zero_right =
+                best_compilations(policy_cache, subs[1].as_ref(), sat_prob, None)?;
+            let mut q_zero_left =
+                best_compilations(policy_cache, subs[0].as_ref(), sat_prob, None)?;
 
             compile_binary!(&mut left, &mut right, [1.0, 1.0], Terminal::AndB);
             compile_binary!(&mut right, &mut left, [1.0, 1.0], Terminal::AndB);
@@ -813,48 +817,56 @@ where
             let rw = subs[1].0 as f64 / total;
 
             //and-or
-            if let (Concrete::And(x), _) = (&subs[0].1, &subs[1].1) {
+            if let (Concrete::And(x), _) = (subs[0].1.as_ref(), subs[1].1.as_ref()) {
                 let mut a1 = best_compilations(
                     policy_cache,
-                    &x[0],
+                    x[0].as_ref(),
                     lw * sat_prob,
                     Some(dissat_prob.unwrap_or(0 as f64) + rw * sat_prob),
                 )?;
-                let mut a2 = best_compilations(policy_cache, &x[0], lw * sat_prob, None)?;
+                let mut a2 = best_compilations(policy_cache, x[0].as_ref(), lw * sat_prob, None)?;
 
                 let mut b1 = best_compilations(
                     policy_cache,
-                    &x[1],
+                    x[1].as_ref(),
                     lw * sat_prob,
                     Some(dissat_prob.unwrap_or(0 as f64) + rw * sat_prob),
                 )?;
-                let mut b2 = best_compilations(policy_cache, &x[1], lw * sat_prob, None)?;
+                let mut b2 = best_compilations(policy_cache, x[1].as_ref(), lw * sat_prob, None)?;
 
-                let mut c =
-                    best_compilations(policy_cache, &subs[1].1, rw * sat_prob, dissat_prob)?;
+                let mut c = best_compilations(
+                    policy_cache,
+                    subs[1].1.as_ref(),
+                    rw * sat_prob,
+                    dissat_prob,
+                )?;
 
                 compile_tern!(&mut a1, &mut b2, &mut c, [lw, rw]);
                 compile_tern!(&mut b1, &mut a2, &mut c, [lw, rw]);
             };
-            if let (_, Concrete::And(x)) = (&subs[0].1, &subs[1].1) {
+            if let (_, Concrete::And(x)) = (&subs[0].1.as_ref(), subs[1].1.as_ref()) {
                 let mut a1 = best_compilations(
                     policy_cache,
-                    &x[0],
+                    x[0].as_ref(),
                     rw * sat_prob,
                     Some(dissat_prob.unwrap_or(0 as f64) + lw * sat_prob),
                 )?;
-                let mut a2 = best_compilations(policy_cache, &x[0], rw * sat_prob, None)?;
+                let mut a2 = best_compilations(policy_cache, x[0].as_ref(), rw * sat_prob, None)?;
 
                 let mut b1 = best_compilations(
                     policy_cache,
-                    &x[1],
+                    x[1].as_ref(),
                     rw * sat_prob,
                     Some(dissat_prob.unwrap_or(0 as f64) + lw * sat_prob),
                 )?;
-                let mut b2 = best_compilations(policy_cache, &x[1], rw * sat_prob, None)?;
+                let mut b2 = best_compilations(policy_cache, x[1].as_ref(), rw * sat_prob, None)?;
 
-                let mut c =
-                    best_compilations(policy_cache, &subs[0].1, lw * sat_prob, dissat_prob)?;
+                let mut c = best_compilations(
+                    policy_cache,
+                    subs[0].1.as_ref(),
+                    lw * sat_prob,
+                    dissat_prob,
+                )?;
 
                 compile_tern!(&mut a1, &mut b2, &mut c, [rw, lw]);
                 compile_tern!(&mut b1, &mut a2, &mut c, [rw, lw]);
@@ -873,12 +885,22 @@ where
             let mut r_comp = vec![];
 
             for dissat_prob in dissat_probs(rw).iter() {
-                let l = best_compilations(policy_cache, &subs[0].1, lw * sat_prob, *dissat_prob)?;
+                let l = best_compilations(
+                    policy_cache,
+                    subs[0].1.as_ref(),
+                    lw * sat_prob,
+                    *dissat_prob,
+                )?;
                 l_comp.push(l);
             }
 
             for dissat_prob in dissat_probs(lw).iter() {
-                let r = best_compilations(policy_cache, &subs[1].1, rw * sat_prob, *dissat_prob)?;
+                let r = best_compilations(
+                    policy_cache,
+                    subs[1].1.as_ref(),
+                    rw * sat_prob,
+                    *dissat_prob,
+                )?;
                 r_comp.push(r);
             }
 
@@ -913,8 +935,8 @@ where
                 let sp = sat_prob * k_over_n;
                 //Expressions must be dissatisfiable
                 let dp = Some(dissat_prob.unwrap_or(0 as f64) + (1.0 - k_over_n) * sat_prob);
-                let be = best(types::Base::B, policy_cache, ast, sp, dp)?;
-                let bw = best(types::Base::W, policy_cache, ast, sp, dp)?;
+                let be = best(types::Base::B, policy_cache, ast.as_ref(), sp, dp)?;
+                let bw = best(types::Base::W, policy_cache, ast.as_ref(), sp, dp)?;
 
                 let diff = be.cost_1d(sp, dp) - bw.cost_1d(sp, dp);
                 best_es.push((be.comp_ext_data, be));
@@ -947,7 +969,7 @@ where
             let key_vec: Vec<Pk> = subs
                 .iter()
                 .filter_map(|s| {
-                    if let Concrete::Key(ref pk) = *s {
+                    if let Concrete::Key(ref pk) = s.as_ref() {
                         Some(pk.clone())
                     } else {
                         None
@@ -967,9 +989,10 @@ where
                 _ if k == subs.len() => {
                     let mut it = subs.iter();
                     let mut policy = it.next().expect("No sub policy in thresh() ?").clone();
-                    policy = it.fold(policy, |acc, pol| Concrete::And(vec![acc, pol.clone()]));
+                    policy =
+                        it.fold(policy, |acc, pol| Concrete::And(vec![acc, pol.clone()]).into());
 
-                    ret = best_compilations(policy_cache, &policy, sat_prob, dissat_prob)?;
+                    ret = best_compilations(policy_cache, policy.as_ref(), sat_prob, dissat_prob)?;
                 }
                 _ => {}
             }
@@ -1178,8 +1201,11 @@ mod tests {
     fn compile_timelocks() {
         // artificially create a policy that is problematic and try to compile
         let pol: SPolicy = Concrete::And(vec![
-            Concrete::Key("A".to_string()),
-            Concrete::And(vec![Concrete::after(9), Concrete::after(1000_000_000)]),
+            Arc::new(Concrete::Key("A".to_string())),
+            Arc::new(Concrete::And(vec![
+                Arc::new(Concrete::after(9)),
+                Arc::new(Concrete::after(1000_000_000)),
+            ])),
         ]);
         assert!(pol.compile::<Segwitv0>().is_err());
 
@@ -1273,13 +1299,22 @@ mod tests {
 
         // Liquid policy
         let policy: BPolicy = Concrete::Or(vec![
-            (127, Concrete::Threshold(3, key_pol[0..5].to_owned())),
+            (
+                127,
+                Arc::new(Concrete::Threshold(
+                    3,
+                    key_pol[0..5].iter().map(|p| (p.clone()).into()).collect(),
+                )),
+            ),
             (
                 1,
-                Concrete::And(vec![
-                    Concrete::Older(Sequence::from_height(10000)),
-                    Concrete::Threshold(2, key_pol[5..8].to_owned()),
-                ]),
+                Arc::new(Concrete::And(vec![
+                    Arc::new(Concrete::Older(Sequence::from_height(10000))),
+                    Arc::new(Concrete::Threshold(
+                        2,
+                        key_pol[5..8].iter().map(|p| (p.clone()).into()).collect(),
+                    )),
+                ])),
             ),
         ]);
 
@@ -1391,8 +1426,10 @@ mod tests {
         // and to a ms thresh otherwise.
         // k = 1 (or 2) does not compile, see https://github.com/rust-bitcoin/rust-miniscript/issues/114
         for k in &[10, 15, 21] {
-            let pubkeys: Vec<Concrete<bitcoin::PublicKey>> =
-                keys.iter().map(|pubkey| Concrete::Key(*pubkey)).collect();
+            let pubkeys: Vec<Arc<Concrete<bitcoin::PublicKey>>> = keys
+                .iter()
+                .map(|pubkey| Arc::new(Concrete::Key(*pubkey)))
+                .collect();
             let big_thresh = Concrete::Threshold(*k, pubkeys);
             let big_thresh_ms: SegwitMiniScript = big_thresh.compile().unwrap();
             if *k == 21 {
@@ -1419,18 +1456,18 @@ mod tests {
         // or(thresh(52, [pubkey; 52]), thresh(52, [pubkey; 52])) results in a 3642-bytes long
         // witness script with only 54 stack elements
         let (keys, _) = pubkeys_and_a_sig(104);
-        let keys_a: Vec<Concrete<bitcoin::PublicKey>> = keys[..keys.len() / 2]
+        let keys_a: Vec<Arc<Concrete<bitcoin::PublicKey>>> = keys[..keys.len() / 2]
             .iter()
-            .map(|pubkey| Concrete::Key(*pubkey))
+            .map(|pubkey| Arc::new(Concrete::Key(*pubkey)))
             .collect();
-        let keys_b: Vec<Concrete<bitcoin::PublicKey>> = keys[keys.len() / 2..]
+        let keys_b: Vec<Arc<Concrete<bitcoin::PublicKey>>> = keys[keys.len() / 2..]
             .iter()
-            .map(|pubkey| Concrete::Key(*pubkey))
+            .map(|pubkey| Arc::new(Concrete::Key(*pubkey)))
             .collect();
 
         let thresh_res: Result<SegwitMiniScript, _> = Concrete::Or(vec![
-            (1, Concrete::Threshold(keys_a.len(), keys_a)),
-            (1, Concrete::Threshold(keys_b.len(), keys_b)),
+            (1, Arc::new(Concrete::Threshold(keys_a.len(), keys_a))),
+            (1, Arc::new(Concrete::Threshold(keys_b.len(), keys_b))),
         ])
         .compile();
         let script_size = thresh_res.clone().and_then(|m| Ok(m.script_size()));
@@ -1443,8 +1480,10 @@ mod tests {
 
         // Hit the maximum witness stack elements limit
         let (keys, _) = pubkeys_and_a_sig(100);
-        let keys: Vec<Concrete<bitcoin::PublicKey>> =
-            keys.iter().map(|pubkey| Concrete::Key(*pubkey)).collect();
+        let keys: Vec<Arc<Concrete<bitcoin::PublicKey>>> = keys
+            .iter()
+            .map(|pubkey| Arc::new(Concrete::Key(*pubkey)))
+            .collect();
         let thresh_res: Result<SegwitMiniScript, _> =
             Concrete::Threshold(keys.len(), keys).compile();
         let n_elements = thresh_res
@@ -1462,8 +1501,10 @@ mod tests {
     fn shared_limits() {
         // Test the maximum number of OPs with a 67-of-68 multisig
         let (keys, _) = pubkeys_and_a_sig(68);
-        let keys: Vec<Concrete<bitcoin::PublicKey>> =
-            keys.iter().map(|pubkey| Concrete::Key(*pubkey)).collect();
+        let keys: Vec<Arc<Concrete<bitcoin::PublicKey>>> = keys
+            .iter()
+            .map(|pubkey| Arc::new(Concrete::Key(*pubkey)))
+            .collect();
         let thresh_res: Result<SegwitMiniScript, _> =
             Concrete::Threshold(keys.len() - 1, keys).compile();
         let ops_count = thresh_res.clone().and_then(|m| Ok(m.ext.ops.op_count()));
@@ -1475,8 +1516,10 @@ mod tests {
         );
         // For legacy too..
         let (keys, _) = pubkeys_and_a_sig(68);
-        let keys: Vec<Concrete<bitcoin::PublicKey>> =
-            keys.iter().map(|pubkey| Concrete::Key(*pubkey)).collect();
+        let keys: Vec<Arc<Concrete<bitcoin::PublicKey>>> = keys
+            .iter()
+            .map(|pubkey| Arc::new(Concrete::Key(*pubkey)))
+            .collect();
         let thresh_res = Concrete::Threshold(keys.len() - 1, keys).compile::<Legacy>();
         let ops_count = thresh_res.clone().and_then(|m| Ok(m.ext.ops.op_count()));
         assert_eq!(
@@ -1488,8 +1531,9 @@ mod tests {
 
         // Test that we refuse to compile policies with duplicated keys
         let (keys, _) = pubkeys_and_a_sig(1);
-        let key = Concrete::Key(keys[0]);
-        let res = Concrete::Or(vec![(1, key.clone()), (1, key.clone())]).compile::<Segwitv0>();
+        let key = Arc::new(Concrete::Key(keys[0]));
+        let res =
+            Concrete::Or(vec![(1, Arc::clone(&key)), (1, Arc::clone(&key))]).compile::<Segwitv0>();
         assert_eq!(
             res,
             Err(CompilerError::PolicyError(policy::concrete::PolicyError::DuplicatePubKeys))