Fix/tx build should not change order of insertion with vector

[nymius]

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

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo +nightly fmt and cargo clippy before committing
• I've added tests for the new code
• I've expanded docs addressing new code
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[coveralls]

Pull Request Test Coverage Report for Build 15566821414

Details

• 138 of 153 (90.2%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.03%) to 85.583%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
wallet/src/wallet/tx_builder.rs	107	108	99.07%
wallet/src/wallet/mod.rs	31	45	68.89%

Totals
Change from base Build 15476130196:	0.03%
Covered Lines:	7379
Relevant Lines:	8622

---

💛 - Coveralls

[ValuedMammal]

Instead of removing elements can we just not push onto params.utxos if it duplicates an already added outpoint? This seems to be more aligned with the original behavior of filter_duplicates where the first item is kept and any duplicates that come later are ignored.

pub fn add_utxos(&mut self, outpoints: &[OutPoint]) -> Result<&mut Self, AddUtxoError> {
    // We want to canonicalize once, instead of once for every call to `get_utxo`.
    let unspent: HashMap<OutPoint, LocalOutput> = self
        .wallet
        .list_unspent()
        .map(|output| (output.outpoint, output))
        .collect();

    // Ensure that only unique outpoints are added.
    let mut visited = HashSet::new();

    let utxos: Vec<WeightedUtxo> = outpoints
        .iter()
        .filter(|&&op| {
            visited.insert(op)
                && !self
                    .params
                    .utxos
                    .iter()
                    .any(|wutxo| wutxo.utxo.outpoint() == op)
        })
        .map(|&op| -> Result<_, AddUtxoError> {
            let output = unspent
                .get(&op)
                .cloned()
                .ok_or(AddUtxoError::UnknownUtxo(op))?;
            Ok(WeightedUtxo {
                satisfaction_weight: self
                    .wallet
                    .public_descriptor(output.keychain)
                    .max_weight_to_satisfy()
                    .expect("descriptor should be satisfiable"),
                utxo: Utxo::Local(output),
            })
        })
        .collect::<Result<_, _>>()?;

    self.params.utxos.extend(utxos);

    Ok(self)
}