A few descriptor improvements to prepare for Taproot support #21238

sipa · 2021-02-19T19:41:26Z

These are a few refactors and non-invasive improvements to the descriptors code to prepare for adding Taproot descriptors.

None of the commits change behavior in any way, except the last one which improves error reporting a bit.

RiccardoMasutti

ACK, seems good to me. Waiting for squash and will review again.

sipa · 2021-02-19T20:12:59Z

I don't intend to squash. They're all independent improvements.

DrahtBot · 2021-02-20T05:05:14Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

descriptor wallet: Cache last hardened xpub and use in normalized descriptors #21329 (descriptor wallet: Cache last hardened xpub and use in normalized descriptors by achow101)
Avoid unnecessary signing provider copies on descriptor expansion #16116 (Avoid unnecessary signing provider copies on descriptor expansion by Empact)

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.

Sjors

Code review 45bc0c04e0f2ea6dcce734652f45cabc6d10d7d5 looks good, but I got confused in one place...

src/script/descriptor.cpp

achow101 · 2021-03-12T01:19:50Z

Partial ACK. Same questions as Sjors.

sipa · 2021-03-12T03:22:18Z

@achow101 @Sjors Good that you raise that point.

The original reason why this behavior of MakeScripts-is-invoked-once-per-evaluation existed was to support things like combo(), where a single evaluation at a single index results in multiple scriptPubKeys. This was particularly complicated to combine with descriptors that support multiple subdescriptors (like tr()) - there isn't really a straightforward way of doing that (cartesian product? requiring the same number of evaluations from each and combining pairwise? ...?). I didn't want to deal with that, so I added the restriction that this once-per-evaluation only existed for descriptors with 1 subdescriptor.

This is all silly, because we don't ever need it. combo() is the only descriptor that produces multiple output scripts, and it is only permitted at the top level - so it's not even possible to have it as a subdescriptor. I've just removed support for multiple-outputscripts-from-subscriptor entirely in a separate commit now. Bye.

Sjors · 2021-03-12T08:28:13Z

Thanks, I'll review again after rebase.

sipa · 2021-03-12T08:40:25Z

Rebased.

Sjors · 2021-03-12T14:02:47Z

ACK 05c633e

Conceptually I find it marginally simpler if you reverse the first and second commit, so the first step is to remove the unused loop:

        if (m_subdescriptor_arg) {
            const auto& subscript = subscripts[0];
            out.scripts.emplace(CScriptID(subscript), subscript);
            output_scripts = MakeScripts(pubkeys, &subscript, out);
        } else {
            output_scripts = MakeScripts(pubkeys, nullptr, out);
        }

And then the second commit drops out.scripts.emplace and moves that into MakeScripts (which is also where combo does this).

To sanity check my understanding: key_exp_index is currently used for multisig descriptors to create a seperate cache for each participating key. With taproot, you might have a root key (key_exp_index == 0), one branch with a key key_exp_index == 2, and another branch with a 3 key multisig (key_exp_index 3,4,5). Is that how it works?

achow101 · 2021-03-12T18:00:34Z

ACK 05c633e0efaf300aa3353303f77c0a622647a588

sipa · 2021-03-12T18:58:28Z

@Sjors

Conceptually I find it marginally simpler if you reverse the first and second commit

Ok, done.

With taproot, you might have a root key (key_exp_index == 0), one branch with a key key_exp_index == 2, and another branch with a 3 key multisig (key_exp_index 3,4,5). Is that how it works?

I think you skipped the 1, but otherwise: exactly.

achow101 · 2021-03-15T19:41:25Z

re-ACK 35cbef1c483ac9905331f8035d32600e105baab4

Only change was commit reordering.

Sjors

re-ACK 35cbef1c483ac9905331f8035d32600e105baab4 modulo unused constructor

src/script/descriptor.cpp

S3RK

Code Review ACK 394e387. Nice and clear improvements!

src/script/descriptor.cpp

…ipts There are currently two DescriptorImpl subclasses that rely on the functionality that ExpandHelper automatically adds subscripts to the output SigningProvider. Taproot descriptors will have subscripts, but we don't want them in the SigningProvider's bare script field. To avoid them ending up there, move this functionality into the specific classes' MakeScripts implementation.

So far, no descriptor exists that supports more than one sub-script descriptor. This will change with taproot, so prepare for this by changing the m_subdescriptor_arg from a unique_ptr to a vector of unique_ptr's.

This has no effect for now, as the only fragments with sub-script expressions (sh, wsh) only allow one, and don't have key expressions in them. A future Taproot descriptor will however violate both, and we want the keys in different sub-scripts to be assigned non-overlapping cache indices.

This will allow subclasses to overwrite the serialization of subscript arguments without needing to reimplement all the rest of the ToString logic.

This is a preparation for parsing xonly pubkeys, which will complicate this logic. It's cleaner to put the decision logic close to the public key parsing itself.

This changes all context dependent checks in the parser to be disjunctions of equality checks, rather than also including inequalities. This makes sure that adding a new context enum in the future won't change semantics for existing checks. The error messages are also made a bit more consistent.

sipa · 2021-03-30T01:19:26Z

Rebased, and addressed all comments I think.

S3RK · 2021-03-30T07:06:39Z

reACK 0b188b7
The only changes are addressing review comments above. 1) removing unused constructor 2) removing redundant signing provided instance creation 3) simplifying ToStringSubScriptHelper signature

Sjors · 2021-04-01T11:57:04Z

re-ACK 0b188b7

achow101 · 2021-04-09T19:46:53Z

re-ACK 0b188b7

Sjors · 2021-04-16T08:50:55Z

@instagibbs what do you think?

instagibbs · 2021-04-18T00:32:49Z

Seems like 3 ACKs is enough? not familiar enough with this code anymore to give a proper ACK

ysangkok · 2021-04-20T00:40:29Z

src/script/descriptor.cpp

        auto bytes = ParseHex(str);
        return std::make_unique<RawDescriptor>(CScript(bytes.begin(), bytes.end()));
+    } else if (Func("raw", expr)) {
+        error = "Can only have raw() at top level";


why is there no new test for this and for addr()?

It looks like bitcoin#21238 introduced a silent merge conflict in the documentation, which fails with `-Wdocumentation` in the CI.

…iptor.cpp e5faec6 doc: Fix doxygen comment silent merge conflict in descriptor.cpp (W. J. van der Laan) Pull request description: It looks like #21238 introduced a silent merge conflict in the documentation, which fails with `-Wdocumentation` in the CI. (please merge only if CI passes) ACKs for top commit: ajtowns: ACK e5faec6 -- fixed it for me meshcollider: ACK e5faec6 modulo CI Tree-SHA512: b07d50fd12aa7c239a92aad8ef29f4e88583c3ce701ebedba7c426aac4981c79113adc4670b7d055ab9535a28bdc3f9a30e6ca1b1ed0d7b9a333a3d9c4b40d8a

…root su…

…n descriptor.cpp e5faec6 doc: Fix doxygen comment silent merge conflict in descriptor.cpp (W. J. van der Laan) Pull request description: It looks like bitcoin#21238 introduced a silent merge conflict in the documentation, which fails with `-Wdocumentation` in the CI. (please merge only if CI passes) ACKs for top commit: ajtowns: ACK e5faec6 -- fixed it for me meshcollider: ACK e5faec6 modulo CI Tree-SHA512: b07d50fd12aa7c239a92aad8ef29f4e88583c3ce701ebedba7c426aac4981c79113adc4670b7d055ab9535a28bdc3f9a30e6ca1b1ed0d7b9a333a3d9c4b40d8a

It looks like bitcoin#21238 introduced a silent merge conflict in the documentation, which fails with `-Wdocumentation` in the CI.

RiccardoMasutti approved these changes Feb 19, 2021

View reviewed changes

DrahtBot added the Tests label Feb 19, 2021

DrahtBot mentioned this pull request Feb 20, 2021

Avoid unnecessary signing provider copies on descriptor expansion #16116

Closed

DrahtBot mentioned this pull request Mar 2, 2021

descriptor wallet: Cache last hardened xpub and use in normalized descriptors #21329

Closed

sipa mentioned this pull request Mar 4, 2021

Basic Taproot signing support for descriptor wallets #21365

Merged

This was referenced Mar 10, 2021

refactor: Remove MakeUnique<T>() #21404

Merged

refactor: remove Optional & nullopt #21415

Merged

Sjors reviewed Mar 11, 2021

View reviewed changes

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

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

sipa force-pushed the 202102_descriptor_prepare_taproot branch from 45bc0c0 to 19ecc8e Compare March 12, 2021 03:13

DrahtBot added the Needs rebase label Mar 12, 2021

sipa force-pushed the 202102_descriptor_prepare_taproot branch from 19ecc8e to 05c633e Compare March 12, 2021 08:40

DrahtBot removed the Needs rebase label Mar 12, 2021

sipa force-pushed the 202102_descriptor_prepare_taproot branch from 05c633e to 35cbef1 Compare March 12, 2021 18:56

Sjors reviewed Mar 16, 2021

View reviewed changes

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

DrahtBot added the Needs rebase label Mar 17, 2021

sipa force-pushed the 202102_descriptor_prepare_taproot branch from 35cbef1 to 394e387 Compare March 18, 2021 05:45

DrahtBot removed the Needs rebase label Mar 18, 2021

S3RK reviewed Mar 25, 2021

View reviewed changes

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

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

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

sipa added 2 commits March 29, 2021 16:40

Remove support for subdescriptors expanding to multiple scripts

84f3939

sipa added 5 commits March 29, 2021 17:38

Make DescriptorImpl support multiple subscripts

4441c6f

So far, no descriptor exists that supports more than one sub-script descriptor. This will change with taproot, so prepare for this by changing the m_subdescriptor_arg from a unique_ptr to a vector of unique_ptr's.

refactor: split off subscript logic from ToStringHelper

17e006f

This will allow subclasses to overwrite the serialization of subscript arguments without needing to reimplement all the rest of the ToString logic.

refactor: move uncompressed-permitted logic into ParsePubkey*

33275a9

This is a preparation for parsing xonly pubkeys, which will complicate this logic. It's cleaner to put the decision logic close to the public key parsing itself.

sipa force-pushed the 202102_descriptor_prepare_taproot branch from 394e387 to 0b188b7 Compare March 30, 2021 01:17

maflcko removed the Tests label Apr 1, 2021

DrahtBot added the Tests label Apr 1, 2021

maflcko added Descriptors and removed Tests labels Apr 1, 2021

ysangkok reviewed Apr 20, 2021

View reviewed changes

laanwj merged commit 906ecb8 into bitcoin:master Apr 20, 2021

laanwj added a commit to laanwj/bitcoin that referenced this pull request Apr 20, 2021

doc: Fix doxygen comment silent merge conflict in descriptor.cpp

e5faec6

It looks like bitcoin#21238 introduced a silent merge conflict in the documentation, which fails with `-Wdocumentation` in the CI.

laanwj mentioned this pull request Apr 20, 2021

doc: Fix doxygen comment silent merge conflict in descriptor.cpp #21736

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 20, 2021

Merge bitcoin#21238: A few descriptor improvements to prepare for Tap…

bcf00cd

…root su…

windsok pushed a commit to windsok/bitcoin that referenced this pull request Apr 23, 2021

doc: Fix doxygen comment silent merge conflict in descriptor.cpp

1bae370

It looks like bitcoin#21238 introduced a silent merge conflict in the documentation, which fails with `-Wdocumentation` in the CI.

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

A few descriptor improvements to prepare for Taproot support #21238

A few descriptor improvements to prepare for Taproot support #21238

Uh oh!

Conversation

sipa commented Feb 19, 2021

Uh oh!

RiccardoMasutti left a comment

Choose a reason for hiding this comment

Uh oh!

sipa commented Feb 19, 2021

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

achow101 commented Mar 12, 2021

Uh oh!

sipa commented Mar 12, 2021

Uh oh!

Sjors commented Mar 12, 2021

Uh oh!

sipa commented Mar 12, 2021

Uh oh!

Sjors commented Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Mar 12, 2021

Uh oh!

sipa commented Mar 12, 2021

Uh oh!

achow101 commented Mar 15, 2021

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

S3RK left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sipa commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

S3RK commented Mar 30, 2021

Uh oh!

Sjors commented Apr 1, 2021

Uh oh!

achow101 commented Apr 9, 2021

Uh oh!

Sjors commented Apr 16, 2021

Uh oh!

instagibbs commented Apr 18, 2021

Uh oh!

ysangkok Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

DrahtBot commented Feb 20, 2021 •

edited

Loading

Sjors commented Mar 12, 2021 •

edited

Loading

sipa commented Mar 30, 2021 •

edited

Loading