Merge pull request #85 from coblox/witness-stack-order-wrong

Miniscript produces witness stacks with wrong order of signatures for `and` instructions

Author: sanket1729

src/descriptor/mod.rs

@@ -555,6 +555,7 @@ mod tests {
     use bitcoin::hashes::{hash160, sha256};
     use bitcoin::{self, secp256k1, PublicKey};
     use miniscript::satisfy::BitcoinSig;
+    use std::collections::HashMap;
     use std::str::FromStr;
     use {Descriptor, DummyKey, Miniscript, Satisfier};
 
@@ -947,4 +948,59 @@ mod tests {
             "unexpected «no arguments given»"
         )
     }
+
+    #[test]
+    fn witness_stack_for_andv_is_arranged_in_correct_order() {
+        // arrange
+        let a = bitcoin::PublicKey::from_str(
+            "02937402303919b3a2ee5edd5009f4236f069bf75667b8e6ecf8e5464e20116a0e",
+        )
+        .unwrap();
+        let sig_a = secp256k1::Signature::from_str("3045022100a7acc3719e9559a59d60d7b2837f9842df30e7edcd754e63227e6168cec72c5d022066c2feba4671c3d99ea75d9976b4da6c86968dbf3bab47b1061e7a1966b1778c").unwrap();
+
+        let b = bitcoin::PublicKey::from_str(
+            "02eb64639a17f7334bb5a1a3aad857d6fec65faef439db3de72f85c88bc2906ad3",
+        )
+        .unwrap();
+        let sig_b = secp256k1::Signature::from_str("3044022075b7b65a7e6cd386132c5883c9db15f9a849a0f32bc680e9986398879a57c276022056d94d12255a4424f51c700ac75122cb354895c9f2f88f0cbb47ba05c9c589ba").unwrap();
+
+        let descriptor = Descriptor::<bitcoin::PublicKey>::from_str(&format!(
+            "wsh(and_v(v:pk({A}),pk({B})))",
+            A = a,
+            B = b
+        ))
+        .unwrap();
+
+        let mut txin = bitcoin::TxIn {
+            previous_output: bitcoin::OutPoint::default(),
+            script_sig: bitcoin::Script::new(),
+            sequence: 0,
+            witness: vec![],
+        };
+        let satisfier = {
+            let mut satisfier = HashMap::with_capacity(2);
+
+            satisfier.insert(a, (sig_a.clone(), ::bitcoin::SigHashType::All));
+            satisfier.insert(b, (sig_b.clone(), ::bitcoin::SigHashType::All));
+
+            satisfier
+        };
+
+        // act
+        descriptor.satisfy(&mut txin, &satisfier).unwrap();
+
+        // assert
+        let witness0 = &txin.witness[0];
+        let witness1 = &txin.witness[1];
+
+        let sig0 = secp256k1::Signature::from_der(&witness0[..witness0.len() - 1]).unwrap();
+        let sig1 = secp256k1::Signature::from_der(&witness1[..witness1.len() - 1]).unwrap();
+
+        // why are we asserting this way?
+        // The witness stack is evaluated from top to bottom. Given an `and` instruction, the left arm of the and is going to evaluate first,
+        // meaning the next witness element (on a three element stack, that is the middle one) needs to be the signature for the left side of the `and`.
+        // The left side of the `and` performs a CHECKSIG against public key `a` so `sig1` needs to be `sig_a` and `sig0` needs to be `sig_b`.
+        assert_eq!(sig1, sig_a);
+        assert_eq!(sig0, sig_b);
+    }
 }

src/miniscript/satisfy.rs

@@ -580,7 +580,7 @@ impl Satisfaction {
                 let l_sat = Self::satisfy(&l.node, stfr);
                 let r_sat = Self::satisfy(&r.node, stfr);
                 Satisfaction {
-                    stack: Witness::combine(l_sat.stack, r_sat.stack),
+                    stack: Witness::combine(r_sat.stack, l_sat.stack),
                     has_sig: l_sat.has_sig || r_sat.has_sig,
                 }
             }
@@ -592,11 +592,11 @@ impl Satisfaction {
 
                 Self::minimum(
                     Satisfaction {
-                        stack: Witness::combine(a_sat.stack, b_sat.stack),
+                        stack: Witness::combine(b_sat.stack, a_sat.stack),
                         has_sig: a_sat.has_sig || b_sat.has_sig,
                     },
                     Satisfaction {
-                        stack: Witness::combine(a_nsat.stack, c_sat.stack),
+                        stack: Witness::combine(c_sat.stack, a_nsat.stack),
                         has_sig: a_nsat.has_sig || c_sat.has_sig,
                     },
                 )