Basic Taproot signing support for descriptor wallets #21365

sipa · 2021-03-04T22:49:33Z

Builds on top of #22051, adding signing support after derivation support.

Nothing is changed in descriptor features. Signing works for key path and script path spending, through the normal sending functions, and PSBT-based RPCs. However, PSBT usability is rather low as no extensions have been defined to convey Taproot-specific information, so all script information must be known to the signing wallet.

DrahtBot · 2021-03-04T23:19:30Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Add support for inferring tr() descriptors #22166 (Add support for inferring tr() descriptors by sipa)
Implement BIP-119 Validation (CheckTemplateVerify) #21702 (Implement BIP-119 Validation (CheckTemplateVerify) by JeremyRubin)
rpc, gui: bumpfee signer support #21576 (rpc: bumpfee signer support by Sjors)
Implement BIP 370 PSBTv2 #21283 (Implement BIP 370 PSBTv2 by achow101)

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.

sipa · 2021-03-06T03:17:10Z

Rebased on top of #21246, renamed "inner key" everywhere in this PR to "internal key" as well, and added very basic PSBT support (you can have a watchonly tr(KEY) wallet now, and get it signed by an offline version that has the private key).

Sjors · 2021-03-07T18:34:01Z

Concept ACK 🎉

roconnor-blockstream · 2021-03-08T14:32:08Z

TBH, I find your "must be specified in depth-first search order" somewhat confusing. As an alternative syntax I suggest simply using some notation for a binary or. For example.

tr(KEY): only taproot output key known (or is it an internal pubkey with common NUMS tweak??)
tr(KEY,A): internal pubkey with a single script A at level 0.
tr(KEY,[A|B]): internal pubkey with a two scripts, A and B at level 1.
tr(KEY.[[A|B]|C]): internal pubkey with a two scripts, A and B at level 2, and a third script at level 1.
tr(KEY.[A|[B|C]]): same as above except for A is at level 1 and C is at level 2 instead.
tr(KEY.[[B|C]|A]): equal to the above descriptor.
tr(KEY.[[A|B]|[C|D]]): internal pubkey with a four scripts, all at level 2.
tr(KEY.[[A|C]|[B|D]]): same four scripts as above, again all at level 2 but now associated differently which makes for a different tweak.
tr(KEY.[A|[[B|C]|D]]): same four scripts as above, but no longer all at the same level.

My claimed advantages are the implicitness of the depth, and clearer notation indicating how the scripts are associated amongst each other.

The notation above is just an example and don't care too much about the specific syntax.

Other things to include in the list of features to be added later, a MerkleRoot descriptor for fixed but unknown branches.

Fixes #147. We relied on an abandoned Bitcoin Core PR for creating regtest networks with custom consensus rules. We switch to using the Taproot soft fork instead to create invalid block. We can switch back to Bitcoin Core master once bitcoin/bitcoin#21365 is merged. After taproot wallet support is merged and released, we can switch back to downloading a Bitcoin Core binary rather than self compiling. The test framework also underwent some changes, in particular (dis)connect_nodes now takes a numerical argument, and wallets have to manually created.

sipa · 2021-03-08T18:17:31Z

@roconnor-blockstream Yeah, I considered something like that, and am open to it. Both notations are equivalent in that you can fairly easily convert in both directions between them (counting bracing level in yours corresponds to the depth specified explicitly in mine, with leaves ultimately listed in the same order). The advantage I see to the current one is that I think it's a bit more readable, avoiding the need to go count braces to see relative depth of leaves. Yours is perhaps a bit easier to write (but maybe that's not something we really expect humans to do much for nontrivial cases).

There are many more variations possible. One extreme would be permitting arbitrary weights for every node, and have the descriptor expansion convert it to a Huffman tree. I'd prefer not to do this because it's adding information that can't be inferred back from the script tree it is converted to.

roconnor-blockstream · 2021-03-08T18:25:57Z

Right. My concern with explicit levels is that it is so easy to violate the invariant that the sum of the weights has to add up to 1, and you cannot really splice together subexpressions to compose a descriptor from fragments without going through and globally adjusting all the levels to be sane.

For non-Taproot signatures, this is interpreted as SIGHASH_ALL.

sipa · 2021-06-12T20:47:37Z

Rebased.

achow101 · 2021-06-14T19:24:55Z

re-ACK 458a345

Sjors · 2021-06-16T09:30:15Z

I'll try to review this today. Having this feature in the signet / testnet wallet makes it easier for people to test taproot before it activates, and potentially find problems (bugs, many eyes, shallow, etc). So it seems worth merging even slightly after the feature freeze.

michaelfolkson · 2021-06-16T14:38:28Z

Concept ACK, Approach ACK. Updated syntax choice makes sense to me under the readability assumption that use cases that need more than say 2 levels will be rarer. Ignoring readability, the updated syntax choice seems superior due to arguments given.

Agree with @Sjors on importance of getting this in for 22.0. Two weeks for any bug fixes from today should be ok and protections in place (e.g. #22156) regardless to prevent users from spending mainnet Taproot outputs pre-activation (November).

Sjors · 2021-06-16T20:08:54Z

Very light testing using this PR rebased onto #22260: I'm still able to spend from my tr(xpub...) and tr(xpub1, pk(xpub2)) signet descriptor wallets in the GUI.

fjahr

Code review ACK 458a345

I don't consider any of my comments blocking. Will test over the next days.

fjahr · 2021-06-16T21:13:11Z

src/script/sign.cpp

+                sigdata.taproot_key_path_sig = sig;
+            }
+        }
+        if (sigdata.taproot_key_path_sig.size()) {


nit: this could go before the block with sigdata.taproot_key_path_sig.size() == 0 to return early and then that other block would not need that other if statement

I tried this for a bit, but I don't find the result cleaner.

fjahr · 2021-06-16T21:41:59Z

src/script/signingprovider.h

@@ -50,11 +52,13 @@ struct FlatSigningProvider final : public SigningProvider
    std::map<CKeyID, CPubKey> pubkeys;
    std::map<CKeyID, std::pair<CPubKey, KeyOriginInfo>> origins;
    std::map<CKeyID, CKey> keys;
+    std::map<XOnlyPubKey, TaprootSpendData> tr_spenddata; /** Map from output key to spend data. */


nit: m_tr_spenddata?

It's a struct, not a class, so the developer notes don't require that (it's inconsistently applied through the codebase, in some cases using m_ prefixes for struct members too, but I don't think that's enough reason to do so here).

src/script/standard.h

src/script/sign.h

src/script/sign.cpp

Sjors

ACK 458a345

src/script/interpreter.cpp

S3RK

two comments

src/key.h

…lets

hebasto · 2021-06-23T10:24:13Z

src/script/standard.cpp

+        merkle_root = other.merkle_root;
+    }
+    for (auto& [key, control_blocks] : other.scripts) {
+        scripts[key].merge(std::move(control_blocks));


On macOS Mojave 10.14.6 (18G9216) system clang fails to compile this line due to the lack of std::set::merge support.

$ clang --version Apple LLVM version 10.0.1 (clang-1001.0.46.4) Target: x86_64-apple-darwin18.7.0 Thread model: posix InstalledDir: /Library/Developer/CommandLineTools/usr/bin

See: https://stackoverflow.com/questions/50444908/clang-6-not-supporting-unordered-mapmerge

Reported by @S3RK.

I presume this would also make it harder to compile on https://packages.debian.org/stretch/gcc

Since #20413 we support gcc 7+ only, right?

In theory there is https://packages.debian.org/stretch/clang-7, but I don't think anyone was able to compile master with system packages on stretch recently.

Confirmed (for fun) that the issue exists with libc++7.

# make V=1 Making all in src make[1]: Entering directory '/bitcoin/src' make[2]: Entering directory '/bitcoin/src' make[3]: Entering directory '/bitcoin' make[3]: Leaving directory '/bitcoin' /usr/bin/ccache clang++-7 -stdlib=libc++ -std=c++17 -DHAVE_CONFIG_H -I. -I../src/config -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -I. -I./secp256k1/include -DBOOST_SP_USE_STD_ATOMIC -DBOOST_AC_USE_STD_ATOMIC -I/usr/include -I./leveldb/include -I./leveldb/helpers/memenv -I./univalue/include -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -DPROVIDE_FUZZ_MAIN_FUNCTION -fdebug-prefix-map=/bitcoin/src=. -Wstack-protector -fstack-protector-all -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wswitch -Wthread-safety -Wrange-loop-analysis -Wredundant-decls -Wunused-variable -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Wsign-compare -Woverloaded-virtual -Wunreachable-code-loop-increment -Wno-unused-parameter -Wno-self-assign -Wno-unused-local-typedef -Wno-implicit-fallthrough -fPIE -g -O2 -MT script/libbitcoin_common_a-standard.o -MD -MP -MF script/.deps/libbitcoin_common_a-standard.Tpo -c -o script/libbitcoin_common_a-standard.o `test -f 'script/standard.cpp' || echo './'`script/standard.cpp script/standard.cpp:410:22: error: no member named 'merge' in 'std::__1::set<std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >, ShortestVectorFirstComparator, std::__1::allocator<std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > > >' scripts[key].merge(std::move(control_blocks)); ~~~~~~~~~~~~ ^ 1 error generated. Makefile:7756: recipe for target 'script/libbitcoin_common_a-standard.o' failed make[2]: *** [script/libbitcoin_common_a-standard.o] Error 1 make[2]: Leaving directory '/bitcoin/src' Makefile:15489: recipe for target 'all-recursive' failed make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory '/bitcoin/src' Makefile:820: recipe for target 'all-recursive' failed make: *** [all-recursive] Error 1

See #22339.

Since bitcoin#21365 was merged, the support of P0083R3 proposal is required.

6cf4ea7 Avoid the use of P0083R3 std::set::merge (Pieter Wuille) Pull request description: This use was introduced in bitcoin#21365, but as pointed out in bitcoin#22339, this causes compatibility problems. Just avoid its use for now. ACKs for top commit: jonatack: re-ACK 6cf4ea7 benthecarman: ACK 6cf4ea7 hebasto: ACK 6cf4ea7, successfully compiled on the following systems: Tree-SHA512: 2b3fdcadb7de98963ebb0b192bd956aa68526457fe5b374c74a69ea10d5b68902763148f11abbcc471010bcdc799e0804faef5f8e8ff8a509b3a053c0cb0ba39

Since bitcoin#21365 was merged, the support of P0083R3 proposal is required.

6cf4ea7 Avoid the use of P0083R3 std::set::merge (Pieter Wuille) Pull request description: This use was introduced in bitcoin#21365, but as pointed out in bitcoin#22339, this causes compatibility problems. Just avoid its use for now. ACKs for top commit: jonatack: re-ACK 6cf4ea7 benthecarman: ACK 6cf4ea7 hebasto: ACK 6cf4ea7, successfully compiled on the following systems: Tree-SHA512: 2b3fdcadb7de98963ebb0b192bd956aa68526457fe5b374c74a69ea10d5b68902763148f11abbcc471010bcdc799e0804faef5f8e8ff8a509b3a053c0cb0ba39

08f57a0 Assert that IsComplete() in GetSpendData() (Pieter Wuille) d8f4b97 Remove default nHashTypeIn arguments to MutableTransactionSignatureCreator (Pieter Wuille) c7048aa Simplify SignTransaction precomputation loop (Pieter Wuille) addb9b5 Improve comments in taproot signing logic (Pieter Wuille) Pull request description: This addresses a few review comments from #21365 that were left at the time of merge (as well as some from #22166 applying to the commit it shared with #21365). I do not think any are blockers for a 22.0 release. ACKs for top commit: theStack: re-ACK 08f57a0 🌴 Zero-1729: crACK 08f57a0 jonatack: Code review ACK 08f57a0 per `git range-diff e9d6eb1 9336670 08f57a0` followed by re-code review per commit to swap context back into memory and debug build/run unit tests + feature_taproot.py as a sanity check Tree-SHA512: 968750109ba8d6faf3016035a38f81c6aefb724c632a3cab0bbf43cf31b9d187b6b0fddd8772768f57338df11eb07ab9c4c6dacdf3cf35b71f29699c67a301ea

08f57a0 Assert that IsComplete() in GetSpendData() (Pieter Wuille) d8f4b97 Remove default nHashTypeIn arguments to MutableTransactionSignatureCreator (Pieter Wuille) c7048aa Simplify SignTransaction precomputation loop (Pieter Wuille) addb9b5 Improve comments in taproot signing logic (Pieter Wuille) Pull request description: This addresses a few review comments from bitcoin#21365 that were left at the time of merge (as well as some from bitcoin#22166 applying to the commit it shared with bitcoin#21365). I do not think any are blockers for a 22.0 release. ACKs for top commit: theStack: re-ACK 08f57a0 🌴 Zero-1729: crACK 08f57a0 jonatack: Code review ACK 08f57a0 per `git range-diff e9d6eb1 9336670 08f57a0` followed by re-code review per commit to swap context back into memory and debug build/run unit tests + feature_taproot.py as a sanity check Tree-SHA512: 968750109ba8d6faf3016035a38f81c6aefb724c632a3cab0bbf43cf31b9d187b6b0fddd8772768f57338df11eb07ab9c4c6dacdf3cf35b71f29699c67a301ea

This introduces Taproot wallet support. I fixed all the merge conflicts and ensured that the tests pass, but this is still using the old sighash (before Russell/Sanket/I redid it) so is not actually production ready. Will be fixed when we bring Elements #1002 in.

sipa force-pushed the 202102_taproot_sign branch from ea57837 to bc7be03 Compare March 4, 2021 23:06

DrahtBot added Consensus Docs RPC/REST/ZMQ Scripts and tools Utils/log/libs Wallet labels Mar 5, 2021

This was referenced Mar 5, 2021

MOVEONLY: CWallet transaction code out of wallet.cpp/.h #21207

Merged

refactor: Make CWalletTx sync state type-safe #21206

Merged

Introduce DeferredSignatureChecker and have SignatureExtractorClass subclass it #21166

Merged

sipa force-pushed the 202102_taproot_sign branch from bc7be03 to f2c695e Compare March 5, 2021 00:56

This was referenced Mar 5, 2021

lib: Add Taproot support to libconsensus #21158

Closed

BIP 350: Implement Bech32m and use it for v1+ segwit addresses #20861

Merged

DrahtBot added the Needs rebase label Mar 5, 2021

DrahtBot mentioned this pull request Mar 5, 2021

Avoid unnecessary signing provider copies on descriptor expansion #16116

Closed

sipa force-pushed the 202102_taproot_sign branch from f2c695e to 676ab37 Compare March 6, 2021 03:13

DrahtBot removed the Needs rebase label Mar 6, 2021

DrahtBot mentioned this pull request Mar 6, 2021

Implement BIP 370 PSBTv2 #21283

Open

sipa added 3 commits June 12, 2021 12:25

Basic Taproot signing logic in script/sign.cpp

a238012

tests: check spending of P2TR

c0f0c8e

Add support for SIGHASH_DEFAULT in RPCs, and make it default

458a345

For non-Taproot signatures, this is interpreted as SIGHASH_ALL.

sipa force-pushed the 202102_taproot_sign branch from c69ad7e to 458a345 Compare June 12, 2021 19:39

DrahtBot removed the Needs rebase label Jun 12, 2021

fjahr reviewed Jun 17, 2021

View reviewed changes

Sjors approved these changes Jun 17, 2021

View reviewed changes

src/script/interpreter.cpp Show resolved Hide resolved

src/script/interpreter.cpp Show resolved Hide resolved

S3RK reviewed Jun 17, 2021

View reviewed changes

src/key.h Show resolved Hide resolved

src/key.h Show resolved Hide resolved

meshcollider merged commit 5c2e2af into bitcoin:master Jun 17, 2021

sipa mentioned this pull request Jun 18, 2021

A few follow-ups for taproot signing #22275

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 18, 2021

Merge bitcoin#21365: Basic Taproot signing support for descriptor wal…

59b8f7b

…lets

hebasto reviewed Jun 23, 2021

View reviewed changes

hebasto added a commit to hebasto/bitcoin that referenced this pull request Jun 25, 2021

doc: Update actual minimum supported compiler versions

440b3c0

Since bitcoin#21365 was merged, the support of P0083R3 proposal is required.

hebasto mentioned this pull request Jun 25, 2021

doc: Document minimum required libc++ version #22339

Merged

hebasto added a commit to hebasto/bitcoin that referenced this pull request Jun 25, 2021

doc: Update actual minimum supported compiler versions

be414e9

Since bitcoin#21365 was merged, the support of P0083R3 proposal is required.

hebasto added a commit to hebasto/bitcoin that referenced this pull request Jun 25, 2021

doc: Update actual minimum supported compiler versions

4ffb9f4

Since bitcoin#21365 was merged, the support of P0083R3 proposal is required.

sipa mentioned this pull request Jun 25, 2021

Avoid the use of P0083R3 std::set::merge #22342

Merged

hebasto added a commit to hebasto/bitcoin that referenced this pull request Jun 26, 2021

doc: Update actual minimum supported compiler versions

22c398f

Since bitcoin#21365 was merged, the support of P0083R3 proposal is required.

JeremyRubin mentioned this pull request Feb 2, 2022

Feature discussion: partial descriptors/miniscript #24114

Open

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Basic Taproot signing support for descriptor wallets #21365

Basic Taproot signing support for descriptor wallets #21365

sipa commented Mar 4, 2021 •

edited

DrahtBot commented Mar 4, 2021 •

edited

sipa commented Mar 6, 2021

Sjors commented Mar 7, 2021

roconnor-blockstream commented Mar 8, 2021 •

edited

sipa commented Mar 8, 2021 •

edited

roconnor-blockstream commented Mar 8, 2021

sipa commented Jun 12, 2021

achow101 commented Jun 14, 2021

Sjors commented Jun 16, 2021

michaelfolkson commented Jun 16, 2021 •

edited

Sjors commented Jun 16, 2021

fjahr left a comment

fjahr Jun 16, 2021

sipa Jun 17, 2021

fjahr Jun 16, 2021

sipa Jun 17, 2021

Sjors left a comment

S3RK left a comment

hebasto Jun 23, 2021 •

edited

MarcoFalke Jun 23, 2021

hebasto Jun 23, 2021

MarcoFalke Jun 23, 2021

MarcoFalke Jun 23, 2021

hebasto Jun 25, 2021

Basic Taproot signing support for descriptor wallets #21365

Basic Taproot signing support for descriptor wallets #21365

Conversation

sipa commented Mar 4, 2021 • edited

DrahtBot commented Mar 4, 2021 • edited

Conflicts

sipa commented Mar 6, 2021

Sjors commented Mar 7, 2021

roconnor-blockstream commented Mar 8, 2021 • edited

sipa commented Mar 8, 2021 • edited

roconnor-blockstream commented Mar 8, 2021

sipa commented Jun 12, 2021

achow101 commented Jun 14, 2021

Sjors commented Jun 16, 2021

michaelfolkson commented Jun 16, 2021 • edited

Sjors commented Jun 16, 2021

fjahr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors left a comment

Choose a reason for hiding this comment

S3RK left a comment

Choose a reason for hiding this comment

hebasto Jun 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sipa commented Mar 4, 2021 •

edited

DrahtBot commented Mar 4, 2021 •

edited

roconnor-blockstream commented Mar 8, 2021 •

edited

sipa commented Mar 8, 2021 •

edited

michaelfolkson commented Jun 16, 2021 •

edited

hebasto Jun 23, 2021 •

edited