Merge #262: Preserve insertion order of manually selected utxos if TxOrdering::Untouched

0522114 test(tx_builder): update precedence check of local UTXOs over add_foreign_utxos (valued mammal)
73bef28 doc(tx_builder): add info about manually selected UTxOs priority (nymius)
3316236 fix(tx_builder): preserve insertion order with TxOrdering::Untouched (nymius)

Pull request description:

  ### Description

  On my attempt to fix bitcoindevkit/bdk#1794 in bitcoindevkit/bdk#1798, I broke the assumption that insertion order is preserved when `TxBuilder::ordering` is `TxOrdering::Untouched`. Some users are relying in this assumption, so here I'm trying to restore it back, without adding a new dependency for this single use case like #252, or creating a new struct just to track this.

  In this fourth alternative solution I'm going back to use `Vec` to store the manually selected UTxOs.

  I was reluctant to do it in this way because `HashMap` seems a better solution giving its property of avoiding duplicates, but as we also want to keep the secuential nature of the insertion of UTxOs in `TxBuilder`, here is an alternative aligned with that principle.

  May replace #252
  May replace #261 .
  Fixes #244

  ### Notes to the reviewers

  Also, as I was working on this, I came back to the following tests:
  - `test_prexisting_foreign_utxo_have_no_precedence_over_local_utxo_with_same_outpoint`
  - `test_prexisting_local_utxo_have_precedence_over_foreign_utxo_with_same_outpoint`

  Motivated during the implementation and review of bitcoindevkit/bdk#1798.

  Which required the underlying structure to also hold the properties of no duplication for manually selected UTxOs, as the structures were accessed directly for these cases.

  The test tries to cover the case when there are two wallets using the same descriptor, one tracks transactions and the other does not. The first passes UTxOs belonging to the second one and this one creates transactions using the `add_foreign_utxo` interface.
  In this case it was expected for any `LocalUtxo` of the offline wallet to supersede any conflicting foreign UTxO. But, the simulation of this case went against the borrowing constraints of rust.
  By how costly was to reproduce this behavior for me in the tests, I would like to have second opinions in the feasibility of the test case.

  ### 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:
    reACK 0522114
  oleonardolima:
    ACK 0522114

Tree-SHA512: f2726d75eab83e28cc748ac5cd6bd0c7f3dddb409ac61baf7d0a7030ddf81c11b10dbd5b18e8ac3d29a6afb4b8f29ee9a88f83094aebec771fdb4da2cd718326

Author: oleonardolima

wallet/src/wallet/mod.rs

@@ -1511,7 +1511,7 @@ impl Wallet {
 
         let (required_utxos, optional_utxos) = {
             // NOTE: manual selection overrides unspendable
-            let mut required: Vec<WeightedUtxo> = params.utxos.values().cloned().collect();
+            let mut required: Vec<WeightedUtxo> = params.utxos.clone();
             let optional = self.filter_utxos(&params, current_height.to_consensus_u32());
 
             // if drain_wallet is true, all UTxOs are required
@@ -1740,52 +1740,45 @@ impl Wallet {
                     })
                     .map(|(prev_tx, prev_txout, chain_position)| {
                         match txout_index.index_of_spk(prev_txout.script_pubkey.clone()) {
-                            Some(&(keychain, derivation_index)) => (
-                                txin.previous_output,
-                                WeightedUtxo {
-                                    satisfaction_weight: self
-                                        .public_descriptor(keychain)
-                                        .max_weight_to_satisfy()
-                                        .unwrap(),
-                                    utxo: Utxo::Local(LocalOutput {
-                                        outpoint: txin.previous_output,
-                                        txout: prev_txout.clone(),
-                                        keychain,
-                                        is_spent: true,
-                                        derivation_index,
-                                        chain_position,
-                                    }),
-                                },
-                            ),
+                            Some(&(keychain, derivation_index)) => WeightedUtxo {
+                                satisfaction_weight: self
+                                    .public_descriptor(keychain)
+                                    .max_weight_to_satisfy()
+                                    .unwrap(),
+                                utxo: Utxo::Local(LocalOutput {
+                                    outpoint: txin.previous_output,
+                                    txout: prev_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(),
                                 );
-
-                                (
-                                    txin.previous_output,
-                                    WeightedUtxo {
-                                        utxo: Utxo::Foreign {
-                                            outpoint: txin.previous_output,
-                                            sequence: txin.sequence,
-                                            psbt_input: Box::new(psbt::Input {
-                                                witness_utxo: prev_txout
-                                                    .script_pubkey
-                                                    .witness_version()
-                                                    .map(|_| prev_txout.clone()),
-                                                non_witness_utxo: Some(prev_tx.as_ref().clone()),
-                                                ..Default::default()
-                                            }),
-                                        },
-                                        satisfaction_weight,
+                                WeightedUtxo {
+                                    utxo: Utxo::Foreign {
+                                        outpoint: txin.previous_output,
+                                        sequence: txin.sequence,
+                                        psbt_input: Box::new(psbt::Input {
+                                            witness_utxo: prev_txout
+                                                .script_pubkey
+                                                .witness_version()
+                                                .map(|_| prev_txout.clone()),
+                                            non_witness_utxo: Some(prev_tx.as_ref().clone()),
+                                            ..Default::default()
+                                        }),
                                     },
-                                )
+                                    satisfaction_weight,
+                                }
                             }
                         }
                     })
             })
-            .collect::<Result<HashMap<OutPoint, WeightedUtxo>, BuildFeeBumpError>>()?;
+            .collect::<Result<Vec<WeightedUtxo>, BuildFeeBumpError>>()?;
 
         if tx.output.len() > 1 {
             let mut change_index = None;
@@ -2099,6 +2092,11 @@ impl Wallet {
             vec![]
         // only process optional UTxOs if manually_selected_only is false
         } else {
+            let manually_selected_outpoints = params
+                .utxos
+                .iter()
+                .map(|wutxo| wutxo.utxo.outpoint())
+                .collect::<HashSet<OutPoint>>();
             self.indexed_graph
                 .graph()
                 // get all unspent UTxOs from wallet
@@ -2116,9 +2114,10 @@ impl Wallet {
                         .is_mature(current_height)
                         .then(|| new_local_utxo(k, i, full_txo))
                 })
-                // 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_key(&may_spend.outpoint))
+                // 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| !manually_selected_outpoints.contains(&may_spend.outpoint))
                 // only add to optional UTxOs those which satisfy the change policy if we reuse
                 // change
                 .filter(|local_output| {
@@ -2745,18 +2744,10 @@ mod test {
         let txid = two_output_tx.compute_txid();
         insert_tx(&mut wallet, two_output_tx);
 
-        let mut params = TxParams::default();
-        let output = wallet.get_utxo(OutPoint { txid, vout: 0 }).unwrap();
-        params.utxos.insert(
-            output.outpoint,
-            WeightedUtxo {
-                satisfaction_weight: wallet
-                    .public_descriptor(output.keychain)
-                    .max_weight_to_satisfy()
-                    .unwrap(),
-                utxo: Utxo::Local(output),
-            },
-        );
+        let outpoint = OutPoint { txid, vout: 0 };
+        let mut builder = wallet.build_tx();
+        builder.add_utxo(outpoint).expect("should add local utxo");
+        let params = builder.params.clone();
         // enforce selection of first output in transaction
         let received = wallet.filter_utxos(&params, wallet.latest_checkpoint().block_id().height);
         // notice expected doesn't include the first output from two_output_tx as it should be