Descriptor unit tests and simplifications #24361

pull sipa wants to merge 4 commits into bitcoin:master from sipa:202202_desctests changing 5 files +103 −79

sipa commented at 8:48 PM on February 16, 2022: member

Builds on top of #24343.

Adds additional tests, and makes ToPrivateString() always succeed, using pubkeys in case privkeys are unavailable.
sipa requested review from achow101 on Feb 16, 2022
DrahtBot added the label Descriptors on Feb 16, 2022
DrahtBot added the label Wallet on Feb 16, 2022
DrahtBot commented at 7:36 AM on February 17, 2022: contributor


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



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #24149 (Signing support for Miniscript Descriptors by darosior)
- #24148 (Miniscript support in Output Descriptors by darosior)
- #22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)
- #19602 (wallet: Migrate legacy wallets to descriptor wallets 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 force-pushed on Feb 17, 2022
sipa commented at 5:29 PM on February 17, 2022: member

Added a commit, suggested by @achow101, to merge ToString and ToPrivateString completely.
in src/test/descriptor_tests.cpp:358 in dcc11864a8 outdated
```
 352 | @@ -312,6 +353,9 @@ void DoCheck(const std::string& prv, const std::string& pub, const std::string&
 353 |  
 354 |                  /* Infer a descriptor from the generated script, and verify its solvability and that it roundtrips. */
 355 |                  auto inferred = InferDescriptor(spks[n], script_provider);
 356 | +                if (ref.size() == 1) {
```
achow101 commented at 6:42 PM on February 17, 2022:

In dcc11864a8f7141298d71e5a61e686ccf0032b17 "Test roundtripping of descriptors ignoring keys"

How come this is only checked for non-ranged descriptors?

sipa commented at 6:44 PM on February 17, 2022:

It's only checked for non-combo descriptors (because for combo is obviously doesn't hold...).

achow101 commented at 6:57 PM on February 17, 2022:

Oh, I see. ref is an array that may have multiple scripts because of combo. The line that creates it had RANGE in it and that confused me.
luke-jr commented at 12:03 AM on February 19, 2022: member

idk, I think it makes more sense to have two separate methods rather than different security-sensitivity based on an argument.
DrahtBot added the label Needs rebase on Feb 21, 2022
sipa commented at 7:05 PM on April 5, 2022: member

@luke-jr I don't think that's exactly it. If you provide a SigningProvider which happens to not contain the necessary private keys, it'll also use public ones. So I think it's better to think of it as there just being one function, which always takes a signing provider, and using its private keys. And then for convenience the ability to omit providing such a provider for the case where you just want an empty one.
Make ToPrivateString use pub where prv is missing f3eb4b59c8
Merge descriptor ToString and ToPrivateString a0cefab5de
Test roundtripping of descriptors ignoring keys 2518098680
Test raw() and addr() descriptors in unit tests 5f2af0c1ac
sipa commented at 7:09 PM on April 5, 2022: member

Rebased.
sipa force-pushed on Apr 5, 2022
DrahtBot removed the label Needs rebase on Apr 5, 2022
fanquake requested review from achow101 on Apr 26, 2022
achow101 commented at 8:48 PM on April 27, 2022: member

ACK 5f2af0c1ac5ee7de3e137afaee84bd31e93abe80
fanquake requested review from instagibbs on Apr 28, 2022

Type	Reviewers
ACK	achow101

in test/functional/wallet_listdescriptors.py:106 in 5f2af0c1ac

 102 | @@ -103,7 +103,7 @@ def run_test(self):
 103 |              'desc': descsum_create('wpkh(' + xpub_acc + ')'),
 104 |              'timestamp': 1296688602,
 105 |          }])
 106 | -        assert_raises_rpc_error(-4, 'Can\'t get descriptor string', watch_only_wallet.listdescriptors, True)
 107 | +        assert_equal(watch_only_wallet.listdescriptors(True), watch_only_wallet.listdescriptors(False))

S3RK commented at 6:36 AM on July 14, 2022:

Is this desired behaviour? Should we check that wallet doesn't have disable_private_keys enabled and return "wallet doesn't contain private keys" error?

darosior commented at 2:42 PM on July 14, 2022: member

Code looks good to me. But i agree with @S3RK we should not change the behaviour of the RPC API.
DrahtBot added the label Needs rebase on Jul 14, 2022
DrahtBot commented at 7:43 PM on July 14, 2022: contributor



🐙 This pull request conflicts with the target branch and needs rebase.

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>
achow101 commented at 7:05 PM on October 12, 2022: member

Are you still working on this?
sipa commented at 10:44 PM on November 8, 2022: member

I do plan to pick this up soon (perhaps after #26076).
DrahtBot commented at 1:16 AM on February 6, 2023: contributor


There hasn't been much activity lately and the patch still needs rebase. What is the status here?
- Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
- Is it no longer relevant? ➡️ Please close.
- Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.
fanquake commented at 3:24 PM on February 16, 2023: member

Drafted for now given it's (maybe) waiting on #26076.
fanquake marked this as a draft on Feb 16, 2023
DrahtBot commented at 12:12 AM on May 17, 2023: contributor


There hasn't been much activity lately and the patch still needs rebase. What is the status here?
- Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
- Is it no longer relevant? ➡️ Please close.
- Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.
maflcko commented at 6:14 AM on May 17, 2023: member

Drafted for now given it's (maybe) waiting on #26076.

That one was merged last week.
DrahtBot removed review request from achow101 on May 17, 2023
DrahtBot commented at 1:40 AM on August 15, 2023: contributor


There hasn't been much activity lately and the patch still needs rebase. What is the status here?
- Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
- Is it no longer relevant? ➡️ Please close.
- Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.
maflcko commented at 6:40 AM on August 15, 2023: member

@sipa ^
sipa commented at 11:09 PM on August 16, 2023: member

Leaving this as up for grabs. I still think it's a good idea, but don't have time to pursue it now.
sipa added the label Up for grabs on Aug 16, 2023
sipa closed this on Aug 16, 2023
bitcoin locked this on Aug 15, 2024