Add descriptor_tests covering tr(), and fix minor bugs

sipa commented at 11:18 pm on February 14, 2022: member

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.

Bugfix: set x-only flag when inferring pk() inside tr() 18ad54c3b2

sipa requested review from achow101 on Feb 14, 2022

sipa added the label Bug on Feb 14, 2022

sipa added the label Descriptors on Feb 14, 2022

sipa added the label Tests on Feb 14, 2022

DrahtBot commented at 1:25 am on February 15, 2022: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24361 (Descriptor unit tests and simplifications by sipa)
#22838 (descriptors: Be able to specify change and receiving in a single descriptor string 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.

Bugfix: make ToPrivateString work with x-only keys 4b2e31a7ae

in src/script/descriptor.cpp:270 in 5e093e9604 outdated

266+                if (key.IsValid()) break;
267+            }
268+        } else {
269+            arg.GetKey(m_pubkey.GetID(), key);
270+        }
271+        if (!key.IsValid()) return false;

achow101 commented at 4:45 pm on February 15, 2022:

In 5e093e9604b9161088a44707694389b883842f4e “Add tr() descriptor unit tests”

This seems like a bug fix that should be in a commit separate from the tests?

sipa commented at 5:01 pm on February 15, 2022:

Done. Also updated PR description.

sipa force-pushed on Feb 15, 2022

sipa renamed this:
~~Add descriptor_tests covering tr(), and fix infer bug~~
Add descriptor_tests covering tr(), and fix minor bugs
on Feb 15, 2022

achow101 commented at 6:21 pm on February 15, 2022: member

ACK 9f5c7830ee36e5a9fa9a8a816316fe01b62eb0de

Add tr() descriptor unit tests 0683f377e1

sipa force-pushed on Feb 15, 2022

sipa commented at 11:35 pm on February 15, 2022: member

Improved the new tests a bit, adding a case with multiple tree branches, and signing with mixed pub/priv descriptors.

laanwj added this to the milestone 23.0 on Feb 17, 2022

laanwj commented at 11:47 am on February 17, 2022: member

Added to 23.0 milestone because bugfix.

achow101 commented at 4:12 pm on February 17, 2022: member

ACK 0683f377e1588758da86368f82efee765f89d890

fanquake requested review from meshcollider on Feb 17, 2022

fanquake requested review from instagibbs on Feb 17, 2022

instagibbs approved

instagibbs commented at 1:32 am on February 18, 2022: member

ACK https://github.com/bitcoin/bitcoin/pull/24343/commits/0683f377e1588758da86368f82efee765f89d890

in src/script/descriptor.cpp:1264 in 0683f377e1

1260@@ -1253,7 +1261,7 @@ std::unique_ptr<DescriptorImpl> InferScript(const CScript& script, ParseScriptCo
1261 {
1262     if (ctx == ParseScriptContext::P2TR && script.size() == 34 && script[0] == 32 && script[33] == OP_CHECKSIG) {
1263         XOnlyPubKey key{Span{script}.subspan(1, 32)};
1264-        return std::make_unique<PKDescriptor>(InferXOnlyPubkey(key, ctx, provider));
1265+        return std::make_unique<PKDescriptor>(InferXOnlyPubkey(key, ctx, provider), true);

jonatack commented at 11:29 pm on February 18, 2022:

18ad54c3

0        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

in src/test/descriptor_tests.cpp:315 in 0683f377e1

308@@ -269,7 +309,12 @@ void DoCheck(const std::string& prv, const std::string& pub, const std::string&
309                     CMutableTransaction spend;
310                     spend.vin.resize(1);
311                     spend.vout.resize(1);
312-                    BOOST_CHECK_MESSAGE(SignSignature(Merge(keys_priv, script_provider), spks[n], spend, 0, 1, SIGHASH_ALL), prv);
313+                    std::vector<CTxOut> utxos(1);
314+                    PrecomputedTransactionData txdata;
315+                    txdata.Init(spend, std::move(utxos), /* force */ true);
316+                    MutableTransactionSignatureCreator creator(&spend, 0, CAmount{0}, &txdata, SIGHASH_DEFAULT);

jonatack commented at 11:50 pm on February 18, 2022:

0683f37 clang-tidy compatible named args

0-                    txdata.Init(spend, std::move(utxos), /* force */ true);
1-                    MutableTransactionSignatureCreator creator(&spend, 0, CAmount{0}, &txdata, SIGHASH_DEFAULT);
2+                    txdata.Init(spend, std::move(utxos), /*force=*/true);
3+                    MutableTransactionSignatureCreator creator(&spend, /*input_idx=*/0, CAmount{0}, &txdata, SIGHASH_DEFAULT);

jonatack commented at 11:54 pm on February 18, 2022: member

Code review ACK 0683f377e1588758da86368f82efee765f89d890

A couple of nitty suggestions in the comments, feel free to ignore.

fanquake merged this on Feb 21, 2022

fanquake closed this on Feb 21, 2022

sidhujag referenced this in commit c7189a1d40 on Feb 22, 2022

michaelfolkson commented at 1:17 pm on February 25, 2022: contributor

Are you going to open a PR with your nits @jonatack? Maybe should be included in 23.0 milestone if you are?

jonatack commented at 1:22 pm on February 25, 2022: member

@michaelfolkson no, if the author considers it worthwhile they could be done in a follow-up.

michaelfolkson commented at 1:36 pm on February 25, 2022: contributor

@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.” :)

jonatack commented at 2:10 pm on February 25, 2022: member

It’s more likely I’d follow up for, say, a bug or missing tests.

DrahtBot locked this on Feb 25, 2023

Add descriptor_tests covering tr(), and fix minor bugs #24343

Conflicts