Refactor/use iterators to preselect utxos

[nymius]

Description

There were multiple calls for de-duplication of selected UTxOs in Wallet::create_tx: (1) and (2).

As the test test_filter_duplicates shows, there are four possible cases for duplication of UTxOs while feeding the coin selection algorithms.

1. no duplication: out of concern
2. duplication in the required utxos only: covered by the source of required_utxos, Wallet::list_unspent, which roots back the provided UTxOs to a HashMap which should avoid any duplication by definition
3. duplication in the optional utxos only: is the only one possible as optional UTxOs are stored in a Vec and no checks are performed about the duplicity of their members.
4. duplication across the required and optional utxos: is already covered by Wallet::preselect_utxos, which avoid the processing of required UTxOs while listing the unspent available UTxOs in the wallet.

This refactor does the following:

• Refactors TxParams::utxos type to be HashSet<LocalOutput> avoiding the duplication case 3
• Keeps avoiding case 4 without overhead and allowing a query closer to O(1) on avg. to cover duplication case 4 (before was O(n) where n is the size of required utxos) thanks to the above change.
• Still covers case 2 because the source of required_utxos, Wallet::list_unspent comes from a HashMap which should avoid duplication by definition.
• Moves the computation of the WeightedUtxos to the last part of UTxO filtering, allowing the unification of the computation for local outputs.
• Removes some extra allocations done for helpers structures or intermediate results while filtering UTxOs.
• Allows for future integration of UTxO filtering methods for other utilities, e.g.: filter locked utxos

  .filter(|utxo| !self.is_utxo_locked(utxo.outpoint, self.latest_checkpoint().height()))

• Adds more comments for each filtering step.
• Differentiates foreign_utxos, which should include a provided satisfation weight to use them effectively, and utxos, manually selected UTxOs for which the wallet can compute their satisfaction weight without external resources.

With these changes all four cases would be covered, and coin_selection::filter_duplicates is no longer needed.

Fixes #1794.

---

As I was developing the changes above I discover issue #1810. So I introduced the fixes with this PR but I can split them in two to ease the review.

The preselect_utxos method (now filter_utxos) had an off-by-one error that was making the selection of optional UTxOs too restrictive, by requiring the coinbase outputs to surpass or equal coinbase maturity time at the current height of the selection, and not in the block in which the transaction may be included in the blockchain (be spent), probably, the next one.

The bug still was in filter_utxos.

The changes in commit 1b57d82 fix it by making use of the correctly defined FullTxOut<A>::is_mature method, which test positively a UTxO as mature if it is elegible for inclusion in the next mined block.

Fixes #1810.

Notes to the reviewers

I added three test to cover the interesting cases for duplication:
- there are no duplicates in required utxos
- there are no duplicates in optional utxos
- there are no duplicates across optional and required utxos
the three of them have been prefixed with not_duplicated_utxos* to allow its joint execution under the command:

cargo test -- not_duplicated_utxos

because the guarantees for the three conditions above are spread in different parts of the code.

---

Tests for issue #1810 have not been explicitly added, as there already was a text_spend_coinbase test which was corrected to ensure coinbase maturation is considered properly.

Changelog notice

No changes to public APIs.

Checklists

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing
• This pull request does not break the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[evanlinjin]

Hey @nymius, does this actually fix something or is this purely a refactor?

Note that we want to redo the tx building / creating logic to use bdk_coin_select and miniscript planning module so I think it's good policy to only do changes that are fixes or implement features that users are requesting.

[nymius]

Hey @nymius, does this actually fix something or is this purely a refactor?

Hi @evanlinjin. Yes, maybe I went to far with the changes, still, it fixes the duplicate issue I open in #1794 by using a HashSet to handle manually selected UTxOs.

Then, about the refactor:

The research of the code push me to isolate the pre selection steps as items on a checklist and that's why It ended up as a refactor.

My idea is to further isolate each filter in their own iterator adaptor (a separated function for each one) that consumes impl Iterator<Item = LocalOutputs> and returns the same type, to later also allow the introduction of custom filters in TxParams which I think can be helpful to implement checks on the ability to spend a Taproot output, for example, in Silent Payments.

The heavy use of iterators came for trying to avoid the allocation of helpers, like satisfies_confirmed.

I envisioned something like this:

let optional_utxos = self
    .list_unspent()
    .check_are_not_already_manually_selected()
    .check_are_not_unspendable()
    .check_confirmed_only_if_RBF()
    .check_is_local_utxo()
    .check_is_mature_if_coinbase();
// then
let (required, optional) = optional_utxos.chain(required_utxos.iter().clone())
    .get_weighted_utxos()
    .chain(foreign_utxos.iter().clone())
    .apply_custom_validation_for_all_tx_inputs()
    .split_utxos_in_required_and_optional()

---

Discussing this today with @ValuedMammal, I decided to do the following changes:

1. Just return optional utxos as they are the only needed values.
2. Move the calculus of WeightedUtxos to another method. Placed inside create_tx for now
3. Rename preselect_utxos to filter_utxos to correct the semantics the above point will change.
4. Implement unit test for the feature to make the PR sound.
5. Make current_height parameter not optional.

If we don't agree on the above, I propose the following alternatives:

• keep points 1 and 2 above.
• keep the old filtering implementation in place.
• keep the changes to the build_fee_bump (new foreign_utxos field in TxParams).
• drop all the other changes.

[nymius]

Rebased

[ValuedMammal]

I'm confused by this comment, is it left over from the other test?

I think this test would be better if we add a bunch of utxos (~10) to the wallet, then call filter_utxos and check that 1) none are duplicated and 2) none that we expect to be present are filtered out by accident.

[nymius]

I'm confused by this comment, is it left over from the other test?

I'm confused too, probably I changed the test after the comment.

I think this test would be better if we add a bunch of utxos (~10) to the wallet, then call filter_utxos and check that 1) none are duplicated and 2) none that we expect to be present are filtered out by accident.

What I wanted to prove with the tests I implemented is that the only needed test case for filter_utxos is when there are duplicated utxos across optional and required.
In order to do that, here, I wanted to prove is impossible for filter_chain_unspent to produce duplicated optional utxos. That's why I mistakenly tried to insert the same tx multiple times. The thing is I have no knowledge of any way to incorporate duplicated utxos in the wallet bypassing those checks, and as the list is generated from an iterator on demand, the only way I can think right now is some kind of mock for the method, as it isn't an input of filter_utxos.

[nymius]

Will remove the test for now, as I don't find a deterministic way to produce duplicated utxos which will be de duplicated later by wallet logic.
In relation to 2) I think this line should be enough to assert that.

[nymius]

Test removed in fe3c17e

[ValuedMammal]

📌 I don't think we've considered what would happen if a third party provides us with a "foreign" utxo that actually duplicates a utxo owned by the wallet, potentially causing us to sign something we didn't agree to. It might need more research so I'll open an issue. #1819

[nymius]

📌 I don't think we've considered what would happen if a third party provides us with a "foreign" utxo that actually duplicates a utxo owned by the wallet, potentially causing us to sign something we didn't agree to. It might need more research so I'll open an issue. #1819

I created a separated draft PR #1823 to address the issue.