#36420 descriptor: compare x-only public keys in Tapscript Miniscript duplicate check

ReviewabilityReviewsAgreementSize
#36420 Yudis-bit descriptor: compare x-only public keys in Tapscript Miniscript duplicate checkReady0 (+1)Strong6 + 20 tests
  • Prevent duplicate public keys differing only by parity from passing Tapscript Miniscript sanity checks
  • Protect descriptor wallets against third-party transaction malleability from identical script branches
  • Ready for review
  • Strong support with thorough test verification (fametrano)
  • Confirmed new tr descriptor tests fail as expected without the descriptor comparison fix (fametrano)
  • Resolved earlier feedback by dropping an invalid multi_a test and adding a WIF test case (fametrano)
  • 6 lines added or modified outside tests
  • 20 lines added or modified in tests
  • 1 lines removed in total
  • 3 files, 1 commits
Wallet#23 of 92
P2 · bug fix
  • P2 because it fixes a descriptor validation bug that breaks BIP-379 non-malleability guarantees
  • Prevents third parties from malleating transactions by switching branches without private keys
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

Compares public keys as 32-byte x-only public keys instead of 33-byte compressed keys during Miniscript duplicate-key checks in Tapscript descriptors. In a Tapscript context, keys sharing an x-coordinate but differing in parity byte are now detected as identical, causing DuplicateKeyCheck() to fail and mark the descriptor as not sane. Segwit v0 descriptors continue to distinguish full 33-byte keys.

Problem: When a tr() Miniscript descriptor contains keys that differ only in parity prefix (such as pk(02X) and pk(03X)), KeyParser::KeyCompare previously treated them as distinct 33-byte keys and passed sanity checks. Because Tapscript drops key parity on-chain, both branches serialize to identical scripts, violating BIP-379 invariants against duplicate keys and allowing third parties to malleate transactions by switching execution branches without possessing private keys.

Evidence of importance:

  • Commit 0956ace45d and PR description cite BIP-379: 'BIP-379 malleability analysis relies on the invariant that no public key is repeated. Because one signature satisfies both branches, a third party can malleate transactions by flipping the branch selector without possessing the private key.'
  • Linked issue #36414 demonstrates that tr() descriptors previously marked scripts with identical x-coordinates and different parities as sane.

Risk of a mistake: low (wallet, tests). A mistake in descriptor key comparison could falsely reject valid descriptors or fail to detect duplicate keys, impacting descriptor-based wallet imports and script sanity checks.

Changed since the previous assessment: The implementation now provides complete diffs and unit test additions verifying both tr() descriptor rejection and continued wsh() parsing of distinct 33-byte compressed keys.

Reviewability

Ready

Nothing known that would invalidate a review now.

Waiting on (from the thread read):

  • reviewer: re-review or confirmation of clean rebase 2026-11-01

Agreement

Strong

substantive support, no open objection (fametrano, fametrano)

Thread notes: Author rebased cleanly on master (c:6551460341).

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
fametrano (none)objection, correctnessdescriptor_tests fails on the multi_a check because multi_a directly in tr() is not Miniscript and has no duplicate-key checkaccepted · fix pushed · author dropped the test and reviewer ACKedchangedyes2026-10-02: 'descriptor_tests fails on the multi_a check because that descriptor parses: a multi_a directly in tr() is not Miniscript and has no duplicate-key check'
Status from: 2026-10-03: fametrano: 'ACK a1c65a776c252ce253972d1712936f315cbc5abc'
fametrano (none)suggestion, correctnessDrop the failing multi_a test check from descriptor_tests.cppaccepted · fix pushed · author dropped the test and reviewer ACKedchangedyes2026-10-02: 'This descriptor parses, so this check fails'
Status from: 2026-10-03: fametrano: 'ACK a1c65a776c252ce253972d1712936f315cbc5abc'
fametrano (none)suggestion, correctnessAdd a test case with the second key as a WIF whose public key is 03Xaccepted · fix pushed · author added the test case and reviewer reACKedansweredyes2026-10-02: 'Optional: the same case with the second key as a WIF whose public key is `03X`.'
Status from: 2026-10-03: fametrano: 'reACK 0956ace45dac99bc11256f4db1ca59f53016af6b'

Support:

  • fametrano (none) (ACK, 2026-10-03) [both: descriptor.cpp, descriptor_tests, miniscript_tests, tr()]: descriptor_tests and miniscript_tests pass, and both new tr() checks fail without the descriptor.cpp change.
  • fametrano (none) (ACK, 2026-10-03) [both: descriptor.cpp, descriptor_tests, miniscript_tests, tr()]: Only the WIF test case was added since a1c65a77. descriptor_tests and miniscript_tests pass; without the descriptor.cpp change, the two tr() cases and the WIF case fail, the WIF one because its privat

Participants:

  • fametrano (none, support, 5 statements 2026-10-02 to 2026-10-03): tested and reACKed after failing multi_a check was dropped and suggested WIF test case was added

Files

20 lines under test/bench/ci.

  • src/test/descriptor_tests.cpp +17/-0
  • src/script/descriptor.cpp +6/-1
  • src/test/miniscript_tests.cpp +3/-0

Card

The short text the ranking pass compares across PRs.

This pull request updates Tapscript Miniscript duplicate-key validation to compare public keys as 32-byte x-only keys rather than full 33-byte compressed keys. Previously, tr() descriptors containing keys that differed only by parity passed sanity checks even though they serialize to identical scripts on-chain. This violated BIP-379 duplicate key guarantees and permitted third-party transaction malleability through branch flipping. P2WSH descriptor parsing remains unaffected and continues to distinguish 33-byte compressed keys.

Data

dossier JSON · extract JSON · model ledger, generated 2026-11-02T01:11, confidence high, input hash fd7e61eae58e9f8e
thread read through 2026-11-01 (9 statements), head 28121591bb
labels: Descriptors

Categories

Wallet

P2 · bug fix

Fixes a validation bug where Tapscript Miniscript descriptors with keys sharing an x-coordinate pass sanity checks, breaking BIP-379 non-malleability guarantees. The PR description notes that 'BIP-379 malleability analysis relies on the invariant that no public key is repeated' and that failing to detect identical x-only keys allows 'a third party [to] malleate transactions by flipping the branch selector without possessing the private key'.

Membership: Touches descriptor management logic in src/script/descriptor.cpp, which governs descriptor validation and parsing used by descriptor wallets.

Factors: security/stability 1, bug 2, performance 0, user value 1, leverage 0