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 |
|---|---|
| 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:
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.
🚧 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.
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.