Merge #828: Various cleanups, mostly around the compiler

bf10e45 compiler: pull non-binary argument errors from PolicyError into CompilerError (Andrew Poelstra)
3bc6c44 compiler: use Miniscript terminal constructors directly (Andrew Poelstra)
b8d8910 miniscript: add infallible multi() and multi_a() constructors (Andrew Poelstra)
fcbcf4e compiler: revert malleability check to simpler version (Andrew Poelstra)
f98f263 compiler: forbid fragment probabilities of 0 (Andrew Poelstra)
16b4d60 fuzz/generate-files: support jujutsu (Andrew Poelstra)
e000146 tr: cache TrSpendInfo when computing it (Andrew Poelstra)
2284858 tr: add `into_control_block` accessor to TrSpendInfoIterItem (Andrew Poelstra)

Pull request description:

  This PR is a grab back of prepatory commits for larger changes. It includes a couple of followups to #815 in the first commits. Then it cleans up some error types and paths.
  
  I don't believe anything in here will be tricky or controversial, though there are API breaks because of the cleaned up error types.


ACKs for top commit:
  sanket1729:
    ACK bf10e45


Tree-SHA512: 8a6c9006fb6473187a2ba69f0311fc29756c5f99544e35ab499658910e8f82a10eff740a19634b5d0c6807c583b8cd79fdc9fbf8a59b78ab96d5b387759db2b7

Author: apoelstra

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

@@ -25,6 +25,7 @@ parse_descriptor,
 parse_descriptor_definite,
 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"
 
@@ -46,6 +46,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,
         }
     }
 }