Merge #1798: Refactor/use iterators to preselect utxos

2f83b45 test(wallet): check there are no duplicates across required and optional utxos (nymius)
39df2b9 refactor(wallet): remove coin_selection::filter_duplicates (nymius)
79bd7da refactor(wallet): use iterators and adaptors in preselect_utxos (nymius)

Pull request description:

  ### Description

  There were multiple calls for de-duplication of selected UTxOs in `Wallet::create_tx`: ([1](https://github.com/bitcoindevkit/bdk/blob/abc305612160c0e2ce85c7bba4c2c162ff488adc/crates/wallet/src/wallet/mod.rs#L2016-L2020)) and ([2](https://github.com/bitcoindevkit/bdk/blob/abc305612160c0e2ce85c7bba4c2c162ff488adc/crates/wallet/src/wallet/mod.rs#L1452)).

  As the test [`test_filter_duplicates`](https://github.com/bitcoindevkit/bdk/blob/master/crates/wallet/src/wallet/coin_selection.rs#L1666-L1695) shows, there are four possible cases for duplication of UTxOs while feeding the coin selection algorithms.

  1. no duplication: out of concern
  2. duplication in the required utxos only: covered by the source of `required_utxos`, `Wallet::list_unspent`, which [roots back the provided `UTxOs` to a `HashMap`](https://github.com/bitcoindevkit/bdk/blob/a5335a18431dfecea1f524af44c953b79a96867c/crates/chain/src/tx_graph.rs#L911-L912) which should avoid any duplication by definition
  3. duplication in the optional utxos only: is the only one possible as optional `UTxOs` are stored in a `Vec` and no checks are performed about the duplicity of their members.
  4. duplication across the required and optional utxos: is already covered by `Wallet::preselect_utxos`, which avoid the processing of required UTxOs while listing the unspent available UTxOs in the wallet.

  This refactor does the following:
  - Refactors `TxParams::utxos` type to be `HashSet<LocalOutput>` avoiding the duplication case 3
  - Keeps avoiding case 4 without overhead and allowing a query closer to O(1) on avg. to cover duplication case 4 (before was O(n) where n is the size of required utxos) thanks to the above change.
  - Still covers case 2 because the [source of `required_utxos`, `Wallet::list_unspent`](https://github.com/bitcoindevkit/bdk/blob/a5335a18431dfecea1f524af44c953b79a96867c/crates/chain/src/tx_graph.rs#L911-L912) comes from a `HashMap` which should avoid duplication by definition.
  - Moves the computation of the `WeightedUtxos` to the last part of UTxO filtering, allowing the unification of the computation for local outputs.
  - Removes some extra allocations done for helpers structures or intermediate results while filtering UTxOs.
  - Allows for future integration of UTxO filtering methods for other utilities, e.g.: filter locked utxos
    ```rust
    .filter(|utxo| !self.is_utxo_locked(utxo.outpoint, self.latest_checkpoint().height()))
    ```
  - Adds more comments for each filtering step.
  - Differentiates `foreign_utxos`, which should include a provided satisfation weight to use them effectively, and `utxos`, manually selected UTxOs for which the wallet can compute their satisfaction weight without external resources.

  With these changes all four cases would be covered, and `coin_selection::filter_duplicates` is no longer needed.

  Fixes #1794.

  ### Notes to the reviewers

  I added three test to cover the interesting cases for duplication:
      - there are no duplicates in required utxos
      - there are no duplicates in optional utxos
      - there are no duplicates across optional and required utxos
  the three of them have been prefixed with `not_duplicated_utxos*` to allow its joint execution under the command:
  ```bash
  cargo test -- not_duplicated_utxos
  ```
  because the guarantees for the three conditions above are spread in different parts of the code.

  ### Changelog notice

  No changes to public APIs.

  ### Checklists

  * [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
  * [x] This pull request does not break 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:
  evanlinjin:
    ACK 2f83b45

Tree-SHA512: 82317acce8a7e83e4ea1a500605142026ab10d6842d4f278b09563566f1eb4d295f77a9c9616eff067211f0faa9f1e94370fc0ec3af48e6e1baef53b46979db7

Author: evanlinjin

crates/wallet/src/wallet/coin_selection.rs

@@ -101,15 +101,13 @@
 //! # Ok::<(), anyhow::Error>(())
 //! ```
 
-use crate::chain::collections::HashSet;
 use crate::wallet::utils::IsDust;
 use crate::Utxo;
 use crate::WeightedUtxo;
 use bitcoin::{Amount, FeeRate, SignedAmount};
 
 use alloc::vec::Vec;
 use bitcoin::consensus::encode::serialize;
-use bitcoin::OutPoint;
 use bitcoin::TxIn;
 use bitcoin::{Script, Weight};
 
@@ -720,38 +718,19 @@ fn calculate_cs_result(
     }
 }
 
-/// Remove duplicate UTXOs.
-///
-/// If a UTXO appears in both `required` and `optional`, the appearance in `required` is kept.
-pub(crate) fn filter_duplicates<I>(required: I, optional: I) -> (I, I)
-where
-    I: IntoIterator<Item = WeightedUtxo> + FromIterator<WeightedUtxo>,
-{
-    let mut visited = HashSet::<OutPoint>::new();
-    let required = required
-        .into_iter()
-        .filter(|utxo| visited.insert(utxo.utxo.outpoint()))
-        .collect::<I>();
-    let optional = optional
-        .into_iter()
-        .filter(|utxo| visited.insert(utxo.utxo.outpoint()))
-        .collect::<I>();
-    (required, optional)
-}
-
 #[cfg(test)]
 mod test {
     use assert_matches::assert_matches;
     use bitcoin::hashes::Hash;
-    use chain::{BlockId, ChainPosition, ConfirmationBlockTime};
+    use bitcoin::OutPoint;
+    use chain::{ChainPosition, ConfirmationBlockTime};
     use core::str::FromStr;
     use rand::rngs::StdRng;
 
     use bitcoin::{Amount, BlockHash, ScriptBuf, TxIn, TxOut};
 
     use super::*;
     use crate::types::*;
-    use crate::wallet::coin_selection::filter_duplicates;
 
     use rand::prelude::SliceRandom;
     use rand::{thread_rng, Rng, RngCore, SeedableRng};
@@ -1618,102 +1597,6 @@ mod test {
         assert_eq!(res.selected_amount(), Amount::from_sat(200_000));
     }
 
-    #[test]
-    fn test_filter_duplicates() {
-        fn utxo(txid: &str, value: u64) -> WeightedUtxo {
-            WeightedUtxo {
-                satisfaction_weight: Weight::ZERO,
-                utxo: Utxo::Local(LocalOutput {
-                    outpoint: OutPoint::new(bitcoin::hashes::Hash::hash(txid.as_bytes()), 0),
-                    txout: TxOut {
-                        value: Amount::from_sat(value),
-                        script_pubkey: ScriptBuf::new(),
-                    },
-                    keychain: KeychainKind::External,
-                    is_spent: false,
-                    derivation_index: 0,
-                    chain_position: ChainPosition::Confirmed {
-                        anchor: ConfirmationBlockTime {
-                            block_id: BlockId {
-                                height: 12345,
-                                hash: BlockHash::all_zeros(),
-                            },
-                            confirmation_time: 12345,
-                        },
-                        transitively: None,
-                    },
-                }),
-            }
-        }
-
-        fn to_utxo_vec(utxos: &[(&str, u64)]) -> Vec<WeightedUtxo> {
-            let mut v = utxos
-                .iter()
-                .map(|&(txid, value)| utxo(txid, value))
-                .collect::<Vec<_>>();
-            v.sort_by_key(|u| u.utxo.outpoint());
-            v
-        }
-
-        struct TestCase<'a> {
-            name: &'a str,
-            required: &'a [(&'a str, u64)],
-            optional: &'a [(&'a str, u64)],
-            exp_required: &'a [(&'a str, u64)],
-            exp_optional: &'a [(&'a str, u64)],
-        }
-
-        let test_cases = [
-            TestCase {
-                name: "no_duplicates",
-                required: &[("A", 1000), ("B", 2100)],
-                optional: &[("C", 1000)],
-                exp_required: &[("A", 1000), ("B", 2100)],
-                exp_optional: &[("C", 1000)],
-            },
-            TestCase {
-                name: "duplicate_required_utxos",
-                required: &[("A", 3000), ("B", 1200), ("C", 1234), ("A", 3000)],
-                optional: &[("D", 2100)],
-                exp_required: &[("A", 3000), ("B", 1200), ("C", 1234)],
-                exp_optional: &[("D", 2100)],
-            },
-            TestCase {
-                name: "duplicate_optional_utxos",
-                required: &[("A", 3000), ("B", 1200)],
-                optional: &[("C", 5000), ("D", 1300), ("C", 5000)],
-                exp_required: &[("A", 3000), ("B", 1200)],
-                exp_optional: &[("C", 5000), ("D", 1300)],
-            },
-            TestCase {
-                name: "duplicate_across_required_and_optional_utxos",
-                required: &[("A", 3000), ("B", 1200), ("C", 2100)],
-                optional: &[("A", 3000), ("D", 1200), ("E", 5000)],
-                exp_required: &[("A", 3000), ("B", 1200), ("C", 2100)],
-                exp_optional: &[("D", 1200), ("E", 5000)],
-            },
-        ];
-
-        for (i, t) in test_cases.into_iter().enumerate() {
-            let (required, optional) =
-                filter_duplicates(to_utxo_vec(t.required), to_utxo_vec(t.optional));
-            assert_eq!(
-                required,
-                to_utxo_vec(t.exp_required),
-                "[{}:{}] unexpected `required` result",
-                i,
-                t.name
-            );
-            assert_eq!(
-                optional,
-                to_utxo_vec(t.exp_optional),
-                "[{}:{}] unexpected `optional` result",
-                i,
-                t.name
-            );
-        }
-    }
-
     #[test]
     fn test_deterministic_coin_selection_picks_same_utxos() {
         enum CoinSelectionAlgo {