Add support for inferring tr() descriptors by sipa · Pull Request #22166 · bitcoin/bitcoin

sipa · 2021-06-06T07:47:33Z

Includes:

First commit from Basic Taproot signing support for descriptor wallets #21365, adding TaprootSpendData in SigningProvider
A refactor to expose ComputeTapleafHash and ComputeTaprootMerkleRoot from script/interpreter
A tiny change to make getaddressinfo report tr() descriptors as solvable (so that inferred descriptors are shown), despite not having signing code for them.
Logic to infer the script tree back from TaprootSpendData, and then use that to infer descriptors.

DrahtBot · 2021-06-06T14:36:07Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Make script interpreter independent from storage type CScript #13062 (Make script interpreter independent from storage type CScript by sipa)

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.

achow101

ACK 77466ae5c58b4a46e545d3d24521fa635f7b8fbe

src/script/standard.cpp

achow101 · 2021-06-08T16:22:36Z

ACK f9d6f9a52c61833bbc5c611ff6a3e59066e1c92f

laanwj · 2021-06-14T15:21:40Z

Reviewed that the consensus commit (e3739fb3b042c3805b5b6357f6f79df787d11144) is a pure refactor.

Code review ACK 13c6ab699eb6906c6554b4319a377c33fa0d157f

achow101 · 2021-06-14T19:33:57Z

re-ACK 13c6ab699eb6906c6554b4319a377c33fa0d157f

laanwj · 2021-06-15T11:35:08Z

src/script/descriptor.cpp

I would have a slight preference to use constants here instead of hardcoding 0x02 (and 0x03 below) and 33.

I agree with this, but prefer keeping it for a follow-up. There are several places where this is already done, and I'd like to introduce constants for it, as well as using them everywhere in the same PR - doing so here would be a distraction.

src/script/descriptor.cpp

src/script/interpreter.cpp

sipa · 2021-06-15T22:01:14Z

Addressed some comments, and also made this observable change:

             if (permit_uncompressed || key.IsCompressed()) {
                 CPubKey pubkey = key.GetPubKey();
                 out.keys.emplace(pubkey.GetID(), key);
-                return std::make_unique<ConstPubkeyProvider>(key_exp_index, pubkey);
+                return std::make_unique<ConstPubkeyProvider>(key_exp_index, pubkey, ctx == ParseScriptContext::P2TR);
             } else {

Which will make descriptors with a provided private key inside tr() be interpreted as the equivalent xonly pubkey rather than the compressed or uncompressed one.

achow101 · 2021-06-16T16:53:22Z

re-ACK 834935511c2c799c8cb2312d617dbed6336000c4

Sjors · 2021-06-16T19:35:21Z

It's too hot here to review code, but I can confirm that this correctly infers my tr(xpub...) and tr(xpub1, pk(xpub2)) descriptors (and surviving a roundtrip through deriveaddresses). Might be a good sanity check to try with a rebased #21329 too.

laanwj · 2021-06-17T10:33:10Z

It's too hot here to review code

Same, I propose we postpone the feature freeze a bit. Not able to concentrate at the moment.

Sjors

tACK 8349355

However I glanced over InferTaprootTree. I think it would help to include tests for the various problems that function is checking for.

src/script/standard.h

src/script/standard.cpp

Sjors · 2021-06-17T13:11:56Z

src/script/descriptor.cpp

834935511c2c799c8cb2312d617dbed6336000c4: rather than failing, could we return raw() (at least for leaf_ver == TAPROOT_LEAF_TAPSCRIPT)? That can wait for a followup though, since we can't have tr() descriptors with raw leaves in the wallet anyway atm, and raw() is still limited to the top level.

I think that's exactly what this code does.

sipa · 2021-06-18T01:23:52Z

Rebased, will work on adding some tests for failure cases of InferTaprootTree.

Sjors

I think something went wrong in your rebase.

For example in the original 8349355 you changed the argument bool xonly = false to bool xonly in the ConstPubkeyProvider provider.

src/script/interpreter.cpp

sipa · 2021-06-18T18:33:03Z

@Sjors Oops, thanks for catching that. I redid the rebase; does it look better now?

achow101 · 2021-06-18T18:44:51Z

re-ACK d637a9b

Rebase looks good.

Sjors · 2021-06-21T15:48:12Z

re-utACK d637a9b

Much better

meshcollider

Code review ACK d637a9b

meshcollider · 2021-06-23T10:02:25Z

src/script/standard.cpp

+    std::vector<std::tuple<int, CScript, int>> ret;
+    if (spenddata.merkle_root.IsNull()) return ret;
+
+    /** Data structure to represent the nodes of the tree we're going to be build. */


nit: to build

touched up the various nits in #22323, feel free to add any others there

meshcollider · 2021-06-23T10:03:15Z

src/script/standard.cpp

+
+    /** Data structure to represent the nodes of the tree we're going to be build. */
+    struct TreeNode {
+        /** Hash of this none, if known; 0 otherwise. */


nit: node

meshcollider · 2021-06-23T10:04:07Z

src/script/standard.cpp

+        bool done = false;
+    };
+
+    // Build tree from the provides branches.


nit: provided

184d453 script, doc: spelling update (Jon Atack) Pull request description: Clears out the report from `test/lint/lint-spelling.sh` and touches up the leftover nits in bitcoin/bitcoin#22166 (review). Happy to add any others people are aware of. ACKs for top commit: MarcoFalke: cr ACK 184d453 Sjors: utACK 184d453 Tree-SHA512: 3b0ef6bd5ff227363b0bda79eeb66763151c74f607bc3a2a7bfe7823e3eef196587bccfe639e714e8e27b918ba57e8317eda06f225143c32c736685087dbcd24

184d453 script, doc: spelling update (Jon Atack) Pull request description: Clears out the report from `test/lint/lint-spelling.sh` and touches up the leftover nits in bitcoin#22166 (review). Happy to add any others people are aware of. ACKs for top commit: MarcoFalke: cr ACK 184d453 Sjors: utACK 184d453 Tree-SHA512: 3b0ef6bd5ff227363b0bda79eeb66763151c74f607bc3a2a7bfe7823e3eef196587bccfe639e714e8e27b918ba57e8317eda06f225143c32c736685087dbcd24

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

roconnor-blockstream · 2022-08-12T15:59:10Z

src/script/interpreter.cpp

 }

-static bool VerifyTaprootCommitment(const std::vector<unsigned char>& control, const std::vector<unsigned char>& program, const CScript& script, uint256& tapleaf_hash)
+uint256 ComputeTapleafHash(uint8_t leaf_version, const CScript& script)


I mildly object the the use of the type CScript here. While it is true that BIP-341 names this stack item "script", this value is only interpreted as a CScript in the case of BIP-342 where the leaf_version is equal to TAPROOT_LEAF_TAPSCRIPT (modulo TAPROOT_LEAF_MASK). Otherwise it is simply a bytestring.

sipa force-pushed the 202106_infer_tap branch from fd9a9fc to b86aa3f Compare June 6, 2021 08:10

DrahtBot added Consensus Descriptors RPC/REST/ZMQ Wallet labels Jun 6, 2021

DrahtBot mentioned this pull request Jun 6, 2021

Basic Taproot signing support for descriptor wallets #21365

Merged

sipa force-pushed the 202106_infer_tap branch from b86aa3f to 77466ae Compare June 6, 2021 18:42

DrahtBot mentioned this pull request Jun 6, 2021

Make script interpreter independent from storage type CScript #13062

Closed

achow101 reviewed Jun 7, 2021

View reviewed changes

src/script/standard.cpp Outdated Show resolved Hide resolved

sipa force-pushed the 202106_infer_tap branch from 77466ae to f9d6f9a Compare June 8, 2021 00:37

sipa force-pushed the 202106_infer_tap branch from f9d6f9a to d81b193 Compare June 9, 2021 19:19

meshcollider added this to the 22.0 milestone Jun 9, 2021

sipa force-pushed the 202106_infer_tap branch from d81b193 to 13c6ab6 Compare June 12, 2021 21:02

laanwj reviewed Jun 15, 2021

View reviewed changes

src/script/descriptor.cpp Outdated Show resolved Hide resolved

laanwj reviewed Jun 15, 2021

View reviewed changes

src/script/interpreter.cpp Outdated Show resolved Hide resolved

sipa force-pushed the 202106_infer_tap branch 2 times, most recently from 3f2ffb2 to 8349355 Compare June 15, 2021 21:56

Sjors approved these changes Jun 17, 2021

View reviewed changes

DrahtBot added the Needs rebase label Jun 17, 2021

sipa mentioned this pull request Jun 18, 2021

A few follow-ups for taproot signing #22275

Merged

sipa force-pushed the 202106_infer_tap branch from 8349355 to 9d1d054 Compare June 18, 2021 01:23

DrahtBot removed the Needs rebase label Jun 18, 2021

Sjors reviewed Jun 18, 2021

View reviewed changes

src/script/interpreter.cpp Outdated Show resolved Hide resolved

sipa added 3 commits June 18, 2021 11:23

consensus refactor: extract ComputeTapleafHash, ComputeTaprootMerkleRoot

29e5dd1

Report address as solvable based on inferred descriptor

c7388e5

Taproot descriptor inference

d637a9b

sipa force-pushed the 202106_infer_tap branch from 9d1d054 to d637a9b Compare June 18, 2021 18:30

meshcollider reviewed Jun 23, 2021

View reviewed changes

meshcollider merged commit 567670b into bitcoin:master Jun 23, 2021

jonatack mentioned this pull request Jun 23, 2021

doc: spelling update #22323

Merged

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

Merge bitcoin#22166: Add support for inferring tr() descriptors

f4fdc90

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge 567670b into merged_master (Bitcoin PR bitcoin/bitcoin#22166)

a503005

roconnor-blockstream reviewed Aug 12, 2022

View reviewed changes

bitcoin locked and limited conversation to collaborators Aug 12, 2023

Conversation

sipa commented Jun 6, 2021

Uh oh!

DrahtBot commented Jun 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Jun 8, 2021

Uh oh!

laanwj commented Jun 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Jun 14, 2021

Uh oh!

laanwj Jun 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sipa commented Jun 15, 2021

Uh oh!

achow101 commented Jun 16, 2021

Uh oh!

Sjors commented Jun 16, 2021

Uh oh!

laanwj commented Jun 17, 2021

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Jun 18, 2021

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sipa commented Jun 18, 2021

Uh oh!

achow101 commented Jun 18, 2021

Uh oh!

Sjors commented Jun 21, 2021

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DrahtBot commented Jun 6, 2021 •

edited

Loading

laanwj commented Jun 14, 2021 •

edited

Loading

laanwj Jun 15, 2021 •

edited

Loading