Support For Arbitrary Number of Keychains

[thunderbiscuit]

Description

This PR allows the Wallet type to track any number of keychains (descriptors). It is a breaking change.

Related Issues: #188, #227
Related PRs: #230, #226
Exploratory Crate: multi-keychain-wallet

Read this one-pager for an overview of user-facing impacts of the proposed feature.

Commit	Changes
1	Comment out Wallet dependent code.
2	Add KeyRing and related types.
3	Modify Wallet to levergage the KeyRing.
4	Docs and examples.
5	Add tests.

Todo

Search for the following pattern to find Todos in the codebase:

// TODO PR #318: We should fix this because...

• Reveal addresses
• Adding a new descriptor to the keyring with the same identifier should not replace the keychain but rather return an error.
• KeyRing::new should throw an errors on certain conditions
• SyncRequest and SyncResponse need to be generic in K
• Apply block tx data
• Apply updates
• Fix up the Wallet API docs (K needs to be Ord for example)
• Should the create_with_params take a struct of params?
• Wallet::new should call Wallet::create_with_params
• Wallet persistence
• Customizing genesis hash: throw if KeyRing is on Mainnet, ok otherwise.
• Fix up the wallet ChangeSet.
• Add persistence example.
• Restore tests if applicable
• Add wallet migration example from a 2.3.0 wallet

Changelog notice

TODO

Deployment Strategy

As per discussions over different dev calls, this feature is proposed as a breaking change on the Wallet API, since the old constructors are removed. It might not be ready in time for the 3.0 release (given the size of the diff and its relative position on the totem pole of priorities). If it does not make the cut for the 3.0 release, it will simply be kept updated and rebased on master until it is ready for merging into a breaking change ready branch for the 4.0 release.

Changes On Wallet

Here is a table to help track changes on the wallet/mod.rs file. I tried to add everything with a visual of New/Changed/Removed/Identical (⭐/🔄/🚫/✅ respectively), as well as some notes on the changes.

Method	N/C/R/I	Notes
# Constructors
Wallet::create_single	🚫
Wallet::create	🔄	Now takes a KeyRing
# Wallet information getters
Wallet::network	✅
Wallet::tx_graph	✅
Wallet::local_chain	✅
Wallet::latest_checkpoint	✅
Wallet::spk_index	✅
Wallet::keychains	⭐	Lists the keychains your wallet tracks.
Wallet::default_keychain	⭐	Returns your current default keychain.
# Deriving addresses
Wallet::reveal_next_address	🔄	Now takes a keychain K parameter
Wallet::reveal_next_default_address	⭐	Convenience method that returns an address on your default keychain
Wallet::next_unused_address	🔄	Now takes a keychain K parameter
Wallet::derivation_index	🔄	Now takes a keychain K parameter

[110CodingP]

The next few commits:

• modify the Wallet struct and add a basic constructor
• add some address apis
• add another constructor for advanced users to provide things like a custom genesis_hash.

[thunderbiscuit]

Quick review on 6c50f00: given your commit message I thought you were adding the Wallet::new constructor here, but the commit only contains the addition of the KeyRing to the Wallet type. Mind you this might be enough for this commit. Feel free to add a new commit with the constructor, or add it to this commit; whatever you think works best.

Also note that fa984e6 and 6c50f00 don't compile because of

impl AsRef<bdk_chain::tx_graph::TxGraph<ConfirmationBlockTime>> for Wallet {
    fn as_ref(&self) -> &bdk_chain::tx_graph::TxGraph<ConfirmationBlockTime> {
        self.keychain_tx_graph.graph()
    }
}

on line 2638 of wallet/mod.rs. You could comment it out with the rest to ensure everything builds.

[thunderbiscuit]

I'm not sure if the renaming of this field is intentional and I don't have an opinion on whether it's a good idea or not yet, but it probably belongs in a different PR.

[110CodingP]

I did the rename since the corresponding field in Wallet is so. reverted the rename in 588121b.

[110CodingP]

I am so sorry 😢 . I completely messed dividing the commits into two. Things should be fixed now!

[thunderbiscuit]

This one now needs a big ol' rebase @110CodingP.

[110CodingP]

Wow! this one was HUGE! For the first few commits I did something like git rebase --onto master HEAD~7 HEAD~6 and for the later ones I cherry-picked each commit onto master and finally did a reset --hard, wondering if there's a simpler way to do difficult rebases 😅 ?

[thunderbiscuit]

Adding an example like this fails:

    // Simple KeyRing, allowing us to build a standard 2-descriptor wallet.
    let external_descriptor: &str = "tr(tpubD6NzVbkrYhZ4WyC5VZLuSJQ14uwfUbus7oAFurAFkZA5N3groeQqtW65m8pG1TT1arPpfWu9RbBsc5rSBncrX2d84BAwJJHQfaRjnMCQwuT/86h/1h/0h/0/*)";
    let internal_descriptor: &str = "tr(tpubD6NzVbkrYhZ4WyC5VZLuSJQ14uwfUbus7oAFurAFkZA5N3groeQqtW65m8pG1TT1arPpfWu9RbBsc5rSBncrX2d84BAwJJHQfaRjnMCQwuT/86h/1h/0h/1/*)";

    let mut keyring: KeyRing<KeychainKind> = KeyRing::new(Network::Regtest, KeychainKind::External, external_descriptor);
    keyring.add_descriptor(KeychainKind::Internal, internal_descriptor, false);

    let mut wallet = Wallet::new(keyring);

With a stacktrace containing:

thread 'main' panicked at /Users/user/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/miniscript-12.3.5/src/descriptor/key.rs:687:14:
The key should not contain any wildcards at this point

After a bit of poking around I found that this is coming from the descriptor_id() method called inside insert_descriptor():

    pub fn insert_descriptor(
        &mut self,
        keychain: K,
        descriptor: Descriptor<DescriptorPublicKey>,
    ) -> Result<bool, InsertDescriptorError<K>> {
        let did = descriptor.descriptor_id();

I don't have time to finish the investigation today, but just pointing it out. I'm not sure yet why this worked well in the multi-keychain-wallet crate and not here, nor why the code for the constructor is so different than the other previous constructors like Wallet::create_with_params.

[110CodingP]

This almost scared me for a moment 😅 ! Using descriptors with unhardened paths seems to work.
This got me into thinking about the other constructor with_custom_params , currently the genesis_hash takes precedence over keyring's network so should there be a check in the constructor which asserts that if KeyRing has Mainnet as the network the genesis_hash should also be of the Mainnet? and similarly for testnets and regtest?

[thunderbiscuit]

Total facepalm. Sorry @110CodingP! Ok so I simplified the example a little bit just to keep a really neat version of how to build a wallet as close as you can from the 2.0 approach.

[110CodingP]

Commit	Description
603b1cf	Just comments out the policy code and removes the policy variant from DescriptorError
4177147	Implements checks when adding descriptors into the keyring, checking that the desc is not multipath or hardened and one of the desc or keychain is already not present. Also add_descriptor returns a ChangeSet now which we can use when we introduce APIs to add descriptors into the wallet during runtime.

Completes first 2 of the Todos?

[110CodingP]

Commit	Description
e33e007	Added from_changeset api to Wallet. Modified the corresponding api for KeyRing to do checks and do better error handling. New error type is introduced for errors related to loading keyring and existing wallet error types are refactored.
35ea57e	Added getter for keyring's network
e6e00df	Added sqlite persistence functions for wallet and keyring. Also added a new trait which is used to persist a keychain type. Implemented the trait for DescriptorId and KeychainKind and added an example to test the functions.
4f87e32	Refactored new to reuse with_custom_params

for e33e007 :
Review especially required in the loop checking whether descriptors are loaded properly. Like None is used to just check presence instead of absence of a keychain, and no checks are there in case extra descriptors get loaded (is it possible?).
for e6e00df :
As an additional check, added UNIQUE to the descriptor column in the sqlite table, would it be better to have (keychain, descriptor) as a PRIMARY KEY? Added STRICT keyword in the table definitions hence cannot use BOOLEAN for the is_default column, should the STRICT keyword be removed? Also added check that id in network table always has value 0. Currently when these checks fail sqlite remains silent about it because of IGNORE, is there a way to return an error while handling the cases that we might try to persist the identical rows (which ig is the reason for the IGNORE) ? PersistedWallet will be added in future commits.

Also ig DescriptorError should be generic so that we can return the keychain along with KeychainAlreadyExists variant but this will require introducing a generic in IntoWalletDescriptor, which approach is better?

[110CodingP]

Also is balance_per_keychain api desirable? ig it would help with this bdk#57.

[110CodingP]

Does the new balance API help with #273 and #16 ?

[thunderbiscuit]

@luisschwab yep good question. This will come into play if we allow dynamically changing the keychains on wallets (it has historically not been possible to do so with BDK wallets, but there might be a case to be made for it? not sure.)

It feels like having to deal with the possibility of a user adding or removing a keychain at any point might add more complexity to the codebase than it brings value (since you can always just create a new wallet with the specific keychains you want) but there might be use cases I'm not thinking about.

[luisschwab]

Makes sense, making dynamic keychain swapping possible would make things a lot more complex.

I noticed you have the sync examples commented out for now. When making synching a wallet, are you planning to add all revealed spk's into an "spk pool" and sync everything, or are we going to have to specify which keychain we want to sync? I ask this because with the first option, I can just take the first transaction from the "spk pool" and set the wallet's birthday as the transaction's BlockId, instead of possibly having to overwrite it if another keychain has an older birthday and we sync it later.

[110CodingP]

I noticed you have the sync examples commented out for now. When making synching a wallet, are you planning to add all revealed spk's into an "spk pool" and sync everything, or are we going to have to specify which keychain we want to sync?

Currently we sync all keychains in one go . But ig having an API to sync a particular keychain would also be great in case of large wallets with multiple keychains?

I ask this because with the first option, I can just take the first transaction from the "spk pool" and set the wallet's birthday as the transaction's BlockId, instead of possibly having to overwrite it if another keychain has an older birthday and we sync it later.

Yup, I agree this should work if we are not allowing addition of keychains!

[thunderbiscuit]

@110CodingP I worked today on basically applying this loop to all 5 commits:

git checkout <commit>
cargo build
cargo test

And seeing if everything compiled and worked well. I ended up finding a few things. Here is a summary of what I did:

1. The second commit added KeyRing but used an error variant (LoadError) on the wallet that was made generic on the only on the 4th commit. Let me know what you think but I decided to keep the error variant within the KeyRing module, and so now KeyRing::from_changeset returns a KeyRingError (basically brought the LoadError variants related to the KeyRing into the KeyRingError variant).