Merge #390: fix(coin_selection): calculate_cs_result returns the required UTXOs first

8a5e763 test(tx_builder): Add `test_add_utxo_final_outpoint_retained` (valued mammal)
0b5d927 test(wallet): Add `test_tx_ordering_untouched_preserves_insertion_ordering_bnb_success` (valued mammal)
e020391 fix(coin_selection): `calculate_cs_result` returns the required UTXOs first (valued mammal)

Pull request description:

  ### Description

  Follow-up to #262 that addresses transaction input ordering when BnB finds a solution.

  Previously `calculate_cs_result` produced a CoinSelectionResult by appending the required UTXOs onto the selected ones, which changed the expected order of transaction inputs.

  `calculate_cs_result` now returns the required UTXOs before the newly selected ones. This behavior aligns with the expectation that the order of manually selected inputs should be preserved in the final transaction whenever `TxOrdering::Untouched` is specified.

  For related discussion refer to #244 (comment).

  ### Changelog notice

  Fixed

  - wallet: Fixed order of selected UTXOs for `BranchAndBoundCoinSelection`, required UTXOs come first

  ### Checklists

  #### All Submissions:

  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)

  #### Bugfixes:

  * [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:
  110CodingP:
    ACK [`8a5e763`](8a5e763).

Tree-SHA512: 4ebf33f7d1fe6e6dcfff89e218aeefcec92ae8ae78c2589d7f496b433991122f48100be38ffa52b8fc2d67feb679d567a707dea8681810167eac1a6a04f9dcc0

Author: ValuedMammal

src/wallet/coin_selection.rs

@@ -704,16 +704,17 @@ impl CoinSelectionAlgorithm for SingleRandomDraw {
 }
 
 fn calculate_cs_result(
-    mut selected_utxos: Vec<OutputGroup>,
-    mut required_utxos: Vec<OutputGroup>,
+    selected_utxos: Vec<OutputGroup>,
+    required_utxos: Vec<OutputGroup>,
     excess: Excess,
 ) -> CoinSelectionResult {
-    selected_utxos.append(&mut required_utxos);
-    let fee_amount = selected_utxos.iter().map(|u| u.fee).sum();
-    let selected = selected_utxos
+    let mut selected = required_utxos;
+    selected.extend(selected_utxos);
+    let fee_amount = selected.iter().map(|u| u.fee).sum();
+    let selected = selected
         .into_iter()
-        .map(|u| u.weighted_utxo.utxo)
-        .collect::<Vec<_>>();
+        .map(|output_group| output_group.weighted_utxo.utxo)
+        .collect();
 
     CoinSelectionResult {
         selected,

src/wallet/tx_builder.rs

@@ -931,6 +931,7 @@ mod test {
         };
     }
 
+    use crate::test_utils::*;
     use bitcoin::consensus::deserialize;
     use bitcoin::hex::FromHex;
     use bitcoin::TxOut;
@@ -1156,7 +1157,6 @@ mod test {
 
     #[test]
     fn test_exclude_unconfirmed() {
-        use crate::test_utils::*;
         use bdk_chain::BlockId;
         use bitcoin::{hashes::Hash, BlockHash, Network};
 
@@ -1246,7 +1246,6 @@ mod test {
 
     #[test]
     fn test_build_fee_bump_remove_change_output_single_desc() {
-        use crate::test_utils::*;
         use bdk_chain::BlockId;
         use bitcoin::{hashes::Hash, BlockHash, Network};
 
@@ -1292,8 +1291,6 @@ mod test {
 
     #[test]
     fn duplicated_utxos_in_add_utxos_are_only_added_once() {
-        use crate::test_utils::get_funded_wallet_wpkh;
-
         let (mut wallet, _) = get_funded_wallet_wpkh();
         let utxo = wallet.list_unspent().next().unwrap();
         let op = utxo.outpoint;
@@ -1306,7 +1303,6 @@ mod test {
 
     #[test]
     fn not_duplicated_utxos_in_required_list() {
-        use crate::test_utils::get_funded_wallet_wpkh;
         let (mut wallet1, _) = get_funded_wallet_wpkh();
         let utxo1 @ LocalOutput { outpoint, .. } = wallet1.list_unspent().next().unwrap();
         let mut builder = wallet1.build_tx();
@@ -1321,10 +1317,54 @@ mod test {
         assert_eq!(vec![fake_weighted_utxo], builder.params.utxos);
     }
 
+    // This test demonstrates that `add_utxo` only considers the final insertion.
     #[test]
-    fn not_duplicated_foreign_utxos_with_same_outpoint_but_different_weight() {
-        use crate::test_utils::{get_funded_wallet_single, get_funded_wallet_wpkh, get_test_wpkh};
+    fn test_add_utxo_final_outpoint_retained() {
+        // Create empty wallet
+        let (desc, change_desc) = get_test_wpkh_and_change_desc();
+        let mut wallet = Wallet::create(desc, change_desc)
+            .network(bdk_wallet::bitcoin::Network::Regtest)
+            .create_wallet_no_persist()
+            .unwrap();
+
+        let outpoint_0 = receive_output(
+            &mut wallet,
+            Amount::from_sat(35_000),
+            ReceiveTo::Mempool(50),
+        );
+        let outpoint_1 = receive_output(
+            &mut wallet,
+            Amount::from_sat(25_200),
+            ReceiveTo::Mempool(100),
+        );
+
+        let send_to = wallet.next_unused_address(KeychainKind::External).address;
+        let mut tx_builder = wallet.build_tx();
+        tx_builder
+            .add_utxo(outpoint_0)
+            .unwrap()
+            .add_utxo(outpoint_1)
+            .unwrap()
+            .add_utxo(outpoint_0)
+            .unwrap()
+            .add_recipient(send_to.script_pubkey(), Amount::from_sat(60_000))
+            .fee_rate(FeeRate::from_sat_per_vb(1).unwrap())
+            .ordering(crate::TxOrdering::Untouched);
+        let psbt = tx_builder.finish().unwrap();
+
+        assert_eq!(
+            psbt.unsigned_tx
+                .input
+                .iter()
+                .map(|txin| txin.previous_output)
+                .collect::<Vec<_>>(),
+            vec![outpoint_1, outpoint_0],
+            "Last outpoint added should be retained"
+        );
+    }
 
+    #[test]
+    fn not_duplicated_foreign_utxos_with_same_outpoint_but_different_weight() {
         // Use two different wallets to avoid adding local UTXOs
         let (wallet1, txid1) = get_funded_wallet_wpkh();
         let (mut wallet2, txid2) = get_funded_wallet_single(get_test_wpkh());
@@ -1380,7 +1420,6 @@ mod test {
     // Test that local outputs have precedence over utxos added via `add_foreign_utxo`
     #[test]
     fn test_local_utxos_have_precedence_over_foreign_utxos() {
-        use crate::test_utils::get_funded_wallet_wpkh;
         let (mut wallet, _) = get_funded_wallet_wpkh();
 
         let utxo = wallet.list_unspent().next().unwrap();

tests/wallet.rs

@@ -2961,3 +2961,55 @@ fn test_tx_ordering_untouched_preserves_insertion_ordering() {
     // Check vout is sorted by recipient insertion order
     assert!(txouts == vec![400, 300, 500]);
 }
+
+// BnB coin selection should find a solution using the optional UTXO.
+// This demonstrates that `calculate_cs_result` correctly orders required UTXOs before selected
+// ones.
+#[test]
+fn test_tx_ordering_untouched_preserves_insertion_ordering_bnb_success() {
+    // Create empty wallet
+    let (desc, change_desc) = get_test_wpkh_and_change_desc();
+    let mut wallet = Wallet::create(desc, change_desc)
+        .network(bdk_wallet::bitcoin::Network::Regtest)
+        .create_wallet_no_persist()
+        .unwrap();
+
+    // Set up UTXOs with specific values so BnB can find an exact match (avoiding change).
+    // - outpoint_0 (required): 35,000 sat - not enough alone
+    // - outpoint_1 (optional): 25,200 sat
+    // - Target: 60,000 sat
+    // - Expected fee: 200 sat
+
+    let outpoint_0 = receive_output(
+        &mut wallet,
+        Amount::from_sat(35_000),
+        ReceiveTo::Mempool(50),
+    );
+    let outpoint_1 = receive_output(
+        &mut wallet,
+        Amount::from_sat(25_200),
+        ReceiveTo::Mempool(100),
+    );
+
+    let send_to = wallet.next_unused_address(KeychainKind::External).address;
+    let mut tx_builder = wallet.build_tx();
+    tx_builder
+        .add_utxo(outpoint_0)
+        .unwrap()
+        .add_recipient(send_to.script_pubkey(), Amount::from_sat(60_000))
+        .fee_rate(FeeRate::from_sat_per_vb(1).unwrap())
+        .ordering(bdk_wallet::TxOrdering::Untouched);
+    let psbt = tx_builder.finish().unwrap();
+
+    // Verify that both UTXOs are selected in the correct order:
+    // required (outpoint_0) should appear before optional (outpoint_1)
+    assert_eq!(
+        psbt.unsigned_tx
+            .input
+            .iter()
+            .map(|txin| txin.previous_output)
+            .collect::<Vec<_>>(),
+        vec![outpoint_0, outpoint_1],
+        "UTXOs should be ordered with required first, then selected"
+    );
+}