wallet: Add importdescriptors interface by polespinasa · Pull Request #34861 · bitcoin/bitcoin

polespinasa · 2026-03-19T09:24:04Z

This PR adds an interface for importing descriptors.

The motivation behind this is that currently, importing descriptors is only possible via RPC. Bitcoin Core GUI doesn't use the RPC interface so it cannot offer descriptor import functionality, which is needed to support more complex wallet setups such as multisig.

This PR also adds a refactor by moving the importdescriptors logic from the RPC layer into CWallet::ImportDescriptor, making it reusable by both the RPC and this new interface.

The main changes are:

Introduces CWallet::ImportDescriptor() containing the core import logic, previously embedded in the RPC ProcessDescriptorImport function.
Introduces wallet::ImportDescriptorResult, a new result struct that carries success status, error message, warnings, and a FailureReason enum. The RPC layer uses FailureReason to map results back to the appropriate JSON-RPC error codes, keeping RPC concerns out of CWallet.
Updates ProcessDescriptorImport in rpc/backup.cpp to delegate to CWallet::ImportDescriptor.
Adds interfaces::Wallet::importDescriptors() as a new interface method, allowing the GUI to import descriptors without going through RPC.

I have a vibe coded PoC commit for how the GUI could consume it here: polespinasa@d1cb120
It might be useful to cherry-pick the commit to test the interface.

DrahtBot · 2026-03-19T09:24:26Z

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/34861.

Reviews

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

Type	Reviewers
Concept ACK	Sjors, sedited, stickies-v, rkrux, davidgumberg
Approach ACK	w0xlt
Stale ACK	xyzconstant

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35690 (wallet: Introduce WalletError with machine-readable error code by pseudoramdom)
#35579 (wallet: reserve walletrescan before checking wallet is at the tip by polespinasa)
#35493 (wallet, descriptor: Fix MuSig private key completeness checks on importdescriptors by w0xlt)
#35436 (wallet: Add addHDkey interface by pseudoramdom)
#35377 (wallet: Allow importing of descriptors without private keys when the wallet has the private keys by achow101)
#34681 (wallet: move rescan logic into ChainScanner and wallet/scan by Eunovo)
#34617 (fees: wallet: remove block policy fee estimator internals from wallet by ismaelsadeeq)
#34075 (fees: Introduce Mempool Based Fee Estimation to reduce overestimation by ismaelsadeeq)

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.

stickies-v · 2026-03-19T13:57:36Z

currently, importing descriptors is only possible via RPC. This makes it impossible for the GUI to offer descriptor import functionality,

Why can a GUI not use the RPC interface?

polespinasa · 2026-03-19T15:03:34Z

Why can a GUI not use the RPC interface?

Sorry, a GUI can. I ment Bitcoin Core GUI needs it because it doesn't use RPC.
Will rephrase to make it more clear.

Sjors · 2026-03-19T15:41:30Z

Concept ACK

We'll eventually want a way to import descriptors in the GUI, and moving this code out of src/wallet/rpc is a sensible refactor in any case.

Let's update the PR description to make it clear this isn't an IPC change. Until #10102
lands it's just an interface change, used internally.

sedited · 2026-03-19T15:49:20Z

Concept ACK

Makes sense to move this into the wallet interface, but this does indeed have nothing to do with ipc for now.

polespinasa · 2026-03-19T16:56:21Z

Updated the title and description to avoid confusion

stickies-v · 2026-03-20T09:52:14Z

Thanks for the clarification, Concept ACK.

w0xlt · 2026-03-22T08:08:59Z

Approach ACK

rkrux

Concept ACK b88462d

I have not reviewed the PR fully but only looked at one portion that was conspicuous in the diff.

davidgumberg · 2026-03-26T22:39:03Z

Concept ACK, this refactor is a nice win for maintainability, and makes adding importdescriptors to the GUI and/or to an IPC interface or any other interface trivial.

I could be mistaken as I haven't tried yet to make this change myself, but I think that the RPC code should consume the interface you are adding, because it deduplicates code and increases confidence that the interface added in this PR (which in the current branch is untested) works correctly since we reuse all of our functional tests on it.

If that's not feasible, the interface commit should probably be held off until the PR that introduces its consumer in the GUI.

achow101 · 2026-03-26T23:22:53Z

but I think that the RPC code should consume the interface you are adding

That is not how RPCs access the wallet, and making RPCs access the wallet through this interface is a pretty significant change that is massively outside of the scope of this PR. It would also mean that a bunch of things use the interface, while other things access CWallet directly, which is very weird.

I think the layout of the commits could be changed to make this easier to review. In general, making a new function that is mostly a copy of another function that will end up replacing the other function makes review more difficult as it does not allow git to show reviewers where things are moved. It also runs the risk of things that are added to the original function not making it into the copy when the PR is rebased, although this is less likely to be an issue for this PR. Since the difference between the 2 functions is primarily in the data types, I would suggest having a first commit that separates ImportDescriptor from ProcessDescriptorImport so that the type changes can be reviewed in that commit. Then having a second commit that moves ImportDescriptor to its new location so that the move diff is minimal.

Sjors · 2026-07-01T15:29:47Z

+        INVALID_DESCRIPTOR,
+        INVALID_PARAMETER,
+        WALLET_ERROR,
+        WALLET_UNLOCK_NEEDED,


In adf25dd wallet: Add ImportDescriptorRequest and ImportDescriptorResult structs: in #35436 there's been some back and forth on the use of enums. Commit 090edbb seems to be going in a better direction, introducing WalletErrorCode::WALLET_UNLOCK_NEEDED, which is worth considering here.

achow101 · 2026-07-07T21:48:12Z

+
+namespace wallet {
+
+struct ImportDescriptorResult {


In 28cce72 "wallet: Add ImportDescriptorRequest and ImportDescriptorResult structs"

Since the structs introduced here are used by the interface, they should live in wallet/types.h as that is where wallet internal structs that get passed across the interface live.

But also, as is being discussed in #35436, it would be preferable to avoid having single function specific structs and enums, especially for error codes.

polespinasa · 2026-07-09T12:09:02Z

@pseudoramdom
re #35436 (comment)

Some calls like importdescriptors have two types of behavior for errors, an error that throws the entire operation or a per-item error. What should we do with those? We need a way to tell the interfaces (rpc, etc) what to do with them. This does not affect this PR, but for #34861 was done is add a bool signaling per-item or not. That could be part of struct WalletError and default to true. Imho that is the easiest way to do it without adding extra complexity to the interfaces.

This sounds very importdescriptors specific request.
I think the best approach for that would be something like this?

using ImportDescriptorsResult = util::Expected<std::vector<ImportDescriptorResult>, WalletError>;

I am not sure, in the proposed code both items ImportDescriptorResult and WalletError can be Errors. And even contain same error codes.
I though the idea of using Expected was to have Expected<success, failure>, not Expected<(Success or failure), failure>

Introduce WalletError as a generic wallet-layer error type that can carry a machine-readable WalletErrorCode and a translated user-facing message. The generic `WALLET_ERROR` code is intended for failures that callers should only display to the user. More specific codes should only be added when callers can handle the condition differently.

Timestamp 0 is allowed and it does not seem a reason to have a minimum timestamp > 0 other than some carried value from legacy wallet. Lowering simplifies importdescriptors because, before this change, if a timestamp 0 was set by the user it would be overwritten by the minimum timestamp, but error messages should still report timestamp 0.

ImportDescriptorRquest will replace the UniValue-based interface of ProcessDescriptorImport(), which will allow to extract the functionality to other interfaces such as the GUI, etc, in a subsequent commit.

…ptorImport Adds a new function ProcessUniValueDescriptor that translates from UniValue to ImportDescriptorRequest. Refactors ProcessDescriptorImport to work with ImportDescriptorRequest objects instead of UniValue. Refactors RPCMethod importdescriptors() to use ProcessUniValueDescriptor.

ImportError struct will substitute the UniValue based response of ProcessDescriptorImport in a subsequent commit. This will allow other interfaces such as the GUI to consume those function. Also add new error codes to the WalletError codes enum, this is necessary in order to propagate error codes to the new interfaces and to translate errors to the rpc wallet error codes.

…orResult Make ProcessDescriptorImport return a ImportDescriptorResult object instead of a UniValue. This commit gets rid of a try-catch that generates a big diff of identation changes, it is recommended to review it with --ignore-space-change

polespinasa · 2026-07-09T23:31:01Z

Last force push https://github.com/bitcoin/bitcoin/compare/4b9379723e07c46266cc0f8c29bcb9befeed816e..4c076bd0ff55c46ceb21c1530c8ae2d61520cd7c

Rebased it on top of #35690

The location and names of the three new errors does not need to be there, I have just added them there temporary so we can discuss.

Also simplified a lot the result struct and removed the OK error code.
@pseudoramdom I started rebasing using util::Expected, but at some point I though this was easier because warnings are shared in both the success and the error case and did not want to duplicate structs.

DrahtBot · 2026-07-10T00:28:42Z

🚧 At least one of the CI tasks failed.
_{Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/29057319093/job/86251426488}
_{LLM reason (✨ experimental): CI failed because clang-tidy reported an error (bugprone-argument-comment) where a comment parameter name (is_general_error) didn’t match the function parameter (is_wallet_error) in wallet/imports.cpp, causing the job to exit non-zero.}

Hints

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.

…ocessDescriptorsImport Rename ProcessDescriptorImport() to ImportDescriptor(). ImportDescriptor() takes a single descriptor request and imports it into the wallet. Add ProcessDescriptorsImport() (descriptors in plural) to handle wallet locking and rescanning over a vector of ImportDescriptorRequest items. It is also in charge of collecting all responses of imports from ImportDescriptor. This allows a next commit to extract ImportDescriptor() and ProcessDescriptorsImport() out of the RPC code so they can be used by other future interfaces.

….cpp Extract ImportDescriptor() and ProcessDescriptorsImport() out of backup.cpp and move them into imports.cpp so other future interfaces can use them without needing to know about RPC code. The commit can be reviewed with the --color-moved=dimmed-zebra option for an easier review.

polespinasa · 2026-07-10T21:27:36Z

@achow101 re: #35690 (comment)

How do you translate them if we use the same INVALID_PARAMETER for different codes?

I would be okay with changing the error codes.

We consider those breaking change acceptable in this case?

DrahtBot · 2026-07-13T22:29:27Z

🐙 This pull request conflicts with the target branch and needs rebase.

polespinasa changed the title ~~wallet, refactor, ipc: Add importdescriptors IPC interface~~ wallet, ipc: Add importdescriptors IPC interface Mar 19, 2026

polespinasa force-pushed the 2026-03-19-importdescriptors branch from a3582a2 to 5e9dab4 Compare March 19, 2026 09:36

DrahtBot added CI failed and removed CI failed labels Mar 19, 2026

polespinasa changed the title ~~wallet, ipc: Add importdescriptors IPC interface~~ wallet, ipc: Add importdescriptors interface Mar 19, 2026

polespinasa changed the title ~~wallet, ipc: Add importdescriptors interface~~ wallet: Add importdescriptors interface Mar 19, 2026

DrahtBot added the Wallet label Mar 19, 2026

This was referenced Mar 19, 2026

wallet: move rescan logic into ChainScanner and wallet/scan #34681

Open

wallet: parallel fast rescan (approx 8x speed up with 8 threads) #34400

Draft

rxbryan reviewed Mar 22, 2026

View reviewed changes

Comment thread src/wallet/interfaces.cpp Outdated

polespinasa force-pushed the 2026-03-19-importdescriptors branch from 5e9dab4 to b88462d Compare March 22, 2026 22:01

rkrux suggested changes Mar 23, 2026

View reviewed changes

Comment thread src/wallet/wallet.cpp Outdated

achow101 mentioned this pull request Mar 24, 2026

wallet: Validate all descriptors before importing in importdescriptors #34903

Closed

3 tasks

davidgumberg reviewed Mar 26, 2026

View reviewed changes

Comment thread src/wallet/interfaces.cpp

achow101 reviewed Mar 26, 2026

View reviewed changes

Comment thread src/wallet/wallet.cpp Outdated

DrahtBot added CI failed and removed CI failed labels Jul 1, 2026

polespinasa mentioned this pull request Jul 1, 2026

test: Add importdescriptors rpc error test coverage #35630

Open

Sjors reviewed Jul 1, 2026

View reviewed changes

pseudoramdom mentioned this pull request Jul 2, 2026

Multisig Wizard tracking issue #35645

Open

11 tasks

DrahtBot added the Needs rebase label Jul 3, 2026

polespinasa force-pushed the 2026-03-19-importdescriptors branch from 65886aa to 4b93797 Compare July 3, 2026 11:45

DrahtBot removed the Needs rebase label Jul 3, 2026

achow101 reviewed Jul 7, 2026

View reviewed changes

pseudoramdom mentioned this pull request Jul 9, 2026

wallet: Introduce WalletError with machine-readable error code #35690

Merged

polespinasa added 7 commits July 9, 2026 19:33

wallet: rpc: Use std::optional in GetImportTimestamp

3b8653b

wallet: refactor: make is_ranged no longer an optional

51705a0

wallet: Add ImportDescriptorRequest structs

922e1c5

ImportDescriptorRquest will replace the UniValue-based interface of ProcessDescriptorImport(), which will allow to extract the functionality to other interfaces such as the GUI, etc, in a subsequent commit.

polespinasa force-pushed the 2026-03-19-importdescriptors branch from 4b93797 to 4c076bd Compare July 9, 2026 23:25

DrahtBot added the CI failed label Jul 10, 2026

polespinasa added 3 commits July 10, 2026 07:54

wallet: Add an importDescriptors() interface for the wallet

2c86272

This was referenced Jul 10, 2026

fees: wallet: remove block policy fee estimator internals from wallet #34617

Open

fees: Introduce Mempool Based Fee Estimation to reduce overestimation #34075

Open

Uh oh!

Conversation

polespinasa commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

stickies-v commented Mar 19, 2026

Uh oh!

polespinasa commented Mar 19, 2026

Uh oh!

Sjors commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sedited commented Mar 19, 2026

Uh oh!

polespinasa commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stickies-v commented Mar 20, 2026

Uh oh!

w0xlt commented Mar 22, 2026

Uh oh!

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davidgumberg commented Mar 26, 2026

Uh oh!

Uh oh!

achow101 commented Mar 26, 2026

Uh oh!

Uh oh!

Sjors Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

achow101 Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

polespinasa commented Jul 9, 2026

Uh oh!

polespinasa commented Jul 9, 2026

Uh oh!

DrahtBot commented Jul 10, 2026

Uh oh!

polespinasa commented Jul 10, 2026

Uh oh!

DrahtBot commented Jul 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

polespinasa commented Mar 19, 2026 •

edited

Loading

DrahtBot commented Mar 19, 2026 •

edited

Loading

Sjors commented Mar 19, 2026 •

edited

Loading

polespinasa commented Mar 19, 2026 •

edited

Loading