Merge #732: Refactor extract_key logic in Taproot compiler

a195c81 fuzz: add compile_taproot fuzztest (Andrew Poelstra)
a6ca4ce policy: clean up error types in compile_tr (Andrew Poelstra)
bedb6d7 policy: unwrap a few impossible error cases in compile_tr (Andrew Poelstra)
a3338fb policy: use new tapleaf_probability_iter for num_tap_leaves (Andrew Poelstra)
3522703 policy: reduce error paths in Concrete::extract_key to one (Andrew Poelstra)
9f53c01 compiler: split segwit_limits test into 2 (Andrew Poelstra)
7de96d3 policy: split up error enum for concrete and semantic (Andrew Poelstra)

Pull request description:

  Right now our Taproot compiler does some complicated analysis to identify a key within a policy that may be extracted to use as an internal key. Specifically, if you look at the "disjunction tree" at the root of a policy, every leaf of this tree can be a tapbranch and any leaf which is just a pubkey-check and nothing else is a candidate to be the internal key.

  However, our logic to do this is convoluted, has many unnecessary error paths, and lots of gratuitous allocations (which ironically make the code harder, not easier, to follow).

  This PR smooths all that out by introducing a new iterator over the potential tapbranches of a policy (along with their probabilities). It also cleans up much of the error handling in the policy module, eliminating 3 calls to the stringly-typed `errstr` function.

ACKs for top commit:
  sanket1729:
    ACK a195c81

Tree-SHA512: 62278a0d778c4e68faf69f757d2ad2fedc43e6b213df7cf9fef6d5a898c6db9b18d5c9a753d974bad26cda693c5eb170bf5e5b28d14c7196d12d64fb6f49e712

Author: apoelstra

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

@@ -19,6 +19,7 @@ jobs:
         # over that limit with fuzzing because of the hour run time.
         fuzz_target: [
 compile_descriptor,
+compile_taproot,
 parse_descriptor,
 parse_descriptor_secret,
 roundtrip_concrete,

fuzz/Cargo.toml

@@ -19,6 +19,10 @@ regex = "1.0"
 name = "compile_descriptor"
 path = "fuzz_targets/compile_descriptor.rs"
 
+[[bin]]
+name = "compile_taproot"
+path = "fuzz_targets/compile_taproot.rs"
+
 [[bin]]
 name = "parse_descriptor"
 path = "fuzz_targets/parse_descriptor.rs"

fuzz/fuzz_targets/compile_taproot.rs

@@ -0,0 +1,37 @@
+#![allow(unexpected_cfgs)]
+
+use std::str::FromStr;
+
+use honggfuzz::fuzz;
+use miniscript::{policy, Miniscript, Tap};
+use policy::Liftable;
+
+type Script = Miniscript<String, Tap>;
+type Policy = policy::Concrete<String>;
+
+fn do_test(data: &[u8]) {
+    let data_str = String::from_utf8_lossy(data);
+    if let Ok(pol) = Policy::from_str(&data_str) {
+        // Compile
+        if let Ok(desc) = pol.compile::<Tap>() {
+            // Lift
+            assert_eq!(desc.lift().unwrap().sorted(), pol.lift().unwrap().sorted());
+            // Try to roundtrip the output of the compiler
+            let output = desc.to_string();
+            if let Ok(desc) = Script::from_str(&output) {
+                let rtt = desc.to_string();
+                assert_eq!(output.to_lowercase(), rtt.to_lowercase());
+            } else {
+                panic!("compiler output something unparseable: {}", output)
+            }
+        }
+    }
+}
+
+fn main() {
+    loop {
+        fuzz!(|data| {
+            do_test(data);
+        });
+    }
+}

fuzz/generate-files.sh

@@ -22,7 +22,7 @@ publish = false
 cargo-fuzz = true
 
 [dependencies]
-honggfuzz = { version = "0.5.55", default-features = false }
+honggfuzz = { version = "0.5.56", default-features = false }
 miniscript = { path = "..", features = [ "compiler" ] }
 
 regex = "1.0"

src/lib.rs

@@ -464,7 +464,9 @@ pub enum Error {
     /// Compiler related errors
     CompilerError(crate::policy::compiler::CompilerError),
     /// Errors related to policy
-    PolicyError(policy::concrete::PolicyError),
+    SemanticPolicy(policy::semantic::PolicyError),
+    /// Errors related to policy
+    ConcretePolicy(policy::concrete::PolicyError),
     /// Errors related to lifting
     LiftError(policy::LiftError),
     /// Forward script context related errors
@@ -529,7 +531,8 @@ impl fmt::Display for Error {
             Error::ContextError(ref e) => fmt::Display::fmt(e, f),
             #[cfg(feature = "compiler")]
             Error::CompilerError(ref e) => fmt::Display::fmt(e, f),
-            Error::PolicyError(ref e) => fmt::Display::fmt(e, f),
+            Error::SemanticPolicy(ref e) => fmt::Display::fmt(e, f),
+            Error::ConcretePolicy(ref e) => fmt::Display::fmt(e, f),
             Error::LiftError(ref e) => fmt::Display::fmt(e, f),
             Error::MaxRecursiveDepthExceeded => write!(
                 f,
@@ -595,7 +598,8 @@ impl error::Error for Error {
             Secp(e) => Some(e),
             #[cfg(feature = "compiler")]
             CompilerError(e) => Some(e),
-            PolicyError(e) => Some(e),
+            ConcretePolicy(e) => Some(e),
+            SemanticPolicy(e) => Some(e),
             LiftError(e) => Some(e),
             ContextError(e) => Some(e),
             AnalysisError(e) => Some(e),
@@ -649,11 +653,6 @@ impl From<crate::policy::compiler::CompilerError> for Error {
     fn from(e: crate::policy::compiler::CompilerError) -> Error { Error::CompilerError(e) }
 }
 
-#[doc(hidden)]
-impl From<policy::concrete::PolicyError> for Error {
-    fn from(e: policy::concrete::PolicyError) -> Error { Error::PolicyError(e) }
-}
-
 fn errstr(s: &str) -> Error { Error::Unexpected(s.to_owned()) }
 
 /// The size of an encoding of a number in Script

src/policy/compiler.rs

@@ -50,6 +50,16 @@ pub enum CompilerError {
     /// There may exist other miniscripts which are under these limits but the
     /// compiler currently does not find them.
     LimitsExceeded,
+    /// In a Taproot compilation, no "unspendable key" was provided and no in-policy
+    /// key could be used as an internal key.
+    NoInternalKey,
+    /// When compiling to Taproot, policy had too many Tapleaves
+    TooManyTapleaves {
+        /// Number of Tapleaves inferred from the policy.
+        n: usize,
+        /// Maximum allowed number of Tapleaves.
+        max: usize,
+    },
     ///Policy related errors
     PolicyError(policy::concrete::PolicyError),
 }
@@ -66,6 +76,12 @@ impl fmt::Display for CompilerError {
             CompilerError::LimitsExceeded => f.write_str(
                 "At least one spending path has exceeded the standardness or consensus limits",
             ),
+            CompilerError::NoInternalKey => {
+                f.write_str("Taproot compilation had no internal key available")
+            }
+            CompilerError::TooManyTapleaves { n, max } => {
+                write!(f, "Policy had too many Tapleaves (found {}, maximum {})", n, max)
+            }
             CompilerError::PolicyError(ref e) => fmt::Display::fmt(e, f),
         }
     }
@@ -77,7 +93,11 @@ impl error::Error for CompilerError {
         use self::CompilerError::*;
 
         match self {
-            TopLevelNonSafe | ImpossibleNonMalleableCompilation | LimitsExceeded => None,
+            TopLevelNonSafe
+            | ImpossibleNonMalleableCompilation
+            | LimitsExceeded
+            | NoInternalKey
+            | TooManyTapleaves { .. } => None,
             PolicyError(e) => Some(e),
         }
     }
@@ -1511,7 +1531,7 @@ mod tests {
     }
 
     #[test]
-    fn segwit_limits() {
+    fn segwit_limits_1() {
         // Hit the maximum witness script size limit.
         // or(thresh(52, [pubkey; 52]), thresh(52, [pubkey; 52])) results in a 3642-bytes long
         // witness script with only 54 stack elements
@@ -1537,7 +1557,10 @@ mod tests {
             "Compilation succeeded with a witscript size of '{:?}'",
             script_size,
         );
+    }
 
+    #[test]
+    fn segwit_limits_2() {
         // Hit the maximum witness stack elements limit
         let (keys, _) = pubkeys_and_a_sig(100);
         let keys: Vec<Arc<Concrete<bitcoin::PublicKey>>> = keys