Descriptor unit tests and simplifications #24361
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
sipa
force-pushed
the
202202_desctests
branch
from
c76c38b
to
2ab644e
Compare
|
Added a commit, suggested by @achow101, to merge ToString and ToPrivateString completely. |
|
idk, I think it makes more sense to have two separate methods rather than different security-sensitivity based on an argument. |
|
@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. |
|
Rebased. |
|
ACK 5f2af0c |
| @@ -103,7 +103,7 @@ def run_test(self): | |||
| 'desc': descsum_create('wpkh(' + xpub_acc + ')'), | |||
| 'timestamp': 1296688602, | |||
| }]) | |||
| assert_raises_rpc_error(-4, 'Can\'t get descriptor string', watch_only_wallet.listdescriptors, True) | |||
| assert_equal(watch_only_wallet.listdescriptors(True), watch_only_wallet.listdescriptors(False)) | |||
There was a problem hiding this comment.
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?
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
|
Are you still working on this? |
|
I do plan to pick this up soon (perhaps after #26076). |
|
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
Drafted for now given it's (maybe) waiting on #26076. |
|
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
That one was merged last week. |
|
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
@sipa ^ |
|
Leaving this as up for grabs. I still think it's a good idea, but don't have time to pursue it now. |
Builds on top of #24343.
Adds additional tests, and makes
ToPrivateString()always succeed, using pubkeys in case privkeys are unavailable.