Feature/multi keychain take 3

[thunderbiscuit]

Take 3 on the multi-keychain PR feature. I'm opening this as a new PR in order to not lose all the work done in #318, since it's a fairly new/reimagined way to work it (2 PRs together, etc.). See #523 for part 1 of this.

Note that this is made against the pre-multi-keychain branch.

Quite experimental at this point, but I wanted to share where I'm at so others can build upon it.

TODO

• I have removed the APIs related to multipath descriptors as they were focused on 2-path descriptors, but we should now be able to handle all multipath. I think this part needed a bit more thinking and I wanted to punt on it for now to focus on the core of the feature. Multipath is still supported, you simply have to "unpack" them before giving them to the KeyRing.
• Figure out how to deal with change in the Wallet::create_psbt API. To keep things simple here I require users always set the keychain explicitly, but it's the sort of thing that users would probably love to be able to set once and forget. The example of wallets that use 2 keychains and one of those is the change keychain is the most obvious one.
• One of the issues left is the Wallet::balance method not being able to tell what's a change keychain and what's not, and so the concept of "trusted" is a bit lost in the current API (all defaults to untrusted until the UTXO is mined). But this is I believe fixed by refactor(wallet): derive balance trust from output ancestry #431 and would land hopefully before this, and so I choose to let this one sit as is for now.

Changelog notice

TODO

Before submitting

• I followed the contribution guidelines
• This PR breaks the existing API

[codecov]

Codecov Report

❌ Patch coverage is 79.74684% with 48 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.20%. Comparing base (9ad7073) to head (eaa0946).

Files with missing lines	Patch %	Lines
src/wallet/persisted.rs	58.82%	14 Missing ⚠️
src/keyring/error.rs	23.07%	10 Missing ⚠️
src/wallet/params.rs	50.00%	8 Missing ⚠️
src/types.rs	68.42%	6 Missing ⚠️
src/wallet/error.rs	0.00%	6 Missing ⚠️
src/wallet/changeset.rs	94.44%	4 Missing ⚠️

Additional details and impacted files

@@                  Coverage Diff                   @@
##           pre-multi-keychain     #524      +/-   ##
======================================================
+ Coverage               81.79%   82.20%   +0.40%     
======================================================
  Files                      25       27       +2     
  Lines                    6362     6371       +9     
  Branches                  289      281       -8     
======================================================
+ Hits                     5204     5237      +33     
+ Misses                   1058     1038      -20     
+ Partials                  100       96       -4     

Flag	Coverage Δ
rust	82.20% <79.74%> (+0.40%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[thunderbiscuit]

A few ideas for interesting helper features to add to the KeyRing:

1. A KeyRing::standard method that takes two descriptors and slots them right into KeychainKind::External and KeychainKind::Internal. This is basically allowing anyone who builds very run-of-the-mill wallets to do it quickly and simply, using the current approach of KeychainKind as their K keychain identifier.
2. A more elegant API for dealing with multipath descriptors. This has been a request for a while and we currently only have so-so methods to work with them, mostly because we only could work with 2 keychains at a time. With the multi-keychain paradigm, really any number of keychains that the multpath descriptors expand to can be added to the wallet. We just need a way to deal with this elegantly (and minimize footguns).

[thunderbiscuit]

TODO: Figure out how to deal with change in the Wallet::create_psbt API. To keep things simple here I require users always set the keychain explicitly, but it's the sort of thing that users would probably love to be able to set once and forget. The example of wallets that use 2 keychains and one of those is the change keychain is the most obvious of those, where clearly the user intends for one of those keychains to be used for change for all transactions.

My current naive idea to deal with this would be to add a field on the wallet change: K where users can define a keychain that the Wallet::create_psbt could always use in case no change keychain is set in the psbt creation.

@ValuedMammal happy to hear your thoughts on this.

One thought on this comment: by default the wallet would currently use UTXOs from all keychains to build transactions (maybe that needs work too; kind of like locking UTXOs, you might want to lock keychains or at least prevent the wallet mixing up all your keychains for privacy reasons), but in any case the base workflow would use all keychains. If each of those had defined a sister change keychain, then the wallet is back at square 1, not really knowing which one to use for change. Here again, explicitly setting the keychain to use for change is preferable IMO.

[thunderbiscuit]

Leaving here for review for tomorrow.

[thunderbiscuit]

Need to confirm where exactly this check will succeed/fail given corner cases, because I know we've discussed this in the past and I need a refresh on it. Posting here for self-review.