descriptor: Expose key expression parsing #35918

pseudoramdom commented at 6:44 PM on August 6, 2026: contributor

This PR exposes a utility function to parse and validate a standalone descriptor key expression. This is particularly useful in GUI multisig setup flows where the UI accepts a pasted key expression. The GUI can use this parser to validate input, inspect the parsed key, and apply setup-specific policy i.e. reject private keys, uncompressed keys etc. (Multisig Wizard tracking issue: #35645)

PubkeyProvider is the existing internal representation of a parsed key expression. This PR exposes its interface in descriptor.h and adds a public ParsePubkey() overload that returns the providers produced by the existing parser.

Key changes

public overload uses P2TR key rules, permitting compressed, x-only, extended, and MuSig key expressions while rejecting uncompressed keys.
Add GetOriginInfo() for retrieving explicitly provided key origin information.

Also, while I'm here, I noticed that musig parser accepted trailing characters apart from /.

Add a fix & test to reject unexpected trailing characters after a musig() expression

DrahtBot added the label Descriptors on Aug 6, 2026

DrahtBot commented at 6:44 PM on August 6, 2026: contributor

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

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/35918
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/35918.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/35918#author-ralph

Reviews

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

Type	Reviewers
ACK	w0xlt
Concept ACK	pablomartin4btc

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:

#36648 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36648.svg"></sub> (descriptor, rpc, wallet: do not echo key material in error messages by fametrano)
#36122 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36122.svg"></sub> (BIP460: CISA for Taproot key path spends by fjahr)

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.

DrahtBot closed this on Aug 7, 2026

DrahtBot reopened this on Aug 7, 2026

DrahtBot added the label Needs rebase on Aug 11, 2026

pseudoramdom force-pushed on Aug 11, 2026

pseudoramdom marked this as ready for review on Aug 11, 2026

DrahtBot removed the label Needs rebase on Aug 11, 2026

w0xlt commented at 8:32 PM on August 13, 2026: contributor

ACK 93600f9f0360bf48a1f616f0f5db3c9ce985ce4b

DrahtBot added the label Needs rebase on Aug 24, 2026

in src/script/descriptor.cpp:2871 in 93600f9f03 outdated

2866 | +std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkey(std::string_view key_expression, FlatSigningProvider& out, std::string& error)
2867 | +{
2868 | +    uint32_t key_exp_index{0};
2869 | +    const std::span<const char> expression{key_expression};
2870 | +    return ParsePubkey(key_exp_index, expression, ParseScriptContext::P2TR, out, error);
2871 | +}

uqlidi commented at 5:05 PM on August 28, 2026:

std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkey(std::string_view key_expression, ParseScriptContext ctx, FlatSigningProvider& out, std::string& error)
{
    uint32_t key_exp_index{0};
    const std::span<const char> expression{key_expression};
    return ParsePubkey(key_exp_index, expression, ctx, out, error);
}

I see you hardcoded it for p2tr. shouldn't be for all script types? Why are you making the overload follow the p2tr rules only?

sedited commented at 7:02 AM on October 13, 2026:

@pseudoramdom can you follow up here?

pseudoramdom commented at 7:30 AM on October 14, 2026:

Using P2TR here is intentional, as mentioned in the PR description. Its key parsing rules match what we want for standalone key expressions: accept compressed, x-only, extended, and musig() expressions, while rejecting uncompressed keys.

sedited commented at 5:53 AM on October 14, 2026: contributor

Ping for rebase @pseudoramdom

descriptor: Reject trailing text after musig()

The musig() parser split the input at the closing parenthesis without
validating the remaining text. This allowed unexpected characters after
the expression to be ignored.
Add a requirement for nonempty char after musig() to begin with '/'.

7835e0ef70

descriptor: move PubkeyProvider to descriptor.h

Expose the abstract interface by moving to descriptor.h to allow other
modules to consume parsed key expressions.
Mostly a move-only commit. Implementation of operator< is retained in
descriptor.cpp

c17a6ab8b7

descriptor: Add public ParsePubKey overload

Add an overload for parsing standalone key expressions using P2TR key
rules.
Also, expose explicitly provided origin information through GetOriginInfo(),
allowing callers to inspect the fingerprint and derivation path without
re-parsing the expression.

17ddfdc0ed

pseudoramdom force-pushed on Oct 14, 2026

pseudoramdom commented at 7:35 AM on October 14, 2026: contributor

Rebased & force-pushed to https://github.com/bitcoin/bitcoin/pull/35918/commits/17ddfdc0edc3ac35b1a87c884d61db2a58941bce. No other changes.

DrahtBot removed the label Needs rebase on Oct 14, 2026

fametrano commented at 6:25 AM on October 18, 2026: none

The first commit, 7835e0ef70, fixes a bug I also found on v31.1: tr(musig(X)xyz) is accepted and xyz is dropped. The worse case is a typo such as tr(musig(X)0/*), missing the /, which v31.1 turns into a non-ranged tr(musig(X)) without an error. Since it is a parser fix and the rest of the PR adds a new API, would you split it into its own PR? It could then be reviewed and backported on its own.

pseudoramdom requested review from w0xlt on Oct 20, 2026

w0xlt commented at 6:05 PM on October 20, 2026: contributor

reACK 17ddfdc0edc3ac35b1a87c884d61db2a58941bce

in src/script/descriptor.h:169 in 17ddfdc0ed

 164 | +
 165 | +protected:
 166 | +    static bool DetermineApostropheUse(StringType type, bool normalized, bool public_apostrophe);
 167 | +};
 168 | +
 169 | +/** Parse a key expression using P2TR key rules.

pablomartin4btc commented at 10:37 PM on October 30, 2026:

nit: "P2TR key rules" may not be obvious to callers, so perhaps spell out what it means, and that multipath returns more than one provider (?):

/** Parse a key expression using P2TR key rules: compressed, x-only, extended and
* musig() key expressions are accepted, uncompressed keys are rejected, and
* WIF private keys are serialized as x-only.

in src/script/descriptor.h:150 in 17ddfdc0ed

 145 | +    /** Return the non-extended public key for this PubkeyProvider, if it has one. */
 146 | +    virtual std::optional<CPubKey> GetRootPubKey() const = 0;
 147 | +    /** Return the extended public key for this PubkeyProvider, if it has one. */
 148 | +    virtual std::optional<CExtPubKey> GetRootExtPubKey() const = 0;
 149 | +
 150 | +    /** Return explicitly provided key origin information, if any. */

pablomartin4btc commented at 10:46 PM on October 30, 2026:

nit: maybe worth noting that it only covers the key's own origin, e.g. a musig() returns none even when its participants have one (there's no test for this yet; I'll suggest one later with some other test cases):

    /** Return explicitly provided key origin information, if any. For musig(), this
     *  does not include its participants' origins. */

pablomartin4btc commented at 2:57 AM on October 31, 2026: member

Concept ACK

A couple of nits in commits:

In 2nd commit (c17a6ab) message body, it mentions "Mostly a move-only commit. Implementation of operator< is retained in descriptor.cpp" but there's no operator< in descriptor.cpp or descriptor.h. What remains in the .cpp are the implementations of HavePrivateKeys() and DetermineApostropheUse(). Perhaps a leftover of a previous version of this change?
In 3rd commit (17ddfdc), tiny nit: the title specifies ParsePubKey but the function name is ParsePubkey.

Please keep in mind that #36442 changes the internal ParsePubkey to return a single provider for multipath expressions, so the new public overload may need to follow, depending on which one gets merged first.

I left another couple of non-blocking nits inline.