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
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34141.
<!--021abf342d371248e50ceaed478a90ca-->
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 <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
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):
<sup>2026-02-02 23:25:49</sup>
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task TSan: https://github.com/bitcoin/bitcoin/actions/runs/20444643161/job/58745329801</sub>
<sub>LLM reason (✨ experimental): descriptor_tests failed with Subprocess aborted (CTest exit code 8).</sub>
<details><summary>Hints</summary>
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.
</details>
Import now works OK, but there is a problem with address generation. Test case:
self.test_success_case("time-locked musig",
"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))}})",
scriptpath=True)
Particular case:
core_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'}]
ms = bitcoind.create_wallet(
wallet_name=name, disable_private_keys=True,
blank=True, passphrase=None, avoid_reuse=False, descriptors=True
)
res = ms.importdescriptors(core_desc_object)
# res = [{'success': True}]
addr = ms.getnewaddress("", "bech32m")
# just random other wallet with funds
x = bitcoind.supply_wallet.sendtoaddress(addr, 49)
# x = a166d63b05162525752a1c911454eb7e3e27a3f509b2efcd5d74069d98de4382
# mine
y =bitcoind.supply_wallet.generatetoaddress(1,bitcoind.supply_wallet.getnewaddress())
# y = ['1826120d4e15d6c880ec5633ca74912bbae3a28c749a502de15379ce39af61a2']
# must have unspent now - but we don't
unspent = ms.listunspent()
# unspent = []
# first external address is not even recognized as ours
ms.getaddressinfo(addr)
# {'address': 'bcrt1pwvnfa5ypmfvnmcguwllehxqreq9ljrvz58hgeqxpwy9xqfta7r6scptqp0',
# 'scriptPubKey': '512073269ed081da593de11c77ff9b9803c80bf90d82a1ee8c80c1710a60257df0f5',
# 'ismine': False, 'solvable': False, 'iswatchonly': False, 'isscript': True, 'iswitness': True,
# 'witness_version': 1,
# 'witness_program': '73269ed081da593de11c77ff9b9803c80bf90d82a1ee8c80c1710a60257df0f5',
# 'ischange': False, 'labels': ['']}
# generating next address (index 1) works, and is recognized as ours
next_addr = ms.getnewaddress("", "bech32m")
bitcoind.supply_wallet.sendtoaddress(next_addr, 10)
bitcoind.supply_wallet.generatetoaddress(1, bitcoind.supply_wallet.getnewaddress())
ms.listunspent()
# [{'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.
Looks like that was caused by the key expression index being counted incorrectly for musig inside of miniscript, and therefore looking up the wrong key in the cache. Should be fixed now.
The CI fails due to UB:
/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?
Done
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).
Updated the comment
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.
Updated the comment
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 tried to @brunoerg-style revert this line, and none of the tests fail (I tried unit and functional tests). Is there a way to get some assurance that the updating is done right?
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?
I added a test case to check that the expression index matches the key count. However, this does require 2 test only functions to be added to Descriptor, and I'm not entirely sure that I like this approach. But it did reveal a bug in the counting for OriginPubkeyProvider.
The best way to judge a test approach is by how many bugs it catches. Seems like this one did its job.
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());
Mental note: we should really rewrite all of the span parsing logic to use 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
I'd prefer non-recursive versions here, but given that they're test-only, we can ignore that until it becomes a problem I guess?
Do you mean having a 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)
uint32_t GetMaxKeyExpr() const final
{
uint32_t max_key_expr{0};
std::vector<const DescriptorImpl*> todo = {this};
while (!todo.empty()) {
const DescriptorImpl* desc = todo.back();
todo.pop_back();
for (const auto& p : desc->m_pubkey_args) {
max_key_expr = std::max(max_key_expr, p->m_expr_index);
}
for (const auto& s : desc->m_subdescriptor_args) todo.push_back(s.get());
}
return max_key_expr;
}
size_t GetKeyCount() const final
{
size_t count{0};
std::vector<const DescriptorImpl*> todo = {this};
while (!todo.empty()) {
const DescriptorImpl* desc = todo.back();
todo.pop_back();
for (const auto& p : desc->m_pubkey_args) count += p->GetKeyCount();
for (const auto& s : desc->m_subdescriptor_args) todo.push_back(s.get());
}
return count;
}
(note the final; this doesn't support individually overridden versions)
Pulled this suggestion and the fuzz test.
crACK 2d44d4176b1f1d790e71aa496c18f3c8229b2a89. I added this, and spent some time fuzzing the descriptor_parse and mocked_descriptor_parse fuzz tests:
diff --git a/src/test/fuzz/descriptor_parse.cpp b/src/test/fuzz/descriptor_parse.cpp
index eeba5f1369e..6b3084e23d1 100644
--- a/src/test/fuzz/descriptor_parse.cpp
+++ b/src/test/fuzz/descriptor_parse.cpp
@@ -61,6 +61,10 @@ static void TestDescriptor(const Descriptor& desc, FlatSigningProvider& sig_prov
const bool is_nontop_or_nonsolvable{!*is_solvable || !desc.GetOutputType()};
const bool is_input_size_info_set{max_sat_maxsig && max_sat_nonmaxsig && max_elems};
assert(is_input_size_info_set || is_nontop_or_nonsolvable);
+
+ auto max_key_expr = desc.GetMaxKeyExpr();
+ auto key_count = desc.GetKeyCount();
+ assert((max_key_expr == 0 && key_count == 0) || max_key_expr + 1 == key_count);
}
void initialize_descriptor_parse()
Partially reverting the diff here doesn't catch any errors:
diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index 298a9501e61..9a5600ae444 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -2285,6 +2285,7 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
error = strprintf("pkh(): %s", error);
return {};
}
+ ++key_exp_index;
for (auto& pubkey : pubkeys) {
ret.emplace_back(std::make_unique<PKHDescriptor>(std::move(pubkey)));
}
@@ -2296,6 +2297,7 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
error = strprintf("combo(): %s", error);
return {};
}
+ ++key_exp_index;
for (auto& pubkey : pubkeys) {
ret.emplace_back(std::make_unique<ComboDescriptor>(std::move(pubkey)));
}
@@ -2404,6 +2406,7 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
error = strprintf("wpkh(): %s", error);
return {};
}
+ ++key_exp_index;
for (auto& pubkey : pubkeys) {
ret.emplace_back(std::make_unique<WPKHDescriptor>(std::move(pubkey)));
}
But, adding
diff --git a/src/test/fuzz/descriptor_parse.cpp b/src/test/fuzz/descriptor_parse.cpp
index eeba5f1369e..6b3084e23d1 100644
--- a/src/test/fuzz/descriptor_parse.cpp
+++ b/src/test/fuzz/descriptor_parse.cpp
@@ -61,6 +61,10 @@ static void TestDescriptor(const Descriptor& desc, FlatSigningProvider& sig_prov
const bool is_nontop_or_nonsolvable{!*is_solvable || !desc.GetOutputType()};
const bool is_input_size_info_set{max_sat_maxsig && max_sat_nonmaxsig && max_elems};
assert(is_input_size_info_set || is_nontop_or_nonsolvable);
+
+ auto max_key_expr = desc.GetMaxKeyExpr();
+ auto key_count = desc.GetKeyCount();
+ assert((max_key_expr == 0 && key_count == 0) || max_key_expr + 1 == key_count);
}
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.
ACK 4b53cbd69220c1c786bb23a72c0b26a6f78a38f7
Concept ACK 4b53cbd because it allows 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:
diff --git a/src/script/descriptor.h b/src/script/descriptor.h
index 3ac748a174..13a850fe76 100644
--- a/src/script/descriptor.h
+++ b/src/script/descriptor.h
@@ -78,6 +78,14 @@ public:
DescriptorCache MergeAndDiff(const DescriptorCache& other);
};
+struct DescriptorTest {
+ /** Get the maximum key expression index. */
+ virtual uint32_t GetMaxKeyExpr() const = 0;
+
+ /** Get the number of key expressions in this descriptor. */
+ virtual size_t GetKeyCount() const = 0;
+};
+
/** \brief Interface for parsed descriptor objects.
*
* Descriptors are strings that describe a set of scriptPubKeys, together with
@@ -95,7 +103,7 @@ public:
* Reference documentation about the descriptor language can be found in
* doc/descriptors.md.
*/
-struct Descriptor {
+struct Descriptor: public DescriptorTest {
virtual ~Descriptor() = default;
/** Whether the expansion of this descriptor depends on the position. */
@@ -181,12 +189,6 @@ struct Descriptor {
/** Semantic/safety warnings (includes subdescriptors). */
virtual std::vector<std::string> Warnings() const = 0;
-
- /** Get the maximum key expression index. Used only for tests */
- virtual uint32_t GetMaxKeyExpr() const = 0;
-
- /** Get the number of key expressions in this descriptor. Used only for tests */
- virtual size_t GetKeyCount() const = 0;
};
I think making that work will end up being a significant refactor of the tests, so I will leave that for a separate PR.
Do you anticipate more changes here? The above diff builds and tests fine.
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.
diff --git a/test/functional/wallet_musig.py b/test/functional/wallet_musig.py
index 0e3e4377c0..e8c89bd7cb 100755
--- a/test/functional/wallet_musig.py
+++ b/test/functional/wallet_musig.py
@@ -338,6 +338,8 @@ class WalletMuSigTest(BitcoinTestFramework):
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)
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)
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)
+ 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])
+ 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))}})")
self.test_failure_case_1("missing participant nonce", "tr(musig($0/<0;1>/*,$1/<1;2>/*,$2/<2;3>/*))")
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.
I don't think these are critical to add at this point, will do if I need to retouch.
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.
diff --git a/src/script/miniscript.cpp b/src/script/miniscript.cpp
index 6dc154a8a2..384434df59 100644
--- a/src/script/miniscript.cpp
+++ b/src/script/miniscript.cpp
@@ -418,6 +418,26 @@ std::optional<int64_t> ParseScriptNumber(const Opcode& in) {
return {};
}
+std::optional<std::vector<unsigned char>> ParseHexStr(const std::string& func, std::span<const char>& in, const size_t expected_size)
+{
+ std::span<const char> expr = script::Expr(in);
+ if (!script::Func(func, expr)) return {};
+ std::string val = std::string(expr.begin(), expr.end());
+ if (!IsHex(val)) return {};
+ auto hash = ParseHex(val);
+ if (hash.size() != expected_size) return {};
+ return hash;
+}
+
int FindNextChar(std::span<const char> sp, const char m)
{
for (int i = 0; i < (int)sp.size(); ++i) {
diff --git a/src/script/miniscript.h b/src/script/miniscript.h
index 8733d34713..cbabece43a 100644
--- a/src/script/miniscript.h
+++ b/src/script/miniscript.h
@@ -1816,18 +1816,10 @@ std::optional<Key> ParseKey(const std::string& func, std::span<const char>& in,
}
/** Parse a hex string fully contained within a fragment with the name given by 'func' */
-template<typename Ctx>
-std::optional<std::vector<unsigned char>> ParseHexStr(const std::string& func, std::span<const char>& in, const size_t expected_size,
- const Ctx& ctx)
-{
- std::span<const char> expr = script::Expr(in);
- if (!script::Func(func, expr)) return {};
- std::string val = std::string(expr.begin(), expr.end());
- if (!IsHex(val)) return {};
- auto hash = ParseHex(val);
- if (hash.size() != expected_size) return {};
- return hash;
-}
+std::optional<std::vector<unsigned char>> ParseHexStr(const std::string& func, std::span<const char>& in, const size_t expected_size);
+
/** BuildBack pops the last two elements off `constructed` and wraps them in the specified Fragment */
template<typename Key>
@@ -1998,37 +1990,33 @@ inline std::optional<Node<Key>> Parse(std::span<const char> in, const Ctx& ctx)
constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_H, Vector(std::move(*key)));
script_size += 23;
} else if (Const("sha256(", in, /*skip=*/false)) {
- std::optional<std::vector<unsigned char>> hash = ParseHexStr("sha256", in, 32, ctx);
+ std::optional<std::vector<unsigned char>> hash = ParseHexStr("sha256", in, 32);
if (!hash) return {};
constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::SHA256, std::move(*hash));
script_size += 38;
} else if (Const("ripemd160(", in, /*skip=*/false)) {
- std::optional<std::vector<unsigned char>> hash = ParseHexStr("ripemd160", in, 20, ctx);
+ std::optional<std::vector<unsigned char>> hash = ParseHexStr("ripemd160", in, 20);
if (!hash) return {};
constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::RIPEMD160, std::move(*hash));
script_size += 26;
} else if (Const("hash256(", in, /*skip=*/false)) {
- std::optional<std::vector<unsigned char>> hash = ParseHexStr("hash256", in, 32, ctx);
+ std::optional<std::vector<unsigned char>> hash = ParseHexStr("hash256", in, 32);
if (!hash) return {};
constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::HASH256, std::move(*hash));
script_size += 38;
} else if (Const("hash160(", in, /*skip=*/false)) {
- std::optional<std::vector<unsigned char>> hash = ParseHexStr("hash160", in, 20, ctx);
+ std::optional<std::vector<unsigned char>> hash = ParseHexStr("hash160", in, 20);
if (!hash) return {};
constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::HASH160, std::move(*hash));
If I need to retouch.
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.
diff --git a/src/script/miniscript.cpp b/src/script/miniscript.cpp
index 6dc154a8a2..384434df59 100644
--- a/src/script/miniscript.cpp
+++ b/src/script/miniscript.cpp
@@ -418,6 +418,26 @@ std::optional<int64_t> ParseScriptNumber(const Opcode& in) {
return {};
}
+std::optional<int64_t> ParseNum(const std::string& func, std::span<const char>& in)
+{
+ std::span<const char> expr = script::Expr(in);
+ if (!script::Func(func, expr)) return {};
+ const auto num{ToIntegral<int64_t>(std::string_view(expr.begin(), expr.end()))};
+ if (!num.has_value() || *num < 1 || *num >= 0x80000000L) return {};
+ return num;
+}
+
int FindNextChar(std::span<const char> sp, const char m)
{
for (int i = 0; i < (int)sp.size(); ++i) {
diff --git a/src/script/miniscript.h b/src/script/miniscript.h
index 8733d34713..cbabece43a 100644
--- a/src/script/miniscript.h
+++ b/src/script/miniscript.h
@@ -1816,18 +1816,10 @@ std::optional<Key> ParseKey(const std::string& func, std::span<const char>& in,
}
+/** Parse a number fully contained within a fragment with the name given by 'func' */
+std::optional<int64_t> ParseNum(const std::string& func, std::span<const char>& in);
/** BuildBack pops the last two elements off `constructed` and wraps them in the specified Fragment */
template<typename Key>
@@ -1998,37 +1990,33 @@ inline std::optional<Node<Key>> Parse(std::span<const char> in, const Ctx& ctx)
constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_H,
} else if (Const("after(", in, /*skip=*/false)) {
- auto expr = Expr(in);
- if (!Func("after", expr)) return {};
- const auto num{ToIntegral<int64_t>(std::string_view(expr.begin(), expr.end()))};
- if (!num.has_value() || *num < 1 || *num >= 0x80000000L) return {};
+ auto num = ParseNum("after", in);
+ if (!num) return {};
constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::AFTER, *num);
script_size += 1 + (*num > 16) + (*num > 0x7f) + (*num > 0x7fff) + (*num > 0x7fffff);
} else if (Const("older(", in, /*skip=*/false)) {
- auto expr = Expr(in);
- if (!Func("older", expr)) return {};
- const auto num{ToIntegral<int64_t>(std::string_view(expr.begin(), expr.end()))};
- if (!num.has_value() || *num < 1 || *num >= 0x80000000L) return {};
+ auto num = ParseNum("older", in);
+ if (!num) return {};
constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::OLDER, *num);
script_size += 1 + (*num > 16) + (*num > 0x7f) + (*num > 0x7fff) + (*num > 0x7fffff);
} else if (Const("multi(", in)) {
If I need to retouch
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.
Milestone
31.0