Add descriptor_tests covering tr(), and fix minor bugs #24343
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
5e093e9 to
9f5c783
Compare
|
ACK 9f5c783 |
9f5c783 to
0683f37
Compare
|
Improved the new tests a bit, adding a case with multiple tree branches, and signing with mixed pub/priv descriptors. |
|
Added to 23.0 milestone because bugfix. |
|
ACK 0683f37 |
| @@ -1253,7 +1261,7 @@ std::unique_ptr<DescriptorImpl> InferScript(const CScript& script, ParseScriptCo | |||
| { | |||
| if (ctx == ParseScriptContext::P2TR && script.size() == 34 && script[0] == 32 && script[33] == OP_CHECKSIG) { | |||
| XOnlyPubKey key{Span{script}.subspan(1, 32)}; | |||
| return std::make_unique<PKDescriptor>(InferXOnlyPubkey(key, ctx, provider)); | |||
| return std::make_unique<PKDescriptor>(InferXOnlyPubkey(key, ctx, provider), true); | |||
There was a problem hiding this comment.
| return std::make_unique<PKDescriptor>(InferXOnlyPubkey(key, ctx, provider), true); | |
| return std::make_unique<PKDescriptor>(InferXOnlyPubkey(key, ctx, provider), /*xonly=*/true); |
Verified descriptor_tests fail without this commit:
test/descriptor_tests.cpp(327): error: in "descriptor_tests/descriptor_test": check spks_inferred[0] == spks[n] has failed
| std::vector<CTxOut> utxos(1); | ||
| PrecomputedTransactionData txdata; | ||
| txdata.Init(spend, std::move(utxos), /* force */ true); | ||
| MutableTransactionSignatureCreator creator(&spend, 0, CAmount{0}, &txdata, SIGHASH_DEFAULT); |
There was a problem hiding this comment.
0683f37 clang-tidy compatible named args
- txdata.Init(spend, std::move(utxos), /* force */ true);
- MutableTransactionSignatureCreator creator(&spend, 0, CAmount{0}, &txdata, SIGHASH_DEFAULT);
+ txdata.Init(spend, std::move(utxos), /*force=*/true);
+ MutableTransactionSignatureCreator creator(&spend, /*input_idx=*/0, CAmount{0}, &txdata, SIGHASH_DEFAULT);…r bugs 0683f37 Add tr() descriptor unit tests (Pieter Wuille) 4b2e31a Bugfix: make ToPrivateString work with x-only keys (Pieter Wuille) 18ad54c Bugfix: set x-only flag when inferring pk() inside tr() (Pieter Wuille) Pull request description: This fixes two bugs in the current logic for `tr()` descriptors: * ToPrivateString does not always work, because the provided private key may mismatch the parity of the x-only public key. * The descriptors inferred for `pk()` inside `tr()` have the wrong x-only flag, leading to such descriptors generating the wrong scriptPubKey (roundtripping through ToString does fix it however, so this seems unobservable in the current code). These were discovered while adding unit tests to descriptor_tests that cover various aspects of `tr()` descriptors, which are now also added here. ACKs for top commit: achow101: ACK 0683f37 instagibbs: ACK bitcoin@0683f37 jonatack: Code review ACK 0683f37 Tree-SHA512: fc0e11b45da53054a108effff2029d67b64e508b160a6e22e00c98b506c39ec12ccc95afd21ea68a6c691eb62930afc7af18908f2fa3a954d102afdc67bc355a
|
Are you going to open a PR with your nits @jonatack? Maybe should be included in 23.0 milestone if you are? |
|
@michaelfolkson no, if the author considers it worthwhile they could be done in a follow-up. |
|
@jonatack: Ok will wait to see whether @sipa thinks the nits are worth it or not. This was merged before #24433 but since that was merged the guidance is: "You are expected to reply to any review comments before your pull request is merged. You may update the code or reject the feedback if you do not agree with it, but you should express so in a reply." :) |
|
It's more likely I'd follow up for, say, a bug or missing tests. |
This fixes two bugs in the current logic for
tr()descriptors:pk()insidetr()have the wrong x-only flag, leading to such descriptors generating the wrong scriptPubKey (roundtripping through ToString does fix it however, so this seems unobservable in the current code).These were discovered while adding unit tests to descriptor_tests that cover various aspects of
tr()descriptors, which are now also added here.