feat: expose wallet signing with provided signers

[reez]

#1030 first

Description

Adds wrapper for bdk_wallet::Wallet::sign_with_signers, allowing bindings consumers to sign a PSBT with explicit SignersContainer values instead of only the wallet-owned signers.

This also adds a SignersContainer wrapper that can be constructed from descriptor key material and passed into the new wallet signing method.

Notes to the reviewers

Documentation

• bdk_wallet
  https://docs.rs/bdk_wallet/3.1.0/bdk_wallet/struct.Wallet.html#method.sign_with_signers
• bitcoin
• uniffi
• Other:

Changelog

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing
• I've added exactly one changelog:* label
• I've linked the relevant upstream docs or specs above

New Features:

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

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[thunderbiscuit]

I have one small question regarding the usefulness of the is_empty method, but otherwise this is ready to go.

Because it's one of the core new features/workflow for the library and hints at where we're going moving forward (creating wallets using only public keys and whatnot (see the upcoming Design page on this topic), I'd like to have it expanded upon in tests if possible.

I'd be happy to add tests if you'd like me to push on this branch @reez. That will get me playing with the API a bit more too. But they could also come in follow-up PRs, I'm not picky.

[thunderbiscuit]

Is it possible for us to hit a false here? The way our constructor works, it appears you'll always have one, unless maybe the public descriptors sort of silently fail and don't populate/produce a valid signer?

The Rust type has a SignersContainer::new constructor which creates one without any signers in it, but here we're not offering that (and I'm not sure why we would either even if we could). https://docs.rs/bdk_wallet/latest/bdk_wallet/signer/struct.SignersContainer.html#method.new

[reez]

Is it possible for us to hit a false here?

Im looking at it like secret descriptors make is_empty() return false, public descriptors make it return true

The way our constructor works, it appears you'll always have one,

Only when the descriptor contains secret key material, public only descriptors have an empty key_map so that's how I see the constructor can produce a container with no signers

unless maybe the public descriptors sort of silently fail and don't populate/produce a valid signer?

I don't think they silently fail, a public descriptor is valid but it doesnt have secret key material in its key_map so there's just nothing for SignersContainer::build(...) to turn into a signer

The Rust type has a SignersContainer::new constructor which creates one without any signers in it, but here we're not offering that (and I'm not sure why we would either even if we could).

Agreed

[reez]

Added explicit coverage in test_signers_container_from_descriptor now

[reez]

I'd be happy to add tests if you'd like me to push on this branch @reez. That will get me playing with the API a bit more too. But they could also come in follow-up PRs, I'm not picky.

Still have this in draft because I need to update it to make sure it's easy to review after breaking it out from the larger pr, so just keep that in mind but if you do want to check it out and have feedback feel free to post it and I can make the updates myself as I get this pr in cleanest shape

[reez]

addressed questions (I think all of them?) from #1030 in a few of the follow up commits here

I'll squash everything into one commit when I merge it though for sure

Just FYI

[reez]

I think this is ready for final review @thunderbiscuit

I can squash commits when merge, but just leaving commits separate for review so its easier to see what changes where made and when, especially since I split this out of a mega pr that might have had some related comments on it

[thunderbiscuit]

ACK 1c8ac28. I also agree that the commit history could be cleaned up a bit before merging.

While trying this out locally I found the flow between public and private descriptors is not as smooth as it could be, especially since this is expected to be the new workflow for users. Currently instead of having access to a direct conversion from a private descriptor to it's public equivalent, you roughly do this:

val privateDescriptor = Descriptor.newBip86(secretKey, KeychainKind.EXTERNAL, NetworkKind.TEST)
val publicDescriptor = Descriptor(privateDescriptor.toString(), NetworkKind.TEST)

There are other ways to do it to, but they're even worse (create the private descriptor, get the xpub, find the fingerprint, and feed them to the Descriptor.newBip84Public API for example).

Food for thoughts for further PRs!

[reez]

ACK 1c8ac28. I also agree that the commit history could be cleaned up a bit before merging.

While trying this out locally I found the flow between public and private descriptors is not as smooth as it could be, especially since this is expected to be the new workflow for users. Currently instead of having access to a direct conversion from a private descriptor to it's public equivalent, you roughly do this:

val privateDescriptor = Descriptor.newBip86(secretKey, KeychainKind.EXTERNAL, NetworkKind.TEST)
val publicDescriptor = Descriptor(privateDescriptor.toString(), NetworkKind.TEST)

There are other ways to do it to, but they're even worse (create the private descriptor, get the xpub, find the fingerprint, and feed them to the Descriptor.newBip84Public API for example).

Food for thoughts for further PRs!

adding a follow up here #1054