From 7de96d3507f3d725eff277c30e36360378c98444 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Tue, 20 Aug 2024 20:54:34 +0000 Subject: [PATCH 1/7] policy: split up error enum for concrete and semantic --- src/lib.rs | 15 +++++++------- src/policy/concrete.rs | 36 +++++++-------------------------- src/policy/mod.rs | 2 +- src/policy/semantic.rs | 45 +++++++++++++++++++++++++++++++++++++++--- 4 files changed, 57 insertions(+), 41 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 3abb5c7db..b463807ce 100644 --- a/src/lib.rs +++ b/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 for Error { fn from(e: crate::policy::compiler::CompilerError) -> Error { Error::CompilerError(e) } } -#[doc(hidden)] -impl From 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 diff --git a/src/policy/concrete.rs b/src/policy/concrete.rs index fbefa510f..6f1d36cb1 100644 --- a/src/policy/concrete.rs +++ b/src/policy/concrete.rs @@ -21,7 +21,6 @@ use { core::cmp::Reverse, }; -use super::ENTAILMENT_MAX_TERMINALS; use crate::expression::{self, FromTree}; use crate::iter::{Tree, TreeLike}; use crate::miniscript::types::extra_props::TimelockInfo; @@ -80,12 +79,6 @@ pub enum PolicyError { NonBinaryArgAnd, /// `Or` fragments only support two args. NonBinaryArgOr, - /// Semantic Policy Error: `And` `Or` fragments must take args: `k > 1`. - InsufficientArgsforAnd, - /// Semantic policy error: `And` `Or` fragments must take args: `k > 1`. - InsufficientArgsforOr, - /// Entailment max terminals exceeded. - EntailmentMaxTerminals, /// Cannot lift policies that have a combination of height and timelocks. HeightTimelockCombination, /// Duplicate Public Keys. @@ -114,15 +107,6 @@ impl fmt::Display for PolicyError { f.write_str("And policy fragment must take 2 arguments") } PolicyError::NonBinaryArgOr => f.write_str("Or policy fragment must take 2 arguments"), - PolicyError::InsufficientArgsforAnd => { - f.write_str("Semantic Policy 'And' fragment must have at least 2 args ") - } - PolicyError::InsufficientArgsforOr => { - f.write_str("Semantic Policy 'Or' fragment must have at least 2 args ") - } - PolicyError::EntailmentMaxTerminals => { - write!(f, "Policy entailment only supports {} terminals", ENTAILMENT_MAX_TERMINALS) - } PolicyError::HeightTimelockCombination => { f.write_str("Cannot lift policies that have a heightlock and timelock combination") } @@ -137,13 +121,7 @@ impl error::Error for PolicyError { use self::PolicyError::*; match self { - NonBinaryArgAnd - | NonBinaryArgOr - | InsufficientArgsforAnd - | InsufficientArgsforOr - | EntailmentMaxTerminals - | HeightTimelockCombination - | DuplicatePubKeys => None, + NonBinaryArgAnd | NonBinaryArgOr | HeightTimelockCombination | DuplicatePubKeys => None, } } } @@ -252,7 +230,7 @@ impl Policy { // TODO: We might require other compile errors for Taproot. #[cfg(feature = "compiler")] pub fn compile_tr(&self, unspendable_key: Option) -> Result, Error> { - self.is_valid()?; // Check for validity + self.is_valid().map_err(Error::ConcretePolicy)?; match self.is_safe_nonmalleable() { (false, _) => Err(Error::from(CompilerError::TopLevelNonSafe)), (_, false) => Err(Error::from(CompilerError::ImpossibleNonMalleableCompilation)), @@ -308,7 +286,7 @@ impl Policy { &self, unspendable_key: Option, ) -> Result, Error> { - self.is_valid()?; // Check for validity + self.is_valid().map_err(Error::ConcretePolicy)?; match self.is_safe_nonmalleable() { (false, _) => Err(Error::from(CompilerError::TopLevelNonSafe)), (_, false) => Err(Error::from(CompilerError::ImpossibleNonMalleableCompilation)), @@ -355,7 +333,7 @@ impl Policy { &self, desc_ctx: DescriptorCtx, ) -> Result, Error> { - self.is_valid()?; + self.is_valid().map_err(Error::ConcretePolicy)?; match self.is_safe_nonmalleable() { (false, _) => Err(Error::from(CompilerError::TopLevelNonSafe)), (_, false) => Err(Error::from(CompilerError::ImpossibleNonMalleableCompilation)), @@ -869,7 +847,7 @@ impl str::FromStr for Policy { let tree = expression::Tree::from_str(s)?; let policy: Policy = FromTree::from_tree(&tree)?; - policy.check_timelocks()?; + policy.check_timelocks().map_err(Error::ConcretePolicy)?; Ok(policy) } } @@ -934,7 +912,7 @@ impl Policy { }), ("and", _) => { if top.args.len() != 2 { - return Err(Error::PolicyError(PolicyError::NonBinaryArgAnd)); + return Err(Error::ConcretePolicy(PolicyError::NonBinaryArgAnd)); } let mut subs = Vec::with_capacity(top.args.len()); for arg in &top.args { @@ -944,7 +922,7 @@ impl Policy { } ("or", _) => { if top.args.len() != 2 { - return Err(Error::PolicyError(PolicyError::NonBinaryArgOr)); + return Err(Error::ConcretePolicy(PolicyError::NonBinaryArgOr)); } let mut subs = Vec::with_capacity(top.args.len()); for arg in &top.args { diff --git a/src/policy/mod.rs b/src/policy/mod.rs index 8f7c1cc30..de9577e68 100644 --- a/src/policy/mod.rs +++ b/src/policy/mod.rs @@ -194,7 +194,7 @@ impl Liftable for Concrete { fn lift(&self) -> Result, Error> { // do not lift if there is a possible satisfaction // involving combination of timelocks and heightlocks - self.check_timelocks()?; + self.check_timelocks().map_err(Error::ConcretePolicy)?; let ret = match *self { Concrete::Unsatisfiable => Semantic::Unsatisfiable, Concrete::Trivial => Semantic::Trivial, diff --git a/src/policy/semantic.rs b/src/policy/semantic.rs index 794ab286b..7d2959764 100644 --- a/src/policy/semantic.rs +++ b/src/policy/semantic.rs @@ -7,10 +7,11 @@ use core::str::FromStr; use core::{fmt, str}; +#[cfg(feature = "std")] +use std::error; use bitcoin::{absolute, relative}; -use super::concrete::PolicyError; use super::ENTAILMENT_MAX_TERMINALS; use crate::iter::{Tree, TreeLike}; use crate::prelude::*; @@ -50,6 +51,44 @@ pub enum Policy { Thresh(Threshold>, 0>), } +/// Detailed error type for concrete policies. +#[derive(Copy, Clone, PartialEq, Eq, Debug, Hash)] +pub enum PolicyError { + /// Semantic Policy Error: `And` `Or` fragments must take args: `k > 1`. + InsufficientArgsforAnd, + /// Semantic policy error: `And` `Or` fragments must take args: `k > 1`. + InsufficientArgsforOr, + /// Entailment max terminals exceeded. + EntailmentMaxTerminals, +} + +impl fmt::Display for PolicyError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match *self { + PolicyError::InsufficientArgsforAnd => { + f.write_str("Semantic Policy 'And' fragment must have at least 2 args ") + } + PolicyError::InsufficientArgsforOr => { + f.write_str("Semantic Policy 'Or' fragment must have at least 2 args ") + } + PolicyError::EntailmentMaxTerminals => { + write!(f, "Policy entailment only supports {} terminals", ENTAILMENT_MAX_TERMINALS) + } + } + } +} + +#[cfg(feature = "std")] +impl error::Error for PolicyError { + fn cause(&self) -> Option<&dyn error::Error> { + match self { + PolicyError::InsufficientArgsforAnd + | PolicyError::InsufficientArgsforOr + | PolicyError::EntailmentMaxTerminals => None, + } + } +} + impl ForEachKey for Policy { fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, mut pred: F) -> bool { self.pre_order_iter().all(|policy| match policy { @@ -312,7 +351,7 @@ impl expression::FromTree for Policy { }), ("and", nsubs) => { if nsubs < 2 { - return Err(Error::PolicyError(PolicyError::InsufficientArgsforAnd)); + return Err(Error::SemanticPolicy(PolicyError::InsufficientArgsforAnd)); } let mut subs = Vec::with_capacity(nsubs); for arg in &top.args { @@ -322,7 +361,7 @@ impl expression::FromTree for Policy { } ("or", nsubs) => { if nsubs < 2 { - return Err(Error::PolicyError(PolicyError::InsufficientArgsforOr)); + return Err(Error::SemanticPolicy(PolicyError::InsufficientArgsforOr)); } let mut subs = Vec::with_capacity(nsubs); for arg in &top.args { From 9f53c01c7c2d7c154f3b3a1289e628d40f64ca05 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Tue, 20 Aug 2024 21:25:23 +0000 Subject: [PATCH 2/7] compiler: split segwit_limits test into 2 This test takes ~28 seconds on my system, but it is doing two independent compilations in serial. If I split it into two, then they can run in parallel and the total time is about 16 seconds. --- src/policy/compiler.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/policy/compiler.rs b/src/policy/compiler.rs index d306da905..c21c981aa 100644 --- a/src/policy/compiler.rs +++ b/src/policy/compiler.rs @@ -1511,7 +1511,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 +1537,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>> = keys From 3522703accce948357400108cb8e45a75977e611 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Tue, 20 Aug 2024 21:23:02 +0000 Subject: [PATCH 3/7] policy: reduce error paths in Concrete::extract_key to one The Taproot compiler has a couple algorithms used to find an optimal internal key to extract from a policy. One is `to_tapleaf_prob_vec` which is a fundamental part of the compiler, which iterates over the disjunction-only root of the tree and returns every branch along with its probabilities. This is currently implemented using both allocations and recursion. By pulling the logic out into an iterator we can get a clearer, faster algorithm that cannot stack-overflow. This algorithm is then fed into Concrete::extract_key. The goal of this algorithm is to find the highest-probability tapbranch consisting of just a single key. This algorithm inexplicably works by: * Lifting the policy to a semantic policy, maybe returning an error * Iterating over the entire policy, extracting a list of every key. * Calling to_tapleaf_prob_vec to get a vector of tapleaves, which it then iterates over to find the key-only branches, which it collects into a BTreeMap mapping the key to probabilities. * Iterating over the extracted lists of keys, and for each key, reducing the semantic policy by that key and comparing to Trivial (logically, this is asking "is this key in a tree of disjunctions from the root"). * For each such key that it finds, looking it up in the map, potentially returning an error (actually this error path is impossible to hit). * and manually minimizing the looked up probability. With to_tapleaf_prob_vec replaced by an iterator there is a simpler and more direct algorithm: * Iterate through all the tapbranches/probability pairs, filtering for key-only branches, and maximizing by probability. This can only fail if there are no key-only branches, and this is reflected by only having one error branch. --- src/policy/concrete.rs | 109 +++++++++++++++++++---------------------- 1 file changed, 51 insertions(+), 58 deletions(-) diff --git a/src/policy/concrete.rs b/src/policy/concrete.rs index 6f1d36cb1..3251bacb2 100644 --- a/src/policy/concrete.rs +++ b/src/policy/concrete.rs @@ -12,9 +12,7 @@ use bitcoin::absolute; use { crate::descriptor::TapTree, crate::miniscript::ScriptContext, - crate::policy::compiler::CompilerError, - crate::policy::compiler::OrdF64, - crate::policy::{compiler, Concrete, Liftable, Semantic}, + crate::policy::compiler::{self, CompilerError, OrdF64}, crate::Descriptor, crate::Miniscript, crate::Tap, @@ -126,8 +124,41 @@ impl error::Error for PolicyError { } } +#[cfg(feature = "compiler")] +struct TapleafProbabilityIter<'p, Pk: MiniscriptKey> { + stack: Vec<(f64, &'p Policy)>, +} + +#[cfg(feature = "compiler")] +impl<'p, Pk: MiniscriptKey> Iterator for TapleafProbabilityIter<'p, Pk> { + type Item = (f64, &'p Policy); + + fn next(&mut self) -> Option { + loop { + let (top_prob, top) = self.stack.pop()?; + + match top { + Policy::Or(ref subs) => { + let total_sub_prob = subs.iter().map(|prob_sub| prob_sub.0).sum::(); + for (sub_prob, sub) in subs.iter().rev() { + let ratio = *sub_prob as f64 / total_sub_prob as f64; + self.stack.push((top_prob * ratio, sub)); + } + } + Policy::Thresh(ref thresh) if thresh.is_or() => { + let n64 = thresh.n() as f64; + for sub in thresh.iter().rev() { + self.stack.push((top_prob / n64, sub)); + } + } + _ => return Some((top_prob, top)), + } + } + } +} + impl Policy { - /// Flattens the [`Policy`] tree structure into a vector of tuples `(leaf script, leaf probability)` + /// Flattens the [`Policy`] tree structure into an iterator of tuples `(leaf script, leaf probability)` /// with leaf probabilities corresponding to odds for each sub-branch in the policy. /// We calculate the probability of selecting the sub-branch at every level and calculate the /// leaf probabilities as the probability of traversing through required branches to reach the @@ -150,60 +181,22 @@ impl Policy { /// Since this splitting might lead to exponential blow-up, we constrain the number of /// leaf-nodes to [`MAX_COMPILATION_LEAVES`]. #[cfg(feature = "compiler")] - fn to_tapleaf_prob_vec(&self, prob: f64) -> Vec<(f64, Policy)> { - match self { - Policy::Or(ref subs) => { - let total_odds: usize = subs.iter().map(|(ref k, _)| k).sum(); - subs.iter() - .flat_map(|(k, ref policy)| { - policy.to_tapleaf_prob_vec(prob * *k as f64 / total_odds as f64) - }) - .collect::>() - } - Policy::Thresh(ref thresh) if thresh.is_or() => { - let total_odds = thresh.n(); - thresh - .iter() - .flat_map(|policy| policy.to_tapleaf_prob_vec(prob / total_odds as f64)) - .collect::>() - } - x => vec![(prob, x.clone())], - } + fn tapleaf_probability_iter(&self) -> TapleafProbabilityIter { + TapleafProbabilityIter { stack: vec![(1.0, self)] } } /// Extracts the internal_key from this policy tree. #[cfg(feature = "compiler")] fn extract_key(self, unspendable_key: Option) -> Result<(Pk, Policy), Error> { - let mut internal_key: Option = None; - { - let mut prob = 0.; - let semantic_policy = self.lift()?; - let concrete_keys = self.keys(); - let key_prob_map: BTreeMap<_, _> = self - .to_tapleaf_prob_vec(1.0) - .into_iter() - .filter(|(_, ref pol)| matches!(pol, Concrete::Key(..))) - .map(|(prob, key)| (key, prob)) - .collect(); - - for key in concrete_keys.into_iter() { - if semantic_policy - .clone() - .satisfy_constraint(&Semantic::Key(key.clone()), true) - == Semantic::Trivial - { - match key_prob_map.get(&Concrete::Key(key.clone())) { - Some(val) => { - if *val > prob { - prob = *val; - internal_key = Some(key.clone()); - } - } - None => return Err(errstr("Key should have existed in the BTreeMap!")), - } - } - } - } + let internal_key = self + .tapleaf_probability_iter() + .filter_map(|(prob, ref pol)| match pol { + Policy::Key(pk) => Some((OrdF64(prob), pk)), + _ => None, + }) + .max_by_key(|(prob, _)| *prob) + .map(|(_, pk)| pk.clone()); + match (internal_key, unspendable_key) { (Some(ref key), _) => Ok((key.clone(), self.translate_unsatisfiable_pk(key))), (_, Some(key)) => Ok((key, self)), @@ -242,14 +235,13 @@ impl Policy { match policy { Policy::Trivial => None, policy => { - let vec_policies: Vec<_> = policy.to_tapleaf_prob_vec(1.0); let mut leaf_compilations: Vec<(OrdF64, Miniscript)> = vec![]; - for (prob, pol) in vec_policies { + for (prob, pol) in policy.tapleaf_probability_iter() { // policy corresponding to the key (replaced by unsatisfiable) is skipped - if pol == Policy::Unsatisfiable { + if *pol == Policy::Unsatisfiable { continue; } - let compilation = compiler::best_compilation::(&pol)?; + let compilation = compiler::best_compilation::(pol)?; compilation.sanity_check()?; leaf_compilations.push((OrdF64(prob), compilation)); } @@ -371,7 +363,7 @@ impl Policy { /// /// This function is supposed to incrementally expand i.e. represent the policy as /// disjunction over sub-policies output by it. The probability calculations are similar - /// to [`Policy::to_tapleaf_prob_vec`]. + /// to [`Policy::tapleaf_probability_iter`]. #[cfg(feature = "compiler")] fn enumerate_pol(&self, prob: f64) -> Vec<(f64, Arc)> { match self { @@ -1085,6 +1077,7 @@ mod compiler_tests { use core::str::FromStr; use super::*; + use crate::policy::Concrete; #[test] fn test_gen_comb() { From a3338fb8386e78893c659b049e9bf4bbc6bb3c44 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Tue, 20 Aug 2024 22:52:41 +0000 Subject: [PATCH 4/7] policy: use new tapleaf_probability_iter for num_tap_leaves --- src/policy/concrete.rs | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/src/policy/concrete.rs b/src/policy/concrete.rs index 3251bacb2..5549ae445 100644 --- a/src/policy/concrete.rs +++ b/src/policy/concrete.rs @@ -578,23 +578,7 @@ impl Policy { /// Gets the number of [TapLeaf](`TapTree::Leaf`)s considering exhaustive root-level [`Policy::Or`] /// and [`Policy::Thresh`] disjunctions for the `TapTree`. #[cfg(feature = "compiler")] - fn num_tap_leaves(&self) -> usize { - use Policy::*; - - let mut nums = vec![]; - for data in self.rtl_post_order_iter() { - let num = match data.node { - Or(subs) => (0..subs.len()).map(|_| nums.pop().unwrap()).sum(), - Thresh(thresh) if thresh.is_or() => { - (0..thresh.n()).map(|_| nums.pop().unwrap()).sum() - } - _ => 1, - }; - nums.push(num); - } - // Ok to unwrap because we know we processed at least one node. - nums.pop().unwrap() - } + fn num_tap_leaves(&self) -> usize { self.tapleaf_probability_iter().count() } /// Does checks on the number of `TapLeaf`s. #[cfg(feature = "compiler")] From bedb6d707f84973e7076a9a56cce4d20a5686688 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Tue, 20 Aug 2024 23:00:02 +0000 Subject: [PATCH 5/7] policy: unwrap a few impossible error cases in compile_tr We have a few error returns that are impossible to hit: * A sanity check on a tapleaf that just came out of the compiler (if this is hit it is a compiler bug and we want to know about it). * An error return from with_huffman_tree which can only happen if it's given an empty input (impossible) * An error if the final compilation (all tapleaves assembled into a tree) can't fit into a Descriptor::tr; but again, this is a compiler bug if we hit it. (Actually, I think that by manually constructing a policy that exceeds the maximum recursion depth you can trigger this error path, but the compiler output is not the place to flag this manual violation of invariants). The next commit will clean up the error types. These changes are in their own commit because they are potentially controversial. --- src/policy/concrete.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/policy/concrete.rs b/src/policy/concrete.rs index 5549ae445..3c8222c6a 100644 --- a/src/policy/concrete.rs +++ b/src/policy/concrete.rs @@ -242,14 +242,17 @@ impl Policy { continue; } let compilation = compiler::best_compilation::(pol)?; - compilation.sanity_check()?; + compilation + .sanity_check() + .expect("compiler produces sane output"); leaf_compilations.push((OrdF64(prob), compilation)); } - let tap_tree = with_huffman_tree::(leaf_compilations)?; + let tap_tree = with_huffman_tree::(leaf_compilations).unwrap(); Some(tap_tree) } }, - )?; + ) + .expect("compiler produces sane output"); Ok(tree) } } From a6ca4cea5c042188aea108af526a5f7a3a02606d Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Tue, 20 Aug 2024 22:36:37 +0000 Subject: [PATCH 6/7] policy: clean up error types in compile_tr This adds a couple variants to policy::compiler::Error and eliminates a couple calls to errstr. It also changes a few internal functions to return compiler errors instead of the top-level error. --- src/policy/compiler.rs | 22 +++++++++++++++++++++- src/policy/concrete.rs | 23 +++++++++++++---------- 2 files changed, 34 insertions(+), 11 deletions(-) diff --git a/src/policy/compiler.rs b/src/policy/compiler.rs index c21c981aa..63e16579e 100644 --- a/src/policy/compiler.rs +++ b/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), } } diff --git a/src/policy/concrete.rs b/src/policy/concrete.rs index 3c8222c6a..6382d837b 100644 --- a/src/policy/concrete.rs +++ b/src/policy/concrete.rs @@ -187,7 +187,7 @@ impl Policy { /// Extracts the internal_key from this policy tree. #[cfg(feature = "compiler")] - fn extract_key(self, unspendable_key: Option) -> Result<(Pk, Policy), Error> { + fn extract_key(self, unspendable_key: Option) -> Result<(Pk, Policy), CompilerError> { let internal_key = self .tapleaf_probability_iter() .filter_map(|(prob, ref pol)| match pol { @@ -200,7 +200,7 @@ impl Policy { match (internal_key, unspendable_key) { (Some(ref key), _) => Ok((key.clone(), self.translate_unsatisfiable_pk(key))), (_, Some(key)) => Ok((key, self)), - _ => Err(errstr("No viable internal key found.")), + _ => Err(CompilerError::NoInternalKey), } } @@ -222,11 +222,11 @@ impl Policy { /// is also *cost-efficient*. // TODO: We might require other compile errors for Taproot. #[cfg(feature = "compiler")] - pub fn compile_tr(&self, unspendable_key: Option) -> Result, Error> { - self.is_valid().map_err(Error::ConcretePolicy)?; + pub fn compile_tr(&self, unspendable_key: Option) -> Result, CompilerError> { + self.is_valid().map_err(CompilerError::PolicyError)?; match self.is_safe_nonmalleable() { - (false, _) => Err(Error::from(CompilerError::TopLevelNonSafe)), - (_, false) => Err(Error::from(CompilerError::ImpossibleNonMalleableCompilation)), + (false, _) => Err(CompilerError::TopLevelNonSafe), + (_, false) => Err(CompilerError::ImpossibleNonMalleableCompilation), _ => { let (internal_key, policy) = self.clone().extract_key(unspendable_key)?; policy.check_num_tapleaves()?; @@ -337,7 +337,9 @@ impl Policy { DescriptorCtx::Sh => Descriptor::new_sh(compiler::best_compilation(self)?), DescriptorCtx::Wsh => Descriptor::new_wsh(compiler::best_compilation(self)?), DescriptorCtx::ShWsh => Descriptor::new_sh_wsh(compiler::best_compilation(self)?), - DescriptorCtx::Tr(unspendable_key) => self.compile_tr(unspendable_key), + DescriptorCtx::Tr(unspendable_key) => self + .compile_tr(unspendable_key) + .map_err(Error::CompilerError), }, } } @@ -585,9 +587,10 @@ impl Policy { /// Does checks on the number of `TapLeaf`s. #[cfg(feature = "compiler")] - fn check_num_tapleaves(&self) -> Result<(), Error> { - if self.num_tap_leaves() > MAX_COMPILATION_LEAVES { - return Err(errstr("Too many Tapleaves")); + fn check_num_tapleaves(&self) -> Result<(), CompilerError> { + let n = self.num_tap_leaves(); + if n > MAX_COMPILATION_LEAVES { + return Err(CompilerError::TooManyTapleaves { n, max: MAX_COMPILATION_LEAVES }); } Ok(()) } From a195c8197c7029d13110cfb9c71df2fe2db5e079 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Tue, 20 Aug 2024 22:57:44 +0000 Subject: [PATCH 7/7] fuzz: add compile_taproot fuzztest --- .github/workflows/cron-daily-fuzz.yml | 1 + fuzz/Cargo.toml | 4 +++ fuzz/fuzz_targets/compile_taproot.rs | 37 +++++++++++++++++++++++++++ fuzz/generate-files.sh | 2 +- 4 files changed, 43 insertions(+), 1 deletion(-) create mode 100644 fuzz/fuzz_targets/compile_taproot.rs diff --git a/.github/workflows/cron-daily-fuzz.yml b/.github/workflows/cron-daily-fuzz.yml index 95582e797..c84eabfab 100644 --- a/.github/workflows/cron-daily-fuzz.yml +++ b/.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, diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index 353a9aa83..db5187266 100644 --- a/fuzz/Cargo.toml +++ b/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" diff --git a/fuzz/fuzz_targets/compile_taproot.rs b/fuzz/fuzz_targets/compile_taproot.rs new file mode 100644 index 000000000..4d43ffff2 --- /dev/null +++ b/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; +type Policy = policy::Concrete; + +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::() { + // 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); + }); + } +} diff --git a/fuzz/generate-files.sh b/fuzz/generate-files.sh index 8dabd7c9d..8a0c2ad20 100755 --- a/fuzz/generate-files.sh +++ b/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"