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

Add rawtr() descriptor for P2TR with specified (tweaked) output key #23480

Merged
merged 3 commits into from
Aug 9, 2022

Conversation

sipa
Copy link
Member

@sipa sipa commented Nov 10, 2021

It may be useful to be able to represent P2TR outputs in descriptors whose script tree and/or internal key aren't known. This PR does that, by adding a rawtr(KEY) descriptor, where the KEY represents the output key directly. If the private key corresponding to that output key is known, it also permits signing with it.

I'm not convinced this is desirable, but presumably "tr(KEY)" sounds more intended for direct use than "rawtr(KEY)".

doc/descriptors.md Outdated Show resolved Hide resolved
@sipa
Copy link
Member Author

sipa commented Nov 11, 2021

@apoelstra @real-or-random @jonasnick @sanket1729 Perhaps the documentation here needs some disclaimer at least about the potential pitfalls of using it directly to construct a wallet. It's not intended for that purpose, only for things like representing otherwise incomplete knowledge in e.g. inference, but adding it to the descriptor language does make it available for all purposes. Can you help think about examples/pitfalls that could be listed?

@sanket1729
Copy link
Contributor

Agreed that the documentation needs a disclaimer. The two drawbacks that I can think of.

  1. This one is listed in the BIP 341: If people directly use output keys in aggregation using proofs of knowledge.(https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#cite_ref-22-0)
  2. In future protocol constructions, it might be desirable to prove that there is no script path given TxOut/ScriptPubKey/Address. This is not possible when using rawtr(), but is possible with tr() by revealing the internal key.

Some more points for discussion:

  • Can we separate descriptors into two types in the spec itself? So, we can have addr, raw, rawtr into the infer-only category. The rest into another category that we can sign. In the infer only category, we can restrict the KEY expression to be a concrete public key without any BIP32 derivations or wildcards.

  • Correct me if I am wrong if the only use of inferred descriptors is that I can give it another party for watching a particular txout for me without revealing/knowing how the txout was constructed. So, assuming we disallow signing rawtr() (to encourage the usage of tr ) is there is any use of rawtr() over raw()?

@real-or-random
Copy link
Contributor

The only pitfall I see is the one mentioned by @sanket1729 : rawtr() is potentially incomplete information because there could always be another way to spend that you don't know about... But that's exactly its feature, so I guess it's fine. Anyway, I think if you do "raw" stuff, you should be know what you're doing.

@Sjors
Copy link
Member

Sjors commented Nov 12, 2021

  • is there is any use of rawtr() over raw()?

I second that question.

@sipa
Copy link
Member Author

sipa commented Nov 12, 2021

Right; I guess the only advantage of rawtr over raw/addr is dealing with cases where the public key is either actually untweaked (and e.g. you need want to provide origin information along with it) or it is tweaked in an unknown way, but you have the tweaked private key.

raw() in those cases cannot represent all information you have, while rawtr can.

@apoelstra
Copy link
Contributor

Agree with Sanket's comments. I also agree that having raw in the name should be sufficient warning for users to avoid this, especially since it's not any harder to use tr.

Regarding the usefulness, I think it's definitely useful for "infer-only" cases like running scanutxoset. May also be useful for recovery scenarios ... although it's hard for me to think of a case where you've generated a weird Taproot key but somehow need Bitcoin Core to spend it.

@sipa
Copy link
Member Author

sipa commented Nov 23, 2021

@apoelstra https://twitter.com/achow101/status/1459617340123926534 may or may not have been related. Arguably, not a good justification, but a use case nonetheless...

@apoelstra
Copy link
Contributor

Not the worst usecase :) I use vanity addresses in Liquid test networks sometimes to make logs easier to read and having descriptor support in Core could plausibly make that better.

@sanket1729
Copy link
Contributor

A usecase for rawtr. Note that this protocol is not reviewed(and might be insecure), but I am starting to think we don't want to preclude people from using new ways to create output keys. In this protocol, it is still possible to prove that there is no script spend using a different way than revealing the internal key. Such proof is not required for the correctness above protocol, but it highlights that there might be other ways to prove the non-existence of script spend than revealing the internal key.

Although as I mentioned later in response comment, this can also be made compatible with tr descriptor. But doing so would be inefficient and it offers no benefit because we can already prove that there is no script spend.

Overall, I am leaning towards a concept ACK for this.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 26, 2022

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #24897 ([Draft / POC] Silent Payments by w0xlt)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

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

Should tr(xpriv) and rawtr(xpriv) return different addresses ?

I created two blank wallets and imported tr(xpriv) into one and rawtr(xpriv) into the other. The xpriv was the same.

The addresses have the same desc property but not the same address, as seen below.

There are also some fields missing from the address generated from rawtr(xpriv) like timestamp.

$ ./src/bitcoin-cli -regtest -named createwallet wallet_name=tr_test blank=true

$ ./src/bitcoin-cli -regtest importdescriptors "[{\"desc\": \"tr(tprv8ZgxMBicQKsPdKziibn63Rm6aNzp7dSjDnufZMStXr71Huz7iihCRpbZZZ6Voy5HyuHCWx6foHMipzMzUq4tZrtkZ24DJwz5EeNWdsuwX5h/*)#223d6gp6\", \"active\": true, \"timestamp\": \"now\"}]"

$ ./src/bitcoin-cli -regtest -named createwallet wallet_name=rawtr_test blank=true

$ ./src/bitcoin-cli -regtest -rpcwallet="rawtr_test" importdescriptors "[{\"desc\": \"rawtr(tprv8ZgxMBicQKsPdKziibn63Rm6aNzp7dSjDnufZMStXr71Huz7iihCRpbZZZ6Voy5HyuHCWx6foHMipzMzUq4tZrtkZ24DJwz5EeNWdsuwX5h/*)#223d6gp6\", \"active\": true, \"timestamp\": \"now\"}]"

$ ./src/bitcoin-cli -regtest -rpcwallet="tr_test" -named getnewaddress address_type='bech32m'
bcrt1p8eafa4hnyfae7kyc42ehndduu49zfuhneerma58j2xexc6lx5wxsm8e5j7

$ ./src/bitcoin-cli -regtest -rpcwallet="rawtr_test" -named getnewaddress address_type='bech32m'
bcrt1pqwnxn65jduuptqhvfgqqh9rjh29pwdrlt7c4nmwafgrsx6n8rr4supy9dh

$ ./src/bitcoin-cli -regtest -rpcwallet="rawtr_test" getaddressinfo "bcrt1pqwnxn65jduuptqhvfgqqh9rjh29pwdrlt7c4nmwafgrsx6n8rr4supy9dh"
{
  "address": "bcrt1pqwnxn65jduuptqhvfgqqh9rjh29pwdrlt7c4nmwafgrsx6n8rr4supy9dh",
  "scriptPubKey": "512003a669ea926f381582ec4a000b9472ba8a17347f5fb159eddd4a07036a6718eb",
  "ismine": true,
  "solvable": true,
  "desc": "rawtr([8324d0e9/0]03a669ea926f381582ec4a000b9472ba8a17347f5fb159eddd4a07036a6718eb)#5rk6yanl",
  "parent_desc": "rawtr(tpubD6NzVbkrYhZ4Wo2WcFSgSqRD9QWkGxddo6WSqsVBx7uQ8QEtM7WncKDRjhFEexK119NigyCsFygA4b7sAPQxqebyFGAZ9XVV1BtcgNzbCRR/*)#x2xz0lq0",
  "iswatchonly": false,
  "isscript": true,
  "iswitness": true,
  "witness_version": 1,
  "witness_program": "03a669ea926f381582ec4a000b9472ba8a17347f5fb159eddd4a07036a6718eb",
  "ischange": false,
  "labels": [
    ""
  ]
}

$ ./src/bitcoin-cli -regtest -rpcwallet="tr_test" getaddressinfo "bcrt1p8eafa4hnyfae7kyc42ehndduu49zfuhneerma58j2xexc6lx5wxsm8e5j7"
{
  "address": "bcrt1p8eafa4hnyfae7kyc42ehndduu49zfuhneerma58j2xexc6lx5wxsm8e5j7",
  "scriptPubKey": "51203e7a9ed6f3227b9f5898aab379b5bce54a24f2f3ce47bed0f251b26c6be6a38d",
  "ismine": true,
  "solvable": true,
  "desc": "tr([8324d0e9/0]03a669ea926f381582ec4a000b9472ba8a17347f5fb159eddd4a07036a6718eb)#05zmkxzl",
  "parent_desc": "tr(tpubD6NzVbkrYhZ4Wo2WcFSgSqRD9QWkGxddo6WSqsVBx7uQ8QEtM7WncKDRjhFEexK119NigyCsFygA4b7sAPQxqebyFGAZ9XVV1BtcgNzbCRR/*)#fdlr9alf",
  "iswatchonly": false,
  "isscript": true,
  "iswitness": true,
  "witness_version": 1,
  "witness_program": "3e7a9ed6f3227b9f5898aab379b5bce54a24f2f3ce47bed0f251b26c6be6a38d",
  "ischange": false,
  "timestamp": 1646090838,
  "hdkeypath": "m/0",
  "hdseedid": "0000000000000000000000000000000000000000",
  "hdmasterfingerprint": "8324d0e9",
  "labels": [
    ""
  ]
}

@sipa
Copy link
Member Author

sipa commented Mar 24, 2022

@w0xlt Yes, that'd definitely expected. rawtr(KEY) literally puts the key in the scriptPubKey directly. tr(KEY) will construct an output key derived from KEY as internal key.

If the two weren't different, we wouldn't need to add rawtr in the first place.

@w0xlt
Copy link
Contributor

w0xlt commented Mar 24, 2022

@sipa Thanks for the clarification.

But since the wallet is able to generate multiple addresses for rawtr(KEY), I assumed that there would also be key derivations for KEY in this case.

For example:

# First address
"desc":    "tr([8324d0e9/0]03a669ea926f381582ec4a000b9472ba8a17347f5fb159eddd4a07036a6718eb)#05zmkxzl"
"desc": "rawtr([8324d0e9/0]03a669ea926f381582ec4a000b9472ba8a17347f5fb159eddd4a07036a6718eb)#5rk6yanl"

# Second address
"desc":    "tr([8324d0e9/1]bbf56b14b119bccafb686adec2e3d2a6b51b1626213590c3afa815d1fd36f85d)#0djkz4yx"
"desc": "rawtr([8324d0e9/1]bbf56b14b119bccafb686adec2e3d2a6b51b1626213590c3afa815d1fd36f85d)#56xhsw4x"

Shouldn't rawtr() generate just a single address ?

@sipa
Copy link
Member Author

sipa commented Mar 24, 2022

Different keys give rise to different addresses....

@sipa
Copy link
Member Author

sipa commented Apr 5, 2022

Rebased.

@w0xlt
Copy link
Contributor

w0xlt commented Apr 12, 2022

It looks like the CI is failing due to the difference between the expected result and the RPC result in rpc_decodescript.py:decodescript_datadriven_tests().

Script (first element in rpc_decodescript.json):
5120eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee

Expected result:
{'asm': '1 eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee', 'address': 'bcrt1pamhwamhwamhwamhwamhwamhwamhwamhwamhwamhwamhwamhwamhqz6nvlh', 'desc': 'addr(bcrt1pamhwamhwamhwamhwamhwamhwamhwamhwamhwamhwamhwamhwamhqz6nvlh)#v52jnujz', 'type': 'witness_v1_taproot'}

RPC result:
{'asm': '1 eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee', 'desc': 'rawtr(eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee)#jk7c6kys', 'address': 'bcrt1pamhwamhwamhwamhwamhwamhwamhwamhwamhwamhwamhwamhwamhqz6nvlh', 'type': 'witness_v1_taproot'}

@w0xlt
Copy link
Contributor

w0xlt commented Apr 12, 2022

wallet_taroot.py is also failing. Apparently the PSBT is not completing and assert(res['complete']) fails.

@sipa
Copy link
Member Author

sipa commented Apr 12, 2022

I'll get back to this soon.

w0xlt added a commit to w0xlt/bitcoin that referenced this pull request Apr 13, 2022
w0xlt added a commit to w0xlt/bitcoin that referenced this pull request Apr 13, 2022
w0xlt added a commit to w0xlt/bitcoin that referenced this pull request Apr 13, 2022
w0xlt added a commit to w0xlt/bitcoin that referenced this pull request Apr 16, 2022
w0xlt added a commit to w0xlt/bitcoin that referenced this pull request Apr 17, 2022
@w0xlt
Copy link
Contributor

w0xlt commented Jul 15, 2022

This might fix the CI errors:
In test/functional/wallet_taproot.py (9342970):

  from test_framework.key import H_POINT
+ from test_framework.segwit_addr import encode_segwit_address

w0xlt added a commit to w0xlt/bitcoin that referenced this pull request Jul 15, 2022
@sipa
Copy link
Member Author

sipa commented Jul 19, 2022

Rebased, and made a few changes:

Copy link
Contributor

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

code review ACK 544b433

Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

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

reACK 544b433

w0xlt added a commit to w0xlt/bitcoin that referenced this pull request Aug 1, 2022
w0xlt added a commit to w0xlt/bitcoin that referenced this pull request Aug 3, 2022
w0xlt added a commit to w0xlt/bitcoin that referenced this pull request Aug 9, 2022
@achow101
Copy link
Member

achow101 commented Aug 9, 2022

ACK 544b433

@achow101 achow101 merged commit ac59112 into bitcoin:master Aug 9, 2022
w0xlt added a commit to w0xlt/bitcoin that referenced this pull request Aug 10, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 10, 2022
w0xlt added a commit to w0xlt/bitcoin that referenced this pull request Aug 17, 2022
w0xlt added a commit to w0xlt/bitcoin that referenced this pull request Aug 20, 2022
w0xlt added a commit to w0xlt/bitcoin that referenced this pull request Sep 4, 2022
w0xlt added a commit to w0xlt/bitcoin that referenced this pull request Sep 15, 2022
w0xlt added a commit to w0xlt/bitcoin that referenced this pull request Sep 29, 2022
w0xlt added a commit to w0xlt/bitcoin that referenced this pull request Oct 5, 2022
w0xlt added a commit to w0xlt/bitcoin that referenced this pull request Oct 5, 2022
w0xlt added a commit to w0xlt/bitcoin that referenced this pull request Oct 10, 2022
w0xlt added a commit to w0xlt/bitcoin that referenced this pull request Oct 13, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Aug 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet