descriptor: fix musig() duplicate key checks and doubled PSBT origin paths #34697

shuv-amp commented at 11:17 PM on February 27, 2026: none

Importing a descriptor that uses the same musig() participants twice in one tapleaf, with different musig subderivations, fails with is not sane: contains duplicate public keys. It only fails when one of the participants is a private key on a hardened path. The all-xpub version of the same descriptor imports fine. That's what gave it away.

The duplicate check (KeyCompare) resolves each key expression to a pubkey and compares the results. It does this at index 0, and the old code used an empty signing provider. With that empty provider, a musig() expression can't resolve when one of its participants is on a hardened path, because deriving that participant needs its private key, so the whole aggregate key comes back empty. Two different musig expressions both came back empty, so the check treated them as duplicates. The fix derives against the signing provider populated during parsing, which holds the private keys, and only compares the expression strings when neither side resolves. 151henry151 had suggested looking at the empty signing provider on the issue.

scgbckbone found a second, separate bug in the same descriptors. When another expression that reuses those participants is handled in the same expansion, its participant origin in the PSBT is added twice, so m/86h/1h/0h becomes m/86h/1h/0h/86h/1h/0h in both the input and output Taproot BIP32 derivation maps. OriginPubkeyProvider::GetPubKey() now derives into a temporary provider, merges it, and writes the corrected origin once, so a later expression can't prepend the same origin again.

Tested:

./build/bin/test_bitcoin --run_test=descriptor_tests
./build/bin/test_bitcoin --run_test=miniscript_tests
./build/bin/test_bitcoin --run_test=bip328_tests
./build/bin/test_bitcoin --run_test=psbt_wallet_tests
./build/test/functional/test_runner.py wallet_musig.py --jobs=1

DrahtBot added the label Consensus on Feb 27, 2026

DrahtBot commented at 11:17 PM on February 27, 2026: 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/34697.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	scgbckbone
Stale ACK	Bortlesboat

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35742 (descriptors: check duplicate keys in all multipath Miniscript branches by yashbhutwala)
#35493 (wallet, descriptor: Fix MuSig private key completeness checks on importdescriptors by w0xlt)
#35445 (wallet, descriptor: Revert StringType::COMPAT for Miniscript expressions and drop the concept of a Descriptor ID that can be validated by achow101)
#35041 (descriptor: speed-up Parse (xpub/xpriv) in ~30% by brunoerg)
#32857 (wallet: allow skipping script paths by Sjors)

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

Check(..., {{0xFFFFFFFFUL, 0}, {0xFFFFFFFFUL, 1}}) in src/test/descriptor_tests.cpp
Check(..., {{}, {0xFFFFFFFFUL}, {0, 0}, {0, 1}, {0, 2}, {0xFFFFFFFFUL, 0}, {0xFFFFFFFFUL, 1}, {0xFFFFFFFFUL, 2}}) in src/test/descriptor_tests.cpp
Check(..., {{86 | 0x80000000UL, 1 | 0x80000000UL, 0 | 0x80000000UL, 1}, {2}}) in src/test/descriptor_tests.cpp

DrahtBot added the label CI failed on Feb 28, 2026

DrahtBot removed the label CI failed on Mar 3, 2026

Bortlesboat commented at 10:44 PM on March 7, 2026: contributor

Code Review ACK d2c5b2ffbd

The bug is clear and the fix is correct:

Before: PubkeyProvider::operator< derived pubkeys using an empty FlatSigningProvider. For hardened derivation steps, GetPubKey() needs private key material from the signing provider, so it returned nullopt. Two distinct musig() expressions with hardened-path participants both resolved to nullopt, causing nullopt < nullopt == false in both directions, making CheckDuplicateKey treat them as duplicates.

After: KeyCompare uses m_out (the signing provider populated during parsing, which has the private keys from xprv participants) instead of an empty provider. This allows hardened-path keys to resolve correctly, so distinct keys compare as distinct.

The removal of PubkeyProvider::operator< is a nice cleanup — the method was only used by KeyCompare and its empty-provider approach was fundamentally broken for hardened keys.

Regression test covers the exact scenario from #34273.

shuv-amp force-pushed on Mar 7, 2026

shuv-amp force-pushed on Mar 8, 2026

shuv-amp renamed this:
~~script: fix false-positive duplicate key detection in musig miniscript~~
descriptor: fix false duplicate-key detection for hardened musig()
on Mar 8, 2026

shuv-amp force-pushed on Mar 8, 2026

shuv-amp commented at 1:14 AM on March 8, 2026: none

For attribution, I also referred to @w0xlt's work here while reworking this:

https://github.com/w0xlt/bitcoin/commit/921cbdb08b271e8e866a1c0e6912e6241c964c9e

Not a direct copy, but it helped point me to the fix direction in the latest revision.

scgbckbone commented at 8:53 PM on March 19, 2026: contributor

this is broken & produces incorrect PSBTs where PSBT_IN_TAP_BIP32_DERIVATION and PSBT_OUT_TAP_BIP32_DERIVATION have wrong key derivation info. So instead for example m/86h/1h/0h has m/86h/1h/0h/86h/1h/0h (doubled).

Example policy with identical musig key expression with different key derivation info:

tr(unspend(),and_v(v:pk(musig(@0,@1,@2)/<0;1>/*),pk(musig(@0,@1,@2)/<2;3>/*)))

Descriptors (from listdescriptors):

{'wallet_name': 'ident_musig_subder', 'descriptors': [{'desc': 'tr(tpubD6NzVbkrYhZ4WMCcYXEgDrM1AFATSGmrFyMnA2Dgx1Y551VueaE3iemHhQBxhKWjepG76Cy5QmXv8z4KwN6ARTnZ5hVuShfxL129NVGXZuE/0/*,and_v(v:pk(musig([0f056943/86h/1h/0h]tpubDCeEX49avtiXrBTv3JWTtco99Ka499jXdZHBRtm7va2gkMAui11ctZjqNAT9dLVNaEozt2C1kfTM88cnvZCXsWLJN2p4viGvsyGjtKVV7A1,[d4cb267a/44h/1h/0h]tpubDDjr2zotTQ9bCX5WFpj4YmUxUnU8fvk8RKGQGEivdbA3rvnbPJmBDC9H5BqQpN8L65ZM5RrGtyUrkbaux1Ys6R54MK2dUz1mgjF9eV9qkR6,[be242744/44h/1h/0h]tpubDDbRaxzhrtUw7XCP9n9kEHymwHhHjYuA51kBDTMNwRD8bM7HZ4vsaPgqZqSzjQyVpUaCRsgWr7egTVbjCTJHy39E3bLP2wwVHuW2K7R3DmY)/0/*),pk(musig([0f056943/86h/1h/0h]tpubDCeEX49avtiXrBTv3JWTtco99Ka499jXdZHBRtm7va2gkMAui11ctZjqNAT9dLVNaEozt2C1kfTM88cnvZCXsWLJN2p4viGvsyGjtKVV7A1,[d4cb267a/44h/1h/0h]tpubDDjr2zotTQ9bCX5WFpj4YmUxUnU8fvk8RKGQGEivdbA3rvnbPJmBDC9H5BqQpN8L65ZM5RrGtyUrkbaux1Ys6R54MK2dUz1mgjF9eV9qkR6,[be242744/44h/1h/0h]tpubDDbRaxzhrtUw7XCP9n9kEHymwHhHjYuA51kBDTMNwRD8bM7HZ4vsaPgqZqSzjQyVpUaCRsgWr7egTVbjCTJHy39E3bLP2wwVHuW2K7R3DmY)/2/*)))#cagg4sg0', 'timestamp': 1773952443, 'active': True, 'internal': False, 'range': [0, 100], 'next': 3, 'next_index': 3}, {'desc': 'tr(tpubD6NzVbkrYhZ4WMCcYXEgDrM1AFATSGmrFyMnA2Dgx1Y551VueaE3iemHhQBxhKWjepG76Cy5QmXv8z4KwN6ARTnZ5hVuShfxL129NVGXZuE/1/*,and_v(v:pk(musig([0f056943/86h/1h/0h]tpubDCeEX49avtiXrBTv3JWTtco99Ka499jXdZHBRtm7va2gkMAui11ctZjqNAT9dLVNaEozt2C1kfTM88cnvZCXsWLJN2p4viGvsyGjtKVV7A1,[d4cb267a/44h/1h/0h]tpubDDjr2zotTQ9bCX5WFpj4YmUxUnU8fvk8RKGQGEivdbA3rvnbPJmBDC9H5BqQpN8L65ZM5RrGtyUrkbaux1Ys6R54MK2dUz1mgjF9eV9qkR6,[be242744/44h/1h/0h]tpubDDbRaxzhrtUw7XCP9n9kEHymwHhHjYuA51kBDTMNwRD8bM7HZ4vsaPgqZqSzjQyVpUaCRsgWr7egTVbjCTJHy39E3bLP2wwVHuW2K7R3DmY)/1/*),pk(musig([0f056943/86h/1h/0h]tpubDCeEX49avtiXrBTv3JWTtco99Ka499jXdZHBRtm7va2gkMAui11ctZjqNAT9dLVNaEozt2C1kfTM88cnvZCXsWLJN2p4viGvsyGjtKVV7A1,[d4cb267a/44h/1h/0h]tpubDDjr2zotTQ9bCX5WFpj4YmUxUnU8fvk8RKGQGEivdbA3rvnbPJmBDC9H5BqQpN8L65ZM5RrGtyUrkbaux1Ys6R54MK2dUz1mgjF9eV9qkR6,[be242744/44h/1h/0h]tpubDDbRaxzhrtUw7XCP9n9kEHymwHhHjYuA51kBDTMNwRD8bM7HZ4vsaPgqZqSzjQyVpUaCRsgWr7egTVbjCTJHy39E3bLP2wwVHuW2K7R3DmY)/3/*)))#wshkxfmd', 'timestamp': 1773952443, 'active': True, 'internal': True, 'range': [0, 100], 'next': 2, 'next_index': 2}]}

Invalid PSBT produced with walletcreatefundedpsbt:

cHNidP8BAIkCAAAAAWGlTAOe5CR+5kIuveSdj4GrIfLLjYDpab2pLTWAUQfiAQAAAAD9////AkoHCJIAAAAAIlEglAEsB4z+Jx2RIT47AF9gnkQMSofYSF/OCnShqqOhPL+ACAiSAAAAACJRIA5PL3SfKi+OfX3IdVQ3DwXhI1WzIYk6V1foWAC2oCzhAAAAAAABASsAERAkAQAAACJRIP/Wc/ylbCwkQAqnhNV+hGRf4ADn7YQVD3t35s6GWSOSIhXBFPtzJ01QvvG2wWKC70L1wQoVa/tEj0ZV8z7WCZ8fWn9FIBlVHXyKX/dzYVa328zOGmjQzL4wOIxMz1HvQPnHNzjRrSCmWXdGmau8dTa2O8EOxiSwiZgmcHFvQ5ol79ewUgJyZqzAIRYU+3MnTVC+8bbBYoLvQvXBChVr+0SPRlXzPtYJnx9afw0AfEYeXQAAAAAAAAAAIRYZVR18il/3c2FWt9vMzhpo0My+MDiMTM9R70D5xzc40S0BzV7ymTROB3u+1SMM80p5oOk4YHcqjFujfePdZCw3bg78+MvlAAAAAAAAAAAhFlfuNxx6b6kxHwwkYH27XQ+N/oRWMHWeWg4yG75U/IqpPQHNXvKZNE4He77VIwzzSnmg6ThgdyqMW6N9491kLDduDr4kJ0QsAACAAQAAgAAAAIAsAACAAQAAgAAAAIAhFqZZd0aZq7x1NrY7wQ7GJLCJmCZwcW9DmiXv17BSAnJmLQHNXvKZNE4He77VIwzzSnmg6ThgdyqMW6N9491kLDduDvz4y+UCAAAAAAAAACEWrylR8DuGe6dyJqmNY8cey+q7FSn6si/uFfYnoHEGGnU9Ac1e8pk0Tgd7vtUjDPNKeaDpOGB3Koxbo33j3WQsN24ODwVpQ1YAAIABAACAAAAAgFYAAIABAACAAAAAgCEWtYiF4esSAfbBlQgaFJBVpMajf8WWFsWRJNb/UIBxgSc9Ac1e8pk0Tgd7vtUjDPNKeaDpOGB3Koxbo33j3WQsN24O1MsmeiwAAIABAACAAAAAgCwAAIABAACAAAAAgAEXIBT7cydNUL7xtsFigu9C9cEKFWv7RI9GVfM+1gmfH1p/ARggzV7ymTROB3u+1SMM80p5oOk4YHcqjFujfePdZCw3bg4iGgMVFs+43JrSwExLgMMLz345ka/0HJsWRVDbnFc1xoG1imMCrylR8DuGe6dyJqmNY8cey+q7FSn6si/uFfYnoHEGGnUCtYiF4esSAfbBlQgaFJBVpMajf8WWFsWRJNb/UIBxgScDV+43HHpvqTEfDCRgfbtdD43+hFYwdZ5aDjIbvlT8iqkAAQUgddcbN0Ssw6D0YVHzmS+O5GkFJH7f9Bbq62BGzadKAPkBBkcAwEQg5eUHYUwmV8VurjFwTSABM5kI9fWZWmSQ5xP0yRNAvIOtIPkQ5Vja06GfNZl7UFPO99YVBPupS/rJFyE1YvFa28furCEHV+43HHpvqTEfDCRgfbtdD43+hFYwdZ5aDjIbvlT8iqk9AYGHvCToOGbNfuwT9YRCMDypanJNofChYliNc4zDrv75viQnRCwAAIABAACAAAAAgCwAAIABAACAAAAAgCEHddcbN0Ssw6D0YVHzmS+O5GkFJH7f9Bbq62BGzadKAPkNAHxGHl0AAAAAAQAAACEHrylR8DuGe6dyJqmNY8cey+q7FSn6si/uFfYnoHEGGnU9AYGHvCToOGbNfuwT9YRCMDypanJNofChYliNc4zDrv75DwVpQ1YAAIABAACAAAAAgFYAAIABAACAAAAAgCEHtYiF4esSAfbBlQgaFJBVpMajf8WWFsWRJNb/UIBxgSc9AYGHvCToOGbNfuwT9YRCMDypanJNofChYliNc4zDrv751MsmeiwAAIABAACAAAAAgCwAAIABAACAAAAAgCEH5eUHYUwmV8VurjFwTSABM5kI9fWZWmSQ5xP0yRNAvIMtAYGHvCToOGbNfuwT9YRCMDypanJNofChYliNc4zDrv75/PjL5QAAAAABAAAAIQf5EOVY2tOhnzWZe1BTzvfWFQT7qUv6yRchNWLxWtvH7i0BgYe8JOg4Zs1+7BP1hEIwPKlqck2h8KFiWI1zjMOu/vn8+MvlAgAAAAEAAAAiCAMVFs+43JrSwExLgMMLz345ka/0HJsWRVDbnFc1xoG1imMCrylR8DuGe6dyJqmNY8cey+q7FSn6si/uFfYnoHEGGnUCtYiF4esSAfbBlQgaFJBVpMajf8WWFsWRJNb/UIBxgScDV+43HHpvqTEfDCRgfbtdD43+hFYwdZ5aDjIbvlT8iqkAAQUgtHS7swPWdk/zmt7YcdWW4D+b3Ug1ewMrGS+2znlJyhgBBkcAwEQgSvLepLCSlDPDiTP61uRCZWHoCteg6uhJN51vcwHFdy6tIIIYPJwweXnPUEFXm/xb7vGCUjQr+3E6Vh/S0jdsX2NirCEHSvLepLCSlDPDiTP61uRCZWHoCteg6uhJN51vcwHFdy4tAU01q8wKs/JAgBbBgMSz35C2d5S4Kqo9dLSQdHUflkdW/PjL5QAAAAACAAAAIQdX7jccem+pMR8MJGB9u10Pjf6EVjB1nloOMhu+VPyKqT0BTTWrzAqz8kCAFsGAxLPfkLZ3lLgqqj10tJB0dR+WR1a+JCdELAAAgAEAAIAAAACALAAAgAEAAIAAAACAIQeCGDycMHl5z1BBV5v8W+7xglI0K/txOlYf0tI3bF9jYi0BTTWrzAqz8kCAFsGAxLPfkLZ3lLgqqj10tJB0dR+WR1b8+MvlAgAAAAIAAAAhB68pUfA7hnunciapjWPHHsvquxUp+rIv7hX2J6BxBhp1PQFNNavMCrPyQIAWwYDEs9+QtneUuCqqPXS0kHR1H5ZHVg8FaUNWAACAAQAAgAAAAIBWAACAAQAAgAAAAIAhB7R0u7MD1nZP85re2HHVluA/m91INXsDKxkvts55ScoYDQB8Rh5dAAAAAAIAAAAhB7WIheHrEgH2wZUIGhSQVaTGo3/FlhbFkSTW/1CAcYEnPQFNNavMCrPyQIAWwYDEs9+QtneUuCqqPXS0kHR1H5ZHVtTLJnosAACAAQAAgAAAAIAsAACAAQAAgAAAAIAiCAMVFs+43JrSwExLgMMLz345ka/0HJsWRVDbnFc1xoG1imMCrylR8DuGe6dyJqmNY8cey+q7FSn6si/uFfYnoHEGGnUCtYiF4esSAfbBlQgaFJBVpMajf8WWFsWRJNb/UIBxgScDV+43HHpvqTEfDCRgfbtdD43+hFYwdZ5aDjIbvlT8iqkA

shuv-amp commented at 9:19 PM on March 19, 2026: none

Thanks, this looks like a real issue. Marking draft while I rework this.

shuv-amp marked this as a draft on Mar 19, 2026

shuv-amp force-pushed on Mar 20, 2026

shuv-amp renamed this:
~~descriptor: fix false duplicate-key detection for hardened musig()~~
descriptor: fix musig duplicate checks and origin handling
on Mar 20, 2026

shuv-amp marked this as ready for review on Mar 20, 2026

scgbckbone commented at 8:45 AM on March 20, 2026: contributor

tACK

sedited commented at 7:26 AM on July 24, 2026: contributor

This has not received review in a while. My feeling is this is because the description makes it seem like this is an LLM generated pull request. Can you re-write it in your own words and include an example for an externally visible behaviour change?

shuv-amp force-pushed on Jul 24, 2026

shuv-amp renamed this:
~~descriptor: fix musig duplicate checks and origin handling~~
descriptor: fix musig() duplicate key checks and doubled PSBT origin paths
on Jul 24, 2026

shuv-amp commented at 3:04 PM on July 24, 2026: none

Thanks. I've rewritten the description in my own words and added an externally visible example.

shuv-amp force-pushed on Jul 24, 2026

in src/test/descriptor_tests.cpp:1291 in 0343feea0e

1287 | @@ -1288,6 +1288,137 @@ BOOST_AUTO_TEST_CASE(descriptor_test)
1288 |      CheckUnparsable("tr(musig(tuus(oldepk(gg)ggggfgg)<,z(((((((((((((((((((((st)", "tr(musig(tuus(oldepk(gg)ggggfgg)<,z(((((((((((((((((((((st)","tr(): Too many ')' in musig() expression");
1289 |  }
1290 |  
1291 | +BOOST_AUTO_TEST_CASE(musig_duplicate_keys)

achow101 commented at 8:34 PM on July 24, 2026:

New fixed descriptor test cases should be added above with Check or CheckUnparseable.

shuv-amp commented at 8:25 PM on July 25, 2026:

Moved these into descriptor_test using Check and CheckUnparsable.

in src/test/descriptor_tests.cpp:1361 in 0343feea0e

1356 | +        BOOST_CHECK_MESSAGE(error.find("duplicate public keys") != std::string::npos,
1357 | +                            "Expected 'duplicate public keys' error for equivalent pubkeys, got: " + error);
1358 | +    }
1359 | +}
1360 | +
1361 | +BOOST_AUTO_TEST_CASE(musig_origin_paths)

achow101 commented at 8:35 PM on July 24, 2026:

This test case should be added above as a Check

shuv-amp commented at 8:26 PM on July 25, 2026:

Moved this into descriptor_test using Check.

in src/script/descriptor.cpp:2257 in 0343feea0e

2252 | @@ -2257,7 +2253,18 @@ struct KeyParser {
2253 |          : m_out(out), m_in(in), m_script_ctx(ctx), m_expr_index(key_exp_index) {}
2254 |  
2255 |      bool KeyCompare(const Key& a, const Key& b) const {
2256 | -        return *m_keys.at(a).at(0) < *m_keys.at(b).at(0);
2257 | +        // Hardened paths need the private keys collected while parsing.
2258 | +        const SigningProvider& sign_provider = m_out ? static_cast<const SigningProvider&>(*m_out) :

achow101 commented at 8:42 PM on July 24, 2026:

Casting is unnecessary.

shuv-amp commented at 8:26 PM on July 25, 2026:

Removed the cast.

in src/script/descriptor.cpp:2258 in 0343feea0e

2252 | @@ -2257,7 +2253,18 @@ struct KeyParser {
2253 |          : m_out(out), m_in(in), m_script_ctx(ctx), m_expr_index(key_exp_index) {}
2254 |  
2255 |      bool KeyCompare(const Key& a, const Key& b) const {
2256 | -        return *m_keys.at(a).at(0) < *m_keys.at(b).at(0);
2257 | +        // Hardened paths need the private keys collected while parsing.
2258 | +        const SigningProvider& sign_provider = m_out ? static_cast<const SigningProvider&>(*m_out) :
2259 | +            m_in ? *m_in : DUMMY_SIGNING_PROVIDER;

achow101 commented at 8:43 PM on July 24, 2026:

Please wrap nested ternary expressions in parentheses to clearly delineate them.

shuv-amp commented at 8:27 PM on July 25, 2026:

Wrapped the nested ternary in parentheses.

achow101 commented at 8:57 PM on July 24, 2026: member

You are fixing 2 problems in one commit. Please make a separate commit for each issue fixed.

descriptor: fix duplicate check for hardened keys

The miniscript duplicate key check compares two key expressions by
deriving each of them at index 0, and it did that with an empty signing
provider. Any expression with a hardened step could therefore not be
derived and came back empty, so two of them compared equal and the
descriptor was rejected with "contains duplicate public keys" even
though the keys were different. musig() makes this easy to run into,
since one participant on a hardened path keeps the whole aggregate key
from resolving, but plain key expressions are affected just the same.

Derive with the signing provider that is filled while parsing, or with
the one we are inferring from, since that is where the private keys for
the hardened steps are. If both keys still cannot be derived, compare
the key expressions instead, so that two different expressions are not
mistaken for one another.

A duplicate written two different ways can still be missed if either
spelling cannot be derived. The private key that is missing for the
comparison is also needed to expand the descriptor, so deriveaddresses
and importdescriptors both refuse it. Once the needed private keys are
available, both expressions resolve and the duplicate is caught again.

12e300ea22

descriptor: don't prepend key origins twice

OriginPubkeyProvider::GetPubKey() derives its sub-provider straight into
the output provider and then prepends its origin to the entry it finds
there. The sub-providers insert with emplace(), so if the same key was
already expanded by another key expression the insert is a no-op and the
entry that gets the origin prepended is the one that already has it.

musig() is where this shows up, because it expands its participants into
the provider it is given, so two musig() expressions in the same key
expression list end up expanding a shared participant twice. A
participant origin of m/86h/1h/0h then comes out as
m/86h/1h/0h/86h/1h/0h in the input and output taproot BIP32 derivation
maps of a PSBT, so a signer that follows them derives the wrong key.
When the two expressions declare different origins for the participant,
the fingerprint of one is combined with the path of both.

Derive into a temporary provider instead and merge that into the output.
Merge() keeps the entry that is already there, which is the one that has
had the origin prepended once.

04a30d7171

shuv-amp force-pushed on Jul 25, 2026

shuv-amp commented at 8:30 PM on July 25, 2026: none

Split the two fixes into separate commits.

DrahtBot added the label CI failed on Jul 29, 2026

DrahtBot removed the label CI failed on Jul 29, 2026