Multi Descriptor Wallet API [MVP Refactoring]

[rajarshimaitra]

Description

Fixes bitcoindevkit/bdk_wallet#188 .

This describes the minimal viable changes to make the wallet APIs generic on K. This gives us a good starting point on to base the remaining questions of multi-desc wallet designs.

The commits are divided into logical chunks. I couldn't separate the major wallet module refactoring commit easily, so that's still a bit pain to review. Following are the commit summaries; more details are added in the commit message.

• 7aefa75: Removes keychain info from LocalUTXO as it's redundant and forces us to mark LocalUTXO and all other derived structure generic on K.

• aba7b96: Modifies the TxBuilder to only work on the default wallet. Because we don't have a suitable TxParam that can satisfy multi-desc requirements, we keep the TxBuilder default wallet only. This maintains all the previous API and behaviors of TxBuilder. Better multi-desc support for TxBuilder and TxParams can be done on top of this in a future PR.

• ce6ee5c: The Major refactoring change. This changes the wallet API to work with generic K. Creates a default wallet to work with KeychainKind. And adds the following new multi-desc APIs,

  • new_multi_descriptor() : create a multi descriptor wallet.
  • add_descriptor() : Adds a descriptor and marks it with a K.
  • get_address_by_keychain (): Get an address for a given keychain.
  • list_utxos_by_keychain (): does what it says.
  • list_transactions_by_keychain() : ''
  • get_balance_by_keychain() : Get a balance for one keychain. The unconfirmed balance is reported as trusted.
  • get_balance_for_keychains() : Get accumulated balance for a list of keychains. uses should_trust predicate to determine "untrusted" keychains.
  • create_tx_multi() : The multi-descriptor create transaction API.

The rest of the commits are self-explanatory.

Notes to Reviewers

The Balance type is moved out from bdk_chain::keychain module into bdk::types. This is to solve the problem described here #966 (comment). It feels easier to not burden bdk_chain to hand us out the balances. It already gives us all the data we need to calculate the balance, so bdk_chain can be less opinionated about balance calculations and we can handle the required get_balance_* functions at the wallet layer.

The create_tx_multi API takes in a list of keychains to spend from. And a list of change keychains to generate change addresses from. For change_keychains is an iterator instead of a single keychain because later, we wanna add the recipient address matching capability of the change address. There could be other ways of doing it, but as it's the simplest of all the floating solutions, kept the API accordingly. Currently, multiple keychains do nothing, and a single one is selected at random to generate change. This will be handled in a separate PR.

TODO

Tests for basic multi-desc APIs. The newly added APIs and their behaviors should be asserted.

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature

[evanlinjin]

Thanks for the work! These are my initial thoughts after a preliminary run-through.

I still haven't fully conceptualized how we plan to progress with this to get to our desired API.

I would have done things a little differently. I.e. trying to split out TxBuilder completely first. It would be a lot more tedious, but my intuition tells me it leaves us with something cleaner to work with.

Would love to hear everyone's thoughts on this.

[rajarshimaitra]

Thanks for the look.

To give a bit of context. This PR isn't meant to be the final API or the internal design for multi descriptor wallet. There are many potential improvements to make. Some of them I mentioned in the description. But without this, it was hard to figure out what exactly the new designs for other structures should be.

The aim of this PR is to do the basic changes needed to make the wallet generic on keychains. Which itself was a big refactoring change because of the ways bdk internals were written. Due to this, it's hard to understand what needs to be changed/refactored/redesigned in the other parts of the wallet module.

I would have done things a little differently. I.e. trying to split out TxBuilder completely first. It would be a lot more tedious, but my intuition tells me it leaves us with something cleaner to work with.

Completely with you that TxBuilder and wallet decoupling should happen, but that's an orthogonal discussion. And should be handled via another parallel PR. I think @danielabrozzoni and @vladimirfomene has also started thinking about it.

The aim of this PR is to show the refactoring can be done without a lot of scope-creeping with the existing design and can guide us on how the new design for TxBuilder and TxParams should fit in the multi-descriptor world. That's a separate discussion on its own.

[vladimirfomene]

Thanks Raj! We finally have something on which we can have conversations around. I think we missing an API to get balance per keychain, but we can certainly add it later.

[rajarshimaitra]

Thanks for the look Vlad.

I think we missing an API to get balance per keychain, but we can certainly add it later.

Good point. I commented this API out because KeychainTracker doesn't impl balance by keychain. This does provide a good starting point to discuss how we should do it. Keychaintracker won't be used eventually, so maybe we do it over the new IndexedTxGraph API and unlock this one over the wallet once the wallet is ported on the new design?

On the current keychaintracker, it seems pretty easy to do, but it probably will be wasted work. On the IndexedTxGraph it's a bit harder because it works on the FullTxOut which doesn't have keychain info.

Maybe @evanlinjin would have more suggestions on this.

[benthecarman]

Super excited for this one!

[rajarshimaitra]

Pushed some review comment fixes and moved all the remaining wallet APIs (that fits) into the generic definition.

[rajarshimaitra]

• Rearranged the commit history.
• Added the rest of the remaining APIs for multi-desc wallet.
• Cleaned up some rough edges.
• Updated the PR description to reflect current state.

This is ready for review from my side. The changeset should look much simpler now. I will move on to add some basic tests to ensure the new multi-desc APIs work.

[vladimirfomene]

@rajarshimaitra, I also initially thought we will need to decouple the wallet from the transaction builder but I no longer have a strong opinion on that. I think we just need to have a TxParams and TxBuilder work for our multi-descriptor wallet.

[rajarshimaitra]

I think we just need to have a TxParams and TxBuilder work for our multi-descriptor wallet.

I would further decouple this problem and fix the TxParams first for multi-desc. My current understanding is the TxParams is already good enough to serve the basic multi-desc operations, especially because it doesn't deal with keychains. We only need a new multi-desc policy_path field for complex descriptors. Things will be more clear once we have the tests.

[evanlinjin]

To give a bit of context. This PR isn't meant to be the final API or the internal design for multi descriptor wallet. There are many potential improvements to make. Some of them I mentioned in the description. But without this, it was hard to figure out what exactly the new designs for other structures should be.

I'm aware. I'm saying that I don't think this is the correct intermediary step since we have now figured that TxBuilder is deeply coupled with Wallet. Knowing this, I think the most intuitive approach is to first decouple TxBuilder.

Completely with you that TxBuilder and wallet decoupling should happen, but that's an orthogonal discussion. And should be handled via another parallel PR. I think @danielabrozzoni and @vladimirfomene has also started thinking about it.

I'm not opposed to experimenting with the Wallet API while TxBuilder is rebuilt. However, I would want the new TxBuilder merged before merging this PR. Merging this PR in it's current state leaves Wallet with a confused API.

[rajarshimaitra]

I'm not opposed to experimenting with the Wallet API while TxBuilder is rebuilt. However, I would want the new TxBuilder merged before merging this PR. Merging this PR in it's current state leaves Wallet with a confused API.

ACK on this and turning this into draft until we have the new TxBuilder. Happy to open it up later and replay the refactoring on the new APIs.

[philipp1992]

Hi,
can you give this PR more attention please? We need that urgently.
kind regards
Philipp

[notmandatory]

Hi, can you give this PR more attention please? We need that urgently. kind regards Philipp

This can currently be done using the bdk_chain crate but won't be a feature for bdk_wallet until the future 2.0 milestone.

[notmandatory]

A lot has changed with bdk since this PR, but I'm leaving it open for research when we get to doing this for 2.0.

[evanlinjin]

I don't think this should be left open since the approach here is wrong and this PR is very outdated. #1539 is a more sane approach. Additionally, we have ticket bitcoindevkit/bdk_wallet#84 to reference all related PRs open or not.