miniscript: Use Func and Expr when parsing keys, hashes, and locktimes #34141

achow101 · 2025-12-22T21:36:58Z

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

DrahtBot · 2025-12-22T21:37:04Z

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/34141.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	scgbckbone

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31713 (miniscript refactor: Remove unique_ptr-indirection 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.

DrahtBot · 2025-12-22T22:11:01Z

🚧 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).}

Hints

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.

scgbckbone

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:

bcrt1p4e7gsehp8xg87gclgf0fct4ng6sgg0a9ctg4laj5u3wuylw0xcfsyh86xw
bcrt1pp4294qkmk43rsmzsan3ckecpv9fktckfkgew2knvnpfh65zl0dqqdaytlp

Core calculated addresses with getnewaddress:

bcrt1pwvnfa5ypmfvnmcguwllehxqreq9ljrvz58hgeqxpwy9xqfta7r6scptqp0
bcrt1pgkt8zf54dgkpf66zt56etr8ksctwczn453ec4m2gqydknstuk7mqg0rrlu

Funnily enough, when I use external descriptor from listdescriptors and run deriveaddresses it matches what my app calculates, not what getnewaddress provides.

achow101 · 2025-12-23T21:54:17Z

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.

scgbckbone · 2025-12-23T23:12:39Z

tACK 2500adc

maflcko · 2025-12-24T08:50:34Z

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

src/script/descriptor.cpp

src/script/miniscript.h

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.

sipa · 2026-01-13T21:23:51Z

src/script/descriptor.cpp

            error = strprintf("combo(): %s", error);
            return {};
        }
-        ++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?

DrahtBot added the Descriptors label Dec 22, 2025

achow101 force-pushed the musig-miniscript branch from 35095cc to 346be2b Compare December 22, 2025 22:10

DrahtBot added the CI failed label Dec 22, 2025

DrahtBot added the Needs rebase label Dec 23, 2025

achow101 force-pushed the musig-miniscript branch from 346be2b to d541f69 Compare December 23, 2025 02:14

DrahtBot removed the Needs rebase label Dec 23, 2025

scgbckbone reviewed Dec 23, 2025

View reviewed changes

DrahtBot mentioned this pull request Dec 23, 2025

miniscript refactor: Remove unique_ptr-indirection #31713

Open

achow101 force-pushed the musig-miniscript branch from d541f69 to 2500adc Compare December 23, 2025 21:53

achow101 mentioned this pull request Dec 23, 2025

MuSig2 Tracking Issue #31246

Open

14 tasks

achow101 force-pushed the musig-miniscript branch 2 times, most recently from 896e522 to 31c7414 Compare January 3, 2026 01:24

DrahtBot removed the CI failed label Jan 3, 2026

achow101 added this to the 31.0 milestone Jan 5, 2026

sipa reviewed Jan 13, 2026

View reviewed changes

src/script/descriptor.cpp Outdated Show resolved Hide resolved

src/script/miniscript.h Outdated Show resolved Hide resolved

src/script/miniscript.h Outdated Show resolved Hide resolved

scgbckbone mentioned this pull request Jan 13, 2026

Importing musig descriptor with identical (private) keys in tapleaf and only musig subderivation different fails #34273

Open

1 task

achow101 added 4 commits January 13, 2026 12:52

miniscript: Use a reference to key_exp_index in KeyParser

b20043a

For key_exp_index to count correctly for miniscript expressions, KeyParser should hold a reference to it.

descriptors: Increment key_exp_index in ParsePubkey(Inner)

3e37970

Simplifies the callsites for incrementing key_exp_index

test: Test for musig() in various miniscript expressions

f846e64

achow101 force-pushed the musig-miniscript branch from 31c7414 to f846e64 Compare January 13, 2026 20:59

sipa reviewed Jan 13, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

miniscript: Use Func and Expr when parsing keys, hashes, and locktimes #34141

miniscript: Use Func and Expr when parsing keys, hashes, and locktimes #34141

achow101 commented Dec 22, 2025

Uh oh!

DrahtBot commented Dec 22, 2025 •

edited

Loading

Uh oh!

DrahtBot commented Dec 22, 2025

Uh oh!

scgbckbone left a comment

Uh oh!

achow101 commented Dec 23, 2025

Uh oh!

scgbckbone commented Dec 23, 2025

Uh oh!

maflcko commented Dec 24, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sipa Jan 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

miniscript: Use Func and Expr when parsing keys, hashes, and locktimes #34141

Are you sure you want to change the base?

miniscript: Use Func and Expr when parsing keys, hashes, and locktimes #34141

Conversation

achow101 commented Dec 22, 2025

Uh oh!

DrahtBot commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented Dec 22, 2025

Uh oh!

scgbckbone left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Dec 23, 2025

Uh oh!

scgbckbone commented Dec 23, 2025

Uh oh!

maflcko commented Dec 24, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sipa Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DrahtBot commented Dec 22, 2025 •

edited

Loading