Implement utxo reservation

[vladimirfomene]

Description

This PR solves issue #849. The current design keeps a set of UTXOs that have been reserved by transactions. UTXOs are released when a transaction is canceled. I'm currently thinking it might also make sense to release UTXOs once a transaction gets broadcasted (this idea is not implemented in this PR).

Notes to the reviewers

With the new ChainOracle design, it no longer makes sense to have this as part of the keychain tracker. It might make sense to have this inside the bdk wallet.

Changelog notice

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature

Bugfixes:

• This pull request breaks 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

[LLFourn]

With the new ChainOracle design, it no longer makes sense to have this as part of the keychain tracker. It might make sense to have this inside the bdk wallet.

Concept ACK. I also suggested that in response to #849. Can you change the PR to only modify bdk::Wallet?

[vladimirfomene]

Thanks @LLFourn for the feedback! Let me work on that update.

[danielabrozzoni]

Concept ACK, thanks for working on this! I think I'll do something similar to what you did with include_reserved_utxos in the new TxBuilder.
I left some comments regarding the Wallet API - I'm worried that by exposing include_reserved_utxos frequently we might make the API harder to use, without a real benefit.

Also, should we have some method on Wallet to query and modify the reserved utxos?

[danielabrozzoni]

I don't think we should have a include_reserved_utxos parameter here... If I want bdk to retrieve a specific outpoint, I usually don't care if it's reserved or not.

I would just call full_utxos(true) a couple of lines below and that's it

[vladimirfomene]

Makes sense.

[danielabrozzoni]

Similarly to above, when users ask for a balance I suppose they always want the reserved utxos anyways

[danielabrozzoni]

Same here, if you want to manually add utxos you probably don't care whether they're unlocked or not.

Unless... we might want to return a specific error (not UnknownUtxo, a more detailed one, like LockedUtxo) if you try to add a locked utxo to a txbuilder that doesn't have include_reserved_utxos. I'm not sure exactly what's the way forward here.

[vladimirfomene]

I will prefer creating an error like LockedUtxo. Make more sense to me.

[vladimirfomene]

@danielabrozzoni, what do you think the potential use cases will be for adding an API for querying and modifying the locked utxos?

[danielabrozzoni]

Think about a scenario where there are multiple bdk wallets able to communicate between each other, each one of them crafting transactions and not broadcasting them immediately. In this case you don't want to have conflicting txs and you want to manually tell each wallet "also lock this specific utxo". But it's a rather exotic scenario so we don't have to implement it right now.

[benthecarman]

what do you think the potential use cases will be for adding an API for querying and modifying the locked utxos?

I could see this useful for things you queue up. I could see using this when a user queues some utxos for a coinjoin, until that coinjoin happens those utxos should be locked, so if they go to send those coins should be locked in the coin selection screen. Being able to see which utxos are locked would be important for that.

[DanGould]

This would be nice in the case a payjoin fails because someone goes offline and the receiver bans their utxo (to prevent probing), the signing algorithm could automatically pick a utxo outside of the reserved set.

[DanGould]

I'm curious what the status of this PR is. Tracking and expiring intermediate but unconfirmed payjoin states (pending original psbt vs augmented payjoin psbt) requires locking. Otherwise subsequent sign attempts spend outputs of pending payjoin state that may never end up confirmed on chain.

@vladimirfomene are you still invested in seeing this PR completed?
@LLFourn how much bandwidth does the BDK team have to review this PR considering the 1.0 stabilization?

Would either of you like a hand or consider my contributions to get this reservation issue fixed?

[LLFourn]

@DanGould reconsidering the problem here. Is there something wrong with just using: add_unspendable when you don't want to spend a txout because you know at the application level which transactions you intend on broadcasting and which you don't.

Also you could consider just adding the transaction to the wallet with insert_tx which will also remove the txs from being spendable while also adding any utxos on that new tx to the pool but I'm guessing you don't want to do that here.

[DanGould]

Edit: I believe I've been able to fix this problem by having payjoin only contribute confirmed inputs. This is a slight degradation vs being able spend unconfirmed-but-seen-in-mempool transactions but it works without reservation for now.

Please bear with me as I outline in detail what I'm trying to do and where I've run into the snag. I'm trying to watch unconfirmed, pending transactions that one of the two payjoin parties have signed but which may never be broadcast, and are out of the one party's control to broadcast. I am running BDK 1.0.0-alpha.5.

Payjoin Sender

The payjoin sender first signs an Original PSBT that's a typical transaction sending funds to the receiver. It wants to add this transaction to its wallet because, even though it never plans on broadcasting this TX, if for some reason the payjoin session times out, the receiver may extract and broadcast this original fallback transaction, making the sender's original tx change spendable. If the sender receives a payjoin PSBT with a signed receiver input, it can sign and broadcast the payjoin and remove the Original tx from its wallet because those funds have already been spent in the payjoin.

Payjoin Receiver

The payjoin receiver augments an incoming original PSBT with its input to create a payjoin PSBT missing the sender's signature, since the sender's original PSBT sighash_all signature becomes invalid when the transaction is augmented. The receiver adds this payjoin transaction to its wallet, sends it to the sender to receive a signature, and waits to see it broadcast. If that transaction never gets signed, the receiver may choose to broadcast the original PSBT tx if it's an automated payment processor or otherwise let the payjoin session expire. During the time this payjoin PSBT in the hands of the sender in the wild and not considered expired nor seen in mempool, the receiver shouldn't try to create more transactions spending the same input as the payjoin PSBT nor spend change outputs from the payjoin PSBT since they might never be confirmed. I would understand these to be 'reserved' utxos.

---

I wasn't considering add_unspendable, but I suppose that could work if I can add pending wallet UTXOs as unspendable and later remove them once a conflicting broadcast is detected. Will this still produce a pending transaction when I call wallet.transactions()?. because that's a TxBuilder method and I'm looking for a way to track pending transactions that may later become invalid. Building a transaction is not a problem. The external UTXO is added to a psbt manually, not via TxBuilder.

`list_transactions` implementation

pub fn list_transactions(
    &self,
    include_raw: bool,
) -> Result<Vec<TransactionDetails>, MutinyError> {
    if let Ok(wallet) = self.wallet.try_read() {
        let txs = wallet
            .transactions()
            .filter_map(|tx| {
                // skip txs that were not relevant to our bdk wallet
                if wallet.spk_index().is_tx_relevant(tx.tx_node.tx) {
                    let (sent, received) = wallet.spk_index().sent_and_received(tx.tx_node.tx);

                    let transaction = if include_raw {
                        Some(tx.tx_node.tx.clone())
                    } else {
                        None
                    };

                    // todo bdk is making an easy function for this
                    // calculate fee if possible
                    let inputs = tx
                        .tx_node
                        .tx
                        .input
                        .iter()
                        .map(|txin| {
                            wallet
                                .spk_index()
                                .txout(txin.previous_output)
                                .map(|(_, _, txout)| txout.value)
                        })
                        .sum::<Option<u64>>();
                    let outputs = tx.tx_node.tx.output.iter().map(|txout| txout.value).sum();
                    let fee = inputs.map(|inputs| inputs.saturating_sub(outputs));

                    Some(TransactionDetails {
                        transaction,
                        txid: tx.tx_node.txid,
                        received,
                        sent,
                        fee,
                        confirmation_time: tx.chain_position.cloned().into(),
                        labels: vec![],
                    })
                } else {
                    None
                }
            })
            .collect();
        return Ok(txs);
    }
    log_error!(
        self.logger,
        "Could not get wallet lock to list transactions"
    );
    Err(MutinyError::WalletOperationFailed)
}

My current attempt (one commit in a PR) has the sender insert_tx an Original transaction it sends to the receiver. The sender then calls cancel_tx on the original tx if it receives, signs, and broadcasts an updated payjoin.

Similarly, the receiver also calls insert_tx on the payjoin before it sends its payjoin psbt it to the sender to await the payjoin broadcast from the sender.

However, I'm running into a problem where subsequent attempts at receiving payjoin try to spend the output of the prior payjoin tx in a new payjoin. That prior payjoin change output has never been broadcast, so the sender rejects it because it does not exist from the sender perspective. My understanding is that if the payjoin tx input and output was marked as unspendable or "reserved," then it would NOT appear as a LocalOutput from list_unspent and chosen for spending in subsequent transactions.

(here's the original referenced commit in a branch separate from the PR in case that PR is updated)

[LLFourn]

@DanGould thanks for the context.

1. Inserting transactions before they are broadcast: I actually thought this would be fine but it looks like we've got bug that means that if you insert a transaction before broadcasting it, it will still show up as "unconfirmed" and therefore utxos from it will show up in your utxo list (and utxos it spends will be gone). Note cancel_tx won't help here. I made an issue here: [chain] last_seen_unconfirmed should have a default of None not 0 #1396. So for now you should never insert transactions before they are broadcast.
2. How to "reserve" utxos: Even if you are not using TxBuilder you can still manually .filter the list of utxos right? I'm not sure what value we'd provide by having an API for doing this more behind the scenes. It seems to me to be better if the application is explicitly in control of enforcing this.

The feature this PR is aimed at providing was more about stopping multiple threads from creating transactions that spend the same utxos. The plan wasn't to persist this list of reserved utxos (just have them in memory). It was just there to "lock" the utxo in the short time between a transaction being created and it being confirmed to broadcast. This might be useful to you too though. I think the thing to keep in mind is that we don't want to persist these lists of reserved utxos if you want that you'll have to do it at the application level.

I'd hesitate to encourage you to take on this PR as I there could be quite some bikesheading around what to do with the balance, when to unreserve utxos etc. Only take it on if you have significant time to dedicate to figuring out the right answers to these (and other) questions.