fix(coin_selection): calculate_cs_result returns the required UTXOs first

[ValuedMammal]

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:

• I followed the contribution guidelines

Bugfixes:

• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.04%. Comparing base (826b19f) to head (8a5e763).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #390   +/-   ##
=======================================
  Coverage   80.04%   80.04%           
=======================================
  Files          24       24           
  Lines        5336     5337    +1     
  Branches      242      242           
=======================================
+ Hits         4271     4272    +1     
  Misses        987      987           
  Partials       78       78           

Flag	Coverage Δ
rust	80.04% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Serious-Sam-Dev]

all test and clippy passed, i dont see any problem, ACK

[110CodingP]

ACK 378cb24

Though unrelated to the PR I was wondering whether we should also have a test to check the following invariant in add_utxos ?
"If a UTXO is inserted multiple times, only the final insertion will take effect."

[ValuedMammal]

Thank you @110CodingP

"If a UTXO is inserted multiple times, only the final insertion will take effect."

We do have these tests, or are you referring to something else?

• duplicated_utxos_in_add_utxos_are_only_added_once
• not_duplicated_utxos_in_required_list

[110CodingP]

Sorry for being brief, I meant something like this:

// This demonstrates that `add_utxo` only considers the final insertion.
#[test]
fn test_add_utxos_final_op_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(bdk_wallet::TxOrdering::Untouched);
    let psbt = tx_builder.finish().unwrap();

    // Fails
    assert_eq!(
        psbt.unsigned_tx
            .input
            .iter()
            .map(|txin| txin.previous_output)
            .collect::<Vec<_>>(),
        vec![outpoint_0, outpoint_1]
    );
}

[Musab1258]

tACK 96c62d5

Although I have a question or a sort of confirmation regarding why you used .extend() instead of the previously used .append().

Since selected_utxos will be dropped either way at the end of the function, did you use .extend() to immediately consume selected_utxos instead of leaving an empty vector till the end of the function?

[ValuedMammal]

why you used .extend() instead of the previously used .append().

It seemed to match the style and ergonomics of the rest of the code base. I think either one is probably fine? Found a related discussion here rust-lang/rust-clippy#4321 (comment) and here https://users.rust-lang.org/t/pearl-extending-a-vec-via-append-or-extend/73456