compiler: replace conjuctions in AstElemExt::type_check with explicit functions

Conjunctions don't need to set 'branch_prob', but we do set them, leading
to confusing logic. By getting rid of these, we can make the conjunction
logic simpler and avoid setting branch_prob to Some(0.0).

Author: apoelstra

src/policy/compiler.rs

@@ -13,7 +13,7 @@ use sync::Arc;
 
 use crate::miniscript::context::SigType;
 use crate::miniscript::limits::{MAX_PUBKEYS_IN_CHECKSIGADD, MAX_PUBKEYS_PER_MULTISIG};
-use crate::miniscript::types::{self, ErrorKind, ExtData, Type};
+use crate::miniscript::types::{self, ErrorKind, Type};
 use crate::miniscript::ScriptContext;
 use crate::policy::Concrete;
 use crate::prelude::*;
@@ -353,7 +353,7 @@ impl CompilerExtData {
     fn and_or(a: Self, b: Self, c: Self) -> Self {
         let aprob = a.branch_prob.expect("andor, a prob must be set");
         let bprob = b.branch_prob.expect("andor, b prob must be set");
-        let cprob = c.branch_prob.expect("andor, c prob must be set");
+        let cprob = c.branch_prob.unwrap_or(0.0);
 
         let adis = a
             .dissat_cost
@@ -562,6 +562,55 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> AstElemExt<Pk, Ctx> {
         }
     }
 
+    /// Helper functions to compose two Miniscript fragments, where we assume
+    /// by construction that all validation parameters are upheld.
+    fn compose_typeck_only(
+        term: Terminal<Pk, Ctx>,
+    ) -> Result<Arc<Miniscript<Pk, Ctx>>, types::Error> {
+        let ty = types::Type::type_check(&term)?;
+        let ext = types::ExtData::type_check(&term);
+        Ok(Arc::new(Miniscript::from_components_unchecked(term, ty, ext)))
+    }
+
+    fn and_b(left: &Self, right: &Self) -> Result<Self, types::Error> {
+        Ok(Self {
+            ms: Self::compose_typeck_only(Terminal::AndB(
+                Arc::clone(&left.ms),
+                Arc::clone(&right.ms),
+            ))?,
+            comp_ext_data: CompilerExtData::and_b(left.comp_ext_data, right.comp_ext_data),
+        })
+    }
+
+    fn and_v(left: &Self, right: &Self) -> Result<Self, types::Error> {
+        Ok(Self {
+            ms: Self::compose_typeck_only(Terminal::AndV(
+                Arc::clone(&left.ms),
+                Arc::clone(&right.ms),
+            ))?,
+            comp_ext_data: CompilerExtData::and_v(left.comp_ext_data, right.comp_ext_data),
+        })
+    }
+
+    /// andor(a,b,0) is a conjunction of a and b
+    fn and_or_0(left: &Self, right: &Self) -> Result<Self, types::Error> {
+        let left_ext_data = CompilerExtData { branch_prob: Some(1.0), ..left.comp_ext_data };
+        let right_ext_data = CompilerExtData { branch_prob: Some(1.0), ..right.comp_ext_data };
+
+        Ok(Self {
+            ms: Self::compose_typeck_only(Terminal::AndOr(
+                Arc::clone(&left.ms),
+                Arc::clone(&right.ms),
+                Arc::new(Miniscript::FALSE),
+            ))?,
+            comp_ext_data: CompilerExtData::and_or(
+                left_ext_data,
+                right_ext_data,
+                CompilerExtData::FALSE,
+            ),
+        })
+    }
+
     fn binary(ast: Terminal<Pk, Ctx>, l: &Self, r: &Self) -> Result<Self, types::Error> {
         let lookup_ext = |n| match n {
             0 => l.comp_ext_data,
@@ -861,26 +910,92 @@ where
         Concrete::Hash160(ref hash) => insert_wrap!(AstElemExt::hash160(hash.clone())),
         Concrete::And(ref subs) => {
             assert_eq!(subs.len(), 2, "and takes 2 args");
-            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);
-            compile_binary!(&mut left, &mut right, [1.0, 1.0], Terminal::AndV);
-            compile_binary!(&mut right, &mut left, [1.0, 1.0], Terminal::AndV);
-            let mut zero_comp = BTreeMap::new();
-            zero_comp.insert(
-                CompilationKey::from_type(Type::FALSE, ExtData::FALSE.has_free_verify, dissat_prob),
-                AstElemExt::unsatisfiable(),
-            );
-            compile_tern!(&mut left, &mut q_zero_right, &mut zero_comp, [1.0, 0.0]);
-            compile_tern!(&mut right, &mut q_zero_left, &mut zero_comp, [1.0, 0.0]);
+            let left = best_compilations(policy_cache, subs[0].as_ref(), sat_prob, dissat_prob)?;
+            let right = best_compilations(policy_cache, subs[1].as_ref(), sat_prob, dissat_prob)?;
+            let q_zero_right = best_compilations(policy_cache, subs[1].as_ref(), sat_prob, None)?;
+            let q_zero_left = best_compilations(policy_cache, subs[0].as_ref(), sat_prob, None)?;
+
+            for l in left.values() {
+                for r in right.values() {
+                    if let Ok(new_ext) = AstElemExt::and_b(l, r) {
+                        insert_best_wrapped(
+                            policy_cache,
+                            policy,
+                            &mut ret,
+                            new_ext,
+                            sat_prob,
+                            dissat_prob,
+                        )?;
+                    }
+                    if let Ok(new_ext) = AstElemExt::and_v(l, r) {
+                        insert_best_wrapped(
+                            policy_cache,
+                            policy,
+                            &mut ret,
+                            new_ext,
+                            sat_prob,
+                            dissat_prob,
+                        )?;
+                    }
+                }
+            }
+            // Do a separate loop with 'l' and 'r' swapped; we could combine the loops,
+            // but this would sometimes result in compiling e.g. and(pk(A),pk(B)) into
+            // an and with A and B swapped, which is surprising to the user since the
+            // cost is the same with or without the swap.
+            for l in left.values() {
+                for r in right.values() {
+                    if let Ok(new_ext) = AstElemExt::and_b(r, l) {
+                        insert_best_wrapped(
+                            policy_cache,
+                            policy,
+                            &mut ret,
+                            new_ext,
+                            sat_prob,
+                            dissat_prob,
+                        )?;
+                    }
+                    if let Ok(new_ext) = AstElemExt::and_v(r, l) {
+                        insert_best_wrapped(
+                            policy_cache,
+                            policy,
+                            &mut ret,
+                            new_ext,
+                            sat_prob,
+                            dissat_prob,
+                        )?;
+                    }
+                }
+            }
+
+            for l in left.values() {
+                for r in q_zero_right.values() {
+                    if let Ok(new_ext) = AstElemExt::and_or_0(l, r) {
+                        insert_best_wrapped(
+                            policy_cache,
+                            policy,
+                            &mut ret,
+                            new_ext,
+                            sat_prob,
+                            dissat_prob,
+                        )?;
+                    }
+                }
+            }
+            for l in right.values() {
+                for r in q_zero_left.values() {
+                    if let Ok(new_ext) = AstElemExt::and_or_0(l, r) {
+                        insert_best_wrapped(
+                            policy_cache,
+                            policy,
+                            &mut ret,
+                            new_ext,
+                            sat_prob,
+                            dissat_prob,
+                        )?;
+                    }
+                }
+            }
         }
         Concrete::Or(ref subs) => {
             let total = u32::from(subs[0].0) as f64 + u32::from(subs[1].0) as f64;