compiler: forbid fragment probabilities of 0

We can cause panics in the compiler comparing OrdF64s which are NaN,
which we can produce by parsing ors where every child has weight 0.

Simply make it a parse error to have any zero-probability branches.
In theory somebody might want an or where some fragments have positive
probability and others are 0, indicating "maximally pessimize these".
But it has never occurred to me to do this, and it's easy to simulate
this by using 100s and 1s, say.

Author: apoelstra

src/expression/error.rs

@@ -197,6 +197,13 @@ pub enum ParseNumError {
     StdParse(num::ParseIntError),
     /// Number had a leading zero, + or -.
     InvalidLeadingDigit(char),
+    /// Number was 0 in a context where zero is not allowed.
+    ///
+    /// Be aware that locktimes have their own error types and do not use this variant.
+    IllegalZero {
+        /// A description of the location where 0 was not allowed.
+        context: &'static str,
+    },
 }
 
 impl fmt::Display for ParseNumError {
@@ -206,6 +213,9 @@ impl fmt::Display for ParseNumError {
             ParseNumError::InvalidLeadingDigit(ch) => {
                 write!(f, "numbers must start with 1-9, not {}", ch)
             }
+            ParseNumError::IllegalZero { context } => {
+                write!(f, "{} may not be 0", context)
+            }
         }
     }
 }
@@ -216,6 +226,7 @@ impl std::error::Error for ParseNumError {
         match self {
             ParseNumError::StdParse(ref e) => Some(e),
             ParseNumError::InvalidLeadingDigit(..) => None,
+            ParseNumError::IllegalZero { .. } => None,
         }
     }
 }

src/expression/mod.rs

@@ -678,11 +678,10 @@ impl<'a> Tree<'a> {
     }
 }
 
-/// Parse a string as a u32, for timelocks or thresholds
-pub fn parse_num(s: &str) -> Result<u32, ParseNumError> {
+/// Parse a string as a u32, forbidding zero.
+pub fn parse_num_nonzero(s: &str, context: &'static str) -> Result<u32, ParseNumError> {
     if s == "0" {
-        // Special-case 0 since it is the only number which may start with a leading zero.
-        return Ok(0);
+        return Err(ParseNumError::IllegalZero { context });
     }
     if let Some(ch) = s.chars().next() {
         if !('1'..='9').contains(&ch) {
@@ -692,6 +691,15 @@ pub fn parse_num(s: &str) -> Result<u32, ParseNumError> {
     u32::from_str(s).map_err(ParseNumError::StdParse)
 }
 
+/// Parse a string as a u32, for timelocks or thresholds
+pub fn parse_num(s: &str) -> Result<u32, ParseNumError> {
+    if s == "0" {
+        // Special-case 0 since it is the only number which may start with a leading zero.
+        return Ok(0);
+    }
+    parse_num_nonzero(s, "")
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -772,6 +780,7 @@ mod tests {
     #[test]
     fn test_parse_num() {
         assert!(parse_num("0").is_ok());
+        assert!(parse_num_nonzero("0", "").is_err());
         assert!(parse_num("00").is_err());
         assert!(parse_num("0000").is_err());
         assert!(parse_num("06").is_err());

src/policy/concrete.rs

@@ -883,7 +883,7 @@ impl<Pk: FromStrKey> expression::FromTree for Policy<Pk> {
 
             let frag_prob = match frag_prob {
                 None => 1,
-                Some(s) => expression::parse_num(s)
+                Some(s) => expression::parse_num_nonzero(s, "fragment probability")
                     .map_err(From::from)
                     .map_err(Error::Parse)? as usize,
             };
@@ -1231,4 +1231,11 @@ mod tests {
         // This implicitly tests the check_timelocks API (has height and time locks).
         let _ = Policy::<String>::from_str("and(after(10),after(500000000))").unwrap();
     }
+
+    #[test]
+    fn parse_zero_disjunction() {
+        "or(0@pk(09),0@TRIVIAL)"
+            .parse::<Policy<String>>()
+            .unwrap_err();
+    }
 }