refactor(wallet): use iterators and adaptors in preselect_utxos

There were multiple calls for de-duplication of selected UTxOs.

As the test `test_filter_duplicates` 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` 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 changes:
- `TxParams::utxos` type to be `HashSet<LocalOutput>` avoiding the
  duplication case 3, 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).
- 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.
- Adds more comments for each filtering step.

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

Author: nymius

crates/wallet/src/wallet/mod.rs

@@ -1618,60 +1618,54 @@ impl Wallet {
             .map_err(|_| BuildFeeBumpError::FeeRateUnavailable)?;
 
         // remove the inputs from the tx and process them
-        let original_txin = tx.input.drain(..).collect::<Vec<_>>();
-        let original_utxos = original_txin
-            .iter()
-            .map(|txin| -> Result<_, BuildFeeBumpError> {
-                let prev_tx = graph
-                    .get_tx(txin.previous_output.txid)
-                    .ok_or(BuildFeeBumpError::UnknownUtxo(txin.previous_output))?;
-                let txout = &prev_tx.output[txin.previous_output.vout as usize];
-
-                let chain_position = chain_positions
-                    .get(&txin.previous_output.txid)
-                    .cloned()
-                    .ok_or(BuildFeeBumpError::UnknownUtxo(txin.previous_output))?;
-
-                let weighted_utxo = match txout_index.index_of_spk(txout.script_pubkey.clone()) {
-                    Some(&(keychain, derivation_index)) => {
-                        let satisfaction_weight = self
-                            .public_descriptor(keychain)
-                            .max_weight_to_satisfy()
-                            .unwrap();
-                        WeightedUtxo {
-                            utxo: Utxo::Local(LocalOutput {
-                                outpoint: txin.previous_output,
-                                txout: txout.clone(),
-                                keychain,
-                                is_spent: true,
-                                derivation_index,
-                                chain_position,
-                            }),
-                            satisfaction_weight,
-                        }
-                    }
-                    None => {
-                        let satisfaction_weight = Weight::from_wu_usize(
-                            serialize(&txin.script_sig).len() * 4 + serialize(&txin.witness).len(),
-                        );
-                        WeightedUtxo {
-                            utxo: Utxo::Foreign {
-                                outpoint: txin.previous_output,
-                                sequence: txin.sequence,
-                                psbt_input: Box::new(psbt::Input {
-                                    witness_utxo: Some(txout.clone()),
-                                    non_witness_utxo: Some(prev_tx.as_ref().clone()),
-                                    ..Default::default()
-                                }),
-                            },
-                            satisfaction_weight,
+        let (local, foreign): (HashSet<LocalOutput>, Vec<WeightedUtxo>) =
+            tx.input.drain(..).try_fold(
+                (HashSet::<LocalOutput>::new(), Vec::<WeightedUtxo>::new()),
+                |(mut local, mut foreign), txin| -> Result<_, BuildFeeBumpError> {
+                    let prev_tx = graph
+                        .get_tx(txin.previous_output.txid)
+                        .ok_or(BuildFeeBumpError::UnknownUtxo(txin.previous_output))?;
+                    let txout = &prev_tx.output[txin.previous_output.vout as usize];
+
+                    let chain_position =
+                        chain_positions
+                            .get(&txin.previous_output.txid)
+                            .cloned()
+                            .ok_or(BuildFeeBumpError::UnknownUtxo(txin.previous_output))?;
+
+                    match txout_index.index_of_spk(txout.script_pubkey.clone()) {
+                        Some(&(keychain, derivation_index)) => local.insert(LocalOutput {
+                            outpoint: txin.previous_output,
+                            txout: txout.clone(),
+                            keychain,
+                            is_spent: true,
+                            derivation_index,
+                            chain_position,
+                        }),
+                        None => {
+                            let satisfaction_weight = Weight::from_wu_usize(
+                                serialize(&txin.script_sig).len() * 4
+                                    + serialize(&txin.witness).len(),
+                            );
+                            foreign.push(WeightedUtxo {
+                                utxo: Utxo::Foreign {
+                                    outpoint: txin.previous_output,
+                                    sequence: txin.sequence,
+                                    psbt_input: Box::new(psbt::Input {
+                                        witness_utxo: Some(txout.clone()),
+                                        non_witness_utxo: Some(prev_tx.as_ref().clone()),
+                                        ..Default::default()
+                                    }),
+                                },
+                                satisfaction_weight,
+                            });
+                            true
                         }
-                    }
-                };
+                    };
 
-                Ok(weighted_utxo)
-            })
-            .collect::<Result<Vec<_>, _>>()?;
+                    Ok((local, foreign))
+                },
+            )?;
 
         if tx.output.len() > 1 {
             let mut change_index = None;
@@ -1698,7 +1692,8 @@ impl Wallet {
                 .into_iter()
                 .map(|txout| (txout.script_pubkey, txout.value))
                 .collect(),
-            utxos: original_utxos,
+            utxos: local,
+            foreign_utxos: foreign,
             bumping_fee: Some(tx_builder::PreviousFee {
                 absolute: fee,
                 rate: fee_rate,
@@ -1976,116 +1971,85 @@ impl Wallet {
         descriptor.at_derivation_index(child).ok()
     }
 
-    fn get_available_utxos(&self) -> Vec<(LocalOutput, Weight)> {
-        self.list_unspent()
-            .map(|utxo| {
-                let keychain = utxo.keychain;
-                (utxo, {
-                    self.public_descriptor(keychain)
-                        .max_weight_to_satisfy()
-                        .unwrap()
-                })
-            })
-            .collect()
-    }
-
     /// Given the options returns the list of utxos that must be used to form the
     /// transaction and any further that may be used if needed.
     fn preselect_utxos(
         &self,
         params: &TxParams,
         current_height: Option<u32>,
     ) -> (Vec<WeightedUtxo>, Vec<WeightedUtxo>) {
-        let TxParams {
-            change_policy,
-            unspendable,
-            utxos,
-            drain_wallet,
-            manually_selected_only,
-            bumping_fee,
-            ..
-        } = params;
-
-        let manually_selected = utxos.clone();
-        // we mandate confirmed transactions if we're bumping the fee
-        let must_only_use_confirmed_tx = bumping_fee.is_some();
-        let must_use_all_available = *drain_wallet;
-
-        //    must_spend <- manually selected utxos
-        //    may_spend  <- all other available utxos
-        let mut may_spend = self.get_available_utxos();
-
-        may_spend.retain(|may_spend| {
-            !manually_selected
-                .iter()
-                .any(|manually_selected| manually_selected.utxo.outpoint() == may_spend.0.outpoint)
-        });
-        let mut must_spend = manually_selected;
-
-        // NOTE: we are intentionally ignoring `unspendable` here. i.e manual
-        // selection overrides unspendable.
-        if *manually_selected_only {
-            return (must_spend, vec![]);
-        }
-
-        let satisfies_confirmed = may_spend
-            .iter()
-            .map(|u| -> bool {
-                let txid = u.0.outpoint.txid;
-                let tx = match self.indexed_graph.graph().get_tx(txid) {
-                    Some(tx) => tx,
-                    None => return false,
-                };
-
-                // Whether the UTXO is mature and, if needed, confirmed
-                let mut spendable = true;
-                let chain_position = u.0.chain_position;
-                if must_only_use_confirmed_tx && !chain_position.is_confirmed() {
-                    return false;
-                }
-                if tx.is_coinbase() {
-                    debug_assert!(
-                        chain_position.is_confirmed(),
-                        "coinbase must always be confirmed"
-                    );
-                    if let Some(current_height) = current_height {
-                        match chain_position {
-                            ChainPosition::Confirmed { anchor, .. } => {
-                                // https://github.com/bitcoin/bitcoin/blob/c5e67be03bb06a5d7885c55db1f016fbf2333fe3/src/validation.cpp#L373-L375
-                                spendable &= (current_height
-                                    .saturating_sub(anchor.block_id.height))
-                                    >= COINBASE_MATURITY;
-                            }
-                            ChainPosition::Unconfirmed { .. } => spendable = false,
-                        }
-                    }
+        self
+            // get all unspent UTxOs from wallet
+            .list_unspent()
+            // only process UTxOs not selected manually, they will be considered later in the chain
+            // NOTE: this avoid UTxOs in both required and optional list
+            .filter(|may_spend| !params.utxos.contains(may_spend))
+            // only process optional UTxOs if manually_selected_only is false
+            .take_while(|_| !params.manually_selected_only)
+            // only add to optional UTxOs those which satisfy the change policy if we reuse change
+            .filter(|local_output| {
+                self.keychains().count() == 1 || params.change_policy.is_satisfied_by(local_output)
+            })
+            // only add to optional UTxOs those marked as spendable
+            .filter(|local_output| !params.unspendable.contains(&local_output.outpoint))
+            // if bumping fees only add to optional UTxOs those confirmed
+            .filter(|local_output| {
+                params.bumping_fee.is_none() || local_output.chain_position.is_confirmed()
+            })
+            // only use UTxOs we posess and are mature if the origin tx is coinbase
+            .filter(|local_output| {
+                self.indexed_graph
+                    .graph()
+                    .get_tx(local_output.outpoint.txid)
+                    .is_some_and(|tx| {
+                        !tx.is_coinbase()
+                            || (local_output.chain_position.is_confirmed()
+                                && current_height.is_some_and(|current_height| {
+                                    local_output
+                                        .chain_position
+                                        .confirmation_height_upper_bound()
+                                        .is_some_and(|local_output_height| {
+                                            current_height.saturating_sub(local_output_height)
+                                                >= COINBASE_MATURITY
+                                        })
+                                }))
+                    })
+            })
+            // combine optional UTxOs with manually selected UTxOs
+            // NOTE: if manually_selected_only is true, then the previous iterator in the chain is
+            // empty and only manually selected UTxOs will be considered
+            // NOTE: manual selection overrides unspendable
+            .chain(params.utxos.clone())
+            // transform every UTxO into a WeightedUtxo
+            .map(|utxo| {
+                let keychain = utxo.keychain;
+                WeightedUtxo {
+                    satisfaction_weight: self
+                        .public_descriptor(keychain)
+                        .max_weight_to_satisfy()
+                        .unwrap(),
+                    utxo: Utxo::Local(utxo),
                 }
-                spendable
             })
-            .collect::<Vec<_>>();
-
-        let mut i = 0;
-        may_spend.retain(|u| {
-            let retain = (self.keychains().count() == 1 || change_policy.is_satisfied_by(&u.0))
-                && !unspendable.contains(&u.0.outpoint)
-                && satisfies_confirmed[i];
-            i += 1;
-            retain
-        });
-
-        let mut may_spend = may_spend
-            .into_iter()
-            .map(|(local_utxo, satisfaction_weight)| WeightedUtxo {
-                satisfaction_weight,
-                utxo: Utxo::Local(local_utxo),
+            // include foreign UTxOs
+            .chain(params.foreign_utxos.clone())
+            // TODO: here preselect_utxos could be splitted further into two functions to allow the
+            // addition of extra filters for inputs based on properties of the WeightedUtxos
+            //
+            // split UTxOs in required and optional
+            // if drain_wallet is true, all UTxOs are required, if false, only manually selected
+            // and foreign are required
+            // TODO: collection could be kept separate from selection
+            .partition::<Vec<WeightedUtxo>, _>(|x| {
+                params.drain_wallet
+                    // keep UTxO in required UTxOs if
+                    || match x.utxo {
+                        // it is in manually selected group
+                        Utxo::Local(ref lo) => params.utxos.contains(lo),
+                        // it is foreign
+                        Utxo::Foreign {..} => true,
+                    }
             })
-            .collect();
-
-        if must_use_all_available {
-            must_spend.append(&mut may_spend);
-        }
-
-        (must_spend, may_spend)
     }
 
     fn complete_transaction(

crates/wallet/src/wallet/tx_builder.rs

@@ -125,7 +125,8 @@ pub(crate) struct TxParams {
     pub(crate) fee_policy: Option<FeePolicy>,
     pub(crate) internal_policy_path: Option<BTreeMap<String, Vec<usize>>>,
     pub(crate) external_policy_path: Option<BTreeMap<String, Vec<usize>>>,
-    pub(crate) utxos: Vec<WeightedUtxo>,
+    pub(crate) utxos: HashSet<LocalOutput>,
+    pub(crate) foreign_utxos: Vec<WeightedUtxo>,
     pub(crate) unspendable: HashSet<OutPoint>,
     pub(crate) manually_selected_only: bool,
     pub(crate) sighash: Option<psbt::PsbtSighashType>,
@@ -274,26 +275,14 @@ impl<'a, Cs> TxBuilder<'a, Cs> {
     /// These have priority over the "unspendable" utxos, meaning that if a utxo is present both in
     /// the "utxos" and the "unspendable" list, it will be spent.
     pub fn add_utxos(&mut self, outpoints: &[OutPoint]) -> Result<&mut Self, AddUtxoError> {
-        {
-            let wallet = &mut self.wallet;
-            let utxos = outpoints
-                .iter()
+        let _ = outpoints.iter().try_for_each(|outpoint| {
+            self.wallet
+                .get_utxo(*outpoint)
                 .map(|outpoint| {
-                    wallet
-                        .get_utxo(*outpoint)
-                        .ok_or(AddUtxoError::UnknownUtxo(*outpoint))
+                    self.params.utxos.insert(outpoint);
                 })
-                .collect::<Result<Vec<_>, _>>()?;
-
-            for utxo in utxos {
-                let descriptor = wallet.public_descriptor(utxo.keychain);
-                let satisfaction_weight = descriptor.max_weight_to_satisfy().unwrap();
-                self.params.utxos.push(WeightedUtxo {
-                    satisfaction_weight,
-                    utxo: Utxo::Local(utxo),
-                });
-            }
-        }
+                .ok_or(AddUtxoError::UnknownUtxo(*outpoint))
+        });
 
         Ok(self)
     }
@@ -393,7 +382,7 @@ impl<'a, Cs> TxBuilder<'a, Cs> {
             }
         }
 
-        self.params.utxos.push(WeightedUtxo {
+        self.params.foreign_utxos.push(WeightedUtxo {
             satisfaction_weight,
             utxo: Utxo::Foreign {
                 outpoint,