Add manual Ord impls for concrete and semantic Policy.

Replace derive(Ord) with variant-name ordering and same-kind locktime
comparison by consensus u32, and add regression tests.

Author: 151henry151

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;
 
@@ -24,6 +24,7 @@ use crate::expression::{self, FromTree};
 use crate::iter::{Tree, TreeLike};
 use crate::miniscript::types::extra_props::TimelockInfo;
 use crate::prelude::*;
+use crate::primitives::locktime_consensus_cmp;
 use crate::sync::Arc;
 #[cfg(all(doc, not(feature = "compiler")))]
 use crate::Descriptor;
@@ -41,7 +42,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<Pk: MiniscriptKey> {
     /// Unsatisfiable.
     Unsatisfiable,
@@ -70,6 +71,57 @@ pub enum Policy<Pk: MiniscriptKey> {
     Thresh(Threshold<Arc<Self>, 0>),
 }
 
+impl<Pk: MiniscriptKey> Policy<Pk> {
+    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<Pk: MiniscriptKey> PartialOrd for Policy<Pk> {
+    fn partial_cmp(&self, other: &Self) -> Option<cmp::Ordering> { Some(self.cmp(other)) }
+}
+
+impl<Pk: MiniscriptKey> Ord for Policy<Pk> {
+    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)) => {
+                locktime_consensus_cmp(a.to_consensus_u32(), b.to_consensus_u32())
+            }
+            (Self::Older(a), Self::Older(b)) => {
+                locktime_consensus_cmp(a.to_consensus_u32(), b.to_consensus_u32())
+            }
+            (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 +1441,26 @@ mod tests {
             .parse::<Policy<String>>()
             .unwrap_err();
     }
+
+    #[test]
+    fn policy_ord_is_consistent() {
+        // Same direction as numeric — passes under both old and new scheme.
+        let a = Policy::<String>::from_str("after(100)").unwrap();
+        let b = Policy::<String>::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::<String>::from_str("older(100)").unwrap();
+        assert!(a != c, "after and older variants must not compare equal");
+
+        // Numeric consensus ordering: 9 < 10.
+        let d = Policy::<String>::from_str("after(9)").unwrap();
+        let e = Policy::<String>::from_str("after(10)").unwrap();
+        assert!(d < e, "after(9) < after(10) under consensus u32 ordering");
+
+        // "trivial" < "unsatisfiable" alphabetically.
+        let trivial = Policy::<String>::from_str("TRIVIAL").unwrap();
+        let unsat = Policy::<String>::from_str("UNSATISFIABLE").unwrap();
+        assert!(trivial < unsat, "trivial < unsatisfiable under variant_name ordering");
+    }
 }

src/policy/semantic.rs

@@ -5,13 +5,14 @@
 //! 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};
 
 use super::ENTAILMENT_MAX_TERMINALS;
 use crate::iter::{Tree, TreeLike};
 use crate::prelude::*;
+use crate::primitives::locktime_consensus_cmp;
 use crate::sync::Arc;
 use crate::{
     expression, AbsLockTime, Error, ForEachKey, FromStrKey, MiniscriptKey, RelLockTime, Threshold,
@@ -24,7 +25,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<Pk: MiniscriptKey> {
     /// Unsatisfiable.
     Unsatisfiable,
@@ -48,6 +49,53 @@ pub enum Policy<Pk: MiniscriptKey> {
     Thresh(Threshold<Arc<Self>, 0>),
 }
 
+impl<Pk: MiniscriptKey> Policy<Pk> {
+    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<Pk: MiniscriptKey> PartialOrd for Policy<Pk> {
+    fn partial_cmp(&self, other: &Self) -> Option<cmp::Ordering> { Some(self.cmp(other)) }
+}
+
+impl<Pk: MiniscriptKey> Ord for Policy<Pk> {
+    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)) => {
+                locktime_consensus_cmp(a.to_consensus_u32(), b.to_consensus_u32())
+            }
+            (Self::Older(a), Self::Older(b)) => {
+                locktime_consensus_cmp(a.to_consensus_u32(), b.to_consensus_u32())
+            }
+            (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<Pk: MiniscriptKey> ForEachKey<Pk> for Policy<Pk> {
     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 +735,28 @@ mod tests {
 
     type StringPolicy = Policy<String>;
 
+    #[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());

src/primitives/mod.rs

@@ -14,3 +14,11 @@
 pub mod absolute_locktime;
 pub mod relative_locktime;
 pub mod threshold;
+
+use core::cmp;
+
+/// Compares two same-kind locktimes by consensus `u32`.
+///
+/// Used for locktime ordering in `Policy::Ord`, `Terminal::cmp`, and (via the latter)
+/// `Miniscript::Ord`.
+pub(crate) fn locktime_consensus_cmp(a: u32, b: u32) -> cmp::Ordering { a.cmp(&b) }