diff --git a/src/miniscript/display.rs b/src/miniscript/display.rs index 0d93799c9..2cca6f89d 100644 --- a/src/miniscript/display.rs +++ b/src/miniscript/display.rs @@ -343,8 +343,12 @@ impl Ord for Terminal { (DisplayNode::ThresholdK(me), DisplayNode::ThresholdK(you)) => me.cmp(&you), (DisplayNode::Key(me), DisplayNode::Key(you)) => me.cmp(you), (DisplayNode::RawKeyHash(me), DisplayNode::RawKeyHash(you)) => me.cmp(you), - (DisplayNode::After(me), DisplayNode::After(you)) => me.cmp(you), - (DisplayNode::Older(me), DisplayNode::Older(you)) => me.cmp(you), + (DisplayNode::After(me), DisplayNode::After(you)) => { + me.cmp_by_consensus(*you) + } + (DisplayNode::Older(me), DisplayNode::Older(you)) => { + me.cmp_by_consensus(*you) + } (DisplayNode::Sha256(me), DisplayNode::Sha256(you)) => me.cmp(you), (DisplayNode::Hash256(me), DisplayNode::Hash256(you)) => me.cmp(you), (DisplayNode::Ripemd160(me), DisplayNode::Ripemd160(you)) => me.cmp(you), diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs index 8cc6d6a9f..c6d78677f 100644 --- a/src/miniscript/mod.rs +++ b/src/miniscript/mod.rs @@ -1262,7 +1262,8 @@ mod tests { use crate::policy::Liftable; use crate::test_utils::{StrKeyTranslator, StrXOnlyKeyTranslator}; use crate::{ - hex_script, BareCtx, Error, Legacy, RelLockTime, Satisfier, ToPublicKey, ValidationError, + hex_script, AbsLockTime, BareCtx, Error, Legacy, RelLockTime, Satisfier, ToPublicKey, + ValidationError, }; type Segwitv0Script = Miniscript; @@ -1979,6 +1980,14 @@ mod tests { ); } + #[test] + fn terminal_ord_locktimes_are_consistent() { + let a = Miniscript::::after(AbsLockTime::from_consensus(9).unwrap()); + let b = Miniscript::::after(AbsLockTime::from_consensus(10).unwrap()); + // Matches policy ordering: 9 < 10 under cmp_by_consensus + assert!(a < b, "after(9) < after(10) under consensus u32 ordering"); + } + #[test] fn template_timelocks() { use crate::{AbsLockTime, RelLockTime}; diff --git a/src/miniscript/satisfy/mod.rs b/src/miniscript/satisfy/mod.rs index 849e959cd..213fc25b2 100644 --- a/src/miniscript/satisfy/mod.rs +++ b/src/miniscript/satisfy/mod.rs @@ -93,18 +93,20 @@ pub trait Satisfier { /// Given a HASH160 hash, look up its preimage fn lookup_hash160(&self, _: &Pk::Hash160) -> Option { None } - /// Assert whether an relative locktime is satisfied + /// Returns whether the given relative locktime is satisfied. /// - /// 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. + /// Composite satisfiers may expose both height- and time-based locktimes. + /// If a single witness path would require incompatible timelock kinds + /// simultaneously, satisfaction for that path becomes [`Witness::Impossible`] + /// and plan building fails or selects another path. fn check_older(&self, _: relative::LockTime) -> bool { false } - /// Assert whether a absolute locktime is satisfied + /// Returns whether the given absolute locktime is satisfied. /// - /// 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. + /// Composite satisfiers may expose both height- and time-based locktimes. + /// If a single witness path would require incompatible timelock kinds + /// simultaneously, satisfaction for that path becomes [`Witness::Impossible`] + /// and plan building fails or selects another path. fn check_after(&self, _: absolute::LockTime) -> bool { false } } @@ -563,29 +565,73 @@ mod tests { #[test] fn regression_895() { - // Tests a pathological descriptor whose cheapest satisfaction would require mixing - // timelocks, although a more expensive satisfaction is available that avoids the - // timelock mixing. This descriptor is accepted by rust-miniscript but its satisfier - // cannot produce a satisfaction for it. + // Tests a pathological descriptor whose cheapest satisfaction involves computing a + // dissatisfaction containing a timelock. Such dissatisfactions exist because the + // `and_v` fragment, uniquely, has a dissatisfaction that includes the satisfaction + // of its left child. (Check sat_dissat.rs and search 'dissat: ' to see how the + // dissatisfactions of every fragment are computed. You will see that exactly one, + // and_v, uses the satisfaction of a child.) // - // Prior to PR #895, the satisfaction logic would yield the invalid timelock-mixing - // satisfaction. After PR #895, it yields no satisfaction at all. - // - // The correct behavior is arguably to find the more expensive satisfaction. Doing - // this would would require some sort of backtracking in the satisfier and is highly - // nontrivial. Since triggering this bug requires generating a satisfaction in a - // context where both a height-based and time-based timelock are available, an - // impossible situation, it is probably not worth fixing. + // Prior to PR #895, the satisfier did not keep track of timelocks that were used + // for dissatisfactions. This can lead to incorrect plans, as demonstrated in the + // below test vectors. - // Setup: an unavailable key, used to make some branches unsatisfiable from the POV + // Setup: unavailable keys, used to make some branches unsatisfiable from the POV // of the satisfier (but not the typechecker). - let unavailable_key = PublicKey::from_str( - "02eb64639a17f7334bb5a1a3aad857d6fec65faef439db3de72f85c88bc2906ad3", - ) - .unwrap(); + let available_keys: [PublicKey; 2] = [ + "02eb64639a17f7334bb5a1a3aad857d6fec65faef439db3de72f85c88bc2906ad1" + .parse() + .unwrap(), + "02eb64639a17f7334bb5a1a3aad857d6fec65faef439db3de72f85c88bc2906ad3" + .parse() + .unwrap(), + ]; + let available_key_map = { + let dummy_sig = bitcoin::ecdsa::Signature::sighash_all( + secp256k1::ecdsa::Signature::from_compact(&[1; 64]).unwrap(), + ); + let mut map = std::collections::BTreeMap::new(); + map.insert( + crate::DefiniteDescriptorKey::new(available_keys[0].into()).unwrap(), + dummy_sig, + ); + map.insert( + crate::DefiniteDescriptorKey::new(available_keys[1].into()).unwrap(), + dummy_sig, + ); + map + }; + + // Generate a large pile of distinct unavailable keys. + let secp = secp256k1::Secp256k1::new(); + let unavailable_keys: Vec = + core::iter::successors(Some(available_keys[0]), |prev| { + prev.inner + .add_exp_tweak(&secp, &secp256k1::Scalar::ONE) + .ok() + .map(PublicKey::new) + }) + .skip(1) + .take(20) + .collect(); + assert_eq!(unavailable_keys.len(), 20); // sanity-check the above loop + + // Create a fragment which is very expensive to dissatisfy. + let expensive_threshold = format!( + "thresh(10,pk({}),{})", + unavailable_keys[1], // we need key[0] below + unavailable_keys + .iter() + .skip(2) + .map(|k| format!("a:pkh({k})")) + .collect::>() + .join(","), + ); + // Setup: a satisfier that thinks that both a height-based and time-based timelock // are available. This is needed for the second test. let satisfier = ( + available_key_map, absolute::LockTime::from_height(1000).unwrap(), absolute::LockTime::from_time(2000000000).unwrap(), ); @@ -593,59 +639,64 @@ mod tests { // Construct a script that would mix timelocks: // or_b( // n:or_i( - // and_v(v:after(144),pk()), // dissatisfied by a height-based timelock - // thresh(3,pk(),s:pk(),s:pk()) // dissatisfied by 3empty sigs (more expensive) + // and_v(v:after(144),and_v(v:pk(),pk())), // dissatisfied by a height-based timelock, a sig, and 0 + // expensive_threshold // dissatisfied by a giant pile of pubkeyhash preimages // ), - // sdv:after(50) // satisfied by a lower height-based timelock + // ajt:and_v(v:after(50),v:pk({})))) // satisfied by a lower height-based timelock and a sig // ) // - // Here the or_i cannot be satisfied because none of the keys are available, so it + // Here the or_i cannot be satisfied due to missing keys on both branches, so it // must be dissatisfied (and the after(50) branch must be satisfied). However, there // are two dissatisfactions for the or_i: one which dissatisfies the first branch, // by using the height-based timelock, and one which dissatisfies the second branch, // which is ignored since it's the more expensive of the two possibilities. // - // We therefore take both the after(144) and after(50) branches, and the resulting - // plan should show after(144) since it's the higher one. However, prior to #895, - // we "did not notice" the after(144) since it appears as part of a dissatisfaction. + // Since the first branch's dissatisfaction is HASSIG but the second branch's is not, + // the satisfier is required to dissatisfy the second branch to avoid malleability. + // But if we use `into_plan_mall` we can see the bug. // - // Unrelatedly: the fact that the LHS of the `or_i` has a malleable dissatisfaction - // means that the whole script is malleable, and the fact that this parses at all is - // an instance of https://github.com/rust-bitcoin/rust-miniscript/issues/734 - + // Itstead, it takes both the after(144) and after(50) branches, and the resulting + // plan should show after(144) since it's the higher one. However, prior to #895, + // we "did not notice" the after(144) since it appears as part of a dissatisfaction, + // leading to a plan that did not match the actual timelock requirement. let descriptor_str = format!( - "wsh(or_b(n:or_i(and_v(v:after(144),pk({})),thresh(3,pk({}),s:pk({}),s:pk({}))),sdv:after(50)))", - unavailable_key, unavailable_key, unavailable_key, unavailable_key, + "wsh(or_b(n:or_i(and_v(v:after(144),and_v(v:pk({}),pk({}))),{expensive_threshold}),ajt:and_v(v:after(50),v:pk({}))))", + available_keys[0], unavailable_keys[0], available_keys[1], ); // Need DefiniteDescriptorKey https://github.com/rust-bitcoin/rust-miniscript/issues/927 let descriptor = Descriptor::::from_str(&descriptor_str).unwrap(); - // Compute plan and confirm the timelock is correct. - let plan = descriptor.into_plan(&satisfier).unwrap(); + // Compute plan and confirm the timelock is correct -- 144 for a malleable transaction + let plan = descriptor.clone().into_plan_mall(&satisfier).unwrap(); assert_eq!(plan.absolute_timelock, Some(absolute::LockTime::from_height(144).unwrap()),); + // ...and 50 for a non-malleable one (since take the expensive_threshold alternate) + let plan = descriptor.into_plan(&satisfier).unwrap(); + assert_eq!(plan.absolute_timelock, Some(absolute::LockTime::from_height(50).unwrap()),); // Same descriptor as above, except that now we use a time-based timelock rather than a - // lower height-based one. This time the "ideal" behavior would be that once we get to - // the final time-based timelock, we somehow backtrack and then use the more-expensive - // dissatisfaction choice for the `or_i`. (The type system guarantees that such a choice - // exists; otherwise the whole script would be flagged as mixing timelocks.) + // lower height-based one. // - // However, our code architecture doesn't let us backtrack like this, and it's only possible - // to get into this situation if (a) you have a pathological script like this, and (b) you - // call .plan() or .satisfy() with both a height-based and time-based timelock set (which - // is impossible for any actual transaction). So for now we just use this unit test to - // document the behavior. + // Again, both timelock branches are considered. When `concatenate_rev` must merge a + // height-based and a time-based absolute locktime on the same path, that satisfaction + // becomes `Witness::Impossible`. Plan building then fails or selects an alternate path. + // Parallel per-kind tracking remains future work (see #979 discussion). let descriptor_str = format!( - "wsh(or_b(n:or_i(and_v(v:after(144),pk({})),thresh(2,pk({}),s:pk({}))),sdv:after(1000000000)))", - unavailable_key, unavailable_key, unavailable_key, + "wsh(or_b(n:or_i(and_v(v:after(144),and_v(v:pk({}),pk({}))),{expensive_threshold}),ajt:and_v(v:after(1000000000),v:pk({}))))", + available_keys[0], unavailable_keys[0], available_keys[1], ); let descriptor = Descriptor::::from_str(&descriptor_str).unwrap(); + // Both `or_b` arms hit a height/time conflict in `concatenate_rev`, so no malleable + // plan exists. + assert!(descriptor.clone().into_plan_mall(&satisfier).is_err()); + // Non-malleable plan still succeeds: `into_plan` dissatisfies the expensive threshold + // branch and satisfies the time-based `after(1000000000)` branch without merging + // incompatible locktimes on one path. let plan = descriptor.into_plan(&satisfier).unwrap(); assert_eq!( plan.absolute_timelock, - Some(absolute::LockTime::from_time(1000000000).unwrap()), + Some(absolute::LockTime::from_time(1_000_000_000).unwrap()), ); } } @@ -978,6 +1029,13 @@ pub struct Satisfaction { } impl Satisfaction> { + const IMPOSSIBLE: Self = Self { + stack: Witness::Impossible, + has_sig: false, + relative_timelock: None, + absolute_timelock: None, + }; + /// The empty satisfaction. /// /// This has the property that, when concatenated on either side with another satisfaction @@ -997,11 +1055,36 @@ impl Satisfaction> { /// This order allows callers to write `left.concatenate_rev(right)` which feels more /// natural than the opposite order, and more importantly, allows this method to be /// used when folding over an iterator of multiple satisfactions. + /// + /// Same-unit locktimes merge to the later value via [`AbsLockTime::max`] and + /// [`RelLockTime::max`]. Mixed height/time on the same path yields + /// [`Witness::Impossible`]. Downstream [`Self::minimum`], [`Self::minimum_mall`], and + /// [`Self::thresh`] treat Impossible like other dead branches; [`Descriptor::into_plan`] and + /// [`Descriptor::into_plan_mall`] fail if the winning path is Impossible. fn concatenate_rev(self, other: Self) -> Self { + if self.stack == Witness::Impossible || other.stack == Witness::Impossible { + return Self::IMPOSSIBLE; + } + + let relative_timelock = match (self.relative_timelock, other.relative_timelock) { + (None, x) | (x, None) => x, + (Some(a), Some(b)) => match RelLockTime::max(a, b) { + Some(t) => Some(t), + None => return Self::IMPOSSIBLE, + }, + }; + let absolute_timelock = match (self.absolute_timelock, other.absolute_timelock) { + (None, x) | (x, None) => x, + (Some(a), Some(b)) => match AbsLockTime::max(a, b) { + Some(t) => Some(t), + None => return Self::IMPOSSIBLE, + }, + }; + Self { has_sig: self.has_sig || other.has_sig, - relative_timelock: cmp::max(self.relative_timelock, other.relative_timelock), - absolute_timelock: cmp::max(self.absolute_timelock, other.absolute_timelock), + relative_timelock, + absolute_timelock, stack: Witness::combine(other.stack, self.stack), } } @@ -1069,14 +1152,7 @@ impl Satisfaction> { // For example, the fragment thresh(2, hash, 0, 0, 0) // is has an impossible witness if sats[sat_indices[k - 1]].stack == Witness::Impossible { - Self { - stack: Witness::Impossible, - // If the witness is impossible, we don't care about the - // has_sig flag, nor about the timelocks - has_sig: false, - relative_timelock: None, - absolute_timelock: None, - } + Self::IMPOSSIBLE } // We are now guaranteed that all elements in `k` satisfactions // are not impossible(we sort by is_impossible bool). diff --git a/src/miniscript/satisfy/sat_dissat.rs b/src/miniscript/satisfy/sat_dissat.rs index 53cb17fd8..6bd8bd36f 100644 --- a/src/miniscript/satisfy/sat_dissat.rs +++ b/src/miniscript/satisfy/sat_dissat.rs @@ -21,13 +21,6 @@ pub(super) struct SatDissat { } impl Satisfaction> { - const IMPOSSIBLE: Self = Self { - stack: Witness::Impossible, - has_sig: false, - relative_timelock: None, - absolute_timelock: None, - }; - const TRIVIAL: Self = Self { stack: Witness::empty(), has_sig: false, @@ -442,18 +435,8 @@ impl Satisfaction> { Terminal::OrI(_, _) => { let SatDissat { dissat: r_dis, sat: r_sat } = stack.pop().unwrap(); let SatDissat { dissat: l_dis, sat: l_sat } = stack.pop().unwrap(); - // Regarding use of `minimum_mall` for dissatisfactions: this is correct, because - // dissatisfactions for individual fragments are not required to be unique. (We - // track this property using the `ty.malleability.dissat` field.) - // - // This is because usually dissatisfactions are eventually dropped, e.g. by the - // `j:` wrapper or because at the top-level a dissatisfaction is simply invalid. - // In cases where non-unique dissatisfactions would cause malleability, e.g. in - // the `or_c` or `or_b` fragments which use dissatisfactions as part of their - // satisfactions, we set `ty.malleability.non_malleable` appropriately (and - // potentially reject the whole script at creation time). SatDissat { - dissat: Self::minimum_mall( + dissat: min_fn( Self { stack: Witness::combine(l_dis.stack, Witness::push_1()), ..l_dis diff --git a/src/policy/concrete.rs b/src/policy/concrete.rs index 6827ce7cc..140345abc 100644 --- a/src/policy/concrete.rs +++ b/src/policy/concrete.rs @@ -3,7 +3,7 @@ //! Concrete Policies //! -use core::{fmt, str}; +use core::{cmp, fmt, str}; #[cfg(feature = "std")] use std::error; @@ -41,7 +41,7 @@ const MAX_COMPILATION_LEAVES: usize = 1024; // Currently the vectors in And/Or are limited to two elements, this is a general miniscript thing // not specific to rust-miniscript. Eventually we would like to extend these to be n-ary, but first // we need to decide on a game plan for how to efficiently compile n-ary disjunctions -#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +#[derive(Clone, PartialEq, Eq, Hash)] pub enum Policy { /// Unsatisfiable. Unsatisfiable, @@ -70,6 +70,53 @@ pub enum Policy { Thresh(Threshold, 0>), } +impl Policy { + fn variant_name(&self) -> &'static str { + match *self { + Self::Unsatisfiable => "unsatisfiable", + Self::Trivial => "trivial", + Self::Key(_) => "key", + Self::After(_) => "after", + Self::Older(_) => "older", + Self::Sha256(_) => "sha256", + Self::Hash256(_) => "hash256", + Self::Ripemd160(_) => "ripemd160", + Self::Hash160(_) => "hash160", + Self::And(_) => "and", + Self::Or(_) => "or", + Self::Thresh(_) => "thresh", + } + } +} + +impl PartialOrd for Policy { + fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) } +} + +impl Ord for Policy { + fn cmp(&self, other: &Self) -> cmp::Ordering { + match self.variant_name().cmp(other.variant_name()) { + cmp::Ordering::Equal => {} + ord => return ord, + } + match (self, other) { + (Self::Unsatisfiable, Self::Unsatisfiable) => cmp::Ordering::Equal, + (Self::Trivial, Self::Trivial) => cmp::Ordering::Equal, + (Self::Key(a), Self::Key(b)) => a.cmp(b), + (Self::After(a), Self::After(b)) => a.cmp_by_consensus(*b), + (Self::Older(a), Self::Older(b)) => a.cmp_by_consensus(*b), + (Self::Sha256(a), Self::Sha256(b)) => a.cmp(b), + (Self::Hash256(a), Self::Hash256(b)) => a.cmp(b), + (Self::Ripemd160(a), Self::Ripemd160(b)) => a.cmp(b), + (Self::Hash160(a), Self::Hash160(b)) => a.cmp(b), + (Self::And(a), Self::And(b)) => a.cmp(b), + (Self::Or(a), Self::Or(b)) => a.cmp(b), + (Self::Thresh(a), Self::Thresh(b)) => a.cmp(b), + _ => unreachable!("variant_name ensures same variant"), + } + } +} + /// Detailed error type for concrete policies. #[derive(Copy, Clone, PartialEq, Eq, Debug, Hash)] pub enum PolicyError { @@ -1389,4 +1436,26 @@ mod tests { .parse::>() .unwrap_err(); } + + #[test] + fn policy_ord_is_consistent() { + // Same direction as numeric — passes under both old and new scheme. + let a = Policy::::from_str("after(100)").unwrap(); + let b = Policy::::from_str("after(200)").unwrap(); + assert!(a < b, "after(100) should be less than after(200)"); + + // Cross-variant: must not be equal. + let c = Policy::::from_str("older(100)").unwrap(); + assert!(a != c, "after and older variants must not compare equal"); + + // Numeric consensus ordering: 9 < 10. + let d = Policy::::from_str("after(9)").unwrap(); + let e = Policy::::from_str("after(10)").unwrap(); + assert!(d < e, "after(9) < after(10) under consensus u32 ordering"); + + // "trivial" < "unsatisfiable" alphabetically. + let trivial = Policy::::from_str("TRIVIAL").unwrap(); + let unsat = Policy::::from_str("UNSATISFIABLE").unwrap(); + assert!(trivial < unsat, "trivial < unsatisfiable under variant_name ordering"); + } } diff --git a/src/policy/semantic.rs b/src/policy/semantic.rs index 13f05e76d..a35ad5aed 100644 --- a/src/policy/semantic.rs +++ b/src/policy/semantic.rs @@ -5,7 +5,7 @@ //! We use the terms "semantic" and "abstract" interchangeably because //! "abstract" is a reserved keyword in Rust. -use core::{fmt, str}; +use core::{cmp, fmt, str}; use bitcoin::{absolute, relative}; @@ -24,7 +24,7 @@ use crate::{ /// Semantic policies store only hashes of keys to ensure that objects /// representing the same policy are lifted to the same abstract `Policy`, /// regardless of their choice of `pk` or `pk_h` nodes. -#[derive(Clone, PartialEq, Eq, PartialOrd, Ord)] +#[derive(Clone, PartialEq, Eq)] pub enum Policy { /// Unsatisfiable. Unsatisfiable, @@ -48,6 +48,49 @@ pub enum Policy { Thresh(Threshold, 0>), } +impl Policy { + fn variant_name(&self) -> &'static str { + match *self { + Self::Unsatisfiable => "unsatisfiable", + Self::Trivial => "trivial", + Self::Key(_) => "key", + Self::After(_) => "after", + Self::Older(_) => "older", + Self::Sha256(_) => "sha256", + Self::Hash256(_) => "hash256", + Self::Ripemd160(_) => "ripemd160", + Self::Hash160(_) => "hash160", + Self::Thresh(_) => "thresh", + } + } +} + +impl PartialOrd for Policy { + fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) } +} + +impl Ord for Policy { + fn cmp(&self, other: &Self) -> cmp::Ordering { + match self.variant_name().cmp(other.variant_name()) { + cmp::Ordering::Equal => {} + ord => return ord, + } + match (self, other) { + (Self::Unsatisfiable, Self::Unsatisfiable) => cmp::Ordering::Equal, + (Self::Trivial, Self::Trivial) => cmp::Ordering::Equal, + (Self::Key(a), Self::Key(b)) => a.cmp(b), + (Self::After(a), Self::After(b)) => a.cmp_by_consensus(*b), + (Self::Older(a), Self::Older(b)) => a.cmp_by_consensus(*b), + (Self::Sha256(a), Self::Sha256(b)) => a.cmp(b), + (Self::Hash256(a), Self::Hash256(b)) => a.cmp(b), + (Self::Ripemd160(a), Self::Ripemd160(b)) => a.cmp(b), + (Self::Hash160(a), Self::Hash160(b)) => a.cmp(b), + (Self::Thresh(a), Self::Thresh(b)) => a.cmp(b), + _ => unreachable!("variant_name ensures same variant"), + } + } +} + 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 { @@ -687,6 +730,28 @@ mod tests { type StringPolicy = Policy; + #[test] + fn policy_ord_is_consistent() { + // Same direction as numeric — passes under both old and new scheme. + let a = StringPolicy::from_str("after(100)").unwrap(); + let b = StringPolicy::from_str("after(200)").unwrap(); + assert!(a < b, "after(100) should be less than after(200)"); + + // Cross-variant: must not be equal. + let c = StringPolicy::from_str("older(100)").unwrap(); + assert!(a != c, "after and older variants must not compare equal"); + + // Numeric consensus ordering: 9 < 10. + let d = StringPolicy::from_str("after(9)").unwrap(); + let e = StringPolicy::from_str("after(10)").unwrap(); + assert!(d < e, "after(9) < after(10) under consensus u32 ordering"); + + // "trivial" < "unsatisfiable" alphabetically. + let trivial = StringPolicy::from_str("TRIVIAL").unwrap(); + let unsat = StringPolicy::from_str("UNSATISFIABLE").unwrap(); + assert!(trivial < unsat, "trivial < unsatisfiable under variant_name ordering"); + } + #[test] fn parse_policy_err() { assert!(StringPolicy::from_str("(").is_err()); diff --git a/src/primitives/absolute_locktime.rs b/src/primitives/absolute_locktime.rs index eaf177091..8027a4e49 100644 --- a/src/primitives/absolute_locktime.rs +++ b/src/primitives/absolute_locktime.rs @@ -42,7 +42,7 @@ impl std::error::Error for AbsLockTimeError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { None } } -/// An absolute locktime that implements `Ord`. +/// An absolute locktime that cannot be zero. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub struct AbsLockTime(absolute::LockTime); @@ -68,22 +68,25 @@ impl AbsLockTime { /// 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) -> Self { lock_time.0 } -} + /// Compares two locktimes by their consensus `u32` encoding. + pub(crate) fn cmp_by_consensus(self, other: Self) -> cmp::Ordering { + self.to_consensus_u32().cmp(&other.to_consensus_u32()) + } -impl cmp::PartialOrd for AbsLockTime { - fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) } + /// Returns the later of two locktimes of the same unit, or `None` if units differ. + pub(crate) fn max(a: Self, b: Self) -> Option { + use core::cmp::Ordering::*; + match absolute::LockTime::from(a).partial_cmp(&absolute::LockTime::from(b)) { + Some(Greater) | Some(Equal) => Some(a), + Some(Less) => Some(b), + None => None, + } + } } -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 From for absolute::LockTime { + fn from(lock_time: AbsLockTime) -> Self { lock_time.0 } } impl fmt::Display for AbsLockTime { diff --git a/src/primitives/relative_locktime.rs b/src/primitives/relative_locktime.rs index 6640963cf..b562b1080 100644 --- a/src/primitives/relative_locktime.rs +++ b/src/primitives/relative_locktime.rs @@ -30,7 +30,7 @@ impl std::error::Error for RelLockTimeError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { None } } -/// A relative locktime which implements `Ord`. +/// A relative locktime that cannot be zero. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub struct RelLockTime(Sequence); @@ -65,6 +65,21 @@ impl RelLockTime { /// Whether this timelock is time-based. pub fn is_time_locked(&self) -> bool { self.0.is_time_locked() } + + /// Compares two locktimes by their consensus `u32` encoding. + pub(crate) fn cmp_by_consensus(self, other: Self) -> cmp::Ordering { + self.to_consensus_u32().cmp(&other.to_consensus_u32()) + } + + /// Returns the later of two locktimes of the same unit, or `None` if units differ. + pub(crate) fn max(a: Self, b: Self) -> Option { + use core::cmp::Ordering::*; + match relative::LockTime::from(a).partial_cmp(&relative::LockTime::from(b)) { + Some(Greater) | Some(Equal) => Some(a), + Some(Less) => Some(b), + None => None, + } + } } impl convert::TryFrom for RelLockTime { @@ -86,18 +101,6 @@ impl From for relative::LockTime { fn from(lock_time: RelLockTime) -> Self { 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) } }