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]

I found some methods that are cut out from the codebase because of commit 5df393d, we should make sure we bring those back (or remove only the ones we really want to remove).

[thunderbiscuit]

Some methods are removed entirely from the module, for example here Wallet::events_helper.

[110CodingP]

Thanks for catching this! This shouldn't have happened though because this is just a rebase over master. Trying to figure out the reason.

[ValuedMammal]

How to preserve the automatic change derivation mechanism

Problem

In the 2-keychain wallet, the change address can be automatically derived because we know which keychain is the "internal" one. However once we allow potentially many keychains we can no longer assume which keychain may be designated for change. It would be annoying, and arguably a regression, for the user to specify the change keychain on the PSBT params every time.

Proposed solution is to add an internal flag to each wallet descriptor at wallet creation, persisted via the ChangeSet. Knowing which keychain(s) can be used for change not only allows us to derive the change script automatically, but match the change script to the dominant output script type (for privacy reasons).

Example

/// Represents a wallet descriptor
pub struct WalletDescriptor<D> {
    descriptor: D,
    internal: bool,
}

impl WalletDescriptor<D> {
    /// A new wallet descriptor for generating receive addresses.
    pub fn new(descriptor: D) -> Self { ... }

    /// A new wallet descriptor with the internal flag set indicating it may be used to generate change addresses.
    pub fn new_internal(descriptor: D) -> Self { ... }
}

impl KeyRing<K> {
    /// Create new from (keychain, wallet_descriptor) tuples.
    pub fn new<D>(descriptors: impl IntoIterator<Item = (K, WalletDescriptor<D>)>) -> Self
    where
        D: IntoWalletDescriptor,
    { ... }
}

[thunderbiscuit]

I like the idea of simplifying the work for users (particularly if they have a given keychain that's always the change keychain for a given descriptor, in which case they would as you say just always have to set it as their change keychain on every PSBT build).

Looking at the pseudocode above though, I think it might need adjusting just tad. In this scenario (at least in my mind), the keychains should be provided in pairs (just a marker that says "this is internal" is not enough if you actually have 6 keychains, and some of them have an internal, sister-keychain but they're all different, some don't have change keychain altogether, etc.).

Also tiny nit: I have seen enough users misunderstand the meaning of internal/external as to scare me for a lifetime. I don't know if this PR is the place to start this journey, but I think we should promote the receive/change taxonomy, or maybe just main/change.

[ValuedMammal]

The main design hurdle is the ability of the wallet to derive a change address during transaction creation. For that we need to encode whether a keychain is eligible to "receive" change. In the process, to remove ambiguity in keychain selection, we should finally move away from using map_keychain.

In the single-keychain case, the user can mark their only keychain as also eligible for change addresses and the process should "just work" for deriving addresses in all cases. Of course the user can override it by specifying the change script directly on PsbtParams. If not explicitly provided, and if no keychains are marked "internal" by the wallet then create_psbt should return a "MissingChangeScript" error or similar.

The need to quickly access descriptor metadata in the wallet (e.g. the internal/change flag) again supports the argument for having KeyRing as a member of the Wallet structure.

[thunderbiscuit]

we should finally move away from using map_keychain

Yep 100% agree!

If not explicitly provided, and if no keychains are marked "internal" by the wallet then create_psbt should return a "MissingChangeScript" error or similar.

Also agree. One thing we need to consider however, is that there is potentially multiple keychains marked as candidates for change, and then again you'd need a way to discriminate. A naive version of this could use keychain pairs, but then what happens when the wallet creates a transaction that pulls in UTXOs from different keychains? They each would potentially have a change keychain and then we haven't quite solved our issue. Maybe requiring users provide a change keychain on all transaction creation seems like a good basic first step, and then we can relax this by providing further shortcuts and helpers?

supports the argument for having KeyRing as a member of the Wallet structure.

Darn! I kind of agree here, metadata is useful of course and we need access. But one of the weird aspects of the keyring as part of the wallet is that the descriptors then exist in two places (in the tx_graph and the keyring field). If there is a way to just use the keychains in the tx_graph I think that's maybe a good way to not duplicate or split the source of truth for the wallet. I've honestly been going back and forth on this (now after thinning the KeyRing as much as I have I kind of even find myself wondering if it's important enough to keep altogether... need to deep dive on this a bit more).

Update on this PR

I worked on the rebase of this yesterday all day and today, and it's become clear to me that the 15k green 15k red line diff is super hard to (1) manage + rebase, and (2) review (too much is just pre-keychain work and too little is actual API changes).

I'm testing out a different approach:

1. Create a branch that's called pre-multi-keychain. This contains work that's required but not directly related to multi-keychain. For example do we just cut the TxBuilder out entirely? Do it there. If so, the tests all need updating. Many of the wallet tests can migrate to Wallet::create_psbt. Then remove the signers. Stuff like that. This one is sort of loosely defined and I understand could be contentious, because it's sort of assuming a 4.0 world. My current work on this
   (a) uses basically the separation between TxBuilder and signers explored in Deprecate wallet-owned signing APIs #505, and (b) removes the signers.
2. Open the multi-keychain PR against that branch! The PR becomes simpler and easier to review conceptually, even if we don't necessarily know what the shape of 4.0 on master will be yet.

Example of this here.