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

pull achow101 wants to merge 4 commits into bitcoin:master from achow101:musig-miniscript changing 4 files +74 −88
  1. achow101 commented at 9:36 pm on December 22, 2025: member

    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

  2. DrahtBot added the label Descriptors on Dec 22, 2025
  3. DrahtBot commented at 9:37 pm on December 22, 2025: contributor

    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
    ACK scgbckbone

    If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> 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.

    LLM Linter (✨ experimental)

    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):

    • Const(“pk(”, in, /skip=/false) in src/script/miniscript.h
    • Const(“pkh(”, in, /skip=/false) in src/script/miniscript.h
    • Const(“pk_k(”, in, /skip=/false) in src/script/miniscript.h
    • Const(“pk_h(”, in, /skip=/false) in src/script/miniscript.h
    • Const(“sha256(”, in, /skip=/false) in src/script/miniscript.h
    • Const(“ripemd160(”, in, /skip=/false) in src/script/miniscript.h
    • Const(“hash256(”, in, /skip=/false) in src/script/miniscript.h
    • Const(“hash160(”, in, /skip=/false) in src/script/miniscript.h
    • Const(“after(”, in, /skip=/false) in src/script/miniscript.h
    • Const(“older(”, in, /skip=/false) in src/script/miniscript.h

    2025-12-23

  4. achow101 force-pushed on Dec 22, 2025
  5. DrahtBot added the label CI failed on Dec 22, 2025
  6. DrahtBot commented at 10:11 pm on December 22, 2025: contributor

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

  7. DrahtBot added the label Needs rebase on Dec 23, 2025
  8. achow101 force-pushed on Dec 23, 2025
  9. DrahtBot removed the label Needs rebase on Dec 23, 2025
  10. scgbckbone commented at 2:57 pm on December 23, 2025: contributor

    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:

    1. bcrt1p4e7gsehp8xg87gclgf0fct4ng6sgg0a9ctg4laj5u3wuylw0xcfsyh86xw
    2. bcrt1pp4294qkmk43rsmzsan3ckecpv9fktckfkgew2knvnpfh65zl0dqqdaytlp

    Core calculated addresses with getnewaddress:

    1. bcrt1pwvnfa5ypmfvnmcguwllehxqreq9ljrvz58hgeqxpwy9xqfta7r6scptqp0
    2. bcrt1pgkt8zf54dgkpf66zt56etr8ksctwczn453ec4m2gqydknstuk7mqg0rrlu

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

  11. miniscript: Use a reference to key_exp_index in KeyParser
    For key_exp_index to count correctly for miniscript expressions,
    KeyParser should hold a reference to it.
    fdd46088a4
  12. descriptors: Increment key_exp_index in ParsePubkey(Inner)
    Simplifies the callsites for incrementing key_exp_index
    5f2bb4056c
  13. miniscript: Using Func and Expr when parsing keys, hashes, and locktimes
    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.
    934de09d89
  14. test: Test for musig() in various miniscript expressions 2500adcbe7
  15. achow101 force-pushed on Dec 23, 2025
  16. achow101 commented at 9:54 pm on December 23, 2025: member
    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.
  17. maflcko commented at 8:50 am on December 24, 2025: member

    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
    

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-12-27 18:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me