Merge #2146: chain 0.23.x: Fix ChainPosition ordering

aff800d fix(chain): correct unconfirmed `ChainPosition` `last_seen` tiebreaker (志宇)
9d2dedc fix(chain): correct ChainPosition ordering for wallet transaction display (志宇)

Pull request description:

  ## Description

  Back-ported from #2074
  Depends on #2148

  This PR fixes incorrect ordering of `ChainPosition` that affected how wallet transactions are displayed. Previously, unconfirmed transactions that were never seen in the mempool would incorrectly appear before transactions with mempool timestamps due to the derived `Ord` implementation treating `None` as less than `Some(_)`.

  ### Problem

  The derived `Ord` implementation for `ChainPosition::Unconfirmed` caused incorrect transaction ordering:
  - `Unconfirmed { first_seen: None, last_seen: None }` (never in mempool)
  - Would appear **before** `Unconfirmed { first_seen: Some(10), last_seen: Some(10) }` (seen in mempool)

  This resulted in a confusing wallet display where transactions never broadcast would appear before pending mempool transactions.

  ### Solution

  Implemented manual `Ord` and `PartialOrd` traits for `ChainPosition` with the correct ordering:

  1. **Confirmed transactions** (earliest first by block height)
  2. **Unconfirmed transactions seen in mempool** (ordered by `first_seen`)
  3. **Unconfirmed transactions never seen** (these come last)

  Additional ordering rules:
  - At the same confirmation height, transitive confirmations come before direct confirmations (as they may actually be confirmed earlier)
  - Tie-breaking uses transaction IDs for deterministic ordering

  ## Changelog notice

  ```md
  ### Fixed
  - Fixed `ChainPosition` ordering so unconfirmed transactions never seen in mempool appear last instead of first

  ### Changed
  - `ChainPosition`, `CanonicalTx`, and `FullTxOut` now require `A: Ord` for their `Ord` implementations
  - Simplified `FullTxOut` ordering to only use essential fields (chain_position, outpoint, spent_by)
  ```

  ## Checklists

  ### All Submissions:

  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)

  ### Bugfixes:

  * [ ] This pull request breaks the existing API
  * [x] I've added tests to reproduce the issue which are now passing
  * [ ] I'm linking the issue being fixed by this PR

Top commit has no ACKs.

Tree-SHA512: eccdf30a47c7e8ef40312f1dd4bf72ce42a3e26a1dd1e6d1f7dc4776350c99029408474439730256414bd0f173e755977a3e3deb81617a930e40ca7962bd490b

Author: evanlinjin

crates/chain/src/chain_data.rs

@@ -5,7 +5,7 @@ use crate::Anchor;
 /// Represents the observed position of some chain data.
 ///
 /// The generic `A` should be a [`Anchor`] implementation.
-#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, core::hash::Hash)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq, core::hash::Hash)]
 #[cfg_attr(
     feature = "serde",
     derive(serde::Deserialize, serde::Serialize),
@@ -85,8 +85,84 @@ impl<A: Anchor> ChainPosition<A> {
     }
 }
 
+/// Ordering for `ChainPosition`:
+///
+/// 1. Confirmed transactions come before unconfirmed
+/// 2. Confirmed transactions are ordered by anchor (lower height = earlier)
+/// 3. At equal anchor height, transitive confirmations come before direct
+/// 4. Unconfirmed transactions with mempool timestamps come before those without
+/// 5. Unconfirmed transactions are ordered by `first_seen`, then `last_seen`
+impl<A: Ord> Ord for ChainPosition<A> {
+    fn cmp(&self, other: &Self) -> core::cmp::Ordering {
+        use core::cmp::Ordering;
+
+        /// Compares options where `None` is greater than `Some` (sorts last).
+        fn cmp_none_last<T: Ord>(t1: &Option<T>, t2: &Option<T>) -> Ordering {
+            match (t1, t2) {
+                (None, None) => Ordering::Equal,
+                (None, Some(_)) => Ordering::Greater,
+                (Some(_), None) => Ordering::Less,
+                (Some(t1), Some(t2)) => t1.cmp(t2),
+            }
+        }
+
+        match (self, other) {
+            // Both confirmed: compare by anchor first
+            (
+                ChainPosition::Confirmed {
+                    anchor: a1,
+                    transitively: t1,
+                },
+                ChainPosition::Confirmed {
+                    anchor: a2,
+                    transitively: t2,
+                },
+            ) => {
+                // First compare anchors
+                match a1.cmp(a2) {
+                    // Same anchor: transitive before direct, tiebreak with txid
+                    Ordering::Equal => cmp_none_last(t1, t2),
+                    other => other,
+                }
+            }
+
+            // Both unconfirmed: special handling for None values
+            (
+                ChainPosition::Unconfirmed {
+                    first_seen: f1,
+                    last_seen: l1,
+                },
+                ChainPosition::Unconfirmed {
+                    first_seen: f2,
+                    last_seen: l2,
+                },
+            ) => {
+                // Never-in-mempool (None, None) ordered last
+                // Compare by first_seen, tie-break with last_seen
+                match cmp_none_last(f1, f2) {
+                    Ordering::Equal => cmp_none_last(l1, l2),
+                    other => other,
+                }
+            }
+
+            // Confirmed always comes before unconfirmed
+            (ChainPosition::Confirmed { .. }, ChainPosition::Unconfirmed { .. }) => Ordering::Less,
+            (ChainPosition::Unconfirmed { .. }, ChainPosition::Confirmed { .. }) => {
+                Ordering::Greater
+            }
+        }
+    }
+}
+
+/// Partial ordering for `ChainPosition` - delegates to `Ord` implementation.
+impl<A: Ord> PartialOrd for ChainPosition<A> {
+    fn partial_cmp(&self, other: &Self) -> Option<core::cmp::Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
 /// A `TxOut` with as much data as we can retrieve about it
-#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)]
+#[derive(Debug, Clone, PartialEq, Eq)]
 pub struct FullTxOut<A> {
     /// The position of the transaction in `outpoint` in the overall chain.
     pub chain_position: ChainPosition<A>,
@@ -100,6 +176,22 @@ pub struct FullTxOut<A> {
     pub is_on_coinbase: bool,
 }
 
+impl<A: Ord> Ord for FullTxOut<A> {
+    fn cmp(&self, other: &Self) -> core::cmp::Ordering {
+        self.chain_position
+            .cmp(&other.chain_position)
+            // Tie-break with `outpoint` and `spent_by`.
+            .then_with(|| self.outpoint.cmp(&other.outpoint))
+            .then_with(|| self.spent_by.cmp(&other.spent_by))
+    }
+}
+
+impl<A: Ord> PartialOrd for FullTxOut<A> {
+    fn partial_cmp(&self, other: &Self) -> Option<core::cmp::Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
 impl<A: Anchor> FullTxOut<A> {
     /// Whether the `txout` is considered mature.
     ///
@@ -167,22 +259,16 @@ impl<A: Anchor> FullTxOut<A> {
 #[cfg_attr(coverage_nightly, coverage(off))]
 mod test {
     use bdk_core::ConfirmationBlockTime;
+    use bitcoin::hashes::Hash;
 
     use crate::BlockId;
 
     use super::*;
 
     #[test]
     fn chain_position_ord() {
-        let unconf1 = ChainPosition::<ConfirmationBlockTime>::Unconfirmed {
-            last_seen: Some(10),
-            first_seen: Some(10),
-        };
-        let unconf2 = ChainPosition::<ConfirmationBlockTime>::Unconfirmed {
-            last_seen: Some(20),
-            first_seen: Some(20),
-        };
-        let conf1 = ChainPosition::Confirmed {
+        // Create test positions
+        let conf_deep = ChainPosition::Confirmed {
             anchor: ConfirmationBlockTime {
                 confirmation_time: 20,
                 block_id: BlockId {
@@ -192,7 +278,17 @@ mod test {
             },
             transitively: None,
         };
-        let conf2 = ChainPosition::Confirmed {
+        let conf_deep_transitive = ChainPosition::Confirmed {
+            anchor: ConfirmationBlockTime {
+                confirmation_time: 20,
+                block_id: BlockId {
+                    height: 9,
+                    ..Default::default()
+                },
+            },
+            transitively: Some(Txid::all_zeros()),
+        };
+        let conf_shallow = ChainPosition::Confirmed {
             anchor: ConfirmationBlockTime {
                 confirmation_time: 15,
                 block_id: BlockId {
@@ -202,12 +298,89 @@ mod test {
             },
             transitively: None,
         };
+        let unconf_seen_early = ChainPosition::<ConfirmationBlockTime>::Unconfirmed {
+            first_seen: Some(10),
+            last_seen: Some(10),
+        };
+        let unconf_seen_late = ChainPosition::<ConfirmationBlockTime>::Unconfirmed {
+            first_seen: Some(20),
+            last_seen: Some(20),
+        };
+        let unconf_seen_early_and_late = ChainPosition::<ConfirmationBlockTime>::Unconfirmed {
+            first_seen: Some(10),
+            last_seen: Some(20),
+        };
+        let unconf_never_seen = ChainPosition::<ConfirmationBlockTime>::Unconfirmed {
+            first_seen: None,
+            last_seen: None,
+        };
+
+        // Test ordering: confirmed < unconfirmed
+        assert!(
+            conf_deep < unconf_seen_early,
+            "confirmed comes before unconfirmed"
+        );
+        assert!(
+            conf_shallow < unconf_seen_early,
+            "confirmed comes before unconfirmed"
+        );
 
-        assert!(unconf2 > unconf1, "higher last_seen means higher ord");
-        assert!(unconf1 > conf1, "unconfirmed is higher ord than confirmed");
+        // Test ordering within confirmed: lower height (more confirmations) comes first
         assert!(
-            conf2 > conf1,
-            "confirmation_height is higher then it should be higher ord"
+            conf_deep < conf_shallow,
+            "deeper blocks (lower height) come first"
+        );
+
+        // Test ordering within confirmed at same height: transitive comes before direct
+        assert!(
+            conf_deep_transitive < conf_deep,
+            "transitive confirmation comes before direct at same height"
+        );
+
+        // Test ordering within unconfirmed: earlier first_seen comes first, tie_break with
+        // last_seen
+        assert!(
+            unconf_seen_early < unconf_seen_late,
+            "earlier first_seen comes first"
+        );
+        assert!(
+            unconf_seen_early < unconf_seen_early_and_late,
+            "if first_seen is equal, tiebreak with last_seen"
+        );
+
+        // Test ordering: never seen in mempool comes last
+        assert!(
+            unconf_seen_early < unconf_never_seen,
+            "seen in mempool comes before never seen"
+        );
+        assert!(
+            unconf_seen_late < unconf_never_seen,
+            "seen in mempool comes before never seen"
+        );
+
+        // Full ordering test: most confirmed -> least confirmed -> unconfirmed seen -> unconfirmed
+        // never seen
+        let mut positions = vec![
+            unconf_never_seen,
+            unconf_seen_late,
+            conf_shallow,
+            unconf_seen_early,
+            conf_deep,
+            conf_deep_transitive,
+            unconf_seen_early_and_late,
+        ];
+        positions.sort();
+        assert_eq!(
+            positions,
+            vec![
+                conf_deep_transitive,       // Most confirmed (potentially)
+                conf_deep,                  // Deep confirmation
+                conf_shallow,               // Shallow confirmation
+                unconf_seen_early,          // Unconfirmed, seen early
+                unconf_seen_early_and_late, // Unconfirmed, seen early and late
+                unconf_seen_late,           // Unconfirmed, seen late
+                unconf_never_seen,          // Never in mempool
+            ]
         );
     }
 

crates/chain/src/tx_graph.rs

@@ -247,14 +247,29 @@ impl Default for TxNodeInternal {
 }
 
 /// A transaction that is deemed to be part of the canonical history.
-#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
+#[derive(Clone, Debug, PartialEq, Eq)]
 pub struct CanonicalTx<'a, T, A> {
     /// How the transaction is observed in the canonical chain (confirmed or unconfirmed).
     pub chain_position: ChainPosition<A>,
     /// The transaction node (as part of the graph).
     pub tx_node: TxNode<'a, T, A>,
 }
 
+impl<'a, T: Ord, A: Ord> Ord for CanonicalTx<'a, T, A> {
+    fn cmp(&self, other: &Self) -> core::cmp::Ordering {
+        self.chain_position
+            .cmp(&other.chain_position)
+            // Txid tiebreaker for same position
+            .then_with(|| self.tx_node.txid.cmp(&other.tx_node.txid))
+    }
+}
+
+impl<'a, T: Ord, A: Ord> PartialOrd for CanonicalTx<'a, T, A> {
+    fn partial_cmp(&self, other: &Self) -> Option<core::cmp::Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
 impl<'a, T, A> From<CanonicalTx<'a, T, A>> for Txid {
     fn from(tx: CanonicalTx<'a, T, A>) -> Self {
         tx.tx_node.txid