descriptor: fix false duplicate-key detection for hardened musig() #34697

1. 
   shuv-amp commented at 11:17 pm on February 27, 2026: none

   Fixes #34273.

   Miniscript duplicate-key checking in descriptor.cpp compares parsed key expressions while checking descriptor sanity.

   For musig() expressions with hardened BIP32 participant derivation, resolving the aggregate pubkey requires private key material. The old comparison path derived pubkeys with an empty FlatSigningProvider, so GetPubKey() could return nullopt. Two distinct musig() expressions could then compare equal and be rejected as contains duplicate public keys.

   Use the parsed signing data for the comparison so hardened participant derivation succeeds and duplicate checking continues to compare resolved pubkeys.

   Add a regression test for the reported hardened musig() case, and keep coverage for true duplicate musig() expressions.

   Tested:

   • build-tsan/bin/test_bitcoin --run_test=descriptor_tests --catch_system_errors=no
2. DrahtBot added the label Consensus on Feb 27, 2026
3. 
   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.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Stale ACK	Bortlesboat

   If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

4. DrahtBot added the label CI failed on Feb 28, 2026
5. DrahtBot removed the label CI failed on Mar 3, 2026
6. 
   Bortlesboat commented at 10:44 pm on March 7, 2026: none

   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.

7. shuv-amp force-pushed on Mar 7, 2026
8. shuv-amp force-pushed on Mar 8, 2026
9. 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
10. descriptor: fix false-positive duplicate key in musig

    KeyParser::KeyCompare compared key expressions via PubkeyProvider::operator<,
    which resolved pubkeys using an empty FlatSigningProvider. For BIP32 keys with
    hardened derivation steps, GetPubKey() requires private key material and returns
    nullopt without it. Two distinct musig() expressions would both resolve to
    nullopt; since (nullopt < nullopt) is false in both directions, std::set treated
    them as duplicates, causing CheckDuplicateKey to incorrectly reject valid
    descriptors.
    
    Replace the comparison with ToString(), which gives each key expression a unique
    string independent of key material availability. Remove PubkeyProvider::operator<,
    which is no longer needed.
    
    Add regression tests for: musig with distinct hardened derivation paths (the
    originally reported case), identical musig expressions (true duplicates must
    still be caught), xpub-only musig with distinct paths, non-musig plain key
    duplicates, musig with different hardened participant paths, and musig with
    reversed participant order.
    
    Fixes #34273

    6413b81540
11. shuv-amp force-pushed on Mar 8, 2026
12. 
    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.

Contributors
shuv-amp DrahtBot Bortlesboat

Labels
Consensus