From 2f667789a72894ac82a3e5a23d5f1b968230f09d Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Wed, 6 Mar 2024 13:42:33 +0000 Subject: [PATCH 01/10] types: replace closure with iterator Simplifies the API and reduces copying. --- src/miniscript/types/correctness.rs | 7 +++---- src/miniscript/types/malleability.rs | 9 +++++---- src/miniscript/types/mod.rs | 10 +++++----- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/miniscript/types/correctness.rs b/src/miniscript/types/correctness.rs index ad7597f5f..2fc490d42 100644 --- a/src/miniscript/types/correctness.rs +++ b/src/miniscript/types/correctness.rs @@ -467,13 +467,12 @@ impl Correctness { /// Constructor for the correctness properties of the `thresh` fragment // Cannot be constfn because it takes a closure. - pub fn threshold(_k: usize, n: usize, mut sub_ck: S) -> Result + pub fn threshold<'a, I>(_k: usize, subs: I) -> Result where - S: FnMut(usize) -> Self, + I: Iterator, { let mut num_args = 0; - for i in 0..n { - let subtype = sub_ck(i); + for (i, subtype) in subs.enumerate() { num_args += match subtype.input { Input::Zero => 0, Input::One | Input::OneNonZero => 1, diff --git a/src/miniscript/types/malleability.rs b/src/miniscript/types/malleability.rs index 4285e2ac3..2658e4699 100644 --- a/src/miniscript/types/malleability.rs +++ b/src/miniscript/types/malleability.rs @@ -278,19 +278,20 @@ impl Malleability { /// Constructor for the malleabilitiy properties of the `thresh` fragment. // Cannot be constfn because it takes a closure. - pub fn threshold(k: usize, n: usize, mut sub_ck: S) -> Self + pub fn threshold<'a, I>(k: usize, subs: I) -> Self where - S: FnMut(usize) -> Self, + I: ExactSizeIterator, { + let n = subs.len(); let mut safe_count = 0; let mut all_are_dissat_unique = true; let mut all_are_non_malleable = true; - for i in 0..n { - let subtype = sub_ck(i); + for subtype in subs { safe_count += usize::from(subtype.safe); all_are_dissat_unique &= subtype.dissat == Dissat::Unique; all_are_non_malleable &= subtype.non_malleable; } + Malleability { dissat: if all_are_dissat_unique && safe_count == n { Dissat::Unique diff --git a/src/miniscript/types/mod.rs b/src/miniscript/types/mod.rs index 887aa8b8d..113e1cd73 100644 --- a/src/miniscript/types/mod.rs +++ b/src/miniscript/types/mod.rs @@ -464,13 +464,13 @@ impl Type { /// Constructor for the type of the `thresh` fragment. // Cannot be a constfn because it takes a closure. - pub fn threshold(k: usize, n: usize, mut sub_ck: S) -> Result + pub fn threshold<'a, I>(k: usize, subs: I) -> Result where - S: FnMut(usize) -> Self, + I: Clone + ExactSizeIterator, { Ok(Type { - corr: Correctness::threshold(k, n, |n| sub_ck(n).corr)?, - mall: Malleability::threshold(k, n, |n| sub_ck(n).mall), + corr: Correctness::threshold(k, subs.clone().map(|s| &s.corr))?, + mall: Malleability::threshold(k, subs.map(|s| &s.mall)), }) } } @@ -593,7 +593,7 @@ impl Type { }); } - let res = Self::threshold(k, subs.len(), |n| subs[n].ty); + let res = Self::threshold(k, subs.iter().map(|ms| &ms.ty)); res.map_err(|kind| Error { fragment_string: fragment.to_string(), error: kind }) } From 60d7c98db16a3c41fb9fabdd53545e40b64826e9 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Wed, 6 Mar 2024 13:51:59 +0000 Subject: [PATCH 02/10] types: drop unused "strongness" errors These are unused since PR #13. We don't even use the term "strong" anymore in Miniscript. --- src/miniscript/types/mod.rs | 33 --------------------------------- 1 file changed, 33 deletions(-) diff --git a/src/miniscript/types/mod.rs b/src/miniscript/types/mod.rs index 113e1cd73..dfb99e351 100644 --- a/src/miniscript/types/mod.rs +++ b/src/miniscript/types/mod.rs @@ -34,12 +34,6 @@ pub enum ErrorKind { /// Multisignature or threshold policy has a `k` value in /// excess of the number of subfragments OverThreshold(usize, usize), - /// Attempted to construct a disjunction (or `andor`) for which - /// none of the child nodes were strong. This means that a 3rd - /// party could produce a satisfaction for any branch, meaning - /// that no matter which one an honest signer chooses, it is - /// possible to malleate the transaction. - NoStrongChild, /// Many fragments (all disjunctions except `or_i` as well as /// `andor` require their left child be dissatisfiable. LeftNotDissatisfiable, @@ -71,15 +65,6 @@ pub enum ErrorKind { ThresholdDissat(usize), /// The nth child of a threshold fragment was not a unit ThresholdNonUnit(usize), - /// Insufficiently many children of a threshold fragment were strong - ThresholdNotStrong { - /// Threshold parameter - k: usize, - /// Number of children - n: usize, - /// Number of strong children - n_strong: usize, - }, } /// Error type for typechecking @@ -115,13 +100,6 @@ impl fmt::Display for Error { make sense", self.fragment_string, k, n, ), - ErrorKind::NoStrongChild => write!( - f, - "fragment «{}» requires at least one strong child \ - (a 3rd party cannot create a witness without having \ - seen one before) to prevent malleability", - self.fragment_string, - ), ErrorKind::LeftNotDissatisfiable => write!( f, "fragment «{}» requires its left child be dissatisfiable", @@ -185,17 +163,6 @@ impl fmt::Display for Error { exactly 1 on the stack given a satisfying input)", self.fragment_string, idx, ), - ErrorKind::ThresholdNotStrong { k, n, n_strong } => write!( - f, - "fragment «{}» is a {}-of-{} threshold, and needs {} of \ - its children to be strong to prevent malleability; however \ - only {} children were strong.", - self.fragment_string, - k, - n, - n - k, - n_strong, - ), } } } From 950b79389502b045b603903989b8d58c2382cd74 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Wed, 6 Mar 2024 14:25:14 +0000 Subject: [PATCH 03/10] AbsLockTime: move to module, consolidate error checking We have the type `AbsLockTime` whose purpose is to provide an ordering for locktimes. This type is converted from strings and script values, but we don't check validity during construction. Instead we have multiple places where validity is manually checked. Fix this. Note that this is an API-breaking change for people who are manually constructing policies and Miniscripts since it removes a few ways to unconditionally convert u32s into locktimes. --- src/lib.rs | 55 +++-------------- src/miniscript/astelem.rs | 6 +- src/miniscript/decode.rs | 2 +- src/miniscript/mod.rs | 4 +- src/miniscript/types/extra_props.rs | 19 ++---- src/miniscript/types/mod.rs | 15 +---- src/policy/compiler.rs | 21 ++----- src/policy/concrete.rs | 28 ++------- src/policy/mod.rs | 4 +- src/policy/semantic.rs | 30 +++------- src/primitives/absolute_locktime.rs | 91 +++++++++++++++++++++++++++++ src/primitives/mod.rs | 14 +++++ 12 files changed, 148 insertions(+), 141 deletions(-) create mode 100644 src/primitives/absolute_locktime.rs create mode 100644 src/primitives/mod.rs diff --git a/src/lib.rs b/src/lib.rs index 1360bdbfe..c8b079070 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -87,6 +87,7 @@ #![deny(missing_docs)] // Clippy lints that we have disabled #![allow(clippy::iter_kv_map)] // https://github.com/rust-lang/rust-clippy/issues/11752 +#![allow(clippy::manual_range_contains)] // I hate this lint -asp #[cfg(target_pointer_width = "16")] compile_error!( @@ -125,19 +126,19 @@ pub mod iter; pub mod miniscript; pub mod plan; pub mod policy; +mod primitives; pub mod psbt; #[cfg(test)] mod test_utils; mod util; -use core::{cmp, fmt, hash, str}; +use core::{fmt, hash, str}; #[cfg(feature = "std")] use std::error; use bitcoin::hashes::{hash160, ripemd160, sha256, Hash}; use bitcoin::hex::DisplayHex; -use bitcoin::locktime::absolute; use bitcoin::{script, Opcode}; pub use crate::blanket_traits::FromStrKey; @@ -149,6 +150,7 @@ pub use crate::miniscript::decode::Terminal; pub use crate::miniscript::satisfy::{Preimage32, Satisfier}; pub use crate::miniscript::{hash256, Miniscript}; use crate::prelude::*; +pub use crate::primitives::absolute_locktime::{AbsLockTime, AbsLockTimeError}; /// Public key trait which can be converted to Hash type pub trait MiniscriptKey: Clone + Eq + Ord + fmt::Debug + fmt::Display + hash::Hash { @@ -501,6 +503,8 @@ pub enum Error { /// At least two BIP389 key expressions in the descriptor contain tuples of /// derivation indexes of different lengths. MultipathDescLenMismatch, + /// Invalid absolute locktime + AbsoluteLockTime(AbsLockTimeError), } // https://github.com/sipa/miniscript/pull/5 for discussion on this number @@ -576,6 +580,7 @@ impl fmt::Display for Error { Error::TrNoScriptCode => write!(f, "No script code for Tr descriptors"), Error::TrNoExplicitScript => write!(f, "No script code for Tr descriptors"), Error::MultipathDescLenMismatch => write!(f, "At least two BIP389 key expressions in the descriptor contain tuples of derivation indexes of different lengths"), + Error::AbsoluteLockTime(ref e) => e.fmt(f), } } } @@ -628,6 +633,7 @@ impl error::Error for Error { ContextError(e) => Some(e), AnalysisError(e) => Some(e), PubKeyCtxError(e, _) => Some(e), + AbsoluteLockTime(e) => Some(e), } } } @@ -711,51 +717,6 @@ fn hex_script(s: &str) -> bitcoin::ScriptBuf { bitcoin::ScriptBuf::from(v) } -/// An absolute locktime that implements `Ord`. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] -pub struct AbsLockTime(absolute::LockTime); - -impl AbsLockTime { - /// Constructs an `AbsLockTime` from an nLockTime value or the argument to OP_CHEKCLOCKTIMEVERIFY. - pub fn from_consensus(n: u32) -> Self { Self(absolute::LockTime::from_consensus(n)) } - - /// Returns the inner `u32` value. This is the value used when creating this `LockTime` - /// i.e., `n OP_CHECKLOCKTIMEVERIFY` or nLockTime. - /// - /// This calls through to `absolute::LockTime::to_consensus_u32()` and the same usage warnings - /// apply. - pub fn to_consensus_u32(self) -> u32 { self.0.to_consensus_u32() } - - /// Returns the inner `u32` value. - /// - /// Equivalent to `AbsLockTime::to_consensus_u32()`. - pub fn to_u32(self) -> u32 { self.to_consensus_u32() } -} - -impl From for AbsLockTime { - fn from(lock_time: absolute::LockTime) -> Self { Self(lock_time) } -} - -impl From for absolute::LockTime { - fn from(lock_time: AbsLockTime) -> absolute::LockTime { lock_time.0 } -} - -impl cmp::PartialOrd for AbsLockTime { - fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) } -} - -impl cmp::Ord for AbsLockTime { - fn cmp(&self, other: &Self) -> cmp::Ordering { - let this = self.0.to_consensus_u32(); - let that = other.0.to_consensus_u32(); - this.cmp(&that) - } -} - -impl fmt::Display for AbsLockTime { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fmt::Display::fmt(&self.0, f) } -} - #[cfg(test)] mod tests { use core::str::FromStr; diff --git a/src/miniscript/astelem.rs b/src/miniscript/astelem.rs index db9d068f1..0364765af 100644 --- a/src/miniscript/astelem.rs +++ b/src/miniscript/astelem.rs @@ -256,9 +256,9 @@ impl crate::expression::FromTree for Termina expression::terminal(&top.args[0], |x| Pk::from_str(x).map(Terminal::PkH)) } ("after", 1) => expression::terminal(&top.args[0], |x| { - expression::parse_num(x).map(|x| { - Terminal::After(AbsLockTime::from(absolute::LockTime::from_consensus(x))) - }) + expression::parse_num(x) + .and_then(|x| AbsLockTime::from_consensus(x).map_err(Error::AbsoluteLockTime)) + .map(Terminal::After) }), ("older", 1) => expression::terminal(&top.args[0], |x| { expression::parse_num(x).map(|x| Terminal::Older(Sequence::from_consensus(x))) diff --git a/src/miniscript/decode.rs b/src/miniscript/decode.rs index a058cf4b4..920702e84 100644 --- a/src/miniscript/decode.rs +++ b/src/miniscript/decode.rs @@ -359,7 +359,7 @@ pub fn parse( Tk::CheckSequenceVerify, Tk::Num(n) => term.reduce0(Terminal::Older(Sequence::from_consensus(n)))?, Tk::CheckLockTimeVerify, Tk::Num(n) - => term.reduce0(Terminal::After(AbsLockTime::from_consensus(n)))?, + => term.reduce0(Terminal::After(AbsLockTime::from_consensus(n).map_err(Error::AbsoluteLockTime)?))?, // hashlocks Tk::Equal => match_token!( tokens, diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs index f73b41a5f..12ab66799 100644 --- a/src/miniscript/mod.rs +++ b/src/miniscript/mod.rs @@ -1370,7 +1370,7 @@ mod tests { (format!("t:or_c(pk({}),v:pkh({}))", key_present, key_missing), None, None), ( format!("thresh(2,pk({}),s:pk({}),snl:after(1))", key_present, key_missing), - Some(AbsLockTime::from_consensus(1)), + Some(AbsLockTime::from_consensus(1).unwrap()), None, ), ( @@ -1388,7 +1388,7 @@ mod tests { "thresh(3,pk({}),s:pk({}),snl:older(10),snl:after(11))", key_present, key_missing ), - Some(AbsLockTime::from_consensus(11)), + Some(AbsLockTime::from_consensus(11).unwrap()), Some(bitcoin::Sequence(10)), ), ( diff --git a/src/miniscript/types/extra_props.rs b/src/miniscript/types/extra_props.rs index 9d8c9274c..2bbf5a05a 100644 --- a/src/miniscript/types/extra_props.rs +++ b/src/miniscript/types/extra_props.rs @@ -6,12 +6,12 @@ use core::cmp; use core::iter::once; -use bitcoin::{absolute, Sequence}; +use bitcoin::Sequence; use super::{Error, ErrorKind, ScriptContext}; use crate::miniscript::context::SigType; use crate::prelude::*; -use crate::{script_num_size, MiniscriptKey, Terminal}; +use crate::{script_num_size, AbsLockTime, MiniscriptKey, Terminal}; /// Timelock information for satisfaction of a fragment. #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Debug, Default, Hash)] @@ -343,7 +343,7 @@ impl ExtData { } /// Extra properties for the `after` fragment. - pub fn after(t: absolute::LockTime) -> Self { + pub fn after(t: AbsLockTime) -> Self { ExtData { pk_cost: script_num_size(t.to_consensus_u32() as usize) + 1, has_free_verify: false, @@ -923,18 +923,7 @@ impl ExtData { _ => unreachable!(), } } - Terminal::After(t) => { - // Note that for CLTV this is a limitation not of Bitcoin but Miniscript. The - // number on the stack would be a 5 bytes signed integer but Miniscript's B type - // only consumes 4 bytes from the stack. - if t == absolute::LockTime::ZERO.into() { - return Err(Error { - fragment_string: fragment.to_string(), - error: ErrorKind::InvalidTime, - }); - } - Self::after(t.into()) - } + Terminal::After(t) => Self::after(t), Terminal::Older(t) => { if t == Sequence::ZERO || !t.is_relative_lock_time() { return Err(Error { diff --git a/src/miniscript/types/mod.rs b/src/miniscript/types/mod.rs index dfb99e351..05e185767 100644 --- a/src/miniscript/types/mod.rs +++ b/src/miniscript/types/mod.rs @@ -14,7 +14,7 @@ use core::fmt; #[cfg(feature = "std")] use std::error; -use bitcoin::{absolute, Sequence}; +use bitcoin::Sequence; pub use self::correctness::{Base, Correctness, Input}; pub use self::extra_props::ExtData; @@ -478,18 +478,7 @@ impl Type { _ => unreachable!(), } } - Terminal::After(t) => { - // Note that for CLTV this is a limitation not of Bitcoin but Miniscript. The - // number on the stack would be a 5 bytes signed integer but Miniscript's B type - // only consumes 4 bytes from the stack. - if t == absolute::LockTime::ZERO.into() { - return Err(Error { - fragment_string: fragment.to_string(), - error: ErrorKind::InvalidTime, - }); - } - Ok(Self::time()) - } + Terminal::After(_) => Ok(Self::time()), Terminal::Older(t) => { if t == Sequence::ZERO || !t.is_relative_lock_time() { return Err(Error { diff --git a/src/policy/compiler.rs b/src/policy/compiler.rs index c24946d57..0989e564d 100644 --- a/src/policy/compiler.rs +++ b/src/policy/compiler.rs @@ -9,7 +9,7 @@ use core::{cmp, f64, fmt, hash, mem}; #[cfg(feature = "std")] use std::error; -use bitcoin::{absolute, Sequence}; +use bitcoin::Sequence; use sync::Arc; use crate::miniscript::context::SigType; @@ -446,18 +446,7 @@ impl CompilerExtData { _ => unreachable!(), } } - Terminal::After(t) => { - // Note that for CLTV this is a limitation not of Bitcoin but Miniscript. The - // number on the stack would be a 5 bytes signed integer but Miniscript's B type - // only consumes 4 bytes from the stack. - if t == absolute::LockTime::ZERO.into() { - return Err(types::Error { - fragment_string: fragment.to_string(), - error: types::ErrorKind::InvalidTime, - }); - } - Ok(Self::time()) - } + Terminal::After(_) => Ok(Self::time()), Terminal::Older(t) => { if t == Sequence::ZERO || !t.is_relative_lock_time() { return Err(types::Error { @@ -1265,7 +1254,7 @@ mod tests { use super::*; use crate::miniscript::{Legacy, Segwitv0, Tap}; use crate::policy::Liftable; - use crate::{script_num_size, ToPublicKey}; + use crate::{script_num_size, AbsLockTime, ToPublicKey}; type SPolicy = Concrete; type BPolicy = Concrete; @@ -1311,8 +1300,8 @@ mod tests { let pol: SPolicy = Concrete::And(vec![ Arc::new(Concrete::Key("A".to_string())), Arc::new(Concrete::And(vec![ - Arc::new(Concrete::after(9)), - Arc::new(Concrete::after(1000_000_000)), + 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()); diff --git a/src/policy/concrete.rs b/src/policy/concrete.rs index bfb40a85f..4908011a1 100644 --- a/src/policy/concrete.rs +++ b/src/policy/concrete.rs @@ -74,12 +74,6 @@ impl Policy where Pk: MiniscriptKey, { - /// Construct a `Policy::After` from `n`. Helper function equivalent to - /// `Policy::After(absolute::LockTime::from_consensus(n))`. - pub fn after(n: u32) -> Policy { - Policy::After(AbsLockTime::from(absolute::LockTime::from_consensus(n))) - } - /// Construct a `Policy::Older` from `n`. Helper function equivalent to /// `Policy::Older(Sequence::from_consensus(n))`. pub fn older(n: u32) -> Policy { Policy::Older(Sequence::from_consensus(n)) } @@ -756,13 +750,7 @@ impl Policy { for policy in self.pre_order_iter() { match *policy { - After(n) => { - if n == absolute::LockTime::ZERO.into() { - return Err(PolicyError::ZeroTime); - } else if n.to_u32() > 2u32.pow(31) { - return Err(PolicyError::TimeTooFar); - } - } + After(_) => {} Older(n) => { if n == Sequence::ZERO { return Err(PolicyError::ZeroTime); @@ -978,15 +966,11 @@ impl Policy { ("UNSATISFIABLE", 0) => Ok(Policy::Unsatisfiable), ("TRIVIAL", 0) => Ok(Policy::Trivial), ("pk", 1) => expression::terminal(&top.args[0], |pk| Pk::from_str(pk).map(Policy::Key)), - ("after", 1) => { - let num = expression::terminal(&top.args[0], expression::parse_num)?; - if num > 2u32.pow(31) { - return Err(Error::PolicyError(PolicyError::TimeTooFar)); - } else if num == 0 { - return Err(Error::PolicyError(PolicyError::ZeroTime)); - } - Ok(Policy::after(num)) - } + ("after", 1) => expression::terminal(&top.args[0], |x| { + expression::parse_num(x) + .and_then(|x| AbsLockTime::from_consensus(x).map_err(Error::AbsoluteLockTime)) + .map(Policy::After) + }), ("older", 1) => { let num = expression::terminal(&top.args[0], expression::parse_num)?; if num > 2u32.pow(31) { diff --git a/src/policy/mod.rs b/src/policy/mod.rs index 8eb24b617..d6525b201 100644 --- a/src/policy/mod.rs +++ b/src/policy/mod.rs @@ -323,11 +323,13 @@ mod tests { ConcretePol::from_str("or(pk())").unwrap_err().to_string(), "Or policy fragment must take 2 arguments" ); + // this weird "unexpected" wrapping of the error will go away in a later PR + // which rewrites the expression parser assert_eq!( ConcretePol::from_str("thresh(3,after(0),pk(),pk())") .unwrap_err() .to_string(), - "Time must be greater than 0; n > 0" + "unexpected «absolute locktimes in Miniscript have a minimum value of 1»", ); assert_eq!( diff --git a/src/policy/semantic.rs b/src/policy/semantic.rs index 8b00d0bd8..c6e0f5a5f 100644 --- a/src/policy/semantic.rs +++ b/src/policy/semantic.rs @@ -53,13 +53,6 @@ impl Policy where Pk: MiniscriptKey, { - /// Constructs a `Policy::After` from `n`. - /// - /// Helper function equivalent to `Policy::After(absolute::LockTime::from_consensus(n))`. - pub fn after(n: u32) -> Policy { - Policy::After(AbsLockTime::from(absolute::LockTime::from_consensus(n))) - } - /// Construct a `Policy::Older` from `n`. /// /// Helper function equivalent to `Policy::Older(Sequence::from_consensus(n))`. @@ -329,7 +322,9 @@ impl expression::FromTree for Policy { ("TRIVIAL", 0) => Ok(Policy::Trivial), ("pk", 1) => expression::terminal(&top.args[0], |pk| Pk::from_str(pk).map(Policy::Key)), ("after", 1) => expression::terminal(&top.args[0], |x| { - expression::parse_num(x).map(|x| Policy::after(x)) + expression::parse_num(x) + .and_then(|x| AbsLockTime::from_consensus(x).map_err(Error::AbsoluteLockTime)) + .map(Policy::After) }), ("older", 1) => expression::terminal(&top.args[0], |x| { expression::parse_num(x).map(|x| Policy::older(x)) @@ -500,7 +495,7 @@ impl Policy { fn real_absolute_timelocks(&self) -> Vec { self.pre_order_iter() .filter_map(|policy| match policy { - Policy::After(t) => Some(t.to_u32()), + Policy::After(t) => Some(t.to_consensus_u32()), _ => None, }) .collect() @@ -555,7 +550,6 @@ impl Policy { /// Filters a policy by eliminating absolute timelock constraints /// that are not satisfied at the given `n` (`n OP_CHECKLOCKTIMEVERIFY`). pub fn at_lock_time(self, n: absolute::LockTime) -> Policy { - use absolute::LockTime::*; use Policy::*; let mut at_age = vec![]; @@ -564,16 +558,10 @@ impl Policy { let new_policy = match data.node.as_ref() { After(t) => { - let t = absolute::LockTime::from(*t); - let is_satisfied_by = match (t, n) { - (Blocks(t), Blocks(n)) => t <= n, - (Seconds(t), Seconds(n)) => t <= n, - _ => false, - }; - if !is_satisfied_by { - Some(Unsatisfiable) + if absolute::LockTime::from(*t).is_implied_by(n) { + Some(After(*t)) } else { - Some(After(t.into())) + Some(Unsatisfiable) } } Threshold(k, ref subs) => { @@ -857,7 +845,7 @@ mod tests { // Block height 1000. let policy = StringPolicy::from_str("after(1000)").unwrap(); - assert_eq!(policy, Policy::after(1000)); + assert_eq!(policy, Policy::After(AbsLockTime::from_consensus(1000).unwrap())); assert_eq!(policy.absolute_timelocks(), vec![1000]); assert_eq!(policy.relative_timelocks(), vec![]); assert_eq!(policy.clone().at_lock_time(absolute::LockTime::ZERO), Policy::Unsatisfiable); @@ -891,7 +879,7 @@ mod tests { // UNIX timestamp of 10 seconds after the epoch. let policy = StringPolicy::from_str("after(500000010)").unwrap(); - assert_eq!(policy, Policy::after(500_000_010)); + assert_eq!(policy, Policy::After(AbsLockTime::from_consensus(500_000_010).unwrap())); assert_eq!(policy.absolute_timelocks(), vec![500_000_010]); assert_eq!(policy.relative_timelocks(), vec![]); // Pass a block height to at_lock_time while policy uses a UNIX timestapm. diff --git a/src/primitives/absolute_locktime.rs b/src/primitives/absolute_locktime.rs new file mode 100644 index 000000000..e2373d781 --- /dev/null +++ b/src/primitives/absolute_locktime.rs @@ -0,0 +1,91 @@ +// SPDX-License-Identifier: CC0-1.0 + +//! Absolute Locktimes + +use core::{cmp, fmt}; + +use bitcoin::absolute; + +/// Maximum allowed absolute locktime value. +pub const MAX_ABSOLUTE_LOCKTIME: u32 = 0x8000_0000; + +/// Minimum allowed absolute locktime value. +/// +/// In Bitcoin 0 is an allowed value, but in Miniscript it is not, because we +/// (ab)use the locktime value as a boolean in our Script fragments, and avoiding +/// this would reduce efficiency. +pub const MIN_ABSOLUTE_LOCKTIME: u32 = 1; + +/// Error parsing an absolute locktime. +#[derive(Debug, PartialEq)] +pub struct AbsLockTimeError { + value: u32, +} + +impl fmt::Display for AbsLockTimeError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + if self.value < MIN_ABSOLUTE_LOCKTIME { + f.write_str("absolute locktimes in Miniscript have a minimum value of 1") + } else { + debug_assert!(self.value > MAX_ABSOLUTE_LOCKTIME); + write!( + f, + "absolute locktimes in Miniscript have a maximum value of 0x{:08x}; got 0x{:08x}", + MAX_ABSOLUTE_LOCKTIME, self.value + ) + } + } +} + +#[cfg(feature = "std")] +impl std::error::Error for AbsLockTimeError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { None } +} + +/// An absolute locktime that implements `Ord`. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub struct AbsLockTime(absolute::LockTime); + +impl AbsLockTime { + /// Constructs an `AbsLockTime` from an nLockTime value or the argument to `CHEKCLOCKTIMEVERIFY`. + pub fn from_consensus(n: u32) -> Result { + if n >= MIN_ABSOLUTE_LOCKTIME && n <= MAX_ABSOLUTE_LOCKTIME { + Ok(AbsLockTime(absolute::LockTime::from_consensus(n))) + } else { + Err(AbsLockTimeError { value: n }) + } + } + + /// Returns the inner `u32` value. This is the value used when creating this `LockTime` + /// i.e., `n OP_CHECKLOCKTIMEVERIFY` or nLockTime. + /// + /// This calls through to `absolute::LockTime::to_consensus_u32()` and the same usage warnings + /// apply. + pub fn to_consensus_u32(self) -> u32 { self.0.to_consensus_u32() } + + /// Whether this is a height-based locktime. + pub fn is_block_height(&self) -> bool { self.0.is_block_height() } + + /// Whether this is a time-based locktime. + pub fn is_block_time(&self) -> bool { self.0.is_block_time() } +} + +impl From for absolute::LockTime { + fn from(lock_time: AbsLockTime) -> absolute::LockTime { lock_time.0 } +} + +impl cmp::PartialOrd for AbsLockTime { + fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) } +} + +impl cmp::Ord for AbsLockTime { + fn cmp(&self, other: &Self) -> cmp::Ordering { + let this = self.0.to_consensus_u32(); + let that = other.0.to_consensus_u32(); + this.cmp(&that) + } +} + +impl fmt::Display for AbsLockTime { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fmt::Display::fmt(&self.0, f) } +} diff --git a/src/primitives/mod.rs b/src/primitives/mod.rs new file mode 100644 index 000000000..b05d777a1 --- /dev/null +++ b/src/primitives/mod.rs @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: CC0-1.0 + +//! Primitive Types +//! +//! In Miniscript we have a few types which have stronger constraints than +//! their equivalents in Bitcoin (or Rust). In particular, locktimes which +//! appear in `after` and `older` fragments are constrained to be nonzero, +//! and the relative locktimes in `older` fragments are only allowed to be +//! the subset of sequence numbers which form valid locktimes. +//! +//! This module exists for code organization and any types defined here +//! should be re-exported at the crate root. + +pub mod absolute_locktime; From bd6ef65f8697501b3798cabe9ab0b6a5aa01b761 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Wed, 6 Mar 2024 18:41:29 +0000 Subject: [PATCH 04/10] interpreter: rename "age" to "sequence" The "age" from the transaction is actually just a sequence number. Whether it is interpreted as an "age" depends on various other factors, as described in BIP112. In the next commits we will start moving much of our `Sequence` usage to `relative::LockTime` and it will be helpful to have clearer names. --- src/interpreter/mod.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs index e54a79fee..8f66d2d26 100644 --- a/src/interpreter/mod.rs +++ b/src/interpreter/mod.rs @@ -35,7 +35,7 @@ pub struct Interpreter<'txin> { /// For non-Taproot spends, the scriptCode; for Taproot script-spends, this /// is the leaf script; for key-spends it is `None`. script_code: Option, - age: Sequence, + sequence: Sequence, lock_time: absolute::LockTime, } @@ -136,11 +136,11 @@ impl<'txin> Interpreter<'txin> { spk: &bitcoin::ScriptBuf, script_sig: &'txin bitcoin::Script, witness: &'txin Witness, - age: Sequence, // CSV, relative lock time. + sequence: Sequence, // CSV, relative lock time. lock_time: absolute::LockTime, // CLTV, absolute lock time. ) -> Result { let (inner, stack, script_code) = inner::from_txdata(spk, script_sig, witness)?; - Ok(Interpreter { inner, stack, script_code, age, lock_time }) + Ok(Interpreter { inner, stack, script_code, sequence, lock_time }) } /// Same as [`Interpreter::iter`], but allows for a custom verification function. @@ -165,7 +165,7 @@ impl<'txin> Interpreter<'txin> { // Cloning the references to elements of stack should be fine as it allows // call interpreter.iter() without mutating interpreter stack: self.stack.clone(), - age: self.age, + sequence: self.sequence, lock_time: self.lock_time, has_errored: false, sig_type: self.sig_type(), @@ -508,7 +508,7 @@ pub struct Iter<'intp, 'txin: 'intp> { public_key: Option<&'intp BitcoinKey>, state: Vec>, stack: Stack<'txin>, - age: Sequence, + sequence: Sequence, lock_time: absolute::LockTime, has_errored: bool, sig_type: SigType, @@ -608,7 +608,7 @@ where Terminal::Older(ref n) => { debug_assert_eq!(node_state.n_evaluated, 0); debug_assert_eq!(node_state.n_satisfied, 0); - let res = self.stack.evaluate_older(n, self.age); + let res = self.stack.evaluate_older(n, self.sequence); if res.is_some() { return res; } @@ -1110,7 +1110,7 @@ mod tests { stack, public_key: None, state: vec![NodeEvaluationState { node: ms, n_evaluated: 0, n_satisfied: 0 }], - age: Sequence::from_height(1002), + sequence: Sequence::from_height(1002), lock_time: absolute::LockTime::from_height(1002).unwrap(), has_errored: false, sig_type: SigType::Ecdsa, From 637720a8280bfdaeb457cd0f7ec364f945f278b7 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Wed, 6 Mar 2024 19:09:32 +0000 Subject: [PATCH 05/10] plan/satisfy: use relative::LockTime instead of Sequence to indicate relative locktimes Eliminates some error logic. Also replaces manual implementations of `is_implied_by` in a couple places. This introduces a couple unwrap()s which will be removed in a later commit, and which occur because there is a mismatch between the `Sequence` type used in `satisfy::Satisfaction` and `bitcoin::relative::LockTime` (and we can't switch this `Sequence` because we depend on `Ord`, which will need a new newtype, which should wait for another commit). This *also* deletes a couple lines in the PSBT code which cites BIP 112 saying that if the sequence number passed to OP_CSV has the disable flag set, to automatically pass `check_older`. With this code change, it is no longer possible to produce such a situation, so we drop the check and the citation. I believe this is correct because no Miniscript-generated call to OP_CSV should have the disable flag set. I believe the Miniscript spec is silent on this and more-or-less says you can pass any crap you want as the argument to `older`, but I am taking a stand here and think we should Miniscripts with the disable flag set. --- src/descriptor/mod.rs | 10 +++- src/miniscript/mod.rs | 2 +- src/miniscript/satisfy.rs | 72 ++++++++++-------------- src/plan.rs | 115 +++++++++++++++++++++++++------------- src/psbt/mod.rs | 10 +--- 5 files changed, 115 insertions(+), 94 deletions(-) diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs index 1059a3b3c..e7fa38777 100644 --- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -512,7 +512,10 @@ impl Descriptor { descriptor: self, template: stack, absolute_timelock: satisfaction.absolute_timelock.map(Into::into), - relative_timelock: satisfaction.relative_timelock, + // unwrap to be removed in a later commit + relative_timelock: satisfaction + .relative_timelock + .map(|s| s.to_relative_lock_time().unwrap()), }) } else { Err(self) @@ -540,7 +543,10 @@ impl Descriptor { descriptor: self, template: stack, absolute_timelock: satisfaction.absolute_timelock.map(Into::into), - relative_timelock: satisfaction.relative_timelock, + // unwrap to be removed in a later commit + relative_timelock: satisfaction + .relative_timelock + .map(|s| s.to_relative_lock_time().unwrap()), }) } else { Err(self) diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs index 12ab66799..3f43860a3 100644 --- a/src/miniscript/mod.rs +++ b/src/miniscript/mod.rs @@ -1426,7 +1426,7 @@ mod tests { } } - fn check_older(&self, _: bitcoin::Sequence) -> bool { true } + fn check_older(&self, _: bitcoin::relative::LockTime) -> bool { true } fn check_after(&self, _: bitcoin::absolute::LockTime) -> bool { true } } diff --git a/src/miniscript/satisfy.rs b/src/miniscript/satisfy.rs index 68ae90429..22c199df1 100644 --- a/src/miniscript/satisfy.rs +++ b/src/miniscript/satisfy.rs @@ -11,7 +11,7 @@ use core::{cmp, fmt, i64, mem}; use bitcoin::hashes::hash160; use bitcoin::key::XOnlyPublicKey; use bitcoin::taproot::{ControlBlock, LeafVersion, TapLeafHash, TapNodeHash}; -use bitcoin::{absolute, ScriptBuf, Sequence}; +use bitcoin::{absolute, relative, ScriptBuf, Sequence}; use sync::Arc; use super::context::SigType; @@ -94,7 +94,7 @@ pub trait Satisfier { /// NOTE: If a descriptor mixes time-based and height-based timelocks, the implementation of /// this method MUST only allow timelocks of either unit, but not both. Allowing both could cause /// miniscript to construct an invalid witness. - fn check_older(&self, _: Sequence) -> bool { false } + fn check_older(&self, _: relative::LockTime) -> bool { false } /// Assert whether a absolute locktime is satisfied /// @@ -108,39 +108,21 @@ pub trait Satisfier { impl Satisfier for () {} impl Satisfier for Sequence { - fn check_older(&self, n: Sequence) -> bool { - if !self.is_relative_lock_time() { - return false; - } - - // We need a relative lock time type in rust-bitcoin to clean this up. - - /* If nSequence encodes a relative lock-time, this mask is - * applied to extract that lock-time from the sequence field. */ - const SEQUENCE_LOCKTIME_MASK: u32 = 0x0000ffff; - const SEQUENCE_LOCKTIME_TYPE_FLAG: u32 = 0x00400000; - - let mask = SEQUENCE_LOCKTIME_MASK | SEQUENCE_LOCKTIME_TYPE_FLAG; - let masked_n = n.to_consensus_u32() & mask; - let masked_seq = self.to_consensus_u32() & mask; - if masked_n < SEQUENCE_LOCKTIME_TYPE_FLAG && masked_seq >= SEQUENCE_LOCKTIME_TYPE_FLAG { - false + fn check_older(&self, n: relative::LockTime) -> bool { + if let Some(lt) = self.to_relative_lock_time() { + Satisfier::::check_older(<, n) } else { - masked_n <= masked_seq + false } } } -impl Satisfier for absolute::LockTime { - fn check_after(&self, n: absolute::LockTime) -> bool { - use absolute::LockTime::*; +impl Satisfier for relative::LockTime { + fn check_older(&self, n: relative::LockTime) -> bool { n.is_implied_by(*self) } +} - match (n, *self) { - (Blocks(n), Blocks(lock_time)) => n <= lock_time, - (Seconds(n), Seconds(lock_time)) => n <= lock_time, - _ => false, // Not the same units. - } - } +impl Satisfier for absolute::LockTime { + fn check_after(&self, n: absolute::LockTime) -> bool { n.is_implied_by(*self) } } macro_rules! impl_satisfier_for_map_key_to_ecdsa_sig { @@ -323,7 +305,7 @@ impl<'a, Pk: MiniscriptKey + ToPublicKey, S: Satisfier> Satisfier for &' fn lookup_hash160(&self, h: &Pk::Hash160) -> Option { (**self).lookup_hash160(h) } - fn check_older(&self, t: Sequence) -> bool { (**self).check_older(t) } + fn check_older(&self, t: relative::LockTime) -> bool { (**self).check_older(t) } fn check_after(&self, n: absolute::LockTime) -> bool { (**self).check_after(n) } } @@ -383,7 +365,7 @@ impl<'a, Pk: MiniscriptKey + ToPublicKey, S: Satisfier> Satisfier for &' fn lookup_hash160(&self, h: &Pk::Hash160) -> Option { (**self).lookup_hash160(h) } - fn check_older(&self, t: Sequence) -> bool { (**self).check_older(t) } + fn check_older(&self, t: relative::LockTime) -> bool { (**self).check_older(t) } fn check_after(&self, n: absolute::LockTime) -> bool { (**self).check_after(n) } } @@ -529,7 +511,7 @@ macro_rules! impl_tuple_satisfier { None } - fn check_older(&self, n: Sequence) -> bool { + fn check_older(&self, n: relative::LockTime) -> bool { let &($(ref $ty,)*) = self; $( if $ty.check_older(n) { @@ -1295,18 +1277,20 @@ impl Satisfaction> { Satisfaction { stack, has_sig: false, relative_timelock: None, absolute_timelock } } Terminal::Older(t) => { - let (stack, relative_timelock) = if stfr.check_older(t) { - (Witness::empty(), Some(t)) - } else if root_has_sig { - // If the root terminal has signature, the - // signature covers the nLockTime and nSequence - // values. The sender of the transaction should - // take care that it signs the value such that the - // timelock is not met - (Witness::Impossible, None) - } else { - (Witness::Unavailable, None) - }; + // unwrap to be removed in a later commit + let (stack, relative_timelock) = + if stfr.check_older(t.to_relative_lock_time().unwrap()) { + (Witness::empty(), Some(t)) + } else if root_has_sig { + // If the root terminal has signature, the + // signature covers the nLockTime and nSequence + // values. The sender of the transaction should + // take care that it signs the value such that the + // timelock is not met + (Witness::Impossible, None) + } else { + (Witness::Unavailable, None) + }; Satisfaction { stack, has_sig: false, relative_timelock, absolute_timelock: None } } Terminal::Ripemd160(ref h) => Satisfaction { diff --git a/src/plan.rs b/src/plan.rs index c26fc5168..1e93274b1 100644 --- a/src/plan.rs +++ b/src/plan.rs @@ -14,15 +14,13 @@ //! Once you've obtained signatures, hash pre-images etc required by the plan, it can create a //! witness/script_sig for the input. -use core::cmp::Ordering; use core::iter::FromIterator; -use bitcoin::absolute::LockTime; use bitcoin::hashes::{hash160, ripemd160, sha256}; use bitcoin::key::XOnlyPublicKey; use bitcoin::script::PushBytesBuf; use bitcoin::taproot::{ControlBlock, LeafVersion, TapLeafHash}; -use bitcoin::{bip32, psbt, ScriptBuf, Sequence, WitnessVersion}; +use bitcoin::{absolute, bip32, psbt, relative, ScriptBuf, WitnessVersion}; use crate::descriptor::{self, Descriptor, DescriptorType, KeyMap}; use crate::miniscript::hash256; @@ -102,10 +100,10 @@ pub trait AssetProvider { fn provider_lookup_hash160(&self, _: &Pk::Hash160) -> bool { false } /// Assert whether a relative locktime is satisfied - fn check_older(&self, _: Sequence) -> bool { false } + fn check_older(&self, _: relative::LockTime) -> bool { false } /// Assert whether an absolute locktime is satisfied - fn check_after(&self, _: LockTime) -> bool { false } + fn check_after(&self, _: absolute::LockTime) -> bool { false } } /// Wrapper around [`Assets`] that logs every query and value returned @@ -138,8 +136,8 @@ impl AssetProvider for LoggerAssetProvider { impl_log_method!(provider_lookup_hash256, hash: &hash256::Hash, -> bool); impl_log_method!(provider_lookup_ripemd160, hash: &ripemd160::Hash, -> bool); impl_log_method!(provider_lookup_hash160, hash: &hash160::Hash, -> bool); - impl_log_method!(check_older, s: Sequence, -> bool); - impl_log_method!(check_after, t: LockTime, -> bool); + impl_log_method!(check_older, s: relative::LockTime, -> bool); + impl_log_method!(check_after, t: absolute::LockTime, -> bool); } impl AssetProvider for T @@ -208,9 +206,9 @@ where Satisfier::lookup_hash160(self, hash).is_some() } - fn check_older(&self, s: Sequence) -> bool { Satisfier::check_older(self, s) } + fn check_older(&self, s: relative::LockTime) -> bool { Satisfier::check_older(self, s) } - fn check_after(&self, l: LockTime) -> bool { Satisfier::check_after(self, l) } + fn check_after(&self, l: absolute::LockTime) -> bool { Satisfier::check_after(self, l) } } /// Representation of a particular spending path on a descriptor. Contains the witness template @@ -222,9 +220,9 @@ pub struct Plan { /// This plan's witness template pub(crate) template: Vec>, /// The absolute timelock this plan uses - pub absolute_timelock: Option, + pub absolute_timelock: Option, /// The relative timelock this plan uses - pub relative_timelock: Option, + pub relative_timelock: Option, pub(crate) descriptor: Descriptor, } @@ -517,9 +515,9 @@ pub struct Assets { /// Set of available hash160 preimages pub hash160_preimages: BTreeSet, /// Maximum absolute timelock allowed - pub absolute_timelock: Option, + pub absolute_timelock: Option, /// Maximum relative timelock allowed - pub relative_timelock: Option, + pub relative_timelock: Option, } // Checks if the `pk` is a "direct child" of the `derivation_path` provided. @@ -618,23 +616,20 @@ impl AssetProvider for Assets { self.hash160_preimages.contains(hash) } - fn check_older(&self, s: Sequence) -> bool { - if let Some(rt) = &self.relative_timelock { - return rt.is_relative_lock_time() - && rt.is_height_locked() == s.is_height_locked() - && s <= *rt; + fn check_older(&self, s: relative::LockTime) -> bool { + if let Some(timelock) = self.relative_timelock { + s.is_implied_by(timelock) + } else { + false } - - false } - fn check_after(&self, l: LockTime) -> bool { - if let Some(at) = &self.absolute_timelock { - let cmp = l.partial_cmp(at); - return cmp == Some(Ordering::Less) || cmp == Some(Ordering::Equal); + fn check_after(&self, l: absolute::LockTime) -> bool { + if let Some(timelock) = self.absolute_timelock { + l.is_implied_by(timelock) + } else { + false } - - false } } @@ -708,13 +703,13 @@ impl Assets { } /// Set the maximum relative timelock allowed - pub fn older(mut self, seq: Sequence) -> Self { + pub fn older(mut self, seq: relative::LockTime) -> Self { self.relative_timelock = Some(seq); self } /// Set the maximum absolute timelock allowed - pub fn after(mut self, lt: LockTime) -> Self { + pub fn after(mut self, lt: absolute::LockTime) -> Self { self.absolute_timelock = Some(lt); self } @@ -746,7 +741,13 @@ mod test { keys: Vec, hashes: Vec, // [ (key_indexes, hash_indexes, older, after, expected) ] - tests: Vec<(Vec, Vec, Option, Option, Option)>, + tests: Vec<( + Vec, + Vec, + Option, + Option, + Option, + )>, ) { let desc = Descriptor::::from_str(desc).unwrap(); @@ -860,6 +861,8 @@ mod test { #[test] fn test_thresh() { + // relative::LockTime has no constructors except by going through Sequence + use bitcoin::Sequence; let keys = vec![ DescriptorPublicKey::from_str( "02c2fd50ceae468857bb7eb32ae9cd4083e6c7e42fbbec179d81134b3e3830586c", @@ -875,19 +878,41 @@ mod test { let tests = vec![ (vec![], vec![], None, None, None), - (vec![], vec![], Some(Sequence(1000)), None, None), + ( + vec![], + vec![], + Some(Sequence(1000).to_relative_lock_time().unwrap()), + None, + None, + ), (vec![0], vec![], None, None, None), // expected weight: 4 (scriptSig len) + 1 (witness len) + 73 (sig) + 1 (OP_0) + 1 (OP_ZERO) - (vec![0], vec![], Some(Sequence(1000)), None, Some(80)), + ( + vec![0], + vec![], + Some(Sequence(1000).to_relative_lock_time().unwrap()), + None, + Some(80), + ), // expected weight: 4 (scriptSig len) + 1 (witness len) + 73 (sig) * 2 + 2 (OP_PUSHBYTE_1 0x01) (vec![0, 1], vec![], None, None, Some(153)), // expected weight: 4 (scriptSig len) + 1 (witness len) + 73 (sig) + 1 (OP_0) + 1 (OP_ZERO) - (vec![0, 1], vec![], Some(Sequence(1000)), None, Some(80)), + ( + vec![0, 1], + vec![], + Some(Sequence(1000).to_relative_lock_time().unwrap()), + None, + Some(80), + ), // expected weight: 4 (scriptSig len) + 1 (witness len) + 73 (sig) * 2 + 2 (OP_PUSHBYTE_1 0x01) ( vec![0, 1], vec![], - Some(Sequence::from_512_second_intervals(10)), + Some( + Sequence::from_512_second_intervals(10) + .to_relative_lock_time() + .unwrap(), + ), None, Some(153), ), // incompatible timelock @@ -899,13 +924,19 @@ mod test { let tests = vec![ // expected weight: 4 (scriptSig len) + 1 (witness len) + 73 (sig) + 1 (OP_0) + 1 (OP_ZERO) - (vec![0], vec![], None, Some(LockTime::from_height(1000).unwrap()), Some(80)), + ( + vec![0], + vec![], + None, + Some(absolute::LockTime::from_height(1000).unwrap()), + Some(80), + ), // expected weight: 4 (scriptSig len) + 1 (witness len) + 73 (sig) * 2 + 2 (OP_PUSHBYTE_1 0x01) ( vec![0, 1], vec![], None, - Some(LockTime::from_time(500_001_000).unwrap()), + Some(absolute::LockTime::from_time(500_001_000).unwrap()), Some(153), ), // incompatible timelock ]; @@ -989,15 +1020,21 @@ mod test { vec![4], vec![], None, - Some(LockTime::from_height(10).unwrap()), + Some(absolute::LockTime::from_height(10).unwrap()), third_leaf_sat_weight, ), // Spend with third leaf (key + timelock), // but timelock is too low (=impossible) - (vec![4], vec![], None, Some(LockTime::from_height(9).unwrap()), None), + (vec![4], vec![], None, Some(absolute::LockTime::from_height(9).unwrap()), None), // Spend with third leaf (key + timelock), // but timelock is in the wrong unit (=impossible) - (vec![4], vec![], None, Some(LockTime::from_time(1296000000).unwrap()), None), + ( + vec![4], + vec![], + None, + Some(absolute::LockTime::from_time(1296000000).unwrap()), + None, + ), // Spend with third leaf (key + timelock), // but don't give the timelock (=impossible) (vec![4], vec![], None, None, None), @@ -1012,7 +1049,7 @@ mod test { vec![2, 3, 4], vec![], None, - Some(LockTime::from_consensus(11)), + Some(absolute::LockTime::from_consensus(11)), third_leaf_sat_weight, ), ]; diff --git a/src/psbt/mod.rs b/src/psbt/mod.rs index 9e7027193..61027d6ad 100644 --- a/src/psbt/mod.rs +++ b/src/psbt/mod.rs @@ -19,7 +19,7 @@ use bitcoin::secp256k1; use bitcoin::secp256k1::{Secp256k1, VerifyOnly}; use bitcoin::sighash::{self, SighashCache}; use bitcoin::taproot::{self, ControlBlock, LeafVersion, TapLeafHash}; -use bitcoin::{absolute, bip32, transaction, Script, ScriptBuf, Sequence}; +use bitcoin::{absolute, bip32, relative, transaction, Script, ScriptBuf}; use crate::miniscript::context::SigType; use crate::prelude::*; @@ -324,15 +324,9 @@ impl<'psbt, Pk: MiniscriptKey + ToPublicKey> Satisfier for PsbtInputSatisfie >::check_after(&lock_time, n) } - fn check_older(&self, n: Sequence) -> bool { + fn check_older(&self, n: relative::LockTime) -> bool { let seq = self.psbt.unsigned_tx.input[self.index].sequence; - // https://github.com/bitcoin/bips/blob/master/bip-0112.mediawiki - // Disable flag set => return true. - if !n.is_relative_lock_time() { - return true; - } - if self.psbt.unsigned_tx.version < transaction::Version::TWO || !seq.is_relative_lock_time() { return false; From fc6bfa8f57cd050d5ab7c887ca9e15928a7c6416 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Wed, 6 Mar 2024 14:39:12 +0000 Subject: [PATCH 06/10] introduce RelLockTime type Just like AbsLockTime, this gives us a locktime which implements Ord and which cannot be constructed unless it meets the invariants specified by Miniscript. --- src/lib.rs | 5 ++ src/primitives/mod.rs | 1 + src/primitives/relative_locktime.rs | 100 ++++++++++++++++++++++++++++ 3 files changed, 106 insertions(+) create mode 100644 src/primitives/relative_locktime.rs diff --git a/src/lib.rs b/src/lib.rs index c8b079070..514ccdfe8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -151,6 +151,7 @@ pub use crate::miniscript::satisfy::{Preimage32, Satisfier}; pub use crate::miniscript::{hash256, Miniscript}; use crate::prelude::*; pub use crate::primitives::absolute_locktime::{AbsLockTime, AbsLockTimeError}; +pub use crate::primitives::relative_locktime::{RelLockTime, RelLockTimeError}; /// Public key trait which can be converted to Hash type pub trait MiniscriptKey: Clone + Eq + Ord + fmt::Debug + fmt::Display + hash::Hash { @@ -505,6 +506,8 @@ pub enum Error { MultipathDescLenMismatch, /// Invalid absolute locktime AbsoluteLockTime(AbsLockTimeError), + /// Invalid absolute locktime + RelativeLockTime(RelLockTimeError), } // https://github.com/sipa/miniscript/pull/5 for discussion on this number @@ -581,6 +584,7 @@ impl fmt::Display for Error { Error::TrNoExplicitScript => write!(f, "No script code for Tr descriptors"), Error::MultipathDescLenMismatch => write!(f, "At least two BIP389 key expressions in the descriptor contain tuples of derivation indexes of different lengths"), Error::AbsoluteLockTime(ref e) => e.fmt(f), + Error::RelativeLockTime(ref e) => e.fmt(f), } } } @@ -634,6 +638,7 @@ impl error::Error for Error { AnalysisError(e) => Some(e), PubKeyCtxError(e, _) => Some(e), AbsoluteLockTime(e) => Some(e), + RelativeLockTime(e) => Some(e), } } } diff --git a/src/primitives/mod.rs b/src/primitives/mod.rs index b05d777a1..84406f182 100644 --- a/src/primitives/mod.rs +++ b/src/primitives/mod.rs @@ -12,3 +12,4 @@ //! should be re-exported at the crate root. pub mod absolute_locktime; +pub mod relative_locktime; diff --git a/src/primitives/relative_locktime.rs b/src/primitives/relative_locktime.rs new file mode 100644 index 000000000..05239a498 --- /dev/null +++ b/src/primitives/relative_locktime.rs @@ -0,0 +1,100 @@ +// SPDX-License-Identifier: CC0-1.0 + +//! Relative Locktimes + +use core::{cmp, convert, fmt}; + +use bitcoin::{relative, Sequence}; + +/// Error parsing an absolute locktime. +#[derive(Debug, PartialEq)] +pub struct RelLockTimeError { + value: u32, +} + +impl fmt::Display for RelLockTimeError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + if self.value == 0 { + f.write_str("relative locktimes in Miniscript have a minimum value of 1") + } else { + debug_assert!(Sequence::from_consensus(self.value) + .to_relative_lock_time() + .is_none()); + write!(f, "locktime value {} is not a valid BIP68 relative locktime", self.value) + } + } +} + +#[cfg(feature = "std")] +impl std::error::Error for RelLockTimeError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { None } +} + +/// A relative locktime which implements `Ord`. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub struct RelLockTime(Sequence); + +impl RelLockTime { + /// The "0 blocks" constant. + pub const ZERO: Self = RelLockTime(Sequence::ZERO); + + /// Constructs an `RelLockTime` from an nLockTime value or the argument to `CHEKCLOCKTIMEVERIFY`. + pub fn from_consensus(n: u32) -> Result { + convert::TryFrom::try_from(Sequence::from_consensus(n)) + } + + /// Returns the inner `u32` value. This is the value used when creating this `LockTime` + /// i.e., `n OP_CHECKSEQUENCEVERIFY` or `nSequence`. + pub fn to_consensus_u32(self) -> u32 { self.0.to_consensus_u32() } + + /// Takes a 16-bit number of blocks and produces a relative locktime from it. + pub fn from_height(height: u16) -> Self { RelLockTime(Sequence::from_height(height)) } + + /// Takes a 16-bit number of 512-second time intervals and produces a relative locktime from it. + pub fn from_512_second_intervals(time: u16) -> Self { + RelLockTime(Sequence::from_512_second_intervals(time)) + } + + /// Whether this timelock is blockheight-based. + pub fn is_height_locked(&self) -> bool { self.0.is_height_locked() } + + /// Whether this timelock is time-based. + pub fn is_time_locked(&self) -> bool { self.0.is_time_locked() } +} + +impl convert::TryFrom for RelLockTime { + type Error = RelLockTimeError; + fn try_from(seq: Sequence) -> Result { + if seq.is_relative_lock_time() { + Ok(RelLockTime(seq)) + } else { + Err(RelLockTimeError { value: seq.to_consensus_u32() }) + } + } +} + +impl From for Sequence { + fn from(lock_time: RelLockTime) -> Sequence { lock_time.0 } +} + +impl From for relative::LockTime { + fn from(lock_time: RelLockTime) -> relative::LockTime { + lock_time.0.to_relative_lock_time().unwrap() + } +} + +impl cmp::PartialOrd for RelLockTime { + fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) } +} + +impl cmp::Ord for RelLockTime { + fn cmp(&self, other: &Self) -> cmp::Ordering { + let this = self.0.to_consensus_u32(); + let that = other.0.to_consensus_u32(); + this.cmp(&that) + } +} + +impl fmt::Display for RelLockTime { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fmt::Display::fmt(&self.0, f) } +} From a7a1b9a4876e89cfedc2f03130108b107a3864cd Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Wed, 6 Mar 2024 19:36:34 +0000 Subject: [PATCH 07/10] miniscript: use RelLockTime in `Satisfaction` Gets rid of a couple unwraps from the last commit :) --- src/descriptor/mod.rs | 9 ++------- src/miniscript/mod.rs | 10 +++++----- src/miniscript/satisfy.rs | 38 +++++++++++++++++++++++--------------- 3 files changed, 30 insertions(+), 27 deletions(-) diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs index e7fa38777..bea539920 100644 --- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -512,10 +512,7 @@ impl Descriptor { descriptor: self, template: stack, absolute_timelock: satisfaction.absolute_timelock.map(Into::into), - // unwrap to be removed in a later commit - relative_timelock: satisfaction - .relative_timelock - .map(|s| s.to_relative_lock_time().unwrap()), + relative_timelock: satisfaction.relative_timelock.map(Into::into), }) } else { Err(self) @@ -544,9 +541,7 @@ impl Descriptor { template: stack, absolute_timelock: satisfaction.absolute_timelock.map(Into::into), // unwrap to be removed in a later commit - relative_timelock: satisfaction - .relative_timelock - .map(|s| s.to_relative_lock_time().unwrap()), + relative_timelock: satisfaction.relative_timelock.map(Into::into), }) } else { Err(self) diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs index 3f43860a3..6944a9544 100644 --- a/src/miniscript/mod.rs +++ b/src/miniscript/mod.rs @@ -1355,7 +1355,7 @@ mod tests { #[test] fn template_timelocks() { - use crate::AbsLockTime; + use crate::{AbsLockTime, RelLockTime}; let key_present = bitcoin::PublicKey::from_str( "0327a6ed0e71b451c79327aa9e4a6bb26ffb1c0056abc02c25e783f6096b79bb4f", ) @@ -1381,7 +1381,7 @@ mod tests { ( format!("or_d(pk({}),and_v(v:pk({}),older(12960)))", key_missing, key_present), None, - Some(bitcoin::Sequence(12960)), + Some(RelLockTime::from_height(12960)), ), ( format!( @@ -1389,12 +1389,12 @@ mod tests { key_present, key_missing ), Some(AbsLockTime::from_consensus(11).unwrap()), - Some(bitcoin::Sequence(10)), + Some(RelLockTime::from_height(10)), ), ( format!("and_v(v:and_v(v:pk({}),older(10)),older(20))", key_present), None, - Some(bitcoin::Sequence(20)), + Some(RelLockTime::from_height(20)), ), ( format!( @@ -1402,7 +1402,7 @@ mod tests { key_present, key_missing ), None, - Some(bitcoin::Sequence(10)), + Some(RelLockTime::from_height(10)), ), ]; diff --git a/src/miniscript/satisfy.rs b/src/miniscript/satisfy.rs index 22c199df1..afd600d0f 100644 --- a/src/miniscript/satisfy.rs +++ b/src/miniscript/satisfy.rs @@ -18,7 +18,9 @@ use super::context::SigType; use crate::plan::AssetProvider; use crate::prelude::*; use crate::util::witness_size; -use crate::{AbsLockTime, Miniscript, MiniscriptKey, ScriptContext, Terminal, ToPublicKey}; +use crate::{ + AbsLockTime, Miniscript, MiniscriptKey, RelLockTime, ScriptContext, Terminal, ToPublicKey, +}; /// Type alias for 32 byte Preimage. pub type Preimage32 = [u8; 32]; @@ -117,6 +119,12 @@ impl Satisfier for Sequence { } } +impl Satisfier for RelLockTime { + fn check_older(&self, n: relative::LockTime) -> bool { + >::check_older(&(*self).into(), n) + } +} + impl Satisfier for relative::LockTime { fn check_older(&self, n: relative::LockTime) -> bool { n.is_implied_by(*self) } } @@ -876,7 +884,7 @@ pub struct Satisfaction { /// The absolute timelock used by this satisfaction pub absolute_timelock: Option, /// The relative timelock used by this satisfaction - pub relative_timelock: Option, + pub relative_timelock: Option, } impl Satisfaction> { @@ -1278,19 +1286,19 @@ impl Satisfaction> { } Terminal::Older(t) => { // unwrap to be removed in a later commit - let (stack, relative_timelock) = - if stfr.check_older(t.to_relative_lock_time().unwrap()) { - (Witness::empty(), Some(t)) - } else if root_has_sig { - // If the root terminal has signature, the - // signature covers the nLockTime and nSequence - // values. The sender of the transaction should - // take care that it signs the value such that the - // timelock is not met - (Witness::Impossible, None) - } else { - (Witness::Unavailable, None) - }; + let t = >::try_from(t).unwrap(); + let (stack, relative_timelock) = if stfr.check_older(t.into()) { + (Witness::empty(), Some(t)) + } else if root_has_sig { + // If the root terminal has signature, the + // signature covers the nLockTime and nSequence + // values. The sender of the transaction should + // take care that it signs the value such that the + // timelock is not met + (Witness::Impossible, None) + } else { + (Witness::Unavailable, None) + }; Satisfaction { stack, has_sig: false, relative_timelock, absolute_timelock: None } } Terminal::Ripemd160(ref h) => Satisfaction { From c490f49fcb7eab6794b0c40c83d397f8cd02fe2d Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Thu, 7 Mar 2024 21:58:35 +0000 Subject: [PATCH 08/10] policy: use `RelLockTime` in semantic and concrete policies This changes the signature of `at_age` to take a `relative::LockTime` to be symmetric with `at_lock_time`. But because `relative::LockTime` has no constructors this is kinda annoying to use. See https://github.com/rust-bitcoin/rust-bitcoin/issues/2547 Anyway for now you can just construct a RelLockTime and tack .into() onto it, which is fine as a workaround. We may want to update docs for it. --- src/miniscript/mod.rs | 9 ++-- src/policy/compiler.rs | 21 ++++----- src/policy/concrete.rs | 49 +++++---------------- src/policy/mod.rs | 13 +++--- src/policy/semantic.rs | 99 ++++++++++++++++++++++++------------------ 5 files changed, 90 insertions(+), 101 deletions(-) diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs index 6944a9544..26f220a9f 100644 --- a/src/miniscript/mod.rs +++ b/src/miniscript/mod.rs @@ -698,7 +698,6 @@ mod tests { use bitcoin::hashes::{hash160, sha256, Hash}; use bitcoin::secp256k1::XOnlyPublicKey; use bitcoin::taproot::TapLeafHash; - use bitcoin::Sequence; use sync::Arc; use super::{Miniscript, ScriptContext, Segwitv0, Tap}; @@ -707,7 +706,7 @@ mod tests { use crate::policy::Liftable; use crate::prelude::*; use crate::test_utils::{StrKeyTranslator, StrXOnlyKeyTranslator}; - use crate::{hex_script, ExtParams, Satisfier, ToPublicKey, TranslatePk}; + use crate::{hex_script, ExtParams, RelLockTime, Satisfier, ToPublicKey, TranslatePk}; type Segwitv0Script = Miniscript; type Tapscript = Miniscript; @@ -1084,13 +1083,13 @@ mod tests { let mut abs = miniscript.lift().unwrap(); assert_eq!(abs.n_keys(), 5); assert_eq!(abs.minimum_n_keys(), Some(2)); - abs = abs.at_age(Sequence::from_height(10000)); + abs = abs.at_age(RelLockTime::from_height(10000).into()); assert_eq!(abs.n_keys(), 5); assert_eq!(abs.minimum_n_keys(), Some(2)); - abs = abs.at_age(Sequence::from_height(9999)); + abs = abs.at_age(RelLockTime::from_height(9999).into()); assert_eq!(abs.n_keys(), 3); assert_eq!(abs.minimum_n_keys(), Some(3)); - abs = abs.at_age(Sequence::ZERO); + abs = abs.at_age(RelLockTime::ZERO.into()); assert_eq!(abs.n_keys(), 3); assert_eq!(abs.minimum_n_keys(), Some(3)); diff --git a/src/policy/compiler.rs b/src/policy/compiler.rs index 0989e564d..8b7dc1957 100644 --- a/src/policy/compiler.rs +++ b/src/policy/compiler.rs @@ -871,7 +871,7 @@ where insert_wrap!(AstElemExt::terminal(Terminal::PkK(pk.clone()))); } Concrete::After(n) => insert_wrap!(AstElemExt::terminal(Terminal::After(n))), - Concrete::Older(n) => insert_wrap!(AstElemExt::terminal(Terminal::Older(n))), + Concrete::Older(n) => insert_wrap!(AstElemExt::terminal(Terminal::Older(n.into()))), Concrete::Sha256(ref hash) => { insert_wrap!(AstElemExt::terminal(Terminal::Sha256(hash.clone()))) } @@ -1254,7 +1254,7 @@ mod tests { use super::*; use crate::miniscript::{Legacy, Segwitv0, Tap}; use crate::policy::Liftable; - use crate::{script_num_size, AbsLockTime, ToPublicKey}; + use crate::{script_num_size, AbsLockTime, RelLockTime, ToPublicKey}; type SPolicy = Concrete; type BPolicy = Concrete; @@ -1406,7 +1406,7 @@ mod tests { ( 1, Arc::new(Concrete::And(vec![ - Arc::new(Concrete::Older(Sequence::from_height(10000))), + Arc::new(Concrete::Older(RelLockTime::from_height(10000))), Arc::new(Concrete::Threshold( 2, key_pol[5..8].iter().map(|p| (p.clone()).into()).collect(), @@ -1436,13 +1436,13 @@ mod tests { let mut abs = policy.lift().unwrap(); assert_eq!(abs.n_keys(), 8); assert_eq!(abs.minimum_n_keys(), Some(2)); - abs = abs.at_age(Sequence::from_height(10000)); + abs = abs.at_age(RelLockTime::from_height(10000).into()); assert_eq!(abs.n_keys(), 8); assert_eq!(abs.minimum_n_keys(), Some(2)); - abs = abs.at_age(Sequence::from_height(9999)); + abs = abs.at_age(RelLockTime::from_height(9999).into()); assert_eq!(abs.n_keys(), 5); assert_eq!(abs.minimum_n_keys(), Some(3)); - abs = abs.at_age(Sequence::ZERO); + abs = abs.at_age(RelLockTime::ZERO.into()); assert_eq!(abs.n_keys(), 5); assert_eq!(abs.minimum_n_keys(), Some(3)); @@ -1467,15 +1467,16 @@ mod tests { assert!(ms.satisfy(no_sat).is_err()); assert!(ms.satisfy(&left_sat).is_ok()); assert!(ms - .satisfy((&right_sat, Sequence::from_height(10001))) + .satisfy((&right_sat, RelLockTime::from_height(10001))) .is_ok()); //timelock not met assert!(ms - .satisfy((&right_sat, Sequence::from_height(9999))) + .satisfy((&right_sat, RelLockTime::from_height(9999))) .is_err()); assert_eq!( - ms.satisfy((left_sat, Sequence::from_height(9999))).unwrap(), + ms.satisfy((left_sat, RelLockTime::from_height(9999))) + .unwrap(), vec![ // sat for left branch vec![], @@ -1486,7 +1487,7 @@ mod tests { ); assert_eq!( - ms.satisfy((right_sat, Sequence::from_height(10000))) + ms.satisfy((right_sat, RelLockTime::from_height(10000))) .unwrap(), vec![ // sat for right branch diff --git a/src/policy/concrete.rs b/src/policy/concrete.rs index 4908011a1..75481ff1d 100644 --- a/src/policy/concrete.rs +++ b/src/policy/concrete.rs @@ -7,7 +7,7 @@ use core::{fmt, str}; #[cfg(feature = "std")] use std::error; -use bitcoin::{absolute, Sequence}; +use bitcoin::absolute; #[cfg(feature = "compiler")] use { crate::descriptor::TapTree, @@ -29,7 +29,9 @@ use crate::prelude::*; use crate::sync::Arc; #[cfg(all(doc, not(feature = "compiler")))] use crate::Descriptor; -use crate::{errstr, AbsLockTime, Error, ForEachKey, FromStrKey, MiniscriptKey, Translator}; +use crate::{ + errstr, AbsLockTime, Error, ForEachKey, FromStrKey, MiniscriptKey, RelLockTime, Translator, +}; /// Maximum TapLeafs allowed in a compiled TapTree #[cfg(feature = "compiler")] @@ -52,7 +54,7 @@ pub enum Policy { /// An absolute locktime restriction. After(AbsLockTime), /// A relative locktime restriction. - Older(Sequence), + Older(RelLockTime), /// A SHA256 whose preimage must be provided to satisfy the descriptor. Sha256(Pk::Sha256), /// A SHA256d whose preimage must be provided to satisfy the descriptor. @@ -70,15 +72,6 @@ pub enum Policy { Threshold(usize, Vec>>), } -impl Policy -where - Pk: MiniscriptKey, -{ - /// Construct a `Policy::Older` from `n`. Helper function equivalent to - /// `Policy::Older(Sequence::from_consensus(n))`. - pub fn older(n: u32) -> Policy { Policy::Older(Sequence::from_consensus(n)) } -} - /// Detailed error type for concrete policies. #[derive(Copy, Clone, PartialEq, Eq, Debug)] pub enum PolicyError { @@ -88,10 +81,6 @@ pub enum PolicyError { NonBinaryArgOr, /// `Thresh` fragment can only have `1<=k<=n`. IncorrectThresh, - /// `older` or `after` fragment can only have `n = 0`. - ZeroTime, - /// `after` fragment can only have `n < 2^31`. - TimeTooFar, /// Semantic Policy Error: `And` `Or` fragments must take args: `k > 1`. InsufficientArgsforAnd, /// Semantic policy error: `And` `Or` fragments must take args: `k > 1`. @@ -129,10 +118,6 @@ impl fmt::Display for PolicyError { PolicyError::IncorrectThresh => { f.write_str("Threshold k must be greater than 0 and less than or equal to n 0 { - f.write_str("Relative/Absolute time must be less than 2^31; n < 2^31") - } - PolicyError::ZeroTime => f.write_str("Time must be greater than 0; n > 0"), PolicyError::InsufficientArgsforAnd => { f.write_str("Semantic Policy 'And' fragment must have at least 2 args ") } @@ -159,8 +144,6 @@ impl error::Error for PolicyError { NonBinaryArgAnd | NonBinaryArgOr | IncorrectThresh - | ZeroTime - | TimeTooFar | InsufficientArgsforAnd | InsufficientArgsforOr | EntailmentMaxTerminals @@ -751,13 +734,7 @@ impl Policy { for policy in self.pre_order_iter() { match *policy { After(_) => {} - Older(n) => { - if n == Sequence::ZERO { - return Err(PolicyError::ZeroTime); - } else if n.to_consensus_u32() > 2u32.pow(31) { - return Err(PolicyError::TimeTooFar); - } - } + Older(_) => {} And(ref subs) => { if subs.len() != 2 { return Err(PolicyError::NonBinaryArgAnd); @@ -971,15 +948,11 @@ impl Policy { .and_then(|x| AbsLockTime::from_consensus(x).map_err(Error::AbsoluteLockTime)) .map(Policy::After) }), - ("older", 1) => { - let num = expression::terminal(&top.args[0], expression::parse_num)?; - if num > 2u32.pow(31) { - return Err(Error::PolicyError(PolicyError::TimeTooFar)); - } else if num == 0 { - return Err(Error::PolicyError(PolicyError::ZeroTime)); - } - Ok(Policy::older(num)) - } + ("older", 1) => expression::terminal(&top.args[0], |x| { + expression::parse_num(x) + .and_then(|x| RelLockTime::from_consensus(x).map_err(Error::RelativeLockTime)) + .map(Policy::Older) + }), ("sha256", 1) => expression::terminal(&top.args[0], |x| { ::from_str(x).map(Policy::Sha256) }), diff --git a/src/policy/mod.rs b/src/policy/mod.rs index d6525b201..190a3a47e 100644 --- a/src/policy/mod.rs +++ b/src/policy/mod.rs @@ -123,7 +123,9 @@ impl Liftable for Terminal { return Err(Error::LiftError(LiftError::RawDescriptorLift)) } Terminal::After(t) => Semantic::After(t), - Terminal::Older(t) => Semantic::Older(t), + Terminal::Older(t) => Semantic::Older( + >::try_from(t).unwrap(), + ), // unwrap to be removed in future commit Terminal::Sha256(ref h) => Semantic::Sha256(h.clone()), Terminal::Hash256(ref h) => Semantic::Hash256(h.clone()), Terminal::Ripemd160(ref h) => Semantic::Ripemd160(h.clone()), @@ -241,13 +243,12 @@ impl Liftable for Arc> { mod tests { use core::str::FromStr; - use bitcoin::Sequence; - use super::*; #[cfg(feature = "compiler")] use crate::descriptor::Tr; use crate::miniscript::context::Segwitv0; use crate::prelude::*; + use crate::RelLockTime; #[cfg(feature = "compiler")] use crate::{descriptor::TapTree, Tap}; @@ -323,7 +324,7 @@ mod tests { ConcretePol::from_str("or(pk())").unwrap_err().to_string(), "Or policy fragment must take 2 arguments" ); - // this weird "unexpected" wrapping of the error will go away in a later PR + // these weird "unexpected" wrapping of errors will go away in a later PR // which rewrites the expression parser assert_eq!( ConcretePol::from_str("thresh(3,after(0),pk(),pk())") @@ -336,7 +337,7 @@ mod tests { ConcretePol::from_str("thresh(2,older(2147483650),pk(),pk())") .unwrap_err() .to_string(), - "Relative/Absolute time must be less than 2^31; n < 2^31" + "unexpected «locktime value 2147483650 is not a valid BIP68 relative locktime»" ); } @@ -370,7 +371,7 @@ mod tests { 2, vec![ Arc::new(Semantic::Key(key_a)), - Arc::new(Semantic::Older(Sequence::from_height(42))) + Arc::new(Semantic::Older(RelLockTime::from_height(42))) ] )), Arc::new(Semantic::Key(key_b)) diff --git a/src/policy/semantic.rs b/src/policy/semantic.rs index c6e0f5a5f..3895e0552 100644 --- a/src/policy/semantic.rs +++ b/src/policy/semantic.rs @@ -8,7 +8,7 @@ use core::str::FromStr; use core::{fmt, str}; -use bitcoin::{absolute, Sequence}; +use bitcoin::{absolute, relative}; use super::concrete::PolicyError; use super::ENTAILMENT_MAX_TERMINALS; @@ -16,7 +16,8 @@ use crate::iter::{Tree, TreeLike}; use crate::prelude::*; use crate::sync::Arc; use crate::{ - errstr, expression, AbsLockTime, Error, ForEachKey, FromStrKey, MiniscriptKey, Translator, + errstr, expression, AbsLockTime, Error, ForEachKey, FromStrKey, MiniscriptKey, RelLockTime, + Translator, }; /// Abstract policy which corresponds to the semantics of a miniscript and @@ -36,7 +37,7 @@ pub enum Policy { /// An absolute locktime restriction. After(AbsLockTime), /// A relative locktime restriction. - Older(Sequence), + Older(RelLockTime), /// A SHA256 whose preimage must be provided to satisfy the descriptor. Sha256(Pk::Sha256), /// A SHA256d whose preimage must be provided to satisfy the descriptor. @@ -49,16 +50,6 @@ pub enum Policy { Threshold(usize, Vec>>), } -impl Policy -where - Pk: MiniscriptKey, -{ - /// Construct a `Policy::Older` from `n`. - /// - /// Helper function equivalent to `Policy::Older(Sequence::from_consensus(n))`. - pub fn older(n: u32) -> Policy { Policy::Older(Sequence::from_consensus(n)) } -} - impl ForEachKey for Policy { fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, mut pred: F) -> bool { self.pre_order_iter().all(|policy| match policy { @@ -327,7 +318,9 @@ impl expression::FromTree for Policy { .map(Policy::After) }), ("older", 1) => expression::terminal(&top.args[0], |x| { - expression::parse_num(x).map(|x| Policy::older(x)) + expression::parse_num(x) + .and_then(|x| RelLockTime::from_consensus(x).map_err(Error::RelativeLockTime)) + .map(Policy::Older) }), ("sha256", 1) => { expression::terminal(&top.args[0], |x| Pk::Sha256::from_str(x).map(Policy::Sha256)) @@ -512,7 +505,7 @@ impl Policy { /// Filters a policy by eliminating relative timelock constraints /// that are not satisfied at the given `age`. - pub fn at_age(self, age: Sequence) -> Policy { + pub fn at_age(self, age: relative::LockTime) -> Policy { use Policy::*; let mut at_age = vec![]; @@ -521,13 +514,10 @@ impl Policy { let new_policy = match data.node.as_ref() { Older(ref t) => { - if t.is_height_locked() && age.is_time_locked() - || t.is_time_locked() && age.is_height_locked() - || t.to_consensus_u32() > age.to_consensus_u32() - { - Some(Policy::Unsatisfiable) - } else { + if relative::LockTime::from(*t).is_implied_by(age) { Some(Policy::Older(*t)) + } else { + Some(Policy::Unsatisfiable) } } Threshold(k, ref subs) => { @@ -722,19 +712,32 @@ mod tests { assert_eq!(policy, Policy::Key("".to_owned())); assert_eq!(policy.relative_timelocks(), vec![]); assert_eq!(policy.absolute_timelocks(), vec![]); - assert_eq!(policy.clone().at_age(Sequence::ZERO), policy); - assert_eq!(policy.clone().at_age(Sequence::from_height(10000)), policy); + assert_eq!(policy.clone().at_age(RelLockTime::ZERO.into()), policy); + assert_eq!( + policy + .clone() + .at_age(RelLockTime::from_height(10000).into()), + policy + ); assert_eq!(policy.n_keys(), 1); assert_eq!(policy.minimum_n_keys(), Some(1)); let policy = StringPolicy::from_str("older(1000)").unwrap(); - assert_eq!(policy, Policy::Older(Sequence::from_height(1000))); + assert_eq!(policy, Policy::Older(RelLockTime::from_height(1000))); assert_eq!(policy.absolute_timelocks(), vec![]); assert_eq!(policy.relative_timelocks(), vec![1000]); - assert_eq!(policy.clone().at_age(Sequence::ZERO), Policy::Unsatisfiable); - assert_eq!(policy.clone().at_age(Sequence::from_height(999)), Policy::Unsatisfiable); - assert_eq!(policy.clone().at_age(Sequence::from_height(1000)), policy); - assert_eq!(policy.clone().at_age(Sequence::from_height(10000)), policy); + assert_eq!(policy.clone().at_age(RelLockTime::ZERO.into()), Policy::Unsatisfiable); + assert_eq!( + policy.clone().at_age(RelLockTime::from_height(999).into()), + Policy::Unsatisfiable + ); + assert_eq!(policy.clone().at_age(RelLockTime::from_height(1000).into()), policy); + assert_eq!( + policy + .clone() + .at_age(RelLockTime::from_height(10000).into()), + policy + ); assert_eq!(policy.n_keys(), 0); assert_eq!(policy.minimum_n_keys(), Some(0)); @@ -745,17 +748,25 @@ mod tests { 1, vec![ Policy::Key("".to_owned()).into(), - Policy::Older(Sequence::from_height(1000)).into(), + Policy::Older(RelLockTime::from_height(1000)).into(), ] ) ); assert_eq!(policy.relative_timelocks(), vec![1000]); assert_eq!(policy.absolute_timelocks(), vec![]); - assert_eq!(policy.clone().at_age(Sequence::ZERO), Policy::Key("".to_owned())); - assert_eq!(policy.clone().at_age(Sequence::from_height(999)), Policy::Key("".to_owned())); - assert_eq!(policy.clone().at_age(Sequence::from_height(1000)), policy.clone().normalized()); + assert_eq!(policy.clone().at_age(RelLockTime::ZERO.into()), Policy::Key("".to_owned())); + assert_eq!( + policy.clone().at_age(RelLockTime::from_height(999).into()), + Policy::Key("".to_owned()) + ); assert_eq!( - policy.clone().at_age(Sequence::from_height(10000)), + policy.clone().at_age(RelLockTime::from_height(1000).into()), + policy.clone().normalized() + ); + assert_eq!( + policy + .clone() + .at_age(RelLockTime::from_height(10000).into()), policy.clone().normalized() ); assert_eq!(policy.n_keys(), 1); @@ -804,11 +815,11 @@ mod tests { Policy::Threshold( 2, vec![ - Policy::Older(Sequence::from_height(1000)).into(), - Policy::Older(Sequence::from_height(10000)).into(), - Policy::Older(Sequence::from_height(1000)).into(), - Policy::Older(Sequence::from_height(2000)).into(), - Policy::Older(Sequence::from_height(2000)).into(), + Policy::Older(RelLockTime::from_height(1000)).into(), + Policy::Older(RelLockTime::from_height(10000)).into(), + Policy::Older(RelLockTime::from_height(1000)).into(), + Policy::Older(RelLockTime::from_height(2000)).into(), + Policy::Older(RelLockTime::from_height(2000)).into(), ] ) ); @@ -828,9 +839,9 @@ mod tests { Policy::Threshold( 2, vec![ - Policy::Older(Sequence::from_height(1000)).into(), - Policy::Older(Sequence::from_height(10000)).into(), - Policy::Older(Sequence::from_height(1000)).into(), + Policy::Older(RelLockTime::from_height(1000)).into(), + Policy::Older(RelLockTime::from_height(10000)).into(), + Policy::Older(RelLockTime::from_height(1000)).into(), Policy::Unsatisfiable.into(), Policy::Unsatisfiable.into(), ] @@ -947,7 +958,11 @@ mod tests { // test liquid backup policy before the emergency timeout let backup_policy = StringPolicy::from_str("thresh(2,pk(A),pk(B),pk(C))").unwrap(); assert!(!backup_policy - .entails(liquid_pol.clone().at_age(Sequence::from_height(4095))) + .entails( + liquid_pol + .clone() + .at_age(RelLockTime::from_height(4095).into()) + ) .unwrap()); // Finally test both spending paths From fc42e48022052935f8d657c7e7755513e8d83223 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Wed, 6 Mar 2024 20:20:11 +0000 Subject: [PATCH 09/10] miniscript: use RelLockTime in Terminal This gets rid of all the new conversions and unwraps :) --- src/interpreter/mod.rs | 2 +- src/miniscript/astelem.rs | 10 ++++++---- src/miniscript/decode.rs | 8 ++++---- src/miniscript/satisfy.rs | 2 -- src/miniscript/types/extra_props.rs | 16 +++------------- src/miniscript/types/mod.rs | 19 +------------------ src/policy/compiler.rs | 13 ++----------- src/policy/mod.rs | 4 +--- 8 files changed, 18 insertions(+), 56 deletions(-) diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs index 8f66d2d26..2c37e4496 100644 --- a/src/interpreter/mod.rs +++ b/src/interpreter/mod.rs @@ -608,7 +608,7 @@ where Terminal::Older(ref n) => { debug_assert_eq!(node_state.n_evaluated, 0); debug_assert_eq!(node_state.n_satisfied, 0); - let res = self.stack.evaluate_older(n, self.sequence); + let res = self.stack.evaluate_older(&(*n).into(), self.sequence); if res.is_some() { return res; } diff --git a/src/miniscript/astelem.rs b/src/miniscript/astelem.rs index 0364765af..d6f47d9b9 100644 --- a/src/miniscript/astelem.rs +++ b/src/miniscript/astelem.rs @@ -11,7 +11,7 @@ use core::fmt; use core::str::FromStr; use bitcoin::hashes::{hash160, Hash}; -use bitcoin::{absolute, opcodes, script, Sequence}; +use bitcoin::{absolute, opcodes, script}; use sync::Arc; use crate::miniscript::context::SigType; @@ -19,8 +19,8 @@ use crate::miniscript::{types, ScriptContext}; use crate::prelude::*; use crate::util::MsKeyBuilder; use crate::{ - errstr, expression, AbsLockTime, Error, FromStrKey, Miniscript, MiniscriptKey, Terminal, - ToPublicKey, + errstr, expression, AbsLockTime, Error, FromStrKey, Miniscript, MiniscriptKey, RelLockTime, + Terminal, ToPublicKey, }; impl Terminal { @@ -261,7 +261,9 @@ impl crate::expression::FromTree for Termina .map(Terminal::After) }), ("older", 1) => expression::terminal(&top.args[0], |x| { - expression::parse_num(x).map(|x| Terminal::Older(Sequence::from_consensus(x))) + expression::parse_num(x) + .and_then(|x| RelLockTime::from_consensus(x).map_err(Error::RelativeLockTime)) + .map(Terminal::Older) }), ("sha256", 1) => expression::terminal(&top.args[0], |x| { Pk::Sha256::from_str(x).map(Terminal::Sha256) diff --git a/src/miniscript/decode.rs b/src/miniscript/decode.rs index 920702e84..de2ab195f 100644 --- a/src/miniscript/decode.rs +++ b/src/miniscript/decode.rs @@ -11,7 +11,7 @@ use core::marker::PhantomData; use std::error; use bitcoin::hashes::{hash160, ripemd160, sha256, Hash}; -use bitcoin::{Sequence, Weight}; +use bitcoin::Weight; use sync::Arc; use crate::miniscript::lex::{Token as Tk, TokenIter}; @@ -22,7 +22,7 @@ use crate::miniscript::ScriptContext; use crate::prelude::*; #[cfg(doc)] use crate::Descriptor; -use crate::{hash256, AbsLockTime, Error, Miniscript, MiniscriptKey, ToPublicKey}; +use crate::{hash256, AbsLockTime, Error, Miniscript, MiniscriptKey, RelLockTime, ToPublicKey}; /// Trait for parsing keys from byte slices pub trait ParseableKey: Sized + ToPublicKey + private::Sealed { @@ -137,7 +137,7 @@ pub enum Terminal { /// `n CHECKLOCKTIMEVERIFY` After(AbsLockTime), /// `n CHECKSEQUENCEVERIFY` - Older(Sequence), + Older(RelLockTime), // hashlocks /// `SIZE 32 EQUALVERIFY SHA256 EQUAL` Sha256(Pk::Sha256), @@ -357,7 +357,7 @@ pub fn parse( }, // timelocks Tk::CheckSequenceVerify, Tk::Num(n) - => term.reduce0(Terminal::Older(Sequence::from_consensus(n)))?, + => term.reduce0(Terminal::Older(RelLockTime::from_consensus(n).map_err(Error::RelativeLockTime)?))?, Tk::CheckLockTimeVerify, Tk::Num(n) => term.reduce0(Terminal::After(AbsLockTime::from_consensus(n).map_err(Error::AbsoluteLockTime)?))?, // hashlocks diff --git a/src/miniscript/satisfy.rs b/src/miniscript/satisfy.rs index afd600d0f..bf0a696fd 100644 --- a/src/miniscript/satisfy.rs +++ b/src/miniscript/satisfy.rs @@ -1285,8 +1285,6 @@ impl Satisfaction> { Satisfaction { stack, has_sig: false, relative_timelock: None, absolute_timelock } } Terminal::Older(t) => { - // unwrap to be removed in a later commit - let t = >::try_from(t).unwrap(); let (stack, relative_timelock) = if stfr.check_older(t.into()) { (Witness::empty(), Some(t)) } else if root_has_sig { diff --git a/src/miniscript/types/extra_props.rs b/src/miniscript/types/extra_props.rs index 2bbf5a05a..dbdf1657c 100644 --- a/src/miniscript/types/extra_props.rs +++ b/src/miniscript/types/extra_props.rs @@ -6,12 +6,10 @@ use core::cmp; use core::iter::once; -use bitcoin::Sequence; - use super::{Error, ErrorKind, ScriptContext}; use crate::miniscript::context::SigType; use crate::prelude::*; -use crate::{script_num_size, AbsLockTime, MiniscriptKey, Terminal}; +use crate::{script_num_size, AbsLockTime, MiniscriptKey, RelLockTime, Terminal}; /// Timelock information for satisfaction of a fragment. #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Debug, Default, Hash)] @@ -365,7 +363,7 @@ impl ExtData { } /// Extra properties for the `older` fragment. - pub fn older(t: Sequence) -> Self { + pub fn older(t: RelLockTime) -> Self { ExtData { pk_cost: script_num_size(t.to_consensus_u32() as usize) + 1, has_free_verify: false, @@ -924,15 +922,7 @@ impl ExtData { } } Terminal::After(t) => Self::after(t), - Terminal::Older(t) => { - if t == Sequence::ZERO || !t.is_relative_lock_time() { - return Err(Error { - fragment_string: fragment.to_string(), - error: ErrorKind::InvalidTime, - }); - } - Self::older(t) - } + Terminal::Older(t) => Self::older(t), Terminal::Sha256(..) => Self::sha256(), Terminal::Hash256(..) => Self::hash256(), Terminal::Ripemd160(..) => Self::ripemd160(), diff --git a/src/miniscript/types/mod.rs b/src/miniscript/types/mod.rs index 05e185767..2ab554ca1 100644 --- a/src/miniscript/types/mod.rs +++ b/src/miniscript/types/mod.rs @@ -14,8 +14,6 @@ use core::fmt; #[cfg(feature = "std")] use std::error; -use bitcoin::Sequence; - pub use self::correctness::{Base, Correctness, Input}; pub use self::extra_props::ExtData; pub use self::malleability::{Dissat, Malleability}; @@ -25,8 +23,6 @@ use crate::{MiniscriptKey, Terminal}; /// Detailed type of a typechecker error #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)] pub enum ErrorKind { - /// Relative or absolute timelock had an invalid time value (either 0, or >=0x80000000) - InvalidTime, /// Passed a `z` argument to a `d` wrapper when `z` was expected NonZeroDupIf, /// Multisignature or threshold policy had a `k` value of 0 @@ -79,11 +75,6 @@ pub struct Error { impl fmt::Display for Error { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self.error { - ErrorKind::InvalidTime => write!( - f, - "fragment «{}» represents a timelock which value is invalid (time must be in [1; 0x80000000])", - self.fragment_string, - ), ErrorKind::NonZeroDupIf => write!( f, "fragment «{}» represents needs to be `z`, needs to consume zero elements from the stack", @@ -479,15 +470,7 @@ impl Type { } } Terminal::After(_) => Ok(Self::time()), - Terminal::Older(t) => { - if t == Sequence::ZERO || !t.is_relative_lock_time() { - return Err(Error { - fragment_string: fragment.to_string(), - error: ErrorKind::InvalidTime, - }); - } - Ok(Self::time()) - } + Terminal::Older(_) => Ok(Self::time()), Terminal::Sha256(..) => Ok(Self::hash()), Terminal::Hash256(..) => Ok(Self::hash()), Terminal::Ripemd160(..) => Ok(Self::hash()), diff --git a/src/policy/compiler.rs b/src/policy/compiler.rs index 8b7dc1957..d9ec92455 100644 --- a/src/policy/compiler.rs +++ b/src/policy/compiler.rs @@ -9,7 +9,6 @@ use core::{cmp, f64, fmt, hash, mem}; #[cfg(feature = "std")] use std::error; -use bitcoin::Sequence; use sync::Arc; use crate::miniscript::context::SigType; @@ -447,15 +446,7 @@ impl CompilerExtData { } } Terminal::After(_) => Ok(Self::time()), - Terminal::Older(t) => { - if t == Sequence::ZERO || !t.is_relative_lock_time() { - return Err(types::Error { - fragment_string: fragment.to_string(), - error: types::ErrorKind::InvalidTime, - }); - } - Ok(Self::time()) - } + Terminal::Older(_) => Ok(Self::time()), Terminal::Sha256(..) => Ok(Self::hash()), Terminal::Hash256(..) => Ok(Self::hash()), Terminal::Ripemd160(..) => Ok(Self::hash()), @@ -871,7 +862,7 @@ where insert_wrap!(AstElemExt::terminal(Terminal::PkK(pk.clone()))); } Concrete::After(n) => insert_wrap!(AstElemExt::terminal(Terminal::After(n))), - Concrete::Older(n) => insert_wrap!(AstElemExt::terminal(Terminal::Older(n.into()))), + Concrete::Older(n) => insert_wrap!(AstElemExt::terminal(Terminal::Older(n))), Concrete::Sha256(ref hash) => { insert_wrap!(AstElemExt::terminal(Terminal::Sha256(hash.clone()))) } diff --git a/src/policy/mod.rs b/src/policy/mod.rs index 190a3a47e..99dade57e 100644 --- a/src/policy/mod.rs +++ b/src/policy/mod.rs @@ -123,9 +123,7 @@ impl Liftable for Terminal { return Err(Error::LiftError(LiftError::RawDescriptorLift)) } Terminal::After(t) => Semantic::After(t), - Terminal::Older(t) => Semantic::Older( - >::try_from(t).unwrap(), - ), // unwrap to be removed in future commit + Terminal::Older(t) => Semantic::Older(t), Terminal::Sha256(ref h) => Semantic::Sha256(h.clone()), Terminal::Hash256(ref h) => Semantic::Hash256(h.clone()), Terminal::Ripemd160(ref h) => Semantic::Ripemd160(h.clone()), From 422b83cb198976ee0932686dc7dd4bdf396a9c8b Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Wed, 6 Mar 2024 20:31:22 +0000 Subject: [PATCH 10/10] interpreter: use `relative::LockTime` for constraints from Miniscript In general, the type from the transaction should be `Sequence` while the type we are comparing to should be `RelLockTime` or `relative::LockTime`. Having different types here should help us avoid getting these comparisons backward. --- src/interpreter/error.rs | 26 ++++++++++++++++---------- src/interpreter/mod.rs | 8 +++++--- src/interpreter/stack.rs | 28 ++++++++++++++-------------- 3 files changed, 35 insertions(+), 27 deletions(-) diff --git a/src/interpreter/error.rs b/src/interpreter/error.rs index f0aa8fb97..d19c28bea 100644 --- a/src/interpreter/error.rs +++ b/src/interpreter/error.rs @@ -9,7 +9,7 @@ use bitcoin::hashes::hash160; use bitcoin::hex::DisplayHex; #[cfg(not(test))] // https://github.com/rust-lang/rust/issues/121684 use bitcoin::secp256k1; -use bitcoin::taproot; +use bitcoin::{absolute, relative, taproot}; use super::BitcoinKey; use crate::prelude::*; @@ -18,9 +18,9 @@ use crate::prelude::*; #[derive(Debug)] pub enum Error { /// Could not satisfy, absolute locktime not met - AbsoluteLocktimeNotMet(u32), + AbsoluteLockTimeNotMet(absolute::LockTime), /// Could not satisfy, lock time values are different units - AbsoluteLocktimeComparisonInvalid(u32, u32), + AbsoluteLockTimeComparisonInvalid(absolute::LockTime, absolute::LockTime), /// Cannot Infer a taproot descriptor /// Key spends cannot infer the internal key of the descriptor /// Inferring script spends is possible, but is hidden nodes are currently @@ -85,7 +85,9 @@ pub enum Error { /// Parse Error while parsing a `stack::Element::Push` as a XOnlyPublicKey (32 bytes) XOnlyPublicKeyParseError, /// Could not satisfy, relative locktime not met - RelativeLocktimeNotMet(u32), + RelativeLockTimeNotMet(relative::LockTime), + /// Could not satisfy, the sequence number on the tx input had the disable flag set. + RelativeLockTimeDisabled(relative::LockTime), /// Forward-secp related errors Secp(secp256k1::Error), /// Miniscript requires the entire top level script to be satisfied. @@ -116,10 +118,10 @@ pub enum Error { impl fmt::Display for Error { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match *self { - Error::AbsoluteLocktimeNotMet(n) => { + Error::AbsoluteLockTimeNotMet(n) => { write!(f, "required absolute locktime CLTV of {} blocks, not met", n) } - Error::AbsoluteLocktimeComparisonInvalid(n, lock_time) => write!( + Error::AbsoluteLockTimeComparisonInvalid(n, lock_time) => write!( f, "could not satisfy, lock time values are different units n: {} lock_time: {}", n, lock_time @@ -159,9 +161,12 @@ impl fmt::Display for Error { Error::PkHashVerifyFail(ref hash) => write!(f, "Pubkey Hash check failed {}", hash), Error::PubkeyParseError => f.write_str("could not parse pubkey"), Error::XOnlyPublicKeyParseError => f.write_str("could not parse x-only pubkey"), - Error::RelativeLocktimeNotMet(n) => { + Error::RelativeLockTimeNotMet(n) => { write!(f, "required relative locktime CSV of {} blocks, not met", n) } + Error::RelativeLockTimeDisabled(n) => { + write!(f, "required relative locktime CSV of {} blocks, but tx sequence number has disable-flag set", n) + } Error::ScriptSatisfactionError => f.write_str("Top level script must be satisfied"), Error::Secp(ref e) => fmt::Display::fmt(e, f), Error::SchnorrSig(ref s) => write!(f, "Schnorr sig error: {}", s), @@ -188,8 +193,8 @@ impl error::Error for Error { use self::Error::*; match self { - AbsoluteLocktimeNotMet(_) - | AbsoluteLocktimeComparisonInvalid(_, _) + AbsoluteLockTimeNotMet(_) + | AbsoluteLockTimeComparisonInvalid(_, _) | CannotInferTrDescriptors | ControlBlockVerificationError | CouldNotEvaluate @@ -212,7 +217,8 @@ impl error::Error for Error { | XOnlyPublicKeyParseError | PkEvaluationError(_) | PkHashVerifyFail(_) - | RelativeLocktimeNotMet(_) + | RelativeLockTimeNotMet(_) + | RelativeLockTimeDisabled(_) | ScriptSatisfactionError | TapAnnexUnsupported | UncompressedPubkey diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs index 2c37e4496..dddffa8f0 100644 --- a/src/interpreter/mod.rs +++ b/src/interpreter/mod.rs @@ -12,7 +12,7 @@ use core::fmt; use core::str::FromStr; use bitcoin::hashes::{hash160, ripemd160, sha256, Hash}; -use bitcoin::{absolute, secp256k1, sighash, taproot, Sequence, TxOut, Witness}; +use bitcoin::{absolute, relative, secp256k1, sighash, taproot, Sequence, TxOut, Witness}; use crate::miniscript::context::{NoChecks, SigType}; use crate::miniscript::ScriptContext; @@ -468,7 +468,7 @@ pub enum SatisfiedConstraint { ///Relative Timelock for CSV. RelativeTimelock { /// The value of RelativeTimelock - n: Sequence, + n: relative::LockTime, }, ///Absolute Timelock for CLTV. AbsoluteTimelock { @@ -1182,7 +1182,9 @@ mod tests { let older_satisfied: Result, Error> = constraints.collect(); assert_eq!( older_satisfied.unwrap(), - vec![SatisfiedConstraint::RelativeTimelock { n: Sequence::from_height(1000) }] + vec![SatisfiedConstraint::RelativeTimelock { + n: crate::RelLockTime::from_height(1000).into() + }] ); //Check Sha256 diff --git a/src/interpreter/stack.rs b/src/interpreter/stack.rs index 760459916..4cf5450af 100644 --- a/src/interpreter/stack.rs +++ b/src/interpreter/stack.rs @@ -5,7 +5,7 @@ use bitcoin::blockdata::{opcodes, script}; use bitcoin::hashes::{hash160, ripemd160, sha256, Hash}; -use bitcoin::{absolute, Sequence}; +use bitcoin::{absolute, relative, Sequence}; use super::error::PkEvalErrInner; use super::{verify_sersig, BitcoinKey, Error, HashLockType, KeySigPair, SatisfiedConstraint}; @@ -212,19 +212,14 @@ impl<'txin> Stack<'txin> { let is_satisfied = match (*n, lock_time) { (Blocks(n), Blocks(lock_time)) => n <= lock_time, (Seconds(n), Seconds(lock_time)) => n <= lock_time, - _ => { - return Some(Err(Error::AbsoluteLocktimeComparisonInvalid( - n.to_consensus_u32(), - lock_time.to_consensus_u32(), - ))) - } + _ => return Some(Err(Error::AbsoluteLockTimeComparisonInvalid(*n, lock_time))), }; if is_satisfied { self.push(Element::Satisfied); Some(Ok(SatisfiedConstraint::AbsoluteTimelock { n: *n })) } else { - Some(Err(Error::AbsoluteLocktimeNotMet(n.to_consensus_u32()))) + Some(Err(Error::AbsoluteLockTimeNotMet(*n))) } } @@ -236,14 +231,19 @@ impl<'txin> Stack<'txin> { /// booleans pub(super) fn evaluate_older( &mut self, - n: &Sequence, - age: Sequence, + n: &relative::LockTime, + sequence: Sequence, ) -> Option> { - if age >= *n { - self.push(Element::Satisfied); - Some(Ok(SatisfiedConstraint::RelativeTimelock { n: *n })) + if let Some(tx_locktime) = sequence.to_relative_lock_time() { + if n.is_implied_by(tx_locktime) { + self.push(Element::Satisfied); + Some(Ok(SatisfiedConstraint::RelativeTimelock { n: *n })) + } else { + Some(Err(Error::RelativeLockTimeNotMet(*n))) + } } else { - Some(Err(Error::RelativeLocktimeNotMet(n.to_consensus_u32()))) + // BIP 112: if the tx locktime has the disable flag set, fail CSV. + Some(Err(Error::RelativeLockTimeDisabled(*n))) } }