From 27d19b7283b28213501ac2371a9caf93bb093981 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Mon, 19 May 2025 22:25:33 +0000 Subject: [PATCH 1/6] policy: replace LiftError and PolicyError with ValidationError We have two error types in the policy module, LiftError and PolicyError. These both cover "duplicate pubkeys" and "mixed timelocks". (Before the previous commit, PolicyError also covered non-binary ands and ors.) These both can now be covered by ValidationError, which reduces the total number of error types in the library and also gets rid of two variants in the top-level Error enum, which I am slowly trying to eliminate. However, there is some changed behavior: 1. When parsing concrete policies we now check for duplicate pubkeys. Previously we only checked for mixed timelocks, and checked for duplicate pubkeys at compile time. I could've gone either way in resolving this inconsistency and chose to go in the direction of "enforce sanity when parsing". 2. When *compiling* concrete policies we don't do validation checks anymore. The premise is that you shouldn't be able to construct an invalid policy, so there's no need to check again before compiling. (Of course, with the existing public `enum` type you can directly make bad policies; we will close this loophole later.) A couple unit tests needed to be changed since they assumed you could parse such policies but not compile them. One unit test, compile_q, bizarrely calls the `best_t` function directly, thereby bypassing a duplicate key check that otherwise would've applied. In this one I just removed the duplicate keys. I don't think they were there on purpose. By the way, for Taproot scripts it's not clear that "no duplicate keys at all" is the correct thing to validate. We probably want to check "no duplicate keys within a single Tapleaf". But we can deal with that later. For now let's retain the existing behavior (no duplicate pubkeys anywhere). 3. You can no longer lift miniscripts with duplicate keys. (I am pretty sure this was just always wrong; see the `duplicate_keys` unit test which I modified which had a comment saying it was "unclear" whether we had intended to allow this behavior. --- src/descriptor/bare.rs | 6 +- src/descriptor/segwitv0.rs | 6 +- src/descriptor/sh.rs | 4 +- src/descriptor/tr/mod.rs | 7 +-- src/descriptor/tr/taptree.rs | 4 +- src/lib.rs | 13 ----- src/miniscript/mod.rs | 11 +--- src/policy/compiler.rs | 45 ++------------- src/policy/concrete.rs | 108 ++++++++++++----------------------- src/policy/mod.rs | 104 +++++++-------------------------- 10 files changed, 76 insertions(+), 232 deletions(-) diff --git a/src/descriptor/bare.rs b/src/descriptor/bare.rs index 18e40116b..ae2d67074 100644 --- a/src/descriptor/bare.rs +++ b/src/descriptor/bare.rs @@ -22,7 +22,7 @@ use crate::prelude::*; use crate::util::{varint_len, witness_to_scriptsig}; use crate::{ BareCtx, Error, ForEachKey, FromStrKey, Miniscript, MiniscriptKey, Satisfier, ToPublicKey, - TranslateErr, Translator, + TranslateErr, Translator, ValidationError, }; /// Create a Bare Descriptor. That is descriptor that is @@ -159,7 +159,7 @@ impl fmt::Display for Bare { } impl Liftable for Bare { - fn lift(&self) -> Result, Error> { self.ms.lift() } + fn lift(&self) -> Result, ValidationError> { self.ms.lift() } } impl FromTree for Bare { @@ -343,7 +343,7 @@ impl fmt::Display for Pkh { } impl Liftable for Pkh { - fn lift(&self) -> Result, Error> { + fn lift(&self) -> Result, ValidationError> { Ok(semantic::Policy::Key(self.pk.clone())) } } diff --git a/src/descriptor/segwitv0.rs b/src/descriptor/segwitv0.rs index 84e3ceb05..1e82a3b1a 100644 --- a/src/descriptor/segwitv0.rs +++ b/src/descriptor/segwitv0.rs @@ -21,7 +21,7 @@ use crate::prelude::*; use crate::util::varint_len; use crate::{ Error, ForEachKey, FromStrKey, Miniscript, MiniscriptKey, Satisfier, Segwitv0, Terminal, - Threshold, ToPublicKey, TranslateErr, Translator, + Threshold, ToPublicKey, TranslateErr, Translator, ValidationError, }; /// A Segwitv0 wsh descriptor #[derive(Clone, Ord, PartialOrd, Eq, PartialEq, Hash)] @@ -176,7 +176,7 @@ impl Wsh { } impl Liftable for Wsh { - fn lift(&self) -> Result, Error> { self.ms.lift() } + fn lift(&self) -> Result, ValidationError> { self.ms.lift() } } impl crate::expression::FromTree for Wsh { @@ -379,7 +379,7 @@ impl fmt::Display for Wpkh { } impl Liftable for Wpkh { - fn lift(&self) -> Result, Error> { + fn lift(&self) -> Result, ValidationError> { Ok(semantic::Policy::Key(self.pk.clone())) } } diff --git a/src/descriptor/sh.rs b/src/descriptor/sh.rs index d926bb381..43f73596a 100644 --- a/src/descriptor/sh.rs +++ b/src/descriptor/sh.rs @@ -25,7 +25,7 @@ use crate::prelude::*; use crate::util::{varint_len, witness_to_scriptsig}; use crate::{ push_opcode_size, Error, ForEachKey, FromStrKey, Legacy, Miniscript, MiniscriptKey, Satisfier, - Segwitv0, Threshold, ToPublicKey, TranslateErr, Translator, + Segwitv0, Threshold, ToPublicKey, TranslateErr, Translator, ValidationError, }; /// A Legacy p2sh Descriptor @@ -47,7 +47,7 @@ pub enum ShInner { } impl Liftable for Sh { - fn lift(&self) -> Result, Error> { + fn lift(&self) -> Result, ValidationError> { match self.inner { ShInner::Wsh(ref wsh) => wsh.lift(), ShInner::Wpkh(ref pk) => Ok(semantic::Policy::Key(pk.as_inner().clone())), diff --git a/src/descriptor/tr/mod.rs b/src/descriptor/tr/mod.rs index 6a1b1893f..1b3d6a68d 100644 --- a/src/descriptor/tr/mod.rs +++ b/src/descriptor/tr/mod.rs @@ -9,15 +9,14 @@ use sync::Arc; use super::checksum; use crate::expression::{self, FromTree}; use crate::miniscript::satisfy::{Placeholder, Satisfaction, SchnorrSigType, Witness}; -use crate::miniscript::Miniscript; use crate::plan::AssetProvider; use crate::policy::semantic::Policy; use crate::policy::Liftable; use crate::prelude::*; use crate::util::{varint_len, witness_size}; use crate::{ - Error, ForEachKey, FromStrKey, MiniscriptKey, ParseError, Satisfier, ScriptContext, Tap, - Threshold, ToPublicKey, TranslateErr, Translator, + Error, ForEachKey, FromStrKey, Miniscript, MiniscriptKey, ParseError, Satisfier, ScriptContext, + Tap, Threshold, ToPublicKey, TranslateErr, Translator, ValidationError, }; mod spend_info; @@ -410,7 +409,7 @@ impl fmt::Display for Tr { } impl Liftable for Tr { - fn lift(&self) -> Result, Error> { + fn lift(&self) -> Result, ValidationError> { match &self.tree { Some(root) => Ok(Policy::Thresh(Threshold::or( Arc::new(Policy::Key(self.internal_key.clone())), diff --git a/src/descriptor/tr/taptree.rs b/src/descriptor/tr/taptree.rs index 8d8d0e03f..13fe58f2b 100644 --- a/src/descriptor/tr/taptree.rs +++ b/src/descriptor/tr/taptree.rs @@ -8,7 +8,7 @@ use crate::miniscript::context::Tap; use crate::policy::{Liftable, Semantic}; use crate::prelude::Vec; use crate::sync::Arc; -use crate::{Miniscript, MiniscriptKey, Threshold, ToPublicKey}; +use crate::{Miniscript, MiniscriptKey, Threshold, ToPublicKey, ValidationError}; /// Tried to construct Taproot tree which was too deep. #[derive(PartialEq, Eq, Debug)] @@ -74,7 +74,7 @@ impl TapTree { } impl Liftable for TapTree { - fn lift(&self) -> Result, crate::Error> { + fn lift(&self) -> Result, ValidationError> { let thresh_vec = self .leaves() .map(|item| item.miniscript().lift().map(Arc::new)) diff --git a/src/lib.rs b/src/lib.rs index 2d9beaf78..ec74672d8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -450,10 +450,6 @@ pub enum Error { #[cfg(feature = "compiler")] /// Compiler related errors CompilerError(crate::policy::compiler::CompilerError), - /// Errors related to policy - ConcretePolicy(policy::concrete::PolicyError), - /// Errors related to lifting - LiftError(policy::LiftError), /// Forward script context related errors ContextError(miniscript::context::ScriptContextError), /// Tried to construct a Taproot tree which was too deep. @@ -514,8 +510,6 @@ impl fmt::Display for Error { Self::TapTreeDepthError(ref e) => fmt::Display::fmt(e, f), #[cfg(feature = "compiler")] Self::CompilerError(ref e) => fmt::Display::fmt(e, f), - Self::ConcretePolicy(ref e) => fmt::Display::fmt(e, f), - Self::LiftError(ref e) => fmt::Display::fmt(e, f), Self::MaxRecursiveDepthExceeded => write!( f, "Recursive depth over {} not permitted", @@ -569,8 +563,6 @@ impl std::error::Error for Error { Secp(e) => Some(e), #[cfg(feature = "compiler")] CompilerError(e) => Some(e), - ConcretePolicy(e) => Some(e), - LiftError(e) => Some(e), ContextError(e) => Some(e), TapTreeDepthError(e) => Some(e), PubKeyCtxError(e, _) => Some(e), @@ -594,11 +586,6 @@ impl From for Error { fn from(e: miniscript::types::Error) -> Self { Self::TypeCheck(e.to_string()) } } -#[doc(hidden)] -impl From for Error { - fn from(e: policy::LiftError) -> Self { Self::LiftError(e) } -} - #[doc(hidden)] impl From for Error { fn from(e: crate::descriptor::TapTreeDepthError) -> Self { Self::TapTreeDepthError(e) } diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs index 74883e7bd..9b18953ec 100644 --- a/src/miniscript/mod.rs +++ b/src/miniscript/mod.rs @@ -1937,10 +1937,8 @@ mod tests { // ...though you can parse one with from_str_insane let ok_insane = Miniscript::::from_str_insane("and_v(v:pk(A),pk(A))").unwrap(); - // ...it can be lifted, though it's unclear whether this is a deliberate - // choice or just an accident. It seems weird given that duplicate public - // keys are forbidden in several other places. - ok_insane.lift().unwrap(); + // ...but you cannot then lift it. + ok_insane.lift().unwrap_err(); } #[test] @@ -1965,10 +1963,7 @@ mod tests { ) .unwrap(); // ...but this cannot lifted - assert!(matches!( - ok_insane.lift().unwrap_err(), - Error::LiftError(crate::policy::LiftError::HeightTimelockCombination) - )); + assert!(matches!(ok_insane.lift().unwrap_err(), ValidationError::MixedTimeLocks,)); // nor can it have sane rules applied to it assert_eq!( ok_insane.validate(&ValidationParams::SANE).unwrap_err(), diff --git a/src/policy/compiler.rs b/src/policy/compiler.rs index 4ffadb804..d6bc02878 100644 --- a/src/policy/compiler.rs +++ b/src/policy/compiler.rs @@ -16,7 +16,7 @@ use crate::miniscript::types::{self, ErrorKind, ExtData, Type}; use crate::miniscript::ScriptContext; use crate::policy::Concrete; use crate::prelude::*; -use crate::{policy, Miniscript, MiniscriptKey, Terminal}; +use crate::{Miniscript, MiniscriptKey, Terminal}; type PolicyCache = BTreeMap<(Concrete, OrdF64, Option), BTreeMap>>; @@ -39,7 +39,7 @@ impl Ord for OrdF64 { } /// Detailed error type for compiler. -#[derive(Copy, Clone, PartialEq, Eq, Debug, Hash)] +#[derive(Clone, PartialEq, Eq, Debug)] pub enum CompilerError { /// `And` fragments only support two args. NonBinaryArgAnd, @@ -69,8 +69,6 @@ pub enum CompilerError { /// Index of the leaf that contains branching fragments. leaf_index: usize, }, - ///Policy related errors - PolicyError(policy::concrete::PolicyError), } impl fmt::Display for CompilerError { @@ -99,7 +97,6 @@ impl fmt::Display for CompilerError { leaf_index ) } - Self::PolicyError(ref e) => fmt::Display::fmt(e, f), } } } @@ -118,16 +115,10 @@ impl error::Error for CompilerError { | NoInternalKey | TooManyTapleaves { .. } | IfFragmentInNativeLeaf { .. } => None, - PolicyError(e) => Some(e), } } } -#[doc(hidden)] -impl From for CompilerError { - fn from(e: policy::concrete::PolicyError) -> Self { Self::PolicyError(e) } -} - /// Hash required for using OrdF64 as key for hashmap impl hash::Hash for OrdF64 { fn hash(&self, state: &mut H) { self.0.to_bits().hash(state); } @@ -1220,8 +1211,8 @@ mod tests { use super::*; use crate::miniscript::{Legacy, Segwitv0, Tap}; - use crate::policy::Liftable; - use crate::{script_num_size, AbsLockTime, RelLockTime, Threshold, ToPublicKey}; + use crate::policy::{self, Liftable}; + use crate::{script_num_size, RelLockTime, Threshold, ToPublicKey}; type SPolicy = Concrete; type BPolicy = Concrete; @@ -1263,16 +1254,6 @@ mod tests { #[test] fn compile_timelocks() { - // artificially create a policy that is problematic and try to compile - let pol: SPolicy = Concrete::And(vec![ - Arc::new(Concrete::Key("A".to_string())), - Arc::new(Concrete::And(vec![ - Arc::new(Concrete::After(AbsLockTime::from_consensus(9).unwrap())), - Arc::new(Concrete::After(AbsLockTime::from_consensus(1_000_000_000).unwrap())), - ])), - ]); - assert!(pol.compile::().is_err()); - // This should compile let pol: SPolicy = SPolicy::from_str("and(pk(A),or(and(after(9),pk(B)),and(after(1000000000),pk(C))))") @@ -1316,7 +1297,7 @@ mod tests { // compile into taproot context to avoid limit errors let policy = SPolicy::from_str( - "and(and(and(or(127@thresh(2,pk(A),pk(B),thresh(2,or(127@pk(A),1@pk(B)),after(100),or(and(pk(C),after(200)),and(pk(D),sha256(66687aadf862bd776c8fc18b8e9f8e20089714856ee233b3902a591d0d5f2925))),pk(E))),1@pk(F)),sha256(66687aadf862bd776c8fc18b8e9f8e20089714856ee233b3902a591d0d5f2925)),or(127@pk(G),1@after(300))),or(127@after(400),pk(H)))" + "and(and(and(or(127@thresh(2,pk(A1),pk(B1),thresh(2,or(127@pk(A),1@pk(B)),after(100),or(and(pk(C),after(200)),and(pk(D),sha256(66687aadf862bd776c8fc18b8e9f8e20089714856ee233b3902a591d0d5f2925))),pk(E))),1@pk(F)),sha256(66687aadf862bd776c8fc18b8e9f8e20089714856ee233b3902a591d0d5f2925)),or(127@pk(G),1@after(300))),or(127@after(400),pk(H)))" ).expect("parsing"); let compilation: TapAstElemExt = best_t(&mut BTreeMap::new(), &policy, 1.0, None).unwrap(); @@ -1608,22 +1589,6 @@ mod tests { "Compilation succeeded with '{:?}' OP count (sat)", ops_count, ); - - // Test that we refuse to compile policies with duplicated keys - let (keys, _) = pubkeys_and_a_sig(1); - let key = Arc::new(Concrete::Key(keys[0])); - let res = - Concrete::Or(vec![(1, Arc::clone(&key)), (1, Arc::clone(&key))]).compile::(); - assert_eq!( - res, - Err(CompilerError::PolicyError(policy::concrete::PolicyError::DuplicatePubKeys)) - ); - // Same for legacy - let res = Concrete::Or(vec![(1, key.clone()), (1, key)]).compile::(); - assert_eq!( - res, - Err(CompilerError::PolicyError(policy::concrete::PolicyError::DuplicatePubKeys)) - ); } #[test] diff --git a/src/policy/concrete.rs b/src/policy/concrete.rs index 3e2220df5..d330cebf2 100644 --- a/src/policy/concrete.rs +++ b/src/policy/concrete.rs @@ -4,8 +4,6 @@ //! use core::{cmp, fmt, str}; -#[cfg(feature = "std")] -use std::error; use bitcoin::absolute; #[cfg(feature = "compiler")] @@ -29,6 +27,7 @@ use crate::sync::Arc; use crate::Descriptor; use crate::{ AbsLockTime, Error, ForEachKey, FromStrKey, MiniscriptKey, RelLockTime, Threshold, Translator, + ValidationError, ValidationParams, }; /// Maximum `TapLeaf`s allowed in a compiled TapTree @@ -117,15 +116,6 @@ impl Ord for Policy { } } -/// Detailed error type for concrete policies. -#[derive(Copy, Clone, PartialEq, Eq, Debug, Hash)] -pub enum PolicyError { - /// Cannot lift policies that have a combination of height and timelocks. - HeightTimelockCombination, - /// Duplicate Public Keys. - DuplicatePubKeys, -} - /// Descriptor context for [`Policy`] compilation into a [`Descriptor`]. pub enum DescriptorCtx { /// See docs for [`Descriptor::Bare`]. @@ -141,28 +131,6 @@ pub enum DescriptorCtx { Tr(Option), } -impl fmt::Display for PolicyError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match *self { - Self::HeightTimelockCombination => { - f.write_str("Cannot lift policies that have a heightlock and timelock combination") - } - Self::DuplicatePubKeys => f.write_str("Policy contains duplicate keys"), - } - } -} - -#[cfg(feature = "std")] -impl error::Error for PolicyError { - fn cause(&self) -> Option<&dyn error::Error> { - use self::PolicyError::*; - - match self { - HeightTimelockCombination | DuplicatePubKeys => None, - } - } -} - #[cfg(feature = "compiler")] struct TapleafProbabilityIter<'p, Pk: MiniscriptKey> { stack: Vec<(f64, &'p Policy)>, @@ -278,7 +246,6 @@ impl Policy { // TODO: We might require other compile errors for Taproot. #[cfg(feature = "compiler")] pub fn compile_tr(&self, unspendable_key: Option) -> Result, CompilerError> { - self.is_valid().map_err(CompilerError::PolicyError)?; self.check_binary_ops()?; match self.is_safe_nonmalleable() { (false, _) => Err(CompilerError::TopLevelSigless), @@ -334,7 +301,6 @@ impl Policy { return Err(CompilerError::TooManyTapleaves { n: 1, max: 0 }); } let max_leaves = max_leaves.min(MAX_COMPILATION_LEAVES); - self.is_valid().map_err(CompilerError::PolicyError)?; self.check_binary_ops()?; match self.is_safe_nonmalleable() { (false, _) => Err(CompilerError::TopLevelSigless), @@ -400,7 +366,6 @@ impl Policy { &self, unspendable_key: Option, ) -> Result, Error> { - self.is_valid().map_err(Error::ConcretePolicy)?; self.check_binary_ops()?; match self.is_safe_nonmalleable() { (false, _) => Err(Error::from(CompilerError::TopLevelSigless)), @@ -454,7 +419,6 @@ impl Policy { &self, desc_ctx: DescriptorCtx, ) -> Result, Error> { - self.is_valid().map_err(Error::ConcretePolicy)?; self.check_binary_ops()?; match self.is_safe_nonmalleable() { (false, _) => Err(Error::from(CompilerError::TopLevelSigless)), @@ -480,7 +444,6 @@ impl Policy { /// the compiler document in doc/compiler.md for more details. #[cfg(feature = "compiler")] pub fn compile(&self) -> Result, CompilerError> { - self.is_valid()?; self.check_binary_ops()?; match self.is_safe_nonmalleable() { (false, _) => Err(CompilerError::TopLevelSigless), @@ -786,33 +749,29 @@ impl Policy { Ok(()) } - /// Checks whether the policy contains duplicate public keys. - pub fn check_duplicate_keys(&self) -> Result<(), PolicyError> { - let pks = self.keys(); - let pks_len = pks.len(); - let unique_pks_len = pks.into_iter().collect::>().len(); - - if pks_len > unique_pks_len { - Err(PolicyError::DuplicatePubKeys) - } else { - Ok(()) + /// Validates the concrete policy against a set of validation parameters. + /// + /// Because policies do not correspond directly to on-chain objects, none of the consensus + /// limits are checked. + pub fn validate(&self, params: &ValidationParams) -> Result<(), ValidationError> { + if !params.allow_duplicate_keys { + let pks = self.keys(); + let pks_len = pks.len(); + let unique_pks_len = pks.into_iter().collect::>().len(); + + if pks_len > unique_pks_len { + return Err(ValidationError::DuplicateKeys); + } } - } - /// Checks whether the given concrete policy contains a combination of - /// timelocks and heightlocks. - /// - /// # Returns - /// - /// Returns an error if there is at least one satisfaction that contains - /// a combination of heightlock and timelock. - pub fn check_timelocks(&self) -> Result<(), PolicyError> { - let aggregated_timelock_info = self.timelock_info(); - if aggregated_timelock_info.contains_combination { - Err(PolicyError::HeightTimelockCombination) - } else { - Ok(()) + if !params.allow_mixed_time_locks { + let aggregated_timelock_info = self.timelock_info(); + if aggregated_timelock_info.contains_combination { + return Err(ValidationError::MixedTimeLocks); + } } + + Ok(()) } /// Processes `Policy` using `post_order_iter`, creates a `TimelockInfo` for each `Nullary` node @@ -861,16 +820,6 @@ impl Policy { infos.pop().unwrap() } - /// This returns whether the given policy is valid or not. It maybe possible that the policy - /// contains Non-two argument `and`, `or` or a `0` arg thresh. - /// Validity condition also checks whether there is a possible satisfaction - /// combination of timelocks and heightlocks - pub fn is_valid(&self) -> Result<(), PolicyError> { - self.check_timelocks()?; - self.check_duplicate_keys()?; - Ok(()) - } - /// Checks if any possible compilation of the policy could be compiled /// as non-malleable and requiring signatures. /// @@ -1000,7 +949,9 @@ impl str::FromStr for Policy { fn from_str(s: &str) -> Result { let tree = expression::Tree::from_str(s)?; let policy: Self = FromTree::from_tree(tree.root())?; - policy.check_timelocks().map_err(Error::ConcretePolicy)?; + policy + .validate(&ValidationParams::SANE) + .map_err(Error::Validation)?; Ok(policy) } } @@ -1430,6 +1381,17 @@ mod tests { let _ = Policy::::from_str("and(after(10),after(500000000))").unwrap(); } + #[test] + fn parse_validation_error() { + let res = "or(pk(A),pk(A))".parse::>(); + assert!(matches!(res.unwrap_err(), Error::Validation(ValidationError::DuplicateKeys))); + let res = "and(after(100),after(1000000000))".parse::>(); + assert!(matches!(res.unwrap_err(), Error::Validation(ValidationError::MixedTimeLocks))); + // Same policy with `or` is not "mixing timelocks" because the timelocks are exclusive. + let res = "or(after(100),after(1000000000))".parse::>(); + assert!(res.is_ok()); + } + #[test] fn parse_zero_disjunction() { "or(0@pk(09),0@TRIVIAL)" diff --git a/src/policy/mod.rs b/src/policy/mod.rs index 068bddc4e..cebd3e8b5 100644 --- a/src/policy/mod.rs +++ b/src/policy/mod.rs @@ -9,9 +9,6 @@ //! The format represents EC public keys abstractly to allow wallets to replace //! these with BIP32 paths, pay-to-contract instructions, etc. //! -use core::fmt; -#[cfg(feature = "std")] -use std::error; #[cfg(feature = "compiler")] pub mod compiler; @@ -26,7 +23,7 @@ use crate::miniscript::{Miniscript, ScriptContext}; use crate::sync::Arc; #[cfg(all(not(feature = "std"), not(test)))] use crate::Vec; -use crate::{Error, MiniscriptKey, Terminal, Threshold}; +use crate::{MiniscriptKey, Terminal, Threshold, ValidationError, ValidationParams}; /// Policy entailment algorithm maximum number of terminals allowed. const ENTAILMENT_MAX_TERMINALS: usize = 20; @@ -49,69 +46,14 @@ const ENTAILMENT_MAX_TERMINALS: usize = 20; /// policies. pub trait Liftable { /// Converts this object into an abstract policy. - fn lift(&self) -> Result, Error>; -} - -/// Error occurring during lifting. -#[derive(Copy, Clone, PartialEq, Eq, Debug, Hash)] -pub enum LiftError { - /// Cannot lift policies that have a combination of height and timelocks. - HeightTimelockCombination, - /// Duplicate public keys. - BranchExceedResourceLimits, - /// Cannot lift raw descriptors. - RawDescriptorLift, -} - -impl fmt::Display for LiftError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match *self { - Self::HeightTimelockCombination => { - f.write_str("Cannot lift policies that have a heightlock and timelock combination") - } - Self::BranchExceedResourceLimits => f.write_str( - "Cannot lift policies containing one branch that exceeds resource limits", - ), - Self::RawDescriptorLift => f.write_str("Cannot lift raw descriptors"), - } - } -} - -#[cfg(feature = "std")] -impl error::Error for LiftError { - fn cause(&self) -> Option<&dyn error::Error> { - match self { - Self::HeightTimelockCombination - | Self::BranchExceedResourceLimits - | Self::RawDescriptorLift => None, - } - } -} - -impl Miniscript { - /// Lifting corresponds to conversion of a miniscript into a [`Semantic`] - /// policy for human readable or machine analysis. However, naively lifting - /// miniscripts can result in incorrect interpretations that don't - /// correspond to the underlying semantics when we try to spend them on - /// bitcoin network. This can occur if the miniscript contains: - /// 1. A combination of timelocks - /// 2. A spend that exceeds resource limits - pub fn lift_check(&self) -> Result<(), LiftError> { - if !self.within_resource_limits() { - Err(LiftError::BranchExceedResourceLimits) - } else if self.has_mixed_timelocks() { - Err(LiftError::HeightTimelockCombination) - } else { - Ok(()) - } - } + fn lift(&self) -> Result, ValidationError>; } impl Liftable for Miniscript { - fn lift(&self) -> Result, Error> { - // check whether the root miniscript can have a spending path that is - // a combination of heightlock and timelock - self.lift_check()?; + fn lift(&self) -> Result, ValidationError> { + // The "sane" rules are pretty-much defined as "the rules that must be followed + // for lifting to work and analysis to make sense" + self.validate(&ValidationParams::SANE)?; let mut stack = vec![]; for item in self.rtl_post_order_iter() { @@ -119,9 +61,7 @@ impl Liftable for Miniscript Terminal::PkK(ref pk) | Terminal::PkH(ref pk) => { Arc::new(Semantic::Key(pk.clone())) } - Terminal::RawPkH(ref _pkh) => { - return Err(Error::LiftError(LiftError::RawDescriptorLift)) - } + Terminal::RawPkH(ref _pkh) => unreachable!("checked by self.validate above"), Terminal::After(t) => Arc::new(Semantic::After(t)), Terminal::Older(t) => Arc::new(Semantic::Older(t)), Terminal::Sha256(ref h) => Arc::new(Semantic::Sha256(h.clone())), @@ -178,7 +118,7 @@ impl Liftable for Miniscript } impl Liftable for Descriptor { - fn lift(&self) -> Result, Error> { + fn lift(&self) -> Result, ValidationError> { match *self { Self::Bare(ref bare) => bare.lift(), Self::Pkh(ref pkh) => pkh.lift(), @@ -191,14 +131,12 @@ impl Liftable for Descriptor { } impl Liftable for Semantic { - fn lift(&self) -> Result { Ok(self.clone()) } + fn lift(&self) -> Result { Ok(self.clone()) } } impl Liftable for Concrete { - fn lift(&self) -> Result, Error> { - // do not lift if there is a possible satisfaction - // involving combination of timelocks and heightlocks - self.check_timelocks().map_err(Error::ConcretePolicy)?; + fn lift(&self) -> Result, ValidationError> { + self.validate(&ValidationParams::SANE)?; let ret = match *self { Self::Unsatisfiable => Semantic::Unsatisfiable, Self::Trivial => Semantic::Trivial, @@ -210,13 +148,13 @@ impl Liftable for Concrete { Self::Ripemd160(ref h) => Semantic::Ripemd160(h.clone()), Self::Hash160(ref h) => Semantic::Hash160(h.clone()), Self::And(ref subs) => { - let semantic_subs: Result>, Error> = + let semantic_subs: Result>, ValidationError> = subs.iter().map(Liftable::lift).collect(); let semantic_subs = semantic_subs?.into_iter().map(Arc::new).collect(); Semantic::Thresh(Threshold::new(2, semantic_subs).unwrap()) } Self::Or(ref subs) => { - let semantic_subs: Result>, Error> = + let semantic_subs: Result>, ValidationError> = subs.iter().map(|(_p, sub)| sub.lift()).collect(); let semantic_subs = semantic_subs?.into_iter().map(Arc::new).collect(); Semantic::Thresh(Threshold::new(1, semantic_subs).unwrap()) @@ -230,7 +168,7 @@ impl Liftable for Concrete { } } impl Liftable for Arc> { - fn lift(&self) -> Result, Error> { self.as_ref().lift() } + fn lift(&self) -> Result, ValidationError> { self.as_ref().lift() } } #[cfg(test)] @@ -276,6 +214,12 @@ mod tests { // thresh with k = 2 assert!(ConcretePol::from_str("thresh(2,after(1000000000),after(100),pk())").is_err()); } + + #[test] + fn parse_duplicate_pubkeys() { + assert!(ConcretePol::from_str("or(and(pk(A),pk(B)),and(pk(A),pk(D)))").is_err()); + } + #[test] fn policy_rtt_tests() { concrete_policy_rtt("pk()"); @@ -402,14 +346,6 @@ mod tests { assert_eq!(descriptor, expected_descriptor); } - { - // Invalid policy compilation (Duplicate PubKeys) - let policy: Concrete = policy_str!("or(and(pk(A),pk(B)),and(pk(A),pk(D)))"); - let descriptor = policy.compile_tr(Some(unspendable_key.clone())); - - assert_eq!(descriptor.unwrap_err().to_string(), "Policy contains duplicate keys"); - } - // Non-trivial multi-node compilation { let node_policies = [ From f78241f6164144bcf4d6053c85a3ba7142c39e04 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Tue, 20 May 2025 03:02:54 +0000 Subject: [PATCH 2/6] descriptor: use new validation in constructors for pkh descriptors Update sortedmult, pkh, wpkh to the new validation framework. These constructors now return a ValidationError rather than an Error, which is a more precise type. The constructors also actually do validation; in particular sortedmulti now does a duplicate pubkey check where it didn't use to. The theme over these next several commits is going to be "we now do checks where we didn't use to". It may be that we get pushback from users over this. In that case we need to adapt somehow but at least we can do it in a principled way, rather than the current "there are a ton of different ways to check and they're all randomly half implemented". --- src/descriptor/bare.rs | 17 ++++++++--------- src/descriptor/mod.rs | 14 +++++++------- src/descriptor/segwitv0.rs | 23 +++++++++++++---------- src/descriptor/sh.rs | 10 +++++++--- src/psbt/finalizer.rs | 8 +++++--- src/psbt/mod.rs | 4 ++++ 6 files changed, 44 insertions(+), 32 deletions(-) diff --git a/src/descriptor/bare.rs b/src/descriptor/bare.rs index ae2d67074..83cf3a61c 100644 --- a/src/descriptor/bare.rs +++ b/src/descriptor/bare.rs @@ -14,7 +14,7 @@ use bitcoin::{Address, Network, ScriptBuf, Weight}; use crate::descriptor::write_descriptor; use crate::expression::{self, FromTree}; -use crate::miniscript::context::{ScriptContext, ScriptContextError}; +use crate::miniscript::context::ScriptContext; use crate::miniscript::satisfy::{Placeholder, Satisfaction, Witness}; use crate::plan::AssetProvider; use crate::policy::{semantic, Liftable}; @@ -193,12 +193,11 @@ pub struct Pkh { impl Pkh { /// Create a new Pkh descriptor - pub fn new(pk: Pk) -> Result { - // do the top-level checks - match BareCtx::check_pk(&pk) { - Ok(()) => Ok(Self { pk }), - Err(e) => Err(e), - } + pub fn new(pk: Pk) -> Result { + BareCtx::SANE + .validate_pk(&pk) + .map_err(ValidationError::Key)?; + Ok(Self { pk }) } /// Get a reference to the inner key @@ -248,7 +247,7 @@ impl Pkh { let res = Pkh::new(t.pk(&self.pk)?); match res { Ok(pk) => Ok(pk), - Err(e) => Err(TranslateErr::OuterError(Error::from(e))), + Err(e) => Err(TranslateErr::OuterError(Error::Validation(e))), } } } @@ -353,7 +352,7 @@ impl FromTree for Pkh { let pk = root .verify_terminal_parent("pkh", "public key") .map_err(Error::Parse)?; - Self::new(pk).map_err(Error::ContextError) + Self::new(pk).map_err(Error::Validation) } } diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs index a2e48bcf2..fb05f2afe 100644 --- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -29,7 +29,7 @@ use crate::plan::{AssetProvider, Plan}; use crate::prelude::*; use crate::{ expression, hash256, BareCtx, Error, ForEachKey, FromStrKey, MiniscriptKey, ParseError, - Satisfier, Threshold, ToPublicKey, TranslateErr, Translator, + Satisfier, Threshold, ToPublicKey, TranslateErr, Translator, ValidationError, }; mod bare; @@ -157,15 +157,15 @@ impl Descriptor { } /// Create a new PkH descriptor - pub fn new_pkh(pk: Pk) -> Result { Ok(Self::Pkh(Pkh::new(pk)?)) } + pub fn new_pkh(pk: Pk) -> Result { Ok(Self::Pkh(Pkh::new(pk)?)) } /// Create a new Wpkh descriptor /// Will return Err if uncompressed key is used - pub fn new_wpkh(pk: Pk) -> Result { Ok(Self::Wpkh(Wpkh::new(pk)?)) } + pub fn new_wpkh(pk: Pk) -> Result { Ok(Self::Wpkh(Wpkh::new(pk)?)) } /// Create a new sh wrapped wpkh from `Pk`. /// Errors when uncompressed keys are supplied - pub fn new_sh_wpkh(pk: Pk) -> Result { Ok(Self::Sh(Sh::new_wpkh(pk)?)) } + pub fn new_sh_wpkh(pk: Pk) -> Result { Ok(Self::Sh(Sh::new_wpkh(pk)?)) } // Miniscripts @@ -210,7 +210,7 @@ impl Descriptor { /// Errors when miniscript exceeds resource limits under p2sh context pub fn new_sh_sortedmulti( thresh: Threshold, - ) -> Result { + ) -> Result { Ok(Self::Sh(Sh::new_sortedmulti(thresh)?)) } @@ -219,7 +219,7 @@ impl Descriptor { /// Errors when miniscript exceeds resource limits under segwit context pub fn new_sh_wsh_sortedmulti( thresh: Threshold, - ) -> Result { + ) -> Result { Ok(Self::Sh(Sh::new_wsh_sortedmulti(thresh)?)) } @@ -227,7 +227,7 @@ impl Descriptor { /// Errors when miniscript exceeds resource limits under p2sh context pub fn new_wsh_sortedmulti( thresh: Threshold, - ) -> Result { + ) -> Result { Ok(Self::Wsh(Wsh::new_sortedmulti(thresh)?)) } diff --git a/src/descriptor/segwitv0.rs b/src/descriptor/segwitv0.rs index 1e82a3b1a..38d0d7b4b 100644 --- a/src/descriptor/segwitv0.rs +++ b/src/descriptor/segwitv0.rs @@ -12,7 +12,7 @@ use bitcoin::{Address, Network, ScriptBuf, Weight}; use crate::descriptor::write_descriptor; use crate::expression::{self, FromTree}; -use crate::miniscript::context::{ScriptContext, ScriptContextError}; +use crate::miniscript::context::ScriptContext; use crate::miniscript::limits::MAX_PUBKEYS_PER_MULTISIG; use crate::miniscript::satisfy::{Placeholder, Satisfaction, Witness}; use crate::plan::AssetProvider; @@ -45,7 +45,11 @@ impl Wsh { } /// Create a new sortedmulti wsh descriptor - pub fn new_sortedmulti(thresh: Threshold) -> Result { + pub fn new_sortedmulti( + thresh: Threshold, + ) -> Result { + // The context checks will be carried out inside new function for + // sortedMultiVec Ok(Self { ms: Miniscript::sortedmulti(thresh) }) } @@ -225,12 +229,11 @@ pub struct Wpkh { impl Wpkh { /// Create a new Wpkh descriptor - pub fn new(pk: Pk) -> Result { - // do the top-level checks - match Segwitv0::check_pk(&pk) { - Ok(_) => Ok(Self { pk }), - Err(e) => Err(e), - } + pub fn new(pk: Pk) -> Result { + Segwitv0::SANE + .validate_pk(&pk) + .map_err(ValidationError::Key)?; + Ok(Self { pk }) } /// Get the inner key @@ -276,7 +279,7 @@ impl Wpkh { let res = Wpkh::new(t.pk(&self.pk)?); match res { Ok(pk) => Ok(pk), - Err(e) => Err(TranslateErr::OuterError(Error::from(e))), + Err(e) => Err(TranslateErr::OuterError(Error::Validation(e))), } } } @@ -389,7 +392,7 @@ impl crate::expression::FromTree for Wpkh { let pk = top .verify_terminal_parent("wpkh", "public key") .map_err(Error::Parse)?; - Self::new(pk).map_err(Error::ContextError) + Self::new(pk).map_err(Error::Validation) } } diff --git a/src/descriptor/sh.rs b/src/descriptor/sh.rs index 43f73596a..9522802d3 100644 --- a/src/descriptor/sh.rs +++ b/src/descriptor/sh.rs @@ -120,7 +120,11 @@ impl Sh { /// Create a new p2sh sortedmulti descriptor with threshold `k` /// and Vec of `pks`. - pub fn new_sortedmulti(thresh: Threshold) -> Result { + pub fn new_sortedmulti( + thresh: Threshold, + ) -> Result { + // The context checks will be carried out inside new function for + // sortedMultiVec Ok(Self { inner: ShInner::Ms(Miniscript::sortedmulti(thresh)) }) } @@ -136,14 +140,14 @@ impl Sh { /// `k` and Vec of `pks` pub fn new_wsh_sortedmulti( thresh: Threshold, - ) -> Result { + ) -> Result { // The context checks will be carried out inside new function for // sortedMultiVec Ok(Self { inner: ShInner::Wsh(Wsh::new_sortedmulti(thresh)?) }) } /// Create a new p2sh wrapped wpkh from `Pk` - pub fn new_wpkh(pk: Pk) -> Result { + pub fn new_wpkh(pk: Pk) -> Result { Ok(Self { inner: ShInner::Wpkh(Wpkh::new(pk)?) }) } diff --git a/src/psbt/finalizer.rs b/src/psbt/finalizer.rs index 79cd50ce5..833d8bbc2 100644 --- a/src/psbt/finalizer.rs +++ b/src/psbt/finalizer.rs @@ -178,7 +178,7 @@ fn get_descriptor(psbt: &Psbt, index: usize) -> Result, In *script_pubkey == addr.script_pubkey() }); match partial_sig_contains_pk { - Some((pk, _sig)) => Descriptor::new_pkh(*pk).map_err(InputError::from), + Some((pk, _sig)) => Descriptor::new_pkh(*pk).map_err(InputError::Validation), None => Err(InputError::MissingPubkey), } } else if script_pubkey.is_p2wpkh() { @@ -195,7 +195,7 @@ fn get_descriptor(psbt: &Psbt, index: usize) -> Result, In } }); match partial_sig_contains_pk { - Some((pk, _sig)) => Ok(Descriptor::new_wpkh(*pk)?), + Some((pk, _sig)) => Descriptor::new_wpkh(*pk).map_err(InputError::Validation), None => Err(InputError::MissingPubkey), } } else if script_pubkey.is_p2wsh() { @@ -256,7 +256,9 @@ fn get_descriptor(psbt: &Psbt, index: usize) -> Result, In } }); match partial_sig_contains_pk { - Some((pk, _sig)) => Ok(Descriptor::new_sh_wpkh(*pk)?), + Some((pk, _sig)) => { + Descriptor::new_sh_wpkh(*pk).map_err(InputError::Validation) + } None => Err(InputError::MissingPubkey), } } else { diff --git a/src/psbt/mod.rs b/src/psbt/mod.rs index 07860217e..c5dcada4e 100644 --- a/src/psbt/mod.rs +++ b/src/psbt/mod.rs @@ -144,6 +144,8 @@ pub enum InputError { /// the corresponding publickey pubkey: bitcoin::PublicKey, }, + /// Pass through the underlying errors in miniscript. + Validation(crate::ValidationError), } #[cfg(feature = "std")] @@ -169,6 +171,7 @@ impl error::Error for InputError { KeyErr(e) => Some(e), Interpreter(e) => Some(e), MiniscriptError(e) => Some(e), + Validation(e) => Some(e), } } } @@ -216,6 +219,7 @@ impl fmt::Display for InputError { Self::NonStandardSighashType(ref e) => { write!(f, "Non-standard sighash type {}", e) } + Self::Validation(ref e) => e.fmt(f), } } } From 8015104432cb02619309190200459fd2f077fb45 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Wed, 3 Jun 2026 14:18:37 +0000 Subject: [PATCH 3/6] miniscript: clean up a typeck error This grammar is weird and hard to understand. --- src/miniscript/types/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/miniscript/types/mod.rs b/src/miniscript/types/mod.rs index 9ad89c349..15e729d17 100644 --- a/src/miniscript/types/mod.rs +++ b/src/miniscript/types/mod.rs @@ -73,7 +73,7 @@ impl fmt::Display for Error { match self.error { ErrorKind::NonZeroDupIf => write!( f, - "fragment «{}» represents needs to be `z`, needs to consume zero elements from the stack", + "fragment «{}» needs to be `z` (consumes zero stack elements) to use `d`", self.fragment_string, ), ErrorKind::LeftNotDissatisfiable => write!( From 85da678bc7a8ba2c0370032acddc13935208e1ef Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Mon, 19 May 2025 23:12:21 +0000 Subject: [PATCH 4/6] descriptor: use new validation in the other constructors This is another big commit that has a bunch of behavior changes of the form "now we enforce sanity rules where we didn't use to". This one lets us delete a huge chunk of context.rs which is nice. This eliminates the NonStandardBareScript error variant, but no tests fail and I didn't replace the functionality. If I open a PR with this commit in this state, we need to file an issue to restore this and to add tests for it. --- src/descriptor/bare.rs | 12 +-- src/descriptor/mod.rs | 54 +++++++------ src/descriptor/segwitv0.rs | 7 +- src/descriptor/sh.rs | 9 +-- src/descriptor/tr/mod.rs | 19 +++-- src/lib.rs | 10 --- src/miniscript/context.rs | 158 +------------------------------------ src/miniscript/mod.rs | 7 +- src/policy/concrete.rs | 16 ++-- src/psbt/finalizer.rs | 9 ++- 10 files changed, 78 insertions(+), 223 deletions(-) diff --git a/src/descriptor/bare.rs b/src/descriptor/bare.rs index 83cf3a61c..f276199a3 100644 --- a/src/descriptor/bare.rs +++ b/src/descriptor/bare.rs @@ -35,9 +35,8 @@ pub struct Bare { impl Bare { /// Create a new raw descriptor - pub fn new(ms: Miniscript) -> Result { - // do the top-level checks - BareCtx::top_level_checks(&ms)?; + pub fn new(ms: Miniscript) -> Result { + ms.validate(&BareCtx::SANE)?; Ok(Self { ms }) } @@ -91,7 +90,9 @@ impl Bare { where T: Translator, { - Bare::new(self.ms.translate_pk(t)?).map_err(TranslateErr::OuterError) + Bare::new(self.ms.translate_pk(t)?) + .map_err(Error::Validation) + .map_err(TranslateErr::OuterError) } } @@ -165,8 +166,7 @@ impl Liftable for Bare { impl FromTree for Bare { fn from_tree(root: expression::TreeIterItem) -> Result { let sub = Miniscript::::from_tree(root)?; - BareCtx::top_level_checks(&sub)?; - Self::new(sub) + Self::new(sub).map_err(Error::Validation) } } diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs index fb05f2afe..6ed6a977c 100644 --- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -172,26 +172,28 @@ impl Descriptor { /// Create a new sh for a given redeem script /// Errors when miniscript exceeds resource limits under p2sh context /// or does not type check at the top level - pub fn new_sh(ms: Miniscript) -> Result { Ok(Self::Sh(Sh::new(ms)?)) } + pub fn new_sh(ms: Miniscript) -> Result { + Ok(Self::Sh(Sh::new(ms)?)) + } /// Create a new wsh descriptor from witness script /// Errors when miniscript exceeds resource limits under p2sh context /// or does not type check at the top level - pub fn new_wsh(ms: Miniscript) -> Result { + pub fn new_wsh(ms: Miniscript) -> Result { Ok(Self::Wsh(Wsh::new(ms)?)) } /// Create a new sh wrapped wsh descriptor with witness script /// Errors when miniscript exceeds resource limits under wsh context /// or does not type check at the top level - pub fn new_sh_wsh(ms: Miniscript) -> Result { + pub fn new_sh_wsh(ms: Miniscript) -> Result { Ok(Self::Sh(Sh::new_wsh(ms)?)) } /// Create a new bare descriptor from witness script /// Errors when miniscript exceeds resource limits under bare context /// or does not type check at the top level - pub fn new_bare(ms: Miniscript) -> Result { + pub fn new_bare(ms: Miniscript) -> Result { Ok(Self::Bare(Bare::new(ms)?)) } @@ -233,7 +235,7 @@ impl Descriptor { /// Create new tr descriptor /// Errors when miniscript exceeds resource limits under Tap context - pub fn new_tr(key: Pk, script: Option>) -> Result { + pub fn new_tr(key: Pk, script: Option>) -> Result { Ok(Self::Tr(Tr::new(key, script)?)) } @@ -1254,13 +1256,15 @@ mod tests { // in terms of the in-memory representation -- OrI(False, False). // Test that the way we display the ambiguous fragment doesn't // change, in case somebody somehow is depending on it. - let desc = StdDescriptor::from_str("sh(u:0)").unwrap(); - assert_eq!("sh(u:0)#ncq3yf9h", desc.to_string()); + let desc = StdDescriptor::from_str("wsh(u:0)").unwrap(); + assert_eq!("wsh(u:0)#6mwq40tt", desc.to_string()); // This is a regression test for https://github.com/rust-bitcoin/rust-miniscript/pull/735 - // which was found at the same time. It's just a bug plain and simple. - let desc = StdDescriptor::from_str("sh(and_n(u:0,1))").unwrap(); - assert_eq!("sh(and_n(u:0,1))#5j5tw8nm", desc.to_string()); + // which was found at the same time. It's just a bug plain and simple. (We were + // encoding and_n as "and_b".) + let desc_str = format!("wsh(and_n({TEST_PK},1))#a23r7pua"); + let desc = desc_str.parse::().unwrap(); + assert_eq!(desc_str, desc.to_string()); } #[test] @@ -1648,7 +1652,9 @@ mod tests { #[test] fn after_is_cltv() { - let descriptor = Descriptor::::from_str("wsh(after(1000))").unwrap(); + let descriptor = format!("wsh(and_v(v:{TEST_PK},after(1000)))") + .parse::>() + .unwrap(); let script = descriptor.explicit_script().unwrap(); let actual_instructions: Vec<_> = script.instructions().collect(); @@ -1659,7 +1665,9 @@ mod tests { #[test] fn older_is_csv() { - let descriptor = Descriptor::::from_str("wsh(older(1000))").unwrap(); + let descriptor = format!("wsh(and_v(v:{TEST_PK},older(1000)))") + .parse::>() + .unwrap(); let script = descriptor.explicit_script().unwrap(); let actual_instructions: Vec<_> = script.instructions().collect(); @@ -2252,17 +2260,17 @@ pk(03f28773c2d975288bc7d1d205c3748651b075fbc6610e58cddeeddf8f19405aa8))"; "sh(wpkh(020000000000000000000000000000000000000000000000000000000000000002))", ); - let wsh = StdDescriptor::from_str("wsh(1)").unwrap(); - assert_eq!(format!("{}", wsh), "wsh(1)#mrg7xj7p"); - assert_eq!(format!("{:#}", wsh), "wsh(1)"); + let wsh = StdDescriptor::from_str("wsh(0)").unwrap(); + assert_eq!(format!("{}", wsh), "wsh(0)#a0qjqdfq"); + assert_eq!(format!("{:#}", wsh), "wsh(0)"); - let sh = StdDescriptor::from_str("sh(1)").unwrap(); - assert_eq!(format!("{}", sh), "sh(1)#l8r75ggs"); - assert_eq!(format!("{:#}", sh), "sh(1)"); + let sh = StdDescriptor::from_str("sh(0)").unwrap(); + assert_eq!(format!("{}", sh), "sh(0)#ettjjhl3"); + assert_eq!(format!("{:#}", sh), "sh(0)"); - let shwsh = StdDescriptor::from_str("sh(wsh(1))").unwrap(); - assert_eq!(format!("{}", shwsh), "sh(wsh(1))#hcyfl07f"); - assert_eq!(format!("{:#}", shwsh), "sh(wsh(1))"); + let shwsh = StdDescriptor::from_str("sh(wsh(0))").unwrap(); + assert_eq!(format!("{}", shwsh), "sh(wsh(0))#f45zwpau"); + assert_eq!(format!("{:#}", shwsh), "sh(wsh(0))"); let tr = StdDescriptor::from_str( "tr(020000000000000000000000000000000000000000000000000000000000000002)", @@ -2320,11 +2328,11 @@ pk(03f28773c2d975288bc7d1d205c3748651b075fbc6610e58cddeeddf8f19405aa8))"; Descriptor::::from_str( "wsh(or_i(pk(0202baaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa0a66a),1))", ) - .unwrap(); + .unwrap_err(); Descriptor::::from_str( "sh(or_i(pk(0202baaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa0a66a),1))", ) - .unwrap(); + .unwrap_err(); Descriptor::::from_str( "tr(02baaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa0a66a,1)", ) diff --git a/src/descriptor/segwitv0.rs b/src/descriptor/segwitv0.rs index 38d0d7b4b..b1755b146 100644 --- a/src/descriptor/segwitv0.rs +++ b/src/descriptor/segwitv0.rs @@ -38,9 +38,8 @@ impl Wsh { pub fn as_inner(&self) -> &Miniscript { &self.ms } /// Create a new wsh descriptor - pub fn new(ms: Miniscript) -> Result { - // do the top-level checks - Segwitv0::top_level_checks(&ms)?; + pub fn new(ms: Miniscript) -> Result { + ms.validate(&Segwitv0::SANE)?; Ok(Self { ms }) } @@ -191,7 +190,7 @@ impl crate::expression::FromTree for Wsh { .map_err(Error::Parse)?; let sub = Miniscript::from_tree(top)?; - Segwitv0::top_level_checks(&sub)?; + sub.validate(&Segwitv0::SANE).map_err(Error::Validation)?; Ok(Self { ms: sub }) } } diff --git a/src/descriptor/sh.rs b/src/descriptor/sh.rs index 9522802d3..384d44c31 100644 --- a/src/descriptor/sh.rs +++ b/src/descriptor/sh.rs @@ -88,7 +88,7 @@ impl crate::expression::FromTree for Sh { "wpkh" => ShInner::Wpkh(Wpkh::from_tree(top)?), _ => { let sub = Miniscript::from_tree(top)?; - Legacy::top_level_checks(&sub)?; + sub.validate(&Legacy::SANE).map_err(Error::Validation)?; ShInner::Ms(sub) } }; @@ -112,9 +112,8 @@ impl Sh { pub fn as_inner(&self) -> &ShInner { &self.inner } /// Create a new p2sh descriptor with the raw miniscript - pub fn new(ms: Miniscript) -> Result { - // do the top-level checks - Legacy::top_level_checks(&ms)?; + pub fn new(ms: Miniscript) -> Result { + ms.validate(&Legacy::SANE)?; Ok(Self { inner: ShInner::Ms(ms) }) } @@ -129,7 +128,7 @@ impl Sh { } /// Create a new p2sh wrapped wsh descriptor with the raw miniscript - pub fn new_wsh(ms: Miniscript) -> Result { + pub fn new_wsh(ms: Miniscript) -> Result { Ok(Self { inner: ShInner::Wsh(Wsh::new(ms)?) }) } diff --git a/src/descriptor/tr/mod.rs b/src/descriptor/tr/mod.rs index 1b3d6a68d..7b83ec15a 100644 --- a/src/descriptor/tr/mod.rs +++ b/src/descriptor/tr/mod.rs @@ -15,8 +15,8 @@ use crate::policy::Liftable; use crate::prelude::*; use crate::util::{varint_len, witness_size}; use crate::{ - Error, ForEachKey, FromStrKey, Miniscript, MiniscriptKey, ParseError, Satisfier, ScriptContext, - Tap, Threshold, ToPublicKey, TranslateErr, Translator, ValidationError, + Error, ForEachKey, FromStrKey, Miniscript, MiniscriptKey, ParseError, Satisfier, + ScriptContext as _, Tap, Threshold, ToPublicKey, TranslateErr, Translator, ValidationError, }; mod spend_info; @@ -91,8 +91,10 @@ impl hash::Hash for Tr { impl Tr { /// Create a new [`Tr`] descriptor from internal key and [`TapTree`] - pub fn new(internal_key: Pk, tree: Option>) -> Result { - Tap::check_pk(&internal_key)?; + pub fn new(internal_key: Pk, tree: Option>) -> Result { + Tap::SANE + .validate_pk(&internal_key) + .map_err(ValidationError::Key)?; Ok(Self { internal_key, tree, spend_info: Mutex::new(None) }) } @@ -249,8 +251,9 @@ impl Tr { Some(tree) => Some(tree.translate_pk(translate)?), None => None, }; - let translate_desc = - Tr::new(translate.pk(&self.internal_key)?, tree).map_err(TranslateErr::OuterError)?; + let translate_desc = Tr::new(translate.pk(&self.internal_key)?, tree) + .map_err(Error::Validation) + .map_err(TranslateErr::OuterError)?; Ok(translate_desc) } } @@ -351,7 +354,7 @@ impl crate::expression::FromTree for Tr { .map_err(Error::Parse)?; let tap_tree = match root_children.next() { - None => return Self::new(internal_key, None), + None => return Self::new(internal_key, None).map_err(Error::Validation), Some(tree) => tree, }; @@ -382,7 +385,7 @@ impl crate::expression::FromTree for Tr { tap_tree_iter.skip_descendants(); } } - Self::new(internal_key, Some(tree_builder.finalize())) + Self::new(internal_key, Some(tree_builder.finalize())).map_err(Error::Validation) } } diff --git a/src/lib.rs b/src/lib.rs index ec74672d8..4c7b3f750 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -456,9 +456,6 @@ pub enum Error { TapTreeDepthError(crate::descriptor::TapTreeDepthError), /// Recursion depth exceeded when parsing policy/miniscript from string MaxRecursiveDepthExceeded, - /// Anything but c:pk(key) (P2PK), c:pk_h(key) (P2PKH), and thresh_m(k,...) - /// up to n=3 is invalid by standardness (bare) - NonStandardBareScript, /// Miniscript is equivalent to false. No possible satisfaction ImpossibleSatisfaction, /// Bare descriptors don't have any addresses @@ -515,12 +512,6 @@ impl fmt::Display for Error { "Recursive depth over {} not permitted", MAX_RECURSION_DEPTH ), - Self::NonStandardBareScript => write!( - f, - "Anything but c:pk(key) (P2PK), c:pk_h(key) (P2PKH), and thresh_m(k,...) \ - up to n=3 is invalid by standardness (bare). - " - ), Self::ImpossibleSatisfaction => write!(f, "Impossible to satisfy Miniscript"), Self::BareDescriptorAddr => write!(f, "Bare descriptors don't have address"), Self::PubKeyCtxError(ref pk, ref ctx) => { @@ -552,7 +543,6 @@ impl std::error::Error for Error { | CouldNotSatisfy | TypeCheck(_) | MaxRecursiveDepthExceeded - | NonStandardBareScript | ImpossibleSatisfaction | BareDescriptorAddr | TrNoScriptCode diff --git a/src/miniscript/context.rs b/src/miniscript/context.rs index e0c889d0f..d3da39765 100644 --- a/src/miniscript/context.rs +++ b/src/miniscript/context.rs @@ -14,7 +14,7 @@ use crate::miniscript::limits::{ MAX_STACK_SIZE, MAX_STANDARD_P2WSH_SCRIPT_SIZE, MAX_STANDARD_P2WSH_STACK_ITEMS, }; use crate::prelude::*; -use crate::{hash256, Error, ForEachKey, Miniscript, MiniscriptKey, Terminal, ValidationParams}; +use crate::{hash256, Miniscript, MiniscriptKey, Terminal, ValidationParams}; /// Error for Script Context #[derive(Clone, PartialEq, Eq, Debug)] @@ -199,12 +199,6 @@ where _frag: &Terminal, ) -> Result<(), ScriptContextError>; - /// Each context has slightly different rules on what Pks are allowed in descriptors - /// Legacy/Bare does not allow x_only keys - /// Segwit does not allow uncompressed keys and x_only keys - /// Tapscript does not allow uncompressed keys - fn check_pk(pk: &Pk) -> Result<(), ScriptContextError>; - /// Depending on script context, the size of a satifaction witness may slightly differ. fn max_satisfaction_size(ms: &Miniscript) -> Option; /// Depending on script Context, some of the Terminals might not @@ -280,60 +274,6 @@ where Ok(()) } - /// Check whether the top-level is type B - fn top_level_type_check(ms: &Miniscript) -> Result<(), Error> { - // (Ab)use `for_each_key` to record the number of derivation paths a multipath key has. - #[derive(PartialEq)] - enum MultipathLenChecker { - SinglePath, - MultipathLen(usize), - LenMismatch, - } - - let mut checker = MultipathLenChecker::SinglePath; - ms.for_each_key(|key| { - match key.num_der_paths() { - 0 | 1 => {} - n => match checker { - MultipathLenChecker::SinglePath => { - checker = MultipathLenChecker::MultipathLen(n); - } - MultipathLenChecker::MultipathLen(len) => { - if len != n { - checker = MultipathLenChecker::LenMismatch; - } - } - MultipathLenChecker::LenMismatch => {} - }, - } - true - }); - - if checker == MultipathLenChecker::LenMismatch { - return Err(Error::MultipathDescLenMismatch); - } - Ok(()) - } - - /// Other top level checks that are context specific - fn other_top_level_checks(_ms: &Miniscript) -> Result<(), Error> { - Ok(()) - } - - /// Check top level consensus rules. - // All the previous check_ were applied at each fragment while parsing script - // Because if any of sub-miniscripts failed the resource level check, the entire - // miniscript would also be invalid. However, there are certain checks like - // in Bare context, only c:pk(key) (P2PK), - // c:pk_h(key) (P2PKH), and thresh_m(k,...) up to n=3 are allowed - // that are only applicable at the top-level - // We can also combine the top-level check for Base::B here - // even though it does not depend on context, but helps in cleaner code - fn top_level_checks(ms: &Miniscript) -> Result<(), Error> { - Self::top_level_type_check(ms)?; - Self::other_top_level_checks(ms) - } - /// The type of signature required for satisfaction // We need to context decide whether the serialize pk to 33 byte or 32 bytes. // And to decide which type of signatures to look for during satisfaction @@ -391,27 +331,11 @@ impl ScriptContext for Legacy { } } - // Only compressed and uncompressed public keys are allowed in Legacy context - fn check_pk(pk: &Pk) -> Result<(), ScriptContextError> { - if pk.is_x_only_key() { - Err(ScriptContextError::XOnlyKeysNotAllowed(pk.to_string(), Self::name_str())) - } else { - Ok(()) - } - } - fn check_global_consensus_validity( ms: &Miniscript, ) -> Result<(), ScriptContextError> { // 1. Check the node first, throw an error on the language itself let node_checked = match ms.node { - Terminal::PkK(ref pk) => Self::check_pk(pk), - Terminal::Multi(ref thresh) | Terminal::SortedMulti(ref thresh) => { - for pk in thresh.iter() { - Self::check_pk(pk)?; - } - Ok(()) - } Terminal::MultiA(..) | Terminal::SortedMultiA(..) => { Err(ScriptContextError::MultiANotAllowed) } @@ -511,29 +435,11 @@ impl ScriptContext for Segwitv0 { Ok(()) } - // No x-only keys or uncompressed keys in Segwitv0 context - fn check_pk(pk: &Pk) -> Result<(), ScriptContextError> { - if pk.is_uncompressed() { - Err(ScriptContextError::UncompressedKeysNotAllowed) - } else if pk.is_x_only_key() { - Err(ScriptContextError::XOnlyKeysNotAllowed(pk.to_string(), Self::name_str())) - } else { - Ok(()) - } - } - fn check_global_consensus_validity( ms: &Miniscript, ) -> Result<(), ScriptContextError> { // 1. Check the node first, throw an error on the language itself let node_checked = match ms.node { - Terminal::PkK(ref pk) => Self::check_pk(pk), - Terminal::Multi(ref thresh) | Terminal::SortedMulti(ref thresh) => { - for pk in thresh.iter() { - Self::check_pk(pk)?; - } - Ok(()) - } Terminal::MultiA(..) | Terminal::SortedMultiA(..) => { Err(ScriptContextError::MultiANotAllowed) } @@ -642,27 +548,11 @@ impl ScriptContext for Tap { Ok(()) } - // No uncompressed keys in Tap context - fn check_pk(pk: &Pk) -> Result<(), ScriptContextError> { - if pk.is_uncompressed() { - Err(ScriptContextError::UncompressedKeysNotAllowed) - } else { - Ok(()) - } - } - fn check_global_consensus_validity( ms: &Miniscript, ) -> Result<(), ScriptContextError> { // 1. Check the node first, throw an error on the language itself let node_checked = match ms.node { - Terminal::PkK(ref pk) => Self::check_pk(pk), - Terminal::MultiA(ref thresh) | Terminal::SortedMultiA(ref thresh) => { - for pk in thresh.iter() { - Self::check_pk(pk)?; - } - Ok(()) - } Terminal::Multi(..) | Terminal::SortedMulti(..) => { Err(ScriptContextError::TaprootMultiDisabled) } @@ -769,27 +659,11 @@ impl ScriptContext for BareCtx { Ok(()) } - // No x-only keys in Bare context - fn check_pk(pk: &Pk) -> Result<(), ScriptContextError> { - if pk.is_x_only_key() { - Err(ScriptContextError::XOnlyKeysNotAllowed(pk.to_string(), Self::name_str())) - } else { - Ok(()) - } - } - fn check_global_consensus_validity( ms: &Miniscript, ) -> Result<(), ScriptContextError> { // 1. Check the node first, throw an error on the language itself let node_checked = match ms.node { - Terminal::PkK(ref key) => Self::check_pk(key), - Terminal::Multi(ref thresh) | Terminal::SortedMulti(ref thresh) => { - for pk in thresh.iter() { - Self::check_pk(pk)?; - } - Ok(()) - } Terminal::MultiA(..) | Terminal::SortedMultiA(..) => { Err(ScriptContextError::MultiANotAllowed) } @@ -826,20 +700,6 @@ impl ScriptContext for BareCtx { } } - fn other_top_level_checks(ms: &Miniscript) -> Result<(), Error> { - match &ms.node { - Terminal::Check(ref ms) => match &ms.node { - Terminal::RawPkH(_pkh) => Ok(()), - Terminal::PkK(_pk) | Terminal::PkH(_pk) => Ok(()), - _ => Err(Error::NonStandardBareScript), - }, - Terminal::Multi(ref thresh) | Terminal::SortedMulti(ref thresh) if thresh.n() <= 3 => { - Ok(()) - } - _ => Err(Error::NonStandardBareScript), - } - } - fn max_satisfaction_size(ms: &Miniscript) -> Option { // For bare outputs the script appears in the scriptpubkey; its cost // is the same as for a legacy scriptsig. @@ -879,9 +739,6 @@ impl ScriptContext for NoChecks { Ok(()) } - // No checks in NoChecks - fn check_pk(_pk: &Pk) -> Result<(), ScriptContextError> { Ok(()) } - fn check_global_policy_validity( _ms: &Miniscript, ) -> Result<(), ScriptContextError> { @@ -937,19 +794,6 @@ impl ScriptContext for NoChecks { Ok(()) } - fn top_level_type_check(_: &Miniscript) -> Result<(), Error> { - Ok(()) - } - - fn other_top_level_checks(_ms: &Miniscript) -> Result<(), Error> { - Ok(()) - } - - fn top_level_checks(ms: &Miniscript) -> Result<(), Error> { - Self::top_level_type_check(ms)?; - Self::other_top_level_checks(ms) - } - fn sig_type() -> SigType { SigType::Ecdsa } } diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs index 9b18953ec..6b09c5ad5 100644 --- a/src/miniscript/mod.rs +++ b/src/miniscript/mod.rs @@ -366,6 +366,7 @@ mod private { // Malleability is only a top-level check since you can fix malleability // in some cases by adding wrappers. if !params.allow_malleability && !self.is_non_malleable() { + println!("{:?}", self); return Err(ValidationError::Malleable); } @@ -937,7 +938,11 @@ impl Miniscript { translated.push(Arc::new(new_ms)); } - Ok(Arc::try_unwrap(translated.pop().unwrap()).unwrap()) + let ret = translated.pop().unwrap(); + ret.validate(&CtxQ::SANE) + .map_err(Error::Validation) + .map_err(TranslateErr::OuterError)?; + Ok(Arc::try_unwrap(ret).unwrap()) } /// Substitutes raw public keys hashes with the public keys as provided by map. diff --git a/src/policy/concrete.rs b/src/policy/concrete.rs index d330cebf2..b99169b82 100644 --- a/src/policy/concrete.rs +++ b/src/policy/concrete.rs @@ -398,7 +398,8 @@ impl Policy { } } }, - )?; + ) + .map_err(Error::Validation)?; Ok(tree) } } @@ -424,10 +425,15 @@ impl Policy { (false, _) => Err(Error::from(CompilerError::TopLevelSigless)), (_, false) => Err(Error::from(CompilerError::ImpossibleNonMalleableCompilation)), _ => match desc_ctx { - DescriptorCtx::Bare => Descriptor::new_bare(compiler::best_compilation(self)?), - DescriptorCtx::Sh => Descriptor::new_sh(compiler::best_compilation(self)?), - DescriptorCtx::Wsh => Descriptor::new_wsh(compiler::best_compilation(self)?), - DescriptorCtx::ShWsh => Descriptor::new_sh_wsh(compiler::best_compilation(self)?), + DescriptorCtx::Bare => Descriptor::new_bare(compiler::best_compilation(self)?) + .map_err(Error::Validation), + DescriptorCtx::Sh => { + Descriptor::new_sh(compiler::best_compilation(self)?).map_err(Error::Validation) + } + DescriptorCtx::Wsh => Descriptor::new_wsh(compiler::best_compilation(self)?) + .map_err(Error::Validation), + DescriptorCtx::ShWsh => Descriptor::new_sh_wsh(compiler::best_compilation(self)?) + .map_err(Error::Validation), DescriptorCtx::Tr(unspendable_key) => self .compile_tr(unspendable_key) .map_err(Error::CompilerError), diff --git a/src/psbt/finalizer.rs b/src/psbt/finalizer.rs index 833d8bbc2..f81d1ea16 100644 --- a/src/psbt/finalizer.rs +++ b/src/psbt/finalizer.rs @@ -211,7 +211,7 @@ fn get_descriptor(psbt: &Psbt, index: usize) -> Result, In }); } let ms = Miniscript::::decode_consensus(witness_script)?; - Ok(Descriptor::new_wsh(ms.substitute_raw_pkh(&map))?) + Ok(Descriptor::new_wsh(ms.substitute_raw_pkh(&map)).map_err(InputError::Validation)?) } else { Err(InputError::MissingWitnessScript) } @@ -237,7 +237,8 @@ fn get_descriptor(psbt: &Psbt, index: usize) -> Result, In let ms = Miniscript::::decode_consensus( witness_script, )?; - Ok(Descriptor::new_sh_wsh(ms.substitute_raw_pkh(&map))?) + Ok(Descriptor::new_sh_wsh(ms.substitute_raw_pkh(&map)) + .map_err(InputError::Validation)?) } else { Err(InputError::MissingWitnessScript) } @@ -270,7 +271,7 @@ fn get_descriptor(psbt: &Psbt, index: usize) -> Result, In let ms = Miniscript::::decode_consensus( redeem_script, )?; - Ok(Descriptor::new_sh(ms)?) + Ok(Descriptor::new_sh(ms).map_err(InputError::Validation)?) } else { Err(InputError::MissingWitnessScript) } @@ -286,7 +287,7 @@ fn get_descriptor(psbt: &Psbt, index: usize) -> Result, In return Err(InputError::NonEmptyRedeemScript); } let ms = Miniscript::::decode_consensus(&script_pubkey)?; - Ok(Descriptor::new_bare(ms.substitute_raw_pkh(&map))?) + Ok(Descriptor::new_bare(ms.substitute_raw_pkh(&map)).map_err(InputError::Validation)?) } } From 3616e040aa277daac150c1a5512a83406b2e0e28 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Wed, 21 May 2025 12:46:44 +0000 Subject: [PATCH 5/6] remove MultipathDescLenMismatch from top-level Error This is superceded by the validation error. For some reason triggers rustfmt to reflow the MaxRecursiveDepthExceeded variant. That will also be deleted in a later commit. --- src/descriptor/mod.rs | 25 ++++++++++++++++--------- src/lib.rs | 15 ++++----------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs index 6ed6a977c..65a042c37 100644 --- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -969,6 +969,12 @@ impl Descriptor { /// /// For multipath descriptors it will return as many descriptors as there is /// "parallel" paths. For regular descriptors it will just return itself. + /// + /// # Panics + /// + /// May panic if given a descriptor which has multiple multi-path keys with different + /// numbers of paths. Such descriptors are rejected by the ordinary constructors, + /// which enforce "sanity" rules, so most users do not need to worry about this. #[allow(clippy::blocks_in_conditions)] pub fn into_single_descriptors(self) -> Result, Error> { // All single-path descriptors contained in this descriptor. @@ -1000,19 +1006,16 @@ impl Descriptor { struct IndexChoser(usize); impl Translator for IndexChoser { type TargetPk = DescriptorPublicKey; - type Error = Error; + type Error = core::convert::Infallible; - fn pk(&mut self, pk: &DescriptorPublicKey) -> Result { + fn pk(&mut self, pk: &DescriptorPublicKey) -> Result { match pk { DescriptorPublicKey::Single(..) | DescriptorPublicKey::XPub(..) => { Ok(pk.clone()) } - DescriptorPublicKey::MultiXPub(_) => pk - .clone() - .into_single_keys() - .get(self.0) - .cloned() - .ok_or(Error::MultipathDescLenMismatch), + DescriptorPublicKey::MultiXPub(_) => { + Ok(pk.clone().into_single_keys()[self.0].clone()) + } } } translate_hash_clone!(DescriptorPublicKey); @@ -1020,9 +1023,13 @@ impl Descriptor { for (i, desc) in descriptors.iter_mut().enumerate() { let mut index_choser = IndexChoser(i); + // This ? could trigger if e.g. there are two multipath keys (a,b) and (a,c); + // these are distinct and will not trigger "duplicate pubkey" checks but once + // we fork the descriptor, 'c' will appear twice in one of them, resulting in + // an error. *desc = desc .translate_pk(&mut index_choser) - .map_err(|e| e.expect_translator_err("No Context errors possible"))?; + .map_err(TranslateErr::into_outer_err)?; } Ok(descriptors) diff --git a/src/lib.rs b/src/lib.rs index 4c7b3f750..9c61df1c3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -464,9 +464,6 @@ pub enum Error { 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 - /// derivation indexes of different lengths. - MultipathDescLenMismatch, /// Invalid absolute locktime AbsoluteLockTime(AbsLockTimeError), /// Invalid absolute locktime @@ -507,18 +504,15 @@ impl fmt::Display for Error { Self::TapTreeDepthError(ref e) => fmt::Display::fmt(e, f), #[cfg(feature = "compiler")] Self::CompilerError(ref e) => fmt::Display::fmt(e, f), - Self::MaxRecursiveDepthExceeded => write!( - f, - "Recursive depth over {} not permitted", - MAX_RECURSION_DEPTH - ), + Self::MaxRecursiveDepthExceeded => { + write!(f, "Recursive depth over {} not permitted", MAX_RECURSION_DEPTH) + } Self::ImpossibleSatisfaction => write!(f, "Impossible to satisfy Miniscript"), Self::BareDescriptorAddr => write!(f, "Bare descriptors don't have address"), Self::PubKeyCtxError(ref pk, ref ctx) => { write!(f, "Pubkey error: {} under {} scriptcontext", pk, ctx) } Self::TrNoScriptCode => write!(f, "No script code for Tr descriptors"), - Self::MultipathDescLenMismatch => write!(f, "At least two BIP389 key expressions in the descriptor contain tuples of derivation indexes of different lengths"), Self::AbsoluteLockTime(ref e) => e.fmt(f), Self::RelativeLockTime(ref e) => e.fmt(f), Self::Threshold(ref e) => e.fmt(f), @@ -545,8 +539,7 @@ impl std::error::Error for Error { | MaxRecursiveDepthExceeded | ImpossibleSatisfaction | BareDescriptorAddr - | TrNoScriptCode - | MultipathDescLenMismatch => None, + | TrNoScriptCode => None, ScriptLexer(e) => Some(e), AddrError(e) => Some(e), AddrP2shError(e) => Some(e), From c90c148e21de8a073c5e8c3e3c0d40da22f5e2f4 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Tue, 20 May 2025 12:44:54 +0000 Subject: [PATCH 6/6] miniscript: delete ExtParams, Ctx::check_* methods, and error types This is a big commit but it's almost all red. This results in a bunch of error messages changing since we now fail due to ValidationParams rather than due to Context::check_*. One observable performance change is that we parse entire Miniscripts before doing bounds checks. (This means that the error messages for exceeded limits are now more accurate, though they will probably revert when we do some optimization passes.) There is a max recursion check in Expression::from_str and another one in Miniscript::from_ast that I have left alone, for now, because of this. The recursion check needs to fire immediately before constructing any object that exceeds the maximum depth. Later we will add a call to validate_non_top_level to Miniscript::from_ast. This will let us drop the maximum depth check, but it will require an API change because the caller will need to provide validation parameters. So we leave it be for now. --- src/descriptor/mod.rs | 22 +- src/lib.rs | 10 - src/miniscript/analyzable.rs | 5 - src/miniscript/context.rs | 558 +---------------------------------- src/miniscript/mod.rs | 44 ++- src/policy/compiler.rs | 18 +- 6 files changed, 48 insertions(+), 609 deletions(-) diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs index 65a042c37..b15e52a4f 100644 --- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -1240,7 +1240,6 @@ mod tests { use super::{checksum, *}; use crate::hex_script; - use crate::miniscript::context::ScriptContextError; #[cfg(feature = "compiler")] use crate::policy; @@ -2949,22 +2948,29 @@ pk(03f28773c2d975288bc7d1d205c3748651b075fbc6610e58cddeeddf8f19405aa8))"; #[test] fn too_many_pubkeys_for_p2sh() { - // Arbitrary 65-byte public key (66 with length prefix). - let pk = PublicKey::from_str( - "0400232a2acfc9b43fa89f1b4f608fde335d330d7114f70ea42bfb4a41db368a3e3be6934a4097dd25728438ef73debb1f2ffdb07fec0f18049df13bdc5285dc5b", - ) - .unwrap(); + // Arbitrary 65-byte public keys (66 with length prefix). We need distinct ones + // to avoid getting DuplicateKey errors. + let pks: Vec = vec![ + "0479be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798b7c52588d95c3b9aa25b0403f1eef75702e84bb7597aabe663b82f6f04ef2777".parse().unwrap(), + "04c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5e51e970159c23cc65c3a7be6b99315110809cd9acd992f1edc9bce55af301705".parse().unwrap(), + "04f9308a019258c31049344f85f89d5229b531c845836f99b08601f113bce036f9c77084f09cd217ebf01cc819d5c80ca99aff5666cb3ddce4934602897b4715bd".parse().unwrap(), + "04e493dbf1c10d80f3581e4904930b1404cc6c13900ee0758474fa94abe8c4cd13ae1266c15f2baa48a9bd1df6715aebb7269851cc404201bf30168422b88c630d".parse().unwrap(), + "042f8bde4d1a07209355b4a7250a5c5128e88b84bddc619ab7cba8d569b240efe42753ddd9c91a1c292b24562259363bd90877d8e454f297bf235782c459539959".parse().unwrap(), + "04fff97bd5755eeea420453a14355235d382f6472f8568a18b2f057a146029755651ed8885530449df0c4169fe80ba3a9f217f0f09ae701b5fc378f3c84f8a0998".parse().unwrap(), + "045cbdf0646e5db4eaa398f365f2ea7a0e3d419b7e0330e39ce92bddedcac4f9bc951435bf45daa69f5ce8729279e5ab2457ec2f47ec02184a5af7d9d6f78d9755".parse().unwrap(), + "042f01e5e15cca351daff3843fb70f3c2f0a1bdd05e5af888a67784ef3e10a2a01a3b25758beac66b6d6c2f7d5ecd2ec4b3d1dec2945a489e84a25d3479342132b".parse().unwrap(), + ]; // This is legal for CHECKMULTISIG, but the 8 keys consume the whole 520 bytes // allowed by P2SH, meaning that the full script goes over the limit. - let thresh = Threshold::new(2, vec![pk; 8]).expect("the thresh is ok.."); + let thresh = Threshold::new(2, pks).expect("the thresh is ok.."); let script = Miniscript::<_, Legacy>::sortedmulti(thresh).encode(); let res = Miniscript::<_, Legacy>::decode(&script); let error = res.expect_err("decoding should err"); match error { - Error::ContextError(ScriptContextError::MaxRedeemScriptSizeExceeded { .. }) => {} // ok + Error::Validation(ValidationError::MaxScriptSizeExceeded { .. }) => {} // ok other => panic!("unexpected error: {:?}", other), } } diff --git a/src/lib.rs b/src/lib.rs index 9c61df1c3..f504adce3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -450,8 +450,6 @@ pub enum Error { #[cfg(feature = "compiler")] /// Compiler related errors CompilerError(crate::policy::compiler::CompilerError), - /// Forward script context related errors - ContextError(miniscript::context::ScriptContextError), /// Tried to construct a Taproot tree which was too deep. TapTreeDepthError(crate::descriptor::TapTreeDepthError), /// Recursion depth exceeded when parsing policy/miniscript from string @@ -500,7 +498,6 @@ impl fmt::Display for Error { Self::CouldNotSatisfy => f.write_str("could not satisfy"), Self::TypeCheck(ref e) => write!(f, "typecheck: {}", e), Self::Secp(ref e) => fmt::Display::fmt(e, f), - Self::ContextError(ref e) => fmt::Display::fmt(e, f), Self::TapTreeDepthError(ref e) => fmt::Display::fmt(e, f), #[cfg(feature = "compiler")] Self::CompilerError(ref e) => fmt::Display::fmt(e, f), @@ -546,7 +543,6 @@ impl std::error::Error for Error { Secp(e) => Some(e), #[cfg(feature = "compiler")] CompilerError(e) => Some(e), - ContextError(e) => Some(e), TapTreeDepthError(e) => Some(e), PubKeyCtxError(e, _) => Some(e), AbsoluteLockTime(e) => Some(e), @@ -574,12 +570,6 @@ impl From for Error { fn from(e: crate::descriptor::TapTreeDepthError) -> Self { Self::TapTreeDepthError(e) } } -#[doc(hidden)] -impl From for Error { - fn from(e: miniscript::context::ScriptContextError) -> Self { Self::ContextError(e) } -} - -#[doc(hidden)] impl From for Error { fn from(e: bitcoin::secp256k1::Error) -> Self { Self::Secp(e) } } diff --git a/src/miniscript/analyzable.rs b/src/miniscript/analyzable.rs index d58a62038..ef65a1ab8 100644 --- a/src/miniscript/analyzable.rs +++ b/src/miniscript/analyzable.rs @@ -15,11 +15,6 @@ impl Miniscript { /// Whether the miniscript is malleable pub fn is_non_malleable(&self) -> bool { self.ty.mall.non_malleable } - /// Whether the miniscript can exceed the resource limits(Opcodes, Stack limit etc) - // It maybe possible to return a detail error type containing why the miniscript - // failed. But doing so may require returning a collection of errors - pub fn within_resource_limits(&self) -> bool { Ctx::check_local_validity(self).is_ok() } - /// Whether the miniscript contains a combination of timelocks pub fn has_mixed_timelocks(&self) -> bool { self.ext.timelock_info.contains_unspendable_path() } diff --git a/src/miniscript/context.rs b/src/miniscript/context.rs index d3da39765..92eadb24e 100644 --- a/src/miniscript/context.rs +++ b/src/miniscript/context.rs @@ -2,162 +2,15 @@ // SPDX-License-Identifier: CC0-1.0 use core::{fmt, hash}; -#[cfg(feature = "std")] -use std::error; use bitcoin::hashes::{hash160, ripemd160, sha256}; -use bitcoin::Weight; use super::decode::ParseableKey; use crate::miniscript::limits::{ - MAX_OPS_PER_SCRIPT, MAX_SCRIPTSIG_SIZE, MAX_SCRIPT_ELEMENT_SIZE, MAX_SCRIPT_SIZE, - MAX_STACK_SIZE, MAX_STANDARD_P2WSH_SCRIPT_SIZE, MAX_STANDARD_P2WSH_STACK_ITEMS, + MAX_OPS_PER_SCRIPT, MAX_SCRIPT_ELEMENT_SIZE, MAX_SCRIPT_SIZE, MAX_STACK_SIZE, + MAX_STANDARD_P2WSH_SCRIPT_SIZE, MAX_STANDARD_P2WSH_STACK_ITEMS, }; -use crate::prelude::*; -use crate::{hash256, Miniscript, MiniscriptKey, Terminal, ValidationParams}; - -/// Error for Script Context -#[derive(Clone, PartialEq, Eq, Debug)] -pub enum ScriptContextError { - /// Script Context does not permit OrI for non-malleability - /// Legacy fragments allow non-minimal IF which results in malleability - MalleableOrI, - /// Script Context does not permit DupIf for non-malleability - /// Legacy fragments allow non-minimal IF which results in malleability - MalleableDupIf, - /// Only Compressed keys allowed under current descriptor - /// Segwitv0 fragments do not allow uncompressed pubkeys - CompressedOnly(String), - /// XOnly keys are only allowed in Tap context - /// The first element is key, and second element is current script context - XOnlyKeysNotAllowed(String, &'static str), - /// Tapscript descriptors cannot contain uncompressed keys - /// Tap context can contain compressed or xonly - UncompressedKeysNotAllowed, - /// At least one satisfaction path in the Miniscript fragment has more than - /// `MAX_STANDARD_P2WSH_STACK_ITEMS` (100) witness elements. - MaxWitnessItemsExceeded { actual: usize, limit: usize }, - /// At least one satisfaction path in the Miniscript fragment contains more - /// than `MAX_OPS_PER_SCRIPT`(201) opcodes. - MaxOpCountExceeded { actual: usize, limit: usize }, - /// The Miniscript(under segwit context) corresponding - /// Script would be larger than `MAX_STANDARD_P2WSH_SCRIPT_SIZE`, - /// `MAX_SCRIPT_SIZE` or `MAX_BLOCK`(`Tap`) bytes. - MaxWitnessScriptSizeExceeded { max: usize, got: usize }, - /// The Miniscript (under p2sh context) corresponding Script would be - /// larger than `MAX_SCRIPT_ELEMENT_SIZE` bytes. - MaxRedeemScriptSizeExceeded { max: usize, got: usize }, - /// The Miniscript(under bare context) corresponding - /// Script would be larger than `MAX_SCRIPT_SIZE` bytes. - MaxBareScriptSizeExceeded { max: usize, got: usize }, - /// The policy rules of bitcoin core only permit Script size upto 1650 bytes - MaxScriptSigSizeExceeded { actual: usize, limit: usize }, - /// Impossible to satisfy the miniscript under the current context - ImpossibleSatisfaction, - /// No Multi Node in Taproot context - TaprootMultiDisabled, - /// Stack size exceeded in script execution - StackSizeLimitExceeded { actual: usize, limit: usize }, - /// MultiA is only allowed in post tapscript - MultiANotAllowed, -} - -#[cfg(feature = "std")] -impl error::Error for ScriptContextError { - fn cause(&self) -> Option<&dyn error::Error> { - use self::ScriptContextError::*; - - match self { - MalleableOrI - | MalleableDupIf - | CompressedOnly(_) - | XOnlyKeysNotAllowed(_, _) - | UncompressedKeysNotAllowed - | MaxWitnessItemsExceeded { .. } - | MaxOpCountExceeded { .. } - | MaxWitnessScriptSizeExceeded { .. } - | MaxRedeemScriptSizeExceeded { .. } - | MaxBareScriptSizeExceeded { .. } - | MaxScriptSigSizeExceeded { .. } - | ImpossibleSatisfaction - | TaprootMultiDisabled - | StackSizeLimitExceeded { .. } - | MultiANotAllowed => None, - } - } -} - -impl fmt::Display for ScriptContextError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match *self { - Self::MalleableOrI => write!(f, "OrI is malleable under Legacy rules"), - Self::MalleableDupIf => { - write!(f, "DupIf is malleable under Legacy rules") - } - Self::CompressedOnly(ref pk) => { - write!(f, "Only Compressed pubkeys are allowed in segwit context. Found {}", pk) - } - Self::XOnlyKeysNotAllowed(ref pk, ref ctx) => { - write!(f, "x-only key {} not allowed in {}", pk, ctx) - } - Self::UncompressedKeysNotAllowed => { - write!(f, "uncompressed keys cannot be used in Taproot descriptors.") - } - Self::MaxWitnessItemsExceeded { actual, limit } => write!( - f, - "At least one satisfaction path in the Miniscript fragment has {} witness items \ - (limit: {}).", - actual, limit - ), - Self::MaxOpCountExceeded { actual, limit } => write!( - f, - "At least one satisfaction path in the Miniscript fragment contains {} opcodes \ - (limit: {}).", - actual, limit - ), - Self::MaxWitnessScriptSizeExceeded { max, got } => write!( - f, - "The Miniscript corresponding Script cannot be larger than \ - {} bytes, but got {} bytes.", - max, got - ), - Self::MaxRedeemScriptSizeExceeded { max, got } => write!( - f, - "The Miniscript corresponding Script cannot be larger than \ - {} bytes, but got {} bytes.", - max, got - ), - Self::MaxBareScriptSizeExceeded { max, got } => write!( - f, - "The Miniscript corresponding Script cannot be larger than \ - {} bytes, but got {} bytes.", - max, got - ), - Self::MaxScriptSigSizeExceeded { actual, limit } => write!( - f, - "At least one satisfaction path in the Miniscript fragment has {} bytes \ - (limit: {}).", - actual, limit - ), - Self::ImpossibleSatisfaction => { - write!(f, "Impossible to satisfy Miniscript under the current context") - } - Self::TaprootMultiDisabled => { - write!(f, "Invalid use of Multi node in taproot context") - } - Self::StackSizeLimitExceeded { actual, limit } => { - write!( - f, - "Stack limit {} can exceed the allowed limit {} in at least one script path during script execution", - actual, limit - ) - } - Self::MultiANotAllowed => { - write!(f, "Multi a(CHECKSIGADD) only allowed post tapscript") - } - } - } -} +use crate::{hash256, Miniscript, MiniscriptKey, ValidationParams}; /// The ScriptContext for Miniscript. /// @@ -187,92 +40,8 @@ where /// explicit choice of parameters is made. const SANE: ValidationParams; - /// Depending on ScriptContext, fragments can be malleable. For Example, - /// under Legacy context, PkH is malleable because it is possible to - /// estimate the cost of satisfaction because of compressed keys - /// This is currently only used in compiler code for removing malleable - /// compilations. - /// This does NOT recursively check if the children of the fragment are - /// valid or not. Since the compilation proceeds in a leaf to root fashion, - /// a recursive check is unnecessary. - fn check_terminal_non_malleable( - _frag: &Terminal, - ) -> Result<(), ScriptContextError>; - /// Depending on script context, the size of a satifaction witness may slightly differ. fn max_satisfaction_size(ms: &Miniscript) -> Option; - /// Depending on script Context, some of the Terminals might not - /// be valid under the current consensus rules. - /// Or some of the script resource limits may have been exceeded. - /// These miniscripts would never be accepted by the Bitcoin network and hence - /// it is safe to discard them - /// For example, in Segwit Context with MiniscriptKey as bitcoin::PublicKey - /// uncompressed public keys are non-standard and thus invalid. - /// In LegacyP2SH context, scripts above 520 bytes are invalid. - /// Post Tapscript upgrade, this would have to consider other nodes. - /// This does *NOT* recursively check the miniscript fragments. - fn check_global_consensus_validity( - _ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - Ok(()) - } - - /// Depending on script Context, some of the script resource limits - /// may have been exceeded under the current bitcoin core policy rules - /// These miniscripts would never be accepted by the Bitcoin network and hence - /// it is safe to discard them. (unless explicitly disabled by non-standard flag) - /// For example, in Segwit Context with MiniscriptKey as bitcoin::PublicKey - /// scripts over 3600 bytes are invalid. - /// Post Tapscript upgrade, this would have to consider other nodes. - /// This does *NOT* recursively check the miniscript fragments. - fn check_global_policy_validity( - _ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - Ok(()) - } - - /// Consensus rules at the Miniscript satisfaction time. - /// It is possible that some paths of miniscript may exceed resource limits - /// and our current satisfier and lifting analysis would not work correctly. - /// For example, satisfaction path(Legacy/Segwitv0) may require more than 201 opcodes. - fn check_local_consensus_validity( - _ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - Ok(()) - } - - /// Policy rules at the Miniscript satisfaction time. - /// It is possible that some paths of miniscript may exceed resource limits - /// and our current satisfier and lifting analysis would not work correctly. - /// For example, satisfaction path in Legacy context scriptSig more - /// than 1650 bytes - fn check_local_policy_validity( - _ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - Ok(()) - } - - /// Check the consensus + policy(if not disabled) rules that are not based - /// satisfaction - fn check_global_validity( - ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - Self::check_global_consensus_validity(ms)?; - Self::check_global_policy_validity(ms)?; - Ok(()) - } - - /// Check the consensus + policy(if not disabled) rules including the - /// ones for satisfaction - fn check_local_validity( - ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - Self::check_global_consensus_validity(ms)?; - Self::check_global_policy_validity(ms)?; - Self::check_local_consensus_validity(ms)?; - Self::check_local_policy_validity(ms)?; - Ok(()) - } /// The type of signature required for satisfaction // We need to context decide whether the serialize pk to 33 byte or 32 bytes. @@ -321,75 +90,6 @@ impl ScriptContext for Legacy { }; const SANE: ValidationParams = Self::CONSENSUS.intersect(&ValidationParams::SANE); - fn check_terminal_non_malleable( - frag: &Terminal, - ) -> Result<(), ScriptContextError> { - match *frag { - Terminal::OrI(ref _a, ref _b) => Err(ScriptContextError::MalleableOrI), - Terminal::DupIf(ref _ms) => Err(ScriptContextError::MalleableDupIf), - _ => Ok(()), - } - } - - fn check_global_consensus_validity( - ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - // 1. Check the node first, throw an error on the language itself - let node_checked = match ms.node { - Terminal::MultiA(..) | Terminal::SortedMultiA(..) => { - Err(ScriptContextError::MultiANotAllowed) - } - _ => Ok(()), - }; - // 2. After fragment and param check, validate the script size finally - match node_checked { - Ok(_) => { - if ms.ext.pk_cost > MAX_SCRIPT_ELEMENT_SIZE { - Err(ScriptContextError::MaxRedeemScriptSizeExceeded { - max: MAX_SCRIPT_ELEMENT_SIZE, - got: ms.ext.pk_cost, - }) - } else { - Ok(()) - } - } - Err(_) => node_checked, - } - } - - fn check_local_consensus_validity( - ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - match ms.ext.sat_op_count() { - None => Err(ScriptContextError::ImpossibleSatisfaction), - Some(op_count) if op_count > MAX_OPS_PER_SCRIPT => { - Err(ScriptContextError::MaxOpCountExceeded { - actual: op_count, - limit: MAX_OPS_PER_SCRIPT, - }) - } - _ => Ok(()), - } - } - - fn check_local_policy_validity( - ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - // Legacy scripts permit upto 1000 stack elements, 520 bytes consensus limits - // on P2SH size, it is not possible to reach the 1000 elements limit and hence - // we do not check it. - match ms.max_satisfaction_size() { - Err(_e) => Err(ScriptContextError::ImpossibleSatisfaction), - Ok(size) if size > MAX_SCRIPTSIG_SIZE => { - Err(ScriptContextError::MaxScriptSigSizeExceeded { - actual: size, - limit: MAX_SCRIPTSIG_SIZE, - }) - } - _ => Ok(()), - } - } - fn max_satisfaction_size(ms: &Miniscript) -> Option { ms.ext.sat_data.map(|data| data.max_script_sig_size) } @@ -429,84 +129,6 @@ impl ScriptContext for Segwitv0 { ..Self::CONSENSUS.intersect(&ValidationParams::SANE) }; - fn check_terminal_non_malleable( - _frag: &Terminal, - ) -> Result<(), ScriptContextError> { - Ok(()) - } - - fn check_global_consensus_validity( - ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - // 1. Check the node first, throw an error on the language itself - let node_checked = match ms.node { - Terminal::MultiA(..) | Terminal::SortedMultiA(..) => { - Err(ScriptContextError::MultiANotAllowed) - } - _ => Ok(()), - }; - // 2. After fragment and param check, validate the script size finally - match node_checked { - Ok(_) => { - if ms.ext.pk_cost > MAX_SCRIPT_SIZE { - Err(ScriptContextError::MaxWitnessScriptSizeExceeded { - max: MAX_SCRIPT_SIZE, - got: ms.ext.pk_cost, - }) - } else { - Ok(()) - } - } - Err(_) => node_checked, - } - } - - fn check_local_consensus_validity( - ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - match ms.ext.sat_op_count() { - None => Err(ScriptContextError::ImpossibleSatisfaction), - Some(op_count) if op_count > MAX_OPS_PER_SCRIPT => { - Err(ScriptContextError::MaxOpCountExceeded { - actual: op_count, - limit: MAX_OPS_PER_SCRIPT, - }) - } - _ => Ok(()), - } - } - - fn check_global_policy_validity( - ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - if ms.ext.pk_cost > MAX_STANDARD_P2WSH_SCRIPT_SIZE { - return Err(ScriptContextError::MaxWitnessScriptSizeExceeded { - max: MAX_STANDARD_P2WSH_SCRIPT_SIZE, - got: ms.ext.pk_cost, - }); - } - Ok(()) - } - - fn check_local_policy_validity( - ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - // We don't need to know if this is actually a p2wsh as the standard satisfaction for - // other Segwitv0 defined programs all require (much) less than 100 elements. - // The witness script item is accounted for in max_satisfaction_witness_elements(). - match ms.max_satisfaction_witness_elements() { - // No possible satisfactions - Err(_e) => Err(ScriptContextError::ImpossibleSatisfaction), - Ok(max_witness_items) if max_witness_items > MAX_STANDARD_P2WSH_STACK_ITEMS => { - Err(ScriptContextError::MaxWitnessItemsExceeded { - actual: max_witness_items, - limit: MAX_STANDARD_P2WSH_STACK_ITEMS, - }) - } - _ => Ok(()), - } - } - fn max_satisfaction_size(ms: &Miniscript) -> Option { ms.ext.sat_data.map(|data| data.max_witness_stack_size) } @@ -540,81 +162,6 @@ impl ScriptContext for Tap { ..Self::CONSENSUS.intersect(&ValidationParams::SANE) }; - fn check_terminal_non_malleable( - _frag: &Terminal, - ) -> Result<(), ScriptContextError> { - // No fragment is malleable in tapscript context. - // Certain fragments like Multi are invalid, but are not malleable - Ok(()) - } - - fn check_global_consensus_validity( - ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - // 1. Check the node first, throw an error on the language itself - let node_checked = match ms.node { - Terminal::Multi(..) | Terminal::SortedMulti(..) => { - Err(ScriptContextError::TaprootMultiDisabled) - } - _ => Ok(()), - }; - // 2. After fragment and param check, validate the script size finally - match node_checked { - Ok(_) => { - // No script size checks for global consensus rules - // Should we really check for block limits here. - // When the transaction sizes get close to block limits, - // some guarantees are not easy to satisfy because of knapsack - // constraints - if ms.ext.pk_cost as u64 > Weight::MAX_BLOCK.to_wu() { - Err(ScriptContextError::MaxWitnessScriptSizeExceeded { - max: Weight::MAX_BLOCK.to_wu() as usize, - got: ms.ext.pk_cost, - }) - } else { - Ok(()) - } - } - Err(_) => node_checked, - } - } - - fn check_local_consensus_validity( - ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - // Taproot introduces the concept of sigops budget. - // All valid miniscripts satisfy the sigops constraint - // Whenever we add new fragment that uses pk(pk() or multi based on checksigadd) - // miniscript typing rules ensure that pk when executed successfully has it's - // own unique signature. That is, there is no way to re-use signatures from one CHECKSIG - // to another checksig. In other words, for each successfully executed checksig - // will have it's corresponding 64 bytes signature. - // sigops budget = witness_script.len() + witness.size() + 50 - // Each signature will cover it's own cost(64 > 50) and thus will will never exceed the budget - if let Some(data) = ms.ext.sat_data { - if data.max_witness_stack_count + data.max_exec_stack_count > MAX_STACK_SIZE { - return Err(ScriptContextError::StackSizeLimitExceeded { - actual: data.max_witness_stack_count + data.max_exec_stack_count, - limit: MAX_STACK_SIZE, - }); - } - } - Ok(()) - } - - fn check_global_policy_validity( - _ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - // No script rules, rules are subject to entire tx rules - Ok(()) - } - - fn check_local_policy_validity( - _ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - Ok(()) - } - fn max_satisfaction_size(ms: &Miniscript) -> Option { ms.ext.sat_data.map(|data| data.max_witness_stack_size) } @@ -649,57 +196,6 @@ impl ScriptContext for BareCtx { }; const SANE: ValidationParams = Self::CONSENSUS.intersect(&ValidationParams::SANE); - fn check_terminal_non_malleable( - _frag: &Terminal, - ) -> Result<(), ScriptContextError> { - // Bare fragments can't contain miniscript because of standardness rules - // This function is only used in compiler which already checks the standardness - // and consensus rules, and because of the limited allowance of bare scripts - // we need check for malleable scripts - Ok(()) - } - - fn check_global_consensus_validity( - ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - // 1. Check the node first, throw an error on the language itself - let node_checked = match ms.node { - Terminal::MultiA(..) | Terminal::SortedMultiA(..) => { - Err(ScriptContextError::MultiANotAllowed) - } - _ => Ok(()), - }; - // 2. After fragment and param check, validate the script size finally - match node_checked { - Ok(_) => { - if ms.ext.pk_cost > MAX_SCRIPT_SIZE { - Err(ScriptContextError::MaxBareScriptSizeExceeded { - max: MAX_SCRIPT_SIZE, - got: ms.ext.pk_cost, - }) - } else { - Ok(()) - } - } - Err(_) => node_checked, - } - } - - fn check_local_consensus_validity( - ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - match ms.ext.sat_op_count() { - None => Err(ScriptContextError::ImpossibleSatisfaction), - Some(op_count) if op_count > MAX_OPS_PER_SCRIPT => { - Err(ScriptContextError::MaxOpCountExceeded { - actual: op_count, - limit: MAX_OPS_PER_SCRIPT, - }) - } - _ => Ok(()), - } - } - fn max_satisfaction_size(ms: &Miniscript) -> Option { // For bare outputs the script appears in the scriptpubkey; its cost // is the same as for a legacy scriptsig. @@ -733,36 +229,6 @@ impl ScriptContext for NoChecks { const CONSENSUS: ValidationParams = ValidationParams::MAX; const SANE: ValidationParams = ValidationParams::MAX; - fn check_terminal_non_malleable( - _frag: &Terminal, - ) -> Result<(), ScriptContextError> { - Ok(()) - } - - fn check_global_policy_validity( - _ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - Ok(()) - } - - fn check_global_consensus_validity( - _ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - Ok(()) - } - - fn check_local_policy_validity( - _ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - Ok(()) - } - - fn check_local_consensus_validity( - _ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - Ok(()) - } - fn max_satisfaction_size(_ms: &Miniscript) -> Option { panic!("Tried to compute a satisfaction size bound on a no-checks ecdsa miniscript") } @@ -776,24 +242,6 @@ impl ScriptContext for NoChecks { "NochecksEcdsa" } - fn check_global_validity( - ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - Self::check_global_consensus_validity(ms)?; - Self::check_global_policy_validity(ms)?; - Ok(()) - } - - fn check_local_validity( - ms: &Miniscript, - ) -> Result<(), ScriptContextError> { - Self::check_global_consensus_validity(ms)?; - Self::check_global_policy_validity(ms)?; - Self::check_local_consensus_validity(ms)?; - Self::check_local_policy_validity(ms)?; - Ok(()) - } - fn sig_type() -> SigType { SigType::Ecdsa } } diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs index 6b09c5ad5..cd2fc5e0b 100644 --- a/src/miniscript/mod.rs +++ b/src/miniscript/mod.rs @@ -23,7 +23,7 @@ use crate::iter::TreeLike; use crate::prelude::*; use crate::{script_num_size, TranslateErr}; -pub mod analyzable; +mod analyzable; pub mod astelem; pub(crate) mod context; pub mod decode; @@ -337,7 +337,7 @@ mod private { if (res.ext.tree_height as u32) > MAX_RECURSION_DEPTH { return Err(Error::MaxRecursiveDepthExceeded); } - Ctx::check_global_validity(&res)?; + Ok(res) } @@ -745,7 +745,6 @@ impl Miniscript { let mut iter = TokenIter::new(tokens); let top = decode::decode(&mut iter)?; - Ctx::check_global_validity(&top)?; types::Type::type_check(&top.node)?; if let Some(leading) = iter.next() { Err(Error::Trailing(leading.to_string())) @@ -1219,13 +1218,6 @@ impl FromTree for Miniscript { assert_eq!(stack.len(), 1); let ret = stack.pop().unwrap(); - // Iterate through every node to check global validity. It is definitely not sufficient - // to check only the root, since this will fail to notice illegal xonly keys at the - // leaves. But probably checking every single node is overkill. This may be worth - // optimizing. - for node in ret.pre_order_iter() { - Ctx::check_global_validity(node)?; - } Ok(Arc::try_unwrap(ret).unwrap()) } } @@ -1806,11 +1798,11 @@ mod tests { let segwit_sortedmulti_a_ms = Segwitv0Ms::from_str_insane("sortedmulti_a(1,A,B,C)"); assert_eq!( segwit_multi_a_ms.unwrap_err().to_string(), - "Multi a(CHECKSIGADD) only allowed post tapscript" + "non-Taproot script with a `multi_a` fragment", ); assert_eq!( segwit_sortedmulti_a_ms.unwrap_err().to_string(), - "Multi a(CHECKSIGADD) only allowed post tapscript" + "non-Taproot script with a `multi_a` fragment" ); let tap_multi_a_ms = TapMs::from_str_insane("multi_a(1,A,B,C)").unwrap(); let tap_sortedmulti_a_ms = TapMs::from_str_insane("sortedmulti_a(1,A,B,C)").unwrap(); @@ -2154,13 +2146,15 @@ mod tests { type BareMs = Miniscript; // multisig script of 20 pubkeys exceeds 520 bytes - let pubkey_vec_20: Vec = (0..20).map(|x| x.to_string()).collect(); + let pubkey_vec_20: Vec> = (0..15) + .map(|n| (n * 20..(n + 1) * 20).map(|x| x.to_string()).collect()) + .collect(); // multisig script of 300 pubkeys exceeds 10,000 bytes let pubkey_vec_300: Vec = (0..300).map(|x| x.to_string()).collect(); // wrong multi_a for non-tapscript, while exceeding consensus size limit let legacy_multi_a_ms = - LegacyMs::from_str(&format!("multi_a(20,{})", pubkey_vec_20.join(","))); + LegacyMs::from_str(&format!("multi_a(20,{})", pubkey_vec_20[0].join(","))); let segwit_multi_a_ms = Segwitv0Ms::from_str(&format!("multi_a(300,{})", pubkey_vec_300.join(","))); let bare_multi_a_ms = @@ -2169,41 +2163,45 @@ mod tests { // Should panic for wrong multi_a, even if it exceeds the max consensus size assert_eq!( legacy_multi_a_ms.unwrap_err().to_string(), - "Multi a(CHECKSIGADD) only allowed post tapscript" + "non-Taproot script with a `multi_a` fragment", ); assert_eq!( segwit_multi_a_ms.unwrap_err().to_string(), - "Multi a(CHECKSIGADD) only allowed post tapscript" + "non-Taproot script with a `multi_a` fragment", ); assert_eq!( bare_multi_a_ms.unwrap_err().to_string(), - "Multi a(CHECKSIGADD) only allowed post tapscript" + "non-Taproot script with a `multi_a` fragment", ); // multisig script of 20 pubkeys exceeds 520 bytes - let multi_ms = format!("multi(20,{})", pubkey_vec_20.join(",")); + let multi_ms: Vec = pubkey_vec_20 + .iter() + .map(|vec_20| format!("multi(20,{})", vec_20.join(","))) + .collect(); // other than legacy, and_v to build 15 nested 20-of-20 multisig script // to exceed 10,000 bytes without violation of threshold limit(max: 20) let and_v_nested_multi_ms = - format!("and_v(v:{},", multi_ms).repeat(14) + &multi_ms + "))))))))))))))"; + format!("and_v(v:{},{}", multi_ms[..14].join(",and_v(v:"), multi_ms[14]) + + "))))))))))))))"; // correct multi for non-tapscript, but exceeding consensus size limit - let legacy_multi_ms = LegacyMs::from_str(&multi_ms); + let legacy_multi_ms = LegacyMs::from_str(&multi_ms[0]); let segwit_multi_ms = Segwitv0Ms::from_str(&and_v_nested_multi_ms); let bare_multi_ms = BareMs::from_str(&and_v_nested_multi_ms); // Should panic for exceeding the max consensus size, as multi properly used assert_eq!( legacy_multi_ms.unwrap_err().to_string(), - "The Miniscript corresponding Script cannot be larger than 520 bytes, but got 685 bytes." + "script has size at least 685 (limit: 520).", ); assert_eq!( segwit_multi_ms.unwrap_err().to_string(), - "The Miniscript corresponding Script cannot be larger than 3600 bytes, but got 4110 bytes." + "script has size at least 10275 (limit: 3600).", ); assert_eq!( bare_multi_ms.unwrap_err().to_string(), - "The Miniscript corresponding Script cannot be larger than 10000 bytes, but got 10275 bytes." + "script has size at least 10275 (limit: 10000).", ); } } diff --git a/src/policy/compiler.rs b/src/policy/compiler.rs index d6bc02878..0a7d02479 100644 --- a/src/policy/compiler.rs +++ b/src/policy/compiler.rs @@ -662,14 +662,16 @@ fn insert_elem( sat_prob: f64, dissat_prob: Option, ) -> bool { - // We check before compiling that non-malleable satisfactions exist, and it appears that - // there are no cases when malleable satisfactions beat non-malleable ones (and if there - // are, we don't want to use them). Anyway, detect these and early return. - if !elem.ms.ty.mall.non_malleable { - return false; - } - - if Ctx::check_local_validity(&elem.ms).is_err() { + // Disable checks which were done on the policy before compilation. + if elem + .ms + .validate_non_top_level(&crate::ValidationParams { + allow_duplicate_keys: true, + allow_mixed_time_locks: true, + ..Ctx::SANE + }) + .is_err() + { return false; }