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.
wallet: Add exportwatchonlywallet
RPC to export a watchonly version of a wallet
#32489
pull
achow101
wants to merge
7
commits into
bitcoin:master
from
achow101:export-watchonly-wallet
changing
10
files
+598 −34
-
achow101 commented at 10:22 pm on May 13, 2025: memberCurrently, 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
-
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.
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 newunused(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 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: noneConcept 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 theexternal_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: memberRebased, 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 awrite_to_db
argument, which determines whether the function here is called with abatch
. And then here we have to translate that again to thepersist
bool ofLoadLockedCoin
.Maybe LockCoin could have an explicit
persist
bool and batch as the third optional argument?
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: memberSome 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, 2025achow101 marked this as a draft on May 22, 2025achow101 force-pushed on May 22, 2025achow101 force-pushed on May 23, 2025vicjuma commented at 7:06 pm on May 28, 2025: noneConcept 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, 2025achow101 force-pushed on Jun 2, 2025achow101 force-pushed on Jun 4, 2025glozow referenced this in commit 287cd04a32 on Jun 13, 2025achow101 force-pushed on Jun 14, 2025in 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 callrequestMempoolTransactions
, which will cause the 225’s tx to be added to the wallet again and subsequently setpreviously_spent
, whether ExportWatchOnlyWallet correctly writes previously_spent data or notAdding a
self.sync_mempools()
statement before generating the new block seems to fix this.
achow101 commented at 6:15 pm on June 17, 2025:Donein 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()])
insteadUsing an
offline_wallet
address here causes this test to fail intermittently because it creates a new UTXO inoffline_wallet
, causing the UTXO of interest in this test to be at index1
instead of0
. This causesoffline_wallet.listunspent()[0]['reused']
to sometimes beFalse
achow101 commented at 6:15 pm on June 17, 2025:DoneEunovo commented at 12:20 pm on June 17, 2025: contributorConcept ACK
Left some comments.
achow101 force-pushed on Jun 17, 2025glozow referenced this in commit 8578fabb95 on Jun 25, 2025DrahtBot added the label Needs rebase on Jul 1, 2025achow101 force-pushed on Jul 1, 2025DrahtBot removed the label Needs rebase on Jul 1, 2025glozow referenced this in commit 5ad79b2035 on Jul 24, 2025descriptor: 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().
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.
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.
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.
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.
wallet, rpc: Add exportwatchonlywallet RPC 4c5635f2dctest: Test for exportwatchonlywallet e6ac2015a8achow101 force-pushed on Jul 24, 2025achow101 marked this as ready for review on Jul 24, 2025achow101 commented at 6:22 pm on July 24, 2025: memberAll prerequisite PRs merged, ready for review.rkrux commented at 3:59 am on July 25, 2025: contributorStrong 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: memberI 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 inmaster
)? could we not useUpgradeDescriptorCache()
from an upper level?pablomartin4btc commented at 6:40 am on July 25, 2025: memberlight cr ACK e6ac2015a852caa0f50e44becf9cd4b7592c48e7
I like the refactoring in moving the logic of extracting descriptors info out of
listdescriptors
RPC toCWallet
itself.I’ll continue reviewing and will test it soon.
Left a couple of comments.
DrahtBot requested review from Eunovo on Jul 25, 2025DrahtBot requested review from Sjors on Jul 25, 2025DrahtBot requested review from shahsb on Jul 25, 2025DrahtBot requested review from rkrux on Jul 25, 2025
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-07-25 18:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me