Add support for Taproot and tr() descriptors

[afilini]

Description

This is a work-in-progress PR to update BDK to rust-bitcoin 0.28 which introduces taproot support and a few other improvements. While updating we also introduce taproot support in BDK.

High level list of subtasks for this PR:

• Update rust-bitcoin and rust-miniscript
• Stop using deprecated structs
• Add taproot metadata to psbts
• Produce schnorr signatures
• Finalize taproot txs
• Support tr() descriptors in the descriptor!() macro
• Write a lot of tests
  • Interoperability with other wallets (Core + ?)
    • Signing/finalizing a psbt made by core
    • Producing a psbt that core can sign and finalize
  • Creating psbts
    • Verify the metadata are correct
    • Verify sighashes are applied correctly
    • Create a tx with a foreign taproot and non-taproot utxo
  • Signing psbts
    • Signing for a key spend
    • Signing for a script spend
    • Signing with a single (wif) key
    • Signing with an xprv (with and without knowing the utxo being spent in the db)
    • Signing with weird sighashes
  • Policy module
    • Simple key spend
    • More complex tap tree with a few keys
    • Verify both contribution and satisfaction of a PSBT input
  • Wallet module
    • Generate addresses

Fixes #63

Notes to the reviewers

Milestone

I'm adding this to the 0.19 milestone because now that rust-bitcoin and rust-miniscript have been released we should not waiting too long to release a version of BDK that supports the new libraries.

API Breaks

Since this is an API-break because of the new version of rust-bitcoin and rust-miniscript, I'm also taking the chance to update a few things in our lib that I had been thinking about for a while.

One example is the signer interface, which had that weird sign_whole_tx() method. This has now been removed, and the Signer trait replaced with TransactionSigner and InputSigner. I'm also starting to think that the signer should not only look at the psbt to figure out what to do, but ideally it should also receive some information about the descriptor (for example, the type) to simplify the code.

One option is to add an extra parameter, but that would probably only be used by our internal signers and not much else (for example, if you ask an hardware wallet to sign, it will probably already know what kind of wallet you have).

Another option is to wrap PrivateKey and DescriptorXKey<ExtendedPrivKey> which are the two internal signers we support with a struct that contains metadata about the descriptor, and then implement the signer traits on that struct. We could construct this in Wallet::new(), after miniscript parses the descriptor.

MSRV Bump

Due to the update of rust-electrum-client, which in turn depends on an updated webpki, we will have to bump our MSRV beacuse 1.46 is not supported by the new webpki version.

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 docs for the new feature
• I've updated CHANGELOG.md

[afilini]

I've updated the PR description with a list of ideas for tests that we need for this. The list is non-exhaustive, just a point from which we can start.

[notmandatory]

I've started looking into the "Creating psbts" tests.

Some notes on setting up a local bitcoind -regtest taproot wallet here: https://hackmd.io/@notmandatory/Sy2yOYeI5

[rajarshimaitra]

I am finding a lot of version conflict while working on this bitcoindevkit/bdk-cli#62 (comment) while trying to update bdk-cli with the latest bitcoincore-rpc and electrsd..

I think the first two commit of this PR if can be merged independently of taproot into master would be really helpful..

It seems recently a lot has changed in the upstreams and we are using bitcoincore-rpc and electrum-client from two different sources (directly, and then from electrsd). And when we try to move electrsd from dev dependency all sort of version conflict is appearing in bdk-cli..

But to sort them in bdk-cli side I need to have the final updated version of Cargo.lock in bdk, which is done by the first two commits..

[rajarshimaitra]

It also might make sense if we just make a patch release with the dependency upgrade, then the downstream libraries can fix them up. Or just having them in master would work too for time being.

[afilini]

I don't think it can be released as patch upgrade because it would be a breaking change. I.e. if you depend on bdk 0.18 you would get the update and it would break your code.

I think I agree, I'm gonna open a new PR now with just the upgrade

[afilini]

As discussed in our last meeting on Discord, I've added commit ea165b0 to introduce the concept of a SignerContext, which helps the signer produce the right signature without having to rely on specific fields of the PSBT being set.

Unfortunately since the context is static (reused for all the signatures of the same signer) I couldn't store the list of taproot leaf hashes in there, because those change with every derivation index. So I only store whether the key matches the "internal_key", but then we still rely on the PSBT to figure out the list of leaf hashes to sign for.

[notmandatory]

I'm working on some commits to add to this PR with tests for "Interoperability with other wallets ", see: afilini#4. So far only testing with core (22.0).

[afilini]

I'm working on the "signing psbts" section

[afilini]

I should have addressed all the comments, here's the range-diff: https://gist.github.com/afilini/6e933f087971c83eb3b422a2a8b5f7aa

[afilini]

I still have a few things I'd like to add to this, but I feel like this is growing a bit too much.

We should be done with the tests, how do you feel about merging this the way it is now, and adding more improvements in a follow-up PR? The main one I have in mind is a way to ask the signer to only sign for some specific leaf hashes, or for the internal key, since we currently sign for everything we can.

[notmandatory]

ACK 3d1f72e

I was only able to grok parts of this, but I agree with merging it now as experimental tr() support so we can get it into users/testers hands. This is an amazing amount of work and I hope over time I and other will be able to help @afilini more on contributing to deep changes like this. From what I've seen even bitcoin core descriptor wallets don't yet have tr() support that is as extensive as this PR provides.

[danielabrozzoni]

Core review ACK 3d1f72e - I reviewed the code to the best of my knowledge, and it looks correct. I haven't done any manual testing, other than running cargo test, but I can do that after this is merged.

I'm hyped about taproot being merged in BDK. Nice job!! 🚀

[afilini]

I've opened #616 so that we don't forget the extra stuff that we can add after this pr.

I'll wait for a comment from @rajarshimaitra before merging, I'm assuming he must be close to the end since he started looking at the macro stuff.

[rajarshimaitra]

tACK 8ce8da1

[rajarshimaitra]

tACK 3d1f72e

All tests and changes look good so far.. I think its good to get merged as experimental taproot capabilities..

[rajarshimaitra]

Why we are removing this line?

[afilini]

Good catch, this was probably a mistake in the rebase

[afilini]

I noticed a new release of rust-bitcoin came out, I'm not sure what's new there but just in case I've updated the Cargo.toml to use that new version.

I've also updated the changelog with a brief list of what we support related to taproot.

[notmandatory]

I noticed a new release of rust-bitcoin came out, I'm not sure what's new there but just in case I've updated the Cargo.toml to use that new version.

Looks like the new rust-bitcoin 0.28.1 is only to make an exception and raise the MSRV to 1.47 for CI testing no_std, so probably doesn't affect bdk but won't hurt to specify as our min version.