Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bitcoin: add support for tr() wallet policies/descriptors #1231

Merged
merged 3 commits into from
Sep 24, 2024

Conversation

benma
Copy link
Collaborator

@benma benma commented Jun 14, 2024

See commit msg.

This can also be tested using

make -j simulator

then in in this branch of bitbox-api-rs: BitBoxSwiss/bitbox-api-rs#80:

SIMULATOR=/path/to/bitbox02-firmware/build-build/bin/simulator cargo test --features=simulator,tokio

@benma benma force-pushed the tapminiscript branch 7 times, most recently from e1ebd6a to e481155 Compare June 19, 2024 10:21
@benma benma force-pushed the tapminiscript branch 2 times, most recently from 86d08c5 to f2dcb4e Compare June 25, 2024 07:59
@benma benma requested a review from Beerosagos July 18, 2024 09:26
@benma benma marked this pull request as ready for review July 18, 2024 09:26
@benma benma force-pushed the tapminiscript branch 3 times, most recently from 9e5112c to e3b8167 Compare July 18, 2024 09:41
@benma
Copy link
Collaborator Author

benma commented Jul 30, 2024

Pushed another commit to implement detection of provably unspendable Taproot internal keys, see commit msg and comments for more infos.

@benma benma force-pushed the tapminiscript branch 3 times, most recently from a4fca57 to 474de0d Compare July 30, 2024 12:25
@benma benma force-pushed the tapminiscript branch 2 times, most recently from 4997ec4 to 3ef2408 Compare September 20, 2024 09:19
@benma
Copy link
Collaborator Author

benma commented Sep 20, 2024

Rebased

Copy link
Collaborator

@Beerosagos Beerosagos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK with a few comments

test/unit-test/test_keystore.c Show resolved Hide resolved
src/keystore.c Outdated Show resolved Hide resolved
src/rust/bitbox02-rust/src/hww/api/bitcoin/policies.rs Outdated Show resolved Hide resolved
@benma benma force-pushed the tapminiscript branch 2 times, most recently from a9bb383 to 11bb9a0 Compare September 24, 2024 12:04
Support for Taproot wallet policies:

`tr(INTERNALKEY,{TREE})` where `TREE=SCRIPT` or `TREE={TREE,TREE}`.

SCRIPT can be an arbitrary miniscript like we already suppot in
`wsh(<miniscript>)` policies, with slight adaptations for the tr()
context.

References:
- https://github.com/bitcoin/bips/blob/master/bip-0388.mediawiki
- https://github.com/bitcoin/bitcoin/blob/efbf4e71ce8e3cd49ccdfb5e55e14fa4b338453c/doc/descriptors.md

`tr(@0/**)` is equivalent to BIP-86 that we already support as a
SimpleType (single signature).

The Taproot output key (which is in the Taproot address) is
`internalkey + tweak` where tweak is either a standard tweak if no
tree is present, or the merkle root hash of the tree.

A tr() UTXO can be spent by signing with the private key of the output
key (i.e. the tweaked private key of the internal key), called a key path
spend, or by providing and satisfying a script that is a leaf of the
tree, called a script path spend.

When spending using the the output key, it is the same as BIP-86 and
the BIP341 sighash computation is the same. When spending using a leaf
script, the sighash algo is extended - see the changes to bip341.rs
and the documentation that is referenced there.

We change keystore_secp256k1_schnorr_bip86_sign to
keystore_secp256k1_schnorr_sign, taking the tweak as an argument
instead, which we feed from signtx.rs depending on whether we are
spending BIP-86, a policy with/without a tree.

This commit adds significant binary bloat (~47kB), in large part
because rust-miniscript uses generics, so Miniscript<Tr> duplicates a
lot of the code of Miniscript<Wsh>, even though it is nearly
identical. This can be solved over time in rust-miniscript to reduce
the binary size cost of this feature.

With these commits since the previous release, we also reduced space
by more than this feature pulls in:

daa745f
debb871
2fa257c
61a82ff

```
749548	build/bin/firmware.bin (based on this commit)
753272	firmware.v9.19.0.bin
```
In Taproot policies, it is a common pattern to use an unspendable
internal public key if one only wants to use script path spends, e.g.

    tr(UNSPENDABLE,{...})

https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#constructing-and-spending-taproot-outputs
decribes that one could use the NUMS point for that:

> One example of such a point is H = lift_x(0x50929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0)
which is constructed by taking the hash of the standard uncompressed
encoding of the secp256k1 base point G as X coordinate.

Wallet policy keys however must be xpubs, and also it is not desirable
to use the NUMS point, as described in
https://delvingbitcoin.org/t/unspendable-keys-in-descriptors/304:

> 1. unspendable keys should be indistinguishable from a random key for
an external observer;
> 2. in a descriptor with the range operator (like the wallet policies
> compatible with most known wallet account formats), each
> change/address_index combination must generate a different unspendable
> pubkey, and they should not be relatable to each other (in order to
> avoid fingerprinting);

The proposal in
https://delvingbitcoin.org/t/unspendable-keys-in-descriptors/304/21 to
use an xpub with the NUMS public key and a chain_code derived as the
hash from the xpubs in the descriptor was adopted by Liana
wallet. This commit implements this. Note that even though this
proposal it not a standard yet, it is still provably unspendable, so
we can display this info to the user. A future standard to achieve the
same can be included later.
@benma benma merged commit 3dfb8fa into BitBoxSwiss:master Sep 24, 2024
4 checks passed
@benma benma deleted the tapminiscript branch September 24, 2024 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants