Merge #660: Introduce Threshold type and use it in SortedMulti as a p…

…review

50bf404 policy: use new Threshold type in semantic policy (Andrew Poelstra)
5964ec3 sortedmulti: use new Threshold type internally (Andrew Poelstra)
36ea5cc expression: add method to validate and construct a threshold (Andrew Poelstra)
452615b rename expression.rs to expression/mod.rs (Andrew Poelstra)
dbf7b32 primitives: introduce Threshold type (Andrew Poelstra)
30a3b59 psbt: fix "redundant target" doc warning (Andrew Poelstra)

Pull request description:

  I have a big branch which uses this type throughout the codebase. I split it up into pieces (separate commit for its use in each policy type, for its use in multi/multi_a, for its use in Terminal::thresh, etc) but it's still a pretty big diff.

  Despite the size of the diff I think this is definitely worth doing -- in my current branch we have eliminated a ton of error paths, including a ton of badly-typed ones (grepping the codebase for `errstr` shows I've reduced it from 30 instances to 18).

  This PR simply introduces the new `Threshold` type and uses it in `SortedMultiVec` to show how the API works. Later PRs will do the more invasive stuff.

ACKs for top commit:
  tcharding:
    ACK 50bf404
  sanket1729:
    ACK 50bf404.

Tree-SHA512: 084034f43a66cb6e73446549aad1e1a01f7f0067e7ab3b401f8d819f7375f7a0f6b695c013e3917f550d07b579dcd8ca21adf6dd854bb82c824911e8d1ead152

src/descriptor/mod.rs

@@ -1042,18 +1042,17 @@ mod tests {
         StdDescriptor::from_str("(\u{7f}()3").unwrap_err();
         StdDescriptor::from_str("pk()").unwrap_err();
         StdDescriptor::from_str("nl:0").unwrap_err(); //issue 63
-        let compressed_pk = String::from("");
         assert_eq!(
             StdDescriptor::from_str("sh(sortedmulti)")
                 .unwrap_err()
                 .to_string(),
-            "unexpected «no arguments given for sortedmulti»"
+            "expected threshold, found terminal",
         ); //issue 202
         assert_eq!(
-            StdDescriptor::from_str(&format!("sh(sortedmulti(2,{}))", compressed_pk))
+            StdDescriptor::from_str(&format!("sh(sortedmulti(2,{}))", &TEST_PK[3..69]))
                 .unwrap_err()
                 .to_string(),
-            "unexpected «higher threshold than there were keys in sortedmulti»"
+            "invalid threshold 2-of-1; cannot have k > n",
         ); //issue 202
 
         StdDescriptor::from_str(TEST_PK).unwrap();

src/descriptor/sortedmulti.rs

@@ -19,62 +19,55 @@ use crate::plan::AssetProvider;
 use crate::prelude::*;
 use crate::sync::Arc;
 use crate::{
-    errstr, expression, policy, script_num_size, Error, ForEachKey, Miniscript, MiniscriptKey,
-    Satisfier, ToPublicKey, TranslateErr, Translator,
+    expression, policy, script_num_size, Error, ForEachKey, Miniscript, MiniscriptKey, Satisfier,
+    Threshold, ToPublicKey, TranslateErr, Translator,
 };
 
 /// Contents of a "sortedmulti" descriptor
 #[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct SortedMultiVec<Pk: MiniscriptKey, Ctx: ScriptContext> {
-    /// signatures required
-    pub k: usize,
-    /// public keys inside sorted Multi
-    pub pks: Vec<Pk>,
+    inner: Threshold<Pk, MAX_PUBKEYS_PER_MULTISIG>,
     /// The current ScriptContext for sortedmulti
-    pub(crate) phantom: PhantomData<Ctx>,
+    phantom: PhantomData<Ctx>,
 }
 
 impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
-    /// Create a new instance of `SortedMultiVec` given a list of keys and the threshold
-    ///
-    /// Internally checks all the applicable size limits and pubkey types limitations according to the current `Ctx`.
-    pub fn new(k: usize, pks: Vec<Pk>) -> Result<Self, Error> {
-        // A sortedmulti() is only defined for <= 20 keys (it maps to CHECKMULTISIG)
-        if pks.len() > MAX_PUBKEYS_PER_MULTISIG {
-            return Err(Error::BadDescriptor("Too many public keys".to_string()));
-        }
-
+    fn constructor_check(&self) -> Result<(), Error> {
         // Check the limits before creating a new SortedMultiVec
         // For example, under p2sh context the scriptlen can only be
         // upto 520 bytes.
-        let term: Terminal<Pk, Ctx> = Terminal::Multi(k, pks.clone());
+        let term: Terminal<Pk, Ctx> = Terminal::Multi(self.inner.k(), self.inner.data().to_owned());
         let ms = Miniscript::from_ast(term)?;
-
         // This would check all the consensus rules for p2sh/p2wsh and
         // even tapscript in future
-        Ctx::check_local_validity(&ms)?;
+        Ctx::check_local_validity(&ms).map_err(From::from)
+    }
 
-        Ok(Self { k, pks, phantom: PhantomData })
+    /// Create a new instance of `SortedMultiVec` given a list of keys and the threshold
+    ///
+    /// Internally checks all the applicable size limits and pubkey types limitations according to the current `Ctx`.
+    pub fn new(k: usize, pks: Vec<Pk>) -> Result<Self, Error> {
+        let ret =
+            Self { inner: Threshold::new(k, pks).map_err(Error::Threshold)?, phantom: PhantomData };
+        ret.constructor_check()?;
+        Ok(ret)
     }
+
     /// Parse an expression tree into a SortedMultiVec
     pub fn from_tree(tree: &expression::Tree) -> Result<Self, Error>
     where
         Pk: FromStr,
         <Pk as FromStr>::Err: fmt::Display,
     {
-        if tree.args.is_empty() {
-            return Err(errstr("no arguments given for sortedmulti"));
-        }
-        let k = expression::parse_num(tree.args[0].name)?;
-        if k > (tree.args.len() - 1) as u32 {
-            return Err(errstr("higher threshold than there were keys in sortedmulti"));
-        }
-        let pks: Result<Vec<Pk>, _> = tree.args[1..]
-            .iter()
-            .map(|sub| expression::terminal(sub, Pk::from_str))
-            .collect();
-
-        pks.map(|pks| SortedMultiVec::new(k as usize, pks))?
+        let ret = Self {
+            inner: tree
+                .to_null_threshold()
+                .map_err(Error::ParseThreshold)?
+                .translate_by_index(|i| expression::terminal(&tree.args[i + 1], Pk::from_str))?,
+            phantom: PhantomData,
+        };
+        ret.constructor_check()?;
+        Ok(ret)
     }
 
     /// This will panic if fpk returns an uncompressed key when
@@ -88,23 +81,39 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
         T: Translator<Pk, Q, FuncError>,
         Q: MiniscriptKey,
     {
-        let pks: Result<Vec<Q>, _> = self.pks.iter().map(|pk| t.pk(pk)).collect();
-        let res = SortedMultiVec::new(self.k, pks?).map_err(TranslateErr::OuterError)?;
-        Ok(res)
+        let ret = SortedMultiVec {
+            inner: self.inner.translate_ref(|pk| t.pk(pk))?,
+            phantom: PhantomData,
+        };
+        ret.constructor_check().map_err(TranslateErr::OuterError)?;
+        Ok(ret)
     }
+
+    /// The threshold value for the multisig.
+    pub fn k(&self) -> usize { self.inner.k() }
+
+    /// The number of keys in the multisig.
+    pub fn n(&self) -> usize { self.inner.n() }
+
+    /// Accessor for the public keys in the multisig.
+    ///
+    /// The keys in this structure might **not** be sorted. In general, they cannot be
+    /// sorted until they are converted to consensus-encoded public keys, which may not
+    /// be possible (for example for BIP32 paths with unfilled wildcards).
+    pub fn pks(&self) -> &[Pk] { self.inner.data() }
 }
 
 impl<Pk: MiniscriptKey, Ctx: ScriptContext> ForEachKey<Pk> for SortedMultiVec<Pk, Ctx> {
     fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, pred: F) -> bool {
-        self.pks.iter().all(pred)
+        self.pks().iter().all(pred)
     }
 }
 
 impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
     /// utility function to sanity a sorted multi vec
     pub fn sanity_check(&self) -> Result<(), Error> {
         let ms: Miniscript<Pk, Ctx> =
-            Miniscript::from_ast(Terminal::Multi(self.k, self.pks.clone()))
+            Miniscript::from_ast(Terminal::Multi(self.k(), self.pks().to_owned()))
                 .expect("Must typecheck");
         // '?' for doing From conversion
         ms.sanity_check()?;
@@ -118,7 +127,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
     where
         Pk: ToPublicKey,
     {
-        let mut pks = self.pks.clone();
+        let mut pks = self.pks().to_owned();
         // Sort pubkeys lexicographically according to BIP 67
         pks.sort_by(|a, b| {
             a.to_public_key()
@@ -127,7 +136,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
                 .partial_cmp(&b.to_public_key().inner.serialize())
                 .unwrap()
         });
-        Terminal::Multi(self.k, pks)
+        Terminal::Multi(self.k(), pks)
     }
 
     /// Encode as a Bitcoin script
@@ -169,10 +178,10 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
     /// to instead call the corresponding function on a `Descriptor`, which
     /// will handle the segwit/non-segwit technicalities for you.
     pub fn script_size(&self) -> usize {
-        script_num_size(self.k)
+        script_num_size(self.k())
             + 1
-            + script_num_size(self.pks.len())
-            + self.pks.iter().map(|pk| Ctx::pk_len(pk)).sum::<usize>()
+            + script_num_size(self.n())
+            + self.pks().iter().map(|pk| Ctx::pk_len(pk)).sum::<usize>()
     }
 
     /// Maximum number of witness elements used to satisfy the Miniscript
@@ -183,7 +192,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
     /// This function may panic on malformed `Miniscript` objects which do
     /// not correspond to semantically sane Scripts. (Such scripts should be
     /// rejected at parse time. Any exceptions are bugs.)
-    pub fn max_satisfaction_witness_elements(&self) -> usize { 2 + self.k }
+    pub fn max_satisfaction_witness_elements(&self) -> usize { 2 + self.k() }
 
     /// Maximum size, in bytes, of a satisfying witness.
     /// In general, it is not recommended to use this function directly, but
@@ -193,19 +202,16 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
     /// All signatures are assumed to be 73 bytes in size, including the
     /// length prefix (segwit) or push opcode (pre-segwit) and sighash
     /// postfix.
-    pub fn max_satisfaction_size(&self) -> usize { 1 + 73 * self.k }
+    pub fn max_satisfaction_size(&self) -> usize { 1 + 73 * self.k() }
 }
 
 impl<Pk: MiniscriptKey, Ctx: ScriptContext> policy::Liftable<Pk> for SortedMultiVec<Pk, Ctx> {
     fn lift(&self) -> Result<policy::semantic::Policy<Pk>, Error> {
-        let ret = policy::semantic::Policy::Thresh(
-            self.k,
-            self.pks
-                .iter()
-                .map(|k| Arc::new(policy::semantic::Policy::Key(k.clone())))
-                .collect(),
-        );
-        Ok(ret)
+        Ok(policy::semantic::Policy::Thresh(
+            self.inner
+                .map_ref(|pk| Arc::new(policy::semantic::Policy::Key(pk.clone())))
+                .forget_maximum(),
+        ))
     }
 }
 
@@ -215,11 +221,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> fmt::Debug for SortedMultiVec<Pk, Ct
 
 impl<Pk: MiniscriptKey, Ctx: ScriptContext> fmt::Display for SortedMultiVec<Pk, Ctx> {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(f, "sortedmulti({}", self.k)?;
-        for k in &self.pks {
-            write!(f, ",{}", k)?;
-        }
-        f.write_str(")")
+        fmt::Display::fmt(&self.inner.display("sortedmulti", true), f)
     }
 }
 
@@ -249,7 +251,7 @@ mod tests {
         let error = res.expect_err("constructor should err");
 
         match error {
-            Error::BadDescriptor(_) => {} // ok
+            Error::Threshold(_) => {} // ok
             other => panic!("unexpected error: {:?}", other),
         }
     }

src/descriptor/tr.rs

@@ -23,7 +23,7 @@ use crate::policy::Liftable;
 use crate::prelude::*;
 use crate::util::{varint_len, witness_size};
 use crate::{
-    errstr, Error, ForEachKey, FromStrKey, MiniscriptKey, Satisfier, ScriptContext, Tap,
+    errstr, Error, ForEachKey, FromStrKey, MiniscriptKey, Satisfier, ScriptContext, Tap, Threshold,
     ToPublicKey, TranslateErr, TranslatePk, Translator,
 };
 
@@ -620,8 +620,7 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for TapTree<Pk> {
         fn lift_helper<Pk: MiniscriptKey>(s: &TapTree<Pk>) -> Result<Policy<Pk>, Error> {
             match *s {
                 TapTree::Tree { ref left, ref right, height: _ } => Ok(Policy::Thresh(
-                    1,
-                    vec![Arc::new(lift_helper(left)?), Arc::new(lift_helper(right)?)],
+                    Threshold::or(Arc::new(lift_helper(left)?), Arc::new(lift_helper(right)?)),
                 )),
                 TapTree::Leaf(ref leaf) => leaf.lift(),
             }
@@ -635,13 +634,10 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for TapTree<Pk> {
 impl<Pk: MiniscriptKey> Liftable<Pk> for Tr<Pk> {
     fn lift(&self) -> Result<Policy<Pk>, Error> {
         match &self.tree {
-            Some(root) => Ok(Policy::Thresh(
-                1,
-                vec![
-                    Arc::new(Policy::Key(self.internal_key.clone())),
-                    Arc::new(root.lift()?),
-                ],
-            )),
+            Some(root) => Ok(Policy::Thresh(Threshold::or(
+                Arc::new(Policy::Key(self.internal_key.clone())),
+                Arc::new(root.lift()?),
+            ))),
             None => Ok(Policy::Key(self.internal_key.clone())),
         }
     }

src/expression/error.rs

@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: CC0-1.0
+
+//! Expression-related errors
+
+use core::fmt;
+
+use crate::prelude::*;
+use crate::ThresholdError;
+
+/// Error parsing a threshold expression.
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub enum ParseThresholdError {
+    /// Expression had no children, not even a threshold value.
+    NoChildren,
+    /// The threshold value appeared to be a sub-expression rather than a number.
+    KNotTerminal,
+    /// Failed to parse the threshold value.
+    // FIXME this should be a more specific type. Will be handled in a later PR
+    // that rewrites the expression parsing logic.
+    ParseK(String),
+    /// Threshold parameters were invalid.
+    Threshold(ThresholdError),
+}
+
+impl fmt::Display for ParseThresholdError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        use ParseThresholdError::*;
+
+        match *self {
+            NoChildren => f.write_str("expected threshold, found terminal"),
+            KNotTerminal => f.write_str("expected positive integer, found expression"),
+            ParseK(ref x) => write!(f, "failed to parse threshold value {}", x),
+            Threshold(ref e) => e.fmt(f),
+        }
+    }
+}
+
+#[cfg(feature = "std")]
+impl std::error::Error for ParseThresholdError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        use ParseThresholdError::*;
+
+        match *self {
+            NoChildren => None,
+            KNotTerminal => None,
+            ParseK(..) => None,
+            Threshold(ref e) => Some(e),
+        }
+    }
+}

src/expression.rs → src/expression/mod.rs

@@ -2,11 +2,15 @@
 
 //! # Function-like Expression Language
 //!
+
+mod error;
+
 use core::fmt;
 use core::str::FromStr;
 
+pub use self::error::ParseThresholdError;
 use crate::prelude::*;
-use crate::{errstr, Error, MAX_RECURSION_DEPTH};
+use crate::{errstr, Error, Threshold, MAX_RECURSION_DEPTH};
 
 /// Allowed characters are descriptor strings.
 pub const INPUT_CHARSET: &str = "0123456789()[],'/*abcdefgh@:$%{}IJKLMNOPQRSTUVWXYZ&+-.;<=>?!^_|~ijklmnopqrstuvwxyzABCDEFGH`#\"\\ ";
@@ -185,6 +189,35 @@ impl<'a> Tree<'a> {
             Err(errstr(rem))
         }
     }
+
+    /// Parses an expression tree as a threshold (a term with at least one child,
+    /// the first of which is a positive integer k).
+    ///
+    /// This sanity-checks that the threshold is well-formed (begins with a valid
+    /// threshold value, etc.) but does not parse the children of the threshold.
+    /// Instead it returns a threshold holding the empty type `()`, which is
+    /// constructed without any allocations, and expects the caller to convert
+    /// this to the "real" threshold type by calling [`Threshold::translate`].
+    ///
+    /// (An alternate API which does the conversion inline turned out to be
+    /// too messy; it needs to take a closure, have multiple generic parameters,
+    /// and be able to return multiple error types.)
+    pub fn to_null_threshold<const MAX: usize>(
+        &self,
+    ) -> Result<Threshold<(), MAX>, ParseThresholdError> {
+        // First, special case "no arguments" so we can index the first argument without panics.
+        if self.args.is_empty() {
+            return Err(ParseThresholdError::NoChildren);
+        }
+
+        if !self.args[0].args.is_empty() {
+            return Err(ParseThresholdError::KNotTerminal);
+        }
+
+        let k = parse_num(self.args[0].name)
+            .map_err(|e| ParseThresholdError::ParseK(e.to_string()))? as usize;
+        Threshold::new(k, vec![(); self.args.len() - 1]).map_err(ParseThresholdError::Threshold)
+    }
 }
 
 /// Filter out non-ASCII because we byte-index strings all over the