Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let TxBuilder avoid previously used UTXOs (UTXO locking) #849

Open
tnull opened this issue Jan 31, 2023 · 8 comments
Open

Let TxBuilder avoid previously used UTXOs (UTXO locking) #849

tnull opened this issue Jan 31, 2023 · 8 comments
Labels
api A breaking API change module-wallet new feature New feature or request
Milestone

Comments

@tnull
Copy link
Contributor

tnull commented Jan 31, 2023

Describe the enhancement
When sequentially creating multiple transactions, TxBuilder might currently reuse the same UTXOs, potentially exposing us to double-spend ourselves if we want to bring all of these generated transactions on-chain. It would be nice if the TxBuilder could track the used UTXOs internally, and allow to auto-unlock them after a set amount of time.

Use case
This is important for sequentially opening multiple Lightning channels, e.g., in LDK Node.

@notmandatory
Copy link
Member

We had some offline discussions about how to approach this issue, recapping here:

Step 1: create example of how to do it outside of bdk such as by manually keeping a set of used but not broadcast utxos, and then feeding that set to the TxBuilder to exclude the UTXOs when creating new transactions.

Step 2: based on experience with 1, create API changes for the Wallet (or TxBuilder?) that will make it possible to automatically manage the set of used but not broadcast UTXOs.

@LLFourn LLFourn mentioned this issue Feb 15, 2023
9 tasks
@LLFourn
Copy link
Contributor

LLFourn commented Feb 15, 2023

I think we want to support this in bdk_chain and bdk::Wallet explicitly. See cancel_tx in v1.0.0-alpha.0 PR:

pub fn cancel_tx(&mut self, tx: &Transaction) {

The idea is when a tx is cancelled in the future we can mark those utxos as no longer used and return them to the "utxo" pool.

See also the necessary feature in bdk_chain: LLFourn/bdk_core_staging#186

There are other cases where you want to reserve utxos that will have to be done via a different method (manual marking) which we can also provide.

@notmandatory notmandatory added this to the Alpha 1.0.0 milestone Mar 2, 2023
@notmandatory notmandatory removed this from the 1.0.0-alpha.0 milestone Mar 3, 2023
@evanlinjin
Copy link
Member

evanlinjin commented Mar 17, 2023

Currently, the proposed solution to this is (for BDK 1.0):

KeychainTracker should have mark_spent and unmark_spent that work in a similar way to mark_used and unmark_used in KeychainTxOutIndex. The reason we want this is to be able to "reserve" utxos so other threads cannot use them when we've decide to construct a transaction using them.

However, as we are replacing the KeychainTracker with IndexedTxGraph (as specified in #895), these methods should be added to IndexedTxGraph.

@evanlinjin evanlinjin added this to the 1.0.0 milestone Mar 17, 2023
@LLFourn
Copy link
Contributor

LLFourn commented Mar 20, 2023

It's a little bit harder to say that IndexedTxGraph should have this method. Perhaps this should just exist in bdk::Wallet for now by storing a HashSet so you can reserve things.

@notmandatory notmandatory removed this from the 1.0.0 milestone May 4, 2023
@notmandatory
Copy link
Member

See #913

@ValuedMammal
Copy link
Contributor

Proposal for UTXO locking feature

approach: to be implemented in the wallet module

  1. Add a coins map to the Wallet structure where the key is an OutPoint and the value is an optional expiration Height of the temporary freeze. This type can be extended in the future to include more complete information about a LocalOutput, or a general coin profile, etc.
pub struct Wallet {
    // map of outpoint to expiration height
    utxo_locks: HashMap<OutPoint, Option<Height>>,
    ...
}
  1. Add methods on Wallet to load/reload coins and to lock a UTXO
/// Populates the the coins map. By default all utxos are unlocked.
pub fn reload_coins(&mut self) {
    // for utxo in self.unspent()
        // self.utxo_locks.insert(outpoint, None)
}

/// Locks the UTXO at `outpoint` for `n` number of blocks.
pub fn lock_utxo(&mut self, outpoint: OutPoint, n: blocks) { ... }
  1. Filter locked UTXOs during get_available_utxos. The function should check if each UTXO is locked given the last known chain tip when getting the available coins for coin selection. A lock will automatically expire after syncing the wallet and advancing the local chain past the expiration height.
fn get_available_utxos(&self) -> Vec<(LocalOutput, Weight)> {
    self.list_unspent()
        .filter(|utxo| !self.is_utxo_locked(utxo.outpoint, self.latest_checkpoint().height()))
        .map(|utxo| { ... } )
        .collect()
}

@storopoli
Copy link
Contributor

pub struct Wallet {
    // map of outpoint to expiration height
    utxo_locks: HashMap<OutPoint, Option<Height>>,
    ...
}

This should be a BTreeMap since we will generally be searching through it. It has $O( \log(n))$ complexity for the worst case scenario since it keeps the keys sorted.
Whereas HashMap needs to sort the keys first in any search, hence complexity $O(n \log (n))$

@notmandatory
Copy link
Member

since this is a new feature and shouldn't break existing API I'm assigning it to the 1.1 milestone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A breaking API change module-wallet new feature New feature or request
Projects
Status: In Progress
Status: In Progress
Development

No branches or pull requests

6 participants