Merge #1779: transactions method should only return relevant transa…

…ctions

75fae3e test(wallet): verify Wallet::transactions method only returns relevant txs (Steve Myers)
3e1fd2b fix(wallet): `transactions` method should only return relevant txs (志宇)

Pull request description:

  Fixes #1239

  ### Description

  Currently the behavior of `Wallet::transactions` is not well defined and unintuitive.

  The approach taken in this PR is to make `Wallet::transactions` return what most wallets would like the caller to see (i.e. transactions that are part of the canonical history and spend from/to a tracked spk). A.k.a make the method more restrictive.

  Documentation is updated to refer the caller to the underlying `bdk_chain` structures for any over usecase.

  After #1765 gets merged, the behavior of `Wallet::transactions` will become even more unintuitive. Refer to #1765 (review).

  ### Notes to the reviewers

  **Why not have multiple methods in `Wallet` that return different sets of transactions?**

  I think it's better to only provide common usecase histories from `Wallet` and I can only think of one.

  ### Changelog notice

  * Change `Wallet::transactions` to only include "relevant" transactions.

  ### 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

ACKs for top commit:
  luisschwab:
    tACK 75fae3e
  notmandatory:
    tACK 75fae3e
  oleonardolima:
    tACK 75fae3e
  ValuedMammal:
    ACK 75fae3e

Tree-SHA512: abf159e0c5d44842d7e0fc5ebc6829d34646fbc45d07bb145ce327f368db0e571ab7c5731a12e63258dfc74abb9d4ff1b841842de8341e0f21b5cbb2becc5e5f

crates/wallet/src/wallet/mod.rs

@@ -19,6 +19,7 @@ use alloc::{
     sync::Arc,
     vec::Vec,
 };
+use chain::Indexer;
 use core::{cmp::Ordering, fmt, mem, ops::Deref};
 
 use bdk_chain::{
@@ -1062,14 +1063,30 @@ impl Wallet {
             .find(|tx| tx.tx_node.txid == txid)
     }
 
-    /// Iterate over the transactions in the wallet.
+    /// Iterate over relevant and canonical transactions in the wallet.
+    ///
+    /// A transaction is relevant when it spends from or spends to at least one tracked output. A
+    /// transaction is canonical when it is confirmed in the best chain, or does not conflict
+    /// with any transaction confirmed in the best chain.
+    ///
+    /// To iterate over all transactions, including those that are irrelevant and not canonical, use
+    /// [`TxGraph::full_txs`].
+    ///
+    /// To iterate over all canonical transactions, including those that are irrelevant, use
+    /// [`TxGraph::list_canonical_txs`].
     pub fn transactions(&self) -> impl Iterator<Item = WalletTx> + '_ {
-        self.indexed_graph
-            .graph()
+        let tx_graph = self.indexed_graph.graph();
+        let tx_index = &self.indexed_graph.index;
+        tx_graph
             .list_canonical_txs(&self.chain, self.chain.tip().block_id())
+            .filter(|c_tx| tx_index.is_tx_relevant(&c_tx.tx_node.tx))
     }
 
-    /// Array of transactions in the wallet sorted with a comparator function.
+    /// Array of relevant and canonical transactions in the wallet sorted with a comparator
+    /// function.
+    ///
+    /// This is a helper method equivalent to collecting the result of [`Wallet::transactions`]
+    /// into a [`Vec`] and then sorting it.
     ///
     /// # Example
     ///

crates/wallet/tests/wallet.rs

@@ -4,15 +4,15 @@ use std::sync::Arc;
 
 use anyhow::Context;
 use assert_matches::assert_matches;
-use bdk_chain::{BlockId, ChainPosition, ConfirmationBlockTime};
+use bdk_chain::{BlockId, ChainPosition, ConfirmationBlockTime, TxUpdate};
 use bdk_wallet::coin_selection::{self, LargestFirstCoinSelection};
 use bdk_wallet::descriptor::{calc_checksum, DescriptorError, IntoWalletDescriptor};
 use bdk_wallet::error::CreateTxError;
 use bdk_wallet::psbt::PsbtUtils;
 use bdk_wallet::signer::{SignOptions, SignerError};
 use bdk_wallet::test_utils::*;
 use bdk_wallet::tx_builder::AddForeignUtxoError;
-use bdk_wallet::{AddressInfo, Balance, ChangeSet, Wallet, WalletPersister, WalletTx};
+use bdk_wallet::{AddressInfo, Balance, ChangeSet, Update, Wallet, WalletPersister, WalletTx};
 use bdk_wallet::{KeychainKind, LoadError, LoadMismatch, LoadWithPersistError};
 use bitcoin::constants::{ChainHash, COINBASE_MATURITY};
 use bitcoin::hashes::Hash;
@@ -4239,3 +4239,52 @@ fn test_tx_builder_is_send_safe() {
     let (mut wallet, _txid) = get_funded_wallet_wpkh();
     let _box: Box<dyn Send + Sync> = Box::new(wallet.build_tx());
 }
+
+#[test]
+fn test_wallet_transactions_relevant() {
+    let (mut test_wallet, _txid) = get_funded_wallet_wpkh();
+    let relevant_tx_count_before = test_wallet.transactions().count();
+    let full_tx_count_before = test_wallet.tx_graph().full_txs().count();
+    let chain_tip = test_wallet.local_chain().tip().block_id();
+    let canonical_tx_count_before = test_wallet
+        .tx_graph()
+        .list_canonical_txs(test_wallet.local_chain(), chain_tip)
+        .count();
+
+    // add not relevant transaction to test wallet
+    let (other_external_desc, other_internal_desc) = get_test_tr_single_sig_xprv_and_change_desc();
+    let (other_wallet, other_txid) = get_funded_wallet(other_internal_desc, other_external_desc);
+    let other_tx_node = other_wallet.get_tx(other_txid).unwrap().tx_node;
+    let other_tx_confirmationblocktime = other_tx_node.anchors.iter().last().unwrap();
+    let other_tx_update = TxUpdate {
+        txs: vec![other_tx_node.tx],
+        txouts: Default::default(),
+        anchors: [(*other_tx_confirmationblocktime, other_txid)].into(),
+        seen_ats: [(other_txid, other_tx_confirmationblocktime.confirmation_time)].into(),
+    };
+    let test_wallet_update = Update {
+        last_active_indices: Default::default(),
+        tx_update: other_tx_update,
+        chain: None,
+    };
+    test_wallet.apply_update(test_wallet_update).unwrap();
+
+    // verify transaction from other wallet was added but is not it relevant transactions list.
+    let relevant_tx_count_after = test_wallet.transactions().count();
+    let full_tx_count_after = test_wallet.tx_graph().full_txs().count();
+    let canonical_tx_count_after = test_wallet
+        .tx_graph()
+        .list_canonical_txs(test_wallet.local_chain(), chain_tip)
+        .count();
+
+    assert_eq!(relevant_tx_count_before, relevant_tx_count_after);
+    assert!(!test_wallet
+        .transactions()
+        .any(|wallet_tx| wallet_tx.tx_node.txid == other_txid));
+    assert!(test_wallet
+        .tx_graph()
+        .list_canonical_txs(test_wallet.local_chain(), chain_tip)
+        .any(|wallet_tx| wallet_tx.tx_node.txid == other_txid));
+    assert!(full_tx_count_before < full_tx_count_after);
+    assert!(canonical_tx_count_before < canonical_tx_count_after);
+}