From 9d2dedcd53a7ccde454c84f2c40327ef18603b51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Fri, 21 Nov 2025 09:34:29 +0000 Subject: [PATCH 1/2] fix(chain): correct ChainPosition ordering for wallet transaction display Previously, unconfirmed transactions never seen in mempool would appear before those with mempool timestamps due to derived Ord implementation. Changes: - Manual Ord impl: confirmed < unconfirmed < never-in-mempool - At same height: transitive confirmations < direct (potentially earlier) - Simplify FullTxOut ordering to only essential fields - Add comprehensive tests and documentation - Update CanonicalTx and FullTxOut to use manual Ord with A: Ord bound --- crates/chain/src/chain_data.rs | 196 ++++++++++++++++++++++++++++++--- crates/chain/src/tx_graph.rs | 17 ++- 2 files changed, 196 insertions(+), 17 deletions(-) diff --git a/crates/chain/src/chain_data.rs b/crates/chain/src/chain_data.rs index b641f15ec..9dbbfdd43 100644 --- a/crates/chain/src/chain_data.rs +++ b/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,86 @@ impl ChainPosition { } } +/// 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 Ord for ChainPosition { + fn cmp(&self, other: &Self) -> core::cmp::Ordering { + use core::cmp::Ordering; + + 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) { + Ordering::Equal => { + // Same anchor: transitive before direct (may be earlier) + match (t1, t2) { + (None, None) => Ordering::Equal, + (None, Some(_)) => Ordering::Greater, // Direct comes after transitive + (Some(_), None) => Ordering::Less, // Transitive comes before direct + // Both transitive: txid tiebreaker + (Some(tx1), Some(tx2)) => tx1.cmp(tx2), + } + } + 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 + match (f1.or(*l1), f2.or(*l2)) { + (None, None) => Ordering::Equal, + (None, Some(_)) => Ordering::Greater, // Never-seen after seen + (Some(_), None) => Ordering::Less, // Seen before never-seen + (Some(_), Some(_)) => { + // Both seen: compare first_seen, then last_seen + f1.cmp(f2).then_with(|| l1.cmp(l2)) + } + } + } + + // 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 PartialOrd for ChainPosition { + fn partial_cmp(&self, other: &Self) -> Option { + 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 { /// The position of the transaction in `outpoint` in the overall chain. pub chain_position: ChainPosition, @@ -100,6 +178,22 @@ pub struct FullTxOut { pub is_on_coinbase: bool, } +impl Ord for FullTxOut { + 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 PartialOrd for FullTxOut { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + impl FullTxOut { /// Whether the `txout` is considered mature. /// @@ -167,6 +261,7 @@ impl FullTxOut { #[cfg_attr(coverage_nightly, coverage(off))] mod test { use bdk_core::ConfirmationBlockTime; + use bitcoin::hashes::Hash; use crate::BlockId; @@ -174,15 +269,8 @@ mod test { #[test] fn chain_position_ord() { - let unconf1 = ChainPosition::::Unconfirmed { - last_seen: Some(10), - first_seen: Some(10), - }; - let unconf2 = ChainPosition::::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 +280,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 +300,78 @@ mod test { }, transitively: None, }; + let unconf_seen_early = ChainPosition::::Unconfirmed { + first_seen: Some(10), + last_seen: Some(10), + }; + let unconf_seen_late = ChainPosition::::Unconfirmed { + first_seen: Some(20), + last_seen: Some(20), + }; + let unconf_never_seen = ChainPosition::::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 + assert!( + unconf_seen_early < unconf_seen_late, + "earlier first_seen comes first" + ); + + // 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, + ]; + 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_late, // Unconfirmed, seen late + unconf_never_seen, // Never in mempool + ] ); } diff --git a/crates/chain/src/tx_graph.rs b/crates/chain/src/tx_graph.rs index 546c4b593..65cc8c59b 100644 --- a/crates/chain/src/tx_graph.rs +++ b/crates/chain/src/tx_graph.rs @@ -247,7 +247,7 @@ 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, @@ -255,6 +255,21 @@ pub struct CanonicalTx<'a, T, A> { 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 { + Some(self.cmp(other)) + } +} + impl<'a, T, A> From> for Txid { fn from(tx: CanonicalTx<'a, T, A>) -> Self { tx.tx_node.txid From aff800d86c760f643bffb9731e46d7f22198a0f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Mon, 5 Jan 2026 01:53:40 +0000 Subject: [PATCH 2/2] fix(chain): correct unconfirmed `ChainPosition` `last_seen` tiebreaker --- crates/chain/src/chain_data.rs | 59 ++++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 25 deletions(-) diff --git a/crates/chain/src/chain_data.rs b/crates/chain/src/chain_data.rs index 9dbbfdd43..43d41e2ed 100644 --- a/crates/chain/src/chain_data.rs +++ b/crates/chain/src/chain_data.rs @@ -96,6 +96,16 @@ impl Ord for ChainPosition { 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(t1: &Option, t2: &Option) -> 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 ( @@ -110,16 +120,8 @@ impl Ord for ChainPosition { ) => { // First compare anchors match a1.cmp(a2) { - Ordering::Equal => { - // Same anchor: transitive before direct (may be earlier) - match (t1, t2) { - (None, None) => Ordering::Equal, - (None, Some(_)) => Ordering::Greater, // Direct comes after transitive - (Some(_), None) => Ordering::Less, // Transitive comes before direct - // Both transitive: txid tiebreaker - (Some(tx1), Some(tx2)) => tx1.cmp(tx2), - } - } + // Same anchor: transitive before direct, tiebreak with txid + Ordering::Equal => cmp_none_last(t1, t2), other => other, } } @@ -136,14 +138,10 @@ impl Ord for ChainPosition { }, ) => { // Never-in-mempool (None, None) ordered last - match (f1.or(*l1), f2.or(*l2)) { - (None, None) => Ordering::Equal, - (None, Some(_)) => Ordering::Greater, // Never-seen after seen - (Some(_), None) => Ordering::Less, // Seen before never-seen - (Some(_), Some(_)) => { - // Both seen: compare first_seen, then last_seen - f1.cmp(f2).then_with(|| l1.cmp(l2)) - } + // Compare by first_seen, tie-break with last_seen + match cmp_none_last(f1, f2) { + Ordering::Equal => cmp_none_last(l1, l2), + other => other, } } @@ -308,6 +306,10 @@ mod test { first_seen: Some(20), last_seen: Some(20), }; + let unconf_seen_early_and_late = ChainPosition::::Unconfirmed { + first_seen: Some(10), + last_seen: Some(20), + }; let unconf_never_seen = ChainPosition::::Unconfirmed { first_seen: None, last_seen: None, @@ -335,11 +337,16 @@ mod test { "transitive confirmation comes before direct at same height" ); - // Test ordering within unconfirmed: earlier first_seen comes first + // 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!( @@ -360,17 +367,19 @@ mod test { 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_late, // Unconfirmed, seen late - unconf_never_seen, // Never in mempool + 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 ] ); }