Merge #269: Fix decoding bug

87c65b6 Add breaking test (sanket1729)
e55ef58 Fix decoding up; cleanup code (sanket1729)

Pull request description:

  Caught while reviewing c++ implementation(PR from meshcollider).

ACKs for top commit:
  apoelstra:
    ACK 87c65b6

Tree-SHA512: f4a32de573d0996818f3653f9913508bfd74921a8fb92b00d2f65097d08ea804323c69304d02330507655671273fea2967e97d953c6f372b1a7e44adba0fd989

Author: sanket1729

src/miniscript/decode.rs

@@ -98,7 +98,8 @@ mod private {
 #[derive(Copy, Clone, Debug)]
 enum NonTerm {
     Expression,
-    MaybeSwap,
+    WExpression,
+    Swap,
     MaybeAndV,
     Alt,
     Check,
@@ -208,6 +209,7 @@ macro_rules! match_token {
 }
 
 ///Vec representing terminals stack while decoding.
+#[derive(Debug)]
 struct TerminalStack<Pk: MiniscriptKey, Ctx: ScriptContext>(Vec<Miniscript<Pk, Ctx>>);
 
 impl<Pk: MiniscriptKey, Ctx: ScriptContext> TerminalStack<Pk, Ctx> {
@@ -284,8 +286,8 @@ pub fn parse<Ctx: ScriptContext>(
     let mut non_term = Vec::with_capacity(tokens.len());
     let mut term = TerminalStack(Vec::with_capacity(tokens.len()));
 
+    // top level cannot be swap, must be B
     non_term.push(NonTerm::MaybeAndV);
-    non_term.push(NonTerm::MaybeSwap);
     non_term.push(NonTerm::Expression);
     loop {
         match non_term.pop() {
@@ -439,34 +441,24 @@ pub fn parse<Ctx: ScriptContext>(
                             // `OP_ADD` or not and do the right thing
                         },
                     ),
-                    // fromaltstack
-                    Tk::FromAltStack => {
-                        non_term.push(NonTerm::Alt);
-                        non_term.push(NonTerm::MaybeAndV);
-                        non_term.push(NonTerm::MaybeSwap);
-                        non_term.push(NonTerm::Expression);
-                    },
                     // most other fragments
                     Tk::Num(0) => term.reduce0(Terminal::False)?,
                     Tk::Num(1) => term.reduce0(Terminal::True)?,
                     Tk::EndIf => {
                         non_term.push(NonTerm::EndIf);
                         non_term.push(NonTerm::MaybeAndV);
-                        non_term.push(NonTerm::MaybeSwap);
                         non_term.push(NonTerm::Expression);
                     },
                     // boolean conjunctions and disjunctions
                     Tk::BoolAnd => {
                         non_term.push(NonTerm::AndB);
                         non_term.push(NonTerm::Expression);
-                        non_term.push(NonTerm::MaybeSwap);
-                        non_term.push(NonTerm::Expression);
+                        non_term.push(NonTerm::WExpression);
                     },
                     Tk::BoolOr => {
                         non_term.push(NonTerm::OrB);
                         non_term.push(NonTerm::Expression);
-                        non_term.push(NonTerm::MaybeSwap);
-                        non_term.push(NonTerm::Expression);
+                        non_term.push(NonTerm::WExpression);
                     },
                     // CHECKMULTISIG based multisig
                     Tk::CheckMultiSig, Tk::Num(n) => {
@@ -522,15 +514,14 @@ pub fn parse<Ctx: ScriptContext>(
                     non_term.push(NonTerm::Expression);
                 }
             }
-            Some(NonTerm::MaybeSwap) => {
+            Some(NonTerm::Swap) => {
                 // Handle `SWAP` prefixing
-                if let Some(&Tk::Swap) = tokens.peek() {
-                    tokens.next();
-                    //                    let top = term.pop().unwrap();
-                    term.reduce1(Terminal::Swap)?;
-                    //                    term.push(Terminal::Swap(Arc::new(top)));
-                    non_term.push(NonTerm::MaybeSwap);
-                }
+                match_token!(
+                    tokens,
+                    Tk::Swap => {},
+                );
+                term.reduce1(Terminal::Swap)?;
+                // Swap must be always be terminating a NonTerm as it cannot be in and_v
             }
             Some(NonTerm::Alt) => {
                 match_token!(
@@ -577,14 +568,14 @@ pub fn parse<Ctx: ScriptContext>(
                     tokens,
                     Tk::Add => {
                         non_term.push(NonTerm::ThreshW { n: n + 1, k });
+                        non_term.push(NonTerm::WExpression);
                     },
                     x => {
                         tokens.un_next(x);
                         non_term.push(NonTerm::ThreshE { n: n + 1, k });
+                        non_term.push(NonTerm::Expression);
                     },
                 );
-                non_term.push(NonTerm::MaybeSwap);
-                non_term.push(NonTerm::Expression);
             }
             Some(NonTerm::ThreshE { n, k }) => {
                 let mut subs = Vec::with_capacity(n);
@@ -599,7 +590,6 @@ pub fn parse<Ctx: ScriptContext>(
                     Tk::Else => {
                         non_term.push(NonTerm::EndIfElse);
                         non_term.push(NonTerm::MaybeAndV);
-                        non_term.push(NonTerm::MaybeSwap);
                         non_term.push(NonTerm::Expression);
                     },
                     Tk::If => match_token!(
@@ -636,6 +626,14 @@ pub fn parse<Ctx: ScriptContext>(
                     },
                 );
             }
+            Some(NonTerm::WExpression) => {
+                // W expression must be either from swap or Fromaltstack
+                match_token!(tokens,
+                    Tk::FromAltStack => { non_term.push(NonTerm::Alt);},
+                    tok => { tokens.un_next(tok); non_term.push(NonTerm::Swap);},);
+                non_term.push(NonTerm::MaybeAndV);
+                non_term.push(NonTerm::Expression);
+            }
             None => {
                 // Done :)
                 break;
@@ -650,7 +648,12 @@ pub fn parse<Ctx: ScriptContext>(
 
 fn is_and_v(tokens: &mut TokenIter) -> bool {
     match tokens.peek() {
-        None | Some(&Tk::If) | Some(&Tk::NotIf) | Some(&Tk::Else) | Some(&Tk::ToAltStack) => false,
+        None
+        | Some(&Tk::If)
+        | Some(&Tk::NotIf)
+        | Some(&Tk::Else)
+        | Some(&Tk::ToAltStack)
+        | Some(&Tk::Swap) => false,
         _ => true,
     }
 }

src/miniscript/mod.rs

@@ -1072,4 +1072,24 @@ mod tests {
         let wit = tap_ms.satisfy(s).unwrap();
         assert_eq!(wit, vec![schnorr_sig.as_ref().to_vec(), vec![], vec![]]);
     }
+
+    #[test]
+    fn decode_bug_cpp_review() {
+        let ms = Miniscript::<String, Segwitv0>::from_str_insane(
+            "and_b(1,s:and_v(v:older(9),c:pk_k(A)))",
+        )
+        .unwrap();
+        let ms_trans = ms.translate_pk_infallible(
+            |_x| {
+                bitcoin::PublicKey::from_str(
+                    "02fbcf092916824cc56c4591abeedd54586f5ffc73c6ba88118162e3500ad695ea",
+                )
+                .unwrap()
+            },
+            |_x| unreachable!(),
+        );
+        let enc = ms_trans.encode();
+        let ms = Miniscript::<bitcoin::PublicKey, Segwitv0>::parse_insane(&enc).unwrap();
+        assert_eq!(ms_trans.encode(), ms.encode());
+    }
 }