Support For Arbitrary Number (1+) of Keychains

[thunderbiscuit]

Description

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

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

Follow along the todos and PR development on this HackMD file.

Commits

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

Todos

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

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

Changelog notice

TODO

[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]

Thanks for providing design feedback @evanlinjin! Lots of food for thoughts. I'll bring those up at the next dev call(s), and maybe with some of our users I know are intending to use the multi-keychain feature.

Here are some of my early thoughts on this:

Default Keychains

1. This is not a defining feature of the PR and multi-keychain idea, and I'm certainly open to revisiting it. I think we initially thought it would be a good little utility piece since most wallets are likely to have a keychain they sort of by default reveal addresses on, but honestly after working on refactoring all methods in the impl blocks (there are 78 of them!) I realize how small of an impact this has (basically just on 2 or 3 methods that reveal/peek addresses and whatnot), and wonder if it was maybe an instance of us overoptimizing too early. One of the small gains we get from it is that the calls to the default keychain don't have to return an option, whereas calls that require the K parameter always do because you could provide a keychain that doesn't exist. Honestly not that big of a deal.
2. I am sympathetic to reducing the scope of the PR, and this indeed feels like something we can leave for further development if ever it's needed, without loosing the heart of the feature we intend to ship for users.
3. Indeed recreating these ideas at the application layer would be easy to do.
4. Given some modifications, I even suspect we could add this later on as a non-breaking change (albeit maybe without the ability to add it to persistence as we have it now).

All of this to say I am cautiously open to removing the default keychain from this PR at the moment, barring any veto from other team members or things I haven't thought of.

Wallet/MultiKeychainWallet

Ok this is an interesting one... thanks for laying it out this way I don't think I had understood you correctly when you first spoke of this a few months back on a dev call.

I see the idea behind the layers. At first glance however, this to me feels like more maintainership burden than is worth (because the 2 keychain wallet and the multi keychain wallet are so similar), but this is just a gut feeling, and of course I'm not the one who bears this burden the most. Will discuss over the next dev calls. We can lay down the new code that allows the Wallet type to handle any number of keychain, and maybe explore what a wrapper type would entail in exploratory branches that build off of #318.