bwt icon indicating copy to clipboard operation
bwt copied to clipboard

Add Taproot support

Open craigraw opened this issue 2 years ago • 10 comments

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

craigraw avatar Jul 16 '21 09:07 craigraw

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

dunxen avatar Oct 02 '21 12:10 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.

Seems like the linked PR is merged.

benma avatar Mar 25 '22 13:03 benma

Seems like the linked PR is merged.

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

dunxen avatar Mar 25 '22 16:03 dunxen

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 :)

craigraw avatar Jun 02 '22 06:06 craigraw

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 avatar Jun 02 '22 07:06 dunxen

We're currently blocked by https://github.com/rust-bitcoin/rust-bitcoincore-rpc/pull/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 avatar Jun 02 '22 11:06 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).

dunxen avatar Jun 03 '22 09:06 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.

craigraw avatar Jun 06 '22 08:06 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.

Yes, I do agree. Descriptor wallets will support all of those. We're actually waiting on https://github.com/rust-bitcoin/rust-bitcoincore-rpc/pull/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.

dunxen avatar Jun 06 '22 08:06 dunxen

It appears that https://github.com/bitcoin/bitcoin/pull/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.

craigraw avatar Nov 24 '22 09:11 craigraw