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 ?