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.

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.

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 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. bitcoindevkit/bdk_wallet#29

[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. bitcoindevkit/bdk_wallet#29

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

[nymius]

review club notes 02/06/2025

1. Include psbt::Input::non_witness_utxo field together with psbt::Input::non_witness_utxo when txout.script_pubkey.witness_version().is_some() while re constructing foreign UTxOs in Wallet::build_fee_bump.
2. The relevance of the integration of miniscript plan (feat(wallet)!: use miniscript plan in place of old policy mod #1786 ) for this PR was mentioned. Wallet::get_available_utxos obtained the satisfaction weight of UTxOs before passing them to preselect_utxos. As satisfaction weight is dependent of planning, there should be future work to integrate this, as it is a previous step to Wallet::filter_utxos.
3. The split of the PR was suggested, in two different ones to allow a faster review and merge of the fix for Off-by-one error is making optional UTxOs selection too restrictive for coinbase outputs #1810.
4. As commits addressing suggested changes had been acknowledge by reviewers, the next step should be to squash them in four or less commits:
   1. Refactor
   2. coin_selection::filter_duplicates removal.
   3. Tests
   4. Miscellaneous not related to refactor
5. As part of the review we talked about [wallet] Document about extra validation when signing transactions including foreign utxos bdk_wallet#29 and its fix candidate Check foreign utxos are not local before inclusion #1823 :
   1. As Check foreign utxos are not local before inclusion #1823 introduces breaking changes, a compromise solution would be to add a silent check which works as a no-op in case transaction is found to be owned by the wallet. Also, one possible approach is to delay the introduction of the new NotForeignUtxo variant error (breaking change) until the release of the new major version, 2.0.0 with a TODO comment.
   2. It wasn't clear the ownership of the transactions stored in TxGraph. The terms canonical and relevant were mentioned as a good source to get an idea of the topic from the documentation.
      Wallet::transactions has a good explanation of this.
      TxGraph's transactions don't have to belong to the wallet necessarily, and get_txout shouldn't be considered a good source of truth to establish if an Outpoint can be unlocked by Wallet or not. Wallet::is_mine is the correct method for this.

Thanks to @ValuedMammal, @oleonardolima, @LagginTimes and @evanlinjin for participating!

[nymius]

Next steps based on review club:

• Include psbt::Input::non_witness_utxo field together with psbt::Input::non_witness_utxo when txout.script_pubkey.witness_version().is_some() while re constructing foreign UTxOs in Wallet::build_fee_bump.
• Squash commits.
• Create separated PR to fix Off-by-one error is making optional UTxOs selection too restrictive for coinbase outputs #1810

[nymius]

905c8dd will be squashed to b1bff02 once #1830 is merged

[nymius]

Now that foreign UTxOs are separated from manually selected UTxOs there are new cases for duplicity:

1. Duplicated UTxOs inside foreign_utxos field.
2. Duplicated UTxOs across foreign_utxos and required/optional UTxOs. Shouldn't be a problem once Check foreign utxos are not local before inclusion #1823 is merged. Because of the new check which avoids the addition of local UTxOs for Wallet as foreign UTxOs.

TODO:

• Ensure foreign_utxos does not contain duplicated UTxOs: TxParams::foreign_utxos transformed into HashSet in 7cb0243 and added test to confirm property. Will be squashed to b1bff02 once change is ACKnowledge. The avoidance of duplicity across foreign and optional/required UTxOs will be guaranteed by changes in Check foreign utxos are not local before inclusion #1823

[ValuedMammal]

I'm worried that this won't be sufficient to check that the outpoint is unique. For instance, one could change the satisfaction weight and get a different WeightedUtxo while using the same outpoint.

[nymius]

Good point, will think more about this. Will reproduce your case in a test and use a HashMap with outpoint as key in the meantime.

[nymius]

I modified the test slightly to cover that case. I didn't add a different one because it is not modifying the logic being tested, but just being more precise with the assertions.

I used HashMap instead of BTreeMap because we don't need the ordering features, however it can be argued that BTreeMap is already use it in crates/wallet/src/wallet/tx_builder.rs and we are adding a new import.
I prefered the former over the later.

[nymius]

To avoid the duplication concern we had for #1582, we can implement Hash manually for WeightedUTxO and make it use the Hash implementation of Outpoint.

[nymius]

I realized this won't be enough, as HashSet requires the following for keys k1, k2:
$k1 == k2 \Rightarrow \text{hash}(k1) == \text{hash}(k2)$.
But this should enforced by us. If we implement Hash as I said above and don't also modify PartialEq implementation to be compatible with this, the case where outpoint is the same but weight is different will introduce two different utxos in foreign utxos set.

I wouldn't modify PartialEq because is probably going to break other logic depending of the usual way of comparing two elements (i.e. considering all its fields).

HashMap is still the best solution so far.

[nymius]

905c8dd will be squashed to b1bff02 once #1830 is merged

Squash done and rebased on master.

[nymius]

Thanks @ValuedMammal! Updated and rebased

[nymius]

Addressed comments and rebased onto master.

[evanlinjin]

Documentation suggestions. Otherwise, LGTM

[evanlinjin]

Ideally, we would error here if an OutPoint conflicts with a foreign UTXO. However, we will need to add new error variants (which is a MAJOR change since the error type does not have non-exhaustive).

For now, how about we make a note in the docs that if you add the same UTXO as both foreign and local, the latest change has precedence (please word that better).

Same goes for the add-foreign-utxo methods.

[evanlinjin]

For the add-foreign-utxo methods, ideally we will error out if it conflicts with pre-existing in TxParams::utxos or utxos in TxBuilder::wallet. However, the error variant is not non-exhaustive so that will be breaking.

For now, how about we just note that it would be unexpected behavior in the docs?

[evanlinjin]

@nymius had a better idea of making local UTXOs have precedence over foreign UTXOs. Let's do this and also update the documentation.

[nymius]

I've added the documentation requested and two tests to address the discussed cases. #1823 will fix the issue against tracked local UTxOs (the one the wallet already has knowledge about, aka optional).

[nymius]

Rebased onto b26ff89

[nymius]

Rebased onto 362c3dc

[evanlinjin]

ACK 2f83b45

Just a nit for future reference.

[evanlinjin]

Nit: trying to make this section fully functional has made it harder to read. There is weird map statements, and it becomes hard to see whether we are map-ing on an iterator or option.

I would prefer the following:

                let prev_tx = graph
                    .get_tx(txin.previous_output.txid)
                    .ok_or(BuildFeeBumpError::UnknownUtxo(txin.previous_output))?;
                let chain_position = chain_positions
                    .get(&txin.previous_output.txid)
                    .cloned()
                    .ok_or(BuildFeeBumpError::UnknownUtxo(txin.previous_output))?;
                let txout = prev_tx.output[txin.previous_output.vout as usize].clone();
                let (outpoint, weighted_utxo) = match txout_index
                    .index_of_spk(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: 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: txout
                                            .script_pubkey
                                            .witness_version()
                                            .map(|_| txout.clone()),
                                        non_witness_utxo: Some(prev_tx.as_ref().clone()),
                                        ..Default::default()
                                    }),
                                },
                                satisfaction_weight,
                            },
                        )
                    }
                };
                Ok((outpoint, weighted_utxo))

[nymius]

I agree, will try to control the bias towards functional style.