Add Taproot support

[craigraw]

Although I expect this will require some of the dependencies to be updated first, I would like to start by adding this issue to request support for Taproot (P2TR) wallets.

I am currently getting the following two errors when connecting Sparrow v1.4.3 to Bitcoin Core 0.21.1 over BWT:

dev.bwt.libbwt.BwtException: JSON-RPC error: JSON decode error: bech32: invalid checksum

dev.bwt.libbwt.BwtException: Invalid config: unexpected «tr(1 args) while parsing Miniscript» at line 1 column 817

[dunxen]

Yes, looks like taproot descriptor support in rust-miniscript is still in progress: rust-bitcoin/rust-miniscript#267
I think I'll be able to help out when there's a new release for it.

[benma]

Yes, looks like taproot descriptor support in rust-miniscript is still in progress: rust-bitcoin/rust-miniscript#267 I think I'll be able to help out when there's a new release for it.

Seems like the linked PR is merged.

[dunxen]

Seems like the linked PR is merged.

Yeah, I've started working on it while waiting on a new release of rust-miniscript.

[craigraw]

Any progress on this? rust-miniscript 7.0.0 was released on 20 April with support for tr descriptors. It also includes the type system bug fix, so it seems like a good idea to upgrade. It would be good to support Taproot wallets on Bitcoin Core in Sparrow :)

[dunxen]

It also includes the type system bug fix, so it seems like a good idea to upgrade.

Yes, I think that was the last issue I ran into. Edit: Nope different issue with rust-bitcoin but I recall it's also sorted.

I'm going to tackle this from today again. I think everything upstream should be good now 🤞

[dunxen]

We're currently blocked by rust-bitcoin/rust-bitcoincore-rpc#216 when using Bitcoin Core 23.0+. I'm working with 22.0 for now but it would be great to have that merged so there's no confusion there for users.

[dunxen]

So it looks like there's no way to import bech32m addresses at all in legacy wallets. Descriptor wallets must be used for all taproot transactions and they're the default in Core v23.0. rust-bitcoincore-rpc does not yet have any descriptor wallet RPCs (such as importdescriptors) so we'd need to wait for that and a few other loose ends or just add them to bitcoincore_ext.rs and do a bit of refactoring (which can be awkward if we want to keep supporting the legacy wallet type).

[craigraw]

It would be good to support descriptor-based wallets in BWT. I believe all ways of adding addresses (descriptors, xpubs and addresses) could be supported. There may need to be a way of resolving a conflict if an existing, non-descriptor-based wallet already exists with the same name, possibly by allowing a client to specify whether to update (by deleting and re-adding) an existing wallet if present, providing the version of Core supports it. I'd also be happy with dropping legacy wallet support altogether if necessary.

[dunxen]

It would be good to support descriptor-based wallets in BWT. I believe all ways of adding addresses (descriptors, xpubs and addresses) could be supported. There may need to be a way of resolving a conflict if an existing, non-descriptor-based wallet already exists with the same name, possibly by allowing a client to specify whether to update (by deleting and re-adding) an existing wallet if present, providing the version of Core supports it. I'd also be happy with dropping legacy wallet support altogether if necessary.

Yes, I do agree. Descriptor wallets will support all of those. We're actually waiting on rust-bitcoin/rust-bitcoincore-rpc#199 (with requested changes) and then I think it's good to go. The easiest is to drop support for legacy wallets entirely. I like the idea of helping resolve conflicts in that case.

Edit: I'll base off the above with changes and start working with that branch on something useable and see how we like/dislike the handling of legacy wallets.

[craigraw]

It appears that bitcoin/bitcoin#25504 (and thus Bitcoin Core 24.0 or higher) will be necessary if using the descriptors passed to BWT and not just adding raw addresses to a descriptor wallet. Without the include_change parameter that has been added, listsinceblock does not return self-transfers within a wallet.