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

pythcoiner commented at 4:52 pm on January 24, 2025: contributor

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 commented at 4:52 pm on January 24, 2025: contributor

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
ACK	sipa, achow101
Concept ACK	brunoerg, 1440000bytes, rkrux

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 (#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.

pythcoiner renamed this:
~~Miniscriptdescriptor to string~~
Account for all `StringType` variants in `Miniscriptdescriptor::ToString()`
on Jan 24, 2025

pythcoiner renamed this:
~~Account for all `StringType` variants in `Miniscriptdescriptor::ToString()`~~
miniscript: ccount for all `StringType` variants in `Miniscriptdescriptor::ToString()`
on Jan 24, 2025

DrahtBot added the label Descriptors on Jan 24, 2025

pythcoiner renamed this:
~~miniscript: ccount for all `StringType` variants in `Miniscriptdescriptor::ToString()`~~
miniscript: acount for all `StringType` variants in `Miniscriptdescriptor::ToString()`
on Jan 24, 2025

pythcoiner force-pushed on Jan 24, 2025

DrahtBot added the label CI failed on Jan 24, 2025

DrahtBot commented at 4:58 pm on January 24, 2025: contributor

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

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.

pythcoiner marked this as a draft on Jan 25, 2025

pythcoiner force-pushed on Jan 25, 2025

DrahtBot removed the label CI failed on Jan 25, 2025

pythcoiner marked this as ready for review on Jan 25, 2025

DrahtBot added the label CI failed on Jan 26, 2025

DrahtBot removed the label CI failed on Jan 26, 2025

in test/functional/wallet_listdescriptors.py:140 in 0bc213db76 outdated

136@@ -137,7 +137,7 @@ def run_test(self):
137         self.log.info('Test taproot descriptor do not have mixed hardened derivation marker')
138         node.createwallet(wallet_name='w5', descriptors=True, disable_private_keys=True)
139         wallet = node.get_wallet_rpc('w5')
140-        imp = wallet.importdescriptors([{

hodlinator commented at 3:27 pm on January 29, 2025:

In commit 0bc213db7686df9ccbd5ef76c5569181851d80a2: This change should be part of the other commit.

pythcoiner commented at 2:54 am on January 30, 2025:

done

in src/script/descriptor.cpp:1291 in 0bc213db76 outdated

1285@@ -1286,20 +1286,31 @@ class StringMaker {
1286     const SigningProvider* m_arg;
1287     //! Keys contained in the Miniscript (a reference to DescriptorImpl::m_pubkey_args).
1288     const std::vector<std::unique_ptr<PubkeyProvider>>& m_pubkeys;
1289-    //! Whether to serialize keys as private or public.
1290-    bool m_private;
1291+    //! StringType to serialize keys
1292+    DescriptorImpl::StringType m_type;
1293+    std::unique_ptr<DescriptorCache> m_cache;

hodlinator commented at 3:28 pm on January 29, 2025:

Seems okay to store as raw pointer, if we take it as such as an argument into the StringMaker constructor?

0    const DescriptorCache* m_cache;

pythcoiner commented at 2:54 am on January 30, 2025:

done

in src/script/descriptor.cpp:1290 in 0bc213db76 outdated

1285@@ -1286,20 +1286,31 @@ class StringMaker {
1286     const SigningProvider* m_arg;
1287     //! Keys contained in the Miniscript (a reference to DescriptorImpl::m_pubkey_args).
1288     const std::vector<std::unique_ptr<PubkeyProvider>>& m_pubkeys;
1289-    //! Whether to serialize keys as private or public.
1290-    bool m_private;
1291+    //! StringType to serialize keys
1292+    DescriptorImpl::StringType m_type;

hodlinator commented at 3:33 pm on January 29, 2025:

nit: Could keep style with rest of the fields:

0    const DescriptorImpl::StringType m_type;

pythcoiner commented at 2:54 am on January 30, 2025:

done

in src/script/descriptor.cpp:1346 in 0bc213db76 outdated

1342@@ -1332,7 +1343,7 @@ class MiniscriptDescriptor final : public DescriptorImpl
1343     bool ToStringHelper(const SigningProvider* arg, std::string& out, const StringType type,
1344                         const DescriptorCache* cache = nullptr) const override
1345     {
1346-        if (const auto res = m_node->ToString(StringMaker(arg, m_pubkey_args, type == StringType::PRIVATE))) {
1347+        if (const auto res = m_node->ToString(StringMaker(arg, m_pubkey_args, type))) {

hodlinator commented at 3:42 pm on January 29, 2025:

Should probably forward on the cache argument to the StringMaker.

0        if (const auto res = m_node->ToString(StringMaker{arg, m_pubkey_args, type, cache})) {

pythcoiner commented at 2:54 am on January 30, 2025:

done

hodlinator commented at 7:36 pm on January 29, 2025: contributor

Code review 0bc213db7686df9ccbd5ef76c5569181851d80a2

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

0- 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.
1+ 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).

pythcoiner renamed this:
~~miniscript: acount for all `StringType` variants in `Miniscriptdescriptor::ToString()`~~
miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`
on Jan 29, 2025

pythcoiner force-pushed on Jan 30, 2025

pythcoiner commented at 2:55 am on January 30, 2025: contributor

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

1440000bytes commented at 6:19 pm on February 1, 2025: none

Concept ACK

in src/script/descriptor.cpp:1297 in 498c47d931 outdated

1296-    StringMaker(const SigningProvider* arg LIFETIMEBOUND, const std::vector<std::unique_ptr<PubkeyProvider>>& pubkeys LIFETIMEBOUND, bool priv)
1297-        : m_arg(arg), m_pubkeys(pubkeys), m_private(priv) {}
1298+    StringMaker(const SigningProvider* arg LIFETIMEBOUND,
1299+                const std::vector<std::unique_ptr<PubkeyProvider>>& pubkeys LIFETIMEBOUND,
1300+                DescriptorImpl::StringType type,
1301+                const DescriptorCache* cache)

hodlinator commented at 9:16 pm on February 1, 2025:

0                const DescriptorCache* cache LIFETIMEBOUND)

Not sure how much it actually helps, especially on raw pointers, but at least LIFETIMEBOUND documents expectations, and is already used on the first arg which is also a raw pointer. See: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#lifetimebound

pythcoiner commented at 8:14 am on February 6, 2025:

done

in test/functional/wallet_listdescriptors.py:141 in 498c47d931 outdated

133@@ -134,6 +134,26 @@ def run_test(self):
134         }
135         assert_equal(expected, wallet.listdescriptors())
136 
137+        self.log.info('Test taproot descriptor do not have mixed hardened derivation marker')
138+        node.createwallet(wallet_name='w5', descriptors=True, disable_private_keys=True)
139+        wallet = node.get_wallet_rpc('w5')
140+        wallet.importdescriptors([{
141+            'desc': 'tr([1dce71b2/48\'/1\'/0\'/2\']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/0/*,and_v(v:pk([1dce71b2/48\'/1\'/0\'/2\']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/2/*),older(65535)))#xhrh0cvn',

hodlinator commented at 9:32 pm on February 1, 2025:

nit: More readable if you think it’s tolerable to use double-quotes in this one case.

0            'desc': "tr([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/0/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/2/*),older(65535)))#xhrh0cvn",

hodlinator commented at 2:20 pm on February 5, 2025:

nit: When debugging the test, it was annoying that both of the keys had identical beginnings (

[1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/

), deriving them from different master keys with different derivation paths would help keep them apart.

pythcoiner commented at 6:25 am on February 6, 2025:

I go to the \' way instead because python linters often yell w/ double quotes but I’m fine w/ double quote if it’s fit the CI

pythcoiner commented at 8:14 am on February 6, 2025:

done

hodlinator commented at 9:18 pm on February 6, 2025:

I ran the test/lint/test_runner/ runner (COMMIT_RANGE="HEAD~3..HEAD" cargo run) on my suggestion (which runs ruff if it’s installed). It did not emit any errors, so I think you should be fine.

Couldn’t find the precise rule against my suggestion, but Q003 seems like it would be in favor of it: https://docs.astral.sh/ruff/rules/#flake8-pytest-style-pt

pythcoiner commented at 9:15 am on February 7, 2025:

done

brunoerg commented at 2:48 pm on February 3, 2025: contributor

Concept ACK

hodlinator commented at 3:05 pm on February 5, 2025: contributor

Code review 498c47d9315ef79c107460b678f834d19cb9a53e

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 bfb036756ad6e187fd6d3abfefe5804bb54a5c71 by @darosior and @sipa.

TODO

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

pythcoiner force-pushed on Feb 6, 2025

in src/script/descriptor.cpp:5 in a5c0516487 outdated

1@@ -2,6 +2,7 @@
2 // Distributed under the MIT software license, see the accompanying
3 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
4 
5+#include "attributes.h"

hodlinator commented at 9:06 pm on February 6, 2025:

Causes linter failure:

0[08:12:43.311] Please use bracket syntax includes ("#include <foo.h>") instead of quote syntax includes:
1[08:12:43.311] src/script/descriptor.cpp:#include "attributes.h"

0#include <attributes.h>

pythcoiner commented at 9:16 am on February 7, 2025:

my bad, my clangd add it automatically and I don’t notice. I drop it as it seems redundant

descriptor: account for all StringType in MiniscriptDescriptor::ToStringHelper() 975783cb79

test: check listdescriptors do not return a mix of hardened derivation marker 28a4fcb03c

pythcoiner force-pushed on Feb 7, 2025

pythcoiner commented at 9:39 am on February 7, 2025: contributor

tahnks for your time @hodlinator!

fanquake commented at 8:57 am on March 21, 2025: member

cc @darosior

achow101 requested review from achow101 on Oct 22, 2025

achow101 requested review from sipa on Oct 22, 2025

sipa commented at 6:12 pm on November 3, 2025: member

Code review ACK 28a4fcb03c0fb1cd5112eca1eb36dcb13e0b4ff2

Conceptually seems fine too, but I’d want to see @achow101’s opinion on that.

DrahtBot requested review from brunoerg on Nov 3, 2025

in test/functional/wallet_listdescriptors.py:137 in 28a4fcb03c

133@@ -134,6 +134,26 @@ def run_test(self):
134         }
135         assert_equal(expected, wallet.listdescriptors())
136 
137+        self.log.info('Test taproot descriptor do not have mixed hardened derivation marker')

rkrux commented at 12:34 pm on November 10, 2025:

Since the issue is not specific to taproot descriptors and instead the ones containing MiniScript expression.

0-        self.log.info('Test taproot descriptor do not have mixed hardened derivation marker')
1+        self.log.info('Test descriptors do not have mixed hardened derivation marker')

in test/functional/wallet_listdescriptors.py:143 in 28a4fcb03c

138+        node.createwallet(wallet_name='w5', descriptors=True, disable_private_keys=True)
139+        wallet = node.get_wallet_rpc('w5')
140+        wallet.importdescriptors([{
141+            'desc': "tr([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/0/*,and_v(v:pk([c658b283/48'/1'/0'/2']tpubDFL5wzgPBYK5pZ2Kh1T8qrxnp43kjE5CXfguZHHBrZSWpkfASy5rVfj7prh11XdqkC1P3kRwUPBeX7AHN8XBNx8UwiprnFnEm5jyswiRD4p/0/*),older(65535)))#xl20m6md",
142+            'timestamp': TIME_GENESIS_BLOCK,
143+        }])

rkrux commented at 12:37 pm on November 10, 2025:

 0         wallet = node.get_wallet_rpc('w5')
 1+        tr_desc = "tr([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2Aoy
 2iKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/0/*,and_v(v:pk([c658b283/48'/1'/0'/2']tpubDFL5wzgPBYK5pZ2Kh1T8
 3qrxnp43kjE5CXfguZHHBrZSWpkfASy5rVfj7prh11XdqkC1P3kRwUPBeX7AHN8XBNx8UwiprnFnEm5jyswiRD4p/0/*),older(65535)))"
 4+        wsh_desc = "wsh(or_d(pk([a233d117/48'/1'/0'/2']tpubDF8d1Q2U8WWfxUHMiqqrYiavBReX2r7hwD7oQsEuq1AiXj5nJc
 5LBkoh1uTLZVx66VrbRfeHyg5bSFvRKzesa1yzYuXDvawZeRi9m97a2Qzd/0/*),and_v(v:pkh([a233d117/48'/1'/0'/2']tpubDF8d1Q2U8WWfxUHMiqqrYiavBReX2r7hwD7oQsEuq1AiXj5nJcLBkoh1uTLZVx66VrbRfeHyg5bSFvRKzesa1yzYuXDvawZeRi9m97a2Qzd/2/*),older(65535))))"
 6         wallet.importdescriptors([{
 7-            'desc': "tr([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/0/*,and_v(v:pk([c658b283/48'/1'/0'/2']tpubDFL5wzgPBYK5pZ2Kh1T8qrxnp43kjE5CXfguZHHBrZSWpkfASy5rVfj7prh11XdqkC1P3kRwUPBeX7AHN8XBNx8UwiprnFnEm5jyswiRD4p/0/*),older(65535)))#xl20m6md",
 8+            'desc': descsum_create(tr_desc),
 9+            'timestamp': TIME_GENESIS_BLOCK,
10+        }, {
11+            'desc': descsum_create(wsh_desc),
12             'timestamp': TIME_GENESIS_BLOCK,
13         }])
14         expected = {
15             'wallet_name': 'w5',
16             'descriptors': [
17                 {'active': False,
18-                 'desc': 'tr([1dce71b2/48h/1h/0h/2h]tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/0/*,and_v(v:pk([c658b283/48h/1h/0h/2h]tpubDFL5wzgPBYK5pZ2Kh1T8qrxnp43kjE5CXfguZHHBrZSWpkfASy5rVfj7prh11XdqkC1P3kRwUPBeX7AHN8XBNx8UwiprnFnEm5jyswiRD4p/0/*),older(65535)))#m4uznndk',
19+                 'desc': descsum_create(tr_desc.replace("'", "h")),
20                  'timestamp': TIME_GENESIS_BLOCK,
21                  'range': [0, 0],
22                  'next': 0,
23-                 'next_index': 0},
24+                 'next_index': 0
25+                },
26+                {'active': False,
27+                 'desc': descsum_create(wsh_desc.replace("'", "h")),
28+                 'timestamp': TIME_GENESIS_BLOCK,
29+                 'range': [0, 0],
30+                 'next': 0,
31+                 'next_index': 0
32+                },

Would be good to add a case for wsh descriptor as well.
Using descsum_create() avoids hardcoding the checksum and also makes it easier to debug the tests as well.

rkrux commented at 12:41 pm on November 10, 2025: contributor

Concept ACK 28a4fcb03c0fb1cd5112eca1eb36dcb13e0b4ff2

Thanks for adding the fix!

The PR description can be modified slightly to emphasise that the issue is with Miniscript expressions and not only the Taproot descriptor.

achow101 commented at 10:24 pm on November 18, 2025: member

ACK 28a4fcb03c0fb1cd5112eca1eb36dcb13e0b4ff2

DrahtBot requested review from rkrux on Nov 18, 2025

achow101 merged this on Nov 18, 2025

achow101 closed this on Nov 18, 2025

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

Code Coverage & Benchmarks

Reviews

Conflicts

PR title/summary

PR title typo

Summary

Commit order

Since last review

Concept

TODO

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