Merge rust-bitcoin#676: Use threshold in Terminal and Satisfaction

6925ae6 clippy: remove now-unneeded i64 import (Andrew Poelstra)
0fb47b7 satisfaction: use new Threshold type (Andrew Poelstra)
ff8f077 miniscript: use new Threshold type for thresh (Andrew Poelstra)
95f8dac compiler: eliminate unused error paths (Andrew Poelstra)

Pull request description:

  This completes the "threshold" conversion and completely drops the various ad-hoc error paths related to threshold values (most of which were just random strings).

  The next set of PRs will be related to cleaning up errors in general a bit, to strongly-type things and add span information. But I am struggling a bit with how to box the associated error types for the MiniscriptKey types so it may be a while. (Very hard to do this in a std/nostd compatible way and the final result will probably be inconvenient/annoying for users.)

ACKs for top commit:
  tcharding:
    ACK 6925ae6
  sanket1729:
    ACK 6925ae6

Tree-SHA512: 04344be051cb5b16f691eafdb64818ffd44c9adc15f71ad6c664e9f337dc4ed52be66a540bf5c99eeb4b384a95d93e2518f290b84cf6abf9cb092e789b57a3fa

src/descriptor/mod.rs

@@ -1498,7 +1498,7 @@ mod tests {
     #[test]
     fn empty_thresh() {
         let descriptor = Descriptor::<bitcoin::PublicKey>::from_str("thresh");
-        assert_eq!(descriptor.unwrap_err().to_string(), "unexpected «no arguments given»")
+        assert_eq!(descriptor.unwrap_err().to_string(), "expected threshold, found terminal");
     }
 
     #[test]

src/interpreter/mod.rs

@@ -800,16 +800,20 @@ where
                         None => return Some(Err(Error::UnexpectedStackEnd)),
                     }
                 }
-                Terminal::Thresh(ref _k, ref subs) if node_state.n_evaluated == 0 => {
+                Terminal::Thresh(ref thresh) if node_state.n_evaluated == 0 => {
                     self.push_evaluation_state(node_state.node, 1, 0);
-                    self.push_evaluation_state(&subs[0], 0, 0);
+                    self.push_evaluation_state(&thresh.data()[0], 0, 0);
                 }
-                Terminal::Thresh(k, ref subs) if node_state.n_evaluated == subs.len() => {
+                Terminal::Thresh(ref thresh) if node_state.n_evaluated == thresh.n() => {
                     match self.stack.pop() {
-                        Some(stack::Element::Dissatisfied) if node_state.n_satisfied == k => {
+                        Some(stack::Element::Dissatisfied)
+                            if node_state.n_satisfied == thresh.k() =>
+                        {
                             self.stack.push(stack::Element::Satisfied)
                         }
-                        Some(stack::Element::Satisfied) if node_state.n_satisfied == k - 1 => {
+                        Some(stack::Element::Satisfied)
+                            if node_state.n_satisfied == thresh.k() - 1 =>
+                        {
                             self.stack.push(stack::Element::Satisfied)
                         }
                         Some(stack::Element::Satisfied) | Some(stack::Element::Dissatisfied) => {
@@ -821,23 +825,31 @@ where
                         None => return Some(Err(Error::UnexpectedStackEnd)),
                     }
                 }
-                Terminal::Thresh(ref _k, ref subs) if node_state.n_evaluated != 0 => {
+                Terminal::Thresh(ref thresh) if node_state.n_evaluated != 0 => {
                     match self.stack.pop() {
                         Some(stack::Element::Dissatisfied) => {
                             self.push_evaluation_state(
                                 node_state.node,
                                 node_state.n_evaluated + 1,
                                 node_state.n_satisfied,
                             );
-                            self.push_evaluation_state(&subs[node_state.n_evaluated], 0, 0);
+                            self.push_evaluation_state(
+                                &thresh.data()[node_state.n_evaluated],
+                                0,
+                                0,
+                            );
                         }
                         Some(stack::Element::Satisfied) => {
                             self.push_evaluation_state(
                                 node_state.node,
                                 node_state.n_evaluated + 1,
                                 node_state.n_satisfied + 1,
                             );
-                            self.push_evaluation_state(&subs[node_state.n_evaluated], 0, 0);
+                            self.push_evaluation_state(
+                                &thresh.data()[node_state.n_evaluated],
+                                0,
+                                0,
+                            );
                         }
                         Some(stack::Element::Push(_v)) => {
                             return Some(Err(Error::UnexpectedStackElementPush))

src/iter/mod.rs

@@ -37,7 +37,7 @@ impl<'a, Pk: MiniscriptKey, Ctx: ScriptContext> TreeLike for &'a Miniscript<Pk,
             | OrC(ref left, ref right)
             | OrI(ref left, ref right) => Tree::Binary(left, right),
             AndOr(ref a, ref b, ref c) => Tree::Nary(Arc::from([a.as_ref(), b, c])),
-            Thresh(_, ref subs) => Tree::Nary(subs.iter().map(Arc::as_ref).collect()),
+            Thresh(ref thresh) => Tree::Nary(thresh.iter().map(Arc::as_ref).collect()),
         }
     }
 }
@@ -64,7 +64,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> TreeLike for Arc<Miniscript<Pk, Ctx>
             AndOr(ref a, ref b, ref c) => {
                 Tree::Nary(Arc::from([Arc::clone(a), Arc::clone(b), Arc::clone(c)]))
             }
-            Thresh(_, ref subs) => Tree::Nary(subs.iter().map(Arc::clone).collect()),
+            Thresh(ref thresh) => Tree::Nary(thresh.iter().map(Arc::clone).collect()),
         }
     }
 }

src/miniscript/astelem.rs

@@ -19,8 +19,8 @@ use crate::miniscript::{types, ScriptContext};
 use crate::prelude::*;
 use crate::util::MsKeyBuilder;
 use crate::{
-    errstr, expression, AbsLockTime, Error, FromStrKey, Miniscript, MiniscriptKey, RelLockTime,
-    Terminal, ToPublicKey,
+    expression, AbsLockTime, Error, FromStrKey, Miniscript, MiniscriptKey, RelLockTime, Terminal,
+    ToPublicKey,
 };
 
 impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> {
@@ -81,7 +81,13 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> {
             {
                 fmt_2(f, "or_i(", l, r, is_debug)
             }
-            Terminal::Thresh(k, ref subs) => fmt_n(f, "thresh(", k, subs, is_debug),
+            Terminal::Thresh(ref thresh) => {
+                if is_debug {
+                    fmt::Debug::fmt(&thresh.debug("thresh", true), f)
+                } else {
+                    fmt::Display::fmt(&thresh.display("thresh", true), f)
+                }
+            }
             Terminal::Multi(ref thresh) => {
                 if is_debug {
                     fmt::Debug::fmt(&thresh.debug("multi", true), f)
@@ -168,21 +174,6 @@ fn fmt_2<D: fmt::Debug + fmt::Display>(
     conditional_fmt(f, b, is_debug)?;
     f.write_str(")")
 }
-fn fmt_n<D: fmt::Debug + fmt::Display>(
-    f: &mut fmt::Formatter,
-    name: &str,
-    first: usize,
-    list: &[D],
-    is_debug: bool,
-) -> fmt::Result {
-    f.write_str(name)?;
-    write!(f, "{}", first)?;
-    for el in list {
-        f.write_str(",")?;
-        conditional_fmt(f, el, is_debug)?;
-    }
-    f.write_str(")")
-}
 fn conditional_fmt<D: fmt::Debug + fmt::Display>(
     f: &mut fmt::Formatter,
     data: &D,
@@ -307,25 +298,11 @@ impl<Pk: FromStrKey, Ctx: ScriptContext> crate::expression::FromTree for Termina
             ("or_d", 2) => expression::binary(top, Terminal::OrD),
             ("or_c", 2) => expression::binary(top, Terminal::OrC),
             ("or_i", 2) => expression::binary(top, Terminal::OrI),
-            ("thresh", n) => {
-                if n == 0 {
-                    return Err(errstr("no arguments given"));
-                }
-                let k = expression::terminal(&top.args[0], expression::parse_num)? as usize;
-                if k > n - 1 {
-                    return Err(errstr("higher threshold than there are subexpressions"));
-                }
-                if n == 1 {
-                    return Err(errstr("empty thresholds not allowed in descriptors"));
-                }
-
-                let subs: Result<Vec<Arc<Miniscript<Pk, Ctx>>>, _> = top.args[1..]
-                    .iter()
-                    .map(expression::FromTree::from_tree)
-                    .collect();
-
-                Ok(Terminal::Thresh(k, subs?))
-            }
+            ("thresh", _) => top
+                .to_null_threshold()
+                .map_err(Error::ParseThreshold)?
+                .translate_by_index(|i| Miniscript::from_tree(&top.args[1 + i]).map(Arc::new))
+                .map(Terminal::Thresh),
             ("multi", _) => top
                 .to_null_threshold()
                 .map_err(Error::ParseThreshold)?
@@ -475,13 +452,13 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> {
                 .push_opcode(opcodes::all::OP_ELSE)
                 .push_astelem(right)
                 .push_opcode(opcodes::all::OP_ENDIF),
-            Terminal::Thresh(k, ref subs) => {
-                builder = builder.push_astelem(&subs[0]);
-                for sub in &subs[1..] {
+            Terminal::Thresh(ref thresh) => {
+                builder = builder.push_astelem(&thresh.data()[0]);
+                for sub in &thresh.data()[1..] {
                     builder = builder.push_astelem(sub).push_opcode(opcodes::all::OP_ADD);
                 }
                 builder
-                    .push_int(k as i64)
+                    .push_int(thresh.k() as i64)
                     .push_opcode(opcodes::all::OP_EQUAL)
             }
             Terminal::Multi(ref thresh) => {

src/miniscript/decode.rs

@@ -181,7 +181,7 @@ pub enum Terminal<Pk: MiniscriptKey, Ctx: ScriptContext> {
     OrI(Arc<Miniscript<Pk, Ctx>>, Arc<Miniscript<Pk, Ctx>>),
     // Thresholds
     /// `[E] ([W] ADD)* k EQUAL`
-    Thresh(usize, Vec<Arc<Miniscript<Pk, Ctx>>>),
+    Thresh(Threshold<Arc<Miniscript<Pk, Ctx>>, 0>),
     /// `k (<key>)* n CHECKMULTISIG`
     Multi(Threshold<Pk, MAX_PUBKEYS_PER_MULTISIG>),
     /// `<key> CHECKSIG (<key> CHECKSIGADD)*(n-1) k NUMEQUAL`
@@ -549,7 +549,7 @@ pub fn parse<Ctx: ScriptContext>(
                 for _ in 0..n {
                     subs.push(Arc::new(term.pop().unwrap()));
                 }
-                term.reduce0(Terminal::Thresh(k, subs))?;
+                term.reduce0(Terminal::Thresh(Threshold::new(k, subs).map_err(Error::Threshold)?))?;
             }
             Some(NonTerm::EndIf) => {
                 match_token!(

src/miniscript/iter.rs

@@ -50,7 +50,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Miniscript<Pk, Ctx> {
 
             Terminal::AndOr(ref node1, ref node2, ref node3) => vec![node1, node2, node3],
 
-            Terminal::Thresh(_, ref node_vec) => node_vec.iter().map(Arc::deref).collect(),
+            Terminal::Thresh(ref thresh) => thresh.iter().map(Arc::deref).collect(),
 
             _ => vec![],
         }
@@ -82,7 +82,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Miniscript<Pk, Ctx> {
             | (1, Terminal::AndOr(_, node, _))
             | (2, Terminal::AndOr(_, _, node)) => Some(node),
 
-            (n, Terminal::Thresh(_, node_vec)) => node_vec.get(n).map(|x| &**x),
+            (n, Terminal::Thresh(thresh)) => thresh.data().get(n).map(|x| &**x),
 
             _ => None,
         }

src/miniscript/mod.rs

@@ -148,11 +148,10 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Miniscript<Pk, Ctx> {
                 Terminal::After(n) => script_num_size(n.to_consensus_u32() as usize) + 1,
                 Terminal::Older(n) => script_num_size(n.to_consensus_u32() as usize) + 1,
                 Terminal::Verify(ref sub) => usize::from(!sub.ext.has_free_verify),
-                Terminal::Thresh(k, ref subs) => {
-                    assert!(!subs.is_empty(), "threshold must be nonempty");
-                    script_num_size(k) // k
+                Terminal::Thresh(ref thresh) => {
+                    script_num_size(thresh.k()) // k
                         + 1 // EQUAL
-                        + subs.len() // ADD
+                        + thresh.n() // ADD
                         - 1 // no ADD on first element
                 }
                 Terminal::Multi(ref thresh) => {
@@ -492,9 +491,9 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Miniscript<Pk, Ctx> {
                 Terminal::OrD(..) => Terminal::OrD(child_n(0), child_n(1)),
                 Terminal::OrC(..) => Terminal::OrC(child_n(0), child_n(1)),
                 Terminal::OrI(..) => Terminal::OrI(child_n(0), child_n(1)),
-                Terminal::Thresh(k, ref subs) => {
-                    Terminal::Thresh(k, (0..subs.len()).map(child_n).collect())
-                }
+                Terminal::Thresh(ref thresh) => Terminal::Thresh(
+                    thresh.map_from_post_order_iter(&data.child_indices, &translated),
+                ),
                 Terminal::Multi(ref thresh) => Terminal::Multi(thresh.translate_ref(|k| t.pk(k))?),
                 Terminal::MultiA(ref thresh) => {
                     Terminal::MultiA(thresh.translate_ref(|k| t.pk(k))?)