The miniscript parser currently only looks for the next ) when parsing key, hash, and locktime expressions. This fails to parse when the expressions contain a nested expression. Currently, this is only possible with musig() inside of key expressions. However, this pattern can be generalized to handling hashes and locktimes, so I implemented those too.
Fixes #34076
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34141.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | sipa |
| Stale ACK | scgbckbone |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Reviewers, this pull request conflicts with the following ones:
exportwatchonlywallet RPC to export a watchonly version of a wallet 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.
Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):
2026-02-02 23:25:49
🚧 At least one of the CI tasks failed.
Task TSan: https://github.com/bitcoin/bitcoin/actions/runs/20444643161/job/58745329801
LLM reason (✨ experimental): descriptor_tests failed with Subprocess aborted (CTest exit code 8).
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.
Import now works OK, but there is a problem with address generation. Test case:
0self.test_success_case("time-locked musig",
1 "tr(musig($0,$1,$2)/<0;1>/*,{and_v(v:pk(musig($0,$1)/<0;1>/*),after(1)),{and_v(v:pk(musig($0,$2)/<0;1>/*),after(1)),and_v(v:pk(musig($1,$2)/<0;1>/*),after(1))}})",
2 scriptpath=True)
Particular case:
0core_desc_object = [{'desc': 'tr(musig([0f056943/86h/1h/0h]tpubDCeEX49avtiXrBTv3JWTtco99Ka499jXdZHBRtm7va2gkMAui11ctZjqNAT9dLVNaEozt2C1kfTM88cnvZCXsWLJN2p4viGvsyGjtKVV7A1,[fdc7b3da/44h/1h/0h]tpubDDdbJUqUksL8pDjkh18Z6YC3kvBW7zufRKnawvvToRh3oTYVvuV3mNyEgav7VZf7JVMvSUxDxAc1MipVETVe6jmTzcaTNMRGf3Gh3eCQFsY,[44fbc2f9/44h/1h/0h]tpubDDMKx1DX6V3wfubHm2jSX11ToHMdYYwmtS3Sm3VJ5ky5efECGPa1kN3zN8hqdeEqrbqez33f8YHLARxUoEGkr2r68ViwXWAxhVcU8Tw1pSV)/<2;3>/*,{and_v(v:pk(musig([0f056943/86h/1h/0h]tpubDCeEX49avtiXrBTv3JWTtco99Ka499jXdZHBRtm7va2gkMAui11ctZjqNAT9dLVNaEozt2C1kfTM88cnvZCXsWLJN2p4viGvsyGjtKVV7A1,[44fbc2f9/44h/1h/0h]tpubDDMKx1DX6V3wfubHm2jSX11ToHMdYYwmtS3Sm3VJ5ky5efECGPa1kN3zN8hqdeEqrbqez33f8YHLARxUoEGkr2r68ViwXWAxhVcU8Tw1pSV)/<0;1>/*),older(10)),{and_v(v:pk(musig([0f056943/86h/1h/0h]tpubDCeEX49avtiXrBTv3JWTtco99Ka499jXdZHBRtm7va2gkMAui11ctZjqNAT9dLVNaEozt2C1kfTM88cnvZCXsWLJN2p4viGvsyGjtKVV7A1,[fdc7b3da/44h/1h/0h]tpubDDdbJUqUksL8pDjkh18Z6YC3kvBW7zufRKnawvvToRh3oTYVvuV3mNyEgav7VZf7JVMvSUxDxAc1MipVETVe6jmTzcaTNMRGf3Gh3eCQFsY)/<0;1>/*),older(10)),and_v(v:pk(musig([fdc7b3da/44h/1h/0h]tpubDDdbJUqUksL8pDjkh18Z6YC3kvBW7zufRKnawvvToRh3oTYVvuV3mNyEgav7VZf7JVMvSUxDxAc1MipVETVe6jmTzcaTNMRGf3Gh3eCQFsY,[44fbc2f9/44h/1h/0h]tpubDDMKx1DX6V3wfubHm2jSX11ToHMdYYwmtS3Sm3VJ5ky5efECGPa1kN3zN8hqdeEqrbqez33f8YHLARxUoEGkr2r68ViwXWAxhVcU8Tw1pSV)/<0;1>/*),older(10))}})#3xfc5d0t', 'active': True, 'range': [0, 100], 'timestamp': 'now'}]
1
2
3ms = bitcoind.create_wallet(
4 wallet_name=name, disable_private_keys=True,
5 blank=True, passphrase=None, avoid_reuse=False, descriptors=True
6)
7
8res = ms.importdescriptors(core_desc_object)
9# res = [{'success': True}]
10
11addr = ms.getnewaddress("", "bech32m")
12
13# just random other wallet with funds
14x = bitcoind.supply_wallet.sendtoaddress(addr, 49)
15# x = a166d63b05162525752a1c911454eb7e3e27a3f509b2efcd5d74069d98de4382
16
17# mine
18y =bitcoind.supply_wallet.generatetoaddress(1,bitcoind.supply_wallet.getnewaddress())
19# y = ['1826120d4e15d6c880ec5633ca74912bbae3a28c749a502de15379ce39af61a2']
20
21# must have unspent now - but we don't
22unspent = ms.listunspent()
23# unspent = []
24
25# first external address is not even recognized as ours
26ms.getaddressinfo(addr)
27# {'address': 'bcrt1pwvnfa5ypmfvnmcguwllehxqreq9ljrvz58hgeqxpwy9xqfta7r6scptqp0',
28# 'scriptPubKey': '512073269ed081da593de11c77ff9b9803c80bf90d82a1ee8c80c1710a60257df0f5',
29# 'ismine': False, 'solvable': False, 'iswatchonly': False, 'isscript': True, 'iswitness': True,
30# 'witness_version': 1,
31# 'witness_program': '73269ed081da593de11c77ff9b9803c80bf90d82a1ee8c80c1710a60257df0f5',
32# 'ischange': False, 'labels': ['']}
33
34# generating next address (index 1) works, and is recognized as ours
35next_addr = ms.getnewaddress("", "bech32m")
36bitcoind.supply_wallet.sendtoaddress(next_addr, 10)
37bitcoind.supply_wallet.generatetoaddress(1, bitcoind.supply_wallet.getnewaddress())
38ms.listunspent()
39# [{'txid': 'bd893fdd25dced57a193cc8b5fe15bc8b7309744c5156c7932712e4e34f7141f', 'vout': 1, 'address': 'bcrt1pgkt8zf54dgkpf66zt56etr8ksctwczn453ec4m2gqydknstuk7mqg0rrlu', 'label': '', 'scriptPubKey': '512045967126956a2c14eb425d35958cf68616ec0a75a4738aed48011b69c17cb7b6', 'amount': Decimal('10.00000000'), 'confirmations': 1, 'spendable': True, 'solvable': True, 'desc': 'tr([26cdffeb/2/1]9978b573defe38f5d849c2a076a6c666302d49c030ff160ebf40f7ee0a2662dc,{{and_v(v:pk([7a999231/0/1]2d40be15b6fd07671c9249a1ebe321e50e7f1332840cb560324613193e067efb),older(10)),and_v(v:pk([92d0b6b5/0/1]aa6d417376de11e860d2dc78280c6035ac547aa74f09514b4af630242efc7e37),older(10))},and_v(v:pk([0b835bd9/0/1]d30d1e36b2bcd571a07c40342d39e2afd498ac3c914ee50698bdf3df6b5f605f),older(10))})#7asktx0z', 'parent_descs': ['tr(musig([0f056943/86h/1h/0h]tpubDCeEX49avtiXrBTv3JWTtco99Ka499jXdZHBRtm7va2gkMAui11ctZjqNAT9dLVNaEozt2C1kfTM88cnvZCXsWLJN2p4viGvsyGjtKVV7A1,[fdc7b3da/44h/1h/0h]tpubDDdbJUqUksL8pDjkh18Z6YC3kvBW7zufRKnawvvToRh3oTYVvuV3mNyEgav7VZf7JVMvSUxDxAc1MipVETVe6jmTzcaTNMRGf3Gh3eCQFsY,[44fbc2f9/44h/1h/0h]tpubDDMKx1DX6V3wfubHm2jSX11ToHMdYYwmtS3Sm3VJ5ky5efECGPa1kN3zN8hqdeEqrbqez33f8YHLARxUoEGkr2r68ViwXWAxhVcU8Tw1pSV)/2/*,{and_v(v:pk(musig([0f056943/86h/1h/0h]tpubDCeEX49avtiXrBTv3JWTtco99Ka499jXdZHBRtm7va2gkMAui11ctZjqNAT9dLVNaEozt2C1kfTM88cnvZCXsWLJN2p4viGvsyGjtKVV7A1,[44fbc2f9/44h/1h/0h]tpubDDMKx1DX6V3wfubHm2jSX11ToHMdYYwmtS3Sm3VJ5ky5efECGPa1kN3zN8hqdeEqrbqez33f8YHLARxUoEGkr2r68ViwXWAxhVcU8Tw1pSV)/0/*),older(10)),{and_v(v:pk(musig([0f056943/86h/1h/0h]tpubDCeEX49avtiXrBTv3JWTtco99Ka499jXdZHBRtm7va2gkMAui11ctZjqNAT9dLVNaEozt2C1kfTM88cnvZCXsWLJN2p4viGvsyGjtKVV7A1,[fdc7b3da/44h/1h/0h]tpubDDdbJUqUksL8pDjkh18Z6YC3kvBW7zufRKnawvvToRh3oTYVvuV3mNyEgav7VZf7JVMvSUxDxAc1MipVETVe6jmTzcaTNMRGf3Gh3eCQFsY)/0/*),older(10)),and_v(v:pk(musig([fdc7b3da/44h/1h/0h]tpubDDdbJUqUksL8pDjkh18Z6YC3kvBW7zufRKnawvvToRh3oTYVvuV3mNyEgav7VZf7JVMvSUxDxAc1MipVETVe6jmTzcaTNMRGf3Gh3eCQFsY,[44fbc2f9/44h/1h/0h]tpubDDMKx1DX6V3wfubHm2jSX11ToHMdYYwmtS3Sm3VJ5ky5efECGPa1kN3zN8hqdeEqrbqez33f8YHLARxUoEGkr2r68ViwXWAxhVcU8Tw1pSV)/0/*),older(10))}})#862yfmft'], 'safe': True}]
According to my calculations the address is still incorrect even tho recognized as is_mine. My app calculated first two external addresses:
Core calculated addresses with getnewaddress:
Funnily enough, when I use external descriptor from listdescriptors and run deriveaddresses it matches what my app calculates, not what getnewaddress provides.
The CI fails due to UB:
0/cxx_build/include/c++/v1/__iterator/bounded_iter.h:123: libc++ Hardening assertion __current_ != __end_ failed: __bounded_iter::operator*: Attempt to dereference an iterator at the end
2060+ uint32_t& m_expr_index;
2061
2062 KeyParser(FlatSigningProvider* out LIFETIMEBOUND, const SigningProvider* in LIFETIMEBOUND,
2063- miniscript::MiniscriptContext ctx, uint32_t offset = 0)
2064- : m_out(out), m_in(in), m_script_ctx(ctx), m_offset(offset) {}
2065+ miniscript::MiniscriptContext ctx, uint32_t& key_exp_index)
In commit “miniscript: Use a reference to key_exp_index in KeyParser”
Nit: worth putting a LIFETIMEBOUND on the key_exp_index argument here?
1747@@ -1748,27 +1748,25 @@ int FindNextChar(std::span<const char> in, const char m);
1748
1749 /** Parse a key string ending at the end of the fragment's text representation. */
In commit “miniscript: Using Func and Expr when parsing keys, hashes, and locktimes”
This comment seems outdated now, as it will also stop at commas (plus the comment should probably explain the new func argument).
1759+ std::span<const char> expr = script::Expr(in);
1760+ if (!script::Func(func, expr)) return {};
1761+ return ctx.FromString(expr);
1762 }
1763
1764 /** Parse a hex string ending at the end of the fragment's text representation. */
In commit “miniscript: Using Func and Expr when parsing keys, hashes, and locktimes”
Same here.
2174@@ -2174,7 +2175,6 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
2175 error = strprintf("combo(): %s", error);
2176 return {};
2177 }
2178- ++key_exp_index;
I think this is actually quite hard to test for currently. For top level single key expressions like combo(), incrementing key_exp_index fundamentally doesn’t matter as it’s not going to be used by anything afterwards.
Really the only place that we could observe a difference in behavior is in looking up the wrong key in the cache. But so long as the incrementing is consistent, I don’t think there even is any observable misbehavior. All that is necessary is that the expression index matches the expression index in the cache, so it can be wrong as long as it is consistently wrong.
The reason that this change is necessary is because musig wasn’t even incrementing at all which led to the cache entries colliding and being overwritten.
Thanks for explaining.
So the concern is about whether two keys might be ending up on the same index. Is there a way to test that? E.g. count the keys anywhere in an expression, and see that the number of used cache positions at the end equals that?
Descriptor, and I’m not entirely sure that I like this approach. But it did reveal a bug in the counting for OriginPubkeyProvider.
1801- int hash_size = FindNextChar(in, ')');
1802- if (hash_size < 1) return {};
1803- std::string val = std::string(in.begin(), in.begin() + hash_size);
1804+ std::span<const char> expr = script::Expr(in);
1805+ if (!script::Func(func, expr)) return {};
1806+ std::string val = std::string(expr.begin(), expr.end());
std::string_views instead, so these conversions aren’t needed.
1046@@ -1041,6 +1047,32 @@ class DescriptorImpl : public Descriptor
1047 }
1048 return all;
1049 }
1050+
1051+ // NOLINTNEXTLINE(misc-no-recursion)
1052+ uint32_t GetMaxKeyExpr() const override
GetMaxKeyExpr() implemented for each *Descriptor class? I actually had done that originally, but the implementations were all just about close enough to identical that it seemed pointless to do that.
(untested)
0 uint32_t GetMaxKeyExpr() const final
1 {
2 uint32_t max_key_expr{0};
3 std::vector<const DescriptorImpl*> todo = {this};
4 while (!todo.empty()) {
5 const DescriptorImpl* desc = todo.back();
6 todo.pop_back();
7 for (const auto& p : desc->m_pubkey_args) {
8 max_key_expr = std::max(max_key_expr, p->m_expr_index);
9 }
10 for (const auto& s : desc->m_subdescriptor_args) todo.push_back(s.get());
11 }
12 return max_key_expr;
13}
14
15size_t GetKeyCount() const final
16{
17 size_t count{0};
18 std::vector<const DescriptorImpl*> todo = {this};
19 while (!todo.empty()) {
20 const DescriptorImpl* desc = todo.back();
21 todo.pop_back();
22 for (const auto& p : desc->m_pubkey_args) count += p->GetKeyCount();
23 for (const auto& s : desc->m_subdescriptor_args) todo.push_back(s.get());
24 }
25 return count;
26}
(note the final; this doesn’t support individually overridden versions)
crACK 2d44d4176b1f1d790e71aa496c18f3c8229b2a89. I added this, and spent some time fuzzing the descriptor_parse and mocked_descriptor_parse fuzz tests:
0diff --git a/src/test/fuzz/descriptor_parse.cpp b/src/test/fuzz/descriptor_parse.cpp
1index eeba5f1369e..6b3084e23d1 100644
2--- a/src/test/fuzz/descriptor_parse.cpp
3+++ b/src/test/fuzz/descriptor_parse.cpp
4@@ -61,6 +61,10 @@ static void TestDescriptor(const Descriptor& desc, FlatSigningProvider& sig_prov
5 const bool is_nontop_or_nonsolvable{!*is_solvable || !desc.GetOutputType()};
6 const bool is_input_size_info_set{max_sat_maxsig && max_sat_nonmaxsig && max_elems};
7 assert(is_input_size_info_set || is_nontop_or_nonsolvable);
8+
9+ auto max_key_expr = desc.GetMaxKeyExpr();
10+ auto key_count = desc.GetKeyCount();
11+ assert((max_key_expr == 0 && key_count == 0) || max_key_expr + 1 == key_count);
12 }
13
14 void initialize_descriptor_parse()
Partially reverting the diff here doesn’t catch any errors:
0diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
1index 298a9501e61..9a5600ae444 100644
2--- a/src/script/descriptor.cpp
3+++ b/src/script/descriptor.cpp
4@@ -2285,6 +2285,7 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
5 error = strprintf("pkh(): %s", error);
6 return {};
7 }
8+ ++key_exp_index;
9 for (auto& pubkey : pubkeys) {
10 ret.emplace_back(std::make_unique<PKHDescriptor>(std::move(pubkey)));
11 }
12@@ -2296,6 +2297,7 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
13 error = strprintf("combo(): %s", error);
14 return {};
15 }
16+ ++key_exp_index;
17 for (auto& pubkey : pubkeys) {
18 ret.emplace_back(std::make_unique<ComboDescriptor>(std::move(pubkey)));
19 }
20@@ -2404,6 +2406,7 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
21 error = strprintf("wpkh(): %s", error);
22 return {};
23 }
24+ ++key_exp_index;
25 for (auto& pubkey : pubkeys) {
26 ret.emplace_back(std::make_unique<WPKHDescriptor>(std::move(pubkey)));
27 }
But, adding
0diff --git a/src/test/fuzz/descriptor_parse.cpp b/src/test/fuzz/descriptor_parse.cpp
1index eeba5f1369e..6b3084e23d1 100644
2--- a/src/test/fuzz/descriptor_parse.cpp
3+++ b/src/test/fuzz/descriptor_parse.cpp
4@@ -61,6 +61,10 @@ static void TestDescriptor(const Descriptor& desc, FlatSigningProvider& sig_prov
5 const bool is_nontop_or_nonsolvable{!*is_solvable || !desc.GetOutputType()};
6 const bool is_input_size_info_set{max_sat_maxsig && max_sat_nonmaxsig && max_elems};
7 assert(is_input_size_info_set || is_nontop_or_nonsolvable);
8+
9+ auto max_key_expr = desc.GetMaxKeyExpr();
10+ auto key_count = desc.GetKeyCount();
11+ assert((max_key_expr == 0 && key_count == 0) || max_key_expr + 1 == key_count);
12 }
13
14 void initialize_descriptor_parse()
to the fuzz tests doesn’t catch the above mutations either, which leads me to suspect that diff maybe not actually have an observable effect?
which leads me to suspect that diff maybe not actually have an observable effect?
Indeed. The three lines reverted were for incrementing key_exp_index after parsing the key for pkh(), combo(), and wpkh(). All of these are expressions in contexts which do not allow any expressions after these are parsed, so incrementing key_exp_index isn’t actually doing anything as it is never passed into another PubkeyProvider. These lines were in fact redundant in the original code.
For key_exp_index to count correctly for miniscript expressions,
KeyParser should hold a reference to it.
Simplifies the callsites for incrementing key_exp_index
Since pk(), pk_k(), pkh(), pk_h(), sha256(), ripemd160(), hash256(),
hash160(), after(), and older() all are single argument expressions that
are parsed immediately, we can use the Expr and Func parsing functions
to determine what the arguments of these expressions are, rather than
searching for the next closing parentheses.
This fixes an issue when pk(), pk_k(), pkh(), and pk_h() include a
musig() expression as Expr properly handles nested expressions.