From 770bb79b577d627dd97547175299f0dc1ccbc843 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Wed, 13 Nov 2024 14:52:00 +0000 Subject: [PATCH 1/9] checksum: use array::from_fn Along the way, fix a bunch of other instances of "manual" slice construction that can be done with try_from or from_fn. Instances where the closure to `from_fn` would be several lines long, I left alone, but we could revisit those later. Fixes https://github.com/rust-bitcoin/rust-miniscript/issues/774 --- src/descriptor/checksum.rs | 10 +++++----- src/interpreter/inner.rs | 3 +-- src/interpreter/stack.rs | 19 ++++--------------- src/psbt/mod.rs | 18 ++++-------------- 4 files changed, 14 insertions(+), 36 deletions(-) diff --git a/src/descriptor/checksum.rs b/src/descriptor/checksum.rs index 6ae296433..04c76dc09 100644 --- a/src/descriptor/checksum.rs +++ b/src/descriptor/checksum.rs @@ -8,8 +8,8 @@ //! [BIP-380]: use core::convert::TryFrom; -use core::fmt; use core::iter::FromIterator; +use core::{array, fmt}; use bech32::primitives::checksum::PackedFe32; use bech32::{Checksum, Fe32}; @@ -115,10 +115,10 @@ pub fn verify_checksum(s: &str) -> Result<&str, Error> { eng.input_unchecked(s[..last_hash_pos].as_bytes()); let expected = eng.checksum_chars(); - let mut actual = ['_'; CHECKSUM_LENGTH]; - for (act, ch) in actual.iter_mut().zip(checksum_str.chars()) { - *act = ch; - } + + let mut iter = checksum_str.chars(); + let actual: [char; CHECKSUM_LENGTH] = + array::from_fn(|_| iter.next().expect("length checked above")); if expected != actual { return Err(Error::InvalidChecksum { actual, expected }); diff --git a/src/interpreter/inner.rs b/src/interpreter/inner.rs index 49134c6de..03e6f2840 100644 --- a/src/interpreter/inner.rs +++ b/src/interpreter/inner.rs @@ -429,8 +429,7 @@ mod tests { ", ) .unwrap(); - let mut dummy_sig = [0u8; 48]; - dummy_sig.copy_from_slice(&dummy_sig_vec[..]); + let dummy_sig = <[u8; 48]>::try_from(&dummy_sig_vec[..]).unwrap(); let pkhash = key.to_pubkeyhash(SigType::Ecdsa).into(); let wpkhash = key.to_pubkeyhash(SigType::Ecdsa).into(); diff --git a/src/interpreter/stack.rs b/src/interpreter/stack.rs index 4cf5450af..647a84713 100644 --- a/src/interpreter/stack.rs +++ b/src/interpreter/stack.rs @@ -261,7 +261,7 @@ impl<'txin> Stack<'txin> { self.push(Element::Satisfied); Some(Ok(SatisfiedConstraint::HashLock { hash: HashLockType::Sha256(*hash), - preimage: preimage_from_sl(preimage), + preimage: <[u8; 32]>::try_from(preimage).expect("length checked above"), })) } else { self.push(Element::Dissatisfied); @@ -286,7 +286,7 @@ impl<'txin> Stack<'txin> { self.push(Element::Satisfied); Some(Ok(SatisfiedConstraint::HashLock { hash: HashLockType::Hash256(*hash), - preimage: preimage_from_sl(preimage), + preimage: <[u8; 32]>::try_from(preimage).expect("length checked above"), })) } else { self.push(Element::Dissatisfied); @@ -311,7 +311,7 @@ impl<'txin> Stack<'txin> { self.push(Element::Satisfied); Some(Ok(SatisfiedConstraint::HashLock { hash: HashLockType::Hash160(*hash), - preimage: preimage_from_sl(preimage), + preimage: <[u8; 32]>::try_from(preimage).expect("length checked above"), })) } else { self.push(Element::Dissatisfied); @@ -336,7 +336,7 @@ impl<'txin> Stack<'txin> { self.push(Element::Satisfied); Some(Ok(SatisfiedConstraint::HashLock { hash: HashLockType::Ripemd160(*hash), - preimage: preimage_from_sl(preimage), + preimage: <[u8; 32]>::try_from(preimage).expect("length checked above"), })) } else { self.push(Element::Dissatisfied); @@ -376,14 +376,3 @@ impl<'txin> Stack<'txin> { } } } - -// Helper function to compute preimage from slice -fn preimage_from_sl(sl: &[u8]) -> [u8; 32] { - if sl.len() != 32 { - unreachable!("Internal: Preimage length checked to be 32") - } else { - let mut preimage = [0u8; 32]; - preimage.copy_from_slice(sl); - preimage - } -} diff --git a/src/psbt/mod.rs b/src/psbt/mod.rs index 655878062..65e8c854d 100644 --- a/src/psbt/mod.rs +++ b/src/psbt/mod.rs @@ -340,38 +340,28 @@ impl Satisfier for PsbtInputSatisfier<'_> { self.psbt.inputs[self.index] .hash160_preimages .get(&Pk::to_hash160(h)) - .and_then(|x: &Vec| try_vec_as_preimage32(x)) + .and_then(|x: &Vec| <[u8; 32]>::try_from(&x[..]).ok()) } fn lookup_sha256(&self, h: &Pk::Sha256) -> Option { self.psbt.inputs[self.index] .sha256_preimages .get(&Pk::to_sha256(h)) - .and_then(|x: &Vec| try_vec_as_preimage32(x)) + .and_then(|x: &Vec| <[u8; 32]>::try_from(&x[..]).ok()) } fn lookup_hash256(&self, h: &Pk::Hash256) -> Option { self.psbt.inputs[self.index] .hash256_preimages .get(&sha256d::Hash::from_byte_array(Pk::to_hash256(h).to_byte_array())) // upstream psbt operates on hash256 - .and_then(|x: &Vec| try_vec_as_preimage32(x)) + .and_then(|x: &Vec| <[u8; 32]>::try_from(&x[..]).ok()) } fn lookup_ripemd160(&self, h: &Pk::Ripemd160) -> Option { self.psbt.inputs[self.index] .ripemd160_preimages .get(&Pk::to_ripemd160(h)) - .and_then(|x: &Vec| try_vec_as_preimage32(x)) - } -} - -fn try_vec_as_preimage32(vec: &[u8]) -> Option { - if vec.len() == 32 { - let mut arr = [0u8; 32]; - arr.copy_from_slice(vec); - Some(arr) - } else { - None + .and_then(|x: &Vec| <[u8; 32]>::try_from(&x[..]).ok()) } } From c8460282205a89d5072ec8fcb0ecb6532f9a0fac Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sun, 8 Sep 2024 16:45:36 +0000 Subject: [PATCH 2/9] expression: rewrite parser to be non-recursive This commit should yield no observable changes. --- src/descriptor/tr.rs | 16 ++-- src/expression/mod.rs | 190 ++++++++++++++---------------------------- 2 files changed, 70 insertions(+), 136 deletions(-) diff --git a/src/descriptor/tr.rs b/src/descriptor/tr.rs index 77cceb7e0..d1ad4b7c4 100644 --- a/src/descriptor/tr.rs +++ b/src/descriptor/tr.rs @@ -23,7 +23,7 @@ use crate::policy::Liftable; use crate::prelude::*; use crate::util::{varint_len, witness_size}; use crate::{ - errstr, Error, ForEachKey, FromStrKey, MiniscriptKey, Satisfier, ScriptContext, Tap, Threshold, + Error, ForEachKey, FromStrKey, MiniscriptKey, Satisfier, ScriptContext, Tap, Threshold, ToPublicKey, TranslateErr, Translator, }; @@ -504,10 +504,11 @@ impl Tr { let right = Self::parse_tr_script_spend(&args[1])?; Ok(TapTree::combine(left, right)) } - _ => Err(Error::Unexpected( + _ => { + Err(Error::Unexpected( "unknown format for script spending paths while parsing taproot descriptor" .to_string(), - )), + ))}, } } } @@ -608,12 +609,9 @@ fn parse_tr_tree(s: &str) -> Result { return Err(Error::Unexpected("invalid taproot internal key".to_string())); } let internal_key = expression::Tree { name: key.name, args: vec![] }; - let (tree, rest) = expression::Tree::from_slice_delim(script, 1, '{')?; - if rest.is_empty() { - Ok(expression::Tree { name: "tr", args: vec![internal_key, tree] }) - } else { - Err(errstr(rest)) - } + let tree = expression::Tree::from_slice_delim(script, expression::Deliminator::Taproot) + .map_err(Error::ParseTree)?; + Ok(expression::Tree { name: "tr", args: vec![internal_key, tree] }) } else { Err(Error::Unexpected("invalid taproot descriptor".to_string())) } diff --git a/src/expression/mod.rs b/src/expression/mod.rs index 844b3c869..5c10c7266 100644 --- a/src/expression/mod.rs +++ b/src/expression/mod.rs @@ -44,94 +44,34 @@ impl Eq for Tree<'_> {} // or_b() // pk(A), pk(B) +/// Whether to treat `{` and `}` as deliminators when parsing an expression. +#[derive(Copy, Clone, PartialEq, Eq)] +pub enum Deliminator { + /// Use `(` and `)` as parentheses. + NonTaproot, + /// Use `{` and `}` as parentheses. + Taproot, +} + /// A trait for extracting a structure from a Tree representation in token form pub trait FromTree: Sized { /// Extract a structure from Tree representation fn from_tree(top: &Tree) -> Result; } -enum Found { - Nothing, - LBracket(usize), // Either a left ( or { - Comma(usize), - RBracket(usize), // Either a right ) or } -} - -fn next_expr(sl: &str, delim: char) -> Found { - let mut found = Found::Nothing; - if delim == '(' { - for (n, ch) in sl.char_indices() { - match ch { - '(' => { - found = Found::LBracket(n); - break; - } - ',' => { - found = Found::Comma(n); - break; - } - ')' => { - found = Found::RBracket(n); - break; - } - _ => {} - } - } - } else if delim == '{' { - let mut new_count = 0; - for (n, ch) in sl.char_indices() { - match ch { - '{' => { - found = Found::LBracket(n); - break; - } - '(' => { - new_count += 1; - } - ',' => { - if new_count == 0 { - found = Found::Comma(n); - break; - } - } - ')' => { - new_count -= 1; - } - '}' => { - found = Found::RBracket(n); - break; - } - _ => {} - } - } - } else { - unreachable!("{}", "Internal: delimiters in parsing must be '(' or '{'"); - } - found -} - -// Get the corresponding delim -fn closing_delim(delim: char) -> char { - match delim { - '(' => ')', - '{' => '}', - _ => unreachable!("Unknown delimiter"), - } -} - impl<'a> Tree<'a> { /// Parse an expression with round brackets - pub fn from_slice(sl: &'a str) -> Result<(Tree<'a>, &'a str), Error> { - // Parsing TapTree or just miniscript - Self::from_slice_delim(sl, 0u32, '(') + pub fn from_slice(sl: &'a str) -> Result, ParseTreeError> { + Self::from_slice_delim(sl, Deliminator::NonTaproot) } /// Check that a string is a well-formed expression string, with optional /// checksum. /// - /// Returns the string with the checksum removed. - fn parse_pre_check(s: &str, open: u8, close: u8) -> Result<&str, ParseTreeError> { - // Do ASCII check first; after this we can use .bytes().enumerate() rather + /// Returns the string with the checksum removed and its tree depth. + fn parse_pre_check(s: &str, open: u8, close: u8) -> Result<(&str, usize), ParseTreeError> { + // First, scan through string to make sure it is well-formed. + // Do ASCII/checksum check first; after this we can use .bytes().enumerate() rather // than .char_indices(), which is *significantly* faster. let s = verify_checksum(s)?; @@ -211,68 +151,64 @@ impl<'a> Tree<'a> { }); } - Ok(s) + Ok((s, max_depth)) } - pub(crate) fn from_slice_delim( - mut sl: &'a str, - depth: u32, - delim: char, - ) -> Result<(Tree<'a>, &'a str), Error> { - if depth == 0 { - if delim == '{' { - sl = Self::parse_pre_check(sl, b'{', b'}').map_err(Error::ParseTree)?; - } else { - sl = Self::parse_pre_check(sl, b'(', b')').map_err(Error::ParseTree)?; + pub(crate) fn from_slice_delim(s: &'a str, delim: Deliminator) -> Result { + let (oparen, cparen) = match delim { + Deliminator::NonTaproot => (b'(', b')'), + Deliminator::Taproot => (b'{', b'}'), + }; + + // First, scan through string to make sure it is well-formed. + let (s, max_depth) = Self::parse_pre_check(s, oparen, cparen)?; + + // Now, knowing it is sane and well-formed, we can easily parse it backward, + // which will yield a post-order right-to-left iterator of its nodes. + let mut stack = Vec::with_capacity(max_depth); + let mut children = None; + let mut node_name_end = s.len(); + let mut tapleaf_depth = 0; + for (pos, ch) in s.bytes().enumerate().rev() { + if ch == cparen { + stack.push(vec![]); + node_name_end = pos; + } else if tapleaf_depth == 0 && ch == b',' { + let top = stack.last_mut().unwrap(); + let mut new_tree = Tree { + name: &s[pos + 1..node_name_end], + args: children.take().unwrap_or(vec![]), + }; + new_tree.args.reverse(); + top.push(new_tree); + node_name_end = pos; + } else if ch == oparen { + let mut top = stack.pop().unwrap(); + let mut new_tree = Tree { + name: &s[pos + 1..node_name_end], + args: children.take().unwrap_or(vec![]), + }; + new_tree.args.reverse(); + top.push(new_tree); + children = Some(top); + node_name_end = pos; + } else if delim == Deliminator::Taproot && ch == b'(' { + tapleaf_depth += 1; + } else if delim == Deliminator::Taproot && ch == b')' { + tapleaf_depth -= 1; } } - match next_expr(sl, delim) { - // String-ending terminal - Found::Nothing => Ok((Tree { name: sl, args: vec![] }, "")), - // Terminal - Found::Comma(n) | Found::RBracket(n) => { - Ok((Tree { name: &sl[..n], args: vec![] }, &sl[n..])) - } - // Function call - Found::LBracket(n) => { - let mut ret = Tree { name: &sl[..n], args: vec![] }; - - sl = &sl[n + 1..]; - loop { - let (arg, new_sl) = Tree::from_slice_delim(sl, depth + 1, delim)?; - ret.args.push(arg); - - if new_sl.is_empty() { - unreachable!() - } - - sl = &new_sl[1..]; - match new_sl.as_bytes()[0] { - b',' => {} - last_byte => { - if last_byte == closing_delim(delim) as u8 { - break; - } else { - unreachable!() - } - } - } - } - Ok((ret, sl)) - } - } + assert_eq!(stack.len(), 0); + let mut children = children.take().unwrap_or(vec![]); + children.reverse(); + Ok(Tree { name: &s[..node_name_end], args: children }) } /// Parses a tree from a string #[allow(clippy::should_implement_trait)] // Cannot use std::str::FromStr because of lifetimes. pub fn from_str(s: &'a str) -> Result, Error> { - let (top, rem) = Tree::from_slice(s)?; - if rem.is_empty() { - Ok(top) - } else { - unreachable!() - } + Self::from_slice_delim(s, Deliminator::NonTaproot).map_err(Error::ParseTree) } /// Parses an expression tree as a threshold (a term with at least one child, From f31191cf97f4f8d00d7bf447469578a55df87ffb Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Tue, 12 Nov 2024 22:13:15 +0000 Subject: [PATCH 3/9] expression: start tracking parenthesis-type and string position When we change the expression parser to start parsing both ()s and {}s at once, we will need to know the parenthesis type. To return nice errors we also need to store some position information in the Tree type. Adding these new fields (which need to be pub to make them accessible from descriptor/tr.rs, but which we will later encapsulate better) is mechanical and pretty noisy, so we do it in its own commit to reduce the size of the real "fix Taproot parsing" commit. --- src/descriptor/tr.rs | 32 +++++++++++++---- src/expression/mod.rs | 82 +++++++++++++++++++++++++++++++------------ 2 files changed, 86 insertions(+), 28 deletions(-) diff --git a/src/descriptor/tr.rs b/src/descriptor/tr.rs index d1ad4b7c4..486162deb 100644 --- a/src/descriptor/tr.rs +++ b/src/descriptor/tr.rs @@ -495,11 +495,11 @@ impl Tr { // Helper function to parse taproot script path fn parse_tr_script_spend(tree: &expression::Tree,) -> Result, Error> { match tree { - expression::Tree { name, args } if !name.is_empty() && args.is_empty() => { + expression::Tree { name, args, .. } if !name.is_empty() && args.is_empty() => { let script = Miniscript::::from_str(name)?; Ok(TapTree::Leaf(Arc::new(script))) } - expression::Tree { name, args } if name.is_empty() && args.len() == 2 => { + expression::Tree { name, args, .. } if name.is_empty() && args.len() == 2 => { let left = Self::parse_tr_script_spend(&args[0])?; let right = Self::parse_tr_script_spend(&args[1])?; Ok(TapTree::combine(left, right)) @@ -597,8 +597,18 @@ fn parse_tr_tree(s: &str) -> Result { if !key.args.is_empty() { return Err(Error::Unexpected("invalid taproot internal key".to_string())); } - let internal_key = expression::Tree { name: key.name, args: vec![] }; - return Ok(expression::Tree { name: "tr", args: vec![internal_key] }); + let internal_key = expression::Tree { + name: key.name, + parens: expression::Parens::None, + children_pos: 0, + args: vec![], + }; + return Ok(expression::Tree { + name: "tr", + parens: expression::Parens::Round, + children_pos: 0, + args: vec![internal_key], + }); } // use str::split_once() method to refactor this when compiler version bumps up let (key, script) = split_once(rest, ',') @@ -608,10 +618,20 @@ fn parse_tr_tree(s: &str) -> Result { if !key.args.is_empty() { return Err(Error::Unexpected("invalid taproot internal key".to_string())); } - let internal_key = expression::Tree { name: key.name, args: vec![] }; + let internal_key = expression::Tree { + name: key.name, + parens: expression::Parens::None, + children_pos: 0, + args: vec![], + }; let tree = expression::Tree::from_slice_delim(script, expression::Deliminator::Taproot) .map_err(Error::ParseTree)?; - Ok(expression::Tree { name: "tr", args: vec![internal_key, tree] }) + Ok(expression::Tree { + name: "tr", + parens: expression::Parens::Round, + children_pos: 0, + args: vec![internal_key, tree], + }) } else { Err(Error::Unexpected("invalid taproot descriptor".to_string())) } diff --git a/src/expression/mod.rs b/src/expression/mod.rs index 5c10c7266..d95008c89 100644 --- a/src/expression/mod.rs +++ b/src/expression/mod.rs @@ -21,6 +21,11 @@ pub const INPUT_CHARSET: &str = "0123456789()[],'/*abcdefgh@:$%{}IJKLMNOPQRSTUVW pub struct Tree<'a> { /// The name `x` pub name: &'a str, + /// Position one past the last character of the node's name. If it has + /// children, the position of the '(' or '{'. + pub children_pos: usize, + /// The type of parentheses surrounding the node's children. + pub parens: Parens, /// The comma-separated contents of the `(...)`, if any pub args: Vec>, } @@ -38,11 +43,17 @@ impl PartialEq for Tree<'_> { } } impl Eq for Tree<'_> {} -// or_b(pk(A),pk(B)) -// -// A = musig(musig(B,C),D,E) -// or_b() -// pk(A), pk(B) + +/// The type of parentheses surrounding a node's children. +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub enum Parens { + /// Node has no children. + None, + /// Round parentheses: `(` and `)`. + Round, + /// Curly braces: `{` and `}`. + Curly, +} /// Whether to treat `{` and `}` as deliminators when parsing an expression. #[derive(Copy, Clone, PartialEq, Eq)] @@ -166,7 +177,7 @@ impl<'a> Tree<'a> { // Now, knowing it is sane and well-formed, we can easily parse it backward, // which will yield a post-order right-to-left iterator of its nodes. let mut stack = Vec::with_capacity(max_depth); - let mut children = None; + let mut children_parens: Option<(Vec<_>, usize, Parens)> = None; let mut node_name_end = s.len(); let mut tapleaf_depth = 0; for (pos, ch) in s.bytes().enumerate().rev() { @@ -174,23 +185,37 @@ impl<'a> Tree<'a> { stack.push(vec![]); node_name_end = pos; } else if tapleaf_depth == 0 && ch == b',' { + let (mut args, children_pos, parens) = + children_parens + .take() + .unwrap_or((vec![], node_name_end, Parens::None)); + args.reverse(); + let top = stack.last_mut().unwrap(); - let mut new_tree = Tree { - name: &s[pos + 1..node_name_end], - args: children.take().unwrap_or(vec![]), - }; - new_tree.args.reverse(); + let new_tree = + Tree { name: &s[pos + 1..node_name_end], children_pos, parens, args }; top.push(new_tree); node_name_end = pos; } else if ch == oparen { + let (mut args, children_pos, parens) = + children_parens + .take() + .unwrap_or((vec![], node_name_end, Parens::None)); + args.reverse(); + let mut top = stack.pop().unwrap(); - let mut new_tree = Tree { - name: &s[pos + 1..node_name_end], - args: children.take().unwrap_or(vec![]), - }; - new_tree.args.reverse(); + let new_tree = + Tree { name: &s[pos + 1..node_name_end], children_pos, parens, args }; top.push(new_tree); - children = Some(top); + children_parens = Some(( + top, + pos, + match ch { + b'(' => Parens::Round, + b'{' => Parens::Curly, + _ => unreachable!(), + }, + )); node_name_end = pos; } else if delim == Deliminator::Taproot && ch == b'(' { tapleaf_depth += 1; @@ -200,9 +225,12 @@ impl<'a> Tree<'a> { } assert_eq!(stack.len(), 0); - let mut children = children.take().unwrap_or(vec![]); - children.reverse(); - Ok(Tree { name: &s[..node_name_end], args: children }) + let (mut args, children_pos, parens) = + children_parens + .take() + .unwrap_or((vec![], node_name_end, Parens::None)); + args.reverse(); + Ok(Tree { name: &s[..node_name_end], children_pos, parens, args }) } /// Parses a tree from a string @@ -300,9 +328,19 @@ mod tests { use super::*; /// Test functions to manually build trees - fn leaf(name: &str) -> Tree { Tree { name, args: vec![] } } + fn leaf(name: &str) -> Tree { + Tree { name, parens: Parens::None, children_pos: name.len(), args: vec![] } + } + + fn paren_node<'a>(name: &'a str, mut args: Vec>) -> Tree<'a> { + let mut offset = name.len() + 1; // +1 for open paren + for arg in &mut args { + arg.children_pos += offset; + offset += arg.name.len() + 1; // +1 for comma + } - fn paren_node<'a>(name: &'a str, args: Vec>) -> Tree<'a> { Tree { name, args } } + Tree { name, parens: Parens::Round, children_pos: name.len(), args } + } #[test] fn test_parse_num() { From c8aa7d22fb5356135d1d495c76b11a1b584d7d95 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sat, 31 Aug 2024 17:47:46 +0000 Subject: [PATCH 4/9] expression: move some ad-hoc validation from descriptor module In the next commit we will change the expression parser to parse both {}s and ()s as parentheses, no longer distinguishing between a "taproot" and "non-taproot" mode. This means that all non-Taproot descriptors need to check that curly-brace {} expressions do not appear. While we are adding this check, we can replace the existing checks for things like "does this start with wsh and have exactly one child" with an encapsulated function with strongly-typed errors. This gets rid of a couple of Error::Unexpected instances. We change one error output (if you pass no children to a sortedmulti). The old text is nonsensical and the new text is explicit about what is wrong. This change is pretty-much mechanical, though unfortunately these are all "manual" calls to validation functions, and if I missed any, the compiler won't give us any help in noticing. But there aren't too many. The only subtle/nonobvious thing maybe is that in Miniscript::from_tree I added a call to verify_no_curly_braces which does a recursive check for curly-braces. None of the other checks are recursive (nor do they need to be). Anyway, later on I will write a fuzz test which checks that we have not changed the set of parseable descriptors (using normal keys, not Strings or anything that might have braces in them, which we know we broke) and that should catch any mistakes. Also, similar to the last commit, this one doesn't really "do" anything because it's still impossible to parse trees with mixed brace styles. But in the next one, it will be possible, and we will be glad to have moved a bunch of the diff into these prepatory commits. --- src/descriptor/bare.rs | 13 ++---- src/descriptor/mod.rs | 2 +- src/descriptor/segwitv0.rs | 36 ++++++--------- src/descriptor/sh.rs | 33 ++++++------- src/descriptor/sortedmulti.rs | 3 ++ src/expression/error.rs | 54 ++++++++++++++++++++++ src/expression/mod.rs | 87 ++++++++++++++++++++++++++++++++++- src/miniscript/mod.rs | 1 + 8 files changed, 175 insertions(+), 54 deletions(-) diff --git a/src/descriptor/bare.rs b/src/descriptor/bare.rs index 63cb8168a..8436adfc1 100644 --- a/src/descriptor/bare.rs +++ b/src/descriptor/bare.rs @@ -370,15 +370,10 @@ impl Liftable for Pkh { impl FromTree for Pkh { fn from_tree(top: &expression::Tree) -> Result { - if top.name == "pkh" && top.args.len() == 1 { - Ok(Pkh::new(expression::terminal(&top.args[0], |pk| Pk::from_str(pk))?)?) - } else { - Err(Error::Unexpected(format!( - "{}({} args) while parsing pkh descriptor", - top.name, - top.args.len(), - ))) - } + let top = top + .verify_toplevel("pkh", 1..=1) + .map_err(Error::ParseTree)?; + Ok(Pkh::new(expression::terminal(top, |pk| Pk::from_str(pk))?)?) } } diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs index 6c973cc5d..a1d29b441 100644 --- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -1102,7 +1102,7 @@ mod tests { StdDescriptor::from_str("sh(sortedmulti)") .unwrap_err() .to_string(), - "expected threshold, found terminal", + "sortedmulti must have at least 1 children, but found 0" ); //issue 202 assert_eq!( StdDescriptor::from_str(&format!("sh(sortedmulti(2,{}))", &TEST_PK[3..69])) diff --git a/src/descriptor/segwitv0.rs b/src/descriptor/segwitv0.rs index c8552eb4b..de2e979c5 100644 --- a/src/descriptor/segwitv0.rs +++ b/src/descriptor/segwitv0.rs @@ -248,21 +248,16 @@ impl Liftable for Wsh { impl crate::expression::FromTree for Wsh { fn from_tree(top: &expression::Tree) -> Result { - if top.name == "wsh" && top.args.len() == 1 { - let top = &top.args[0]; - if top.name == "sortedmulti" { - return Ok(Wsh { inner: WshInner::SortedMulti(SortedMultiVec::from_tree(top)?) }); - } - let sub = Miniscript::from_tree(top)?; - Segwitv0::top_level_checks(&sub)?; - Ok(Wsh { inner: WshInner::Ms(sub) }) - } else { - Err(Error::Unexpected(format!( - "{}({} args) while parsing wsh descriptor", - top.name, - top.args.len(), - ))) + let top = top + .verify_toplevel("wsh", 1..=1) + .map_err(Error::ParseTree)?; + + if top.name == "sortedmulti" { + return Ok(Wsh { inner: WshInner::SortedMulti(SortedMultiVec::from_tree(top)?) }); } + let sub = Miniscript::from_tree(top)?; + Segwitv0::top_level_checks(&sub)?; + Ok(Wsh { inner: WshInner::Ms(sub) }) } } @@ -488,15 +483,10 @@ impl Liftable for Wpkh { impl crate::expression::FromTree for Wpkh { fn from_tree(top: &expression::Tree) -> Result { - if top.name == "wpkh" && top.args.len() == 1 { - Ok(Wpkh::new(expression::terminal(&top.args[0], |pk| Pk::from_str(pk))?)?) - } else { - Err(Error::Unexpected(format!( - "{}({} args) while parsing wpkh descriptor", - top.name, - top.args.len(), - ))) - } + let top = top + .verify_toplevel("wpkh", 1..=1) + .map_err(Error::ParseTree)?; + Ok(Wpkh::new(expression::terminal(top, |pk| Pk::from_str(pk))?)?) } } diff --git a/src/descriptor/sh.rs b/src/descriptor/sh.rs index f9f21544d..4cdc0c41e 100644 --- a/src/descriptor/sh.rs +++ b/src/descriptor/sh.rs @@ -82,26 +82,19 @@ impl fmt::Display for Sh { impl crate::expression::FromTree for Sh { fn from_tree(top: &expression::Tree) -> Result { - if top.name == "sh" && top.args.len() == 1 { - let top = &top.args[0]; - let inner = match top.name { - "wsh" => ShInner::Wsh(Wsh::from_tree(top)?), - "wpkh" => ShInner::Wpkh(Wpkh::from_tree(top)?), - "sortedmulti" => ShInner::SortedMulti(SortedMultiVec::from_tree(top)?), - _ => { - let sub = Miniscript::from_tree(top)?; - Legacy::top_level_checks(&sub)?; - ShInner::Ms(sub) - } - }; - Ok(Sh { inner }) - } else { - Err(Error::Unexpected(format!( - "{}({} args) while parsing sh descriptor", - top.name, - top.args.len(), - ))) - } + let top = top.verify_toplevel("sh", 1..=1).map_err(Error::ParseTree)?; + + let inner = match top.name { + "wsh" => ShInner::Wsh(Wsh::from_tree(top)?), + "wpkh" => ShInner::Wpkh(Wpkh::from_tree(top)?), + "sortedmulti" => ShInner::SortedMulti(SortedMultiVec::from_tree(top)?), + _ => { + let sub = Miniscript::from_tree(top)?; + Legacy::top_level_checks(&sub)?; + ShInner::Ms(sub) + } + }; + Ok(Sh { inner }) } } diff --git a/src/descriptor/sortedmulti.rs b/src/descriptor/sortedmulti.rs index b8a378a26..be5678a82 100644 --- a/src/descriptor/sortedmulti.rs +++ b/src/descriptor/sortedmulti.rs @@ -64,6 +64,9 @@ impl SortedMultiVec { Pk: FromStr, ::Err: fmt::Display, { + tree.verify_toplevel("sortedmulti", 1..) + .map_err(Error::ParseTree)?; + let ret = Self { inner: tree .to_null_threshold() diff --git a/src/expression/error.rs b/src/expression/error.rs index d705e2df5..b9292a445 100644 --- a/src/expression/error.rs +++ b/src/expression/error.rs @@ -53,6 +53,29 @@ pub enum ParseTreeError { /// The position of the closing parethesis. close_pos: usize, }, + /// A node had the wrong name. + IncorrectName { + /// The name that was found. + actual: String, + /// The name that was expected. + expected: &'static str, + }, + /// A node had the wrong number of children. + IncorrectNumberOfChildren { + /// A description of the node in question. + description: &'static str, + /// The number of children the node had. + n_children: usize, + /// The minimum of children the node should have had. + minimum: Option, + /// The minimum of children the node should have had. + maximum: Option, + }, + /// A Taproot child occurred somewhere it was not allowed. + IllegalCurlyBrace { + /// The position of the opening curly brace. + pos: usize, + }, /// Data occurred after the final ). TrailingCharacter { /// The first trailing character. @@ -93,6 +116,34 @@ impl fmt::Display for ParseTreeError { open_ch, open_pos, close_ch, close_pos ) } + ParseTreeError::IllegalCurlyBrace { pos } => { + write!(f, "illegal `{{` at position {} (Taproot branches not allowed here)", pos) + } + ParseTreeError::IncorrectName { actual, expected } => { + if expected.is_empty() { + write!(f, "found node '{}', expected nameless node", actual) + } else { + write!(f, "expected node '{}', found '{}'", expected, actual) + } + } + ParseTreeError::IncorrectNumberOfChildren { + description, + n_children, + minimum, + maximum, + } => { + write!(f, "{} must have ", description)?; + match (minimum, maximum) { + (_, Some(0)) => f.write_str("no children"), + (Some(min), Some(max)) if min == max => write!(f, "{} children", min), + (Some(min), None) if n_children < min => write!(f, "at least {} children", min), + (Some(min), Some(max)) if n_children < min => write!(f, "at least {} children (maximum {})", min, max), + (None, Some(max)) if n_children > max => write!(f, "at most {} children", max), + (Some(min), Some(max)) if n_children > max => write!(f, "at most {} children (minimum {})", max, min), + (x, y) => panic!("IncorrectNumberOfChildren error was constructed inconsistently (min {:?} max {:?})", x, y), + }?; + write!(f, ", but found {}", n_children) + } ParseTreeError::TrailingCharacter { ch, pos } => { write!(f, "trailing data `{}...` (position {})", ch, pos) } @@ -109,6 +160,9 @@ impl std::error::Error for ParseTreeError { | ParseTreeError::UnmatchedOpenParen { .. } | ParseTreeError::UnmatchedCloseParen { .. } | ParseTreeError::MismatchedParens { .. } + | ParseTreeError::IllegalCurlyBrace { .. } + | ParseTreeError::IncorrectName { .. } + | ParseTreeError::IncorrectNumberOfChildren { .. } | ParseTreeError::TrailingCharacter { .. } => None, } } diff --git a/src/expression/mod.rs b/src/expression/mod.rs index d95008c89..6f8367827 100644 --- a/src/expression/mod.rs +++ b/src/expression/mod.rs @@ -5,11 +5,12 @@ mod error; -use core::fmt; use core::str::FromStr; +use core::{fmt, ops}; pub use self::error::{ParseThresholdError, ParseTreeError}; use crate::descriptor::checksum::verify_checksum; +use crate::iter::{self, TreeLike}; use crate::prelude::*; use crate::{errstr, Error, Threshold, MAX_RECURSION_DEPTH}; @@ -44,6 +45,21 @@ impl PartialEq for Tree<'_> { } impl Eq for Tree<'_> {} +impl<'a, 't> TreeLike for &'t Tree<'a> { + type NaryChildren = &'t [Tree<'a>]; + + fn nary_len(tc: &Self::NaryChildren) -> usize { tc.len() } + fn nary_index(tc: Self::NaryChildren, idx: usize) -> Self { &tc[idx] } + + fn as_node(&self) -> iter::Tree { + if self.args.is_empty() { + iter::Tree::Nullary + } else { + iter::Tree::Nary(&self.args) + } + } +} + /// The type of parentheses surrounding a node's children. #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum Parens { @@ -71,6 +87,75 @@ pub trait FromTree: Sized { } impl<'a> Tree<'a> { + /// Check that a tree node has the given number of children. + /// + /// The `description` argument is only used to populate the error return, + /// and is not validated in any way. + pub fn verify_n_children( + &self, + description: &'static str, + n_children: impl ops::RangeBounds, + ) -> Result<(), ParseTreeError> { + if n_children.contains(&self.n_children()) { + Ok(()) + } else { + let minimum = match n_children.start_bound() { + ops::Bound::Included(n) => Some(*n), + ops::Bound::Excluded(n) => Some(*n + 1), + ops::Bound::Unbounded => None, + }; + let maximum = match n_children.end_bound() { + ops::Bound::Included(n) => Some(*n), + ops::Bound::Excluded(n) => Some(*n - 1), + ops::Bound::Unbounded => None, + }; + Err(ParseTreeError::IncorrectNumberOfChildren { + description, + n_children: self.n_children(), + minimum, + maximum, + }) + } + } + + /// Check that a tree node has the given name, one child, and round braces. + /// + /// Returns the first child. + /// + /// # Panics + /// + /// Panics if zero is in bounds for `n_children` (since then there may be + /// no sensible value to return). + pub fn verify_toplevel( + &self, + name: &'static str, + n_children: impl ops::RangeBounds, + ) -> Result<&Self, ParseTreeError> { + assert!( + !n_children.contains(&0), + "verify_toplevel is intended for nodes with >= 1 child" + ); + + if self.name != name { + Err(ParseTreeError::IncorrectName { actual: self.name.to_owned(), expected: name }) + } else if self.parens == Parens::Curly { + Err(ParseTreeError::IllegalCurlyBrace { pos: self.children_pos }) + } else { + self.verify_n_children(name, n_children)?; + Ok(&self.args[0]) + } + } + + /// Check that a tree has no curly-brace children in it. + pub fn verify_no_curly_braces(&self) -> Result<(), ParseTreeError> { + for tree in self.pre_order_iter() { + if tree.parens == Parens::Curly { + return Err(ParseTreeError::IllegalCurlyBrace { pos: tree.children_pos }); + } + } + Ok(()) + } + /// Parse an expression with round brackets pub fn from_slice(sl: &'a str) -> Result, ParseTreeError> { Self::from_slice_delim(sl, Deliminator::NonTaproot) diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs index 03c242ec8..f4fdfca5c 100644 --- a/src/miniscript/mod.rs +++ b/src/miniscript/mod.rs @@ -802,6 +802,7 @@ impl crate::expression::FromTree for Miniscr /// Parse an expression tree into a Miniscript. As a general rule, this /// should not be called directly; rather go through the descriptor API. fn from_tree(top: &expression::Tree) -> Result, Error> { + top.verify_no_curly_braces().map_err(Error::ParseTree)?; let inner: Terminal = expression::FromTree::from_tree(top)?; Miniscript::from_ast(inner) } From beacc8dc4c42e057a40d3854ad880248dc00620f Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Wed, 13 Nov 2024 14:45:38 +0000 Subject: [PATCH 5/9] Introduce `error` module with single `ParseError` enum in it. We will use this error type in the next commit. For now we will continue to use the giant global Error enum, so we add a variant to hold the new `ParseError`. But eventually `ParseError` will become a top-level error and `Error` will go away. --- src/error.rs | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++ src/lib.rs | 12 ++++++++---- 2 files changed, 61 insertions(+), 4 deletions(-) create mode 100644 src/error.rs diff --git a/src/error.rs b/src/error.rs new file mode 100644 index 000000000..f66cd42f1 --- /dev/null +++ b/src/error.rs @@ -0,0 +1,53 @@ +// Written in 2019 by Andrew Poelstra +// SPDX-License-Identifier: CC0-1.0 + +//! Errors + +use core::fmt; +#[cfg(feature = "std")] +use std::error; + +use crate::blanket_traits::StaticDebugAndDisplay; +use crate::Box; +/// An error parsing a Miniscript object (policy, descriptor or miniscript) +/// from a string. +#[derive(Debug)] +pub enum ParseError { + /// Failed to parse a public key or hash. + /// + /// Note that the error information is lost for nostd compatibility reasons. See + /// . + FromStr(Box), + /// Error parsing a string into an expression tree. + Tree(crate::ParseTreeError), +} + +impl ParseError { + /// Boxes a `FromStr` error for a `Pk` (or associated types) into a `ParseError` + pub(crate) fn box_from_str(e: E) -> Self { + ParseError::FromStr(Box::new(e)) + } +} + +impl From for ParseError { + fn from(e: crate::ParseTreeError) -> Self { Self::Tree(e) } +} + +impl fmt::Display for ParseError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + ParseError::FromStr(ref e) => e.fmt(f), + ParseError::Tree(ref e) => e.fmt(f), + } + } +} + +#[cfg(feature = "std")] +impl error::Error for ParseError { + fn source(&self) -> Option<&(dyn error::Error + 'static)> { + match self { + ParseError::FromStr(..) => None, + ParseError::Tree(ref e) => Some(e), + } + } +} diff --git a/src/lib.rs b/src/lib.rs index f15d99e46..90a92d32b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -114,6 +114,7 @@ mod pub_macros; mod benchmarks; mod blanket_traits; pub mod descriptor; +mod error; pub mod expression; pub mod interpreter; pub mod iter; @@ -128,8 +129,6 @@ mod test_utils; mod util; use core::{fmt, hash, str}; -#[cfg(feature = "std")] -use std::error; use bitcoin::hashes::{hash160, ripemd160, sha256, Hash}; use bitcoin::hex::DisplayHex; @@ -137,6 +136,7 @@ use bitcoin::{script, Opcode}; pub use crate::blanket_traits::FromStrKey; pub use crate::descriptor::{DefiniteDescriptorKey, Descriptor, DescriptorPublicKey}; +pub use crate::error::ParseError; pub use crate::expression::{ParseThresholdError, ParseTreeError}; pub use crate::interpreter::Interpreter; pub use crate::miniscript::analyzable::{AnalysisError, ExtParams}; @@ -494,6 +494,8 @@ pub enum Error { ParseThreshold(ParseThresholdError), /// Invalid expression tree. ParseTree(ParseTreeError), + /// Invalid expression tree. + Parse(ParseError), } // https://github.com/sipa/miniscript/pull/5 for discussion on this number @@ -556,13 +558,14 @@ impl fmt::Display for Error { Error::Threshold(ref e) => e.fmt(f), Error::ParseThreshold(ref e) => e.fmt(f), Error::ParseTree(ref e) => e.fmt(f), + Error::Parse(ref e) => e.fmt(f), } } } #[cfg(feature = "std")] -impl error::Error for Error { - fn cause(&self) -> Option<&dyn error::Error> { +impl std::error::Error for Error { + fn cause(&self) -> Option<&dyn std::error::Error> { use self::Error::*; match self { @@ -607,6 +610,7 @@ impl error::Error for Error { Threshold(e) => Some(e), ParseThreshold(e) => Some(e), ParseTree(e) => Some(e), + Parse(e) => Some(e), } } } From 7b6f49dd4a6018848c216a9905ab36e50f8559db Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sun, 8 Sep 2024 16:47:03 +0000 Subject: [PATCH 6/9] expression: unify Taproot and non-Taproot parsing The `expression::Tree` type now parses expression trees containing both {} and () as children. It marks which is which, and it is the responsibility of FromTree implementors to make sure that the correct style is used. This eliminates a ton of ad-hoc string parsing code from descriptor/tr.rs, including 8 instances of Error::Unexpected. It is also the first change that preserves (sorta) a pubkey parsing error rather than converting it to a string. Because of https://github.com/rust-bitcoin/rust-miniscript/issues/734 this inserts a call to `Descriptor::sanity_check` when parsing a string, specifically for Taproot descriptors. This is weird and wrong but we want to address it separately from this PR, whose goal is to preserve all behavior. --- src/descriptor/mod.rs | 38 +++++-- src/descriptor/tr.rs | 234 +++++++++++++++++++----------------------- src/expression/mod.rs | 69 +++++-------- 3 files changed, 158 insertions(+), 183 deletions(-) diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs index a1d29b441..f0791dc1d 100644 --- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -21,6 +21,7 @@ use bitcoin::{ }; use sync::Arc; +use crate::expression::FromTree as _; use crate::miniscript::decode::Terminal; use crate::miniscript::{satisfy, Legacy, Miniscript, Segwitv0}; use crate::plan::{AssetProvider, Plan}; @@ -981,17 +982,17 @@ impl crate::expression::FromTree for Descriptor { impl FromStr for Descriptor { type Err = Error; fn from_str(s: &str) -> Result, Error> { - // tr tree parsing has special code - // Tr::from_str will check the checksum - // match "tr(" to handle more extensibly - let desc = if s.starts_with("tr(") { - Ok(Descriptor::Tr(Tr::from_str(s)?)) - } else { - let top = expression::Tree::from_str(s)?; - expression::FromTree::from_tree(&top) - }?; - - Ok(desc) + let top = expression::Tree::from_str(s)?; + let ret = Self::from_tree(&top)?; + if let Descriptor::Tr(ref inner) = ret { + // FIXME preserve weird/broken behavior from 12.x. + // See https://github.com/rust-bitcoin/rust-miniscript/issues/734 + ret.sanity_check()?; + for (_, ms) in inner.iter_scripts() { + ms.ext_check(&crate::miniscript::analyzable::ExtParams::sane())?; + } + } + Ok(ret) } } @@ -1545,6 +1546,21 @@ mod tests { ) } + #[test] + fn tr_named_branch() { + use crate::{ParseError, ParseTreeError}; + + assert!(matches!( + StdDescriptor::from_str( + "tr(0202d44008000010100000000084F0000000dd0dd00000000000201dceddd00d00,abc{0,0})" + ), + Err(Error::Parse(ParseError::Tree(ParseTreeError::IncorrectName { + expected: "", + .. + }))), + )); + } + #[test] fn roundtrip_tests() { let descriptor = Descriptor::::from_str("multi"); diff --git a/src/descriptor/tr.rs b/src/descriptor/tr.rs index 486162deb..cc0dd1945 100644 --- a/src/descriptor/tr.rs +++ b/src/descriptor/tr.rs @@ -1,6 +1,5 @@ // SPDX-License-Identifier: CC0-1.0 -use core::str::FromStr; use core::{cmp, fmt, hash}; #[cfg(not(test))] // https://github.com/rust-lang/rust/issues/121684 @@ -12,9 +11,10 @@ use bitcoin::taproot::{ use bitcoin::{opcodes, Address, Network, ScriptBuf, Weight}; use sync::Arc; -use super::checksum::{self, verify_checksum}; +use super::checksum; use crate::descriptor::DefiniteDescriptorKey; use crate::expression::{self, FromTree}; +use crate::iter::TreeLike as _; use crate::miniscript::satisfy::{Placeholder, Satisfaction, SchnorrSigType, Witness}; use crate::miniscript::Miniscript; use crate::plan::AssetProvider; @@ -23,8 +23,8 @@ use crate::policy::Liftable; use crate::prelude::*; use crate::util::{varint_len, witness_size}; use crate::{ - Error, ForEachKey, FromStrKey, MiniscriptKey, Satisfier, ScriptContext, Tap, Threshold, - ToPublicKey, TranslateErr, Translator, + Error, ForEachKey, FromStrKey, MiniscriptKey, ParseError, Satisfier, ScriptContext, Tap, + Threshold, ToPublicKey, TranslateErr, Translator, }; /// A Taproot Tree representation. @@ -490,79 +490,114 @@ where } } -#[rustfmt::skip] -impl Tr { - // Helper function to parse taproot script path - fn parse_tr_script_spend(tree: &expression::Tree,) -> Result, Error> { - match tree { - expression::Tree { name, args, .. } if !name.is_empty() && args.is_empty() => { - let script = Miniscript::::from_str(name)?; - Ok(TapTree::Leaf(Arc::new(script))) - } - expression::Tree { name, args, .. } if name.is_empty() && args.len() == 2 => { - let left = Self::parse_tr_script_spend(&args[0])?; - let right = Self::parse_tr_script_spend(&args[1])?; - Ok(TapTree::combine(left, right)) - } - _ => { - Err(Error::Unexpected( - "unknown format for script spending paths while parsing taproot descriptor" - .to_string(), - ))}, - } +impl core::str::FromStr for Tr { + type Err = Error; + + fn from_str(s: &str) -> Result { + let expr_tree = expression::Tree::from_str(s)?; + Self::from_tree(&expr_tree) } } impl crate::expression::FromTree for Tr { - fn from_tree(top: &expression::Tree) -> Result { - if top.name == "tr" { - match top.args.len() { - 1 => { - let key = &top.args[0]; - if !key.args.is_empty() { - return Err(Error::Unexpected(format!( - "#{} script associated with `key-path` while parsing taproot descriptor", - key.args.len() - ))); + fn from_tree(expr_tree: &expression::Tree) -> Result { + use crate::expression::{Parens, ParseTreeError}; + + expr_tree + .verify_toplevel("tr", 1..=2) + .map_err(From::from) + .map_err(Error::Parse)?; + + let mut round_paren_depth = 0; + + let mut internal_key = None; + let mut tree_stack = vec![]; + + for item in expr_tree.verbose_pre_order_iter() { + // Top-level "tr" node. + if item.index == 0 { + if item.is_complete { + debug_assert!( + internal_key.is_some(), + "checked above that top-level 'tr' has children" + ); + } + } else if item.index == 1 { + // First child of tr, which must be the internal key + if !item.node.args.is_empty() { + return Err(Error::Parse(ParseError::Tree( + ParseTreeError::IncorrectNumberOfChildren { + description: "internal key", + n_children: item.node.args.len(), + minimum: Some(0), + maximum: Some(0), + }, + ))); + } + internal_key = Some( + Pk::from_str(item.node.name) + .map_err(ParseError::box_from_str) + .map_err(Error::Parse)?, + ); + } else { + // From here on we are into the taptree. + if item.n_children_yielded == 0 { + match item.node.parens { + Parens::Curly => { + if !item.node.name.is_empty() { + return Err(Error::Parse(ParseError::Tree( + ParseTreeError::IncorrectName { + actual: item.node.name.to_owned(), + expected: "", + }, + ))); + } + if round_paren_depth > 0 { + return Err(Error::Parse(ParseError::Tree( + ParseTreeError::IllegalCurlyBrace { + pos: item.node.children_pos, + }, + ))); + } + } + Parens::Round => round_paren_depth += 1, + _ => {} } - Tr::new(expression::terminal(key, Pk::from_str)?, None) } - 2 => { - let key = &top.args[0]; - if !key.args.is_empty() { - return Err(Error::Unexpected(format!( - "#{} script associated with `key-path` while parsing taproot descriptor", - key.args.len() - ))); + if item.is_complete { + if item.node.parens == Parens::Curly { + if item.n_children_yielded == 2 { + let rchild = tree_stack.pop().unwrap(); + let lchild = tree_stack.pop().unwrap(); + tree_stack.push(TapTree::combine(lchild, rchild)); + } else { + return Err(Error::Parse(ParseError::Tree( + ParseTreeError::IncorrectNumberOfChildren { + description: "Taptree node", + n_children: item.n_children_yielded, + minimum: Some(2), + maximum: Some(2), + }, + ))); + } + } else { + if item.node.parens == Parens::Round { + round_paren_depth -= 1; + } + if round_paren_depth == 0 { + let script = Miniscript::from_tree(item.node)?; + // FIXME hack for https://github.com/rust-bitcoin/rust-miniscript/issues/734 + if script.ty.corr.base != crate::miniscript::types::Base::B { + return Err(Error::NonTopLevel(format!("{:?}", script))); + }; + tree_stack.push(TapTree::Leaf(Arc::new(script))); + } } - let tree = &top.args[1]; - let ret = Self::parse_tr_script_spend(tree)?; - Tr::new(expression::terminal(key, Pk::from_str)?, Some(ret)) } - _ => Err(Error::Unexpected(format!( - "{}[#{} args] while parsing taproot descriptor", - top.name, - top.args.len() - ))), } - } else { - Err(Error::Unexpected(format!( - "{}[#{} args] while parsing taproot descriptor", - top.name, - top.args.len() - ))) } - } -} - -impl core::str::FromStr for Tr { - type Err = Error; - fn from_str(s: &str) -> Result { - let desc_str = verify_checksum(s) - .map_err(From::from) - .map_err(Error::ParseTree)?; - let top = parse_tr_tree(desc_str)?; - Self::from_tree(&top) + assert!(tree_stack.len() <= 1); + Tr::new(internal_key.unwrap(), tree_stack.pop()) } } @@ -588,69 +623,6 @@ impl fmt::Display for Tr { } } -// Helper function to parse string into miniscript tree form -fn parse_tr_tree(s: &str) -> Result { - if s.len() > 3 && &s[..3] == "tr(" && s.as_bytes()[s.len() - 1] == b')' { - let rest = &s[3..s.len() - 1]; - if !rest.contains(',') { - let key = expression::Tree::from_str(rest)?; - if !key.args.is_empty() { - return Err(Error::Unexpected("invalid taproot internal key".to_string())); - } - let internal_key = expression::Tree { - name: key.name, - parens: expression::Parens::None, - children_pos: 0, - args: vec![], - }; - return Ok(expression::Tree { - name: "tr", - parens: expression::Parens::Round, - children_pos: 0, - args: vec![internal_key], - }); - } - // use str::split_once() method to refactor this when compiler version bumps up - let (key, script) = split_once(rest, ',') - .ok_or_else(|| Error::BadDescriptor("invalid taproot descriptor".to_string()))?; - - let key = expression::Tree::from_str(key)?; - if !key.args.is_empty() { - return Err(Error::Unexpected("invalid taproot internal key".to_string())); - } - let internal_key = expression::Tree { - name: key.name, - parens: expression::Parens::None, - children_pos: 0, - args: vec![], - }; - let tree = expression::Tree::from_slice_delim(script, expression::Deliminator::Taproot) - .map_err(Error::ParseTree)?; - Ok(expression::Tree { - name: "tr", - parens: expression::Parens::Round, - children_pos: 0, - args: vec![internal_key, tree], - }) - } else { - Err(Error::Unexpected("invalid taproot descriptor".to_string())) - } -} - -fn split_once(inp: &str, delim: char) -> Option<(&str, &str)> { - if inp.is_empty() { - None - } else { - // find the first character that matches delim - let res = inp - .chars() - .position(|ch| ch == delim) - .map(|idx| (&inp[..idx], &inp[idx + 1..])) - .unwrap_or((inp, "")); - Some(res) - } -} - impl Liftable for TapTree { fn lift(&self) -> Result, Error> { fn lift_helper(s: &TapTree) -> Result, Error> { @@ -767,6 +739,8 @@ where #[cfg(test)] mod tests { + use core::str::FromStr; + use super::*; fn descriptor() -> String { diff --git a/src/expression/mod.rs b/src/expression/mod.rs index 6f8367827..8b0a4258f 100644 --- a/src/expression/mod.rs +++ b/src/expression/mod.rs @@ -71,15 +71,6 @@ pub enum Parens { Curly, } -/// Whether to treat `{` and `}` as deliminators when parsing an expression. -#[derive(Copy, Clone, PartialEq, Eq)] -pub enum Deliminator { - /// Use `(` and `)` as parentheses. - NonTaproot, - /// Use `{` and `}` as parentheses. - Taproot, -} - /// A trait for extracting a structure from a Tree representation in token form pub trait FromTree: Sized { /// Extract a structure from Tree representation @@ -156,16 +147,11 @@ impl<'a> Tree<'a> { Ok(()) } - /// Parse an expression with round brackets - pub fn from_slice(sl: &'a str) -> Result, ParseTreeError> { - Self::from_slice_delim(sl, Deliminator::NonTaproot) - } - /// Check that a string is a well-formed expression string, with optional /// checksum. /// /// Returns the string with the checksum removed and its tree depth. - fn parse_pre_check(s: &str, open: u8, close: u8) -> Result<(&str, usize), ParseTreeError> { + fn parse_pre_check(s: &str) -> Result<(&str, usize), ParseTreeError> { // First, scan through string to make sure it is well-formed. // Do ASCII/checksum check first; after this we can use .bytes().enumerate() rather // than .char_indices(), which is *significantly* faster. @@ -174,12 +160,12 @@ impl<'a> Tree<'a> { let mut max_depth = 0; let mut open_paren_stack = Vec::with_capacity(128); for (pos, ch) in s.bytes().enumerate() { - if ch == open { + if ch == b'(' || ch == b'{' { open_paren_stack.push((ch, pos)); if max_depth < open_paren_stack.len() { max_depth = open_paren_stack.len(); } - } else if ch == close { + } else if ch == b')' || ch == b'}' { if let Some((open_ch, open_pos)) = open_paren_stack.pop() { if (open_ch == b'(' && ch == b'}') || (open_ch == b'{' && ch == b')') { return Err(ParseTreeError::MismatchedParens { @@ -250,26 +236,26 @@ impl<'a> Tree<'a> { Ok((s, max_depth)) } - pub(crate) fn from_slice_delim(s: &'a str, delim: Deliminator) -> Result { - let (oparen, cparen) = match delim { - Deliminator::NonTaproot => (b'(', b')'), - Deliminator::Taproot => (b'{', b'}'), - }; + /// Parses a tree from a string + #[allow(clippy::should_implement_trait)] // Cannot use std::str::FromStr because of lifetimes. + pub fn from_str(s: &'a str) -> Result { + Self::from_str_inner(s).map_err(Error::ParseTree) + } + fn from_str_inner(s: &'a str) -> Result { // First, scan through string to make sure it is well-formed. - let (s, max_depth) = Self::parse_pre_check(s, oparen, cparen)?; + let (s, max_depth) = Self::parse_pre_check(s)?; // Now, knowing it is sane and well-formed, we can easily parse it backward, // which will yield a post-order right-to-left iterator of its nodes. let mut stack = Vec::with_capacity(max_depth); let mut children_parens: Option<(Vec<_>, usize, Parens)> = None; let mut node_name_end = s.len(); - let mut tapleaf_depth = 0; for (pos, ch) in s.bytes().enumerate().rev() { - if ch == cparen { + if ch == b')' || ch == b'}' { stack.push(vec![]); node_name_end = pos; - } else if tapleaf_depth == 0 && ch == b',' { + } else if ch == b',' { let (mut args, children_pos, parens) = children_parens .take() @@ -281,7 +267,7 @@ impl<'a> Tree<'a> { Tree { name: &s[pos + 1..node_name_end], children_pos, parens, args }; top.push(new_tree); node_name_end = pos; - } else if ch == oparen { + } else if ch == b'(' || ch == b'{' { let (mut args, children_pos, parens) = children_parens .take() @@ -302,10 +288,6 @@ impl<'a> Tree<'a> { }, )); node_name_end = pos; - } else if delim == Deliminator::Taproot && ch == b'(' { - tapleaf_depth += 1; - } else if delim == Deliminator::Taproot && ch == b')' { - tapleaf_depth -= 1; } } @@ -318,12 +300,6 @@ impl<'a> Tree<'a> { Ok(Tree { name: &s[..node_name_end], children_pos, parens, args }) } - /// Parses a tree from a string - #[allow(clippy::should_implement_trait)] // Cannot use std::str::FromStr because of lifetimes. - pub fn from_str(s: &'a str) -> Result, Error> { - Self::from_slice_delim(s, Deliminator::NonTaproot).map_err(Error::ParseTree) - } - /// Parses an expression tree as a threshold (a term with at least one child, /// the first of which is a positive integer k). /// @@ -427,6 +403,16 @@ mod tests { Tree { name, parens: Parens::Round, children_pos: name.len(), args } } + fn brace_node<'a>(name: &'a str, mut args: Vec>) -> Tree<'a> { + let mut offset = name.len() + 1; // +1 for open paren + for arg in &mut args { + arg.children_pos += offset; + offset += arg.name.len() + 1; // +1 for comma + } + + Tree { name, parens: Parens::Curly, children_pos: name.len(), args } + } + #[test] fn test_parse_num() { assert!(parse_num("0").is_ok()); @@ -518,11 +504,10 @@ mod tests { #[test] fn parse_tree_taproot() { - // This test will change in a later PR which unifies TR and non-TR parsing. - assert!(matches!( - Tree::from_str("a{b(c),d}").unwrap_err(), - Error::ParseTree(ParseTreeError::TrailingCharacter { ch: ',', pos: 6 }), - )); + assert_eq!( + Tree::from_str("a{b(c),d}").unwrap(), + brace_node("a", vec![paren_node("b", vec![leaf("c")]), leaf("d")]), + ); } #[test] From a8b98e6e24d141f360d18f686ecbbaf00a58e4c8 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Mon, 2 Sep 2024 17:33:51 +0000 Subject: [PATCH 7/9] expression: drop Error::ParseTree variant The correct way to build a Error::ParseTree is to make an Error::Parse containing a ParseError::Tree. Eventually when we get rid of the top-level Error enum this will be more sensible, and we will be able to use ? instead of this awful `.map_err(From::from).map_err(Error::parse)` construction. This commit rightfully belongs as part of the previous commit, but it's noisy and mechanical so I separated it out. --- src/descriptor/bare.rs | 3 ++- src/descriptor/mod.rs | 3 ++- src/descriptor/segwitv0.rs | 6 ++++-- src/descriptor/sh.rs | 5 ++++- src/descriptor/sortedmulti.rs | 3 ++- src/expression/mod.rs | 37 ++++++++++++++++++++++------------- src/lib.rs | 4 ---- src/miniscript/mod.rs | 4 +++- 8 files changed, 40 insertions(+), 25 deletions(-) diff --git a/src/descriptor/bare.rs b/src/descriptor/bare.rs index 8436adfc1..3ff49cc39 100644 --- a/src/descriptor/bare.rs +++ b/src/descriptor/bare.rs @@ -372,7 +372,8 @@ impl FromTree for Pkh { fn from_tree(top: &expression::Tree) -> Result { let top = top .verify_toplevel("pkh", 1..=1) - .map_err(Error::ParseTree)?; + .map_err(From::from) + .map_err(Error::Parse)?; Ok(Pkh::new(expression::terminal(top, |pk| Pk::from_str(pk))?)?) } } diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs index f0791dc1d..72d2f57a2 100644 --- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -1852,9 +1852,10 @@ mod tests { // https://github.com/bitcoin/bitcoin/blob/7ae86b3c6845873ca96650fc69beb4ae5285c801/src/test/descriptor_tests.cpp#L355-L360 macro_rules! check_invalid_checksum { ($secp: ident,$($desc: expr),*) => { + use crate::{ParseError, ParseTreeError}; $( match Descriptor::parse_descriptor($secp, $desc) { - Err(Error::ParseTree(crate::ParseTreeError::Checksum(_))) => {}, + Err(Error::Parse(ParseError::Tree(ParseTreeError::Checksum(_)))) => {}, Err(e) => panic!("Expected bad checksum for {}, got '{}'", $desc, e), _ => panic!("Invalid checksum treated as valid: {}", $desc), }; diff --git a/src/descriptor/segwitv0.rs b/src/descriptor/segwitv0.rs index de2e979c5..bfb33b935 100644 --- a/src/descriptor/segwitv0.rs +++ b/src/descriptor/segwitv0.rs @@ -250,7 +250,8 @@ impl crate::expression::FromTree for Wsh { fn from_tree(top: &expression::Tree) -> Result { let top = top .verify_toplevel("wsh", 1..=1) - .map_err(Error::ParseTree)?; + .map_err(From::from) + .map_err(Error::Parse)?; if top.name == "sortedmulti" { return Ok(Wsh { inner: WshInner::SortedMulti(SortedMultiVec::from_tree(top)?) }); @@ -485,7 +486,8 @@ impl crate::expression::FromTree for Wpkh { fn from_tree(top: &expression::Tree) -> Result { let top = top .verify_toplevel("wpkh", 1..=1) - .map_err(Error::ParseTree)?; + .map_err(From::from) + .map_err(Error::Parse)?; Ok(Wpkh::new(expression::terminal(top, |pk| Pk::from_str(pk))?)?) } } diff --git a/src/descriptor/sh.rs b/src/descriptor/sh.rs index 4cdc0c41e..f04ddae1c 100644 --- a/src/descriptor/sh.rs +++ b/src/descriptor/sh.rs @@ -82,7 +82,10 @@ impl fmt::Display for Sh { impl crate::expression::FromTree for Sh { fn from_tree(top: &expression::Tree) -> Result { - let top = top.verify_toplevel("sh", 1..=1).map_err(Error::ParseTree)?; + let top = top + .verify_toplevel("sh", 1..=1) + .map_err(From::from) + .map_err(Error::Parse)?; let inner = match top.name { "wsh" => ShInner::Wsh(Wsh::from_tree(top)?), diff --git a/src/descriptor/sortedmulti.rs b/src/descriptor/sortedmulti.rs index be5678a82..1aa96c094 100644 --- a/src/descriptor/sortedmulti.rs +++ b/src/descriptor/sortedmulti.rs @@ -65,7 +65,8 @@ impl SortedMultiVec { ::Err: fmt::Display, { tree.verify_toplevel("sortedmulti", 1..) - .map_err(Error::ParseTree)?; + .map_err(From::from) + .map_err(Error::Parse)?; let ret = Self { inner: tree diff --git a/src/expression/mod.rs b/src/expression/mod.rs index 8b0a4258f..14978def2 100644 --- a/src/expression/mod.rs +++ b/src/expression/mod.rs @@ -239,7 +239,9 @@ impl<'a> Tree<'a> { /// Parses a tree from a string #[allow(clippy::should_implement_trait)] // Cannot use std::str::FromStr because of lifetimes. pub fn from_str(s: &'a str) -> Result { - Self::from_str_inner(s).map_err(Error::ParseTree) + Self::from_str_inner(s) + .map_err(From::from) + .map_err(Error::Parse) } fn from_str_inner(s: &'a str) -> Result { @@ -387,6 +389,7 @@ where #[cfg(test)] mod tests { use super::*; + use crate::ParseError; /// Test functions to manually build trees fn leaf(name: &str) -> Tree { @@ -429,29 +432,35 @@ mod tests { assert!(matches!( Tree::from_str("thresh,").unwrap_err(), - Error::ParseTree(ParseTreeError::TrailingCharacter { ch: ',', pos: 6 }), + Error::Parse(ParseError::Tree(ParseTreeError::TrailingCharacter { ch: ',', pos: 6 })), )); assert!(matches!( Tree::from_str("thresh,thresh").unwrap_err(), - Error::ParseTree(ParseTreeError::TrailingCharacter { ch: ',', pos: 6 }), + Error::Parse(ParseError::Tree(ParseTreeError::TrailingCharacter { ch: ',', pos: 6 })), )); assert!(matches!( Tree::from_str("thresh()thresh()").unwrap_err(), - Error::ParseTree(ParseTreeError::TrailingCharacter { ch: 't', pos: 8 }), + Error::Parse(ParseError::Tree(ParseTreeError::TrailingCharacter { ch: 't', pos: 8 })), )); assert_eq!(Tree::from_str("thresh()").unwrap(), paren_node("thresh", vec![leaf("")])); assert!(matches!( Tree::from_str("thresh(a()b)"), - Err(Error::ParseTree(ParseTreeError::ExpectedParenOrComma { ch: 'b', pos: 10 })), + Err(Error::Parse(ParseError::Tree(ParseTreeError::ExpectedParenOrComma { + ch: 'b', + pos: 10 + }))), )); assert!(matches!( Tree::from_str("thresh()xyz"), - Err(Error::ParseTree(ParseTreeError::TrailingCharacter { ch: 'x', pos: 8 })), + Err(Error::Parse(ParseError::Tree(ParseTreeError::TrailingCharacter { + ch: 'x', + pos: 8 + }))), )); } @@ -459,45 +468,45 @@ mod tests { fn parse_tree_parens() { assert!(matches!( Tree::from_str("a(").unwrap_err(), - Error::ParseTree(ParseTreeError::UnmatchedOpenParen { ch: '(', pos: 1 }), + Error::Parse(ParseError::Tree(ParseTreeError::UnmatchedOpenParen { ch: '(', pos: 1 })), )); assert!(matches!( Tree::from_str(")").unwrap_err(), - Error::ParseTree(ParseTreeError::UnmatchedCloseParen { ch: ')', pos: 0 }), + Error::Parse(ParseError::Tree(ParseTreeError::UnmatchedCloseParen { ch: ')', pos: 0 })), )); assert!(matches!( Tree::from_str("x(y))").unwrap_err(), - Error::ParseTree(ParseTreeError::TrailingCharacter { ch: ')', pos: 4 }), + Error::Parse(ParseError::Tree(ParseTreeError::TrailingCharacter { ch: ')', pos: 4 })), )); /* Will be enabled in a later PR which unifies TR and non-TR parsing. assert!(matches!( Tree::from_str("a{").unwrap_err(), - Error::ParseTree(ParseTreeError::UnmatchedOpenParen { ch: '{', pos: 1 }), + Error::Parse(ParseError::Tree(ParseTreeError::UnmatchedOpenParen { ch: '{', pos: 1 })), )); assert!(matches!( Tree::from_str("}").unwrap_err(), - Error::ParseTree(ParseTreeError::UnmatchedCloseParen { ch: '}', pos: 0 }), + Error::Parse(ParseError::Tree(ParseTreeError::UnmatchedCloseParen { ch: '}', pos: 0 })), )); */ assert!(matches!( Tree::from_str("x(y)}").unwrap_err(), - Error::ParseTree(ParseTreeError::TrailingCharacter { ch: '}', pos: 4 }), + Error::Parse(ParseError::Tree(ParseTreeError::TrailingCharacter { ch: '}', pos: 4 })), )); /* Will be enabled in a later PR which unifies TR and non-TR parsing. assert!(matches!( Tree::from_str("x{y)").unwrap_err(), - Error::ParseTree(ParseTreeError::MismatchedParens { + Error::Parse(ParseError::Tree(ParseTreeError::MismatchedParens { open_ch: '{', open_pos: 1, close_ch: ')', close_pos: 3, - }), + }),) )); */ } diff --git a/src/lib.rs b/src/lib.rs index 90a92d32b..541a0c5c5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -493,8 +493,6 @@ pub enum Error { /// Invalid threshold. ParseThreshold(ParseThresholdError), /// Invalid expression tree. - ParseTree(ParseTreeError), - /// Invalid expression tree. Parse(ParseError), } @@ -557,7 +555,6 @@ impl fmt::Display for Error { Error::RelativeLockTime(ref e) => e.fmt(f), Error::Threshold(ref e) => e.fmt(f), Error::ParseThreshold(ref e) => e.fmt(f), - Error::ParseTree(ref e) => e.fmt(f), Error::Parse(ref e) => e.fmt(f), } } @@ -609,7 +606,6 @@ impl std::error::Error for Error { RelativeLockTime(e) => Some(e), Threshold(e) => Some(e), ParseThreshold(e) => Some(e), - ParseTree(e) => Some(e), Parse(e) => Some(e), } } diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs index f4fdfca5c..763df9a31 100644 --- a/src/miniscript/mod.rs +++ b/src/miniscript/mod.rs @@ -802,7 +802,9 @@ impl crate::expression::FromTree for Miniscr /// Parse an expression tree into a Miniscript. As a general rule, this /// should not be called directly; rather go through the descriptor API. fn from_tree(top: &expression::Tree) -> Result, Error> { - top.verify_no_curly_braces().map_err(Error::ParseTree)?; + top.verify_no_curly_braces() + .map_err(From::from) + .map_err(Error::Parse)?; let inner: Terminal = expression::FromTree::from_tree(top)?; Miniscript::from_ast(inner) } From 78b4d442a0087ee9b2af7c182bd54e32d8e925c6 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sun, 1 Sep 2024 14:13:44 +0000 Subject: [PATCH 8/9] error: remove 3 now-unused variants from the global Error enum This includes the stringly-typed `BadDescriptor` :). --- src/lib.rs | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 541a0c5c5..20b0e4331 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -427,10 +427,6 @@ pub enum Error { CmsTooManyKeys(u32), /// A tapscript multi_a cannot support more than Weight::MAX_BLOCK/32 keys MultiATooManyKeys(u64), - /// Encountered unprintable character in descriptor - Unprintable(u8), - /// expected character while parsing descriptor; didn't find one - ExpectedChar(char), /// While parsing backward, hit beginning of script UnexpectedStart, /// Got something we were not expecting @@ -451,8 +447,6 @@ pub enum Error { CouldNotSatisfy, /// Typechecking failed TypeCheck(String), - /// General error in creating descriptor - BadDescriptor(String), /// Forward-secp related errors Secp(bitcoin::secp256k1::Error), #[cfg(feature = "compiler")] @@ -511,8 +505,6 @@ impl fmt::Display for Error { Error::AddrError(ref e) => fmt::Display::fmt(e, f), Error::AddrP2shError(ref e) => fmt::Display::fmt(e, f), Error::CmsTooManyKeys(n) => write!(f, "checkmultisig with {} keys", n), - Error::Unprintable(x) => write!(f, "unprintable character 0x{:02x}", x), - Error::ExpectedChar(c) => write!(f, "expected {}", c), Error::UnexpectedStart => f.write_str("unexpected start of script"), Error::Unexpected(ref s) => write!(f, "unexpected «{}»", s), Error::MultiColon(ref s) => write!(f, "«{}» has multiple instances of «:»", s), @@ -523,7 +515,6 @@ impl fmt::Display for Error { Error::MissingSig(ref pk) => write!(f, "missing signature for key {:?}", pk), Error::CouldNotSatisfy => f.write_str("could not satisfy"), Error::TypeCheck(ref e) => write!(f, "typecheck: {}", e), - Error::BadDescriptor(ref e) => write!(f, "Invalid descriptor: {}", e), Error::Secp(ref e) => fmt::Display::fmt(e, f), Error::ContextError(ref e) => fmt::Display::fmt(e, f), #[cfg(feature = "compiler")] @@ -571,8 +562,6 @@ impl std::error::Error for Error { | InvalidPush(_) | CmsTooManyKeys(_) | MultiATooManyKeys(_) - | Unprintable(_) - | ExpectedChar(_) | UnexpectedStart | Unexpected(_) | MultiColon(_) @@ -583,7 +572,6 @@ impl std::error::Error for Error { | MissingSig(_) | CouldNotSatisfy | TypeCheck(_) - | BadDescriptor(_) | MaxRecursiveDepthExceeded | NonStandardBareScript | ImpossibleSatisfaction From 6a8d68749b0a2a8c25f9779071cbaf83bc05dc76 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sun, 1 Sep 2024 16:14:19 +0000 Subject: [PATCH 9/9] error: remove two more variants which were redundant with threshold errors --- src/lib.rs | 8 -------- src/miniscript/decode.rs | 15 ++++++--------- src/primitives/threshold.rs | 16 +++++++++++----- 3 files changed, 17 insertions(+), 22 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 20b0e4331..fde7ac66b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -423,10 +423,6 @@ pub enum Error { AddrError(bitcoin::address::ParseError), /// rust-bitcoin p2sh address error AddrP2shError(bitcoin::address::P2shError), - /// A `CHECKMULTISIG` opcode was preceded by a number > 20 - CmsTooManyKeys(u32), - /// A tapscript multi_a cannot support more than Weight::MAX_BLOCK/32 keys - MultiATooManyKeys(u64), /// While parsing backward, hit beginning of script UnexpectedStart, /// Got something we were not expecting @@ -504,7 +500,6 @@ impl fmt::Display for Error { Error::Script(ref e) => fmt::Display::fmt(e, f), Error::AddrError(ref e) => fmt::Display::fmt(e, f), Error::AddrP2shError(ref e) => fmt::Display::fmt(e, f), - Error::CmsTooManyKeys(n) => write!(f, "checkmultisig with {} keys", n), Error::UnexpectedStart => f.write_str("unexpected start of script"), Error::Unexpected(ref s) => write!(f, "unexpected «{}»", s), Error::MultiColon(ref s) => write!(f, "«{}» has multiple instances of «:»", s), @@ -539,7 +534,6 @@ impl fmt::Display for Error { Error::PubKeyCtxError(ref pk, ref ctx) => { write!(f, "Pubkey error: {} under {} scriptcontext", pk, ctx) } - Error::MultiATooManyKeys(k) => write!(f, "MultiA too many keys {}", k), Error::TrNoScriptCode => write!(f, "No script code for Tr descriptors"), Error::MultipathDescLenMismatch => write!(f, "At least two BIP389 key expressions in the descriptor contain tuples of derivation indexes of different lengths"), Error::AbsoluteLockTime(ref e) => e.fmt(f), @@ -560,8 +554,6 @@ impl std::error::Error for Error { InvalidOpcode(_) | NonMinimalVerify(_) | InvalidPush(_) - | CmsTooManyKeys(_) - | MultiATooManyKeys(_) | UnexpectedStart | Unexpected(_) | MultiColon(_) diff --git a/src/miniscript/decode.rs b/src/miniscript/decode.rs index 0090b7516..a8be0bc63 100644 --- a/src/miniscript/decode.rs +++ b/src/miniscript/decode.rs @@ -17,6 +17,7 @@ use crate::miniscript::lex::{Token as Tk, TokenIter}; use crate::miniscript::limits::{MAX_PUBKEYS_IN_CHECKSIGADD, MAX_PUBKEYS_PER_MULTISIG}; use crate::miniscript::ScriptContext; use crate::prelude::*; +use crate::primitives::threshold; #[cfg(doc)] use crate::Descriptor; use crate::{ @@ -538,10 +539,8 @@ pub fn parse( }, // CHECKMULTISIG based multisig Tk::CheckMultiSig, Tk::Num(n) => { - // Check size before allocating keys - if n as usize > MAX_PUBKEYS_PER_MULTISIG { - return Err(Error::CmsTooManyKeys(n)); - } + threshold::validate_k_n::(1, n as usize).map_err(Error::Threshold)?; + let mut keys = Vec::with_capacity(n as usize); for _ in 0..n { match_token!( @@ -562,11 +561,9 @@ pub fn parse( }, // MultiA Tk::NumEqual, Tk::Num(k) => { - // Check size before allocating keys - if k as usize > MAX_PUBKEYS_IN_CHECKSIGADD { - return Err(Error::MultiATooManyKeys(MAX_PUBKEYS_IN_CHECKSIGADD as u64)) - } - let mut keys = Vec::with_capacity(k as usize); // atleast k capacity + threshold::validate_k_n::(k as usize, k as usize).map_err(Error::Threshold)?; + + let mut keys = Vec::with_capacity(k as usize); // at least k capacity while tokens.peek() == Some(&Tk::CheckSigAdd) { match_token!( tokens, diff --git a/src/primitives/threshold.rs b/src/primitives/threshold.rs index 0045f9183..7c40f7ccd 100644 --- a/src/primitives/threshold.rs +++ b/src/primitives/threshold.rs @@ -40,6 +40,15 @@ impl std::error::Error for ThresholdError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { None } } +/// Check whether `k` and `n` are valid for an instance of [`Self`]. +pub fn validate_k_n(k: usize, n: usize) -> Result<(), ThresholdError> { + if k == 0 || k > n || (MAX > 0 && n > MAX) { + Err(ThresholdError { k, n, max: (MAX > 0).then_some(MAX) }) + } else { + Ok(()) + } +} + /// Structure representing a k-of-n threshold collection of some arbitrary /// object `T`. /// @@ -54,11 +63,8 @@ pub struct Threshold { impl Threshold { /// Constructs a threshold directly from a threshold value and collection. pub fn new(k: usize, inner: Vec) -> Result { - if k == 0 || k > inner.len() || (MAX > 0 && inner.len() > MAX) { - Err(ThresholdError { k, n: inner.len(), max: (MAX > 0).then_some(MAX) }) - } else { - Ok(Threshold { k, inner }) - } + validate_k_n::(k, inner.len())?; + Ok(Threshold { k, inner }) } /// Constructs a threshold from a threshold value and an iterator that yields collection