Various cleanups, mostly around the compiler

.github/workflows/cron-daily-fuzz.yml

@@ -24,6 +24,7 @@ miniscript_satisfy,
 parse_descriptor,
 parse_descriptor_priv,
 parse_descriptor_secret,
+regression_compiler,
 regression_descriptor_parse,
 regression_taptree,
 roundtrip_concrete,

fuzz/Cargo.toml

@@ -14,7 +14,7 @@ honggfuzz = { version = "0.5.56", default-features = false }
 # We shouldn't need an explicit version on the next line, but Andrew's tools
 # choke on it otherwise. See https://github.com/nix-community/crate2nix/issues/373
 miniscript = { path = "..", features = [ "compiler" ], version = "13.0" }
-old_miniscript = { package = "miniscript", version = "12.3" }
+old_miniscript = { package = "miniscript", features = [ "compiler" ], version = "12.3" }
 
 regex = "1.0"
 
@@ -42,6 +42,10 @@ path = "fuzz_targets/parse_descriptor_priv.rs"
 name = "parse_descriptor_secret"
 path = "fuzz_targets/parse_descriptor_secret.rs"
 
+[[bin]]
+name = "regression_compiler"
+path = "fuzz_targets/regression_compiler.rs"
+
 [[bin]]
 name = "regression_descriptor_parse"
 path = "fuzz_targets/regression_descriptor_parse.rs"

fuzz/cycle.sh

@@ -6,7 +6,7 @@
 # For hfuzz options see https://github.com/google/honggfuzz/blob/master/docs/USAGE.md
 
 set -e
-REPO_DIR=$(git rev-parse --show-toplevel)
+REPO_DIR=$(git rev-parse --show-toplevel || jj workspace root)
 # can't find the file because of the ENV var
 # shellcheck source=/dev/null
 source "$REPO_DIR/fuzz/fuzz-util.sh"

fuzz/fuzz-util.sh

@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 
-REPO_DIR=$(git rev-parse --show-toplevel)
+REPO_DIR=$(git rev-parse --show-toplevel || jj workspace root)
 
 # Sort order is effected by locale. See `man sort`.
 # > Set LC_ALL=C to get the traditional sort order that uses native byte values.

fuzz/fuzz.sh

@@ -1,7 +1,7 @@
 #!/usr/bin/env bash
 set -ex
 
-REPO_DIR=$(git rev-parse --show-toplevel)
+REPO_DIR=$(git rev-parse --show-toplevel || jj workspace show)
 
 # can't find the file because of the ENV var
 # shellcheck source=/dev/null

fuzz/fuzz_targets/regression_compiler.rs

@@ -0,0 +1,66 @@
+use descriptor_fuzz::FuzzPk;
+use honggfuzz::fuzz;
+use miniscript::{policy, ParseError, ParseNumError};
+use old_miniscript::policy as old_policy;
+
+type Policy = policy::Concrete<FuzzPk>;
+type OldPolicy = old_policy::Concrete<FuzzPk>;
+
+fn do_test(data: &[u8]) {
+    let data_str = String::from_utf8_lossy(data);
+    match (data_str.parse::<Policy>(), data_str.parse::<OldPolicy>()) {
+        (Err(_), Err(_)) => {}
+        (Ok(x), Err(e)) => panic!("new logic parses {} as {:?}, old fails with {}", data_str, x, e),
+        // These is anew parse error
+        (
+            Err(miniscript::Error::Parse(ParseError::Num(ParseNumError::IllegalZero { .. }))),
+            Ok(_),
+        ) => {}
+        (Err(e), Ok(x)) => {
+            panic!("old logic parses {} as {:?}, new fails with {:?}", data_str, x, e)
+        }
+        (Ok(new), Ok(old)) => {
+            assert_eq!(
+                old.to_string(),
+                new.to_string(),
+                "input {} (left is old, right is new)",
+                data_str
+            );
+
+            let comp = new.compile::<miniscript::Legacy>();
+            let old_comp = old.compile::<old_miniscript::Legacy>();
+
+            match (comp, old_comp) {
+                (Err(_), Err(_)) => {}
+                (Ok(x), Err(e)) => {
+                    panic!("new logic compiles {} as {:?}, old fails with {}", data_str, x, e)
+                }
+                (Err(e), Ok(x)) => {
+                    panic!("old logic compiles {} as {:?}, new fails with {}", data_str, x, e)
+                }
+                (Ok(new), Ok(old)) => {
+                    assert_eq!(
+                        old.to_string(),
+                        new.to_string(),
+                        "input {} (left is old, right is new)",
+                        data_str
+                    );
+                }
+            }
+        }
+    }
+}
+
+fn main() {
+    loop {
+        fuzz!(|data| {
+            do_test(data);
+        });
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    #[test]
+    fn duplicate_crash() { crate::do_test(b"or(0@pk(09),0@TRIVIAL)") }
+}

fuzz/generate-files.sh

@@ -2,7 +2,7 @@
 
 set -e
 
-REPO_DIR=$(git rev-parse --show-toplevel)
+REPO_DIR=$(git rev-parse --show-toplevel || jj workspace root)
 
 # can't find the file because of the ENV var
 # shellcheck source=/dev/null
@@ -26,7 +26,7 @@ honggfuzz = { version = "0.5.56", default-features = false }
 # We shouldn't need an explicit version on the next line, but Andrew's tools
 # choke on it otherwise. See https://github.com/nix-community/crate2nix/issues/373
 miniscript = { path = "..", features = [ "compiler" ], version = "13.0" }
-old_miniscript = { package = "miniscript", version = "12.3" }
+old_miniscript = { package = "miniscript", features = [ "compiler" ], version = "12.3" }
 
 regex = "1.0"
 EOF

src/descriptor/tr/mod.rs

@@ -125,11 +125,19 @@ impl<Pk: MiniscriptKey> Tr<Pk> {
     /// This data is needed to compute the Taproot output, so this method is implicitly
     /// called through [`Self::script_pubkey`], [`Self::address`], etc. It is also needed
     /// to compute the hash needed to sign the output.
-    pub fn spend_info(&self) -> TrSpendInfo<Pk>
+    pub fn spend_info(&self) -> Arc<TrSpendInfo<Pk>>
     where
         Pk: ToPublicKey,
     {
-        TrSpendInfo::from_tr(self)
+        let mut lock = self.spend_info.lock().unwrap();
+        match *lock {
+            Some(ref res) => Arc::clone(res),
+            None => {
+                let arc = Arc::new(TrSpendInfo::from_tr(self));
+                *lock = Some(Arc::clone(&arc));
+                arc
+            }
+        }
     }
 
     /// Checks whether the descriptor is safe.

src/descriptor/tr/spend_info.rs

@@ -339,11 +339,19 @@ impl<'sp, Pk: MiniscriptKey> TrSpendInfoIterItem<'sp, Pk> {
     /// The control block of this leaf.
     ///
     /// Unlike the other data obtainable from [`TrSpendInfoIterItem`], this one is computed
-    /// dynamically during iteration and therefore will not outlive the iterator item. If
-    /// you need access to multiple control blocks at once, will likely need to clone and
-    /// store them separately.
+    /// dynamically during iteration and therefore will not outlive the iterator item. See
+    /// [`Self::into_control_block`], which consumes the iterator item but will give you an
+    /// owned copy of the control block.
+    ///
+    /// If you need access to multiple control blocks at once, you may need to `clone` the
+    /// return value of this method, or call [`Self::into_control_block`], and store the
+    /// result in a separate container.
     #[inline]
     pub fn control_block(&self) -> &ControlBlock { &self.control_block }
+
+    /// Extract the control block of this leaf, consuming `self`.
+    #[inline]
+    pub fn into_control_block(self) -> ControlBlock { self.control_block }
 }
 
 #[cfg(test)]

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/miniscript/mod.rs

@@ -52,10 +52,10 @@ mod ms_tests;
 mod private {
     use core::marker::PhantomData;
 
-    use super::types::{ExtData, Type};
+    use super::limits::{MAX_PUBKEYS_IN_CHECKSIGADD, MAX_PUBKEYS_PER_MULTISIG};
+    use super::types::{self, ExtData, Type};
     use crate::iter::TreeLike as _;
     pub use crate::miniscript::context::ScriptContext;
-    use crate::miniscript::types;
     use crate::prelude::sync::Arc;
     use crate::{AbsLockTime, Error, MiniscriptKey, RelLockTime, Terminal, MAX_RECURSION_DEPTH};
 
@@ -270,6 +270,28 @@ mod private {
             }
         }
 
+        // non-const because Thresh::n is not becasue Vec::len is not (needs Rust 1.87)
+        /// The `multi` combinator.
+        pub fn multi(thresh: crate::Threshold<Pk, MAX_PUBKEYS_PER_MULTISIG>) -> Self {
+            Self {
+                ty: types::Type::multi(),
+                ext: types::extra_props::ExtData::multi(thresh.k(), thresh.n()),
+                node: Terminal::Multi(thresh),
+                phantom: PhantomData,
+            }
+        }
+
+        // non-const because Thresh::n is not becasue Vec::len is not
+        /// The `multi` combinator.
+        pub fn multi_a(thresh: crate::Threshold<Pk, MAX_PUBKEYS_IN_CHECKSIGADD>) -> Self {
+            Self {
+                ty: types::Type::multi_a(),
+                ext: types::extra_props::ExtData::multi_a(thresh.k(), thresh.n()),
+                node: Terminal::MultiA(thresh),
+                phantom: PhantomData,
+            }
+        }
+
         /// Add type information(Type and Extdata) to Miniscript based on
         /// `AstElem` fragment. Dependent on display and clone because of Error
         /// Display code of type_check.