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.

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.

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
Concept ACK	shahsb, Sjors

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:

#bitcoin-core/gui/872 (Menu action to export a watchonly wallet by achow101)
#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)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#27286 (wallet: Keep track of the wallet’s own transaction outputs in memory 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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

“Error: Export " -> “Error: Export destination cannot be empty” [The error message literal is truncated, making the description of the expected error incomplete and confusing.]

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:1177 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

wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC

If the locked coin needs to be persisted to the wallet database,
insteead of having the RPC figure out when to create a WalletBatch and
having LockCoin's behavior depend on it, have LockCoin take whether to
persist as a parameter so it makes the batch.

Since unlocking a persisted locked coin requires a database write as
well, we need to track whether the locked coin was persisted to the
wallet database so that it can erase the locked coin when necessary.

Keeping track of whether a locked coin was persisted is also useful
information for future PRs.

3d36d5d441

wallet, rpc: Push the normalized parent descriptor

Instead of prividing the descriptor string as stored in the db, use the
normalized descriptor as is done for getaddressinfo's parent_desc field.

c38dff1ab6

test: Enable default wallet for wallet_descriptor.py 981f9153f7

test: Verify parent_desc in RPCs

getaddressinfo, listunspent, listtransactions, listsinceblock, and
gettransaction all include parent_desc(s). Make sure that these are
consistent with each other, as well as being in normalized form.

dba4cc118a

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

Split the LockCoin commit into #3259, 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

wallet: Set upgraded descriptor cache flag for newly created wallets

Although WalletBatch::LoadWallet performs the descriptor cache upgrade,
because new wallets do not have the descriptor flag set yet, the upgrade
does not run and set the flag.

Since new wallets will always being using the upgraded cache, there's no
reason to wait to set the flag, so set it when the wallet flags are
being initialized for new wallets.

d70285ae48

wallet: Add GetWalletFlags 5d7fb31e1d

wallet, rpc: Output wallet flags in getwalletinfo 353f13ee79

walletdb: Log additional exception error messages for corrupted wallets 6e44df7f27

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().

c73d06b059

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.

35194da585

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.

03ba95db1d

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.

a61edd54d2

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.

dbe6ab1fad

wallet, rpc: Add exportwatchonlywallet RPC 8e59d6f86c

test: Test for exportwatchonlywallet a7375a6ba1

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

Split the LockCoin commit into #3259,

#32593

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

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)

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