From f6885e358bfe78a790226e6246dad4922cf82d02 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 23 Mar 2023 19:07:43 +0100 Subject: [PATCH] descriptors: cleanup error types This removes circular dependencies and apply the appropriate variants to the appropriate enums. --- src/descriptors/analysis.rs | 102 +++++++++++++++++++++++------------- src/descriptors/keys.rs | 4 -- src/descriptors/mod.rs | 26 +++------ 3 files changed, 75 insertions(+), 57 deletions(-) diff --git a/src/descriptors/analysis.rs b/src/descriptors/analysis.rs index 9d1b3bbe..07e34a1c 100644 --- a/src/descriptors/analysis.rs +++ b/src/descriptors/analysis.rs @@ -8,10 +8,46 @@ use miniscript::{ use std::{ collections::{HashMap, HashSet}, convert::TryFrom, - sync, + error, fmt, sync, }; -use crate::descriptors::{keys::DescKeyError, LianaDescError}; +#[derive(Debug)] +pub enum LianaPolicyError { + InsaneTimelock(u32), + InvalidKey(Box), + DuplicateKey(Box), + InvalidMultiThresh(usize), + InvalidMultiKeys(usize), + IncompatibleDesc, +} + +impl std::fmt::Display for LianaPolicyError { + fn fmt(&self, f: &mut fmt::Formatter) -> std::fmt::Result { + match self { + Self::InsaneTimelock(tl) => { + write!(f, "Timelock value '{}' isn't valid or safe to use", tl) + } + Self::InvalidKey(key) => { + write!( + f, + "Invalid key '{}'. Need a wildcard ('ranged') xpub with an origin and a multipath for (and only for) deriving change addresses. That is, an xpub of the form '[aaff0099]xpub.../<0;1>/*'.", + key + ) + } + Self::InvalidMultiThresh(thresh) => write!(f, "Invalid multisig threshold value '{}'. The threshold must be > to 0 and <= to the number of keys.", thresh), + Self::InvalidMultiKeys(n_keys) => write!(f, "Invalid number of keys '{}'. Between 2 and 20 keys must be given to use multiple keys in a specific path.", n_keys), + Self::DuplicateKey(key) => { + write!(f, "Duplicate key '{}'.", key) + } + Self::IncompatibleDesc => write!( + f, + "Descriptor is not compatible with a Liana spending policy." + ), + } + } +} + +impl error::Error for LianaPolicyError {} // Whether a Miniscript policy node represents a key check (or several of them). fn is_single_key_or_multisig(policy: &SemanticPolicy) -> bool { @@ -58,11 +94,11 @@ fn is_valid_desc_key(key: &descriptor::DescriptorPublicKey) -> bool { // // All this is achieved simply through asking for a 16-bit integer, since all the // above are signaled in leftmost bits. -fn csv_check(csv_value: u32) -> Result { +fn csv_check(csv_value: u32) -> Result { if csv_value > 0 { - u16::try_from(csv_value).map_err(|_| LianaDescError::InsaneTimelock(csv_value)) + u16::try_from(csv_value).map_err(|_| LianaPolicyError::InsaneTimelock(csv_value)) } else { - Err(LianaDescError::InsaneTimelock(csv_value)) + Err(LianaPolicyError::InsaneTimelock(csv_value)) } } @@ -90,20 +126,20 @@ impl PathInfo { /// descriptor (that is, a set of keys). pub fn from_primary_path( policy: SemanticPolicy, - ) -> Result { + ) -> Result { match policy { SemanticPolicy::Key(key) => Ok(PathInfo::Single(key)), SemanticPolicy::Threshold(k, subs) => { - let keys: Result<_, LianaDescError> = subs + let keys: Result<_, LianaPolicyError> = subs .into_iter() .map(|sub| match sub { SemanticPolicy::Key(key) => Ok(key), - _ => Err(LianaDescError::IncompatibleDesc), + _ => Err(LianaPolicyError::IncompatibleDesc), }) .collect(); Ok(PathInfo::Multi(k, keys?)) } - _ => Err(LianaDescError::IncompatibleDesc), + _ => Err(LianaPolicyError::IncompatibleDesc), } } @@ -112,14 +148,14 @@ impl PathInfo { /// descriptor (that is, a set of keys after a timelock). pub fn from_recovery_path( policy: SemanticPolicy, - ) -> Result<(u16, PathInfo), LianaDescError> { + ) -> Result<(u16, PathInfo), LianaPolicyError> { // The recovery spending path must always be a policy of type `thresh(2, older(x), thresh(n, key1, // key2, ..))`. In the special case n == 1, it is only `thresh(2, older(x), key)`. In the // special case n == len(keys) (i.e. it's an N-of-N multisig), it is normalized as // `thresh(n+1, older(x), key1, key2, ...)`. let (k, subs) = match policy { SemanticPolicy::Threshold(k, subs) => (k, subs), - _ => return Err(LianaDescError::IncompatibleDesc), + _ => return Err(LianaPolicyError::IncompatibleDesc), }; if k == 2 && subs.len() == 2 { // The general case (as well as the n == 1 case). The sub that is not the timelock is @@ -130,11 +166,11 @@ impl PathInfo { SemanticPolicy::Older(val) => Some(csv_check(val.0)), _ => None, }) - .ok_or(LianaDescError::IncompatibleDesc)??; + .ok_or(LianaPolicyError::IncompatibleDesc)??; let keys_sub = subs .into_iter() .find(is_single_key_or_multisig) - .ok_or(LianaDescError::IncompatibleDesc)?; + .ok_or(LianaPolicyError::IncompatibleDesc)?; PathInfo::from_primary_path(keys_sub).map(|info| (tl_value, info)) } else if k == subs.len() && subs.len() > 2 { // The N-of-N case. All subs but the threshold must be keys (if one had been thresh() @@ -146,22 +182,22 @@ impl PathInfo { SemanticPolicy::Key(key) => keys.push(key), SemanticPolicy::Older(val) => { if tl_value.is_some() { - return Err(LianaDescError::IncompatibleDesc); + return Err(LianaPolicyError::IncompatibleDesc); } tl_value = Some(csv_check(val.0)?); } - _ => return Err(LianaDescError::IncompatibleDesc), + _ => return Err(LianaPolicyError::IncompatibleDesc), } } assert!(keys.len() > 1); // At least 3 subs, only one of which may be older(). Ok(( - tl_value.ok_or(LianaDescError::IncompatibleDesc)?, + tl_value.ok_or(LianaPolicyError::IncompatibleDesc)?, PathInfo::Multi(k - 1, keys), )) } else { // If there is less than 2 subs, there can't be both a timelock and keys. If the // threshold is not equal to the number of subs, the timelock can't be mandatory. - Err(LianaDescError::IncompatibleDesc) + Err(LianaPolicyError::IncompatibleDesc) } } @@ -287,7 +323,7 @@ impl LianaPolicy { primary_path: PathInfo, recovery_path: PathInfo, recovery_timelock: u16, - ) -> Result { + ) -> Result { // We require the locktime to: // - not be disabled // - be in number of blocks @@ -296,21 +332,17 @@ impl LianaPolicy { // // All this is achieved through asking for a 16-bit integer. if recovery_timelock == 0 { - return Err(LianaDescError::InsaneTimelock(recovery_timelock as u32)); + return Err(LianaPolicyError::InsaneTimelock(recovery_timelock as u32)); } // If any of the paths is a multisig, make sure they are within the CHECKMULTISIG bounds. for path_info in &[&primary_path, &recovery_path] { if let PathInfo::Multi(thresh, keys) = path_info { if keys.len() < 2 || keys.len() > 20 { - return Err(LianaDescError::DescKey(DescKeyError::InvalidMultiKeys( - keys.len(), - ))); + return Err(LianaPolicyError::InvalidMultiKeys(keys.len())); } if thresh == &0 || thresh > &keys.len() { - return Err(LianaDescError::DescKey(DescKeyError::InvalidMultiThresh( - *thresh, - ))); + return Err(LianaPolicyError::InvalidMultiThresh(*thresh)); } } } @@ -319,7 +351,7 @@ impl LianaPolicy { let (prim_keys, rec_keys) = (primary_path.keys(), recovery_path.keys()); let all_keys = prim_keys.iter().chain(rec_keys.iter()); if let Some(key) = all_keys.clone().find(|k| !is_valid_desc_key(k)) { - return Err(LianaDescError::InvalidKey((*key).clone().into())); + return Err(LianaPolicyError::InvalidKey((*key).clone().into())); } // Check for key duplicates. They are invalid in (nonmalleable) miniscripts. @@ -330,7 +362,7 @@ impl LianaPolicy { _ => unreachable!("Just checked it was a multixpub above"), }; if key_set.contains(&xpub) { - return Err(LianaDescError::DuplicateKey(key.clone().into())); + return Err(LianaPolicyError::DuplicateKey(key.clone().into())); } key_set.insert(xpub); } @@ -346,18 +378,18 @@ impl LianaPolicy { /// (P2WSH, multipath, ..) and has a valid Liana semantic. pub fn from_multipath_descriptor( desc: &descriptor::Descriptor, - ) -> Result { + ) -> Result { // For now we only allow P2WSH descriptors. let wsh_desc = match &desc { descriptor::Descriptor::Wsh(desc) => desc, - _ => return Err(LianaDescError::IncompatibleDesc), + _ => return Err(LianaPolicyError::IncompatibleDesc), }; // Get the Miniscript from the descriptor and make sure it only contains valid multipath // descriptor keys. let ms = match wsh_desc.as_inner() { descriptor::WshInner::Ms(ms) => ms, - _ => return Err(LianaDescError::IncompatibleDesc), + _ => return Err(LianaPolicyError::IncompatibleDesc), }; let invalid_key = ms.iter_pk().find_map(|pk| { if is_valid_desc_key(&pk) { @@ -367,7 +399,7 @@ impl LianaPolicy { } }); if let Some(key) = invalid_key { - return Err(LianaDescError::InvalidKey(key.into())); + return Err(LianaPolicyError::InvalidKey(key.into())); } // Now lift a semantic policy out of this Miniscript and normalize it to make sure we @@ -382,9 +414,9 @@ impl LianaPolicy { SemanticPolicy::Threshold(1, subs) => Some(subs), _ => None, } - .ok_or(LianaDescError::IncompatibleDesc)?; + .ok_or(LianaPolicyError::IncompatibleDesc)?; if subs.len() != 2 { - return Err(LianaDescError::IncompatibleDesc); + return Err(LianaPolicyError::IncompatibleDesc); } // Fetch the two spending paths' semantic policies. The primary path is identified as the @@ -400,8 +432,8 @@ impl LianaPolicy { (prim_sub, reco_sub) }); let (prim_path_sub, reco_path_sub) = ( - prim_path_sub.ok_or(LianaDescError::IncompatibleDesc)?, - reco_path_sub.ok_or(LianaDescError::IncompatibleDesc)?, + prim_path_sub.ok_or(LianaPolicyError::IncompatibleDesc)?, + reco_path_sub.ok_or(LianaPolicyError::IncompatibleDesc)?, ); // Now parse information about each spending path. diff --git a/src/descriptors/keys.rs b/src/descriptors/keys.rs index 422ef59b..29e1d13e 100644 --- a/src/descriptors/keys.rs +++ b/src/descriptors/keys.rs @@ -12,16 +12,12 @@ use std::{error, fmt, str}; #[derive(Debug)] pub enum DescKeyError { DerivedKeyParsing, - InvalidMultiThresh(usize), - InvalidMultiKeys(usize), } impl std::fmt::Display for DescKeyError { fn fmt(&self, f: &mut fmt::Formatter) -> std::fmt::Result { match self { DescKeyError::DerivedKeyParsing => write!(f, "Parsing derived key"), - Self::InvalidMultiThresh(thresh) => write!(f, "Invalid threshold value '{}'. The threshold must be > to 0 and <= to the number of keys.", thresh), - Self::InvalidMultiKeys(n_keys) => write!(f, "Invalid number of keys '{}'. Between 2 and 20 keys must be given to use multiple keys in a specific path.", n_keys), } } } diff --git a/src/descriptors/mod.rs b/src/descriptors/mod.rs index 142787e5..e9ecee40 100644 --- a/src/descriptors/mod.rs +++ b/src/descriptors/mod.rs @@ -30,12 +30,9 @@ fn wu_to_vb(vb: usize) -> usize { #[derive(Debug)] pub enum LianaDescError { - InsaneTimelock(u32), - InvalidKey(Box), - DuplicateKey(Box), Miniscript(miniscript::Error), - IncompatibleDesc, DescKey(DescKeyError), + Policy(LianaPolicyError), /// Different number of PSBT vs tx inputs, etc.. InsanePsbt, /// Not all inputs' sequence the same, not all inputs signed with the same key, .. @@ -45,22 +42,9 @@ pub enum LianaDescError { impl std::fmt::Display for LianaDescError { fn fmt(&self, f: &mut fmt::Formatter) -> std::fmt::Result { match self { - Self::InsaneTimelock(tl) => { - write!(f, "Timelock value '{}' isn't valid or safe to use", tl) - } - Self::InvalidKey(key) => { - write!( - f, - "Invalid key '{}'. Need a wildcard ('ranged') xpub with an origin and a multipath for (and only for) deriving change addresses. That is, an xpub of the form '[aaff0099]xpub.../<0;1>/*'.", - key - ) - } - Self::DuplicateKey(key) => { - write!(f, "Duplicate key '{}'.", key) - } Self::Miniscript(e) => write!(f, "Miniscript error: '{}'.", e), - Self::IncompatibleDesc => write!(f, "Descriptor is not compatible."), Self::DescKey(e) => write!(f, "{}", e), + Self::Policy(e) => write!(f, "{}", e), Self::InsanePsbt => write!(f, "Analyzed PSBT is empty or malformed."), Self::InconsistentPsbt => write!(f, "Analyzed PSBT is inconsistent across inputs."), } @@ -69,6 +53,12 @@ impl std::fmt::Display for LianaDescError { impl error::Error for LianaDescError {} +impl From for LianaDescError { + fn from(e: LianaPolicyError) -> LianaDescError { + LianaDescError::Policy(e) + } +} + /// An [InheritanceDescriptor] that contains multipath keys for (and only for) the receive keychain /// and the change keychain. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]