miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()` #31734

pythcoiner · 2025-01-24T16:52:40Z

In MiniscriptDescriptor::ToStringHelper() only the StringType::Private variant of the type argument was handled. This PR implements serializing w/ all variants of StringType & add a functional test for the descriptor triggering the related issue.

Closes #31694: previously when calling listdescriptors RPC on a wallet containing a taproot descriptor w/ a (miniscript) taptree, origins of internal key & taptree were serialized w/ differents hardened derivation markers:

origin of the internal key were serialized w/ StringType::Normalized type (using h as marker)
origins of taptree keys were serialized w/ StringType::Private type (using ' as marker)

Note: Origins in segwit (wsh()) miniscript descriptors were also serialized w/ StringType::Private type (' marker) and are now serialized w/ StringType::Normalized type (h marker).

DrahtBot · 2025-01-24T16:52:43Z

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31734.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	1440000bytes, brunoerg

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:

#32471 (wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key by Eunovo)
#31713 (miniscript refactor: Remove unique_ptr-indirection (descriptor: Add proper Clone function to miniscript::Node #30866 follow-up) by hodlinator)

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.

DrahtBot · 2025-01-24T16:58:47Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/36134804974}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

hodlinator

Code review 0bc213d

Thanks for taking on this issue! Not very familiar with this area but maybe I can assist in refining the PR anyway. Seems directionally correct to use h consistently for hardened derivation paths unless backwards compatibility is requested.

PR title/summary

PR title typo

acount -> account

Summary

- In `MiniscriptDescriptor::ToStringHelper()` only `StringType::Private` variant of `type` argument was accounted. This PR implement serializing w/ all variants of `StringType` & add a functionnal test for the descriptor triggereing the related issue.
+ In `MiniscriptDescriptor::ToStringHelper()` only the `StringType::Private` variant of the `type` argument was handled. This PR implements serializing w/ all variants of `StringType` & add a functional test for the descriptor triggering the related issue.

Maybe the summary could also include a brief explanation of why this change ends up fixing the issue?

Commit order

The commit order is wrong for our CI setup, adding the test before the fix. Our CI expects each commit to succeed, so the test needs to be added last (reviewers can drop the other commit or run with binaries from master in order to trigger test failures).

test/functional/wallet_listdescriptors.py

src/script/descriptor.cpp

pythcoiner · 2025-01-30T02:55:29Z

Thanks for the review @hodlinator! comments addressed & added more details to the summary

brunoerg · 2025-02-03T14:48:17Z

Concept ACK

hodlinator

Code review 498c47d

Since last review

git range-diff master 0bc213d 498c47d

Reordered commits and avoid touching new test code in 2 commits. ✅
Forwards on cache argument from calling function, changed types and const. ✅
Improved PR title and summary. ✅

Concept

Before the PR, first key origin/derivation path in the descriptor undergoes ' -> h conversion, but the second one does not. There seems to be some code in descriptor.cpp that deliberately tries to preserve apostrophes. I'm unsure whether the change breaks any existing requirements. My instinct says software depending on Bitcoin Core should handle h in all cases. But there may be cases where changing the format leads to errors, worst case leading to users having problems recovering funds. If we go ahead with this PR, it should probably have release notes (example of PR with release notes).

Wallet and surrounding code is a beast that I don't have enough familiarity with to produce a Concept A-C-K at this point. Main code being changed seems to be from bfb0367 by @darosior and @sipa.

TODO

Lacking LIFETIMEBOUND (see inline comment).
nit: PR summary typos: "differents hardened derivation marker" -> "different hardened derivation markers"

src/script/descriptor.cpp

test/functional/wallet_listdescriptors.py

src/script/descriptor.cpp

…ingHelper()

…n marker

pythcoiner · 2025-02-07T09:39:38Z

tahnks for your time @hodlinator!

fanquake · 2025-03-21T08:57:57Z

cc @darosior

pythcoiner changed the title ~~Miniscriptdescriptor to string~~ Account for all StringType variants in Miniscriptdescriptor::ToString() Jan 24, 2025

pythcoiner changed the title ~~Account for all StringType variants in Miniscriptdescriptor::ToString()~~ miniscript: ccount for all StringType variants in Miniscriptdescriptor::ToString() Jan 24, 2025

DrahtBot added the Descriptors label Jan 24, 2025

pythcoiner changed the title ~~miniscript: ccount for all StringType variants in Miniscriptdescriptor::ToString()~~ miniscript: acount for all StringType variants in Miniscriptdescriptor::ToString() Jan 24, 2025

pythcoiner force-pushed the miniscriptdescriptor_to_string branch from 2c70728 to 87be050 Compare January 24, 2025 16:58

DrahtBot added the CI failed label Jan 24, 2025

pythcoiner marked this pull request as draft January 25, 2025 00:38

pythcoiner force-pushed the miniscriptdescriptor_to_string branch 2 times, most recently from 8ce78e5 to 0bc213d Compare January 25, 2025 01:18

DrahtBot removed the CI failed label Jan 25, 2025

pythcoiner marked this pull request as ready for review January 25, 2025 02:04

DrahtBot mentioned this pull request Jan 25, 2025

miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up) #31713

Open

DrahtBot added CI failed and removed CI failed labels Jan 26, 2025

hodlinator reviewed Jan 29, 2025

View reviewed changes

test/functional/wallet_listdescriptors.py Outdated Show resolved Hide resolved

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

pythcoiner changed the title ~~miniscript: acount for all StringType variants in Miniscriptdescriptor::ToString()~~ miniscript: account for all StringType variants in Miniscriptdescriptor::ToString() Jan 29, 2025

pythcoiner force-pushed the miniscriptdescriptor_to_string branch from 0bc213d to 498c47d Compare January 30, 2025 02:10

This comment was marked as abuse.

Sign in to view

hodlinator reviewed Feb 5, 2025

View reviewed changes

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

test/functional/wallet_listdescriptors.py Outdated Show resolved Hide resolved

test/functional/wallet_listdescriptors.py Outdated Show resolved Hide resolved

pythcoiner force-pushed the miniscriptdescriptor_to_string branch from 498c47d to a5c0516 Compare February 6, 2025 08:11

hodlinator reviewed Feb 6, 2025

View reviewed changes

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

pythcoiner added 2 commits February 7, 2025 10:05

descriptor: account for all StringType in MiniscriptDescriptor::ToStr…

975783c

…ingHelper()

test: check listdescriptors do not return a mix of hardened derivatio…

28a4fcb

…n marker

pythcoiner force-pushed the miniscriptdescriptor_to_string branch from a5c0516 to 28a4fcb Compare February 7, 2025 09:13

hodlinator mentioned this pull request Feb 13, 2025

qa debug: Add --debug_runs/-waitfordebugger [DRAFT] #31723

Draft

DrahtBot mentioned this pull request Apr 10, 2025

descriptor: handle listdescriptors(private=true) for descriptors having partial keys #32186

Closed

DrahtBot mentioned this pull request May 12, 2025

wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key #32471

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()` #31734

miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()` #31734

Uh oh!

pythcoiner commented Jan 24, 2025 •

edited

Loading

Uh oh!

DrahtBot commented Jan 24, 2025 •

edited

Loading

Uh oh!

DrahtBot commented Jan 24, 2025

Uh oh!

hodlinator left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pythcoiner commented Jan 30, 2025

Uh oh!

This comment was marked as abuse.

brunoerg commented Feb 3, 2025

Uh oh!

hodlinator left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pythcoiner commented Feb 7, 2025

Uh oh!

fanquake commented Mar 21, 2025

Uh oh!

Uh oh!

miniscript: account for all StringType variants in Miniscriptdescriptor::ToString() #31734

Are you sure you want to change the base?

miniscript: account for all StringType variants in Miniscriptdescriptor::ToString() #31734

Uh oh!

Conversation

pythcoiner commented Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented Jan 24, 2025

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

PR title/summary

PR title typo

Summary

Commit order

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pythcoiner commented Jan 30, 2025

Uh oh!

This comment was marked as abuse.

brunoerg commented Feb 3, 2025

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Since last review

Concept

TODO

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pythcoiner commented Feb 7, 2025

Uh oh!

fanquake commented Mar 21, 2025

Uh oh!

Uh oh!

miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()` #31734

miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()` #31734

pythcoiner commented Jan 24, 2025 •

edited

Loading

DrahtBot commented Jan 24, 2025 •

edited

Loading