bitcoindevkit / bdk

A modern, lightweight, descriptor-based wallet library written in Rust!
Other
860 stars 307 forks source link

BDK Descriptor Improvements: Add multi descriptor capability in wallet #486

Open rajarshimaitra opened 2 years ago

rajarshimaitra commented 2 years ago

Description

In order for BDK to extend beyond just a Bitcoin wallet, it needs a more powerful descriptor capability. This project aims at improving the descriptors used in the library. Currently BDK uses only a single descriptor for a single wallet. Giving a new descriptor to BDK will create a brand new wallet. The goal of this project is to create a wallet that can handle multiple descriptors within itself (a "super wallet"), and can send funds from any or all of them together. **Expected Outcomes** - Through understanding of [descriptors](https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md). - Basic understanding of the [rust-miniscript](https://github.com/rust-bitcoin/rust-miniscript) library. - Multi descriptor capability in BDK. **Resources** **Skills Required**

Mentor(s)

Difficulty: Hard

Competency Test (optional)

afilini commented 2 years ago

Note that the main goal for this task is not really to add more descriptors to the Wallet structure, but mainly to find a way to group multiple wallets into a "super wallet" that can operate on all of them at the same time, for instance by creating a transaction that spends utxos from all of them

rajarshimaitra commented 2 years ago

Noted. Paraphrased the above in the description. Let me know if it can be elaborated more..

LLFourn commented 2 years ago

Note that the main goal for this task is not really to add more descriptors to the Wallet structure, but mainly to find a way to group multiple wallets into a "super wallet" that can operate on all of them at the same time, for instance by creating a transaction that spends utxos from all of them

I think the simplest solution is create a API that allows you to create a PSBT using the databases of multiple wallets. Could even be a TxBuilder method add_utxos_from_wallet(wallet). Internally this could just take list_utxos do a weak version of add_foreign_utxo where the UTXO would be a non-mandatory spend. This would give you all the functionality that isn't available in BDK at the moment.

Of course this leaves you to manually add the balances of the individual wallets to find the joint balance etc. But more sophisticated things can be done later. In my mind wallets already have two descriptors with different semantics. I don't see why they couldn't have three or four or twenty four all in a Vec.

rajarshimaitra commented 2 years ago

I think once we converge on an approach, the description can be elaborated further. I feel this will be a little difficult project for the students and they would need as much context as possible to tackle this issue.

Thanks for all the inputs @LLFourn @afilini , I will keep following this thread and update the project template accordingly.

afilini commented 2 years ago

In my mind wallets already have two descriptors with different semantics. I don't see why they couldn't have three or four or twenty four all in a Vec.

Yes, what worries me is how complex it can get to define all the different semantics for n descriptors inside a wallet. If this "super-wallet" thing turns out well we could also consider changing the wallets to only have one descriptor (which would simplify a ton of stuff) and then handling the semantics outside (for instance the super-wallet create_tx knows that it needs to generate the change address from one specific sub-wallet that is designated as the "internal" one).

evanlinjin commented 2 years ago

I would be interested in having a go at this issue.

notmandatory commented 1 year ago

Per our discussion from bitcoindevkit/.github#51 the new bdk module now has a Wallet using the new KeychainTracker which can track keychains for multiple descriptors; it currently only uses two: External and optional Internal. My initial thought was to keep this as is to not complicate the bdk::Wallet API. But with prompting from @rajarshimaitra I've taken a deeper look and think supporting multiple descriptors in Wallet can simplify the wallet internals and improve the user facing API. The biggest down side I see is this is a fair bit of work, but if we're going to make major API changes now is the time.

Below is my high-level take on what would need to be done, what am I missing? Any reasons not to make this change?

Wallet

Becomes a collection of descriptor signers with corresponding keychain_tracker, persist, network, secp. Provides typical wallet functions across multiple descriptors for getting addresses, balances, listing transactions and utxos, creating new transactions via TxBuilder, and signing and finalizing PSBTs.

  1. add a new K type param with traits needed by KeychainTracker's K type
  2. remove change_signers field, only have signers
  3. modify new to take a Vec of tuple (K, E: IntoWalletDescriptor)
  4. modify address functions to use a K param to specify which keychain to use see #898
  5. modify balances, listing transactions and utxos functions to use optional Vec of K param for filtered results
  6. update examples for a simple KeychainKind.External and KeychainKind.Internal descriptor Wallet
  7. add examples for a more than two descriptor Wallet

TxBuilder

Create spending and fee bump transactions for a multi descriptor Wallet.

  1. add required Vec of K keychains allowed to spend UTXOs from, with optional policy paths
  2. add optional Vec of K keychains not allowed to spend UTXOs from
  3. add optional K keychain to use for change output
  4. remove change_policy, it can be represented with above
notmandatory commented 1 year ago

Per my chat with @evanlinjin I've done some more digging into how the Core wallet works, in particular regarding privacy and multiple descriptors, see https://github.com/bitcoindevkit/bdk/issues/918#issuecomment-1491221046. I also want to incorporate @rajarshimaitra suggestion for keeping simplified wallet functions for users who only have external + (optional) internal descriptors. My original ideas above will need to be updated.

notmandatory commented 1 year ago

From the above research plus goal of making Wallet easier to use for the basic two descriptor wallet scenario I propose a few modifications to above:

Wallet

  1. add a DefaultKeychainKind trait requirement on K to set a keychain kind (External or Internal) and is_default bool
  2. add address functions that take a script type and uses first default external keychain for that script type if one exists, if more than one take the first?
  3. add function to list all the wallet keychains

TxBuilder

  1. make the spend keychains optional, if none given use the default external and/or internal keychains of appropriate type (ie. avoid mixing different utxo output types), should probably be an error if none found
  2. don't remove change_policy since it would still be needed to decide which kind of keychains to get utxos from
  3. If no keychain specified for change use default internal keychain, if none then use first default external keychain, in either case use the appropriate type (ie. try to match tx change type output type to improve privacy)

Examples

The basic example should be setup similar to what Core does for a new single key descriptor wallet:

  1. 4 external default single key descriptors, one for each script type, pkh(), sh(wpkh()), tr() and wpkh()
  2. 4 internal default single descriptors, one for each script type
  3. get new deposit addresses from default external descriptor for requested script type
  4. get balance, list transactions and utxos for all wallet descriptors
  5. use appropriate internal descriptor for change, depending on the script type of the transaction output

An advanced example would be similar to above but also include using non-default descriptors. A scenario I'm thinking of is migrating utxos from an old wallet to a new wallet. We can do this by adding the old wallet descriptors as non-default descriptors to our new wallet. Over time old wallet utxos will be spent, but wallet continues to monitor old chains for any received utxos to old descriptors and spend them when needed with change back to new descriptors:

  1. 4 external default descriptors for new wallet, one for each script type
  2. 4 internal default descriptors for new wallet, one for each script type
  3. 4 external non-default descriptors for old wallet, one for each script type
  4. 4 internal non-default descriptors for old wallet, one for each script type
  5. get new deposit addresses only from new wallet external descriptors
  6. get balance, list transactions and utxos from old and new wallet descriptors
  7. spend utxos from all wallet keychains
  8. use only appropriate new wallet default internal descriptor for change, depending on the script type of the transaction output

EDIT: changed above examples from 3+3 to 4+4 descriptors per @vladimirfomene comment below.

LLFourn commented 1 year ago

From the above research plus goal of making Wallet easier to use for the basic two descriptor wallet scenario I propose a few modifications to above:

Wallet

1. add a `DefaultKeychainKind` trait requirement on `K` to set a keychain kind (External or Internal) and is_default `bool`

What's wrong with the usual Default trait here?

rajarshimaitra commented 1 year ago

add a DefaultKeychainKind trait requirement on K to set a keychain kind (External or Internal) and is_default bool

I was thinking of struct Wallet<D = (), K = KeychainKind>, In that way we can keep all the existing APIs in impl <D : PeristentBackend> Wallet<D> {}

And all the new multi descriptor API in impl<D: PersistentBackend, K: Debug + Clone + Ord> Wallet <D, K> {} scope.

In this way all the users who don't care about keychains and used bdk in previous way can use the Wallet::new() constructor without specifying anything related to keychains and the current implementation of new will take care of using the KeychainKind enum internally to manage the two keychains.

I think @vladimirfomene tried this pattern out and and it seemed to work. Vlad can you confirm on this?

notmandatory commented 1 year ago
1. add a `DefaultKeychainKind` trait requirement on `K` to set a keychain kind (External or Internal) and is_default `bool`

What's wrong with the usual Default trait here?

I'm not thinking of a Default as in the default variant of K. A better name for the trait I have in mind is ActiveKeychainKind to filter K for ones that are "active" and "external" or "internal" when I need to pick a 'K' for new addresses.

If we don't want to impose any additional traits on K, the other way I can think to do it is for a non-default K, multi-descriptor Wallet (as @rajarshimaitra suggests above) we add a K param to the functions that need it, like getting a new receive address, or for change address in the TxBuilder.

LLFourn commented 1 year ago

Can we just make the user be explicit about which addresses they want rather than it being implicitly defined by a trait? It seems to me like the application should always know when requesting one. When creating a tx we could force the user to tell us about which keychain they want to use to derive a change address too.

vladimirfomene commented 1 year ago

add a DefaultKeychainKind trait requirement on K to set a keychain kind (External or Internal) and is_default bool

I was thinking of struct Wallet<D = (), K = KeychainKind>, In that way we can keep all the existing APIs in impl <D : PeristentBackend> Wallet<D> {}

And all the new multi descriptor API in impl<D: PersistentBackend, K: Debug + Clone + Ord> Wallet <D, K> {} scope.

In this way all the users who don't care about keychains and used bdk in previous way can use the Wallet::new() constructor without specifying anything related to keychains and the current implementation of new will take care of using the KeychainKind enum internally to manage the two keychains.

I think @vladimirfomene tried this pattern out and and it seemed to work. Vlad can you confirm on this?

I find this strategy workable. Let me sync with Steve to better understand how he thinks about this.

rajarshimaitra commented 1 year ago

Can we just make the user be explicit about which addresses they want rather than it being implicitly defined by a trait? It seems to me like the application should always know when requesting one. When creating a tx we could force the user to tell us about which keychain they want to use to derive a change address too.

If I understand correctly, that's exactly the plan. Force the user to specify a keychain to get addresses and create transactions from them. And this should not require any extra traits. We just need to manage the generics right while implementing multi-desc APIs, and provide a default for users who don't care about keychains/multi-descs.

Maybe eventually, we should not have the "default" wallet behavior and treat everything as multi-desc and always force users to specify keychains. But it will be a smoother transition to keep the default alive for now, as that makes BDK wallet really easy to spawn as a personal wallet setup.

rajarshimaitra commented 1 year ago

I'm not thinking of a Default as in the default variant of K. A better name for the trait I have in mind is ActiveKeychainKind to filter K for ones that are "active" and "external" or "internal" when I need to pick a 'K' for new addresses.

I think this will be a pretty massive break for the API. The concept of keychain wasn't present before.

I think

vladimirfomene commented 1 year ago

Examples

The basic example should be setup similar to what Core does for a new single key descriptor wallet:

  1. 3 external default single key descriptors, one for each script type
  2. 3 internal default single descriptors, one for each script type
  3. get new deposit addresses from default external descriptor for requested script type
  4. get balance, list transactions and utxos for all wallet descriptors
  5. use appropriate internal descriptor for change, depending on the script type of the transaction output

@notmandatory , I think you meant to say 4 external and 4 internal default. The core wallet has 4 script types for its DescriptorScriptPubKeyMan. 3 descriptors is for the LegacyScriptPubKeyMan.

notmandatory commented 1 year ago

OK, I agree with above that we don't need any new trait on K, and should go with struct Wallet<D = (), K = KeychainKind> and impl <D : PeristentBackend> Wallet<D> {} with functions that let the users not worry about multiple descriptors. Then for impl<D: PersistentBackend, K: Debug + Clone + Ord> Wallet <D, K> {} have equivalent functions for advanced users where they will need to specify which K for some functions.

notmandatory commented 1 year ago

@notmandatory , I think you meant to say 4 external and 4 internal default. The core wallet has 4 script types for its DescriptorScriptPubKeyMan. 3 descriptors is for the LegacyScriptPubKeyMan.

@vladimirfomene ah yes, you are correct, I must have been looking at an old pre-TR PR. I did a test just now to confirm and for a new default descriptor wallet Core creates 8 descriptors, internal + external for: pkh(), sh(wpkh()), tr() and wpkh(). I'll edit my above comment example.

vladimirfomene commented 1 year ago

With @rajarshimaitra and @notmandatory, we have settled on an initial approach to implementing a multi-descriptor wallet in BDK. Here are the highlights: