Merge bitcoindevkit#265: Prefer Utxo::Local over Utxo::Foreign in OldestFirstCoinSelection

345cc6e fix(coin_selection): prefer Utxo::Local over Utxo::Foreign in OldestFirstCoinSelection (nymius)

Pull request description:

  ### Description

  The comments in the `OldestFirstCoinSelection` implementation stated the following:
  > // For utxo that doesn't exist in DB, they will have lowest priority to be selected

  But this was not honored in the code.

  This PR enforces this and ensures the expected behaviour through the two following new tests:
  - `test_oldest_first_coin_selection_uses_all_optional_with_foreign_utxo_locals_sorted_first`
  - `test_oldest_first_coin_selection_uses_only_all_optional_local_utxos_not_a_single_foreign`

  Fixes bitcoindevkit#264

  ### Changelog notice

  No public APIs are changed by these commits.

  ### Checklists

  > [!IMPORTANT]
  > This pull request **DOES NOT** break the existing API
  * [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 +nightly fmt` and `cargo clippy` before committing
  * [x] I've added tests for the new code
  * [x] I've expanded docs addressing new code
  * [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:
  ValuedMammal:
    ACK 345cc6e
  oleonardolima:
    ACK 345cc6e

Tree-SHA512: 8a538e0765ca55f870fcbc33e4c865656e9074765a5bb13c370afe2626dbf3ecbb71e8a372607e1e405a9e35f26418bfdd9c71fec018999858a5fe5680af866b

Author: oleonardolima

wallet/src/wallet/coin_selection.rs

@@ -275,11 +275,12 @@ impl CoinSelectionAlgorithm for OldestFirstCoinSelection {
     ) -> Result<CoinSelectionResult, InsufficientFunds> {
         // We put the "required UTXOs" first and make sure the optional UTXOs are sorted from
         // oldest to newest according to blocktime
-        // For utxo that doesn't exist in DB, they will have lowest priority to be selected
+        // For UTXOs that doesn't exist in DB (Utxo::Foreign), they will have lowest priority to be
+        // selected
         let utxos = {
             optional_utxos.sort_unstable_by_key(|wu| match &wu.utxo {
-                Utxo::Local(local) => Some(local.chain_position),
-                Utxo::Foreign { .. } => None,
+                Utxo::Local(local) => (false, Some(local.chain_position)),
+                Utxo::Foreign { .. } => (true, None),
             });
 
             required_utxos
@@ -726,10 +727,11 @@ fn calculate_cs_result(
 mod test {
     use assert_matches::assert_matches;
     use bitcoin::hashes::Hash;
-    use bitcoin::OutPoint;
+    use bitcoin::{psbt, OutPoint, Sequence};
     use chain::{ChainPosition, ConfirmationBlockTime};
     use core::str::FromStr;
     use rand::rngs::StdRng;
+    use std::boxed::Box;
 
     use bitcoin::{Amount, BlockHash, ScriptBuf, TxIn, TxOut};
 
@@ -778,6 +780,30 @@ mod test {
         )
     }
 
+    fn foreign_utxo(value: Amount, index: u32) -> WeightedUtxo {
+        assert!(index < 10);
+        let outpoint = OutPoint::from_str(&format!(
+            "000000000000000000000000000000000000000000000000000000000000000{}:0",
+            index
+        ))
+        .unwrap();
+        WeightedUtxo {
+            utxo: Utxo::Foreign {
+                outpoint,
+                sequence: Sequence(0xFFFFFFFD),
+                psbt_input: Box::new(psbt::Input {
+                    witness_utxo: Some(TxOut {
+                        value,
+                        script_pubkey: ScriptBuf::new(),
+                    }),
+                    non_witness_utxo: None,
+                    ..Default::default()
+                }),
+            },
+            satisfaction_weight: Weight::from_wu_usize(P2WPKH_SATISFACTION_SIZE),
+        }
+    }
+
     fn utxo(
         value: Amount,
         index: u32,
@@ -1051,6 +1077,59 @@ mod test {
         assert_eq!(result.fee_amount, Amount::from_sat(204));
     }
 
+    #[test]
+    fn test_oldest_first_coin_selection_uses_all_optional_with_foreign_utxo_locals_sorted_first() {
+        let utxos = get_oldest_first_test_utxos();
+        let mut all_utxos = vec![foreign_utxo(Amount::from_sat(120_000), 1)];
+        all_utxos.extend_from_slice(&utxos);
+        let drain_script = ScriptBuf::default();
+        let target_amount = Amount::from_sat(619_000) + FEE_AMOUNT;
+
+        let result = OldestFirstCoinSelection
+            .coin_select(
+                vec![],
+                all_utxos,
+                FeeRate::from_sat_per_vb_unchecked(1),
+                target_amount,
+                &drain_script,
+                &mut thread_rng(),
+            )
+            .unwrap();
+
+        assert_eq!(result.selected.len(), 4);
+        assert_eq!(result.selected_amount(), Amount::from_sat(620_000));
+        assert_eq!(result.fee_amount, Amount::from_sat(272));
+        assert!(matches!(result.selected[3], Utxo::Foreign { .. }));
+    }
+
+    #[test]
+    fn test_oldest_first_coin_selection_uses_only_all_optional_local_utxos_not_a_single_foreign() {
+        let utxos = get_oldest_first_test_utxos();
+        let mut all_utxos = vec![foreign_utxo(Amount::from_sat(120_000), 1)];
+        all_utxos.extend_from_slice(&utxos);
+        let drain_script = ScriptBuf::default();
+        let target_amount = Amount::from_sat(499_000) + FEE_AMOUNT;
+
+        let result = OldestFirstCoinSelection
+            .coin_select(
+                vec![],
+                all_utxos,
+                FeeRate::from_sat_per_vb_unchecked(1),
+                target_amount,
+                &drain_script,
+                &mut thread_rng(),
+            )
+            .unwrap();
+
+        assert_eq!(result.selected.len(), 3);
+        assert_eq!(result.selected_amount(), Amount::from_sat(500_000));
+        assert_eq!(result.fee_amount, Amount::from_sat(204));
+        assert!(result
+            .selected
+            .iter()
+            .all(|utxo| matches!(utxo, Utxo::Local(..))));
+    }
+
     #[test]
     fn test_oldest_first_coin_selection_use_only_necessary() {
         let utxos = get_oldest_first_test_utxos();