Merge #1416: [chain] Change tx_last_seen to Option<u64>

af75817 ref(tx_graph): Change last_seen to `HashMap<Txid, u64>` (valued mammal)
6204d2c feat(tx_graph): Add method `txs_with_no_anchor_or_last_seen` (valued mammal)
496601b test(tx_graph): Add test for `list_canonical_txs` (valued mammal)
c405729 wallet: delete method `insert_anchor` (valued mammal)
b34790c ref(tx_graph)!: Rename `list_chain_txs` to `list_canonical_txs` (valued mammal)
2ce4bb4 test(indexed_tx_graph): Add test_get_chain_position (valued mammal)
36f5887 test(wallet): Add test_insert_tx_balance_and_utxos (valued mammal)
bbc19c3 fix(tx_graph)!: Change tx_last_seen to `Option<u64>` (valued mammal)
324eeb3 fix(wallet)!: Rework `Wallet::insert_tx` to no longer insert anchors (valued mammal)

Pull request description:

  The PR changes the type of last_seen to `Option<u64>` for `txs` member of `TxGraph`.

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

  fixes #1446
  fixes #1396

  ### Notes to the reviewers

  ### Changelog notice

  Changed
  - Member `last_seen_unconfirmed` of `TxNode` is changed to `Option<u64>`
  - Renamed `TxGraph` method `list_chain_txs` to `list_canonical_txs`
  - Changed `Wallet::insert_tx` to take a single `tx: Transaction` as parameter

  Added
  - Add method `txs_with_no_anchor_or_last_seen` for `TxGraph`
  - Add method `unbroadcast_transactions` for `Wallet`

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

  #### Bugfixes:

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

ACKs for top commit:
  notmandatory:
    Re ACK af75817

Tree-SHA512: e664b3b49e2f547873923f15dffbbc7fa032b6240e5b856b180e9e26123ca141864d10448912dc4a31bbb200c75bef4251a910a4330dac17ee6841b564612d13

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

@@ -109,10 +109,11 @@ use core::{
 /// [module-level documentation]: crate::tx_graph
 #[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)>,
+    // all transactions that the graph is aware of in format: `(tx_node, tx_anchors)`
+    txs: HashMap<Txid, (TxNodeInternal, BTreeSet<A>)>,
     spends: BTreeMap<OutPoint, HashSet<Txid>>,
     anchors: BTreeSet<(A, Txid)>,
+    last_seen: HashMap<Txid, u64>,
 
     // This atrocity exists so that `TxGraph::outspends()` can return a reference.
     // FIXME: This can be removed once `HashSet::new` is a const fn.
@@ -125,6 +126,7 @@ impl<A> Default for TxGraph<A> {
             txs: Default::default(),
             spends: Default::default(),
             anchors: Default::default(),
+            last_seen: Default::default(),
             empty_outspends: Default::default(),
         }
     }
@@ -140,7 +142,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> {
@@ -210,7 +212,7 @@ impl<A> TxGraph<A> {
     ///
     /// This includes txouts of both full transactions as well as floating transactions.
     pub fn all_txouts(&self) -> impl Iterator<Item = (OutPoint, &TxOut)> {
-        self.txs.iter().flat_map(|(txid, (tx, _, _))| match tx {
+        self.txs.iter().flat_map(|(txid, (tx, _))| match tx {
             TxNodeInternal::Whole(tx) => tx
                 .as_ref()
                 .output
@@ -232,7 +234,7 @@ impl<A> TxGraph<A> {
     pub fn floating_txouts(&self) -> impl Iterator<Item = (OutPoint, &TxOut)> {
         self.txs
             .iter()
-            .filter_map(|(txid, (tx_node, _, _))| match tx_node {
+            .filter_map(|(txid, (tx_node, _))| match tx_node {
                 TxNodeInternal::Whole(_) => None,
                 TxNodeInternal::Partial(txouts) => Some(
                     txouts
@@ -247,17 +249,30 @@ impl<A> TxGraph<A> {
     pub fn full_txs(&self) -> impl Iterator<Item = TxNode<'_, Arc<Transaction>, A>> {
         self.txs
             .iter()
-            .filter_map(|(&txid, (tx, anchors, last_seen))| match tx {
+            .filter_map(|(&txid, (tx, anchors))| match tx {
                 TxNodeInternal::Whole(tx) => Some(TxNode {
                     txid,
                     tx: tx.clone(),
                     anchors,
-                    last_seen_unconfirmed: *last_seen,
+                    last_seen_unconfirmed: self.last_seen.get(&txid).copied(),
                 }),
                 TxNodeInternal::Partial(_) => None,
             })
     }
 
+    /// Iterate over graph transactions with no anchors or last-seen.
+    pub fn txs_with_no_anchor_or_last_seen(
+        &self,
+    ) -> impl Iterator<Item = TxNode<'_, Arc<Transaction>, A>> {
+        self.full_txs().filter_map(|tx| {
+            if tx.anchors.is_empty() && tx.last_seen_unconfirmed.is_none() {
+                Some(tx)
+            } else {
+                None
+            }
+        })
+    }
+
     /// Get a transaction by txid. This only returns `Some` for full transactions.
     ///
     /// Refer to [`get_txout`] for getting a specific [`TxOut`].
@@ -270,11 +285,11 @@ impl<A> TxGraph<A> {
     /// Get a transaction node by txid. This only returns `Some` for full transactions.
     pub fn get_tx_node(&self, txid: Txid) -> Option<TxNode<'_, Arc<Transaction>, A>> {
         match &self.txs.get(&txid)? {
-            (TxNodeInternal::Whole(tx), anchors, last_seen) => Some(TxNode {
+            (TxNodeInternal::Whole(tx), anchors) => Some(TxNode {
                 txid,
                 tx: tx.clone(),
                 anchors,
-                last_seen_unconfirmed: *last_seen,
+                last_seen_unconfirmed: self.last_seen.get(&txid).copied(),
             }),
             _ => None,
         }
@@ -504,7 +519,6 @@ impl<A: Clone + Ord> TxGraph<A> {
             (
                 TxNodeInternal::Partial([(outpoint.vout, txout)].into()),
                 BTreeSet::new(),
-                0,
             ),
         );
         self.apply_update(update)
@@ -518,7 +532,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()),
         );
         self.apply_update(update)
     }
@@ -559,8 +573,7 @@ impl<A: Clone + Ord> TxGraph<A> {
     /// [`update_last_seen_unconfirmed`]: Self::update_last_seen_unconfirmed
     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.insert(txid, seen_at);
         self.apply_update(update)
     }
 
@@ -607,7 +620,7 @@ impl<A: Clone + Ord> TxGraph<A> {
             .txs
             .iter()
             .filter_map(
-                |(&txid, (_, anchors, _))| {
+                |(&txid, (_, anchors))| {
                     if anchors.is_empty() {
                         Some(txid)
                     } else {
@@ -656,21 +669,19 @@ impl<A: Clone + Ord> TxGraph<A> {
                 });
 
             match self.txs.get_mut(&txid) {
-                Some((tx_node @ TxNodeInternal::Partial(_), _, _)) => {
+                Some((tx_node @ TxNodeInternal::Partial(_), _)) => {
                     *tx_node = TxNodeInternal::Whole(wrapped_tx.clone());
                 }
-                Some((TxNodeInternal::Whole(tx), _, _)) => {
+                Some((TxNodeInternal::Whole(tx), _)) => {
                     debug_assert_eq!(
                         tx.as_ref().compute_txid(),
                         txid,
                         "tx should produce txid that is same as key"
                     );
                 }
                 None => {
-                    self.txs.insert(
-                        txid,
-                        (TxNodeInternal::Whole(wrapped_tx), BTreeSet::new(), 0),
-                    );
+                    self.txs
+                        .insert(txid, (TxNodeInternal::Whole(wrapped_tx), BTreeSet::new()));
                 }
             }
         }
@@ -679,23 +690,22 @@ impl<A: Clone + Ord> TxGraph<A> {
             let tx_entry = self.txs.entry(outpoint.txid).or_default();
 
             match tx_entry {
-                (TxNodeInternal::Whole(_), _, _) => { /* do nothing since we already have full tx */
-                }
-                (TxNodeInternal::Partial(txouts), _, _) => {
+                (TxNodeInternal::Whole(_), _) => { /* do nothing since we already have full tx */ }
+                (TxNodeInternal::Partial(txouts), _) => {
                     txouts.insert(outpoint.vout, txout);
                 }
             }
         }
 
         for (anchor, txid) in changeset.anchors {
             if self.anchors.insert((anchor.clone(), txid)) {
-                let (_, anchors, _) = self.txs.entry(txid).or_default();
+                let (_, anchors) = self.txs.entry(txid).or_default();
                 anchors.insert(anchor);
             }
         }
 
         for (txid, new_last_seen) in changeset.last_seen {
-            let (_, _, last_seen) = self.txs.entry(txid).or_default();
+            let last_seen = self.last_seen.entry(txid).or_default();
             if new_last_seen > *last_seen {
                 *last_seen = new_last_seen;
             }
@@ -709,30 +719,24 @@ impl<A: Clone + Ord> TxGraph<A> {
     pub(crate) fn determine_changeset(&self, update: TxGraph<A>) -> ChangeSet<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) {
+        for (&txid, (update_tx_node, _)) in &update.txs {
+            match (self.txs.get(&txid), update_tx_node) {
                 (None, TxNodeInternal::Whole(update_tx)) => {
                     changeset.txs.insert(update_tx.clone());
-                    0
                 }
                 (None, TxNodeInternal::Partial(update_txos)) => {
                     changeset.txouts.extend(
                         update_txos
                             .iter()
                             .map(|(&vout, txo)| (OutPoint::new(txid, vout), txo.clone())),
                     );
-                    0
                 }
-                (Some((TxNodeInternal::Whole(_), _, last_seen)), _) => *last_seen,
-                (
-                    Some((TxNodeInternal::Partial(_), _, last_seen)),
-                    TxNodeInternal::Whole(update_tx),
-                ) => {
+                (Some((TxNodeInternal::Whole(_), _)), _) => {}
+                (Some((TxNodeInternal::Partial(_), _)), TxNodeInternal::Whole(update_tx)) => {
                     changeset.txs.insert(update_tx.clone());
-                    *last_seen
                 }
                 (
-                    Some((TxNodeInternal::Partial(txos), _, last_seen)),
+                    Some((TxNodeInternal::Partial(txos), _)),
                     TxNodeInternal::Partial(update_txos),
                 ) => {
                     changeset.txouts.extend(
@@ -741,12 +745,14 @@ impl<A: Clone + Ord> TxGraph<A> {
                             .filter(|(vout, _)| !txos.contains_key(*vout))
                             .map(|(&vout, txo)| (OutPoint::new(txid, vout), txo.clone())),
                     );
-                    *last_seen
                 }
-            };
+            }
+        }
 
-            if *update_last_seen > prev_last_seen {
-                changeset.last_seen.insert(txid, *update_last_seen);
+        for (txid, update_last_seen) in update.last_seen {
+            let prev_last_seen = self.last_seen.get(&txid).copied();
+            if Some(update_last_seen) > prev_last_seen {
+                changeset.last_seen.insert(txid, update_last_seen);
             }
         }
 
@@ -786,7 +792,7 @@ impl<A: Anchor> TxGraph<A> {
         chain_tip: BlockId,
         txid: Txid,
     ) -> Result<Option<ChainPosition<&A>>, C::Error> {
-        let (tx_node, anchors, last_seen) = match self.txs.get(&txid) {
+        let (tx_node, anchors) = match self.txs.get(&txid) {
             Some(v) => v,
             None => return Ok(None),
         };
@@ -798,6 +804,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 self.last_seen.get(&txid) {
+            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 +897,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 +906,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`.
@@ -971,10 +984,10 @@ impl<A: Anchor> TxGraph<A> {
     /// If the [`ChainOracle`] implementation (`chain`) fails, an error will be returned with the
     /// returned item.
     ///
-    /// If the [`ChainOracle`] is infallible, [`list_chain_txs`] can be used instead.
+    /// If the [`ChainOracle`] is infallible, [`list_canonical_txs`] can be used instead.
     ///
-    /// [`list_chain_txs`]: Self::list_chain_txs
-    pub fn try_list_chain_txs<'a, C: ChainOracle + 'a>(
+    /// [`list_canonical_txs`]: Self::list_canonical_txs
+    pub fn try_list_canonical_txs<'a, C: ChainOracle + 'a>(
         &'a self,
         chain: &'a C,
         chain_tip: BlockId,
@@ -993,15 +1006,15 @@ impl<A: Anchor> TxGraph<A> {
 
     /// List graph transactions that are in `chain` with `chain_tip`.
     ///
-    /// This is the infallible version of [`try_list_chain_txs`].
+    /// This is the infallible version of [`try_list_canonical_txs`].
     ///
-    /// [`try_list_chain_txs`]: Self::try_list_chain_txs
-    pub fn list_chain_txs<'a, C: ChainOracle + 'a>(
+    /// [`try_list_canonical_txs`]: Self::try_list_canonical_txs
+    pub fn list_canonical_txs<'a, C: ChainOracle + 'a>(
         &'a self,
         chain: &'a C,
         chain_tip: BlockId,
     ) -> impl Iterator<Item = CanonicalTx<'a, Arc<Transaction>, A>> {
-        self.try_list_chain_txs(chain, chain_tip)
+        self.try_list_canonical_txs(chain, chain_tip)
             .map(|r| r.expect("oracle is infallible"))
     }
 

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)
 }