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	rkrux
Concept ACK	shahsb, Sjors, vicjuma, Eunovo, stringintech, enirox001
Stale ACK	pablomartin4btc, ryanofsky

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:

#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)
#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)
#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:1179 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.

ryanofsky commented at 1:29 am on August 7, 2025:

re: #32489 (review)

In commit “wallet: Use Descriptor::CanSelfExpand() in CanGetAddresses()” (1170ade3a6d1fbb50ee2a83b906bb83560b0484d)

Can confirm without this change the test fails as expected with self.funds.sendtoaddress(online_wallet.getnewaddress(), 10) throwing “No addresses available”

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

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:3679 in e6ac2015a8 outdated

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?

achow101 commented at 6:49 pm on July 29, 2025:

The cache can only be generated if private keys are available, so we need this to write the cache to disk of the watchonly wallet.

ryanofsky commented at 1:32 am on August 7, 2025:

re: #32489 (review)

In commit “wallet: Write new descriptor’s cache in AddWalletDescriptor” (fcf7dfe08519aefd193d12958070bfc5ea52f090)

Can confirm if I revert this change I see a Wallet corrupted (-4) error from self.online.restorewallet("imports_watchonly", res["exported_file"]) running the test with Error: Unable to expand wallet descriptor from cache in the logs

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

in src/wallet/wallet.cpp:4485 in e6ac2015a8 outdated

4478@@ -4475,4 +4479,197 @@ std::optional<WalletTXO> CWallet::GetTXO(const COutPoint& outpoint) const
4479     }
4480     return it->second;
4481 }
4482+
4483+util::Result<std::vector<WalletDescInfo>> CWallet::ExportDescriptors(bool export_private) const

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

nit: this could be just GetDescriptors? you can list them or export them later?

achow101 commented at 6:50 pm on July 29, 2025:

I think export is a reasonable name since listing them is also an export.

in src/script/descriptor.cpp:1413 in 01b72fd4f5 outdated

1409@@ -1368,6 +1410,7 @@ class RawTRDescriptor final : public DescriptorImpl
1410     RawTRDescriptor(std::unique_ptr<PubkeyProvider> output_key) : DescriptorImpl(Vector(std::move(output_key)), "rawtr") {}
1411     std::optional<OutputType> GetOutputType() const override { return OutputType::BECH32M; }
1412     bool IsSingleType() const final { return true; }
1413+    bool CanSelfExpand() const override { return m_pubkey_args[0]->CanSelfExpand(); }

ryanofsky commented at 7:03 pm on July 31, 2025:

In commit “descriptor: Add CanSelfExpand()” (01b72fd4f5e5bcbbf851c217a895b09204a760a3)

I was wondering whether it’s possible to provide a generic implementation of CanSelfExpand() to avoid needing all these specializations. Would the following work?

  0--- a/src/script/descriptor.cpp
  1+++ b/src/script/descriptor.cpp
  2@@ -786,6 +786,16 @@ public:
  3         }
  4     }
  5 
  6+    bool CanSelfExpand() const override {
  7+        for (const auto& key : m_pubkey_args) {
  8+            if (!key->CanSelfExpand()) return false;
  9+        }
 10+        for (const auto& sub : m_subdescriptor_args) {
 11+            if (!sub->CanSelfExpand()) return false;
 12+        }
 13+        return true;
 14+    }
 15+
 16     virtual std::unique_ptr<DescriptorImpl> Clone() const = 0;
 17 };
 18 
 19@@ -806,7 +816,6 @@ public:
 20     }
 21     bool IsSingleType() const final { return true; }
 22     bool ToPrivateString(const SigningProvider& arg, std::string& out) const final { return false; }
 23-    bool CanSelfExpand() const final { return true; }
 24 
 25     std::optional<int64_t> ScriptSize() const override { return GetScriptForDestination(m_destination).size(); }
 26     std::unique_ptr<DescriptorImpl> Clone() const override
 27@@ -834,7 +843,6 @@ public:
 28     }
 29     bool IsSingleType() const final { return true; }
 30     bool ToPrivateString(const SigningProvider& arg, std::string& out) const final { return false; }
 31-    bool CanSelfExpand() const final { return true; }
 32 
 33     std::optional<int64_t> ScriptSize() const override { return m_script.size(); }
 34 
 35@@ -862,7 +870,6 @@ protected:
 36 public:
 37     PKDescriptor(std::unique_ptr<PubkeyProvider> prov, bool xonly = false) : DescriptorImpl(Vector(std::move(prov)), "pk"), m_xonly(xonly) {}
 38     bool IsSingleType() const final { return true; }
 39-    bool CanSelfExpand() const override { return m_pubkey_args[0]->CanSelfExpand(); }
 40 
 41     std::optional<int64_t> ScriptSize() const override {
 42         return 1 + (m_xonly ? 32 : m_pubkey_args[0]->GetSize()) + 1;
 43@@ -898,7 +905,6 @@ public:
 44     PKHDescriptor(std::unique_ptr<PubkeyProvider> prov) : DescriptorImpl(Vector(std::move(prov)), "pkh") {}
 45     std::optional<OutputType> GetOutputType() const override { return OutputType::LEGACY; }
 46     bool IsSingleType() const final { return true; }
 47-    bool CanSelfExpand() const override { return m_pubkey_args[0]->CanSelfExpand(); }
 48 
 49     std::optional<int64_t> ScriptSize() const override { return 1 + 1 + 1 + 20 + 1 + 1; }
 50 
 51@@ -932,7 +938,6 @@ public:
 52     WPKHDescriptor(std::unique_ptr<PubkeyProvider> prov) : DescriptorImpl(Vector(std::move(prov)), "wpkh") {}
 53     std::optional<OutputType> GetOutputType() const override { return OutputType::BECH32; }
 54     bool IsSingleType() const final { return true; }
 55-    bool CanSelfExpand() const override { return m_pubkey_args[0]->CanSelfExpand(); }
 56 
 57     std::optional<int64_t> ScriptSize() const override { return 1 + 1 + 20; }
 58 
 59@@ -974,7 +979,6 @@ protected:
 60 public:
 61     ComboDescriptor(std::unique_ptr<PubkeyProvider> prov) : DescriptorImpl(Vector(std::move(prov)), "combo") {}
 62     bool IsSingleType() const final { return false; }
 63-    bool CanSelfExpand() const override { return m_pubkey_args[0]->CanSelfExpand(); }
 64     std::unique_ptr<DescriptorImpl> Clone() const override
 65     {
 66         return std::make_unique<ComboDescriptor>(m_pubkey_args.at(0)->Clone());
 67@@ -999,13 +1003,6 @@ protected:
 68 public:
 69     MultisigDescriptor(int threshold, std::vector<std::unique_ptr<PubkeyProvider>> providers, bool sorted = false) : DescriptorImpl(std::move(providers), sorted ? "sortedmulti" : "multi"), m_threshold(threshold), m_sorted(sorted) {}
 70     bool IsSingleType() const final { return true; }
 71-    bool CanSelfExpand() const override {
 72-        bool can_expand = true;
 73-        for (const auto& key : m_pubkey_args) {
 74-            can_expand &= key->CanSelfExpand();
 75-        }
 76-        return can_expand;
 77-    }
 78 
 79     std::optional<int64_t> ScriptSize() const override {
 80         const auto n_keys = m_pubkey_args.size();
 81@@ -1057,13 +1054,6 @@ protected:
 82 public:
 83     MultiADescriptor(int threshold, std::vector<std::unique_ptr<PubkeyProvider>> providers, bool sorted = false) : DescriptorImpl(std::move(providers), sorted ? "sortedmulti_a" : "multi_a"), m_threshold(threshold), m_sorted(sorted) {}
 84     bool IsSingleType() const final { return true; }
 85-    bool CanSelfExpand() const override {
 86-        bool can_expand = true;
 87-        for (const auto& key : m_pubkey_args) {
 88-            can_expand &= key->CanSelfExpand();
 89-        }
 90-        return can_expand;
 91-    }
 92 
 93     std::optional<int64_t> ScriptSize() const override {
 94         const auto n_keys = m_pubkey_args.size();
 95@@ -1110,7 +1100,6 @@ public:
 96         return OutputType::LEGACY;
 97     }
 98     bool IsSingleType() const final { return true; }
 99-    bool CanSelfExpand() const override { return m_subdescriptor_args[0]->CanSelfExpand(); }
100 
101     std::optional<int64_t> ScriptSize() const override { return 1 + 1 + 20 + 1; }
102 
103@@ -1152,7 +1141,6 @@ public:
104     WSHDescriptor(std::unique_ptr<DescriptorImpl> desc) : DescriptorImpl({}, std::move(desc), "wsh") {}
105     std::optional<OutputType> GetOutputType() const override { return OutputType::BECH32; }
106     bool IsSingleType() const final { return true; }
107-    bool CanSelfExpand() const override { return m_subdescriptor_args[0]->CanSelfExpand(); }
108 
109     std::optional<int64_t> ScriptSize() const override { return 1 + 1 + 32; }
110 
111@@ -1230,13 +1218,6 @@ public:
112     }
113     std::optional<OutputType> GetOutputType() const override { return OutputType::BECH32M; }
114     bool IsSingleType() const final { return true; }
115-    bool CanSelfExpand() const override {
116-        bool can_expand = m_pubkey_args[0]->CanSelfExpand();
117-        for (const auto& sub : m_subdescriptor_args) {
118-            can_expand &= sub->CanSelfExpand();
119-        }
120-        return can_expand;
121-    }
122 
123     std::optional<int64_t> ScriptSize() const override { return 1 + 1 + 32; }
124 
125@@ -1364,13 +1345,6 @@ public:
126 
127     bool IsSolvable() const override { return true; }
128     bool IsSingleType() const final { return true; }
129-    bool CanSelfExpand() const override {
130-        bool can_expand = true;
131-        for (const auto& key : m_pubkey_args) {
132-            can_expand &= key->CanSelfExpand();
133-        }
134-        return can_expand;
135-    }
136 
137     std::optional<int64_t> ScriptSize() const override { return m_node->ScriptSize(); }
138 
139@@ -1410,7 +1384,6 @@ public:
140     RawTRDescriptor(std::unique_ptr<PubkeyProvider> output_key) : DescriptorImpl(Vector(std::move(output_key)), "rawtr") {}
141     std::optional<OutputType> GetOutputType() const override { return OutputType::BECH32M; }
142     bool IsSingleType() const final { return true; }
143-    bool CanSelfExpand() const override { return m_pubkey_args[0]->CanSelfExpand(); }
144 
145     std::optional<int64_t> ScriptSize() const override { return 1 + 1 + 32; }
146

achow101 commented at 11:23 pm on July 31, 2025:

Done

DrahtBot added the label Needs rebase on Jul 31, 2025

achow101 force-pushed on Jul 31, 2025

DrahtBot removed the label Needs rebase on Aug 1, 2025

DrahtBot added the label CI failed on Aug 1, 2025

DrahtBot commented at 1:24 am on August 1, 2025: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/runs/47161464649 LLM reason (✨ experimental): Clang-tidy detected a recursive function call in descriptor.cpp, which caused the CI failure.

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 Aug 1, 2025

achow101 force-pushed on Aug 2, 2025

DrahtBot removed the label CI failed on Aug 2, 2025

ryanofsky commented at 4:13 pm on August 4, 2025: contributor

For anyone who’s interested, I’m hosting a review club meeting about this PR on Wednesday: https://bitcoincore.reviews/32489

jonatack commented at 9:23 pm on August 4, 2025: member

For anyone who’s interested, I’m hosting a review club meeting about this PR on Wednesday: bitcoincore.reviews/32489

There is Review Club label in this repo that can be added, too.

ryanofsky added the label Review club on Aug 4, 2025

in src/wallet/rpc/wallet.cpp:855 in 84d3bc68cc outdated

850+{
851+    return RPCHelpMan{"exportwatchonlywallet",
852+        "Creates a wallet file at the specified path and name containing a watchonly version "
853+        "of the wallet. This watchonly wallet contains the wallet's public descriptors, "
854+        "its transactions, and address book data. The watchonly wallet can be imported to "
855+        "another node using 'restorewallet'.",

pablomartin4btc commented at 3:34 am on August 6, 2025:

nit:

0        "Creates a wallet file at the specified destination containing a watchonly version "
1        "of the current wallet. This watchonly wallet contains the wallet's public descriptors, "
2        "its transactions, and address book data. The watchonly wallet can be imported into "
3        "another node using 'restorewallet'.",

achow101 commented at 7:57 pm on August 14, 2025:

Done

pablomartin4btc commented at 3:39 am on August 6, 2025: member

re-ACK 84d3bc68cc4d23ab9f7a91a955cd0e26ffb1b6e4

Since my last review CanSelfExpand() has been refactored as @ryanofsky suggested.

We’d need release notes for this new RPC.

in src/wallet/wallet.cpp:4458 in 3c5cdae7a1 outdated

4453+    AssertLockHeld(cs_wallet);
4454+    std::vector<WalletDescInfo> wallet_descriptors;
4455+    for (const auto& spk_man : GetAllScriptPubKeyMans()) {
4456+        const auto desc_spk_man = dynamic_cast<DescriptorScriptPubKeyMan*>(spk_man);
4457+        if (!desc_spk_man) {
4458+            return util::Error{_("Unexpected ScriptPubKey manager type.")};

ryanofsky commented at 1:16 am on August 7, 2025:

In commit “wallet: Move listdescriptors retrieving from RPC to CWallet” (3c5cdae7a15cb046010d753b918b313423861582)

Since these are errors are pretty technical and obscure, seems like it probably makes sense to use Untranslated instead of _ to avoid these becoming translated strings.

achow101 commented at 7:57 pm on August 14, 2025:

Done

in src/wallet/wallet.cpp:4489 in 236970d25c outdated

4484+        return util::Error{_("Error: Export destination cannot be empty")};
4485+    }
4486+    if (fs::exists(destination)) {
4487+        return util::Error{strprintf(_("Error: Export destination '%s' already exists"), fs::PathToString(destination))};
4488+    }
4489+    fs::path canonical_dest = fs::canonical(destination.parent_path());

ryanofsky commented at 2:59 pm on August 7, 2025:

In commit “wallet: Add CWallet::ExportWatchOnly” (236970d25cc45b02678e410cfdc7e064da22cf11)

Calling fs::canonical on the destination parent path seems like a nice way of making sure the parent path exists and throwing an error early if it doesn’t. But it doesn’t handle other problems like the destination path not being writable. Also I’m wary of code that manipulates paths unnecessarily instead of passing them directly to the OS.

Not critical, but I’d suggest a change like the following to improve behavior and also simplify the function by eliminating the canonical_dest variable:

 0--- a/src/wallet/wallet.cpp
 1+++ b/src/wallet/wallet.cpp
 2@@ -19,6 +19,7 @@
 3 #include <consensus/consensus.h>
 4 #include <consensus/validation.h>
 5 #include <external_signer.h>
 6+#include <fstream>
 7 #include <interfaces/chain.h>
 8 #include <interfaces/handler.h>
 9 #include <interfaces/wallet.h>
10@@ -4486,8 +4487,10 @@ util::Result<std::string> CWallet::ExportWatchOnlyWallet(const fs::path& destina
11     if (fs::exists(destination)) {
12         return util::Error{strprintf(_("Error: Export destination '%s' already exists"), fs::PathToString(destination))};
13     }
14-    fs::path canonical_dest = fs::canonical(destination.parent_path());
15-    canonical_dest /= destination.filename();
16+    if (!std::ofstream{destination}) {
17+        return util::Error{strprintf(_("Error: Could not create file '%s'"), fs::PathToString(destination))};
18+    }
19+    fs::remove(destination);
20 
21     // Get the descriptors from this wallet
22     util::Result<std::vector<WalletDescInfo>> exported = ExportDescriptors(/*export_private=*/false);
23@@ -4630,7 +4633,7 @@ util::Result<std::string> CWallet::ExportWatchOnlyWallet(const fs::path& destina
24         }
25 
26         // Make a backup of this wallet at the specified destination directory
27-        watchonly_wallet->BackupWallet(fs::PathToString(canonical_dest));
28+        watchonly_wallet->BackupWallet(fs::PathToString(destination));
29     }
30 
31     // Delete the watchonly wallet now that it has been exported to the desired location
32@@ -4638,6 +4641,6 @@ util::Result<std::string> CWallet::ExportWatchOnlyWallet(const fs::path& destina
33     watchonly_wallet.reset();
34     fs::remove_all(watchonly_path);
35 
36-    return fs::PathToString(canonical_dest);
37+    return fs::PathToString(destination);
38 }
39 } // namespace wallet

achow101 commented at 7:57 pm on August 14, 2025:

Done as suggested

in src/wallet/wallet.cpp:4633 in 236970d25c outdated

4628+                return util::Error{_("Error: cannot commit db transaction for watchonly wallet export")};
4629+            }
4630+        }
4631+
4632+        // Make a backup of this wallet at the specified destination directory
4633+        watchonly_wallet->BackupWallet(fs::PathToString(canonical_dest));

ryanofsky commented at 3:02 pm on August 7, 2025:

In commit “wallet: Add CWallet::ExportWatchOnly” (236970d25cc45b02678e410cfdc7e064da22cf11)

No error is returned here if this fails. If I test with an unwritable path like bitcoin-cli exportwatchonlywallet /foo I see the RPC return success, but no backup file is created and SQLite Error. Code: 14. Message: os_unix.c:44906: (2) open(/foo) - No such file or directory is in the debug log.

Would be good to fix, and may also want to add [[nodiscard]] to the BackupWallet method to prevent problems like this.

achow101 commented at 7:57 pm on August 14, 2025:

Done

in src/wallet/wallet.cpp:4672 in 236970d25c outdated

4634+    }
4635+
4636+    // Delete the watchonly wallet now that it has been exported to the desired location
4637+    fs::path watchonly_path = fs::PathFromString(watchonly_wallet->GetDatabase().Filename()).parent_path();
4638+    watchonly_wallet.reset();
4639+    fs::remove_all(watchonly_path);

ryanofsky commented at 3:16 pm on August 7, 2025:

In commit “wallet: Add CWallet::ExportWatchOnly” (236970d25cc45b02678e410cfdc7e064da22cf11)

I don’t think fs::remove_all is appropriate here or elsewhere in wallet code. We should have a wallet delete function that explicitly removes known database and log files, then removes the directory, rather than recursively deleting everything. remove_all is fine in the happy case, but if there’s a bug (e.g. an extra parent_path call) or if users mistakenly store other data or bind mounts in the wallet directory, the results could be catastrophic. Seems fine to keep for now since there are other instances, but this would be good to address eventually.

achow101 commented at 7:57 pm on August 14, 2025:

I’ll leave that for a followup

in src/wallet/rpc/wallet.cpp:867 in 71e25ee9ad outdated

862+                {RPCResult::Type::STR, "exported_file", "The full path that the file has been exported to"},
863+            },
864+        },
865+        RPCExamples{
866+            HelpExampleCli("exportwatchonlywallet", "\"home\\user\\\"")
867+            + HelpExampleRpc("exportwatchonlywallet", "\"home\\user\\\"")

ryanofsky commented at 3:30 pm on August 7, 2025:

In commit “wallet, rpc: Add exportwatchonlywallet RPC” (71e25ee9ad52d8b9a763782b795690a196f218ef)

Seems like a mistake here, did you mean to write "\"home\\user\\backup.dat\""?

achow101 commented at 7:57 pm on August 14, 2025:

Done

in test/functional/wallet_exported_watchonly.py:45 in 84d3bc68cc outdated

40+        # Export the watchonly wallet file and load onto online node
41+        watchonly_export = os.path.join(self.export_path, "basic_watchonly.dat")
42+        res = offline_wallet.exportwatchonlywallet(watchonly_export)
43+        assert_equal(res["exported_file"], watchonly_export)
44+        self.online.restorewallet("basic_watchonly", res["exported_file"])
45+        online_wallet = self.online.get_wallet_rpc("basic_watchonly")

ryanofsky commented at 3:45 pm on August 7, 2025:

In commit “test: Test for exportwatchonlywallet” (84d3bc68cc4d23ab9f7a91a955cd0e26ffb1b6e4)

This chunk of code is repeated many times. Maybe factor out into an export_and_restore help method

achow101 commented at 7:57 pm on August 14, 2025:

Done

in test/functional/wallet_exported_watchonly.py:126 in 84d3bc68cc outdated

121+
122+        # In order to make transactions in the offline wallet, briefly connect offline to online
123+        self.connect_nodes(0, 1)
124+        txids = [self.funds.sendtoaddress(offline_wallet.getnewaddress("funds"), i) for i in range(1, 4)]
125+        self.generate(self.online, 1)
126+        self.disconnect_nodes(0 ,1)

ryanofsky commented at 3:48 pm on August 7, 2025:

In commit “test: Test for exportwatchonlywallet” (84d3bc68cc4d23ab9f7a91a955cd0e26ffb1b6e4)

Does this need a sync_mempools like the other case below to make sure offline wallet receives the funds?

Also would be nice here and other places to replace 0, 1 with self.online.index and self.offline.index

achow101 commented at 7:57 pm on August 14, 2025:

Done

ryanofsky approved

ryanofsky commented at 3:55 pm on August 7, 2025: contributor

Code review ACK 84d3bc68cc4d23ab9f7a91a955cd0e26ffb1b6e4. This is great. Nice feature cleanly implemented with very good test coverage. I left a bunch of comments but none are critical.

in src/wallet/wallet.cpp:4584 in 84d3bc68cc outdated

4542+            WalletDescriptor w_desc(std::move(descs.at(0)), desc_info.creation_time, range_start, range_end, desc_info.next_index);
4543+
4544+            // For descriptors that cannot self expand (i.e. needs private keys or cache), retrieve the cache
4545+            uint256 desc_id = w_desc.id;
4546+            if (!w_desc.descriptor->CanSelfExpand()) {
4547+                DescriptorScriptPubKeyMan* desc_spkm = dynamic_cast<DescriptorScriptPubKeyMan*>(GetScriptPubKeyMan(desc_id));

stringintech commented at 6:51 pm on August 9, 2025:

Perhaps dynamic_cast could be wrapped by CHECK_NONFATAL?

enirox001 commented at 5:07 pm on August 13, 2025:

Here GetScriptPubKeyMan(desc_id) we dynamic_cast to DescriptorScriptPubKeyMan* and use it directly. Would it be worth adding a null-check here to handle the case where desc_id is missing or the type is unexpected, to avoid a possible crash?

achow101 commented at 7:03 pm on August 14, 2025:

I don’t think it’s really useful to have a check on that since earlier functions already enforce that the retrieved ScriptPubKeyMan must be a DescriptorScriptPubKeyMan.

achow101 commented at 7:05 pm on August 14, 2025:

#32489 (review)

stringintech commented at 6:51 pm on August 9, 2025: contributor

Concept ACK. I did a light code review.

in src/wallet/wallet.cpp:3679 in 84d3bc68cc outdated

3672@@ -3673,6 +3673,10 @@ util::Result<std::reference_wrapper<DescriptorScriptPubKeyMan>> CWallet::AddWall
3673         // Save the descriptor to memory
3674         uint256 id = new_spk_man->GetID();
3675         AddScriptPubKeyMan(id, std::move(new_spk_man));
3676+
3677+        // Write the existing cache to disk
3678+        WalletBatch batch(GetDatabase());
3679+        batch.WriteDescriptorCacheItems(id, desc.cache);

enirox001 commented at 4:09 pm on August 13, 2025:

Small question: WalletBatch::WriteDescriptorCacheItems(…) returns a bool and can fail on DB errors — should we check the return and surface/propagate the error instead of ignoring it? That would avoid silently losing the cache on disk

achow101 commented at 7:57 pm on August 14, 2025:

Done

in src/wallet/wallet.cpp:4558 in 84d3bc68cc outdated

4517+    empty_context.args = context.args;
4518+    std::shared_ptr<CWallet> watchonly_wallet = CWallet::Create(empty_context, wallet_name, std::move(database), options.create_flags, error, warnings);
4519+    if (!watchonly_wallet) {
4520+        return util::Error{_("Error: Failed to create new watchonly wallet")};
4521+    }
4522+

enirox001 commented at 5:17 pm on August 13, 2025:

can we add a runtime sanity check here to ensure the newly created watch-only wallet does not have private keys enabled (i.e. !watchonly_wallet->HavePrivateKeys())? If the flag didn’t take effect we should fail early with a clear error rather than continuing.

achow101 commented at 7:43 pm on August 14, 2025:

I don’t think that’s necessary. It uses code that is well tested to behave correctly.

in src/wallet/rpc/wallet.cpp:852 in 84d3bc68cc outdated

845@@ -846,6 +846,46 @@ static RPCHelpMan createwalletdescriptor()
846     };
847 }
848 
849+static RPCHelpMan exportwatchonlywallet()
850+{
851+    return RPCHelpMan{"exportwatchonlywallet",
852+        "Creates a wallet file at the specified path and name containing a watchonly version "

enirox001 commented at 5:31 pm on August 13, 2025:

Small nit: could we reword the help text to be clearer and use “watch-only” (hyphenated) consistently?

Suggest: “Creates a wallet file at the specified destination containing a watch-only version of the current wallet.”

achow101 commented at 7:58 pm on August 14, 2025:

Done

enirox001 commented at 7:17 pm on August 13, 2025: contributor

Concept ACK 84d3bc6

Left some nits, comments

achow101 force-pushed on Aug 14, 2025

in src/wallet/wallet.cpp:4590 in 4cc7be4ed9 outdated

4585+
4586+            // Copy minversion
4587+            // Don't use SetMinVersion to account for the newly created wallet having FEATURE_LATEST
4588+            // while the source wallet doesn't.
4589+            watchonly_wallet->LoadMinVersion(GetVersion());
4590+            watchonly_batch.WriteMinVersion(watchonly_wallet->GetVersion());

pablomartin4btc commented at 7:46 pm on August 20, 2025:

These were all removed in #32977.

achow101 commented at 10:18 pm on August 20, 2025:

Removed

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

a3be2d8745

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.

9317ea0b1c

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.

4d7aeab150

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.

67216700ce

achow101 force-pushed on Aug 20, 2025

in test/functional/wallet_exported_watchonly.py:165 in cf588607fa outdated

163+
164+        # Make sure that the hardened derivation has some pregenerated keys
165+        offline_wallet.keypoolrefill(10)
166+
167+        # Export the watchonly wallet file and load onto online node
168+        online_wallet = self.export_and_restore(offline_wallet, "imports_watchonly")

rkrux commented at 1:39 pm on August 25, 2025:

In cf588607fa36964597a6d6acc853eeb26e51b0ea “test: Test for exportwatchonlywallet”

In this particular subtest, there are 5 inactive and 8 active descriptors in the listdescriptors RPC response after importing few descriptors. This subtest can also check that the active and inactive descriptors remain the same after export.

achow101 commented at 9:09 pm on August 25, 2025:

Done

in test/functional/wallet_exported_watchonly.py:185 in cf588607fa outdated

180+        self.generate(self.online, 1, sync_fun=self.no_op)
181+
182+        # The hardened derivation should have 9 remaining addresses
183+        for _ in range(9):
184+            online_wallet.getnewaddress(address_type="p2sh-segwit")
185+        assert_raises_rpc_error(-12, "No addresses available", online_wallet.getnewaddress, address_type="p2sh-segwit")

rkrux commented at 1:45 pm on August 25, 2025:

In cf588607fa36964597a6d6acc853eeb26e51b0ea “test: Test for exportwatchonlywallet”

This getnewaddress fails as expected because 10 pre-generated keys were there in the offline before export. I understand this seems to be a caveat for the hardened ranged descriptors that few pre-generated keys must be there before export otherwise the online wallet can’t create new addresses - I feel this should be mentioned in the RPC Help section wrt the usability of the online wallet where the user might face this error straightaway if the keys were not pre-generated for export (though a little less likely with the default keypool size of 1000)

achow101 commented at 9:09 pm on August 25, 2025:

Added extra text to the help.

in test/functional/wallet_exported_watchonly.py:249 in cf588607fa outdated

244+        online_wallet.unloadwallet()
245+
246+    def run_test(self):
247+        self.online = self.nodes[0]
248+        self.offline = self.nodes[1]
249+        self.funds = self.online.get_wallet_rpc(self.default_wallet_name)

rkrux commented at 2:32 pm on August 25, 2025:

In cf588607fa36964597a6d6acc853eeb26e51b0ea “test: Test for exportwatchonlywallet”

Nit for clarity in the subtests. Otherwise it takes a while to figure out that these funds are not part of the wallet(s) under consideration.

0- self.funds = self.online.get_wallet_rpc(self.default_wallet_name)
1+ self.outside_wallet_funds = self.online.get_wallet_rpc(self.default_wallet_name)

achow101 commented at 9:10 pm on August 25, 2025:

Renamed to funder.

in test/functional/wallet_exported_watchonly.py:67 in cf588607fa outdated

62+
63+        # Verify that online wallet cannot spend, but offline can
64+        self.funds.sendtoaddress(online_wallet.getnewaddress(), 10)
65+        self.generate(self.online, 1, sync_fun=self.no_op)
66+        assert_equal(online_wallet.getbalances()["mine"]["trusted"], 10)
67+        assert_equal(offline_wallet.getbalances()["mine"]["trusted"], 0)

rkrux commented at 2:40 pm on August 25, 2025:

In https://github.com/bitcoin/bitcoin/commit/cf588607fa36964597a6d6acc853eeb26e51b0ea “test: Test for exportwatchonlywallet”

In this subtest, can also assert for few getwalletinfo RPC response values being equal as well such as txcount, keypoolsize, keypoolsize_hd_internal, birthtime, lastprocessedblock. For the flags RPC response key, it can be asserted that for the watch-only one, the value is the union of the original flags and disable_private_keys.

Might as well put all these assertions in a assert_common_values function that can be called in all the subtests.

achow101 commented at 9:06 pm on August 25, 2025:

I don’t think any of those are actually useful things to assert. The point of the offline wallet is that it doesn’t have access to the blockchain, so actually txcount, and lastprocessedblock should be different. The keypool sizes may be different depending on the node startup options, and birthtime may be different if something was used before being imported to the offline wallet.

rkrux commented at 2:05 pm on August 26, 2025:

Fair point. I can imagine one of the use cases being that the user creates a offline wallet and immediately exports the watchonly version that’s to be imported on a connected node.

in test/functional/wallet_exported_watchonly.py:165 in cf588607fa outdated

160+            ]
161+        )
162+        assert_equal(all([r["success"] for r in import_res]), True)
163+
164+        # Make sure that the hardened derivation has some pregenerated keys
165+        offline_wallet.keypoolrefill(10)

rkrux commented at 2:42 pm on August 25, 2025:

In https://github.com/bitcoin/bitcoin/commit/cf588607fa36964597a6d6acc853eeb26e51b0ea “test: Test for exportwatchonlywallet”

While calling this RPC here is fine, maybe add 10 as the default value for keypool in the node extra_args argument? That would represent a scenario that’s more likely to happen in the real world.

achow101 commented at 9:10 pm on August 25, 2025:

Done

in test/functional/wallet_exported_watchonly.py:183 in cf588607fa outdated

178+                    assert_equal(addr, online_wallet.getnewaddress(address_type=address_type))
179+                self.funds.sendtoaddress(addr, 1)
180+        self.generate(self.online, 1, sync_fun=self.no_op)
181+
182+        # The hardened derivation should have 9 remaining addresses
183+        for _ in range(9):

rkrux commented at 2:43 pm on August 25, 2025:

In https://github.com/bitcoin/bitcoin/commit/cf588607fa36964597a6d6acc853eeb26e51b0ea “test: Test for exportwatchonlywallet”

Nit: would be nice to tie this 9 to the 10 above in the keypoolrefill RPC - KEYPOOL_SIZE - 1

achow101 commented at 9:10 pm on August 25, 2025:

Done

in src/wallet/rpc/wallet.cpp:849 in cdaed2bab9 outdated

845@@ -846,6 +846,46 @@ static RPCHelpMan createwalletdescriptor()
846     };
847 }
848 
849+static RPCHelpMan exportwatchonlywallet()

rkrux commented at 2:49 pm on August 25, 2025:

In cdaed2bab95870f77efb3cbca1dc98e5d4f5695c “wallet, rpc: Add exportwatchonlywallet RPC”

Based on some threshold value of remaining unused keys available, should this RPC also pre-generate keys before exporting the watch-only wallet so that the user doesn’t end up with not being able to generate new addresses quickly after exporting and has to export again?

achow101 commented at 9:10 pm on August 25, 2025:

Done, however there should not be an expectation that the topup will always work as it does not work for wallets that are encrypted but not unlocked.

rkrux commented at 2:58 pm on August 25, 2025: contributor

Started reviewing.

Code review cf588607fa36964597a6d6acc853eeb26e51b0ea

achow101 force-pushed on Aug 25, 2025

in src/wallet/wallet.cpp:4568 in 3a30ce4cec outdated

4563+        for (const WalletDescInfo& desc_info : *exported) {
4564+            // Parse the descriptor
4565+            FlatSigningProvider keys;
4566+            std::string parse_err;
4567+            std::vector<std::unique_ptr<Descriptor>> descs = Parse(desc_info.descriptor, keys, parse_err, /*require_checksum=*/true);
4568+            assert(descs.size() == 1); // All of our descriptors should be valid, and not multipath

rkrux commented at 11:47 am on August 26, 2025:

In 3a30ce4cecfb25c353ec4e5f178f47668214bb4a “wallet: Add CWallet::ExportWatchOnly”

Because there is no actual use of keys and error here in this flow.

 0diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
 1index 4bfa11c0bf..95061559c7 100644
 2--- a/src/wallet/wallet.cpp
 3+++ b/src/wallet/wallet.cpp
 4@@ -4562,10 +4562,11 @@ util::Result<std::string> CWallet::ExportWatchOnlyWallet(const fs::path& destina
 5         // Parse the descriptors and add them to the new wallet
 6         for (const WalletDescInfo& desc_info : *exported) {
 7             // Parse the descriptor
 8-            FlatSigningProvider keys;
 9-            std::string parse_err;
10-            std::vector<std::unique_ptr<Descriptor>> descs = Parse(desc_info.descriptor, keys, parse_err, /*require_checksum=*/true);
11+            FlatSigningProvider dummy_keys;
12+            std::string dummy_err;
13+            std::vector<std::unique_ptr<Descriptor>> descs = Parse(desc_info.descriptor, dummy_keys, dummy_err, /*require_checksum=*/true);
14-            assert(descs.size() == 1); // All of our descriptors should be valid, and not multipath
15+            assert(descs.size() == 1); // All of our descriptors should be valid, and not multipath as they are made 'singlepath' during import
16+            assert(dummy_keys.keys.size() == 0); // No private keys should be present in case of public descriptors
17 
18             // Get the range if there is one
19             int32_t range_start = 0;
20@@ -4585,7 +4586,7 @@ util::Result<std::string> CWallet::ExportWatchOnlyWallet(const fs::path& destina
21             }
22 
23             // Add to the watchonly wallet
24-            if (auto spkm_res = watchonly_wallet->AddWalletDescriptor(w_desc, keys, "", false); !spkm_res) {
25+            if (auto spkm_res = watchonly_wallet->AddWalletDescriptor(w_desc, dummy_keys, "", false); !spkm_res) {
26                 return util::Error{util::ErrorString(spkm_res)};
27             }

achow101 commented at 6:26 pm on August 26, 2025:

Done

in src/wallet/wallet.cpp:4611 in 3a30ce4cec outdated

4606+            if (!persisted) continue;
4607+            watchonly_wallet->LockCoin(coin, persisted);
4608+        }
4609+
4610+        {
4611+            // Make a WalletBatch for the watchonly_wallet so that everything else can be written atomically

rkrux commented at 11:49 am on August 26, 2025:

In 3a30ce4cecfb25c353ec4e5f178f47668214bb4a “wallet: Add CWallet::ExportWatchOnly”

Nit: s/watchonly_wallet/watchonly wallet

achow101 commented at 6:26 pm on August 26, 2025:

Done

in src/wallet/wallet.cpp:4588 in 3a30ce4cec outdated

4583+                DescriptorScriptPubKeyMan* desc_spkm = dynamic_cast<DescriptorScriptPubKeyMan*>(GetScriptPubKeyMan(desc_id));
4584+                w_desc.cache = WITH_LOCK(desc_spkm->cs_desc_man, return desc_spkm->GetWalletDescriptor().cache);
4585+            }
4586+
4587+            // Add to the watchonly wallet
4588+            if (auto spkm_res = watchonly_wallet->AddWalletDescriptor(w_desc, keys, "", false); !spkm_res) {

rkrux commented at 12:18 pm on August 26, 2025:

In 3a30ce4cecfb25c353ec4e5f178f47668214bb4a “wallet: Add CWallet::ExportWatchOnly”

Nit:

0             // Add to the watchonly wallet
1-            if (auto spkm_res = watchonly_wallet->AddWalletDescriptor(w_desc, keys, "", false); !spkm_res) {
2+            if (auto spkm_res = watchonly_wallet->AddWalletDescriptor(w_desc, dummy_keys, /*label*/ "", /*internal*/ false); !spkm_res) {

rkrux commented at 12:18 pm on August 26, 2025:

In https://github.com/bitcoin/bitcoin/commit/3a30ce4cecfb25c353ec4e5f178f47668214bb4a “wallet: Add CWallet::ExportWatchOnly”

All the inactive descriptors are hardcoded to not being internal, is this correct? I can’t seem to come up with a test case that fails this assumption.

achow101 commented at 6:15 pm on August 26, 2025:

internal is only used for setting the labels in the address book within AddWalletDescriptor, and only if the descriptor is not ranged and that there is a label provided. Since we copy the labels from the address book right after this, there’s no need for us to provide label or internal to this call.

achow101 commented at 6:26 pm on August 26, 2025:

Done

in test/functional/wallet_exported_watchonly.py:107 in c9eb6c853d outdated

102+        for wallet in [online_wallet, offline_wallet]:
103+            for purpose in ["receive", "send"]:
104+                label = f"addrbook_{purpose}"
105+                assert_equal(wallet.listlabels(purpose), [label])
106+                addr = send_addr if purpose == "send" else receive_addr
107+                assert_equal(offline_wallet.getaddressesbylabel(label), {addr: {"purpose": purpose}})

rkrux commented at 12:34 pm on August 26, 2025:

In c9eb6c853dbc96f47c89ab5ca26dde028cd5e108 “test: Test for exportwatchonlywallet”

Otherwise assertion for online wallet is missed.

0--- a/test/functional/wallet_exported_watchonly.py
1+++ b/test/functional/wallet_exported_watchonly.py
2@@ -104,7 +104,7 @@ class WalletExportedWatchOnly(BitcoinTestFramework):
3                 label = f"addrbook_{purpose}"
4                 assert_equal(wallet.listlabels(purpose), [label])
5                 addr = send_addr if purpose == "send" else receive_addr
6-                assert_equal(offline_wallet.getaddressesbylabel(label), {addr: {"purpose": purpose}})
7+                assert_equal(wallet.getaddressesbylabel(label), {addr: {"purpose": purpose}})

achow101 commented at 6:26 pm on August 26, 2025:

Done

in test/functional/wallet_exported_watchonly.py:214 in c9eb6c853d outdated

209+        self.funder.sendtoaddress(addr, 1)
210+        self.generate(self.online, 1)
211+        # Spend funds in order to mark addr as previously spent
212+        offline_wallet.sendall([self.funder.getnewaddress()])
213+        self.funder.sendtoaddress(addr, 1)
214+        self.sync_mempools()

rkrux commented at 1:04 pm on August 26, 2025:

In https://github.com/bitcoin/bitcoin/commit/c9eb6c853dbc96f47c89ab5ca26dde028cd5e108 “test: Test for exportwatchonlywallet”

Nit: don’t think this call is necessary here.

0@@ -211,7 +215,6 @@ class WalletExportedWatchOnly(BitcoinTestFramework):
1         # Spend funds in order to mark addr as previously spent
2         offline_wallet.sendall([self.funder.getnewaddress()])
3         self.funder.sendtoaddress(addr, 1)
4-        self.sync_mempools()
5         self.generate(self.online, 1)

achow101 commented at 6:26 pm on August 26, 2025:

Done

in test/functional/wallet_exported_watchonly.py:216 in c9eb6c853d outdated

211+        # Spend funds in order to mark addr as previously spent
212+        offline_wallet.sendall([self.funder.getnewaddress()])
213+        self.funder.sendtoaddress(addr, 1)
214+        self.sync_mempools()
215+        self.generate(self.online, 1)
216+        assert_equal(offline_wallet.listunspent()[0]["reused"], True)

rkrux commented at 1:12 pm on August 26, 2025:

In https://github.com/bitcoin/bitcoin/commit/c9eb6c853dbc96f47c89ab5ca26dde028cd5e108 “test: Test for exportwatchonlywallet”

Nit to make it explicit that why the unspent is marked as reused.

 0-        assert_equal(offline_wallet.listunspent()[0]["reused"], True)
 1+        assert_equal(offline_wallet.listunspent(addresses=[addr])[0]["reused"], True)
 2         self.disconnect_nodes(self.offline.index ,self.online.index)
 3 
 4         # Export the watchonly wallet file and load onto online node
 5@@ -221,7 +224,7 @@ class WalletExportedWatchOnly(BitcoinTestFramework):
 6 
 7         # check avoid_reuse is still set
 8         assert_equal(online_wallet.getwalletinfo()["avoid_reuse"], True)
 9-        assert_equal(online_wallet.listunspent()[0]["reused"], True)
10+        assert_equal(online_wallet.listunspent(addresses=[addr])[0]["reused"], True)

achow101 commented at 6:26 pm on August 26, 2025:

Done

in src/wallet/rpc/wallet.cpp:881 in 34137b3270 outdated

876+
877+            std::string dest = request.params[0].get_str();
878+
879+            LOCK(pwallet->cs_wallet);
880+            pwallet->TopUpKeyPool();
881+            util::Result<std::string> exported = pwallet->ExportWatchOnlyWallet(fs::PathFromString(dest), context);

rkrux commented at 1:53 pm on August 26, 2025:

In 34137b327085b8e1d18f4dbf30f773fb507505dc “wallet, rpc: Add exportwatchonlywallet RPC”

Any particular reason to convert from string to path before passing it to ExportWatchOnlyWallet? Asking because I noticed 4 fs::PathToString(destination) calls inside ExportWatchOnlyWallet that could be avoided if the string is accepted and this conversion is done over there.

achow101 commented at 6:25 pm on August 26, 2025:

I think it conceptually makes more sense to be passing paths as fs::Path, rather than strings. Ideally, BackupWallet should be taking a fs::Path instead of a string, and the other conversions are only necessary for logging.

in src/wallet/wallet.cpp:4546 in 3a30ce4cec outdated

4541+    options.create_flags = GetWalletFlags() | WALLET_FLAG_DISABLE_PRIVATE_KEYS;
4542+
4543+    // Make the watchonly wallet
4544+    DatabaseStatus status;
4545+    std::vector<bilingual_str> warnings;
4546+    std::string wallet_name = GetName() + "_watchonly";

rkrux commented at 2:03 pm on August 26, 2025:

In https://github.com/bitcoin/bitcoin/commit/3a30ce4cecfb25c353ec4e5f178f47668214bb4a “wallet: Add CWallet::ExportWatchOnly”

Logs while exporting:

02025-08-26T13:55:18Z [test_watchonly] Setting spkMan to active: id = c21a5fab36193920bcc2f13558b436ac4b1866c56a97bf03bb4c128610279a21, type = bech32m, internal = true

Perhaps we can also add a temp in the name to emphasise in the logs that this is a temporary wallet. Maybe <name>_watchonly_temp?

achow101 commented at 6:26 pm on August 26, 2025:

Done

rkrux commented at 2:04 pm on August 26, 2025: contributor

Code review c9eb6c853dbc96f47c89ab5ca26dde028cd5e108

Looking good, mentioned few points.

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.

2e833c1ea7

wallet, rpc: Add exportwatchonlywallet RPC 7775c549f6

test: Test for exportwatchonlywallet 6e8cddbd30

achow101 force-pushed on Aug 26, 2025

doc: update offline-signing-tutorial to use exportwatchonlywallet rpc 0f54714148

rkrux approved

rkrux commented at 11:31 am on August 28, 2025: contributor

lgtm ACK 0f547141487e3964a55102f6ae441233d7144aaf

Thanks for addressing the comments.

DrahtBot requested review from enirox001 on Aug 28, 2025

DrahtBot requested review from ryanofsky on Aug 28, 2025

DrahtBot requested review from stringintech on Aug 28, 2025

DrahtBot requested review from pablomartin4btc on Aug 28, 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