Merge bitcoindevkit/bdk#1279: Filter duplicate coins before coin selection

5299db34cb9117ad1b66a6afcb51f6ca7e1f0d95 fix(wallet): filter duplicates before coin selection (志宇)
d9501187ef3c4436b7baedb23a4d4f6b1f46a03d test(wallet): fix tests helpers to generate unique utxos (志宇)

Pull request description:

  Fixes #1240

  ### Description

  We now filter out duplicate coins before calling `CoinSelectionAlgorithm::coin_select`. If a UTXO exists in both `required_utxos` and `optional_utxos`, only the copy in `required_utxos` will be kept.

  Test helper methods are also updated to not create duplicate UTXOs.

  ### Changelog notice

  Fixed

  * Filter out duplicate UTXOs before calling `CoinSelectionAlgorithm::coin_select`. If a UTXO exists in both `required_utxos` and `optional_utxos`, only the copy in `required_utxos` will be kept.

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

  ~* [ ] 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:
  danielabrozzoni:
    utACK 5299db34cb9117ad1b66a6afcb51f6ca7e1f0d95

Tree-SHA512: 9cb5517b7f74f89c06172efc344766b16b3216a25b1ebdd6eb84767a9e103124cead9eb0a7f3b5feb562fbb925517a9bf0404399de74b4e898982a5b3795aa04

crates/bdk/src/wallet/coin_selection.rs

@@ -100,6 +100,7 @@
 //! # Ok::<(), anyhow::Error>(())
 //! ```
 
+use crate::chain::collections::HashSet;
 use crate::types::FeeRate;
 use crate::wallet::utils::IsDust;
 use crate::Utxo;
@@ -107,6 +108,7 @@ use crate::WeightedUtxo;
 
 use alloc::vec::Vec;
 use bitcoin::consensus::encode::serialize;
+use bitcoin::OutPoint;
 use bitcoin::{Script, Weight};
 
 use core::convert::TryInto;
@@ -711,6 +713,25 @@ impl BranchAndBoundCoinSelection {
     }
 }
 
+/// 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;
@@ -721,6 +742,7 @@ mod test {
 
     use super::*;
     use crate::types::*;
+    use crate::wallet::coin_selection::filter_duplicates;
     use crate::wallet::Vbytes;
 
     use rand::rngs::StdRng;
@@ -799,13 +821,14 @@ mod test {
 
     fn generate_random_utxos(rng: &mut StdRng, utxos_number: usize) -> Vec<WeightedUtxo> {
         let mut res = Vec::new();
-        for _ in 0..utxos_number {
+        for i in 0..utxos_number {
             res.push(WeightedUtxo {
                 satisfaction_weight: P2WPKH_SATISFACTION_SIZE,
                 utxo: Utxo::Local(LocalOutput {
-                    outpoint: OutPoint::from_str(
+                    outpoint: OutPoint::from_str(&format!(
-                        "ebd9813ecebc57ff8f30797de7c205e3c7498ca950ea4341ee51a685ff2fa30a:0",
+                        "ebd9813ecebc57ff8f30797de7c205e3c7498ca950ea4341ee51a685ff2fa30a:{}",
-                    )
+                        i
+                    ))
                     .unwrap(),
                     txout: TxOut {
                         value: rng.gen_range(0..200000000),
@@ -829,12 +852,14 @@ mod test {
     }
 
     fn generate_same_value_utxos(utxos_value: u64, utxos_number: usize) -> Vec<WeightedUtxo> {
-        let utxo = WeightedUtxo {
+        (0..utxos_number)
+            .map(|i| WeightedUtxo {
                 satisfaction_weight: P2WPKH_SATISFACTION_SIZE,
                 utxo: Utxo::Local(LocalOutput {
-                outpoint: OutPoint::from_str(
+                    outpoint: OutPoint::from_str(&format!(
-                    "ebd9813ecebc57ff8f30797de7c205e3c7498ca950ea4341ee51a685ff2fa30a:0",
+                        "ebd9813ecebc57ff8f30797de7c205e3c7498ca950ea4341ee51a685ff2fa30a:{}",
-                )
+                        i
+                    ))
                     .unwrap(),
                     txout: TxOut {
                         value: utxos_value,
@@ -845,8 +870,8 @@ mod test {
                     derivation_index: 42,
                     confirmation_time: ConfirmationTime::Unconfirmed { last_seen: 0 },
                 }),
-        };
+            })
-        vec![utxo; utxos_number]
+            .collect()
     }
 
     fn sum_random_utxos(mut rng: &mut StdRng, utxos: &mut Vec<WeightedUtxo>) -> u64 {
@@ -1478,4 +1503,95 @@ mod test {
             })
         );
     }
+
+    #[test]
+    fn test_filter_duplicates() {
+        fn utxo(txid: &str, value: u64) -> WeightedUtxo {
+            WeightedUtxo {
+                satisfaction_weight: 0,
+                utxo: Utxo::Local(LocalOutput {
+                    outpoint: OutPoint::new(bitcoin::hashes::Hash::hash(txid.as_bytes()), 0),
+                    txout: TxOut {
+                        value,
+                        script_pubkey: ScriptBuf::new(),
+                    },
+                    keychain: KeychainKind::External,
+                    is_spent: false,
+                    derivation_index: 0,
+                    confirmation_time: ConfirmationTime::Confirmed {
+                        height: 12345,
+                        time: 12345,
+                    },
+                }),
+            }
+        }
+
+        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() {
+            println!("Case {}: {}", i, t.name);
+            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
+            );
+        }
+    }
 }

crates/bdk/src/wallet/mod.rs

@@ -1546,6 +1546,9 @@ impl<D> Wallet<D> {
             }
         };
 
+        let (required_utxos, optional_utxos) =
+            coin_selection::filter_duplicates(required_utxos, optional_utxos);
+
         let coin_selection = coin_selection.coin_select(
             required_utxos,
             optional_utxos,