Wallet must have unique Internal and External descriptors

[notmandatory]

Per discussion in our last backlog review call, since the bdk wallet APIs are opinionated about how to correctly build and use an on-chain wallet we should make unique internal (change address) and external (receive address) descriptors mandatory when creating a new Wallet. This will also allow us to remove current behavior of getting new change addresses from the external descriptor if no internal descriptor is given. Using external addresses for internal (change) breaks other wallet features like asking the tx builder to create transactions that don't spend change.

If a user wants to create a wallet without an internal descriptor they can do what ever they want using the lower level bdk_chain and other crates.

[rustaceanrob]

Does this change imply that all wallets have both an internal and external descriptor, and are unique, or is a change descriptor still optional?

[storopoli]

Does this change imply that all wallets have both an internal and external descriptor, and are unique, or is a change descriptor still optional?

This means that the new bdk_wallet (old bdk, see #1305) crate is opinionated.
You can use whatever descriptors you want if you use the low-level stuff in bdk_chain.

[rustaceanrob]

Currently, if None is provided as change, this implies that Internal and External descriptors are not unique. I checked out a local branch that removes Option<E> for change and replaces it with E, as this would force unique descriptors as the title implies, so long as there is a check in new. This is a significant change especially in the test vectors, so I wanted to make sure I understood this correctly before proceeding on my branch. It sounds like opinionated here may also imply conventional bip32 derivation paths or similar?

[notmandatory]

Hi yes this will be a significant change and will be a big change for tests. We discussed in during our recent backlog call and team decided it's best to make this change for the 1.0.0 milestone, I added some more description above.

[ValuedMammal]

My thought for tests is to change the implementation of get_funded_wallet to init the wallet with two descriptors, but maybe that was already obvious. I'm certainly willing to help out on this one.

We might be able to allow non-unique if the user insists on passing in the same descriptor for both external and internal, but I haven't looked deeply into it.

^ scratch that. not if it breaks key functionality

[ValuedMammal]

I believe I got this to build, just need to do tests and docs.

Highlights:

• Make internal required
• Add new descriptor error NonUniqueInternal
• Remove CreateTxError::ChangePolicyDescriptor
• Get rid of map_keychain

[casey-bowman]

I've asked the following question in issue #1390, too.

If two descriptors are the same except that each has a different extended public key that is replaced by the corresponding extended private key, do these qualify as distinct?

[notmandatory]

@casey-bowman do you mean one descriptor uses pubkey A and the other uses the corresponding private key A' ? these should not be distinct. This is a good point, when comparing descriptors we should always compare them with any private keys converted to the corresponding public keys.

There is another case where we could get into trouble, which is when we have one descriptor which uses the derived public key, and the other that uses the root key plus derivation path. The descriptors will derive the same scripts but on a string comparison be different. Should we compare descriptors by comparing their first (index 0) derived script pubkey ?

[casey-bowman]

@casey-bowman do you mean one descriptor uses pubkey A and the other uses the corresponding private key A' ? these should not be distinct. This is a good point, when comparing descriptors we should always compare them with any private keys converted to the corresponding public keys.

Yes, that's exactly what I mean. I agree with you that they should be treated the same if all the corresponding (extended) public keys line up. ( UPDATE : please see the next comment for an important qualification on this that fits in with your proposed zero index value test.)

As to the other situation, yes, I see what you mean there, too. The test that would use the zero index value seems simple and effective :)

[casey-bowman]

One note though - I am planning to have wallets that use derivation paths to the first index (or starred index) that might have the same extended public keys but distinct paths down to their respective first indices, say three levels down instead of just two, for example. This helps economize on the gathering of extended public keys in the channel factory protocol.

So your test would work there, too. One would still just be comparing the values for the first index, which would be, in the case I just described, distinct.

[ValuedMammal]

Another edge case: one descriptor has no wildcard but still derives the same first address. Interestingly, KeychainTxOutIndex will catch if we try to index a duplicate script pubkey, and we can possibly take advantage of that.

[evanlinjin]

Hmmm another problem is if we have a wildcard descriptor for one keychain, but a non-wildcard descriptor for another, where the non-wildcard descriptor derives an address that belongs to the wildcard descriptor (lets say at index 1200). How do we detect this?

As mentioned by @ValuedMammal, KeychainTxOutIndex will freak out if we have the same spk under different keychains...

Do we need further restrictions such as "cannot use non-wildcard descriptor with a wildcard descriptor"? Or is the better option to allow non-unique descriptors for internal/external and change SpkTxOutIndex to allow for multiple "indexes" (I) per spk?

[LLFourn]

I think I mentioned elsewhere that we shouldn't try to detect this other than a simple eq comparison and just consider a user error that leads to undefined behavior. I don't think that anything world ending happens if they overlap. It just means the spk txout index will only associate the spk with the first keychain it is revealed under. This is fine isn't it? There's no coherent way to have the same addresses on two different keychains so you get incoherent results if you do this. You can't lose coins though.

[matthiasdebernardini]

Current API:

let external_descriptor ="wpkh(tprv8ZgxMBicQKsPdy6LMhUtFHAgpocR8GC6QmwMSFpZs7h6Eziw3SpThFfczTDh5rW2krkqffa11UpX3XkeTTB2FvzZKWXqPY54Y6Rq4AQ5R8L/84'/1'/0'/0/*)";
let internal_descriptor = "wpkh(tprv8ZgxMBicQKsPdy6LMhUtFHAgpocR8GC6QmwMSFpZs7h6Eziw3SpThFfczTDh5rW2krkqffa11UpX3XkeTTB2FvzZKWXqPY54Y6Rq4AQ5R8L/84'/1'/0'/1/*)";
let mut wallet = Wallet::new_or_load(external_descriptor, Some(internal_descriptor), (), Signet).unwrap();

My ideal API:

let external_descriptor = "wpkh(tprv8ZgxMBicQKsPdy6LMhUtFHAgpocR8GC6QmwMSFpZs7h6Eziw3SpThFfczTDh5rW2krkqffa11UpX3XkeTTB2FvzZKWXqPY54Y6Rq4AQ5R8L/84'/1'/0'/0/*)";
let mut wallet = Wallet::new_or_load(external_descriptor, None, (), Signet).unwrap();

Where both version of the API result in an identical wallet.

In the background, the API would take the external descriptor and modify the path to generate the internal descriptor. It implicitly assumes that you want a change descriptor and that its path is derived from the external descriptor by replacing the last hardened index with "1/*".

This would be my preference as I think it's the most reasonable default behavior for creating an on-chain wallet where the internal and external descriptors are identical except for the path.

I would propose a Default implementation for the wallet descriptor, but that may be a significant undertaking.

If this goes against the design goals of the wallet API, I think at the least adding a method that modifies the internal descriptor and provides a default change descriptor would be highly ergonomic.

[rustaceanrob]

In the background, the API would take the external descriptor and modify the path to generate the internal descriptor. It implicitly assumes that you want a change descriptor and that its path is derived from the external descriptor by replacing the last hardened index with "1/*".

While the single-signature case is very common, I do not think there is a strong case for default behavior like this. Descriptors with timelocks and some multi-signature constructions don't necessarily adhere to this behavior, let alone more complex scripts.

If this goes against the design goals of the wallet API, I think at the least adding a method that modifies the internal descriptor and provides a default change descriptor would be highly ergonomic.

I would argue this behavior exists already by using bitcoin::bip32::ExtendedPubKey, miniscript::descriptor::DescriptorXKey, and miniscript::descriptor::DescriptorPublicKey, as you may get and modify the DerivationPath and create a new descriptor from it. The existing API allows you to alter derivation paths without any extra dependencies, regex, etc. Perhaps a method DescriptorXKey would be helpful to change an index on the derivation path, but that is outside the scope of BDK.