wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet

achow101 commented at 10:22 pm on May 13, 2025: member

Currently, if a user wants to use an airgapped setup, they need to manually create the watchonly wallet that will live on the online node by importing the public descriptors. This PR introduces exportwatchonlywallet which will create a wallet file with the public descriptors to avoid exposing the specific internals to the user. Additionally, this RPC will copy any existing labels, transactions, and wallet flags. This ensures that the exported watchonly wallet is almost entirely a copy of the original wallet but without private keys.

DrahtBot commented at 10:22 pm on May 13, 2025: 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/32489.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pablomartin4btc
Concept ACK	shahsb, Sjors, vicjuma, Eunovo, rkrux

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33043 ([POC] wallet: Enable non-electronic (paper-based) wallet backup with codex32 by w0xlt)
#33034 (wallet: Store transactions in a separate sqlite table by achow101)
#33008 (wallet: support bip388 policy with external signer by Sjors)
#32895 (wallet: Prepare for future upgrades by recording versions of last client to open and decrypt by achow101)
#32861 (Have createwalletdescriptor auto-detect an unused(KEY) by Sjors)
#32784 (wallet: derivehdkey RPC to get xpub at arbitrary path by Sjors)
#32724 (Musig2 tests by w0xlt)
#32652 (wallet: add codex32 argument to addhdkey by roconnor-blockstream)
#32471 (wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key by Eunovo)
#31244 (descriptors: MuSig2 by achow101)
#29675 (wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys by achow101)
#29136 (wallet: addhdkey RPC to add just keys to wallets via new unused(KEY) descriptor by achow101)

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 added the label Wallet on May 13, 2025

achow101 force-pushed on May 13, 2025

DrahtBot added the label CI failed on May 13, 2025

DrahtBot commented at 10:39 pm on May 13, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/42170822821 LLM reason (✨ experimental): The CI failure is due to linting errors in Python and C++ code.

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.

achow101 force-pushed on May 13, 2025

achow101 force-pushed on May 14, 2025

DrahtBot removed the label CI failed on May 14, 2025

shahsb commented at 2:30 pm on May 15, 2025: none

Concept ACK

DrahtBot added the label Needs rebase on May 16, 2025

achow101 force-pushed on May 16, 2025

DrahtBot removed the label Needs rebase on May 16, 2025

Sjors commented at 12:50 pm on May 19, 2025: member

Concept ACK

This might also be useful in a multisig airgapped scenario (where two private keys never go near each other).

E.g. create a 2-of-2 MuSig2 wallet on your offline machine, with one hot key held by Bitcoin Core and the other a hardware wallet. Export the watch-only wallet and put it on the online machine. You can then use the hardware wallet on either the online or offline machine to co-sign.

Though for that to work, the exportwatchonlywallet should have an option to set the external_signer flag (the offline wallet might not have this flag, depending on how you set it up).

DrahtBot added the label Needs rebase on May 19, 2025

achow101 force-pushed on May 20, 2025

DrahtBot removed the label Needs rebase on May 20, 2025

DrahtBot added the label Needs rebase on May 21, 2025

achow101 force-pushed on May 21, 2025

achow101 marked this as ready for review on May 21, 2025

achow101 commented at 5:54 pm on May 21, 2025: member

Rebased, ready for review

DrahtBot removed the label Needs rebase on May 21, 2025

in src/wallet/wallet.cpp:2634 in 129fdf7b9b outdated

2630+
2631 bool CWallet::LockCoin(const COutPoint& output, WalletBatch* batch)
2632 {
2633     AssertLockHeld(cs_wallet);
2634-    setLockedCoins.insert(output);
2635+    LoadLockedCoin(output, batch != nullptr);

Sjors commented at 8:14 am on May 22, 2025:

In 129fdf7b9b7f52dc6e41f3562fd9d8601f75792c “wallet: Track whether a locked coin is persisted”: this seems a bit cryptic. At the interface level we have lockCoin() which has a write_to_db argument, which determines whether the function here is called with a batch. And then here we have to translate that again to the persist bool of LoadLockedCoin.

Maybe LockCoin could have an explicit persist bool and batch as the third optional argument?

achow101 commented at 9:20 pm on May 22, 2025:

I removed the batch argument is having the caller provide that is entirely unnecessary. It’s been replaced with bool persist in LockCoin, and no extra argument is needed for UnlockCoin.

Also split into #32593

in src/wallet/rpc/util.cpp:112 in c61c87a453 outdated

108@@ -109,7 +109,10 @@ void PushParentDescriptors(const CWallet& wallet, const CScript& script_pubkey,
109 {
110     UniValue parent_descs(UniValue::VARR);
111     for (const auto& desc: wallet.GetWalletDescriptors(script_pubkey)) {
112-        parent_descs.push_back(desc.descriptor->ToString());

Sjors commented at 8:23 am on May 22, 2025:

In c61c87a453c1e5c1ad2f7d9c5b0e04c2d6df87ec “wallet, rpc: Push the normalized parent descriptor”: typo in the commit message “prividing”.

I’m a bit surprised such a change doesn’t break a test.

Would also be good to explain why this needs to happen.

achow101 commented at 8:28 pm on May 22, 2025:

We don’t have tests that check that the resulting parent descriptors exactly match, just that they are valid parent descriptors.

achow101 commented at 9:20 pm on May 22, 2025:

Added a test.

in src/wallet/scriptpubkeyman.cpp:1176 in 52583d3269 outdated

1173@@ -1174,7 +1174,7 @@ bool DescriptorScriptPubKeyMan::CanGetAddresses(bool internal) const
1174     LOCK(cs_desc_man);
1175     return m_wallet_descriptor.descriptor->IsSingleType() &&
1176            m_wallet_descriptor.descriptor->IsRange() &&
1177-           (HavePrivateKeys() || m_wallet_descriptor.next_index < m_wallet_descriptor.range_end);
1178+           (HavePrivateKeys() || m_wallet_descriptor.next_index < m_wallet_descriptor.range_end || m_wallet_descriptor.descriptor->CanSelfExpand());

Sjors commented at 8:25 am on May 22, 2025:

In 52583d3269cc747ff58b6d0b8c213a1ac521aeb3 “wallet: Use Descriptor::CanSelfExpand() in CanGetAddresses()”: would be nice to have to test that illustrates a case where this matters

achow101 commented at 8:26 pm on May 22, 2025:

This PR does that.

Without this PR, that test would not be possible as it is inherently contradictory. We want to test that we are able to get addresses from something for which we are not able to get addresses for in a watchonly wallet. The only way a watchonly wallet could have those addresses are if it had private keys and derived the keys already. But a watchonly wallet cannot have private keys. This situation is literally impossible to test prior to this PR.

The only reason such a test works with this PR is because we can bring along the descriptor caches when making the watchonly wallet, so the pubkeys are already derived.

Sjors commented at 8:30 am on May 22, 2025: member

Some initial observations on the early commits inline…

Those are primarily bug fixes for existing issues that came up during testing.

It’s a bit hard to follow, so I would suggest splitting (some of) them out. Unless they’re actual preparation like cb43639babdc961fda5b0b2e3426fe6dcd4b7cc6.

achow101 force-pushed on May 22, 2025

achow101 commented at 9:21 pm on May 22, 2025: member

Split the LockCoin commit into #32593, the parent_descs to #32594, the flag setting and retrieval stuff to #32597, and exception logging to #32598

achow101 marked this as a draft on May 22, 2025

achow101 force-pushed on May 22, 2025

achow101 force-pushed on May 23, 2025

Sjors commented at 7:52 am on May 23, 2025: member

Split the LockCoin commit into #3259,

#32593

vicjuma commented at 7:06 pm on May 28, 2025: none

Concept ACK

This separates viewing and spending logic and will be efficient especially if exporting it to an easily importable format

fanquake referenced this in commit 4b1d48a686 on May 30, 2025

achow101 force-pushed on Jun 2, 2025

achow101 force-pushed on Jun 4, 2025

glozow referenced this in commit 287cd04a32 on Jun 13, 2025

achow101 force-pushed on Jun 14, 2025

Sjors commented at 11:27 am on June 16, 2025: member

Can you update the description with the prerequisite PRs? Just #32593 and #32597 are left I believe.

in test/functional/wallet_exported_watchonly.py:226 in ab7e1f248d outdated

221+        addr = offline_wallet.getnewaddress()
222+        self.funds.sendtoaddress(addr, 1)
223+        self.generate(self.online, 1)
224+        # Spend funds in order to mark addr as previously spent
225+        offline_wallet.sendall([offline_wallet.getnewaddress()])
226+        self.funds.sendtoaddress(addr, 1)

Eunovo commented at 11:59 am on June 17, 2025:

https://github.com/bitcoin/bitcoin/pull/32489/commits/ab7e1f248d2588472029514a4a643f043c853f9b: This test does not fail when https://github.com/bitcoin/bitcoin/blob/ab7e1f248d2588472029514a4a643f043c853f9b/src/wallet/wallet.cpp#L4608 is commented out

The TX on line 225 will not be in node0’s mempool before generate is called on line 227. The tx will remain in mempool when the avoidreuse wallet is exported. avoidreuse_watchonly on the online node will call requestMempoolTransactions, which will cause the 225’s tx to be added to the wallet again and subsequently set previously_spent, whether ExportWatchOnlyWallet correctly writes previously_spent data or not

Adding a self.sync_mempools() statement before generating the new block seems to fix this.

achow101 commented at 6:15 pm on June 17, 2025:

Done

in test/functional/wallet_exported_watchonly.py:225 in ab7e1f248d outdated

220+        self.connect_nodes(0, 1)
221+        addr = offline_wallet.getnewaddress()
222+        self.funds.sendtoaddress(addr, 1)
223+        self.generate(self.online, 1)
224+        # Spend funds in order to mark addr as previously spent
225+        offline_wallet.sendall([offline_wallet.getnewaddress()])

Eunovo commented at 12:18 pm on June 17, 2025:

conssider using offline_wallet.sendall([self.funds.getnewaddress()]) instead

Using an offline_wallet address here causes this test to fail intermittently because it creates a new UTXO in offline_wallet, causing the UTXO of interest in this test to be at index 1 instead of 0. This causes offline_wallet.listunspent()[0]['reused'] to sometimes be False

achow101 commented at 6:15 pm on June 17, 2025:

Done

Eunovo commented at 12:20 pm on June 17, 2025: contributor

Concept ACK

Left some comments.

achow101 force-pushed on Jun 17, 2025

glozow referenced this in commit 8578fabb95 on Jun 25, 2025

DrahtBot added the label Needs rebase on Jul 1, 2025

achow101 force-pushed on Jul 1, 2025

DrahtBot removed the label Needs rebase on Jul 1, 2025

glozow referenced this in commit 5ad79b2035 on Jul 24, 2025

descriptor: Add CanSelfExpand()

CanSelfExpand() reports whether a descriptor can be expanded without
needing any caches or private keys to be provided by the caller of
Expand().

01b72fd4f5

wallet: Use Descriptor::CanSelfExpand() in CanGetAddresses()

If a descriptor does not need any caches or private keys in order to
expand, then CanGetAddresses() should return true for that descriptor.

873e40dd76

wallet: Write new descriptor's cache in AddWalletDescriptor

If a new WalletDescriptor is provided to us with a cache, write the
cache to disk as well.

40cf720462

wallet: Move listdescriptors retrieving from RPC to CWallet

When listdescriptors retrieves the descriptors from the wallet, instead
of having this logic in the RPC, move it into CWallet itself. This
will enable other functions to get the descriptors in an exportable
form.

24d7332b2b

wallet: Add CWallet::ExportWatchOnly

ExportWatchOnly produces a watchonly wallet file from a CWallet. This
can be restored onto another instance of Bitcoin Core to allow that
instance to watch the same descriptors, and also have all of the same
initial address book and transactions.

d222bc16e3

wallet, rpc: Add exportwatchonlywallet RPC 4c5635f2dc

test: Test for exportwatchonlywallet e6ac2015a8

achow101 force-pushed on Jul 24, 2025

achow101 marked this as ready for review on Jul 24, 2025

achow101 commented at 6:22 pm on July 24, 2025: member

All prerequisite PRs merged, ready for review.

rkrux commented at 3:59 am on July 25, 2025: contributor

Strong Concept ACK e6ac2015a852caa0f50e44becf9cd4b7592c48e7 Very useful for the users, the GUI option (in the separate PR) makes creating a watch-only version even easier. I will review this PR soon.

I believe the following is done now and can be removed from the PR description?

Some of the first several commits can be split into separate PRs if so desired. Those are primarily bug fixes for existing issues that came up during testing.

achow101 commented at 5:03 am on July 25, 2025: member

I believe the following is done now and can be removed from the PR description?

Yes, removed.

in src/wallet/wallet.cpp:3712 in e6ac2015a8

3705@@ -3706,6 +3706,10 @@ util::Result<std::reference_wrapper<DescriptorScriptPubKeyMan>> CWallet::AddWall
3706         // Save the descriptor to memory
3707         uint256 id = new_spk_man->GetID();
3708         AddScriptPubKeyMan(id, std::move(new_spk_man));
3709+
3710+        // Write the existing cache to disk
3711+        WalletBatch batch(GetDatabase());
3712+        batch.WriteDescriptorCacheItems(id, desc.cache);

pablomartin4btc commented at 6:36 am on July 25, 2025:

out of curiosity… why do we need this here now? what’s the issue if we don’t (currently in master)? could we not use UpgradeDescriptorCache() from an upper level?

pablomartin4btc commented at 6:40 am on July 25, 2025: member

light cr ACK e6ac2015a852caa0f50e44becf9cd4b7592c48e7

I like the refactoring in moving the logic of extracting descriptors info out of listdescriptors RPC to CWallet itself.

I’ll continue reviewing and will test it soon.

Left a couple of comments.

DrahtBot requested review from Eunovo on Jul 25, 2025

DrahtBot requested review from Sjors on Jul 25, 2025

DrahtBot requested review from shahsb on Jul 25, 2025

DrahtBot requested review from rkrux on Jul 25, 2025

wallet: Add exportwatchonlywallet RPC to export a watchonly version of a wallet #32489

Code Coverage & Benchmarks

Reviews

Conflicts

wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet #32489