wallet: Allow importing of descriptors without private keys when the wallet has the private keys #35377

achow101 commented at 11:55 PM on May 25, 2026: member

Currently importing a descriptor to a wallet that has private keys requires the descriptor to include the private keys. This is not ideal as it means exposing private key material. This PR makes it so that the wallet will lookup and substitute the private keys for their respective public keys in such descriptors, thus enabling importing of public descriptors into wallets with private keys.

The underlying mechanism is that the wallet retrieves all of the pubkeys from the descriptor and checks to see if any of them have private keys in any ScriptPubKeyMan. Additionally, if the descriptor has a xpub with key origin info, we will check if any xprvs known to the wallet have a matching fingerprint and derive to the specified xpub. If so, the origin + xpub are replaced with the single xprv with the origin derivation path prepended to the key expression's derivation path.

Possible future work is to allow the xpub substitution to work when the wallet has some child in the key origin, i.e. the wallet xprv has a key origin that is a prefix of the key origin specified for an xpub in the descriptor. Currently this kind of substitution is not being done, only root master xprvs will be substituted.

If a wallet does not have the private keys for a public descriptor, the import is still disallowed.

This PR is based on #34861 to avoid an annoying rebase.

Closes #27336

DrahtBot added the label Wallet on May 25, 2026

DrahtBot commented at 11:55 PM on May 25, 2026: contributor

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

Code Coverage & Benchmarks

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

Reviews

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

Type	Reviewers
Concept ACK	rkrux, polespinasa, Sjors

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:

#36257 (qa: assert_equals -> assert_true/assert_false by hodlinator)
#36236 (wallet, rpc: add verify_balance option to importdescriptors by musaHaruna)
#34520 (refactor: Add [[nodiscard]] to functions returning bool+mutable ref by maflcko)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

// Whether ' or h is used in harded derivation -> // Whether ' or h is used in hardened derivation [“harded” is misspelled]
self.log.info("Test import of descriptors without private keys that the wallet has the privkeys for") -> self.log.info("Test import of descriptors without private keys that the wallet has the private keys for") [“privkeys” is unclear/abbreviated in a way that may hinder comprehension]
# xpub substition -> # xpub substitution [“substition” is misspelled]

2026-09-30 18:06:49

DrahtBot added the label Needs rebase on May 26, 2026

achow101 force-pushed on May 26, 2026

DrahtBot removed the label Needs rebase on May 26, 2026

rkrux commented at 10:28 AM on June 5, 2026: contributor

Definite Concept ACK 2e6d8d0 because it allows the users to not deal with private keys manually.

polespinasa commented at 10:15 AM on June 8, 2026: member

seems like a good idea, concept ACK

Will review after #34861 is merged

DrahtBot added the label Needs rebase on Jun 12, 2026

achow101 force-pushed on Jun 13, 2026

DrahtBot added the label CI failed on Jun 13, 2026

DrahtBot commented at 3:24 AM on June 13, 2026: contributor

🚧 At least one of the CI tasks failed. Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/27453755862/job/81154189319 LLM reason (✨ experimental): CI failed because IWYU reported a header include issue (it modified src/script/descriptor.h and deliberately exited with “Failure generated from IWYU”).

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

DrahtBot removed the label Needs rebase on Jun 13, 2026

achow101 force-pushed on Jun 19, 2026

DrahtBot removed the label CI failed on Jun 19, 2026

DrahtBot added the label Needs rebase on Jun 26, 2026

achow101 force-pushed on Jun 27, 2026

DrahtBot removed the label Needs rebase on Jun 27, 2026

DrahtBot added the label Needs rebase on Jul 3, 2026

achow101 force-pushed on Jul 8, 2026

DrahtBot removed the label Needs rebase on Jul 8, 2026

DrahtBot added the label Needs rebase on Jul 14, 2026

achow101 force-pushed on Jul 31, 2026

achow101 force-pushed on Aug 5, 2026

DrahtBot removed the label Needs rebase on Aug 6, 2026

DrahtBot added the label CI failed on Aug 6, 2026

DrahtBot commented at 1:23 AM on August 6, 2026: contributor

🚧 At least one of the CI tasks failed. Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/31056225146/job/92474198450 LLM reason (✨ experimental): CI failed due to a C++ build error in src/script/descriptor.cpp (invalid access to MuSigPubkeyProvider::m_participants plus a related std::equal compilation error).

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

achow101 force-pushed on Aug 8, 2026

DrahtBot removed the label CI failed on Aug 8, 2026

DrahtBot added the label Needs rebase on Aug 11, 2026

achow101 force-pushed on Aug 26, 2026

DrahtBot removed the label Needs rebase on Aug 26, 2026

Sjors commented at 6:24 PM on August 31, 2026: member

Concept ACK, I've been using some version of this in https://github.com/Sjors/bitcoin/pull/91. I'll try to switch to exactly this PR after #34861 lands and you've rebased.

Sjors commented at 6:00 PM on September 2, 2026: member

An alternative approach, using bits of #36133, is to give Parse access to the wallet's extended keys: 2026/09/parse-known-keys (2 commits)

Although it works fine when you give it all keys, I further simplified it by only considering hd keys.

DrahtBot added the label Needs rebase on Sep 14, 2026

achow101 force-pushed on Sep 14, 2026

DrahtBot removed the label Needs rebase on Sep 14, 2026

DrahtBot added the label Needs rebase on Sep 22, 2026

achow101 force-pushed on Sep 22, 2026

achow101 marked this as ready for review on Sep 22, 2026

DrahtBot removed the label Needs rebase on Sep 22, 2026

rxbryan referenced this in commit feb2f59090 on Sep 24, 2026

rxbryan referenced this in commit 5f0da0f148 on Sep 24, 2026

rxbryan referenced this in commit 2803e1518b on Sep 25, 2026

Sjors referenced this in commit e515bef2a5 on Sep 28, 2026

in src/script/descriptor.cpp:1193 in e59a2f2070

1188 | +            std::unique_ptr<PubkeyProvider> sub_prov;
1189 | +            for (const auto& [xpub, xprv] : xprvs) {
1190 | +                const CKeyID& id = xpub.pubkey.GetID();
1191 | +                unsigned char fingerprint[4];
1192 | +                std::copy(id.begin(), id.begin() + 4, fingerprint);
1193 | +                if (!std::ranges::equal(fingerprint, origin_pub->m_origin.fingerprint)) continue;

polespinasa commented at 11:36 AM on September 30, 2026:

in e59a2f20702103562cfc6fe574d5470f496c88c2 descriptor: Implement SubstituteMasterExtPubs

I think this should work and would be cleaner?

if (xpub.id_key_fingerprint() != origin_pub->m_origin.fingerprint) continue;

achow101 commented at 6:06 PM on September 30, 2026:

Done

in src/script/descriptor.cpp:1200 in e59a2f2070 outdated

1195 | +                for (const auto& p : origin_pub->m_origin.path) {
1196 | +                    if (!derived.Derive(derived, p)) {
1197 | +                        break;
1198 | +                    }
1199 | +                }
1200 | +                if (derived.Neuter() != xpub_prov->m_root_extkey) continue;

polespinasa commented at 5:34 PM on September 30, 2026:

in e59a2f20702103562cfc6fe574d5470f496c88c2 descriptor: Implement SubstituteMasterExtPubs

Idk if what I am going to say makes sense but, shouldn't we only compare the pubkey? What if some metadata such as depth, fingerprint, etc. is missing (or is 0)? They key still a valid key but we would silently omit it. Probably this with Core descriptors will not happen, but should we assume other wallets might miss this data?

achow101 commented at 5:57 PM on September 30, 2026:

I think we want to be as specific as possible here to avoid any unexpected surprises. Given that changing any of the other fields in an xpub can drastically change how the xpub is encoded, I think it would be surprising to users if something very visually different to their private key magically gets a private key.

polespinasa commented at 5:45 PM on September 30, 2026: member

did a first review, looks pretty good, I have a concept question/suggestion, left a comment below :)

descriptor: Implement SubstituteMasterExtPubs

SubstituteMasterExtPubs replaces Origin + BIP32 inside of a descriptor
when a master xprv is provided that matches the origin and derives the
BIP32 xpub.

ad363cc57b

wallet: Substitute known keys when importing a descriptor

When an imported descriptor contains pubkeys for which the wallet knows
the private keys, substitute those pubkeys for the privkeys so that the
descriptor can be imported. This allows such descriptors without private keys
to be imported into wallets with private keys enabled.

48ef905c02

descriptor, musig: Return participants in GetPubKeys a8fdc5f21c

test: Test importdescriptors with descriptors without privkeys

Test that importdescriptors can import descriptors that don't have
private keys, but the wallet already has the private key for them.

b58383aca6

achow101 force-pushed on Sep 30, 2026

DrahtBot added the label CI failed on Sep 30, 2026

DrahtBot commented at 7:04 PM on September 30, 2026: contributor

🚧 At least one of the CI tasks failed. Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/36756207544/job/110026972644 LLM reason (✨ experimental): CI failed because the IWYU lint step (Fixing #includes) detected missing/incorrect includes and returned a failure (“Failure generated from IWYU”).

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

BenWestgate referenced this in commit 25e0c8dfc4 on Oct 3, 2026