From cfb93eb49f865f33907111c08deaa258fbe76404 Mon Sep 17 00:00:00 2001
From: Andrew Poelstra <apoelstra@wpsoftware.net>
Date: Mon, 19 May 2025 23:12:21 +0000
Subject: [PATCH 1/6] descriptor: fix up regression_895 test

This is supposed to be testing mixed timelocks, but it uses descriptors
that are invalid for several unrelated reasons. These will be enforced
in a later PR, so fix the issues now.

Specifically:

* It is not allowed to use the same `unavailable_key` multiple times
* The rightmost branch must not be sigless; the timelock needs to have a
   pubkey and'ed with it. Otherwise the whole script is malleable.
* Similarly the "cheap dissatisfaction" of v:after(144),pk({})) must
   not be sigless. We turn it into v:after(144),and_v(v:pk({})),pk({}))
   which requires a signature even to be dissatisfied.
* Also, the giant comments don't accurately reflect what's going on in
   this test. They claim that post-#895, we will refuse to create a
   satisfaction if we'd be forced into mixing timelocks. But we don't.
   We actually do the same wrong thing as before, just for different
   reasons.

The resulting test now passes even when all `ValidationParams` are turned
on, though in this PR we don't confirm this.

HOWEVER, despite the test working with a non-malleable descriptor now, the
satisfaction that this test produces is malleable. For this I have filed
https://github.com/rust-bitcoin/rust-miniscript/issues/976, which will be
fixed in the next commit.
---
 src/miniscript/satisfy/mod.rs | 130 +++++++++++++++++++++++-----------
 1 file changed, 89 insertions(+), 41 deletions(-)

diff --git a/src/miniscript/satisfy/mod.rs b/src/miniscript/satisfy/mod.rs
index 849e959cd..fba411c38 100644
--- a/src/miniscript/satisfy/mod.rs
+++ b/src/miniscript/satisfy/mod.rs
@@ -563,29 +563,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<PublicKey> =
+            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::<Vec<_>>()
+                .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,29 +637,29 @@ 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.
+        // However, because of #976, it does not.
         //
-        // 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 =
@@ -625,19 +669,23 @@ mod tests {
         assert_eq!(plan.absolute_timelock, Some(absolute::LockTime::from_height(144).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.
+        //
+        // Again, we wind up taking both timelock branches. Prior to #895, we would do this because
+        // we did not track the after(144) at all. After #895, we do it because our timelock logic
+        // handles mixed timelocks by silently clobbering one of them. (See the implementation of
+        // `concatenate_rev`, which calls `cmp::max` on the timelocks. This clobbers a locktime.
+        // See #979 for an explanation of this.)
+        //
+        // This time the "ideal" behavior would be that we track best satisfactions for both time-
+        // and height-based locktimes, and whenever we are forced into a conflict we throw one away.
+        // (The type system guarantees that we will always have at least one satisfaction left;
+        // otherwise the whole script would be flagged as mixing timelocks.)
         //
-        // 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.
+        // For now we just use this unit test to document the behavior.
         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::<crate::DefiniteDescriptorKey>::from_str(&descriptor_str).unwrap();

From 99501a5e52b039f40971c73ffea2ab2cb5bd5014 Mon Sep 17 00:00:00 2001
From: Andrew Poelstra <apoelstra@wpsoftware.net>
Date: Fri, 5 Jun 2026 17:47:12 +0000
Subject: [PATCH 2/6] miniscript: don't use malleable satisfier, even for or_i
 dissatisfactions

...unless the user has explicitly requested a malleable satisfaction.

Fixes #976
---
 src/miniscript/satisfy/mod.rs        | 11 +++++++----
 src/miniscript/satisfy/sat_dissat.rs | 12 +-----------
 2 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/src/miniscript/satisfy/mod.rs b/src/miniscript/satisfy/mod.rs
index fba411c38..31259d182 100644
--- a/src/miniscript/satisfy/mod.rs
+++ b/src/miniscript/satisfy/mod.rs
@@ -651,7 +651,7 @@ mod tests {
         //
         // 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.
-        // However, because of #976, it does not.
+        // But if we use `into_plan_mall` we can see the bug.
         //
         // 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,
@@ -664,9 +664,12 @@ mod tests {
         // Need DefiniteDescriptorKey https://github.com/rust-bitcoin/rust-miniscript/issues/927
         let descriptor =
             Descriptor::<crate::DefiniteDescriptorKey>::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.
@@ -690,7 +693,7 @@ mod tests {
         let descriptor =
             Descriptor::<crate::DefiniteDescriptorKey>::from_str(&descriptor_str).unwrap();
 
-        let plan = descriptor.into_plan(&satisfier).unwrap();
+        let plan = descriptor.into_plan_mall(&satisfier).unwrap();
         assert_eq!(
             plan.absolute_timelock,
             Some(absolute::LockTime::from_time(1000000000).unwrap()),
diff --git a/src/miniscript/satisfy/sat_dissat.rs b/src/miniscript/satisfy/sat_dissat.rs
index 53cb17fd8..2c5775af2 100644
--- a/src/miniscript/satisfy/sat_dissat.rs
+++ b/src/miniscript/satisfy/sat_dissat.rs
@@ -442,18 +442,8 @@ impl<Pk: MiniscriptKey + ToPublicKey> Satisfaction<Placeholder<Pk>> {
                 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

From f168ca04077d9f5606d02c80900f0722331194ee Mon Sep 17 00:00:00 2001
From: Henry Romp <151henry151@gmail.com>
Date: Sat, 13 Jun 2026 11:41:50 -0400
Subject: [PATCH 3/6] Add manual Ord impls for concrete and semantic Policy.

Replace derive(Ord) with variant-name ordering and same-kind locktime
comparison via cmp_by_consensus on the locktime types, and add regression tests.
---
 src/policy/concrete.rs              | 73 ++++++++++++++++++++++++++++-
 src/policy/semantic.rs              | 69 ++++++++++++++++++++++++++-
 src/primitives/absolute_locktime.rs |  5 ++
 src/primitives/relative_locktime.rs |  5 ++
 4 files changed, 148 insertions(+), 4 deletions(-)

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<Pk: MiniscriptKey> {
     /// Unsatisfiable.
     Unsatisfiable,
@@ -70,6 +70,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::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)) => 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::<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");
+    }
 }
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<Pk: MiniscriptKey> {
     /// Unsatisfiable.
     Unsatisfiable,
@@ -48,6 +48,49 @@ 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)) => 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<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 +730,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());
diff --git a/src/primitives/absolute_locktime.rs b/src/primitives/absolute_locktime.rs
index eaf177091..4d73078c0 100644
--- a/src/primitives/absolute_locktime.rs
+++ b/src/primitives/absolute_locktime.rs
@@ -68,6 +68,11 @@ impl AbsLockTime {
 
     /// Whether this is a time-based locktime.
     pub fn is_block_time(&self) -> bool { self.0.is_block_time() }
+
+    /// 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 From<AbsLockTime> for absolute::LockTime {
diff --git a/src/primitives/relative_locktime.rs b/src/primitives/relative_locktime.rs
index 6640963cf..cf814a601 100644
--- a/src/primitives/relative_locktime.rs
+++ b/src/primitives/relative_locktime.rs
@@ -65,6 +65,11 @@ 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())
+    }
 }
 
 impl convert::TryFrom<Sequence> for RelLockTime {

From 46b28127a77fc1efb0dbb6d992b683e921834633 Mon Sep 17 00:00:00 2001
From: Henry Romp <151henry151@gmail.com>
Date: Sat, 13 Jun 2026 11:41:54 -0400
Subject: [PATCH 4/6] Sort same-kind locktimes by consensus u32 in
 Terminal::cmp.

Compare After and Older nodes via cmp_by_consensus rather than locktime
Ord, and add a Miniscript ordering regression test.
---
 src/miniscript/display.rs |  8 ++++++--
 src/miniscript/mod.rs     | 11 ++++++++++-
 2 files changed, 16 insertions(+), 3 deletions(-)

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<Pk: MiniscriptKey, Ctx: ScriptContext> Ord for Terminal<Pk, Ctx> {
                         (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<bitcoin::PublicKey, Segwitv0>;
@@ -1979,6 +1980,14 @@ mod tests {
         );
     }
 
+    #[test]
+    fn terminal_ord_locktimes_are_consistent() {
+        let a = Miniscript::<String, Segwitv0>::after(AbsLockTime::from_consensus(9).unwrap());
+        let b = Miniscript::<String, Segwitv0>::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};

From 013ad7da27d44f397e4691c3f68f62886338fdfa Mon Sep 17 00:00:00 2001
From: Henry Romp <151henry151@gmail.com>
Date: Sat, 13 Jun 2026 11:42:03 -0400
Subject: [PATCH 5/6] Mark timelock kind conflicts as Witness::Impossible in
 concatenate_rev.

Mixed height/time merges now invalidate the whole satisfaction path
instead of leaving a valid stack with None timelock metadata. Add max
helpers on locktime types and update satisfier docs.
---
 src/miniscript/satisfy/mod.rs        | 89 ++++++++++++++++++----------
 src/miniscript/satisfy/sat_dissat.rs |  7 ---
 src/primitives/absolute_locktime.rs  | 10 ++++
 src/primitives/relative_locktime.rs  | 10 ++++
 4 files changed, 77 insertions(+), 39 deletions(-)

diff --git a/src/miniscript/satisfy/mod.rs b/src/miniscript/satisfy/mod.rs
index 31259d182..213fc25b2 100644
--- a/src/miniscript/satisfy/mod.rs
+++ b/src/miniscript/satisfy/mod.rs
@@ -93,18 +93,20 @@ pub trait Satisfier<Pk: MiniscriptKey + ToPublicKey> {
     /// Given a HASH160 hash, look up its preimage
     fn lookup_hash160(&self, _: &Pk::Hash160) -> Option<Preimage32> { 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 }
 }
 
@@ -674,18 +676,10 @@ mod tests {
         // Same descriptor as above, except that now we use a time-based timelock rather than a
         // lower height-based one.
         //
-        // Again, we wind up taking both timelock branches. Prior to #895, we would do this because
-        // we did not track the after(144) at all. After #895, we do it because our timelock logic
-        // handles mixed timelocks by silently clobbering one of them. (See the implementation of
-        // `concatenate_rev`, which calls `cmp::max` on the timelocks. This clobbers a locktime.
-        // See #979 for an explanation of this.)
-        //
-        // This time the "ideal" behavior would be that we track best satisfactions for both time-
-        // and height-based locktimes, and whenever we are forced into a conflict we throw one away.
-        // (The type system guarantees that we will always have at least one satisfaction left;
-        // otherwise the whole script would be flagged as mixing timelocks.)
-        //
-        // 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),and_v(v:pk({}),pk({}))),{expensive_threshold}),ajt:and_v(v:after(1000000000),v:pk({}))))",
             available_keys[0], unavailable_keys[0], available_keys[1],
@@ -693,10 +687,16 @@ mod tests {
         let descriptor =
             Descriptor::<crate::DefiniteDescriptorKey>::from_str(&descriptor_str).unwrap();
 
-        let plan = descriptor.into_plan_mall(&satisfier).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()),
         );
     }
 }
@@ -1029,6 +1029,13 @@ pub struct Satisfaction<T> {
 }
 
 impl<Pk: MiniscriptKey + ToPublicKey> Satisfaction<Placeholder<Pk>> {
+    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
@@ -1048,11 +1055,36 @@ impl<Pk: MiniscriptKey + ToPublicKey> Satisfaction<Placeholder<Pk>> {
     /// 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),
         }
     }
@@ -1120,14 +1152,7 @@ impl<Pk: MiniscriptKey + ToPublicKey> Satisfaction<Placeholder<Pk>> {
         // 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 2c5775af2..6bd8bd36f 100644
--- a/src/miniscript/satisfy/sat_dissat.rs
+++ b/src/miniscript/satisfy/sat_dissat.rs
@@ -21,13 +21,6 @@ pub(super) struct SatDissat<Pk: MiniscriptKey> {
 }
 
 impl<Pk: MiniscriptKey + ToPublicKey> Satisfaction<Placeholder<Pk>> {
-    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,
diff --git a/src/primitives/absolute_locktime.rs b/src/primitives/absolute_locktime.rs
index 4d73078c0..ce7c2725b 100644
--- a/src/primitives/absolute_locktime.rs
+++ b/src/primitives/absolute_locktime.rs
@@ -73,6 +73,16 @@ impl AbsLockTime {
     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<Self> {
+        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 From<AbsLockTime> for absolute::LockTime {
diff --git a/src/primitives/relative_locktime.rs b/src/primitives/relative_locktime.rs
index cf814a601..229cce887 100644
--- a/src/primitives/relative_locktime.rs
+++ b/src/primitives/relative_locktime.rs
@@ -70,6 +70,16 @@ impl RelLockTime {
     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<Self> {
+        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<Sequence> for RelLockTime {

From 476dac5ec4c9d93dc94ab2e3e4fc97e29b4223df Mon Sep 17 00:00:00 2001
From: Henry Romp <151henry151@gmail.com>
Date: Sat, 13 Jun 2026 11:42:05 -0400
Subject: [PATCH 6/6] Remove Ord and PartialOrd from AbsLockTime and
 RelLockTime.

Drop the consensus-u32 ordering that incorrectly compared height-based
and time-based locktimes against each other.
---
 src/primitives/absolute_locktime.rs | 14 +-------------
 src/primitives/relative_locktime.rs | 14 +-------------
 2 files changed, 2 insertions(+), 26 deletions(-)

diff --git a/src/primitives/absolute_locktime.rs b/src/primitives/absolute_locktime.rs
index ce7c2725b..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);
 
@@ -89,18 +89,6 @@ impl From<AbsLockTime> for absolute::LockTime {
     fn from(lock_time: AbsLockTime) -> Self { lock_time.0 }
 }
 
-impl cmp::PartialOrd for AbsLockTime {
-    fn partial_cmp(&self, other: &Self) -> Option<cmp::Ordering> { 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/relative_locktime.rs b/src/primitives/relative_locktime.rs
index 229cce887..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);
 
@@ -101,18 +101,6 @@ impl From<RelLockTime> 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<cmp::Ordering> { 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) }
 }