Merge #803: [backport] cleanup: refactor a bunch of concatenation-related code in satisfy

9d81dcf bump version to 12.3.1 (Andrew Poelstra)
7be767c satisfy: clean up a bunch of satisfaction code (Andrew Poelstra)
8efce62 ci: fix new clippy lint related to doccomments (Andrew Poelstra)

Pull request description:

  This backports the main commit in #798 so that the code in (released) 12.x is substantially the same as the code in (unreleased) 13.x. This makes it easier to write and maintain regression tests.

ACKs for top commit:
  sanket1729:
    ACK 9d81dcf

Tree-SHA512: 1996ba4e2d01fc469e43724d7c6b2fbd5d1536181501ecb71ae99566bf7f635e829cfb7383d22241226becc84fcb95efcd105c95e165a401e9bd31e89e89ff19

Author: apoelstra

Cargo-minimal.lock

@@ -316,7 +316,7 @@ dependencies = [
 
 [[package]]
 name = "miniscript"
-version = "12.3.0"
+version = "12.3.1"
 dependencies = [
  "bech32",
  "bitcoin",

Cargo-recent.lock

@@ -294,7 +294,7 @@ dependencies = [
 
 [[package]]
 name = "miniscript"
-version = "12.3.0"
+version = "12.3.1"
 dependencies = [
  "bech32",
  "bitcoin",

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "miniscript"
-version = "12.3.0"
+version = "12.3.1"
 authors = ["Andrew Poelstra <[email protected]>, Sanket Kanjalkar <[email protected]>"]
 license = "CC0-1.0"
 homepage = "https://github.com/rust-bitcoin/rust-miniscript/"

src/miniscript/iter.rs

@@ -199,22 +199,24 @@ impl<'a, Pk: MiniscriptKey, Ctx: ScriptContext> Iterator for PkIter<'a, Pk, Ctx>
     }
 }
 
-// Module is public since it export testcase generation which may be used in
-// dependent libraries for their own tasts based on Miniscript AST
+/// Module is public since it export testcase generation which may be used in
+/// dependent libraries for their own tasts based on Miniscript AST
 #[cfg(test)]
 pub mod test {
     use bitcoin::hashes::{hash160, ripemd160, sha256, sha256d, Hash};
 
     use super::Miniscript;
     use crate::miniscript::context::Segwitv0;
 
+    /// Test case.
     pub type TestData = (
         Miniscript<bitcoin::PublicKey, Segwitv0>,
         Vec<bitcoin::PublicKey>,
         Vec<hash160::Hash>,
         bool, // Indicates that the top-level contains public key or hashes
     );
 
+    /// Generate a deterministic list of public keys of the given length.
     pub fn gen_secp_pubkeys(n: usize) -> Vec<secp256k1::PublicKey> {
         let mut ret = Vec::with_capacity(n);
         let secp = secp256k1::Secp256k1::new();
@@ -233,13 +235,15 @@ pub mod test {
         ret
     }
 
+    /// Generate a deterministic list of Bitcoin public keys of the given length.
     pub fn gen_bitcoin_pubkeys(n: usize, compressed: bool) -> Vec<bitcoin::PublicKey> {
         gen_secp_pubkeys(n)
             .into_iter()
             .map(|inner| bitcoin::PublicKey { inner, compressed })
             .collect()
     }
 
+    /// Generate a deterministic list of test cases of the given length.
     pub fn gen_testcases() -> Vec<TestData> {
         let k = gen_bitcoin_pubkeys(10, true);
         let _h: Vec<hash160::Hash> = k

src/miniscript/satisfy.rs

@@ -889,6 +889,34 @@ pub struct Satisfaction<T> {
 }
 
 impl<Pk: MiniscriptKey + ToPublicKey> Satisfaction<Placeholder<Pk>> {
+    /// The empty satisfaction.
+    ///
+    /// This has the property that, when concatenated on either side with another satisfaction
+    /// X, the result will be X.
+    fn empty() -> Self {
+        Satisfaction {
+            has_sig: false,
+            relative_timelock: None,
+            absolute_timelock: None,
+            stack: Witness::Stack(vec![]),
+        }
+    }
+
+    /// Forms a satisfaction which is the concatenation of two satisfactions, with `other`'s
+    /// stack before `self`'s.
+    ///
+    /// This order allows callers to write `left.concatenate_rev(right)` which feels more
+    /// natural than the opposite order, and more importantly, allows this method to be
+    /// used when folding over an iterator of multiple satisfactions.
+    fn concatenate_rev(self, other: Self) -> Self {
+        Satisfaction {
+            has_sig: self.has_sig || other.has_sig,
+            relative_timelock: cmp::max(self.relative_timelock, other.relative_timelock),
+            absolute_timelock: cmp::max(self.absolute_timelock, other.absolute_timelock),
+            stack: Witness::combine(other.stack, self.stack),
+        }
+    }
+
     pub(crate) fn build_template<P, Ctx>(
         term: &Terminal<Pk, Ctx>,
         provider: &P,
@@ -1041,20 +1069,9 @@ impl<Pk: MiniscriptKey + ToPublicKey> Satisfaction<Placeholder<Pk>> {
             }
         } else {
             // Otherwise flatten everything out
-            Satisfaction {
-                has_sig: ret_stack.iter().any(|sat| sat.has_sig),
-                relative_timelock: ret_stack
-                    .iter()
-                    .filter_map(|sat| sat.relative_timelock)
-                    .max(),
-                absolute_timelock: ret_stack
-                    .iter()
-                    .filter_map(|sat| sat.absolute_timelock)
-                    .max(),
-                stack: ret_stack
-                    .into_iter()
-                    .fold(Witness::empty(), |acc, next| Witness::combine(next.stack, acc)),
-            }
+            ret_stack
+                .into_iter()
+                .fold(Satisfaction::empty(), Satisfaction::concatenate_rev)
         }
     }
 
@@ -1125,20 +1142,9 @@ impl<Pk: MiniscriptKey + ToPublicKey> Satisfaction<Placeholder<Pk>> {
 
         // combine the witness
         // no non-malleability checks needed
-        Satisfaction {
-            has_sig: ret_stack.iter().any(|sat| sat.has_sig),
-            relative_timelock: ret_stack
-                .iter()
-                .filter_map(|sat| sat.relative_timelock)
-                .max(),
-            absolute_timelock: ret_stack
-                .iter()
-                .filter_map(|sat| sat.absolute_timelock)
-                .max(),
-            stack: ret_stack
-                .into_iter()
-                .fold(Witness::empty(), |acc, next| Witness::combine(next.stack, acc)),
-        }
+        ret_stack
+            .into_iter()
+            .fold(Satisfaction::empty(), Satisfaction::concatenate_rev)
     }
 
     fn minimum(sat1: Self, sat2: Self) -> Self {
@@ -1360,12 +1366,7 @@ impl<Pk: MiniscriptKey + ToPublicKey> Satisfaction<Placeholder<Pk>> {
                     Self::satisfy_helper(&l.node, stfr, root_has_sig, leaf_hash, min_fn, thresh_fn);
                 let r_sat =
                     Self::satisfy_helper(&r.node, stfr, root_has_sig, leaf_hash, min_fn, thresh_fn);
-                Satisfaction {
-                    stack: Witness::combine(r_sat.stack, l_sat.stack),
-                    has_sig: l_sat.has_sig || r_sat.has_sig,
-                    relative_timelock: cmp::max(l_sat.relative_timelock, r_sat.relative_timelock),
-                    absolute_timelock: cmp::max(l_sat.absolute_timelock, r_sat.absolute_timelock),
-                }
+                l_sat.concatenate_rev(r_sat)
             }
             Terminal::AndOr(ref a, ref b, ref c) => {
                 let a_sat =
@@ -1383,27 +1384,7 @@ impl<Pk: MiniscriptKey + ToPublicKey> Satisfaction<Placeholder<Pk>> {
                 let c_sat =
                     Self::satisfy_helper(&c.node, stfr, root_has_sig, leaf_hash, min_fn, thresh_fn);
 
-                min_fn(
-                    Satisfaction {
-                        stack: Witness::combine(b_sat.stack, a_sat.stack),
-                        has_sig: a_sat.has_sig || b_sat.has_sig,
-                        relative_timelock: cmp::max(
-                            a_sat.relative_timelock,
-                            b_sat.relative_timelock,
-                        ),
-                        absolute_timelock: cmp::max(
-                            a_sat.absolute_timelock,
-                            b_sat.absolute_timelock,
-                        ),
-                    },
-                    Satisfaction {
-                        stack: Witness::combine(c_sat.stack, a_nsat.stack),
-                        has_sig: a_nsat.has_sig || c_sat.has_sig,
-                        // timelocks can't be dissatisfied, so here we ignore a_nsat and only consider c_sat
-                        relative_timelock: c_sat.relative_timelock,
-                        absolute_timelock: c_sat.absolute_timelock,
-                    },
-                )
+                min_fn(a_sat.concatenate_rev(b_sat), a_nsat.concatenate_rev(c_sat))
             }
             Terminal::OrB(ref l, ref r) => {
                 let l_sat =
@@ -1431,18 +1412,8 @@ impl<Pk: MiniscriptKey + ToPublicKey> Satisfaction<Placeholder<Pk>> {
                 assert!(!r_nsat.has_sig);
 
                 min_fn(
-                    Satisfaction {
-                        stack: Witness::combine(r_sat.stack, l_nsat.stack),
-                        has_sig: r_sat.has_sig,
-                        relative_timelock: r_sat.relative_timelock,
-                        absolute_timelock: r_sat.absolute_timelock,
-                    },
-                    Satisfaction {
-                        stack: Witness::combine(r_nsat.stack, l_sat.stack),
-                        has_sig: l_sat.has_sig,
-                        relative_timelock: l_sat.relative_timelock,
-                        absolute_timelock: l_sat.absolute_timelock,
-                    },
+                    Satisfaction::concatenate_rev(l_nsat, r_sat),
+                    Satisfaction::concatenate_rev(l_sat, r_nsat),
                 )
             }
             Terminal::OrD(ref l, ref r) | Terminal::OrC(ref l, ref r) => {
@@ -1461,15 +1432,7 @@ impl<Pk: MiniscriptKey + ToPublicKey> Satisfaction<Placeholder<Pk>> {
 
                 assert!(!l_nsat.has_sig);
 
-                min_fn(
-                    l_sat,
-                    Satisfaction {
-                        stack: Witness::combine(r_sat.stack, l_nsat.stack),
-                        has_sig: r_sat.has_sig,
-                        relative_timelock: r_sat.relative_timelock,
-                        absolute_timelock: r_sat.absolute_timelock,
-                    },
-                )
+                min_fn(l_sat, Satisfaction::concatenate_rev(l_nsat, r_sat))
             }
             Terminal::OrI(ref l, ref r) => {
                 let l_sat =
@@ -1492,7 +1455,24 @@ impl<Pk: MiniscriptKey + ToPublicKey> Satisfaction<Placeholder<Pk>> {
                 )
             }
             Terminal::Thresh(ref thresh) => {
-                thresh_fn(thresh, stfr, root_has_sig, leaf_hash, min_fn)
+                if thresh.k() == thresh.n() {
+                    // this is just an and
+                    thresh
+                        .iter()
+                        .map(|s| {
+                            Self::satisfy_helper(
+                                &s.node,
+                                stfr,
+                                root_has_sig,
+                                leaf_hash,
+                                min_fn,
+                                thresh_fn,
+                            )
+                        })
+                        .fold(Satisfaction::empty(), Satisfaction::concatenate_rev)
+                } else {
+                    thresh_fn(thresh, stfr, root_has_sig, leaf_hash, min_fn)
+                }
             }
             Terminal::Multi(ref thresh) => {
                 // Collect all available signatures
@@ -1682,12 +1662,7 @@ impl<Pk: MiniscriptKey + ToPublicKey> Satisfaction<Placeholder<Pk>> {
                     min_fn,
                     thresh_fn,
                 );
-                Satisfaction {
-                    stack: Witness::combine(odissat.stack, vsat.stack),
-                    has_sig: vsat.has_sig || odissat.has_sig,
-                    relative_timelock: None,
-                    absolute_timelock: None,
-                }
+                vsat.concatenate_rev(odissat)
             }
             Terminal::AndB(ref l, ref r)
             | Terminal::OrB(ref l, ref r)
@@ -1709,12 +1684,7 @@ impl<Pk: MiniscriptKey + ToPublicKey> Satisfaction<Placeholder<Pk>> {
                     min_fn,
                     thresh_fn,
                 );
-                Satisfaction {
-                    stack: Witness::combine(rnsat.stack, lnsat.stack),
-                    has_sig: rnsat.has_sig || lnsat.has_sig,
-                    relative_timelock: None,
-                    absolute_timelock: None,
-                }
+                lnsat.concatenate_rev(rnsat)
             }
             Terminal::OrI(ref l, ref r) => {
                 let lnsat = Self::dissatisfy_helper(
@@ -1750,23 +1720,19 @@ impl<Pk: MiniscriptKey + ToPublicKey> Satisfaction<Placeholder<Pk>> {
                 // Dissatisfactions don't need to non-malleable. Use minimum_mall always
                 Satisfaction::minimum_mall(dissat_1, dissat_2)
             }
-            Terminal::Thresh(ref thresh) => Satisfaction {
-                stack: thresh.iter().fold(Witness::empty(), |acc, sub| {
-                    let nsat = Self::dissatisfy_helper(
-                        &sub.node,
+            Terminal::Thresh(ref thresh) => thresh
+                .iter()
+                .map(|s| {
+                    Self::dissatisfy_helper(
+                        &s.node,
                         stfr,
                         root_has_sig,
                         leaf_hash,
                         min_fn,
                         thresh_fn,
-                    );
-                    assert!(!nsat.has_sig);
-                    Witness::combine(nsat.stack, acc)
-                }),
-                has_sig: false,
-                relative_timelock: None,
-                absolute_timelock: None,
-            },
+                    )
+                })
+                .fold(Satisfaction::empty(), Satisfaction::concatenate_rev),
             Terminal::Multi(ref thresh) => Satisfaction {
                 stack: Witness::Stack(vec![Placeholder::PushZero; thresh.k() + 1]),
                 has_sig: false,