Taproot support #985

darosior · 2024-02-27T12:46:51Z

This introduces Taproot support in the Liana daemon / core library.

We start by introducing support for tr() descriptors alongside wsh() descriptors in the Taproot modules. For Taproot-Liana descriptors whose primary spending path isn't a single key, we deterministically derive an unspendable internal key as per https://delvingbitcoin.org/t/unspendable-keys-in-descriptors/304/21. This is to allow signing devices to not display the internal key as a spendable key when verifying a descriptor. Currently signing device vendors signaled willingness to implement this scheme.

We then adapt the PSBT management logic to use Taproot fields when necessary and upgrade the hot signer to provide Schnorr signatures depending on the PSBT information.

Finally, the functional tests are adapted to be able to run the whole test suite under either P2WSH or Taproot. This is done in a somewhat hacky way as i bailed out of re-implementing a Taproot PSBT signer and finalizer in Python after implementing TapMiniscript support in upstream python-bip380. Instead we use a small Rust program to sign PSBTs for now and we skip a test which explicitly requires an external finalizer.

darosior · 2024-02-27T12:49:17Z

I still need to:

Cleanup the bugfixes to Quick and dirty TapMiniscript support darosior/python-bip380#23 i've got locally and release a new version to update it in our functional tests;
Merge some preparatory PRs;
Update the CI to run functional tests under both P2WSH and Taproot.

I should open a few issues for needed followups before i forget about it.

darosior · 2024-02-27T13:29:33Z

Rebased on master now the PRs this depended on were merged.

darosior · 2024-02-27T15:46:18Z

Alright, i updated python-bip380 and the CI to run the Taproot tests. Looks like one of them is a bit flaky, i'll have a look later. In the meantime it's ready for a first round of review!

darosior · 2024-03-11T10:31:30Z

So it wasn't that one was flaky, it's just the migration test that we need to disable under Taproot. Obviously older versions can't start with a Taproot descriptor.

darosior · 2024-03-11T13:43:31Z

Rebased on master. I adapted the tests for the selection of unconfirmed coins from #873 in a separate commit, as it was less mechanic / obvious than the other tests.

What i still need to investigate:

test_coin_selection is very slightly flaky. The assertion of the vsize of the ancestor might sometimes fail. This is unrelated to this PR, but can potentially make CI fail here (tracked here: Flaky test_coin_selection #1000);
it seems the performance of createspend has been greatly reduced. We might see some timeout on functional tests making heavy use of this call. I need to profile it i suppose. But my hunch is rust-miniscript's way of populating a PSBT input is inefficient.

jp1ac4

I went through most of the changes and found a couple of small typos in comments. Otherwise, the code made sense according to my limited understanding of Taproot :)

tests/tools/taproot_signer/src/main.rs

src/descriptors/mod.rs

darosior

Just did a mechanical review. In addition to checking the correctness of what's here we need to think about what's missing.

src/signer.rs

src/descriptors/mod.rs

darosior · 2024-03-12T14:29:54Z

src/descriptors/mod.rs

+    // avoid having to duplicate the cumbersome logic here. Could use translate_pk() instead in the
+    // future.
+    fn definite_desc(&self) -> descriptor::Descriptor<descriptor::DefiniteDescriptorKey> {
+        descriptor::Descriptor::<_>::from_str(&self.0.to_string()).expect("Must roundtrip")


3017b88 fuzz: fuzzing integrations, fuzz the descriptors module (Antoine Poinsot) f7924fb descriptors: lifting *can* fail (Antoine Poinsot) Pull request description: This introduces fuzzing into our project with two fuzz targets exercising our descriptor parsing logic. See the commit messages for details. This found a crash (first commit). This was motivated by testing the work on Taproot (#985). ACKs for top commit: darosior: ACK 3017b88 - it's not interfering with anything in the repo, been running these for half a day with no crash. Tree-SHA512: 25c1b64a86585fc5f676c3526e2dae945b74c6b0cb4ce2d9db33dc48aa85aaa11a07b279838703d62c9ca00cf39cc34577ca19c0a8f9aaf5327266eb7be6dce0

tests/test_spend.py

It'll make it easier to switch to update Taproot info.

darosior · 2024-03-13T16:06:29Z

Rebased on master. I found another crash using the fuzz tests which is now fixed. Unfortunately the compiler is insanely slow (like more than 1000x slower) for Taproot which makes the descriptors target useless.

src/descriptors/mod.rs

src/descriptors/analysis.rs

It will be useful when we introduce Taproot support in the next commit.

We introduce support for tr() descriptors alongside wsh() descriptors in creating (compiling from policy, parsing from string) and working with (analyizing its policy, getting spend information) a descriptor. When compiling a Taproot descriptor, if no key from the policy could be used as single internal key we deterministically generate an unspendable internal key as per https://delvingbitcoin.org/t/unspendable-keys-in-descriptors/304/21. Similarly when lifting the policy of a Taproot descriptor, if the internal key matches the deterministic unspendable key for this descriptor we discard it from the analysis. To fill information about an output for signers, we re-use rust-miniscript PSBT input updated instead of re-inventing the wheel. It does necessitate a hack however to use a type they would accept. We don't change the "max size of a spending input" for now, even though it means we would significantly overpay fees for descriptors with a spendable internal key.

TapMiniscript support was only released in 26.0: https://bitcoincore.org/en/releases/26.0

It's more robust and bitcoind serializes `wsh()` descriptors using `'` as hardened marker and `tr()` descriptors using `h`..

We introduce Taproot support in the test framework through a global toggle. A few modifications are made to some tests to adapt them under Taproot (notably the hardcoded fees / amounts). This is based on my introduction of a quick and dirty support for TapMiniscript in my python-bip380 library: darosior/python-bip380#23. In addition to this i didn't want to implement a signer in the Python test suite so here we introduce a simple Rust program based on our "hot signer" which will sign a PSBT with an xpriv provided through its stdin and output the signed PSBT on its stdout. Eventually it would be nicer to have a Python signer instead of having to call a program. The whole test suite should pass under both Taproot and P2WSH. Only a single test is skipped for now under Taproot since it needs a finalizer in the test suite. I also caught a bug in the RBF tests which i fixed in place.

They don't add much value and cause flakiness because the size might sometimes be lower than that due to low-R signature grinding. Fixes wizardsardine#1000.

darosior · 2024-03-14T10:17:36Z

ACK 4bb2372 -- this underwent multiple rounds of review, Edouard tested it in his follow-up PR to the GUI and i wrote a couple fuzz targets exercising part of the logic introduced here

52c3613 descriptors: adapt 'change_indexes()' to Taproot (Antoine Poinsot) 0c65d20 descriptors: encapsulate change_indexes unit test (Antoine Poinsot) Pull request description: I remember thinking about this helper when working on #985, but for some reason i didn't address it there. Here it is. ACKs for top commit: edouardparis: utACK 52c3613 Tree-SHA512: 276fdb4087ba0ec097142d7ff7a4c8762814d6423b9f5535c8951d6ef628c164e4a5191aeb9aa48eaad9afd1361416157774341ef7873fee284d039f9dcf5b3e

b7f35c0 Add installer dropdown for advanced settings (edouardparis) 59a4b18 fix: merge tap_script_sigs from signed psbt (edouardparis) 02a52b9 add ledger version support for tapminiscript (edouardparis) 2debb32 Add taproot support to installer descriptor editor step (edouardparis) 8bc0cac gui: async-hwi:0.0.16 (edouardparis) 4a4c78d bump liana:master (edouardparis) Pull request description: based on #985 ACKs for top commit: darosior: tACK b7f35c0 -- i've lightly tested this a couple times. It's good enough to get in and get tested along with the other changes. Tree-SHA512: 4481be4797cf6fa901de9fec989837381c7817d18dd2c9a45ff1802a15982d1251696e577fb23da2d4ca9f0ed27044dade28f0e7b56703594dae4b3a5065306b

darosior force-pushed the 2401_taproot branch from a491825 to 6da8714 Compare February 27, 2024 13:28

edouardparis mentioned this pull request Feb 27, 2024

Gui taproot #986

Merged

darosior force-pushed the 2401_taproot branch 3 times, most recently from 35a8003 to 98a81b4 Compare February 27, 2024 15:26

darosior marked this pull request as ready for review February 27, 2024 15:46

darosior force-pushed the 2401_taproot branch from 98a81b4 to 0e034e0 Compare March 11, 2024 10:29

darosior force-pushed the 2401_taproot branch from 0e034e0 to 39735ff Compare March 11, 2024 13:38

jp1ac4 mentioned this pull request Mar 11, 2024

Flaky test_coin_selection #1000

Closed

jp1ac4 reviewed Mar 12, 2024

View reviewed changes

tests/tools/taproot_signer/src/main.rs Outdated Show resolved Hide resolved

src/descriptors/mod.rs Outdated Show resolved Hide resolved

darosior commented Mar 12, 2024

View reviewed changes

darosior force-pushed the 2401_taproot branch from 39735ff to 7f4d1eb Compare March 12, 2024 14:50

darosior mentioned this pull request Mar 13, 2024

Fuzzing integration #1004

Merged

jp1ac4 reviewed Mar 13, 2024

View reviewed changes

tests/test_spend.py Outdated Show resolved Hide resolved

tests/test_spend.py Outdated Show resolved Hide resolved

descriptor: encapsulate PSBT in/out information update

85fdc40

It'll make it easier to switch to update Taproot info.

darosior force-pushed the 2401_taproot branch 2 times, most recently from 7086124 to 1639ddf Compare March 13, 2024 16:03

edouardparis reviewed Mar 13, 2024

View reviewed changes

src/descriptors/mod.rs Outdated Show resolved Hide resolved

src/descriptors/analysis.rs Outdated Show resolved Hide resolved

darosior added 5 commits March 13, 2024 19:21

descriptors: encapsulate key matching logic

c897d41

It will be useful when we introduce Taproot support in the next commit.

config: deser_from_str isn't descriptor specific.

6cf8eaa

config: unit test a valid config with a Taproot descriptor

d3b7e4c

bitcoind: sanity check min supported version for Taproot descriptor

602c862

TapMiniscript support was only released in 26.0: https://bitcoincore.org/en/releases/26.0

darosior added 11 commits March 13, 2024 19:21

bitcoind: compare descriptors, not their string representation.

8596ca7

It's more robust and bitcoind serializes `wsh()` descriptors using `'` as hardened marker and `tr()` descriptors using `h`..

spend: don't populate non_witness_utxo for Taproot

e05039f

spend: check for either p2wsh or Taproot sigs in sanity checks

714bd3c

command: also update Taproot sigs in 'updatespend'

d622258

signer: move p2wsh signing into a dedicated function

80a7dc3

signer: taproot support in hot signer

96d30db

qa: adapt hardcoded coin selection tests to Taproot

687a0c2

ci: run functional tests both under Taproot and P2WSH

b9a4625

fuzz: alternate between generating wsh() and tr() descs

7eb024a

qa: drop specific value assertions in coin selection test

4bb2372

They don't add much value and cause flakiness because the size might sometimes be lower than that due to low-R signature grinding. Fixes wizardsardine#1000.

darosior force-pushed the 2401_taproot branch from 1639ddf to 4bb2372 Compare March 13, 2024 18:23

This was referenced Mar 14, 2024

Under Taproot don't (always) use the max satisfaction weight for a script #1006

Open

Have a proper Taproot signer in the functional tests #1007

Open

Have a Taproot finalizer in the functional test framework #1008

Open

darosior merged commit b22f22f into wizardsardine:master Mar 14, 2024
21 checks passed

darosior deleted the 2401_taproot branch March 14, 2024 10:21

darosior mentioned this pull request Mar 20, 2024

descriptors: change detection under Taproot #1020

Merged

darosior mentioned this pull request Mar 22, 2024

Taproot support #55

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Taproot support #985

Taproot support #985

darosior commented Feb 27, 2024

darosior commented Feb 27, 2024 •

edited

Loading

darosior commented Feb 27, 2024

darosior commented Feb 27, 2024

darosior commented Mar 11, 2024

darosior commented Mar 11, 2024 •

edited

Loading

jp1ac4 left a comment

darosior left a comment

darosior Mar 12, 2024

darosior commented Mar 13, 2024

darosior commented Mar 14, 2024

Taproot support #985

Taproot support #985

Conversation

darosior commented Feb 27, 2024

darosior commented Feb 27, 2024 • edited Loading

darosior commented Feb 27, 2024

darosior commented Feb 27, 2024

darosior commented Mar 11, 2024

darosior commented Mar 11, 2024 • edited Loading

jp1ac4 left a comment

Choose a reason for hiding this comment

darosior left a comment

Choose a reason for hiding this comment

darosior Mar 12, 2024

Choose a reason for hiding this comment

darosior commented Mar 13, 2024

darosior commented Mar 14, 2024

darosior commented Feb 27, 2024 •

edited

Loading

darosior commented Mar 11, 2024 •

edited

Loading