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, rkrux |
| Approach ACK | darosior |
| 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.
musig() while parsing Miniscript, will review soon.
Doesn’t having the repro as the first commit break bisectability?
Other than that, Approach ACK 4b53cbd69220c1c786bb23a72c0b26a6f78a38f7. That makes sense to me but i have not closely reviewed the code.
Doesn’t having the repro as the first commit break bisectability?
It isn’t. The first commit is the test for key expression count, whose behavior is not supposed to change in this PR.
180@@ -181,6 +181,12 @@ struct Descriptor {
181
182 /** Semantic/safety warnings (includes subdescriptors). */
183 virtual std::vector<std::string> Warnings() const = 0;
184+
185+ /** Get the maximum key expression index. Used only for tests */
186+ virtual uint32_t GetMaxKeyExpr() const = 0;
187+
188+ /** Get the number of key expressions in this descriptor. Used only for tests */
189+ virtual size_t GetKeyCount() const = 0;
In ce4c66eb7c5e99e3df1c20d5c0ae8278a714b9f8 “test: Test that key expression indexes match key count”
Not a fan of these caveats in the comments that these functions are used only in tests. It would be nice if the code highlighted these itself. Consider the following working minimal diff:
0diff --git a/src/script/descriptor.h b/src/script/descriptor.h
1index 3ac748a174..13a850fe76 100644
2--- a/src/script/descriptor.h
3+++ b/src/script/descriptor.h
4@@ -78,6 +78,14 @@ public:
5 DescriptorCache MergeAndDiff(const DescriptorCache& other);
6 };
7
8+struct DescriptorTest {
9+ /** Get the maximum key expression index. */
10+ virtual uint32_t GetMaxKeyExpr() const = 0;
11+
12+ /** Get the number of key expressions in this descriptor. */
13+ virtual size_t GetKeyCount() const = 0;
14+};
15+
16 /** \brief Interface for parsed descriptor objects.
17 *
18 * Descriptors are strings that describe a set of scriptPubKeys, together with
19@@ -95,7 +103,7 @@ public:
20 * Reference documentation about the descriptor language can be found in
21 * doc/descriptors.md.
22 */
23-struct Descriptor {
24+struct Descriptor: public DescriptorTest {
25 virtual ~Descriptor() = default;
26
27 /** Whether the expansion of this descriptor depends on the position. */
28@@ -181,12 +189,6 @@ struct Descriptor {
29
30 /** Semantic/safety warnings (includes subdescriptors). */
31 virtual std::vector<std::string> Warnings() const = 0;
32-
33- /** Get the maximum key expression index. Used only for tests */
34- virtual uint32_t GetMaxKeyExpr() const = 0;
35-
36- /** Get the number of key expressions in this descriptor. Used only for tests */
37- virtual size_t GetKeyCount() const = 0;
38 };
Ah I misread the suggested change.
I don’t think it makes sense to have a Test class outside of tests that the actual class callers should use is a subclass of. Rather it should be the other way around (which is what I thought you originally proposed), but that would be a larger change.
I don’t think it makes sense to have a Test class outside of tests that the actual class callers should use is a subclass of. Rather it should be the other way around (which is what I thought you originally proposed),
Yeah, I thought of that earlier but later suggested this approach because it came about to be much shorter than the other one - makes sense to me if we don’t prefer Test structures to be present outside of the tests.
332@@ -328,6 +333,11 @@ def run_test(self):
333 self.test_success_case("tr(H,{pk(musig/*), pk(same keys different musig/*)})", "tr($H,{pk(musig($0,$1,$2)/<0;1>/*),pk(musig($1,$2)/0/*)})", scriptpath=True)
334 self.test_success_case("tr(musig/*,{pk(partial keys diff musig-1/*),pk(partial keys diff musig-2/*)})}", "tr(musig($0,$1,$2)/<3;4>/*,{pk(musig($0,$1)/<5;6>/*),pk(musig($1,$2)/7/*)})")
335 self.test_success_case("tr(musig/*,{pk(partial keys diff musig-1/*),pk(partial keys diff musig-2/*)})} script-path", "tr(musig($0,$1,$2)/<3;4>/*,{pk(musig($0,$1)/<5;6>/*),pk(musig($1,$2)/7/*)})", scriptpath=True, nosign_wallets=[0])
336+ self.test_success_case("tr(H,and(pk(musig/*),after(1)))", "tr($H,and_v(v:pk(musig($0,$1,$2)/<0;1>/*),after(1)))", scriptpath=True)
337+ self.test_success_case("tr(H,and(pk_k(musig/*),after(1)))", "tr($H,and_v(vc:pk_k(musig($0,$1,$2)/<0;1>/*),after(1)))", scriptpath=True)
338+ self.test_success_case("tr(H,and(pkh(musig/*),after(1)))", "tr($H,and_v(v:pkh(musig($0,$1,$2)/<0;1>/*),after(1)))", scriptpath=True)
339+ self.test_success_case("tr(H,and(pk_h(musig/*),after(1)))", "tr($H,and_v(vc:pk_h(musig($0,$1,$2)/<0;1>/*),after(1)))", scriptpath=True)
340+ self.test_success_case("tr(H,{and(pk(musig/*),after(1)),and(pk(musig/*),after(1))})", "tr($H,{and_v(v:pk(musig($0,$2)/0/*),after(1)),and_v(v:pk(musig($1,$2)/0/*),after(1))})", scriptpath=True)
341
In 4b53cbd69220c1c786bb23a72c0b26a6f78a38f7 “test: Test for musig() in various miniscript expressions”
It would be helpful to add the test cases where the musig expressions are in the key path along with the script path, while spending both the paths. Consider the following working tests, the first of which is very much related to the one shared in the issue #34076.
0diff --git a/test/functional/wallet_musig.py b/test/functional/wallet_musig.py
1index 0e3e4377c0..e8c89bd7cb 100755
2--- a/test/functional/wallet_musig.py
3+++ b/test/functional/wallet_musig.py
4@@ -338,6 +338,8 @@ class WalletMuSigTest(BitcoinTestFramework):
5 self.test_success_case("tr(H,and(pkh(musig/*),after(1)))", "tr($H,and_v(v:pkh(musig($0,$1,$2)/<0;1>/*),after(1)))", scriptpath=True)
6 self.test_success_case("tr(H,and(pk_h(musig/*),after(1)))", "tr($H,and_v(vc:pk_h(musig($0,$1,$2)/<0;1>/*),after(1)))", scriptpath=True)
7 self.test_success_case("tr(H,{and(pk(musig/*),after(1)),and(pk(musig/*),after(1))})", "tr($H,{and_v(v:pk(musig($0,$2)/0/*),after(1)),and_v(v:pk(musig($1,$2)/0/*),after(1))})", scriptpath=True)
8+ self.test_success_case("tr(musig/*,{and(pk(musig/*),after(3)),and(pk(musig/*),after(2)),and(pk(musig/*),after(5))})", "tr(musig($0,$1,$2)/<0;1>/*,{and_v(v:pk(musig($0,$1)/<0;1>/*),after(3)),{and_v(v:pk(musig($0,$2)/<0;1>/*),after(2)),and_v(v:pk(musig($1,$2)/<0;1>/*),after(5))}})", scriptpath=True, nosign_wallets=[0])
9+ self.test_success_case("tr(musig/*,{and(pk(musig/*),older(3)),and(pk(musig/*),older(2)),and(pk(musig/*),older(5))})", "tr(musig($0,$1,$2)/<0;1>/*,{and_v(v:pk(musig($0,$1)/<0;1>/*),older(3)),{and_v(v:pk(musig($0,$2)/<0;1>/*),older(2)),and_v(v:pk(musig($1,$2)/<0;1>/*),older(5))}})")
10
11 self.test_failure_case_1("missing participant nonce", "tr(musig($0/<0;1>/*,$1/<1;2>/*,$2/<2;3>/*))")
12 self.test_failure_case_2("insufficient partial signatures", "rawtr(musig($0/<0;1>/*,$1/<1;2>/*,$2/<2;3>/*))")
For actually testing the script path with older, block(s) would need to be generated but I have not suggested it at the moment.
1825-/** Parse a hex string ending at the end of the fragment's text representation. */
1826+/** Parse a hex string fully contained within a fragment with the name given by 'func' */
1827 template<typename Ctx>
1828-std::optional<std::pair<std::vector<unsigned char>, int>> ParseHexStrEnd(std::span<const char> in, const size_t expected_size,
1829+std::optional<std::vector<unsigned char>> ParseHexStr(const std::string& func, std::span<const char>& in, const size_t expected_size,
1830 const Ctx& ctx)
In ec0f47b15cb3269015523e6fab8ae9241f4181a1 “miniscript: Using Func and Expr when parsing keys, hashes, and locktimes”
const Ctx& ctx)
ctx seems to be unused.
0diff --git a/src/script/miniscript.cpp b/src/script/miniscript.cpp
1index 6dc154a8a2..384434df59 100644
2--- a/src/script/miniscript.cpp
3+++ b/src/script/miniscript.cpp
4@@ -418,6 +418,26 @@ std::optional<int64_t> ParseScriptNumber(const Opcode& in) {
5 return {};
6 }
7
8+std::optional<std::vector<unsigned char>> ParseHexStr(const std::string& func, std::span<const char>& in, const size_t expected_size)
9+{
10+ std::span<const char> expr = script::Expr(in);
11+ if (!script::Func(func, expr)) return {};
12+ std::string val = std::string(expr.begin(), expr.end());
13+ if (!IsHex(val)) return {};
14+ auto hash = ParseHex(val);
15+ if (hash.size() != expected_size) return {};
16+ return hash;
17+}
18+
19 int FindNextChar(std::span<const char> sp, const char m)
20 {
21 for (int i = 0; i < (int)sp.size(); ++i) {
22diff --git a/src/script/miniscript.h b/src/script/miniscript.h
23index 8733d34713..cbabece43a 100644
24--- a/src/script/miniscript.h
25+++ b/src/script/miniscript.h
26@@ -1816,18 +1816,10 @@ std::optional<Key> ParseKey(const std::string& func, std::span<const char>& in,
27 }
28
29 /** Parse a hex string fully contained within a fragment with the name given by 'func' */
30-template<typename Ctx>
31-std::optional<std::vector<unsigned char>> ParseHexStr(const std::string& func, std::span<const char>& in, const size_t expected_size,
32- const Ctx& ctx)
33-{
34- std::span<const char> expr = script::Expr(in);
35- if (!script::Func(func, expr)) return {};
36- std::string val = std::string(expr.begin(), expr.end());
37- if (!IsHex(val)) return {};
38- auto hash = ParseHex(val);
39- if (hash.size() != expected_size) return {};
40- return hash;
41-}
42+std::optional<std::vector<unsigned char>> ParseHexStr(const std::string& func, std::span<const char>& in, const size_t expected_size);
43+
44
45 /** BuildBack pops the last two elements off `constructed` and wraps them in the specified Fragment */
46 template<typename Key>
47@@ -1998,37 +1990,33 @@ inline std::optional<Node<Key>> Parse(std::span<const char> in, const Ctx& ctx)
48 constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_H, Vector(std::move(*key)));
49 script_size += 23;
50 } else if (Const("sha256(", in, /*skip=*/false)) {
51- std::optional<std::vector<unsigned char>> hash = ParseHexStr("sha256", in, 32, ctx);
52+ std::optional<std::vector<unsigned char>> hash = ParseHexStr("sha256", in, 32);
53 if (!hash) return {};
54 constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::SHA256, std::move(*hash));
55 script_size += 38;
56 } else if (Const("ripemd160(", in, /*skip=*/false)) {
57- std::optional<std::vector<unsigned char>> hash = ParseHexStr("ripemd160", in, 20, ctx);
58+ std::optional<std::vector<unsigned char>> hash = ParseHexStr("ripemd160", in, 20);
59 if (!hash) return {};
60 constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::RIPEMD160, std::move(*hash));
61 script_size += 26;
62 } else if (Const("hash256(", in, /*skip=*/false)) {
63- std::optional<std::vector<unsigned char>> hash = ParseHexStr("hash256", in, 32, ctx);
64+ std::optional<std::vector<unsigned char>> hash = ParseHexStr("hash256", in, 32);
65 if (!hash) return {};
66 constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::HASH256, std::move(*hash));
67 script_size += 38;
68 } else if (Const("hash160(", in, /*skip=*/false)) {
69- std::optional<std::vector<unsigned char>> hash = ParseHexStr("hash160", in, 20, ctx);
70+ std::optional<std::vector<unsigned char>> hash = ParseHexStr("hash160", in, 20);
71 if (!hash) return {};
72 constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::HASH160, std::move(*hash));
2087+ const auto num{ToIntegral<int64_t>(std::string_view(expr.begin(), expr.end()))};
2088 if (!num.has_value() || *num < 1 || *num >= 0x80000000L) return {};
2089 constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::OLDER, *num);
2090- in = in.subspan(arg_size + 1);
2091 script_size += 1 + (*num > 16) + (*num > 0x7f) + (*num > 0x7fff) + (*num > 0x7fffff);
2092 } else if (Const("multi(", in)) {
In ec0f47b15cb3269015523e6fab8ae9241f4181a1 “miniscript: Using Func and Expr when parsing keys, hashes, and locktimes”
Along similar lines as the above ones.
0diff --git a/src/script/miniscript.cpp b/src/script/miniscript.cpp
1index 6dc154a8a2..384434df59 100644
2--- a/src/script/miniscript.cpp
3+++ b/src/script/miniscript.cpp
4@@ -418,6 +418,26 @@ std::optional<int64_t> ParseScriptNumber(const Opcode& in) {
5 return {};
6 }
7
8+std::optional<int64_t> ParseNum(const std::string& func, std::span<const char>& in)
9+{
10+ std::span<const char> expr = script::Expr(in);
11+ if (!script::Func(func, expr)) return {};
12+ const auto num{ToIntegral<int64_t>(std::string_view(expr.begin(), expr.end()))};
13+ if (!num.has_value() || *num < 1 || *num >= 0x80000000L) return {};
14+ return num;
15+}
16+
17 int FindNextChar(std::span<const char> sp, const char m)
18 {
19 for (int i = 0; i < (int)sp.size(); ++i) {
20diff --git a/src/script/miniscript.h b/src/script/miniscript.h
21index 8733d34713..cbabece43a 100644
22--- a/src/script/miniscript.h
23+++ b/src/script/miniscript.h
24@@ -1816,18 +1816,10 @@ std::optional<Key> ParseKey(const std::string& func, std::span<const char>& in,
25 }
26
27+/** Parse a number fully contained within a fragment with the name given by 'func' */
28+std::optional<int64_t> ParseNum(const std::string& func, std::span<const char>& in);
29
30 /** BuildBack pops the last two elements off `constructed` and wraps them in the specified Fragment */
31 template<typename Key>
32@@ -1998,37 +1990,33 @@ inline std::optional<Node<Key>> Parse(std::span<const char> in, const Ctx& ctx)
33 constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_H,
34 } else if (Const("after(", in, /*skip=*/false)) {
35- auto expr = Expr(in);
36- if (!Func("after", expr)) return {};
37- const auto num{ToIntegral<int64_t>(std::string_view(expr.begin(), expr.end()))};
38- if (!num.has_value() || *num < 1 || *num >= 0x80000000L) return {};
39+ auto num = ParseNum("after", in);
40+ if (!num) return {};
41 constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::AFTER, *num);
42 script_size += 1 + (*num > 16) + (*num > 0x7f) + (*num > 0x7fff) + (*num > 0x7fffff);
43 } else if (Const("older(", in, /*skip=*/false)) {
44- auto expr = Expr(in);
45- if (!Func("older", expr)) return {};
46- const auto num{ToIntegral<int64_t>(std::string_view(expr.begin(), expr.end()))};
47- if (!num.has_value() || *num < 1 || *num >= 0x80000000L) return {};
48+ auto num = ParseNum("older", in);
49+ if (!num) return {};
50 constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::OLDER, *num);
51 script_size += 1 + (*num > 16) + (*num > 0x7f) + (*num > 0x7fff) + (*num > 0x7fffff);
52 } else if (Const("multi(", in)) {
ACK 4b53cbd69220c1c786bb23a72c0b26a6f78a38f7
+1 on adding the first commit that adds the unit test helpers and also helps in ensuring the fix is correct later.
I do like pushing down the key_exp_index increments within the ParsePubkeyInner function instead of the caller ParseScript - looks cleaner and more logical.