#35742 descriptors: check duplicate keys in all multipath Miniscript branches
https://github.com/bitcoin/bitcoin/pull/35742 · · +39/-3 in 2 files, 1 commits · labels: Needs rebase, Descriptors
Goal
- Reject invalid multipath Miniscript descriptors with duplicate keys across any branch
- Prevents wallets from importing invalid descriptors that cause reload failures after restart
Fixes duplicate-key sanity checking for multipath Miniscript descriptors by evaluating all expanded multipath branches rather than only the first branch. If any branch contains duplicate keys, descriptor parsing fails with an informative sanity check error.
Problem: Multipath Miniscript descriptor parsing only evaluated duplicate keys on branch index 0. If a descriptor contained duplicate keys only on later branches (such as a change path), it was accepted as valid and could be imported into a wallet, potentially leading to wallet reload failures or invalid Miniscript spending policies.
Category: Wallet (#18 of 84)
P2 · bug fix
- P2 because importing an invalid descriptor can break wallet reloads on restart
- Prevents wallets from accepting invalid Miniscript spending policies on change paths
Fixes an unhandled edge case in descriptor sanity checks where duplicate keys across multipath Miniscript branches are accepted. Accepting invalid descriptors causes wallet reliability issues, including wallets failing to reload after restart once an invalid descriptor has been imported.
Membership: Modifies descriptor parsing and validation in src/script/descriptor.cpp for Miniscript descriptors used by descriptor wallets.
Factors: security/stability 1, bug 2, performance 0, user value 2, leverage 0
Reviewability: Stale: Needs rebase
- Needs rebase due to merge conflicts with master
- Author has not addressed requested commit restructuring and extra tests
PR has merge conflicts with master and open PRs (#36122, #35445), and the author has not addressed changes requested by l0rinc 8 days ago.
Author status: silent since force-push on 2026-09-05; has not responded to 2026-09-08 review comments.
Open concerns:
- l0rinc requested splitting the PR into focused commits characterizing old behavior with tests first, followed by the fix.
- l0rinc asked for broader test coverage (wildcards, mixed single/multipath, hardened paths) and a functional test for wallet reload.
- Code simplification proposed by l0rinc using FindInsaneSub directly.
Resolved concerns:
- Verification of bug reproduction on master versus PR branch confirmed by Zeegaths and vicjuma.
Agreement: Positive
- Strong concept support for fixing the BIP 389 sanity check violation
- Verified bug reproduction on master and fix on branch (Zeegaths)
- Concept approval noting alignment with BIP 389 (vicjuma)
- Requested split commits, simpler error extraction, and broader test coverage (l0rinc)
Strong concept support; l0rinc requested commit restructuring, simplified error extraction, and additional test coverage.
All reviewers agree this is a bug violating BIP 389 and Miniscript sanity rules; no concept objections exist, only requests for cleaner commit history and additional test cases.
- Zeegaths verified and confirmed bug reproduction on master and fix on branch via getdescriptorinfo and importdescriptors
- vicjuma gave Concept ACK noting the fix aligns with BIP 389
- l0rinc gave Concept ACK, later requested changes for commit organization and expanded test coverage
Review verdicts (DrahtBot): 0
Files
21 lines under test/bench/ci.
- src/script/descriptor.cpp +18/-3
- src/test/descriptor_tests.cpp +21/-0
Card
PR 35742 fixes duplicate-key sanity checking for multipath Miniscript descriptors in src/script/descriptor.cpp. Previously, duplicate key detection only inspected the first branch (index 0), permitting descriptors with key collisions on later multipath branches to be accepted and imported into wallets. Concept agreement is unanimous with reproduction confirmed by several contributors, though review is currently blocked on a rebase and commit restructuring requested by l0rinc.