fix(tx_graph)!: Change tx_last_seen to Option<u64>

Also fixup `test_list_owned_txouts` to check that the right
outputs, utxos, and balance are returned at different local
chain heights.

This fixes an issue where unbroadcast and otherwise non-canonical
transactions were returned from methods `list_chain_txs` and
`Wallet::transactions` because every tx inserted had a last_seen
of 0 making it appear unconfirmed.

Note this commit changes the way `Balance` is represented due to
new logic in `try_get_chain_position` that no longer considers
txs with non-canonical anchors. Before this change, a tx anchored
to a block that is reorged out had a permanent effect on the
pending balance, and now only txs with a last_seen time or an
anchor confirmed in the best chain will return a `ChainPosition`.

crates/bitcoind_rpc/tests/test_emitter.rs

@@ -392,7 +392,6 @@ fn tx_can_become_unconfirmed_after_reorg() -> anyhow::Result<()> {
             get_balance(&recv_chain, &recv_graph)?,
             Balance {
                 confirmed: SEND_AMOUNT * (ADDITIONAL_COUNT - reorg_count) as u64,
-                trusted_pending: SEND_AMOUNT * reorg_count as u64,
                 ..Balance::default()
             },
             "reorg_count: {}",

crates/chain/src/tx_graph.rs

@@ -110,7 +110,7 @@ use core::{
 #[derive(Clone, Debug, PartialEq)]
 pub struct TxGraph<A = ()> {
     // all transactions that the graph is aware of in format: `(tx_node, tx_anchors, tx_last_seen)`
-    txs: HashMap<Txid, (TxNodeInternal, BTreeSet<A>, u64)>,
+    txs: HashMap<Txid, (TxNodeInternal, BTreeSet<A>, Option<u64>)>,
     spends: BTreeMap<OutPoint, HashSet<Txid>>,
     anchors: BTreeSet<(A, Txid)>,
 
@@ -140,7 +140,7 @@ pub struct TxNode<'a, T, A> {
     /// The blocks that the transaction is "anchored" in.
     pub anchors: &'a BTreeSet<A>,
     /// The last-seen unix timestamp of the transaction as unconfirmed.
-    pub last_seen_unconfirmed: u64,
+    pub last_seen_unconfirmed: Option<u64>,
 }
 
 impl<'a, T, A> Deref for TxNode<'a, T, A> {
@@ -504,7 +504,7 @@ impl<A: Clone + Ord> TxGraph<A> {
             (
                 TxNodeInternal::Partial([(outpoint.vout, txout)].into()),
                 BTreeSet::new(),
-                0,
+                None,
             ),
         );
         self.apply_update(update)
@@ -518,7 +518,7 @@ impl<A: Clone + Ord> TxGraph<A> {
         let mut update = Self::default();
         update.txs.insert(
             tx.compute_txid(),
-            (TxNodeInternal::Whole(tx), BTreeSet::new(), 0),
+            (TxNodeInternal::Whole(tx), BTreeSet::new(), None),
         );
         self.apply_update(update)
     }
@@ -560,7 +560,7 @@ impl<A: Clone + Ord> TxGraph<A> {
     pub fn insert_seen_at(&mut self, txid: Txid, seen_at: u64) -> ChangeSet<A> {
         let mut update = Self::default();
         let (_, _, update_last_seen) = update.txs.entry(txid).or_default();
-        *update_last_seen = seen_at;
+        *update_last_seen = Some(seen_at);
         self.apply_update(update)
     }
 
@@ -669,7 +669,7 @@ impl<A: Clone + Ord> TxGraph<A> {
                 None => {
                     self.txs.insert(
                         txid,
-                        (TxNodeInternal::Whole(wrapped_tx), BTreeSet::new(), 0),
+                        (TxNodeInternal::Whole(wrapped_tx), BTreeSet::new(), None),
                     );
                 }
             }
@@ -696,8 +696,8 @@ impl<A: Clone + Ord> TxGraph<A> {
 
         for (txid, new_last_seen) in changeset.last_seen {
             let (_, _, last_seen) = self.txs.entry(txid).or_default();
-            if new_last_seen > *last_seen {
-                *last_seen = new_last_seen;
+            if Some(new_last_seen) > *last_seen {
+                *last_seen = Some(new_last_seen);
             }
         }
     }
@@ -710,18 +710,18 @@ impl<A: Clone + Ord> TxGraph<A> {
         let mut changeset = ChangeSet::<A>::default();
 
         for (&txid, (update_tx_node, _, update_last_seen)) in &update.txs {
-            let prev_last_seen: u64 = match (self.txs.get(&txid), update_tx_node) {
+            let prev_last_seen = match (self.txs.get(&txid), update_tx_node) {
                 (None, TxNodeInternal::Whole(update_tx)) => {
                     changeset.txs.insert(update_tx.clone());
-                    0
+                    None
                 }
                 (None, TxNodeInternal::Partial(update_txos)) => {
                     changeset.txouts.extend(
                         update_txos
                             .iter()
                             .map(|(&vout, txo)| (OutPoint::new(txid, vout), txo.clone())),
                     );
-                    0
+                    None
                 }
                 (Some((TxNodeInternal::Whole(_), _, last_seen)), _) => *last_seen,
                 (
@@ -746,7 +746,10 @@ impl<A: Clone + Ord> TxGraph<A> {
             };
 
             if *update_last_seen > prev_last_seen {
-                changeset.last_seen.insert(txid, *update_last_seen);
+                changeset.last_seen.insert(
+                    txid,
+                    update_last_seen.expect("checked is greater, so we must have a last_seen"),
+                );
             }
         }
 
@@ -798,6 +801,13 @@ impl<A: Anchor> TxGraph<A> {
             }
         }
 
+        // If no anchors are in best chain and we don't have a last_seen, we can return
+        // early because by definition the tx doesn't have a chain position.
+        let last_seen = match last_seen {
+            Some(t) => *t,
+            None => return Ok(None),
+        };
+
         // The tx is not anchored to a block in the best chain, which means that it
         // might be in mempool, or it might have been dropped already.
         // Let's check conflicts to find out!
@@ -884,7 +894,7 @@ impl<A: Anchor> TxGraph<A> {
                 if conflicting_tx.last_seen_unconfirmed > tx_last_seen {
                     return Ok(None);
                 }
-                if conflicting_tx.last_seen_unconfirmed == *last_seen
+                if conflicting_tx.last_seen_unconfirmed == Some(last_seen)
                     && conflicting_tx.as_ref().compute_txid() > tx.as_ref().compute_txid()
                 {
                     // Conflicting tx has priority if txid of conflicting tx > txid of original tx
@@ -893,7 +903,7 @@ impl<A: Anchor> TxGraph<A> {
             }
         }
 
-        Ok(Some(ChainPosition::Unconfirmed(*last_seen)))
+        Ok(Some(ChainPosition::Unconfirmed(last_seen)))
     }
 
     /// Get the position of the transaction in `chain` with tip `chain_tip`.

crates/chain/tests/common/tx_template.rs

@@ -131,9 +131,7 @@ pub fn init_graph<'a, A: Anchor + Clone + 'a>(
         for anchor in tx_tmp.anchors.iter() {
             let _ = graph.insert_anchor(tx.compute_txid(), anchor.clone());
         }
-        if let Some(seen_at) = tx_tmp.last_seen {
-            let _ = graph.insert_seen_at(tx.compute_txid(), seen_at);
-        }
+        let _ = graph.insert_seen_at(tx.compute_txid(), tx_tmp.last_seen.unwrap_or(0));
     }
     (graph, spk_index, tx_ids)
 }

crates/chain/tests/test_indexed_tx_graph.rs

@@ -116,8 +116,8 @@ fn insert_relevant_txs() {
 /// tx1: A Coinbase, sending 70000 sats to "trusted" address. [Block 0]
 /// tx2: A external Receive, sending 30000 sats to "untrusted" address. [Block 1]
 /// tx3: Internal Spend. Spends tx2 and returns change of 10000 to "trusted" address. [Block 2]
-/// tx4: Mempool tx, sending 20000 sats to "trusted" address.
-/// tx5: Mempool tx, sending 15000 sats to "untested" address.
+/// tx4: Mempool tx, sending 20000 sats to "untrusted" address.
+/// tx5: Mempool tx, sending 15000 sats to "trusted" address.
 /// tx6: Complete unrelated tx. [Block 3]
 ///
 /// Different transactions are added via `insert_relevant_txs`.
@@ -160,7 +160,7 @@ fn test_list_owned_txouts() {
     let mut untrusted_spks: Vec<ScriptBuf> = Vec::new();
 
     {
-        // we need to scope here to take immutanble reference of the graph
+        // we need to scope here to take immutable reference of the graph
         for _ in 0..10 {
             let ((_, script), _) = graph
                 .index
@@ -226,7 +226,7 @@ fn test_list_owned_txouts() {
         ..common::new_tx(0)
     };
 
-    // tx5 is spending tx3 and receiving change at trusted keychain, unconfirmed.
+    // tx5 is an external transaction receiving at trusted keychain, unconfirmed.
     let tx5 = Transaction {
         output: vec![TxOut {
             value: Amount::from_sat(15000),
@@ -239,7 +239,7 @@ fn test_list_owned_txouts() {
     let tx6 = common::new_tx(0);
 
     // Insert transactions into graph with respective anchors
-    // For unconfirmed txs we pass in `None`.
+    // Insert unconfirmed txs with a last_seen timestamp
 
     let _ =
         graph.batch_insert_relevant([&tx1, &tx2, &tx3, &tx6].iter().enumerate().map(|(i, tx)| {
@@ -291,9 +291,6 @@ fn test_list_owned_txouts() {
                 |_, spk: &Script| trusted_spks.contains(&spk.to_owned()),
             );
 
-            assert_eq!(txouts.len(), 5);
-            assert_eq!(utxos.len(), 4);
-
             let confirmed_txouts_txid = txouts
                 .iter()
                 .filter_map(|(_, full_txout)| {
@@ -359,29 +356,25 @@ fn test_list_owned_txouts() {
             balance,
         ) = fetch(0, &graph);
 
+        // tx1 is a confirmed txout and is unspent
+        // tx4, tx5 are unconfirmed
         assert_eq!(confirmed_txouts_txid, [tx1.compute_txid()].into());
         assert_eq!(
             unconfirmed_txouts_txid,
-            [
-                tx2.compute_txid(),
-                tx3.compute_txid(),
-                tx4.compute_txid(),
-                tx5.compute_txid()
-            ]
-            .into()
+            [tx4.compute_txid(), tx5.compute_txid()].into()
         );
 
         assert_eq!(confirmed_utxos_txid, [tx1.compute_txid()].into());
         assert_eq!(
             unconfirmed_utxos_txid,
-            [tx3.compute_txid(), tx4.compute_txid(), tx5.compute_txid()].into()
+            [tx4.compute_txid(), tx5.compute_txid()].into()
         );
 
         assert_eq!(
             balance,
             Balance {
                 immature: Amount::from_sat(70000),          // immature coinbase
-                trusted_pending: Amount::from_sat(25000),   // tx3 + tx5
+                trusted_pending: Amount::from_sat(15000),   // tx5
                 untrusted_pending: Amount::from_sat(20000), // tx4
                 confirmed: Amount::ZERO                     // Nothing is confirmed yet
             }
@@ -405,23 +398,26 @@ fn test_list_owned_txouts() {
         );
         assert_eq!(
             unconfirmed_txouts_txid,
-            [tx3.compute_txid(), tx4.compute_txid(), tx5.compute_txid()].into()
+            [tx4.compute_txid(), tx5.compute_txid()].into()
         );
 
-        // tx2 doesn't get into confirmed utxos set
-        assert_eq!(confirmed_utxos_txid, [tx1.compute_txid()].into());
+        // tx2 gets into confirmed utxos set
+        assert_eq!(
+            confirmed_utxos_txid,
+            [tx1.compute_txid(), tx2.compute_txid()].into()
+        );
         assert_eq!(
             unconfirmed_utxos_txid,
-            [tx3.compute_txid(), tx4.compute_txid(), tx5.compute_txid()].into()
+            [tx4.compute_txid(), tx5.compute_txid()].into()
         );
 
         assert_eq!(
             balance,
             Balance {
                 immature: Amount::from_sat(70000),          // immature coinbase
-                trusted_pending: Amount::from_sat(25000),   // tx3 + tx5
+                trusted_pending: Amount::from_sat(15000),   // tx5
                 untrusted_pending: Amount::from_sat(20000), // tx4
-                confirmed: Amount::ZERO                     // Nothing is confirmed yet
+                confirmed: Amount::from_sat(30_000)         // tx2 got confirmed
             }
         );
     }
@@ -477,6 +473,7 @@ fn test_list_owned_txouts() {
             balance,
         ) = fetch(98, &graph);
 
+        // no change compared to block 2
         assert_eq!(
             confirmed_txouts_txid,
             [tx1.compute_txid(), tx2.compute_txid(), tx3.compute_txid()].into()
@@ -502,14 +499,14 @@ fn test_list_owned_txouts() {
                 immature: Amount::from_sat(70000),          // immature coinbase
                 trusted_pending: Amount::from_sat(15000),   // tx5
                 untrusted_pending: Amount::from_sat(20000), // tx4
-                confirmed: Amount::from_sat(10000)          // tx1 got matured
+                confirmed: Amount::from_sat(10000)          // tx3 is confirmed
             }
         );
     }
 
     // AT Block 99
     {
-        let (_, _, _, _, balance) = fetch(100, &graph);
+        let (_, _, _, _, balance) = fetch(99, &graph);
 
         // Coinbase maturity hits
         assert_eq!(

crates/chain/tests/test_tx_graph.rs

@@ -977,16 +977,6 @@ fn test_chain_spends() {
         }))
     );
 
-    // Even if unconfirmed tx has a last_seen of 0, it can still be part of a chain spend.
-    assert_eq!(
-        graph.get_chain_spend(
-            &local_chain,
-            tip.block_id(),
-            OutPoint::new(tx_0.compute_txid(), 1)
-        ),
-        Some((ChainPosition::Unconfirmed(0), tx_2.compute_txid())),
-    );
-
     // Mark the unconfirmed as seen and check correct ObservedAs status is returned.
     let _ = graph.insert_seen_at(tx_2.compute_txid(), 1234567);
 
@@ -1099,10 +1089,10 @@ fn update_last_seen_unconfirmed() {
     let txid = tx.compute_txid();
 
     // insert a new tx
-    // initially we have a last_seen of 0, and no anchors
+    // initially we have a last_seen of None and no anchors
     let _ = graph.insert_tx(tx);
     let tx = graph.full_txs().next().unwrap();
-    assert_eq!(tx.last_seen_unconfirmed, 0);
+    assert_eq!(tx.last_seen_unconfirmed, None);
     assert!(tx.anchors.is_empty());
 
     // higher timestamp should update last seen
@@ -1117,7 +1107,15 @@ fn update_last_seen_unconfirmed() {
     let _ = graph.insert_anchor(txid, ());
     let changeset = graph.update_last_seen_unconfirmed(4);
     assert!(changeset.is_empty());
-    assert_eq!(graph.full_txs().next().unwrap().last_seen_unconfirmed, 2);
+    assert_eq!(
+        graph
+            .full_txs()
+            .next()
+            .unwrap()
+            .last_seen_unconfirmed
+            .unwrap(),
+        2
+    );
 }
 
 #[test]

crates/electrum/tests/test_electrum.rs

@@ -198,17 +198,14 @@ fn tx_can_become_unconfirmed_after_reorg() -> anyhow::Result<()> {
             .apply_update(update.chain_update)
             .map_err(|err| anyhow::anyhow!("LocalChain update error: {:?}", err))?;
 
-        // Check to see if a new anchor is added during current reorg.
-        if !initial_anchors.is_superset(update.graph_update.all_anchors()) {
-            println!("New anchor added at reorg depth {}", depth);
-        }
+        // Check that no new anchors are added during current reorg.
+        assert!(initial_anchors.is_superset(update.graph_update.all_anchors()));
         let _ = recv_graph.apply_update(update.graph_update);
 
         assert_eq!(
             get_balance(&recv_chain, &recv_graph)?,
             Balance {
                 confirmed: SEND_AMOUNT * (REORG_COUNT - depth) as u64,
-                trusted_pending: SEND_AMOUNT * depth as u64,
                 ..Balance::default()
             },
             "reorg_count: {}",