descriptors: check duplicate keys in all multipath Miniscript branches #35742

Fixes #35629.

Multipath Miniscript descriptors performed duplicate-key sanity checking while parsing the template, when key comparison used the first multipath branch. As a result, duplicate keys present only in a later branch were accepted.

Select each expanded multipath branch and rerun the existing duplicate-key check before constructing the final descriptors. If a branch contains a duplicate, report the offending concrete Miniscript expression.

The regression test covers:

• a valid two-branch wsh control;
• a duplicate in the second wsh branch;
• a duplicate in the third of three tr branches.

Tested with:

build/bin/test_bitcoin --run_test=descriptor_tests/multipath_miniscript_duplicate_keys
build/bin/test_bitcoin --run_test=descriptor_tests
build/bin/test_bitcoin

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35742.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

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

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #36122 (BIP460: CISA for Taproot key path spends by fjahr)
• #35445 (wallet, descriptor: Revert StringType::COMPAT for Miniscript expressions and drop the concept of a Descriptor ID that can be validated by achow101)

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.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

I ran this on master and on pr-35742,

bitcoin-cli getdescriptorinfo "wsh(or_i(pk(xpub6BgBgsespWvERF3LHQu6CnqdvfEvtMcQjYrcRzx53QJjSxarj2afYWcLteoGVky7D3UKDP9QyrLprQ3VCECoY49yfdDEHGCtMMj92pReUsQ/<0;1>),pk(xpub6BgBgsespWvERF3LHQu6CnqdvfEvtMcQjYrcRzx53QJjSxarj2afYWcLteoGVky7D3UKDP9QyrLprQ3VCECoY49yfdDEHGCtMMj92pReUsQ/<2;1>)))"

and this is what I got:

On master Same public key is generated

{
  "descriptor": "wsh(or_i(pk(xpub6BgBgsespWvERF3LHQu6CnqdvfEvtMcQjYrcRzx53QJjSxarj2afYWcLteoGVky7D3UKDP9QyrLprQ3VCECoY49yfdDEHGCtMMj92pReUsQ/0),pk(xpub6BgBgsespWvERF3LHQu6CnqdvfEvtMcQjYrcRzx53QJjSxarj2afYWcLteoGVky7D3UKDP9QyrLprQ3VCECoY49yfdDEHGCtMMj92pReUsQ/2)))#kzwg47ay",
  "multipath_expansion": [
    "wsh(or_i(pk(xpub6BgBgsespWvERF3LHQu6CnqdvfEvtMcQjYrcRzx53QJjSxarj2afYWcLteoGVky7D3UKDP9QyrLprQ3VCECoY49yfdDEHGCtMMj92pReUsQ/0),pk(xpub6BgBgsespWvERF3LHQu6CnqdvfEvtMcQjYrcRzx53QJjSxarj2afYWcLteoGVky7D3UKDP9QyrLprQ3VCECoY49yfdDEHGCtMMj92pReUsQ/2)))#kzwg47ay",
    "wsh(or_i(pk(xpub6BgBgsespWvERF3LHQu6CnqdvfEvtMcQjYrcRzx53QJjSxarj2afYWcLteoGVky7D3UKDP9QyrLprQ3VCECoY49yfdDEHGCtMMj92pReUsQ/1),pk(xpub6BgBgsespWvERF3LHQu6CnqdvfEvtMcQjYrcRzx53QJjSxarj2afYWcLteoGVky7D3UKDP9QyrLprQ3VCECoY49yfdDEHGCtMMj92pReUsQ/1)))#jkt2efkp"
  ],
  "checksum": "mn7hqkkh",
  "isrange": false,
  "issolvable": true,
  "hasprivatekeys": false
}

On branch

error code: -5
error message:
or_i(pk(xpub6BgBgsespWvERF3LHQu6CnqdvfEvtMcQjYrcRzx53QJjSxarj2afYWcLteoGVky7D3UKDP9QyrLprQ3VCECoY49yfdDEHGCtMMj92pReUsQ/1),pk(xpub6BgBgsespWvERF3LHQu6CnqdvfEvtMcQjYrcRzx53QJjSxarj2afYWcLteoGVky7D3UKDP9QyrLprQ3VCECoY49yfdDEHGCtMMj92pReUsQ/1)) is not sane: contains duplicate public keys

I also checked using importdescriptors to observe wallet action.

On master, the unsafe branch is added to the wallet:

bitcoin-cli -rpcwallet=testwallet_master importdescriptors '[{"desc": "wsh(or_i(pk(xpub6BgBgsespWvERF3LHQu6CnqdvfEvtMcQjYrcRzx53QJjSxarj2afYWcLteoGVky7D3UKDP9QyrLprQ3VCECoY49yfdDEHGCtMMj92pReUsQ/<0;1>),pk(xpub6BgBgsespWvERF3LHQu6CnqdvfEvtMcQjYrcRzx53QJjSxarj2afYWcLteoGVky7D3UKDP9QyrLprQ3VCECoY49yfdDEHGCtMMj92pReUsQ/<2;1>)))#mn7hqkkh", "timestamp": "now", "active": false}]'

[
  {
    "success": true
  }
]

On the PR:

[
  {
    "success": false,
    "error": {
      "code": -5,
      "message": "or_i(pk(xpub6BgBgsespWvERF3LHQu6CnqdvfEvtMcQjYrcRzx53QJjSxarj2afYWcLteoGVky7D3UKDP9QyrLprQ3VCECoY49yfdDEHGCtMMj92pReUsQ/1),pk(xpub6BgBgsespWvERF3LHQu6CnqdvfEvtMcQjYrcRzx53QJjSxarj2afYWcLteoGVky7D3UKDP9QyrLprQ3VCECoY49yfdDEHGCtMMj92pReUsQ/1)) is not sane: contains duplicate public keys"
    }
  }
]

This shows exactly how the PR fixes the silent passing on master

Concept ACK, I also found the same issue - please rebase.

Concept ACK

I suggest splitting this into focused commits so reviewers can understand the old behavior and the problem before assessing the fix: test infrastructure and controls, characterization of the old behavior with TODOs explaining why those expectations are wrong, then the fix and updated expectations. See https://github.com/l0rinc/bitcoin/pull/295 for how I split the same change.

We should also take this opportunity to add coverage for mixed single-path/multipath keys, wildcards, hardened paths with and without private keys, differing origins, nested errors, and valid key sharing across branches and Taproot leaves.

A functional test would demonstrate the wallet impact: these imports currently succeed but prevent reload. With the fix, rejected imports leave the descriptor set unchanged and the wallet reloadable.

Edit: please make the PR description less robotic, show us how you tested this manually for example