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

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

Open
Tracked by #50 ...
rajarshimaitra opened this issue Nov 30, 2021 · 20 comments
Open
Tracked by #50 ...
Labels
module-descriptor new feature New feature or request

Comments

@rajarshimaitra
Copy link
Contributor

rajarshimaitra commented Nov 30, 2021

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

Resources

Skills Required

Mentor(s)

Difficulty: Hard

Competency Test (optional)

  • Install rust, compile and run all bdk examples and tests.
  • Read through the BDK docs.
  • Make a dummy wallet with BDK with different blockchain backends.
  • Familiarity with basic rust, should be able to write basic custom trait implementations on foreign structures.
@rajarshimaitra rajarshimaitra added the summer-of-bitcoin Summer of Bitcoin Project Proposal label Nov 30, 2021
@afilini
Copy link
Member

afilini commented Dec 1, 2021

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
Copy link
Contributor Author

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

@LLFourn
Copy link
Contributor

LLFourn commented Dec 2, 2021

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
Copy link
Contributor Author

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.

@rajarshimaitra rajarshimaitra changed the title BDK Descriptor Improvements: 02 - Add multi descriptor capability in wallet BDK Descriptor Improvements: Add multi descriptor capability in wallet Dec 2, 2021
@afilini
Copy link
Member

afilini commented Dec 2, 2021

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).

@notmandatory notmandatory removed the summer-of-bitcoin Summer of Bitcoin Project Proposal label Feb 14, 2022
afilini added a commit to afilini/bdk that referenced this issue Mar 15, 2022
Add three new traits:
- `StatelessBlockchain` is used to tag `Blockchain`s that don't have any
  wallet-specic state, i.e. they can be used as-is to sync multiple wallets.
- `StatefulBlockchain` is the opposite of `StatelessBlockchain`: it
  provides a method to "clone" a `Blockchain` with an updated internal
  state (a new wallet checksum and, optionally, a different number of
  blocks to skip from genesis). Potentially this also allows reusing the
  underlying connection on `Blockchain` types that support it.
- `MultiBlockchain` is a generalization of this concept: it's
  implemented automatically for every type that implements
  `StatefulBlockchain` and for every `Arc<T>` where `T` is a
  `StatelessBlockchain`. This allows a piece of code that deals with
  multiple sub-wallets to just get a `&B: MultiBlockchain` without having
  to deal with stateful and statless blockchains individually.

These new traits have been implemented for Electrum, Esplora and RPC
(the first two being stateless and the latter stateful). It hasn't been
implemented on the CBF blockchain, because I don't think it would work
in its current form (it throws away old block filters, so it's hard to
go back and rescan).

This is the first step for bitcoindevkit#549, as BIP47 needs to sync many different
descriptors internally.

It's also very useful for bitcoindevkit#486.
afilini added a commit to afilini/bdk that referenced this issue Mar 15, 2022
Add three new traits:
- `StatelessBlockchain` is used to tag `Blockchain`s that don't have any
  wallet-specic state, i.e. they can be used as-is to sync multiple wallets.
- `StatefulBlockchain` is the opposite of `StatelessBlockchain`: it
  provides a method to "clone" a `Blockchain` with an updated internal
  state (a new wallet checksum and, optionally, a different number of
  blocks to skip from genesis). Potentially this also allows reusing the
  underlying connection on `Blockchain` types that support it.
- `MultiBlockchain` is a generalization of this concept: it's
  implemented automatically for every type that implements
  `StatefulBlockchain` and for every `Arc<T>` where `T` is a
  `StatelessBlockchain`. This allows a piece of code that deals with
  multiple sub-wallets to just get a `&B: MultiBlockchain` without having
  to deal with stateful and statless blockchains individually.

These new traits have been implemented for Electrum, Esplora and RPC
(the first two being stateless and the latter stateful). It hasn't been
implemented on the CBF blockchain, because I don't think it would work
in its current form (it throws away old block filters, so it's hard to
go back and rescan).

This is the first step for bitcoindevkit#549, as BIP47 needs to sync many different
descriptors internally.

It's also very useful for bitcoindevkit#486.
afilini added a commit to afilini/bdk that referenced this issue Mar 23, 2022
Add two new traits:
- `StatelessBlockchain` is used to tag `Blockchain`s that don't have any
  wallet-specic state, i.e. they can be used as-is to sync multiple wallets.
- `BlockchainFactory` is a trait for objects that can build multiple
  blockchains for different descriptors. It's implemented automatically
  for every `Arc<T>` where `T` is a `StatelessBlockchain`. This allows a
  piece of code that deals with multiple sub-wallets to just get a
  `&B: BlockchainFactory` to sync all of them.

These new traits have been implemented for Electrum, Esplora and RPC
(the first two being stateless and the latter having a dedicated
`RpcBlockchainFactory` struct). It hasn't been implemented on the CBF
blockchain, because I don't think it would work in its current form
(it throws away old block filters, so it's hard to go back and rescan).

This is the first step for bitcoindevkit#549, as BIP47 needs to sync many different
descriptors internally.

It's also very useful for bitcoindevkit#486.
afilini added a commit to afilini/bdk that referenced this issue Mar 23, 2022
Add two new traits:
- `StatelessBlockchain` is used to tag `Blockchain`s that don't have any
  wallet-specic state, i.e. they can be used as-is to sync multiple wallets.
- `BlockchainFactory` is a trait for objects that can build multiple
  blockchains for different descriptors. It's implemented automatically
  for every `Arc<T>` where `T` is a `StatelessBlockchain`. This allows a
  piece of code that deals with multiple sub-wallets to just get a
  `&B: BlockchainFactory` to sync all of them.

These new traits have been implemented for Electrum, Esplora and RPC
(the first two being stateless and the latter having a dedicated
`RpcBlockchainFactory` struct). It hasn't been implemented on the CBF
blockchain, because I don't think it would work in its current form
(it throws away old block filters, so it's hard to go back and rescan).

This is the first step for bitcoindevkit#549, as BIP47 needs to sync many different
descriptors internally.

It's also very useful for bitcoindevkit#486.
afilini added a commit to afilini/bdk that referenced this issue Mar 23, 2022
Add two new traits:
- `StatelessBlockchain` is used to tag `Blockchain`s that don't have any
  wallet-specic state, i.e. they can be used as-is to sync multiple wallets.
- `BlockchainFactory` is a trait for objects that can build multiple
  blockchains for different descriptors. It's implemented automatically
  for every `Arc<T>` where `T` is a `StatelessBlockchain`. This allows a
  piece of code that deals with multiple sub-wallets to just get a
  `&B: BlockchainFactory` to sync all of them.

These new traits have been implemented for Electrum, Esplora and RPC
(the first two being stateless and the latter having a dedicated
`RpcBlockchainFactory` struct). It hasn't been implemented on the CBF
blockchain, because I don't think it would work in its current form
(it throws away old block filters, so it's hard to go back and rescan).

This is the first step for bitcoindevkit#549, as BIP47 needs to sync many different
descriptors internally.

It's also very useful for bitcoindevkit#486.
afilini added a commit to afilini/bdk that referenced this issue Mar 23, 2022
Add two new traits:
- `StatelessBlockchain` is used to tag `Blockchain`s that don't have any
  wallet-specic state, i.e. they can be used as-is to sync multiple wallets.
- `BlockchainFactory` is a trait for objects that can build multiple
  blockchains for different descriptors. It's implemented automatically
  for every `Arc<T>` where `T` is a `StatelessBlockchain`. This allows a
  piece of code that deals with multiple sub-wallets to just get a
  `&B: BlockchainFactory` to sync all of them.

These new traits have been implemented for Electrum, Esplora and RPC
(the first two being stateless and the latter having a dedicated
`RpcBlockchainFactory` struct). It hasn't been implemented on the CBF
blockchain, because I don't think it would work in its current form
(it throws away old block filters, so it's hard to go back and rescan).

This is the first step for bitcoindevkit#549, as BIP47 needs to sync many different
descriptors internally.

It's also very useful for bitcoindevkit#486.
afilini added a commit to afilini/bdk that referenced this issue Mar 23, 2022
Add two new traits:
- `StatelessBlockchain` is used to tag `Blockchain`s that don't have any
  wallet-specic state, i.e. they can be used as-is to sync multiple wallets.
- `BlockchainFactory` is a trait for objects that can build multiple
  blockchains for different descriptors. It's implemented automatically
  for every `Arc<T>` where `T` is a `StatelessBlockchain`. This allows a
  piece of code that deals with multiple sub-wallets to just get a
  `&B: BlockchainFactory` to sync all of them.

These new traits have been implemented for Electrum, Esplora and RPC
(the first two being stateless and the latter having a dedicated
`RpcBlockchainFactory` struct). It hasn't been implemented on the CBF
blockchain, because I don't think it would work in its current form
(it throws away old block filters, so it's hard to go back and rescan).

This is the first step for bitcoindevkit#549, as BIP47 needs to sync many different
descriptors internally.

It's also very useful for bitcoindevkit#486.
afilini added a commit to afilini/bdk that referenced this issue Mar 23, 2022
Add two new traits:
- `StatelessBlockchain` is used to tag `Blockchain`s that don't have any
  wallet-specic state, i.e. they can be used as-is to sync multiple wallets.
- `BlockchainFactory` is a trait for objects that can build multiple
  blockchains for different descriptors. It's implemented automatically
  for every `Arc<T>` where `T` is a `StatelessBlockchain`. This allows a
  piece of code that deals with multiple sub-wallets to just get a
  `&B: BlockchainFactory` to sync all of them.

These new traits have been implemented for Electrum, Esplora and RPC
(the first two being stateless and the latter having a dedicated
`RpcBlockchainFactory` struct). It hasn't been implemented on the CBF
blockchain, because I don't think it would work in its current form
(it throws away old block filters, so it's hard to go back and rescan).

This is the first step for bitcoindevkit#549, as BIP47 needs to sync many different
descriptors internally.

It's also very useful for bitcoindevkit#486.
afilini added a commit to afilini/bdk that referenced this issue Apr 13, 2022
Add two new traits:
- `StatelessBlockchain` is used to tag `Blockchain`s that don't have any
  wallet-specic state, i.e. they can be used as-is to sync multiple wallets.
- `BlockchainFactory` is a trait for objects that can build multiple
  blockchains for different descriptors. It's implemented automatically
  for every `Arc<T>` where `T` is a `StatelessBlockchain`. This allows a
  piece of code that deals with multiple sub-wallets to just get a
  `&B: BlockchainFactory` to sync all of them.

These new traits have been implemented for Electrum, Esplora and RPC
(the first two being stateless and the latter having a dedicated
`RpcBlockchainFactory` struct). It hasn't been implemented on the CBF
blockchain, because I don't think it would work in its current form
(it throws away old block filters, so it's hard to go back and rescan).

This is the first step for bitcoindevkit#549, as BIP47 needs to sync many different
descriptors internally.

It's also very useful for bitcoindevkit#486.
afilini added a commit to afilini/bdk that referenced this issue Apr 13, 2022
Add two new traits:
- `StatelessBlockchain` is used to tag `Blockchain`s that don't have any
  wallet-specic state, i.e. they can be used as-is to sync multiple wallets.
- `BlockchainFactory` is a trait for objects that can build multiple
  blockchains for different descriptors. It's implemented automatically
  for every `Arc<T>` where `T` is a `StatelessBlockchain`. This allows a
  piece of code that deals with multiple sub-wallets to just get a
  `&B: BlockchainFactory` to sync all of them.

These new traits have been implemented for Electrum, Esplora and RPC
(the first two being stateless and the latter having a dedicated
`RpcBlockchainFactory` struct). It hasn't been implemented on the CBF
blockchain, because I don't think it would work in its current form
(it throws away old block filters, so it's hard to go back and rescan).

This is the first step for bitcoindevkit#549, as BIP47 needs to sync many different
descriptors internally.

It's also very useful for bitcoindevkit#486.
afilini added a commit to afilini/bdk that referenced this issue Apr 13, 2022
Add two new traits:
- `StatelessBlockchain` is used to tag `Blockchain`s that don't have any
  wallet-specic state, i.e. they can be used as-is to sync multiple wallets.
- `BlockchainFactory` is a trait for objects that can build multiple
  blockchains for different descriptors. It's implemented automatically
  for every `Arc<T>` where `T` is a `StatelessBlockchain`. This allows a
  piece of code that deals with multiple sub-wallets to just get a
  `&B: BlockchainFactory` to sync all of them.

These new traits have been implemented for Electrum, Esplora and RPC
(the first two being stateless and the latter having a dedicated
`RpcBlockchainFactory` struct). It hasn't been implemented on the CBF
blockchain, because I don't think it would work in its current form
(it throws away old block filters, so it's hard to go back and rescan).

This is the first step for bitcoindevkit#549, as BIP47 needs to sync many different
descriptors internally.

It's also very useful for bitcoindevkit#486.
afilini added a commit to afilini/bdk that referenced this issue Apr 13, 2022
Add two new traits:
- `StatelessBlockchain` is used to tag `Blockchain`s that don't have any
  wallet-specic state, i.e. they can be used as-is to sync multiple wallets.
- `BlockchainFactory` is a trait for objects that can build multiple
  blockchains for different descriptors. It's implemented automatically
  for every `Arc<T>` where `T` is a `StatelessBlockchain`. This allows a
  piece of code that deals with multiple sub-wallets to just get a
  `&B: BlockchainFactory` to sync all of them.

These new traits have been implemented for Electrum, Esplora and RPC
(the first two being stateless and the latter having a dedicated
`RpcBlockchainFactory` struct). It hasn't been implemented on the CBF
blockchain, because I don't think it would work in its current form
(it throws away old block filters, so it's hard to go back and rescan).

This is the first step for bitcoindevkit#549, as BIP47 needs to sync many different
descriptors internally.

It's also very useful for bitcoindevkit#486.
afilini added a commit to afilini/bdk that referenced this issue Apr 13, 2022
Add two new traits:
- `StatelessBlockchain` is used to tag `Blockchain`s that don't have any
  wallet-specic state, i.e. they can be used as-is to sync multiple wallets.
- `BlockchainFactory` is a trait for objects that can build multiple
  blockchains for different descriptors. It's implemented automatically
  for every `Arc<T>` where `T` is a `StatelessBlockchain`. This allows a
  piece of code that deals with multiple sub-wallets to just get a
  `&B: BlockchainFactory` to sync all of them.

These new traits have been implemented for Electrum, Esplora and RPC
(the first two being stateless and the latter having a dedicated
`RpcBlockchainFactory` struct). It hasn't been implemented on the CBF
blockchain, because I don't think it would work in its current form
(it throws away old block filters, so it's hard to go back and rescan).

This is the first step for bitcoindevkit#549, as BIP47 needs to sync many different
descriptors internally.

It's also very useful for bitcoindevkit#486.
afilini added a commit to afilini/bdk that referenced this issue Apr 15, 2022
Add two new traits:
- `StatelessBlockchain` is used to tag `Blockchain`s that don't have any
  wallet-specic state, i.e. they can be used as-is to sync multiple wallets.
- `BlockchainFactory` is a trait for objects that can build multiple
  blockchains for different descriptors. It's implemented automatically
  for every `Arc<T>` where `T` is a `StatelessBlockchain`. This allows a
  piece of code that deals with multiple sub-wallets to just get a
  `&B: BlockchainFactory` to sync all of them.

These new traits have been implemented for Electrum, Esplora and RPC
(the first two being stateless and the latter having a dedicated
`RpcBlockchainFactory` struct). It hasn't been implemented on the CBF
blockchain, because I don't think it would work in its current form
(it throws away old block filters, so it's hard to go back and rescan).

This is the first step for bitcoindevkit#549, as BIP47 needs to sync many different
descriptors internally.

It's also very useful for bitcoindevkit#486.
afilini added a commit to afilini/bdk that referenced this issue Apr 15, 2022
Add two new traits:
- `StatelessBlockchain` is used to tag `Blockchain`s that don't have any
  wallet-specic state, i.e. they can be used as-is to sync multiple wallets.
- `BlockchainFactory` is a trait for objects that can build multiple
  blockchains for different descriptors. It's implemented automatically
  for every `Arc<T>` where `T` is a `StatelessBlockchain`. This allows a
  piece of code that deals with multiple sub-wallets to just get a
  `&B: BlockchainFactory` to sync all of them.

These new traits have been implemented for Electrum, Esplora and RPC
(the first two being stateless and the latter having a dedicated
`RpcBlockchainFactory` struct). It hasn't been implemented on the CBF
blockchain, because I don't think it would work in its current form
(it throws away old block filters, so it's hard to go back and rescan).

This is the first step for bitcoindevkit#549, as BIP47 needs to sync many different
descriptors internally.

It's also very useful for bitcoindevkit#486.
afilini added a commit to afilini/bdk that referenced this issue Apr 15, 2022
Add two new traits:
- `StatelessBlockchain` is used to tag `Blockchain`s that don't have any
  wallet-specic state, i.e. they can be used as-is to sync multiple wallets.
- `BlockchainFactory` is a trait for objects that can build multiple
  blockchains for different descriptors. It's implemented automatically
  for every `Arc<T>` where `T` is a `StatelessBlockchain`. This allows a
  piece of code that deals with multiple sub-wallets to just get a
  `&B: BlockchainFactory` to sync all of them.

These new traits have been implemented for Electrum, Esplora and RPC
(the first two being stateless and the latter having a dedicated
`RpcBlockchainFactory` struct). It hasn't been implemented on the CBF
blockchain, because I don't think it would work in its current form
(it throws away old block filters, so it's hard to go back and rescan).

This is the first step for bitcoindevkit#549, as BIP47 needs to sync many different
descriptors internally.

It's also very useful for bitcoindevkit#486.
afilini added a commit to afilini/bdk that referenced this issue Apr 15, 2022
Add two new traits:
- `StatelessBlockchain` is used to tag `Blockchain`s that don't have any
  wallet-specic state, i.e. they can be used as-is to sync multiple wallets.
- `BlockchainFactory` is a trait for objects that can build multiple
  blockchains for different descriptors. It's implemented automatically
  for every `Arc<T>` where `T` is a `StatelessBlockchain`. This allows a
  piece of code that deals with multiple sub-wallets to just get a
  `&B: BlockchainFactory` to sync all of them.

These new traits have been implemented for Electrum, Esplora and RPC
(the first two being stateless and the latter having a dedicated
`RpcBlockchainFactory` struct). It hasn't been implemented on the CBF
blockchain, because I don't think it would work in its current form
(it throws away old block filters, so it's hard to go back and rescan).

This is the first step for bitcoindevkit#549, as BIP47 needs to sync many different
descriptors internally.

It's also very useful for bitcoindevkit#486.
william-devinci pushed a commit to william-devinci/bdk that referenced this issue Nov 16, 2022
Add two new traits:
- `StatelessBlockchain` is used to tag `Blockchain`s that don't have any
  wallet-specic state, i.e. they can be used as-is to sync multiple wallets.
- `BlockchainFactory` is a trait for objects that can build multiple
  blockchains for different descriptors. It's implemented automatically
  for every `Arc<T>` where `T` is a `StatelessBlockchain`. This allows a
  piece of code that deals with multiple sub-wallets to just get a
  `&B: BlockchainFactory` to sync all of them.

These new traits have been implemented for Electrum, Esplora and RPC
(the first two being stateless and the latter having a dedicated
`RpcBlockchainFactory` struct). It hasn't been implemented on the CBF
blockchain, because I don't think it would work in its current form
(it throws away old block filters, so it's hard to go back and rescan).

This is the first step for bitcoindevkit#549, as BIP47 needs to sync many different
descriptors internally.

It's also very useful for bitcoindevkit#486.
@notmandatory notmandatory moved this from In Progress to Todo in BDK Maintenance Mar 26, 2023
@notmandatory
Copy link
Member

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 Improve wallet address API #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
Copy link
Member

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 #918 (comment). 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
Copy link
Member

notmandatory commented Apr 14, 2023

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
Copy link
Contributor

LLFourn commented Apr 17, 2023

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
Copy link
Contributor Author

rajarshimaitra commented Apr 19, 2023

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
Copy link
Member

notmandatory commented Apr 20, 2023

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
Copy link
Contributor

LLFourn commented Apr 20, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

rajarshimaitra commented Apr 22, 2023

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
Copy link
Contributor Author

rajarshimaitra commented Apr 22, 2023

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

  • We can keep the original API without users ever knowing about keychains. Just internal/external apis that they used before.
  • We can add the new APIs where users are forced to specify keychains, and can add their custom keychains of any kind that they like.
  • All this while not creating another new trait.

Vlad is close to coming up with the concept code, (waiting on my review). At this point, it would be helpful to have the code to talk over.

@vladimirfomene
Copy link
Contributor

vladimirfomene commented Apr 22, 2023

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
Copy link
Member

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
Copy link
Member

@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
Copy link
Contributor

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

  • We will make the wallet struct generic over a certain K (K has to be Ord + Clone + Debug as the generic keychain identifier in TxoutIndex) and give it a default type of KeychainKind so that we can have a default wallet that just supports two descriptors (internal and external).
  • The multi-descriptor wallet will take in a Vec<K, E: IntoWalletDescriptor> in its constructor and initialize the wallet.
  • While spending with a multi-descriptor wallet, we need to specify the keychains we want to spend utxos from and also specify the keychain to use for change outputs. In a more advanced version of this wallet, you can give it a list of keychains for spending and a list of potential keychains to use for change. The wallet will be smart enough to pick which keychain to use for change and will do so by picking one that will help your wallet preserve its privacy. The wallet here will pick the keychain that will allow it produce a change output script that is same as the payment output script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module-descriptor new feature New feature or request
Projects
Status: In Progress
6 participants