miniscript: don't use malleable satisfier, even for or_i dissatisfactions

...unless the user has explicitly requested a malleable satisfaction.

Fixes #976

Author: apoelstra

src/miniscript/satisfy/mod.rs

@@ -651,7 +651,7 @@ mod tests {
         //
         // Since the first branch's dissatisfaction is HASSIG but the second branch's is not,
         // the satisfier is required to dissatisfy the second branch to avoid malleability.
-        // However, because of #976, it does not.
+        // But if we use `into_plan_mall` we can see the bug.
         //
         // Itstead, it takes both the after(144) and after(50) branches, and the resulting
         // plan should show after(144) since it's the higher one. However, prior to #895,
@@ -664,9 +664,12 @@ mod tests {
         // Need DefiniteDescriptorKey https://github.com/rust-bitcoin/rust-miniscript/issues/927
         let descriptor =
             Descriptor::<crate::DefiniteDescriptorKey>::from_str(&descriptor_str).unwrap();
-        // Compute plan and confirm the timelock is correct.
-        let plan = descriptor.into_plan(&satisfier).unwrap();
+        // Compute plan and confirm the timelock is correct -- 144 for a malleable transaction
+        let plan = descriptor.clone().into_plan_mall(&satisfier).unwrap();
         assert_eq!(plan.absolute_timelock, Some(absolute::LockTime::from_height(144).unwrap()),);
+        // ...and 50 for a non-malleable one (since take the expensive_threshold alternate)
+        let plan = descriptor.into_plan(&satisfier).unwrap();
+        assert_eq!(plan.absolute_timelock, Some(absolute::LockTime::from_height(50).unwrap()),);
 
         // Same descriptor as above, except that now we use a time-based timelock rather than a
         // lower height-based one.
@@ -690,7 +693,7 @@ mod tests {
         let descriptor =
             Descriptor::<crate::DefiniteDescriptorKey>::from_str(&descriptor_str).unwrap();
 
-        let plan = descriptor.into_plan(&satisfier).unwrap();
+        let plan = descriptor.into_plan_mall(&satisfier).unwrap();
         assert_eq!(
             plan.absolute_timelock,
             Some(absolute::LockTime::from_time(1000000000).unwrap()),

src/miniscript/satisfy/sat_dissat.rs

@@ -442,18 +442,8 @@ impl<Pk: MiniscriptKey + ToPublicKey> Satisfaction<Placeholder<Pk>> {
                 Terminal::OrI(_, _) => {
                     let SatDissat { dissat: r_dis, sat: r_sat } = stack.pop().unwrap();
                     let SatDissat { dissat: l_dis, sat: l_sat } = stack.pop().unwrap();
-                    // Regarding use of `minimum_mall` for dissatisfactions: this is correct, because
-                    // dissatisfactions for individual fragments are not required to be unique. (We
-                    // track this property using the `ty.malleability.dissat` field.)
-                    //
-                    // This is because usually dissatisfactions are eventually dropped, e.g. by the
-                    // `j:` wrapper or because at the top-level a dissatisfaction is simply invalid.
-                    // In cases where non-unique dissatisfactions would cause malleability, e.g. in
-                    // the `or_c` or `or_b` fragments which use dissatisfactions as part of their
-                    // satisfactions, we set `ty.malleability.non_malleable` appropriately (and
-                    // potentially reject the whole script at creation time).
                     SatDissat {
-                        dissat: Self::minimum_mall(
+                        dissat: min_fn(
                             Self {
                                 stack: Witness::combine(l_dis.stack, Witness::push_1()),
                                 ..l_dis