From ceadd0d4d98bf8f7bc2a80f1727769cb9e442f9a Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sun, 11 May 2025 19:29:47 +0000 Subject: [PATCH 1/5] miniscript: use byte arrays in lexer This is a little less space efficient but it gets rid of a lifetime parameter on the lexer token, which is annoying because it prevents us from putting it into error types. (Currently we convert it to a string before putting it into an error.) It also eliminates a whole bunch of panics where we convert slices to hashes and .expect() on the length being right, when we know it's right. --- src/miniscript/decode.rs | 32 +++++++++---------- src/miniscript/lex.rs | 69 ++++++++++++++++++++-------------------- 2 files changed, 51 insertions(+), 50 deletions(-) diff --git a/src/miniscript/decode.rs b/src/miniscript/decode.rs index a8be0bc63..bd704a6db 100644 --- a/src/miniscript/decode.rs +++ b/src/miniscript/decode.rs @@ -373,12 +373,12 @@ pub fn parse( tokens, // pubkey Tk::Bytes33(pk) => { - let ret = Ctx::Key::from_slice(pk) + let ret = Ctx::Key::from_slice(&pk) .map_err(|e| Error::PubKeyCtxError(e, Ctx::name_str()))?; term.reduce0(Terminal::PkK(ret))? }, Tk::Bytes65(pk) => { - let ret = Ctx::Key::from_slice(pk) + let ret = Ctx::Key::from_slice(&pk) .map_err(|e| Error::PubKeyCtxError(e, Ctx::name_str()))?; term.reduce0(Terminal::PkK(ret))? }, @@ -395,7 +395,7 @@ pub fn parse( // after bytes32 means bytes32 is in a hashlock // Finally for the first case, K being parsed as a solo expression is a Pk type Tk::Bytes32(pk) => { - let ret = Ctx::Key::from_slice(pk).map_err(|e| Error::PubKeyCtxError(e, Ctx::name_str()))?; + let ret = Ctx::Key::from_slice(&pk).map_err(|e| Error::PubKeyCtxError(e, Ctx::name_str()))?; term.reduce0(Terminal::PkK(ret))? }, // checksig @@ -414,20 +414,20 @@ pub fn parse( tokens, Tk::Dup => { term.reduce0(Terminal::RawPkH( - hash160::Hash::from_slice(hash).expect("valid size") + hash160::Hash::from_byte_array(hash) ))? }, Tk::Verify, Tk::Equal, Tk::Num(32), Tk::Size => { non_term.push(NonTerm::Verify); term.reduce0(Terminal::Hash160( - hash160::Hash::from_slice(hash).expect("valid size") + hash160::Hash::from_byte_array(hash) ))? }, ), Tk::Ripemd160, Tk::Verify, Tk::Equal, Tk::Num(32), Tk::Size => { non_term.push(NonTerm::Verify); term.reduce0(Terminal::Ripemd160( - ripemd160::Hash::from_slice(hash).expect("valid size") + ripemd160::Hash::from_byte_array(hash) ))? }, ), @@ -437,13 +437,13 @@ pub fn parse( Tk::Sha256, Tk::Verify, Tk::Equal, Tk::Num(32), Tk::Size => { non_term.push(NonTerm::Verify); term.reduce0(Terminal::Sha256( - sha256::Hash::from_slice(hash).expect("valid size") + sha256::Hash::from_byte_array(hash) ))? }, Tk::Hash256, Tk::Verify, Tk::Equal, Tk::Num(32), Tk::Size => { non_term.push(NonTerm::Verify); term.reduce0(Terminal::Hash256( - hash256::Hash::from_slice(hash).expect("valid size") + hash256::Hash::from_byte_array(hash) ))? }, ), @@ -480,14 +480,14 @@ pub fn parse( Tk::Equal, Tk::Num(32), Tk::Size => term.reduce0(Terminal::Sha256( - sha256::Hash::from_slice(hash).expect("valid size") + sha256::Hash::from_byte_array(hash) ))?, Tk::Hash256, Tk::Verify, Tk::Equal, Tk::Num(32), Tk::Size => term.reduce0(Terminal::Hash256( - hash256::Hash::from_slice(hash).expect("valid size") + hash256::Hash::from_byte_array(hash) ))?, ), Tk::Hash20(hash) => match_token!( @@ -497,14 +497,14 @@ pub fn parse( Tk::Equal, Tk::Num(32), Tk::Size => term.reduce0(Terminal::Ripemd160( - ripemd160::Hash::from_slice(hash).expect("valid size") + ripemd160::Hash::from_byte_array(hash) ))?, Tk::Hash160, Tk::Verify, Tk::Equal, Tk::Num(32), Tk::Size => term.reduce0(Terminal::Hash160( - hash160::Hash::from_slice(hash).expect("valid size") + hash160::Hash::from_byte_array(hash) ))?, ), // thresholds @@ -545,9 +545,9 @@ pub fn parse( for _ in 0..n { match_token!( tokens, - Tk::Bytes33(pk) => keys.push(::from_slice(pk) + Tk::Bytes33(pk) => keys.push(::from_slice(&pk) .map_err(|e| Error::PubKeyCtxError(e, Ctx::name_str()))?), - Tk::Bytes65(pk) => keys.push(::from_slice(pk) + Tk::Bytes65(pk) => keys.push(::from_slice(&pk) .map_err(|e| Error::PubKeyCtxError(e, Ctx::name_str()))?), ); } @@ -567,14 +567,14 @@ pub fn parse( while tokens.peek() == Some(&Tk::CheckSigAdd) { match_token!( tokens, - Tk::CheckSigAdd, Tk::Bytes32(pk) => keys.push(::from_slice(pk) + Tk::CheckSigAdd, Tk::Bytes32(pk) => keys.push(::from_slice(&pk) .map_err(|e| Error::PubKeyCtxError(e, Ctx::name_str()))?), ); } // Last key must be with a CheckSig match_token!( tokens, - Tk::CheckSig, Tk::Bytes32(pk) => keys.push(::from_slice(pk) + Tk::CheckSig, Tk::Bytes32(pk) => keys.push(::from_slice(&pk) .map_err(|e| Error::PubKeyCtxError(e, Ctx::name_str()))?), ); keys.reverse(); diff --git a/src/miniscript/lex.rs b/src/miniscript/lex.rs index 8d64fbaf5..6b8549aa5 100644 --- a/src/miniscript/lex.rs +++ b/src/miniscript/lex.rs @@ -8,6 +8,7 @@ use core::fmt; use bitcoin::blockdata::{opcodes, script}; +use bitcoin::hex::DisplayHex as _; use super::Error; use crate::prelude::*; @@ -15,7 +16,7 @@ use crate::prelude::*; /// Atom of a tokenized version of a script #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] #[allow(missing_docs)] -pub enum Token<'s> { +pub enum Token { BoolAnd, BoolOr, Add, @@ -44,22 +45,20 @@ pub enum Token<'s> { Sha256, Hash256, Num(u32), - Hash20(&'s [u8]), - Bytes32(&'s [u8]), - Bytes33(&'s [u8]), - Bytes65(&'s [u8]), + Hash20([u8; 20]), + Bytes32([u8; 32]), + Bytes33([u8; 33]), + Bytes65([u8; 65]), } -impl fmt::Display for Token<'_> { +impl fmt::Display for Token { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match *self { Token::Num(n) => write!(f, "#{}", n), - Token::Hash20(b) | Token::Bytes33(b) | Token::Bytes32(b) | Token::Bytes65(b) => { - for ch in b { - write!(f, "{:02x}", *ch)?; - } - Ok(()) - } + Token::Hash20(b) => write!(f, "{}", b.as_hex()), + Token::Bytes32(b) => write!(f, "{}", b.as_hex()), + Token::Bytes33(b) => write!(f, "{}", b.as_hex()), + Token::Bytes65(b) => write!(f, "{}", b.as_hex()), x => write!(f, "{:?}", x), } } @@ -68,18 +67,18 @@ impl fmt::Display for Token<'_> { #[derive(Debug, Clone)] /// Iterator that goes through a vector of tokens backward (our parser wants to read /// backward and this is more efficient anyway since we can use `Vec::pop()`). -pub struct TokenIter<'s>(Vec>); +pub struct TokenIter(Vec); -impl<'s> TokenIter<'s> { +impl TokenIter { /// Create a new TokenIter - pub fn new(v: Vec>) -> TokenIter<'s> { TokenIter(v) } + pub fn new(v: Vec) -> TokenIter { TokenIter(v) } /// Look at the top at Iterator - pub fn peek(&self) -> Option<&'s Token> { self.0.last() } + pub fn peek(&self) -> Option<&Token> { self.0.last() } /// Push a value to the iterator /// This will be first value consumed by popun_ - pub fn un_next(&mut self, tok: Token<'s>) { self.0.push(tok) } + pub fn un_next(&mut self, tok: Token) { self.0.push(tok) } /// The len of the iterator pub fn len(&self) -> usize { self.0.len() } @@ -88,14 +87,14 @@ impl<'s> TokenIter<'s> { pub fn is_empty(&self) -> bool { self.0.is_empty() } } -impl<'s> Iterator for TokenIter<'s> { - type Item = Token<'s>; +impl Iterator for TokenIter { + type Item = Token; - fn next(&mut self) -> Option> { self.0.pop() } + fn next(&mut self) -> Option { self.0.pop() } } /// Tokenize a script -pub fn lex(script: &'_ script::Script) -> Result>, Error> { +pub fn lex(script: &'_ script::Script) -> Result, Error> { let mut ret = Vec::with_capacity(script.len()); for ins in script.instructions_minimal() { @@ -207,20 +206,22 @@ pub fn lex(script: &'_ script::Script) -> Result>, Error> { ret.push(Token::Hash256); } script::Instruction::PushBytes(bytes) => { - match bytes.len() { - 20 => ret.push(Token::Hash20(bytes.as_bytes())), - 32 => ret.push(Token::Bytes32(bytes.as_bytes())), - 33 => ret.push(Token::Bytes33(bytes.as_bytes())), - 65 => ret.push(Token::Bytes65(bytes.as_bytes())), - _ => { - // check minimality of the number - match script::read_scriptint(bytes.as_bytes()) { - Ok(v) if v >= 0 => { - ret.push(Token::Num(v as u32)); - } - Ok(_) => return Err(Error::InvalidPush(bytes.to_owned().into())), - Err(e) => return Err(Error::Script(e)), + if let Ok(bytes) = bytes.as_bytes().try_into() { + ret.push(Token::Hash20(bytes)); + } else if let Ok(bytes) = bytes.as_bytes().try_into() { + ret.push(Token::Bytes32(bytes)); + } else if let Ok(bytes) = bytes.as_bytes().try_into() { + ret.push(Token::Bytes33(bytes)); + } else if let Ok(bytes) = bytes.as_bytes().try_into() { + ret.push(Token::Bytes65(bytes)); + } else { + // check minimality of the number + match script::read_scriptint(bytes.as_bytes()) { + Ok(v) if v >= 0 => { + ret.push(Token::Num(v as u32)); } + Ok(_) => return Err(Error::InvalidPush(bytes.to_owned().into())), + Err(e) => return Err(Error::Script(e)), } } } From 5b736bdede89b15866659c4d38383db4af4240cd Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sun, 11 May 2025 19:29:47 +0000 Subject: [PATCH 2/5] miniscript: move lexer errors into their own type Also clean up the names and format messages for them. --- src/lib.rs | 32 ++++++++---------------- src/miniscript/lex.rs | 57 ++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 63 insertions(+), 26 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 9dbdbae6d..120c0bfbe 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -131,8 +131,6 @@ mod util; use core::{fmt, hash, str}; use bitcoin::hashes::{hash160, ripemd160, sha256, Hash}; -use bitcoin::hex::DisplayHex; -use bitcoin::{script, Opcode}; pub use crate::blanket_traits::FromStrKey; pub use crate::descriptor::{DefiniteDescriptorKey, Descriptor, DescriptorPublicKey}; @@ -436,15 +434,8 @@ pub trait ForEachKey { #[derive(Debug)] pub enum Error { - /// Opcode appeared which is not part of the script subset - InvalidOpcode(Opcode), - /// Some opcode occurred followed by `OP_VERIFY` when it had - /// a `VERIFY` version that should have been used instead - NonMinimalVerify(String), - /// Push was illegal in some context - InvalidPush(Vec), - /// rust-bitcoin script error - Script(script::Error), + /// Error when lexing a bitcoin Script. + ScriptLexer(crate::miniscript::lex::Error), /// rust-bitcoin address error AddrError(bitcoin::address::ParseError), /// rust-bitcoin p2sh address error @@ -519,12 +510,7 @@ const MAX_RECURSION_DEPTH: u32 = 402; impl fmt::Display for Error { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match *self { - Error::InvalidOpcode(op) => write!(f, "invalid opcode {}", op), - Error::NonMinimalVerify(ref tok) => write!(f, "{} VERIFY", tok), - Error::InvalidPush(ref push) => { - write!(f, "invalid push {:x}", push.as_hex()) - }, - Error::Script(ref e) => fmt::Display::fmt(e, f), + Error::ScriptLexer(ref e) => e.fmt(f), Error::AddrError(ref e) => fmt::Display::fmt(e, f), Error::AddrP2shError(ref e) => fmt::Display::fmt(e, f), Error::UnexpectedStart => f.write_str("unexpected start of script"), @@ -576,10 +562,7 @@ impl std::error::Error for Error { use self::Error::*; match self { - InvalidOpcode(_) - | NonMinimalVerify(_) - | InvalidPush(_) - | UnexpectedStart + UnexpectedStart | Unexpected(_) | UnknownWrapper(_) | NonTopLevel(_) @@ -593,7 +576,7 @@ impl std::error::Error for Error { | BareDescriptorAddr | TrNoScriptCode | MultipathDescLenMismatch => None, - Script(e) => Some(e), + ScriptLexer(e) => Some(e), AddrError(e) => Some(e), AddrP2shError(e) => Some(e), Secp(e) => Some(e), @@ -614,6 +597,11 @@ impl std::error::Error for Error { } } +#[doc(hidden)] +impl From for Error { + fn from(e: miniscript::lex::Error) -> Error { Error::ScriptLexer(e) } +} + #[doc(hidden)] impl From for Error { fn from(e: miniscript::types::Error) -> Error { Error::TypeCheck(e.to_string()) } diff --git a/src/miniscript/lex.rs b/src/miniscript/lex.rs index 6b8549aa5..040840908 100644 --- a/src/miniscript/lex.rs +++ b/src/miniscript/lex.rs @@ -10,7 +10,6 @@ use core::fmt; use bitcoin::blockdata::{opcodes, script}; use bitcoin::hex::DisplayHex as _; -use super::Error; use crate::prelude::*; /// Atom of a tokenized version of a script @@ -187,7 +186,7 @@ pub fn lex(script: &'_ script::Script) -> Result, Error> { Some(op @ &Token::Equal) | Some(op @ &Token::CheckSig) | Some(op @ &Token::CheckMultiSig) => { - return Err(Error::NonMinimalVerify(format!("{:?}", op))) + return Err(Error::NonMinimalVerify(*op)); } _ => {} } @@ -220,8 +219,8 @@ pub fn lex(script: &'_ script::Script) -> Result, Error> { Ok(v) if v >= 0 => { ret.push(Token::Num(v as u32)); } - Ok(_) => return Err(Error::InvalidPush(bytes.to_owned().into())), - Err(e) => return Err(Error::Script(e)), + Ok(n) => return Err(Error::NegativeInt { bytes: bytes.to_owned(), n }), + Err(err) => return Err(Error::InvalidInt { bytes: bytes.to_owned(), err }), } } } @@ -281,3 +280,53 @@ pub fn lex(script: &'_ script::Script) -> Result, Error> { } Ok(ret) } + +/// Lexer error. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Error { + /// Parsed a negative number. + InvalidInt { + /// The bytes of the push that were attempted to be parsed. + bytes: bitcoin::script::PushBytesBuf, + /// The error that occured. + err: bitcoin::script::Error, + }, + /// Parsed an opcode outside of the Miniscript language. + InvalidOpcode(bitcoin::Opcode), + /// Parsed a negative number. + NegativeInt { + /// The bytes of the push that were parsed to a negative number. + bytes: bitcoin::script::PushBytesBuf, + /// The resulting number. + n: i64, + }, + /// Non-minimal verify (e.g. `CHECKSIG VERIFY` in place of `CHECKSIGVERIFY`). + NonMinimalVerify(Token), + /// Error iterating through script. + Script(bitcoin::script::Error), +} + +impl fmt::Display for Error { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match *self { + Self::Script(ref e) => e.fmt(f), + Self::InvalidInt { ref bytes, ref err } => write!(f, "push {} of length {} is not a key, hash or minimal integer: {}", bytes.as_bytes().as_hex(), bytes.len(), err), + Self::InvalidOpcode(ref op) => write!(f, "found opcode {} which does not occur in Miniscript", op), + Self::NegativeInt { ref bytes, n } => write!(f, "push {} of length {} parses as a negative number {} which does not occur in Miniscript", bytes.as_bytes().as_hex(), bytes.len(), n), + Self::NonMinimalVerify(ref op) => write!(f, "found {} VERIFY (should be one opcode, {}VERIFY)", op, op), + } + } +} + +#[cfg(feature = "std")] +impl std::error::Error for Error { + fn cause(&self) -> Option<&(dyn std::error::Error + 'static)> { + match *self { + Self::InvalidInt { ref err, .. } => Some(err), + Self::InvalidOpcode(..) => None, + Self::NegativeInt { .. } => None, + Self::NonMinimalVerify(..) => None, + Self::Script(ref e) => Some(e), + } + } +} From a57fab3ca26a20140a90b8ca66408c9246fdf9cd Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sun, 11 May 2025 21:05:27 +0000 Subject: [PATCH 3/5] miniscript: replace many calls to reduce0 in decode with infallible constructors We have infallible constructors for all the terminals except multi/multia. We should use them and eliminate a bunch of error paths. --- src/miniscript/decode.rs | 59 +++++++++++++++++++++------------------- 1 file changed, 31 insertions(+), 28 deletions(-) diff --git a/src/miniscript/decode.rs b/src/miniscript/decode.rs index bd704a6db..014907e51 100644 --- a/src/miniscript/decode.rs +++ b/src/miniscript/decode.rs @@ -320,9 +320,12 @@ macro_rules! match_token { struct TerminalStack(Vec>); impl TerminalStack { - ///Wrapper around self.0.pop() + /// Wrapper around self.0.pop() fn pop(&mut self) -> Option> { self.0.pop() } + /// Wrapper around self.0.push() + fn push(&mut self, ms: Miniscript) { self.0.push(ms) } + ///reduce, type check and push a 0-arg node fn reduce0(&mut self, ms: Terminal) -> Result<(), Error> { let ms = Miniscript::from_ast(ms)?; @@ -375,12 +378,12 @@ pub fn parse( Tk::Bytes33(pk) => { let ret = Ctx::Key::from_slice(&pk) .map_err(|e| Error::PubKeyCtxError(e, Ctx::name_str()))?; - term.reduce0(Terminal::PkK(ret))? + term.push(Miniscript::pk_k(ret)); }, Tk::Bytes65(pk) => { let ret = Ctx::Key::from_slice(&pk) .map_err(|e| Error::PubKeyCtxError(e, Ctx::name_str()))?; - term.reduce0(Terminal::PkK(ret))? + term.push(Miniscript::pk_k(ret)); }, // Note this does not collide with hash32 because they always followed by equal // and would be parsed in different branch. If we get a naked Bytes32, it must be @@ -396,7 +399,7 @@ pub fn parse( // Finally for the first case, K being parsed as a solo expression is a Pk type Tk::Bytes32(pk) => { let ret = Ctx::Key::from_slice(&pk).map_err(|e| Error::PubKeyCtxError(e, Ctx::name_str()))?; - term.reduce0(Terminal::PkK(ret))? + term.push(Miniscript::pk_k(ret)); }, // checksig Tk::CheckSig => { @@ -413,22 +416,22 @@ pub fn parse( Tk::Hash160 => match_token!( tokens, Tk::Dup => { - term.reduce0(Terminal::RawPkH( + term.push(Miniscript::expr_raw_pkh( hash160::Hash::from_byte_array(hash) - ))? + )); }, Tk::Verify, Tk::Equal, Tk::Num(32), Tk::Size => { non_term.push(NonTerm::Verify); - term.reduce0(Terminal::Hash160( + term.push(Miniscript::hash160( hash160::Hash::from_byte_array(hash) - ))? + )); }, ), Tk::Ripemd160, Tk::Verify, Tk::Equal, Tk::Num(32), Tk::Size => { non_term.push(NonTerm::Verify); - term.reduce0(Terminal::Ripemd160( + term.push(Miniscript::ripemd160( ripemd160::Hash::from_byte_array(hash) - ))? + )); }, ), // Tk::Hash20(hash), @@ -436,15 +439,15 @@ pub fn parse( tokens, Tk::Sha256, Tk::Verify, Tk::Equal, Tk::Num(32), Tk::Size => { non_term.push(NonTerm::Verify); - term.reduce0(Terminal::Sha256( + term.push(Miniscript::sha256( sha256::Hash::from_byte_array(hash) - ))? + )); }, Tk::Hash256, Tk::Verify, Tk::Equal, Tk::Num(32), Tk::Size => { non_term.push(NonTerm::Verify); - term.reduce0(Terminal::Hash256( + term.push(Miniscript::hash256( hash256::Hash::from_byte_array(hash) - ))? + )); }, ), Tk::Num(k) => { @@ -467,9 +470,9 @@ pub fn parse( }, // timelocks Tk::CheckSequenceVerify, Tk::Num(n) - => term.reduce0(Terminal::Older(RelLockTime::from_consensus(n).map_err(Error::RelativeLockTime)?))?, + => term.push(Miniscript::older(RelLockTime::from_consensus(n).map_err(Error::RelativeLockTime)?)), Tk::CheckLockTimeVerify, Tk::Num(n) - => term.reduce0(Terminal::After(AbsLockTime::from_consensus(n).map_err(Error::AbsoluteLockTime)?))?, + => term.push(Miniscript::after(AbsLockTime::from_consensus(n).map_err(Error::AbsoluteLockTime)?)), // hashlocks Tk::Equal => match_token!( tokens, @@ -479,16 +482,16 @@ pub fn parse( Tk::Verify, Tk::Equal, Tk::Num(32), - Tk::Size => term.reduce0(Terminal::Sha256( + Tk::Size => term.push(Miniscript::sha256( sha256::Hash::from_byte_array(hash) - ))?, + )), Tk::Hash256, Tk::Verify, Tk::Equal, Tk::Num(32), - Tk::Size => term.reduce0(Terminal::Hash256( + Tk::Size => term.push(Miniscript::hash256( hash256::Hash::from_byte_array(hash) - ))?, + )), ), Tk::Hash20(hash) => match_token!( tokens, @@ -496,16 +499,16 @@ pub fn parse( Tk::Verify, Tk::Equal, Tk::Num(32), - Tk::Size => term.reduce0(Terminal::Ripemd160( + Tk::Size => term.push(Miniscript::ripemd160( ripemd160::Hash::from_byte_array(hash) - ))?, + )), Tk::Hash160, Tk::Verify, Tk::Equal, Tk::Num(32), - Tk::Size => term.reduce0(Terminal::Hash160( + Tk::Size => term.push(Miniscript::hash160( hash160::Hash::from_byte_array(hash) - ))?, + )), ), // thresholds Tk::Num(k) => { @@ -519,8 +522,8 @@ pub fn parse( }, ), // most other fragments - Tk::Num(0) => term.reduce0(Terminal::False)?, - Tk::Num(1) => term.reduce0(Terminal::True)?, + Tk::Num(0) => term.push(Miniscript::FALSE), + Tk::Num(1) => term.push(Miniscript::TRUE), Tk::EndIf => { non_term.push(NonTerm::EndIf); non_term.push(NonTerm::MaybeAndV); @@ -557,7 +560,7 @@ pub fn parse( ); keys.reverse(); let thresh = Threshold::new(k as usize, keys).map_err(Error::Threshold)?; - term.reduce0(Terminal::Multi(thresh))?; + term.push(Miniscript::multi(thresh)); }, // MultiA Tk::NumEqual, Tk::Num(k) => { @@ -579,7 +582,7 @@ pub fn parse( ); keys.reverse(); let thresh = Threshold::new(k as usize, keys).map_err(Error::Threshold)?; - term.reduce0(Terminal::MultiA(thresh))?; + term.push(Miniscript::multi_a(thresh)); }, ); } From 5fea2b3cee70f02b18f1d6fbba7b777ab329e7b1 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sun, 11 May 2025 20:32:23 +0000 Subject: [PATCH 4/5] miniscript: rename and cleanup decode::KeyParseError The word "parse" is redundant at best, and wrong at worst (we use "decode" when converting a script and "parse" when converting a string). The variants were also overly verbose and the formatting text lost information. Clean all this up. --- src/lib.rs | 2 +- src/miniscript/decode.rs | 67 ++++++++++++++++++++-------------------- 2 files changed, 34 insertions(+), 35 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 120c0bfbe..84e1b5844 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -481,7 +481,7 @@ pub enum Error { /// Bare descriptors don't have any addresses BareDescriptorAddr, /// PubKey invalid under current context - PubKeyCtxError(miniscript::decode::KeyParseError, &'static str), + PubKeyCtxError(miniscript::decode::KeyError, &'static str), /// No script code for Tr descriptors TrNoScriptCode, /// At least two BIP389 key expressions in the descriptor contain tuples of diff --git a/src/miniscript/decode.rs b/src/miniscript/decode.rs index 014907e51..4bfa56b68 100644 --- a/src/miniscript/decode.rs +++ b/src/miniscript/decode.rs @@ -27,47 +27,18 @@ use crate::{ /// Trait for parsing keys from byte slices pub trait ParseableKey: Sized + ToPublicKey + private::Sealed { /// Parse a key from slice - fn from_slice(sl: &[u8]) -> Result; + fn from_slice(sl: &[u8]) -> Result; } impl ParseableKey for bitcoin::PublicKey { - fn from_slice(sl: &[u8]) -> Result { - bitcoin::PublicKey::from_slice(sl).map_err(KeyParseError::FullKeyParseError) + fn from_slice(sl: &[u8]) -> Result { + bitcoin::PublicKey::from_slice(sl).map_err(KeyError::Full) } } impl ParseableKey for bitcoin::secp256k1::XOnlyPublicKey { - fn from_slice(sl: &[u8]) -> Result { - bitcoin::secp256k1::XOnlyPublicKey::from_slice(sl) - .map_err(KeyParseError::XonlyKeyParseError) - } -} - -/// Decoding error while parsing keys -#[derive(Debug, Clone, PartialEq, Eq)] -pub enum KeyParseError { - /// Bitcoin PublicKey parse error - FullKeyParseError(bitcoin::key::FromSliceError), - /// Xonly key parse Error - XonlyKeyParseError(bitcoin::secp256k1::Error), -} - -impl fmt::Display for KeyParseError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - KeyParseError::FullKeyParseError(_e) => write!(f, "FullKey Parse Error"), - KeyParseError::XonlyKeyParseError(_e) => write!(f, "XonlyKey Parse Error"), - } - } -} - -#[cfg(feature = "std")] -impl error::Error for KeyParseError { - fn cause(&self) -> Option<&(dyn error::Error + 'static)> { - match self { - KeyParseError::FullKeyParseError(e) => Some(e), - KeyParseError::XonlyKeyParseError(e) => Some(e), - } + fn from_slice(sl: &[u8]) -> Result { + bitcoin::secp256k1::XOnlyPublicKey::from_slice(sl).map_err(KeyError::XOnly) } } @@ -727,3 +698,31 @@ fn is_and_v(tokens: &mut TokenIter) -> bool { | Some(&Tk::Swap) ) } + +/// Decoding error while parsing keys +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum KeyError { + /// Bitcoin PublicKey parse error + Full(bitcoin::key::FromSliceError), + /// Xonly key parse Error + XOnly(bitcoin::secp256k1::Error), +} + +impl fmt::Display for KeyError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Full(e) => e.fmt(f), + Self::XOnly(e) => e.fmt(f), + } + } +} + +#[cfg(feature = "std")] +impl error::Error for KeyError { + fn cause(&self) -> Option<&(dyn error::Error + 'static)> { + match self { + Self::Full(e) => Some(e), + Self::XOnly(e) => Some(e), + } + } +} From cdb36c19b536ef8d8716dd1423e7e6b96433101e Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sun, 18 May 2025 12:38:56 +0000 Subject: [PATCH 5/5] miniscript: rename parse_* functions to decode_* We should backport this one to 12.x (with a deprecation). Would also be open to doing a deprecation here in master. But this has been bugging me for years, and also it's much less common to decode miniscript from script than we imagined back in 2018. Provides symmetry with encode() and consistency with the docs (though this PR does not fix up the docs). --- .../roundtrip_miniscript_script.rs | 2 +- .../roundtrip_miniscript_script_tap.rs | 2 +- src/interpreter/inner.rs | 4 +- src/miniscript/analyzable.rs | 2 +- src/miniscript/decode.rs | 2 +- src/miniscript/mod.rs | 78 +++++++++---------- src/psbt/finalizer.rs | 10 +-- 7 files changed, 50 insertions(+), 50 deletions(-) diff --git a/fuzz/fuzz_targets/roundtrip_miniscript_script.rs b/fuzz/fuzz_targets/roundtrip_miniscript_script.rs index cd44c455c..5e5fa4225 100644 --- a/fuzz/fuzz_targets/roundtrip_miniscript_script.rs +++ b/fuzz/fuzz_targets/roundtrip_miniscript_script.rs @@ -8,7 +8,7 @@ fn do_test(data: &[u8]) { // Try round-tripping as a script let script = script::Script::from_bytes(data); - if let Ok(pt) = Miniscript::::parse(script) { + if let Ok(pt) = Miniscript::::decode(script) { let output = pt.encode(); assert_eq!(pt.script_size(), output.len()); assert_eq!(&output, script); diff --git a/fuzz/fuzz_targets/roundtrip_miniscript_script_tap.rs b/fuzz/fuzz_targets/roundtrip_miniscript_script_tap.rs index c372547bc..7dd16fc9c 100644 --- a/fuzz/fuzz_targets/roundtrip_miniscript_script_tap.rs +++ b/fuzz/fuzz_targets/roundtrip_miniscript_script_tap.rs @@ -8,7 +8,7 @@ fn do_test(data: &[u8]) { // Try round-tripping as a script let script = script::Script::from_bytes(data); - if let Ok(pt) = Miniscript::::parse(script) { + if let Ok(pt) = Miniscript::::decode(script) { let output = pt.encode(); assert_eq!(pt.script_size(), output.len()); assert_eq!(&output, script); diff --git a/src/interpreter/inner.rs b/src/interpreter/inner.rs index f3b7ea099..4cfab3ab1 100644 --- a/src/interpreter/inner.rs +++ b/src/interpreter/inner.rs @@ -43,7 +43,7 @@ fn script_from_stack_elem( ) -> Result, Error> { match *elem { stack::Element::Push(sl) => { - Miniscript::parse_with_ext(bitcoin::Script::from_bytes(sl), &ExtParams::allow_all()) + Miniscript::decode_with_ext(bitcoin::Script::from_bytes(sl), &ExtParams::allow_all()) .map_err(Error::from) } stack::Element::Satisfied => Ok(Miniscript::TRUE), @@ -327,7 +327,7 @@ pub(super) fn from_txdata<'txin>( } else { if wit_stack.is_empty() { // Bare script parsed in BareCtx - let miniscript = Miniscript::::parse_with_ext( + let miniscript = Miniscript::::decode_with_ext( spk, &ExtParams::allow_all(), )?; diff --git a/src/miniscript/analyzable.rs b/src/miniscript/analyzable.rs index 2e2aa9326..13ccb5e65 100644 --- a/src/miniscript/analyzable.rs +++ b/src/miniscript/analyzable.rs @@ -13,7 +13,7 @@ use crate::prelude::*; use crate::{Miniscript, MiniscriptKey, ScriptContext, Terminal}; /// Params for parsing miniscripts that either non-sane or non-specified(experimental) in the spec. -/// Used as a parameter [`Miniscript::from_str_ext`] and [`Miniscript::parse_with_ext`]. +/// Used as a parameter [`Miniscript::from_str_ext`] and [`Miniscript::decode_with_ext`]. /// /// This allows parsing miniscripts if /// 1. It is unsafe(does not require a digital signature to spend it) diff --git a/src/miniscript/decode.rs b/src/miniscript/decode.rs index 4bfa56b68..213ac8266 100644 --- a/src/miniscript/decode.rs +++ b/src/miniscript/decode.rs @@ -331,7 +331,7 @@ impl TerminalStack { /// Parse a script fragment into an `Miniscript` #[allow(unreachable_patterns)] -pub fn parse( +pub fn decode( tokens: &mut TokenIter, ) -> Result, Error> { let mut non_term = Vec::with_capacity(tokens.len()); diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs index 9dc964fa5..1eb180b3e 100644 --- a/src/miniscript/mod.rs +++ b/src/miniscript/mod.rs @@ -529,8 +529,8 @@ impl Miniscript { /// Some of the analysis guarantees of miniscript are lost when dealing with /// insane scripts. In general, in a multi-party setting users should only /// accept sane scripts. - pub fn parse_insane(script: &script::Script) -> Result, Error> { - Miniscript::parse_with_ext(script, &ExtParams::insane()) + pub fn decode_insane(script: &script::Script) -> Result, Error> { + Miniscript::decode_with_ext(script, &ExtParams::insane()) } /// Attempt to parse an miniscript with extra features that not yet specified in the spec. @@ -540,14 +540,14 @@ impl Miniscript { /// - Parsing miniscripts with raw pubkey hashes /// /// Allowed extra features can be specified by the ext [`ExtParams`] argument. - pub fn parse_with_ext( + pub fn decode_with_ext( script: &script::Script, ext: &ExtParams, ) -> Result, Error> { let tokens = lex(script)?; let mut iter = TokenIter::new(tokens); - let top = decode::parse(&mut iter)?; + let top = decode::decode(&mut iter)?; Ctx::check_global_validity(&top)?; let type_check = types::Type::type_check(&top.node)?; if type_check.corr.base != types::Base::B { @@ -564,7 +564,7 @@ impl Miniscript { /// Attempt to parse a Script into Miniscript representation. /// /// This function will fail parsing for scripts that do not clear the - /// [`Miniscript::sanity_check`] checks. Use [`Miniscript::parse_insane`] to + /// [`Miniscript::sanity_check`] checks. Use [`Miniscript::decode_insane`] to /// parse such scripts. /// /// ## Decode/Parse a miniscript from script hex @@ -578,24 +578,24 @@ impl Miniscript { /// type TapScript = Miniscript; /// /// // parse x-only miniscript in Taproot context - /// let tapscript_ms = TapScript::parse(&bitcoin::ScriptBuf::from_hex( + /// let tapscript_ms = TapScript::decode(&bitcoin::ScriptBuf::from_hex( /// "202788ee41e76f4f3af603da5bc8fa22997bc0344bb0f95666ba6aaff0242baa99ac", /// ).expect("Even length hex")) /// .expect("Xonly keys are valid only in taproot context"); /// // tapscript fails decoding when we use them with compressed keys - /// let err = TapScript::parse(&bitcoin::ScriptBuf::from_hex( + /// let err = TapScript::decode(&bitcoin::ScriptBuf::from_hex( /// "21022788ee41e76f4f3af603da5bc8fa22997bc0344bb0f95666ba6aaff0242baa99ac", /// ).expect("Even length hex")) /// .expect_err("Compressed keys cannot be used in Taproot context"); /// // Segwitv0 succeeds decoding with full keys. - /// Segwitv0Script::parse(&bitcoin::ScriptBuf::from_hex( + /// Segwitv0Script::decode(&bitcoin::ScriptBuf::from_hex( /// "21022788ee41e76f4f3af603da5bc8fa22997bc0344bb0f95666ba6aaff0242baa99ac", /// ).expect("Even length hex")) /// .expect("Compressed keys are allowed in Segwit context"); /// /// ``` - pub fn parse(script: &script::Script) -> Result, Error> { - let ms = Self::parse_with_ext(script, &ExtParams::sane())?; + pub fn decode(script: &script::Script) -> Result, Error> { + let ms = Self::decode_with_ext(script, &ExtParams::sane())?; Ok(ms) } } @@ -1165,7 +1165,7 @@ mod tests { assert_eq!(format!("{:x}", bitcoin_script), expected); } // Parse scripts with all extensions - let roundtrip = Segwitv0Script::parse_with_ext(&bitcoin_script, &ExtParams::allow_all()) + let roundtrip = Segwitv0Script::decode_with_ext(&bitcoin_script, &ExtParams::allow_all()) .expect("parse string serialization"); assert_eq!(roundtrip, script); } @@ -1175,7 +1175,7 @@ mod tests { let ser = tree.encode(); assert_eq!(ser.len(), tree.script_size()); assert_eq!(ser.to_string(), s); - let deser = Segwitv0Script::parse_insane(&ser).expect("deserialize result of serialize"); + let deser = Segwitv0Script::decode_insane(&ser).expect("deserialize result of serialize"); assert_eq!(*tree, deser); } @@ -1316,19 +1316,19 @@ mod tests { fn verify_parse() { let ms = "and_v(v:hash160(20195b5a3d650c17f0f29f91c33f8f6335193d07),or_d(sha256(96de8fc8c256fa1e1556d41af431cace7dca68707c78dd88c3acab8b17164c47),older(16)))"; let ms: Segwitv0Script = Miniscript::from_str_insane(ms).unwrap(); - assert_eq!(ms, Segwitv0Script::parse_insane(&ms.encode()).unwrap()); + assert_eq!(ms, Segwitv0Script::decode_insane(&ms.encode()).unwrap()); let ms = "and_v(v:sha256(96de8fc8c256fa1e1556d41af431cace7dca68707c78dd88c3acab8b17164c47),or_d(sha256(96de8fc8c256fa1e1556d41af431cace7dca68707c78dd88c3acab8b17164c47),older(16)))"; let ms: Segwitv0Script = Miniscript::from_str_insane(ms).unwrap(); - assert_eq!(ms, Segwitv0Script::parse_insane(&ms.encode()).unwrap()); + assert_eq!(ms, Segwitv0Script::decode_insane(&ms.encode()).unwrap()); let ms = "and_v(v:ripemd160(20195b5a3d650c17f0f29f91c33f8f6335193d07),or_d(sha256(96de8fc8c256fa1e1556d41af431cace7dca68707c78dd88c3acab8b17164c47),older(16)))"; let ms: Segwitv0Script = Miniscript::from_str_insane(ms).unwrap(); - assert_eq!(ms, Segwitv0Script::parse_insane(&ms.encode()).unwrap()); + assert_eq!(ms, Segwitv0Script::decode_insane(&ms.encode()).unwrap()); let ms = "and_v(v:hash256(96de8fc8c256fa1e1556d41af431cace7dca68707c78dd88c3acab8b17164c47),or_d(sha256(96de8fc8c256fa1e1556d41af431cace7dca68707c78dd88c3acab8b17164c47),older(16)))"; let ms: Segwitv0Script = Miniscript::from_str_insane(ms).unwrap(); - assert_eq!(ms, Segwitv0Script::parse_insane(&ms.encode()).unwrap()); + assert_eq!(ms, Segwitv0Script::decode_insane(&ms.encode()).unwrap()); } #[test] @@ -1519,21 +1519,21 @@ mod tests { #[test] fn deserialize() { // Most of these came from fuzzing, hence the increasing lengths - assert!(Segwitv0Script::parse_insane(&hex_script("")).is_err()); // empty - assert!(Segwitv0Script::parse_insane(&hex_script("00")).is_ok()); // FALSE - assert!(Segwitv0Script::parse_insane(&hex_script("51")).is_ok()); // TRUE - assert!(Segwitv0Script::parse_insane(&hex_script("69")).is_err()); // VERIFY - assert!(Segwitv0Script::parse_insane(&hex_script("0000")).is_err()); //and_v(FALSE,FALSE) - assert!(Segwitv0Script::parse_insane(&hex_script("1001")).is_err()); // incomplete push - assert!(Segwitv0Script::parse_insane(&hex_script("03990300b2")).is_err()); // non-minimal # - assert!(Segwitv0Script::parse_insane(&hex_script("8559b2")).is_err()); // leading bytes - assert!(Segwitv0Script::parse_insane(&hex_script("4c0169b2")).is_err()); // non-minimal push - assert!(Segwitv0Script::parse_insane(&hex_script("0000af0000ae85")).is_err()); // OR not BOOLOR + assert!(Segwitv0Script::decode_insane(&hex_script("")).is_err()); // empty + assert!(Segwitv0Script::decode_insane(&hex_script("00")).is_ok()); // FALSE + assert!(Segwitv0Script::decode_insane(&hex_script("51")).is_ok()); // TRUE + assert!(Segwitv0Script::decode_insane(&hex_script("69")).is_err()); // VERIFY + assert!(Segwitv0Script::decode_insane(&hex_script("0000")).is_err()); //and_v(FALSE,FALSE) + assert!(Segwitv0Script::decode_insane(&hex_script("1001")).is_err()); // incomplete push + assert!(Segwitv0Script::decode_insane(&hex_script("03990300b2")).is_err()); // non-minimal # + assert!(Segwitv0Script::decode_insane(&hex_script("8559b2")).is_err()); // leading bytes + assert!(Segwitv0Script::decode_insane(&hex_script("4c0169b2")).is_err()); // non-minimal push + assert!(Segwitv0Script::decode_insane(&hex_script("0000af0000ae85")).is_err()); // OR not BOOLOR // misc fuzzer problems - assert!(Segwitv0Script::parse_insane(&hex_script("0000000000af")).is_err()); - assert!(Segwitv0Script::parse_insane(&hex_script("04009a2970af00")).is_err()); // giant CMS key num - assert!(Segwitv0Script::parse_insane(&hex_script( + assert!(Segwitv0Script::decode_insane(&hex_script("0000000000af")).is_err()); + assert!(Segwitv0Script::decode_insane(&hex_script("04009a2970af00")).is_err()); // giant CMS key num + assert!(Segwitv0Script::decode_insane(&hex_script( "2102ffffffffffffffefefefefefefefefefefef394c0fe5b711179e124008584753ac6900" )) .is_err()); @@ -1573,22 +1573,22 @@ mod tests { //---------------- test script <-> miniscript --------------- // Test parsing from scripts: x-only fails decoding in segwitv0 ctx - Segwitv0Script::parse_insane(&hex_script( + Segwitv0Script::decode_insane(&hex_script( "202788ee41e76f4f3af603da5bc8fa22997bc0344bb0f95666ba6aaff0242baa99ac", )) .unwrap_err(); // x-only succeeds in tap ctx - Tapscript::parse_insane(&hex_script( + Tapscript::decode_insane(&hex_script( "202788ee41e76f4f3af603da5bc8fa22997bc0344bb0f95666ba6aaff0242baa99ac", )) .unwrap(); // tapscript fails decoding with compressed - Tapscript::parse_insane(&hex_script( + Tapscript::decode_insane(&hex_script( "21022788ee41e76f4f3af603da5bc8fa22997bc0344bb0f95666ba6aaff0242baa99ac", )) .unwrap_err(); // Segwitv0 succeeds decoding with tapscript. - Segwitv0Script::parse_insane(&hex_script( + Segwitv0Script::decode_insane(&hex_script( "21022788ee41e76f4f3af603da5bc8fa22997bc0344bb0f95666ba6aaff0242baa99ac", )) .unwrap(); @@ -1624,7 +1624,7 @@ mod tests { .unwrap(); // script rtt test assert_eq!( - Miniscript::::parse_insane(&tap_ms.encode()).unwrap(), + Miniscript::::decode_insane(&tap_ms.encode()).unwrap(), tap_ms ); assert_eq!(tap_ms.script_size(), 104); @@ -1665,7 +1665,7 @@ mod tests { .unwrap(); let ms_trans = ms.translate_pk(&mut StrKeyTranslator::new()).unwrap(); let enc = ms_trans.encode(); - let ms = Miniscript::::parse_insane(&enc).unwrap(); + let ms = Miniscript::::decode_insane(&enc).unwrap(); assert_eq!(ms_trans.encode(), ms.encode()); } @@ -1687,9 +1687,9 @@ mod tests { let script = ms.encode(); // The same test, but parsing from script - SegwitMs::parse(&script).unwrap_err(); - SegwitMs::parse_insane(&script).unwrap_err(); - SegwitMs::parse_with_ext(&script, &ExtParams::allow_all()).unwrap(); + SegwitMs::decode(&script).unwrap_err(); + SegwitMs::decode_insane(&script).unwrap_err(); + SegwitMs::decode_with_ext(&script, &ExtParams::allow_all()).unwrap(); // Try replacing the raw_pkh with a pkh let mut map = BTreeMap::new(); @@ -1881,7 +1881,7 @@ mod tests { for _ in 0..10000 { script = script.push_opcode(bitcoin::opcodes::all::OP_0NOTEQUAL); } - Tapscript::parse_insane(&script.into_script()).unwrap_err(); + Tapscript::decode_insane(&script.into_script()).unwrap_err(); } #[test] diff --git a/src/psbt/finalizer.rs b/src/psbt/finalizer.rs index 2bfb24a88..6e0ff973d 100644 --- a/src/psbt/finalizer.rs +++ b/src/psbt/finalizer.rs @@ -71,7 +71,7 @@ fn construct_tap_witness( // We don't know how to satisfy non default version scripts yet continue; } - let ms = match Miniscript::::parse_with_ext( + let ms = match Miniscript::::decode_with_ext( script, &ExtParams::allow_all(), ) { @@ -213,7 +213,7 @@ fn get_descriptor(psbt: &Psbt, index: usize) -> Result, In p2wsh_expected: script_pubkey.clone(), }); } - let ms = Miniscript::::parse_with_ext( + let ms = Miniscript::::decode_with_ext( witness_script, &ExtParams::allow_all(), )?; @@ -240,7 +240,7 @@ fn get_descriptor(psbt: &Psbt, index: usize) -> Result, In p2wsh_expected: redeem_script.clone(), }); } - let ms = Miniscript::::parse_with_ext( + let ms = Miniscript::::decode_with_ext( witness_script, &ExtParams::allow_all(), )?; @@ -272,7 +272,7 @@ fn get_descriptor(psbt: &Psbt, index: usize) -> Result, In return Err(InputError::NonEmptyWitnessScript); } if let Some(ref redeem_script) = inp.redeem_script { - let ms = Miniscript::::parse_with_ext( + let ms = Miniscript::::decode_with_ext( redeem_script, &ExtParams::allow_all(), )?; @@ -291,7 +291,7 @@ fn get_descriptor(psbt: &Psbt, index: usize) -> Result, In if inp.redeem_script.is_some() { return Err(InputError::NonEmptyRedeemScript); } - let ms = Miniscript::::parse_with_ext( + let ms = Miniscript::::decode_with_ext( &script_pubkey, &ExtParams::allow_all(), )?;