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

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

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35043 (refactor: Properly return from ThreadSafeQuestion signal + btcsignals follow-ups by maflcko)
#34681 (wallet: refactor ScanForWalletTransactions by Eunovo)
#34400 (wallet: parallel fast rescan (approx 5x speed up with 8 threads) by Eunovo)
#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)
#32857 (wallet: allow skipping script paths by Sjors)
#32784 (wallet: derivehdkey RPC to get xpub at arbitrary path by Sjors)
#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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

specified destination directory -> specified destination path/filename [The function exports to a file at destination (a file path), not a directory; “directory” could confuse readers about what the argument should be.]

2026-04-20 20:42:53

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.

<details><summary>Hints</summary>

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.

</details>

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.

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 3:12 PM on December 10, 2025:

In 9317ea0b1cfa2f31dd78c3ec58c6f6eeedf9ba20 wallet: Use Descriptor::CanSelfExpand() in CanGetAddresses():

The explanation makes sense, but I can drop || m_wallet_descriptor.descriptor->CanSelfExpand() (after 3ebf3f7b58aade3da20b50bf69c5ef8fe5ac2f0f test: Test for exportwatchonlywallet) without breaking wallet_exported_watchonly.py.

Eunovo commented at 3:02 PM on January 27, 2026:

The explanation makes sense, but I can drop || m_wallet_descriptor.descriptor->CanSelfExpand() (after 3ebf3f7b58aade3da20b50bf69c5ef8fe5ac2f0f test: Test for exportwatchonlywallet) without breaking wallet_exported_watchonly.py.

Can confirm this happens. AFAICT, the test in https://github.com/bitcoin/bitcoin/pull/32489/changes/44835957287fd7638f51a47d4cf41b9619942ae8 doesn't fail because it does not try to expand a non-hardened key past range_end on a watchonly wallet, so CanGetAddresses() will evaluate to true even without the CanSelfExpand() check.

achow101 commented at 11:56 PM on January 27, 2026:

The test ended up being broken by the change to the keypool size. I've added an additional check that should hit this case.

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

in test/functional/wallet_exported_watchonly.py:230 in 3010b614ac outdated

 225 | +        offline_wallet.sendall([self.funds.getnewaddress()])
 226 | +        self.funds.sendtoaddress(addr, 1)
 227 | +        self.sync_mempools()
 228 | +        self.generate(self.online, 1)
 229 | +        assert_equal(offline_wallet.listunspent()[0]["reused"], True)
 230 | +        self.disconnect_nodes(0 ,1)

Eunovo commented at 8:27 AM on June 19, 2025:

nit: self.disconnect_nodes(0 ,1) should be self.disconnect_nodes(0, 1)

achow101 commented at 11:55 PM on January 27, 2026:

Fixed

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:3712 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:4483 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?

--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -786,6 +786,16 @@ public:
         }
     }
 
+    bool CanSelfExpand() const override {
+        for (const auto& key : m_pubkey_args) {
+            if (!key->CanSelfExpand()) return false;
+        }
+        for (const auto& sub : m_subdescriptor_args) {
+            if (!sub->CanSelfExpand()) return false;
+        }
+        return true;
+    }
+
     virtual std::unique_ptr<DescriptorImpl> Clone() const = 0;
 };
 
@@ -806,7 +816,6 @@ public:
     }
     bool IsSingleType() const final { return true; }
     bool ToPrivateString(const SigningProvider& arg, std::string& out) const final { return false; }
-    bool CanSelfExpand() const final { return true; }
 
     std::optional<int64_t> ScriptSize() const override { return GetScriptForDestination(m_destination).size(); }
     std::unique_ptr<DescriptorImpl> Clone() const override
@@ -834,7 +843,6 @@ public:
     }
     bool IsSingleType() const final { return true; }
     bool ToPrivateString(const SigningProvider& arg, std::string& out) const final { return false; }
-    bool CanSelfExpand() const final { return true; }
 
     std::optional<int64_t> ScriptSize() const override { return m_script.size(); }
 
@@ -862,7 +870,6 @@ protected:
 public:
     PKDescriptor(std::unique_ptr<PubkeyProvider> prov, bool xonly = false) : DescriptorImpl(Vector(std::move(prov)), "pk"), m_xonly(xonly) {}
     bool IsSingleType() const final { return true; }
-    bool CanSelfExpand() const override { return m_pubkey_args[0]->CanSelfExpand(); }
 
     std::optional<int64_t> ScriptSize() const override {
         return 1 + (m_xonly ? 32 : m_pubkey_args[0]->GetSize()) + 1;
@@ -898,7 +905,6 @@ public:
     PKHDescriptor(std::unique_ptr<PubkeyProvider> prov) : DescriptorImpl(Vector(std::move(prov)), "pkh") {}
     std::optional<OutputType> GetOutputType() const override { return OutputType::LEGACY; }
     bool IsSingleType() const final { return true; }
-    bool CanSelfExpand() const override { return m_pubkey_args[0]->CanSelfExpand(); }
 
     std::optional<int64_t> ScriptSize() const override { return 1 + 1 + 1 + 20 + 1 + 1; }
 
@@ -932,7 +938,6 @@ public:
     WPKHDescriptor(std::unique_ptr<PubkeyProvider> prov) : DescriptorImpl(Vector(std::move(prov)), "wpkh") {}
     std::optional<OutputType> GetOutputType() const override { return OutputType::BECH32; }
     bool IsSingleType() const final { return true; }
-    bool CanSelfExpand() const override { return m_pubkey_args[0]->CanSelfExpand(); }
 
     std::optional<int64_t> ScriptSize() const override { return 1 + 1 + 20; }
 
@@ -974,7 +979,6 @@ protected:
 public:
     ComboDescriptor(std::unique_ptr<PubkeyProvider> prov) : DescriptorImpl(Vector(std::move(prov)), "combo") {}
     bool IsSingleType() const final { return false; }
-    bool CanSelfExpand() const override { return m_pubkey_args[0]->CanSelfExpand(); }
     std::unique_ptr<DescriptorImpl> Clone() const override
     {
         return std::make_unique<ComboDescriptor>(m_pubkey_args.at(0)->Clone());
@@ -999,13 +1003,6 @@ protected:
 public:
     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) {}
     bool IsSingleType() const final { return true; }
-    bool CanSelfExpand() const override {
-        bool can_expand = true;
-        for (const auto& key : m_pubkey_args) {
-            can_expand &= key->CanSelfExpand();
-        }
-        return can_expand;
-    }
 
     std::optional<int64_t> ScriptSize() const override {
         const auto n_keys = m_pubkey_args.size();
@@ -1057,13 +1054,6 @@ protected:
 public:
     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) {}
     bool IsSingleType() const final { return true; }
-    bool CanSelfExpand() const override {
-        bool can_expand = true;
-        for (const auto& key : m_pubkey_args) {
-            can_expand &= key->CanSelfExpand();
-        }
-        return can_expand;
-    }
 
     std::optional<int64_t> ScriptSize() const override {
         const auto n_keys = m_pubkey_args.size();
@@ -1110,7 +1100,6 @@ public:
         return OutputType::LEGACY;
     }
     bool IsSingleType() const final { return true; }
-    bool CanSelfExpand() const override { return m_subdescriptor_args[0]->CanSelfExpand(); }
 
     std::optional<int64_t> ScriptSize() const override { return 1 + 1 + 20 + 1; }
 
@@ -1152,7 +1141,6 @@ public:
     WSHDescriptor(std::unique_ptr<DescriptorImpl> desc) : DescriptorImpl({}, std::move(desc), "wsh") {}
     std::optional<OutputType> GetOutputType() const override { return OutputType::BECH32; }
     bool IsSingleType() const final { return true; }
-    bool CanSelfExpand() const override { return m_subdescriptor_args[0]->CanSelfExpand(); }
 
     std::optional<int64_t> ScriptSize() const override { return 1 + 1 + 32; }
 
@@ -1230,13 +1218,6 @@ public:
     }
     std::optional<OutputType> GetOutputType() const override { return OutputType::BECH32M; }
     bool IsSingleType() const final { return true; }
-    bool CanSelfExpand() const override {
-        bool can_expand = m_pubkey_args[0]->CanSelfExpand();
-        for (const auto& sub : m_subdescriptor_args) {
-            can_expand &= sub->CanSelfExpand();
-        }
-        return can_expand;
-    }
 
     std::optional<int64_t> ScriptSize() const override { return 1 + 1 + 32; }
 
@@ -1364,13 +1345,6 @@ public:
 
     bool IsSolvable() const override { return true; }
     bool IsSingleType() const final { return true; }
-    bool CanSelfExpand() const override {
-        bool can_expand = true;
-        for (const auto& key : m_pubkey_args) {
-            can_expand &= key->CanSelfExpand();
-        }
-        return can_expand;
-    }
 
     std::optional<int64_t> ScriptSize() const override { return m_node->ScriptSize(); }
 
@@ -1410,7 +1384,6 @@ public:
     RawTRDescriptor(std::unique_ptr<PubkeyProvider> output_key) : DescriptorImpl(Vector(std::move(output_key)), "rawtr") {}
     std::optional<OutputType> GetOutputType() const override { return OutputType::BECH32M; }
     bool IsSingleType() const final { return true; }
-    bool CanSelfExpand() const override { return m_pubkey_args[0]->CanSelfExpand(); }
 
     std::optional<int64_t> ScriptSize() const override { return 1 + 1 + 32; }

</details>

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.

<details><summary>Hints</summary>

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.

</details>

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:

        "Creates a wallet file at the specified destination containing a watchonly version "
        "of the current wallet. This watchonly wallet contains the wallet's public descriptors, "
        "its transactions, and address book data. The watchonly wallet can be imported into "
        "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:

--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -19,6 +19,7 @@
 #include <consensus/consensus.h>
 #include <consensus/validation.h>
 #include <external_signer.h>
+#include <fstream>
 #include <interfaces/chain.h>
 #include <interfaces/handler.h>
 #include <interfaces/wallet.h>
@@ -4486,8 +4487,10 @@ util::Result<std::string> CWallet::ExportWatchOnlyWallet(const fs::path& destina
     if (fs::exists(destination)) {
         return util::Error{strprintf(_("Error: Export destination '%s' already exists"), fs::PathToString(destination))};
     }
-    fs::path canonical_dest = fs::canonical(destination.parent_path());
-    canonical_dest /= destination.filename();
+    if (!std::ofstream{destination}) {
+        return util::Error{strprintf(_("Error: Could not create file '%s'"), fs::PathToString(destination))};
+    }
+    fs::remove(destination);
 
     // Get the descriptors from this wallet
     util::Result<std::vector<WalletDescInfo>> exported = ExportDescriptors(/*export_private=*/false);
@@ -4630,7 +4633,7 @@ util::Result<std::string> CWallet::ExportWatchOnlyWallet(const fs::path& destina
         }
 
         // Make a backup of this wallet at the specified destination directory
-        watchonly_wallet->BackupWallet(fs::PathToString(canonical_dest));
+        watchonly_wallet->BackupWallet(fs::PathToString(destination));
     }
 
     // Delete the watchonly wallet now that it has been exported to the desired location
@@ -4638,6 +4641,6 @@ util::Result<std::string> CWallet::ExportWatchOnlyWallet(const fs::path& destina
     watchonly_wallet.reset();
     fs::remove_all(watchonly_path);
 
-    return fs::PathToString(canonical_dest);
+    return fs::PathToString(destination);
 }
 } // namespace wallet

</details>

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

w0xlt commented at 6:39 PM on September 24, 2025:

Wouldn't it be safe to delete only the watchonly wallet file ?

achow101 commented at 6:43 PM on September 24, 2025:

Wouldn't it be safe to delete only the watchonly wallet file ?

I don't think we should be leaving behind the wallet directory, and possibly the -journal file either.

Sjors commented at 3:47 PM on December 10, 2025:

After #33032 this intermediate watch-only wallet could in-memory.

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:4547 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:4522 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

achow101 force-pushed on Aug 20, 2025

in test/functional/wallet_exported_watchonly.py:168 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.

- self.funds = self.online.get_wallet_rpc(self.default_wallet_name)
+ 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.

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

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:

             // Add to the watchonly wallet
-            if (auto spkm_res = watchonly_wallet->AddWalletDescriptor(w_desc, keys, "", false); !spkm_res) {
+            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.

--- a/test/functional/wallet_exported_watchonly.py
+++ b/test/functional/wallet_exported_watchonly.py
@@ -104,7 +104,7 @@ class WalletExportedWatchOnly(BitcoinTestFramework):
                 label = f"addrbook_{purpose}"
                 assert_equal(wallet.listlabels(purpose), [label])
                 addr = send_addr if purpose == "send" else receive_addr
-                assert_equal(offline_wallet.getaddressesbylabel(label), {addr: {"purpose": purpose}})
+                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.

@@ -211,7 +215,6 @@ class WalletExportedWatchOnly(BitcoinTestFramework):
         # Spend funds in order to mark addr as previously spent
         offline_wallet.sendall([self.funder.getnewaddress()])
         self.funder.sendtoaddress(addr, 1)
-        self.sync_mempools()
         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.

-        assert_equal(offline_wallet.listunspent()[0]["reused"], True)
+        assert_equal(offline_wallet.listunspent(addresses=[addr])[0]["reused"], True)
         self.disconnect_nodes(self.offline.index ,self.online.index)
 
         # Export the watchonly wallet file and load onto online node
@@ -221,7 +224,7 @@ class WalletExportedWatchOnly(BitcoinTestFramework):
 
         # check avoid_reuse is still set
         assert_equal(online_wallet.getwalletinfo()["avoid_reuse"], True)
-        assert_equal(online_wallet.listunspent()[0]["reused"], True)
+        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:

2025-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.

achow101 force-pushed on Aug 26, 2025

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

polespinasa commented at 1:53 AM on September 9, 2025: member

slightly code review tACK 0f547141487e3964a55102f6ae441233d7144aaf

The behaviour of this new RPC was tested to update the guide in commit 0f547141487e3964a55102f6ae441233d7144aaf and the behavior is the expected.

in src/wallet/wallet.cpp:4619 in 0f54714148

4614 | +            if (!watchonly_batch.TxnBegin()) {
4615 | +                return util::Error{strprintf(_("Error: database transaction cannot be executed for new watchonly wallet %s"), watchonly_wallet->GetName())};
4616 | +            }
4617 | +
4618 | +            // Copy orderPosNext
4619 | +            watchonly_batch.WriteOrderPosNext(watchonly_wallet->nOrderPosNext);

w0xlt commented at 6:27 PM on September 24, 2025:

Is this copying nOrderPosNext from source wallet to the watchonly one ?

            watchonly_batch.WriteOrderPosNext(this->nOrderPosNext);

achow101 commented at 6:41 PM on September 24, 2025:

Oops fixed

achow101 force-pushed on Sep 24, 2025

in src/wallet/wallet.cpp:3710 in 4d7aeab150 outdated

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

w0xlt commented at 6:52 PM on September 24, 2025:

Can desc.cache be empty here ?

achow101 commented at 5:49 PM on September 25, 2025:

Yes, but that's not an issue.

in test/functional/wallet_exported_watchonly.py:37 in 6fafa9dd48 outdated

  32 | +
  33 | +    def export_and_restore(self, wallet, export_name):
  34 | +        export_path = os.path.join(self.export_path, f"{export_name}.dat")
  35 | +        res = wallet.exportwatchonlywallet(export_path)
  36 | +        assert_equal(res["exported_file"], export_path)
  37 | +        self.online.restorewallet(export_name, res["exported_file"])

w0xlt commented at 9:41 PM on September 24, 2025:

The assertion below (same as found in wallet_migration.py) fails. Is this expected ?

        # Assert no rescan occurs when restoring the exported watch-only wallet.
        with self.online.assert_debug_log(unexpected_msgs=["Rescanning"]):
            self.online.restorewallet(export_name, res["exported_file"])

achow101 commented at 5:53 PM on September 25, 2025:

Yes. The nodes in this test are not always in sync with each other.

in src/wallet/wallet.cpp:4523 in a453ea6ff0

4518 | +        return util::Error{_("Error: Export destination cannot be empty")};
4519 | +    }
4520 | +    if (fs::exists(destination)) {
4521 | +        return util::Error{strprintf(_("Error: Export destination '%s' already exists"), fs::PathToString(destination))};
4522 | +    }
4523 | +    if (!std::ofstream{destination}) {

Sjors commented at 2:02 PM on December 10, 2025:

In a453ea6ff06797224adbb1dd839299fe3d8636ae wallet: Add CWallet::ExportWatchOnly: silent merge conflict? When I rebase on master:

 error: conversion function from 'const fs::path' to 'const string' (aka 'const basic_string<char>') invokes a deleted function
 4532 |     if (!std::ofstream{destination}) {

achow101 commented at 8:06 PM on December 10, 2025:

Fixed

in src/script/descriptor.cpp:363 in a3be2d8745 outdated

 359 | @@ -356,6 +360,7 @@ class ConstPubkeyProvider final : public PubkeyProvider
 360 |      {
 361 |          return std::make_unique<ConstPubkeyProvider>(m_expr_index, m_pubkey, m_xonly);
 362 |      }
 363 | +    bool CanSelfExpand() const final { return true; }

Sjors commented at 2:16 PM on December 10, 2025:

In a3be2d874503dc24b5d52fe03d4cf2c7b5ff607d descriptor: Add CanSelfExpand(): this can't expand. Maybe the description can clarify that the return value should be ignored if it can't expand with or without keys.

Alternatively it can return false, though that needs more changes:

diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index 4ed88277e2..7e978140b0 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -360,7 +360,7 @@ public:
     {
         return std::make_unique<ConstPubkeyProvider>(m_expr_index, m_pubkey, m_xonly);
     }
-    bool CanSelfExpand() const final { return true; }
+    bool CanSelfExpand() const final { return false; }
 };

 enum class DeriveType {
@@ -790,7 +790,7 @@ public:
     bool CanSelfExpand() const override
     {
         for (const auto& key : m_participants) {
-            if (!key->CanSelfExpand()) return false;
+            if (key->IsRange() && !key->CanSelfExpand()) return false;
         }
         return true;
     }
@@ -1008,7 +1008,7 @@ public:
     bool CanSelfExpand() const override
     {
         for (const auto& key : m_pubkey_args) {
-            if (!key->CanSelfExpand()) return false;
+            if (key->IsRange() && !key->CanSelfExpand()) return false;
         }
         for (const auto& sub : m_subdescriptor_args) {
             if (!sub->CanSelfExpand()) return false;

achow101 commented at 7:23 PM on December 10, 2025:

ConstPubkeyProvider can self expand. Self expand in the context of PubkeyProviders means that pubkeys can always be retrieved, and this is always true for ConstPubkeyProvider. ConstPubkeyProvider cannot be ranged, nor can it be something that requires a private key in order to get the pubkey.

Sjors commented at 2:28 PM on December 10, 2025: member

Reviewed first commit, will continue later.

in src/wallet/wallet.h:1082 in 67216700ce

1076 | @@ -1065,6 +1077,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
1077 |      //! Find the private key for the given key id from the wallet's descriptors, if available
1078 |      //! Returns nullopt when no descriptor has the key or if the wallet is locked.
1079 |      std::optional<CKey> GetKey(const CKeyID& keyid) const;
1080 | +
1081 | +    //! Export the descriptors from this wallet so that they can be imported elsewhere
1082 | +    util::Result<std::vector<WalletDescInfo>> ExportDescriptors(bool export_private) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

Sjors commented at 3:36 PM on December 10, 2025:

In 67216700ce8ec5e84bca5f83860091f6ac4f67f4 wallet: Move listdescriptors retrieving from RPC to CWallet:

Could try the new util::Expected<<std::vector<WalletDescInfo>>,std::string> from #34006 here.

achow101 commented at 8:07 PM on December 10, 2025:

Done

in src/wallet/wallet.cpp:4563 in a453ea6ff0 outdated

4558 | +
4559 | +    {
4560 | +        LOCK(watchonly_wallet->cs_wallet);
4561 | +
4562 | +        // Parse the descriptors and add them to the new wallet
4563 | +        for (const WalletDescInfo& desc_info : *exported) {

Sjors commented at 3:56 PM on December 10, 2025:

In a453ea6ff06797224adbb1dd839299fe3d8636ae wallet: Add CWallet::ExportWatchOnly: nit *Assert(exported)

achow101 commented at 11:58 PM on January 27, 2026:

Done

in src/wallet/wallet.cpp:4583 in a453ea6ff0 outdated

4578 | +
4579 | +            WalletDescriptor w_desc(std::move(descs.at(0)), desc_info.creation_time, range_start, range_end, desc_info.next_index);
4580 | +
4581 | +            // For descriptors that cannot self expand (i.e. needs private keys or cache), retrieve the cache
4582 | +            uint256 desc_id = w_desc.id;
4583 | +            if (!w_desc.descriptor->CanSelfExpand()) {

Sjors commented at 4:02 PM on December 10, 2025:

In a453ea6ff06797224adbb1dd839299fe3d8636ae wallet: Add CWallet::ExportWatchOnly: if you take my earlier suggestion #32489 (review) for having this return false for non-ranged descriptors, you'll need to add w_desc.descriptor->IsRange() &&

in src/wallet/wallet.cpp:4585 in a453ea6ff0 outdated

4580 | +
4581 | +            // For descriptors that cannot self expand (i.e. needs private keys or cache), retrieve the cache
4582 | +            uint256 desc_id = w_desc.id;
4583 | +            if (!w_desc.descriptor->CanSelfExpand()) {
4584 | +                DescriptorScriptPubKeyMan* desc_spkm = dynamic_cast<DescriptorScriptPubKeyMan*>(GetScriptPubKeyMan(desc_id));
4585 | +                w_desc.cache = WITH_LOCK(desc_spkm->cs_desc_man, return desc_spkm->GetWalletDescriptor().cache);

Sjors commented at 4:09 PM on December 10, 2025:

In a453ea6ff06797224adbb1dd839299fe3d8636ae wallet: Add CWallet::ExportWatchOnly: at the moment DescriptorCache doesn't contain any private key material, but if that ever changes this line would put that into the watch-only wallet.

Maybe add a note to the DescriptorCache description that if private key material is added, ExportWatchOnlyWallet needs to be modified to avoid it.

achow101 commented at 8:06 PM on December 10, 2025:

I think that is unnecessary, and unlikely to happen.

Sjors commented at 4:12 PM on December 10, 2025: member

Reviewed 6fafa9dd48a814a929efb31b746dce343a3c96dc and tested it, though not with hardened derivation.

achow101 force-pushed on Dec 10, 2025

DrahtBot added the label CI failed on Dec 10, 2025

DrahtBot commented at 8:11 PM on December 10, 2025: contributor

🚧 At least one of the CI tasks failed. Task fuzzer,address,undefined,integer: https://github.com/bitcoin/bitcoin/actions/runs/20111780022/job/57710791590 LLM reason (✨ experimental): Compilation failed due to a type mismatch: ExportDescriptors returns util::Expected<vector<WalletDescInfo>, string>, which cannot be converted to util::Result<vector<WalletDescInfo>> in wallet.cpp.

<details><summary>Hints</summary>

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.

</details>

achow101 force-pushed on Dec 10, 2025

DrahtBot removed the label CI failed on Dec 11, 2025

DrahtBot added the label Needs rebase on Dec 17, 2025

achow101 force-pushed on Dec 22, 2025

DrahtBot removed the label Needs rebase on Dec 22, 2025

DrahtBot added the label CI failed on Dec 23, 2025

DrahtBot removed the label CI failed on Dec 26, 2025

DrahtBot added the label Needs rebase on Jan 3, 2026

achow101 force-pushed on Jan 3, 2026

DrahtBot removed the label Needs rebase on Jan 3, 2026

DrahtBot added the label Needs rebase on Jan 20, 2026

achow101 force-pushed on Jan 22, 2026

DrahtBot removed the label Needs rebase on Jan 22, 2026

in src/script/descriptor.h:134 in e17067d280 outdated

 129 | @@ -130,6 +130,9 @@ struct Descriptor {
 130 |      /** Convert the descriptor to a normalized string. Normalized descriptors have the xpub at the last hardened step. This fails if the provided provider does not have the private keys to derive that xpub. */
 131 |      virtual bool ToNormalizedString(const SigningProvider& provider, std::string& out, const DescriptorCache* cache = nullptr) const = 0;
 132 |  
 133 | +    /** Whether the descriptor can be used to get more addresses without needing a cache or private keys. */
 134 | +    virtual bool CanSelfExpand() const = 0;

Eunovo commented at 3:13 PM on January 27, 2026:

Consider adding unit tests for CanSelfExpand() to descriptor_tests.cpp to properly cover all descriptors

achow101 commented at 11:57 PM on January 27, 2026:

Done

in src/wallet/wallet.cpp:4692 in a5bc048333

4687 | +                // This is only set for active spkms
4688 | +                bool internal = false;
4689 | +                if (desc_info.internal) {
4690 | +                    internal = *desc_info.internal;
4691 | +                }
4692 | +                watchonly_wallet->AddActiveScriptPubKeyMan(desc_id, *w_desc.descriptor->GetOutputType(), internal);

Eunovo commented at 4:22 PM on January 27, 2026:

https://github.com/bitcoin/bitcoin/pull/32489/changes/a5bc048333be6c46b886d5f8a5701c103d75059c:

Consider asserting that w_desc.descriptor->GetOutputType() has a value before deferencing.

achow101 commented at 11:57 PM on January 27, 2026:

Done

achow101 force-pushed on Jan 27, 2026

DrahtBot added the label CI failed on Jan 27, 2026

achow101 force-pushed on Jan 28, 2026

DrahtBot removed the label CI failed on Jan 28, 2026

Eunovo commented at 1:58 PM on January 30, 2026: contributor

ACK https://github.com/bitcoin/bitcoin/pull/32489/changes/1caddd47d60b72e22caedeb4c5bfa395411a735b

DrahtBot requested review from polespinasa on Jan 30, 2026

DrahtBot requested review from rkrux on Jan 30, 2026

DrahtBot requested review from Sjors on Jan 30, 2026

Eunovo commented at 2:15 PM on February 20, 2026: contributor

Before the PR, it was not possible for a watch-only wallet to generate addresses past the range specified in the imported descriptors. Now, with the introduction of CanSelfExpand(), a scenario where the watch-only node generates addresses past the offline signing node's keypool is possible. When this happens, the offline signing node cannot sign the PSBT because it does not recognise the scripts and pubkeys in the PSBT.

The user can manually call keypoolrefill to remedy this, but I think it should be possible to use the hd_paths provided with the inputs in the PSBT to automatically top-up the keypool during walletprocesspsbt.

This may be out of scope for this PR, so I'm happy to open a separate PR to explore this after this is merged.

DrahtBot added the label Needs rebase on Feb 21, 2026

achow101 force-pushed on Feb 24, 2026

DrahtBot removed the label Needs rebase on Feb 25, 2026

in src/wallet/wallet.cpp:4661 in e23b8d5111 outdated

4656 | +    empty_context.args = context.args;
4657 | +    std::shared_ptr<CWallet> watchonly_wallet = CWallet::CreateNew(empty_context, wallet_name, std::move(database), options.create_flags, error, warnings);
4658 | +    if (!watchonly_wallet) {
4659 | +        return util::Error{_("Error: Failed to create new watchonly wallet")};
4660 | +    }
4661 | +

adyshimony commented at 6:56 PM on March 2, 2026:

We can use RAII approach here to cover all cases:

// Always remove the temporary wallet files, even when returning early on error.
auto cleanup = interfaces::MakeCleanupHandler([&watchonly_wallet] {
    if (!watchonly_wallet) return;
    fs::path wallet_path = fs::PathFromString(watchonly_wallet->GetDatabase().Filename()).parent_path();
    std::vector<fs::path> files = watchonly_wallet->GetDatabase().Files();
    watchonly_wallet.reset();
    for (const auto& file : files) {
        fs::remove(file);
    }
    fs::remove(wallet_path);
});

achow101 commented at 10:03 PM on March 23, 2026:

Taken the suggestion

in src/wallet/wallet.cpp:4772 in e23b8d5111 outdated

4767 | +        if (!watchonly_wallet->BackupWallet(fs::PathToString(destination))) {
4768 | +            return util::Error{_("Error: Unable to write the exported wallet")};
4769 | +        }
4770 | +    }
4771 | +
4772 | +    // Delete the watchonly wallet now that it has been exported to the desired location

adyshimony commented at 7:02 PM on March 2, 2026:

If we use the RAII approach, we can remove this clean up.

achow101 commented at 10:03 PM on March 23, 2026:

Done

adyshimony commented at 7:32 PM on March 2, 2026: none

e23b8d51112c reviewed and tested

Found one issue in exportwatchonlywallet error handling.

If export fails in the middle of writing (destination disappears), RPC returns Error: Unable to write the exported wallet, but temp wallet data can be left behind under wallets/<name>_watchonly_temp/.

Repro:

Create a descriptor wallet on regtest. Start exportwatchonlywallet to a droppable path. Remove/unmount destination during export. The temp wallet stays in _watchonly_temp.

Suggested solution:

Always clean up the temp wallet in CWallet::ExportWatchOnlyWallet (including early returns), using an RAII approach.

DrahtBot added the label Needs rebase on Mar 2, 2026

achow101 force-pushed on Mar 13, 2026

DrahtBot removed the label Needs rebase on Mar 13, 2026

sedited commented at 7:35 AM on March 23, 2026: contributor

Ping for previous reviewers of this PR given the amount of stale approvals.

achow101 force-pushed on Mar 23, 2026

in src/script/descriptor.cpp:803 in 3a2a88fa67 outdated

 799 | @@ -794,6 +800,13 @@ class MuSigPubkeyProvider final : public PubkeyProvider
 800 |      {
 801 |          return 1 + m_participants.size();
 802 |      }
 803 | +    bool CanSelfExpand() const override

Eunovo commented at 2:47 PM on April 16, 2026:

https://github.com/bitcoin/bitcoin/pull/32489/commits/3a2a88fa67ed4f9a586144ea93a38e20cfb8d510:

descriptor_tests passes even when participating keys cannot self-expand. Reproduce applying this diff and running cmake --build build -j 14 && build/bin/test_bitcoin --run_test=descriptor_tests

diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index b895edd033..536b4efbdb 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -802,9 +802,9 @@ public:
     }
     bool CanSelfExpand() const override
     {
-        for (const auto& key : m_participants) {
-            if (!key->CanSelfExpand()) return false;
-        }
+        // for (const auto& key : m_participants) {
+        //     if (!key->CanSelfExpand()) return false;
+        // }
         return true;
     }
 };

Adding a musig descriptor to descriptor_tests with hardened paths fixes this.

diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp
index c9e156cdf5..b0c9883cf3 100644
--- a/src/test/descriptor_tests.cpp
+++ b/src/test/descriptor_tests.cpp
@@ -1168,6 +1168,7 @@ BOOST_AUTO_TEST_CASE(descriptor_test)
     Check("tr(musig(KwDiBf89QgGbjEhKnhXJuH7LrciVrZi3qYjgd9M7rFU74sHUHy8S,03dff1d77f2a671c5f36183726db2341be58feae1da2deced843240f7b502ba659,023590a94e768f8e1815c2f24b4d80a8e3149316c3518ce7b7ad338368d038ca66))", "tr(musig(02f9308a019258c31049344f85f89d5229b531c845836f99b08601f113bce036f9,03dff1d77f2a671c5f36183726db2341be58feae1da2deced843240f7b502ba659,023590a94e768f8e1815c2f24b4d80a8e3149316c3518ce7b7ad338368d038ca66))", "tr(musig(02f9308a019258c31049344f85f89d5229b531c845836f99b08601f113bce036f9,03dff1d77f2a671c5f36183726db2341be58feae1da2deced843240f7b502ba659,023590a94e768f8e1815c2f24b4d80a8e3149316c3518ce7b7ad338368d038ca66))", XONLY_KEYS | MUSIG, {{"512079e6c3e628c9bfbce91de6b7fb28e2aec7713d377cf260ab599dcbc40e542312"}}, OutputType::BECH32M);
     Check("rawtr(musig(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc/0/*,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y/0/*))","rawtr(musig(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/0/*,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y/0/*))","rawtr(musig(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/0/*,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y/0/*))", XONLY_KEYS | RANGE | MUSIG, {{"5120754ccfd18ed4051de3b1144b6145cad4b2999387338dfb85ec392f2963ceaa3a"}, {"5120be80016576d2691ccc4077bc91d7ece4db34667d6e84829d5e08480cd4bc0b78"}, {"5120b7139e2f8b92570ad96c40c3b5e6557a5194e288a96df6f29980523365239d58"}}, OutputType::BECH32M, /*op_desc_id=*/std::nullopt, {{}, {0, 0}, {0, 1}, {0, 2}});
     Check("rawtr(musig(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y)/0/*)","rawtr(musig(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y)/0/*)","rawtr(musig(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y)/0/*)", XONLY_KEYS | RANGE | MUSIG | MUSIG_DERIVATION, {{"51209508c08832f3bb9d5e8baf8cb5cfa3669902e2f2da19acea63ff47b93faa9bfc"}, {"51205ca1102663025a83dd9b5dbc214762c5a6309af00d48167d2d6483808525a298"}, {"51207dbed1b89c338df6a1ae137f133a19cae6e03d481196ee6f1a5c7d1aeb56b166"}}, OutputType::BECH32M, /*op_desc_id=*/std::nullopt, {{}, {0, 0}, {0, 1}, {0, 2}});
+    Check("rawtr(musig(xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/2147483647'/0,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y)/1)","rawtr(musig(xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/2147483647'/0,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y)/1)","rawtr(musig([bd16bee5/2147483647h]xpub69H7F5dQzmVd3vPuLKtcXJziMEQByuDidnX3YdwgtNsecY5HRGtAAQC5mXTt4dsv9RzyjgDjAQs9VGVV6ydYCHnprc9vvaA5YtqWyL6hyds/0,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y)/1)", HARDENED | XONLY_KEYS | MUSIG | MUSIG_DERIVATION, {{"5120ebf2bcce516ef6567a9001ce6e5dc43a02bb62d37b51d86d773fa96dcd3a8d4c"}}, OutputType::BECH32M, /*op_desc_id=*/std::nullopt, {{}, {0xFFFFFFFFUL,0}, {1}});
     Check("rawtr(musig(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc/0,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y)/1)","rawtr(musig(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/0,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y)/1)","rawtr(musig(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/0,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y)/1)", XONLY_KEYS | MUSIG | MUSIG_DERIVATION, {{"51200e355f2bc9e754268e12bbd337499c2f7ffafc3101c41792709007b25a862532"}}, OutputType::BECH32M, /*op_desc_id=*/std::nullopt, {{}, {0}, {1}});
     Check("tr(musig(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y)/0/*,pk(KwDiBf89QgGbjEhKnhXJuH7LrciVrZi3qYjgd9M7rFU74sHUHy8S))","tr(musig(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y)/0/*,pk(f9308a019258c31049344f85f89d5229b531c845836f99b08601f113bce036f9))","tr(musig(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y)/0/*,pk(f9308a019258c31049344f85f89d5229b531c845836f99b08601f113bce036f9))", XONLY_KEYS | RANGE | MUSIG | MUSIG_DERIVATION, {{"51201d377b637b5c73f670f5c8a96a2c0bb0d1a682a1fca6aba91fe673501a189782"}, {"51208950c83b117a6c208d5205ffefcf75b187b32512eb7f0d8577db8d9102833036"}, {"5120a49a477c61df73691b77fcd563a80a15ea67bb9c75470310ce5c0f25918db60d"}}, OutputType::BECH32M, /*op_desc_id=*/std::nullopt, {{}, {0, 0}, {0, 1}, {0, 2}});
     Check("tr(KwDiBf89QgGbjEhKnhXJuH7LrciVrZi3qYjgd9M7rFU74sHUHy8S,pk(musig(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y)/0/*))","tr(f9308a019258c31049344f85f89d5229b531c845836f99b08601f113bce036f9,pk(musig(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y)/0/*))","tr(f9308a019258c31049344f85f89d5229b531c845836f99b08601f113bce036f9,pk(musig(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y)/0/*))", XONLY_KEYS | RANGE | MUSIG | MUSIG_DERIVATION, {{"512068983d461174afc90c26f3b2821d8a9ced9534586a756763b68371a404635cc8"}, {"5120368e2d864115181bdc8bb5dc8684be8d0760d5c33315570d71a21afce4afd43e"}, {"512097a1e6270b33ad85744677418bae5f59ea9136027223bc6e282c47c167b471d5"}}, OutputType::BECH32M, /*op_desc_id=*/std::nullopt, {{}, {0, 0}, {0, 1}, {0, 2}});

achow101 commented at 7:53 PM on April 20, 2026:

Done

in src/wallet/wallet.cpp:4616 in 1ab59d2c85 outdated

4611 | +        std::string descriptor;
4612 | +        if (!Assume(desc_spk_man->GetDescriptorString(descriptor, export_private))) {
4613 | +            return util::Unexpected{"Can't get descriptor string."};
4614 | +        }
4615 | +        const bool is_range = wallet_descriptor.descriptor->IsRange();
4616 | +        wallet_descriptors.push_back({

Eunovo commented at 2:58 PM on April 16, 2026:

https://github.com/bitcoin/bitcoin/pull/32489/commits/1ab59d2c853810872b245fcbfce52ea4826e66db:

Consider switching to emplace_back.

achow101 commented at 7:53 PM on April 20, 2026:

Done

achow101 force-pushed on Apr 20, 2026

DrahtBot added the label CI failed on Apr 20, 2026

DrahtBot commented at 8:14 PM on April 20, 2026: contributor

🚧 At least one of the CI tasks failed. Task Windows-cross to x86_64, msvcrt: https://github.com/bitcoin/bitcoin/actions/runs/24687134412/job/72199488115 LLM reason (✨ experimental): Build failed due to C++ compilation errors in wallet.cpp (missing/undefined RPCHelpMan, causing exportwatchonlywallet and the RPC command initializer to fail).

<details><summary>Hints</summary>

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.

</details>

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

2f496d038e

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.

056a35feb0

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.

b3ba2420e5

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.

e01da1cee2

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.

f41cbe8f4e

wallet, rpc: Add exportwatchonlywallet RPC 86e2b1ae3c

test: Test for exportwatchonlywallet 764d0218dd

doc: update offline-signing-tutorial to use exportwatchonlywallet rpc fc85c444e8

achow101 force-pushed on Apr 20, 2026

DrahtBot removed the label CI failed on Apr 20, 2026

Eunovo commented at 12:02 PM on April 29, 2026: contributor

Tested ACK https://github.com/bitcoin/bitcoin/pull/32489/commits/fc85c444e88bfad4e16604d2273ef5c609efc33a

The exportwatchonlywallet RPC works as expected, and I also tested backwards compatibility with this diff:

diff --git a/test/functional/wallet_backwards_compatibility.py b/test/functional/wallet_backwards_compatibility.py
index a67b4676a8..53b64c8dd2 100755
--- a/test/functional/wallet_backwards_compatibility.py
+++ b/test/functional/wallet_backwards_compatibility.py
@@ -359,8 +359,12 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
 
             # Make backup so the wallet can be copied back to old node
             down_wallet_name = f"re_down_{node.version}"
+            wo_down_wallet_name = f"re_wodown_{node.version}"
             down_backup_path = os.path.join(self.options.tmpdir, f"{down_wallet_name}.dat")
+            wo_down_backup_path = os.path.join(self.options.tmpdir, f"{wo_down_wallet_name}.dat")
+
             wallet.backupwallet(down_backup_path)
+            wallet.exportwatchonlywallet(wo_down_backup_path)
 
             # Check that taproot descriptors can be added to 0.21 wallets
             # This must be done after the backup is created so that 0.21 can still load
@@ -395,19 +399,22 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
                     lh_cache_recs = conn.execute(f"SELECT value FROM main where key >= x'{ser_string(b'walletdescriptorlhcache').hex()}' AND key < x'{ser_string(b'walletdescriptorlhcachf').hex()}'").fetchall()
                     assert_greater_than(len(lh_cache_recs), 0)
 
-            self.inspect_sqlite_db(down_backup_path, check_upgraded_records, old_flags)
-
             # Check that no automatic upgrade broke downgrading the wallet
-            target_dir = node.wallets_path / down_wallet_name
-            os.makedirs(target_dir, exist_ok=True)
-            shutil.copyfile(
-                down_backup_path,
-                target_dir / "wallet.dat"
-            )
-            node.loadwallet(down_wallet_name)
-            wallet_res = node.get_wallet_rpc(down_wallet_name)
-            info = wallet_res.getaddressinfo(address)
-            assert_equal(info, addr_info)
+            for (wallet_name, backup_path) in [
+                (down_wallet_name, down_backup_path),
+                (wo_down_wallet_name, wo_down_backup_path)
+            ]:
+                # self.inspect_sqlite_db(backup_path, check_upgraded_records, old_flags)
+                target_dir = node.wallets_path / wallet_name
+                os.makedirs(target_dir, exist_ok=True)
+                shutil.copyfile(
+                    backup_path,
+                    target_dir / "wallet.dat"
+                )
+                node.loadwallet(wallet_name)
+                wallet_res = node.get_wallet_rpc(wallet_name)
+                info = wallet_res.getaddressinfo(address)
+                assert_equal(info, addr_info)
 
         self.log.info("Test that a wallet from a legacy only node must be migrated, from:")
         for node in legacy_nodes:

in src/wallet/wallet.cpp:4628 in e01da1cee2

4623 | +            is_range ? std::optional(std::make_pair(wallet_descriptor.range_start, wallet_descriptor.range_end)) : std::nullopt,
4624 | +            wallet_descriptor.next_index
4625 | +        );
4626 | +    }
4627 | +    return wallet_descriptors;
4628 | +}

rkrux commented at 2:03 PM on May 8, 2026:

In e01da1cee2af62e5fbce527f2cbcb2521c19af34 "wallet: Move listdescriptors retrieving from RPC to CWallet"

I think we should continue with the approach that has been decided in #34861 and avoid adding more lengthy code in wallet/wallet.cpp. PR #34681 also takes a similar approach to extract out chain scanning code, PR #34909 tries to do the same for wallet migration.

Along with the wallet helper classes and functions that act as a support for CWallet, I believe wallet functionalities that are rarely executed by the user also makes for a good reason to not have them in the main wallet/wallet.cpp file that contains code that's regularly triggered while using the wallet - I feel that the exporting of the watch only wallet falls under the same scope.

The following diff creates a walllet/export.cpp|h and moves ExportDescriptors there while making it not a method of CWallet.

<details> <summary>Extract out ExportDescriptors in wallet/export.cpp|h</summary>

commit 85ecab6f9296b696ccef9ee397c8929fda1042f6
Author: rkrux <rkrux.connect@gmail.com>
Date:   Fri May 8 19:07:49 2026 +0530

    wallet: move out exportdescriptors

diff --git a/src/wallet/CMakeLists.txt b/src/wallet/CMakeLists.txt
index 36fd3ef95a..040569f7d6 100644
--- a/src/wallet/CMakeLists.txt
+++ b/src/wallet/CMakeLists.txt
@@ -10,6 +10,7 @@ add_library(bitcoin_wallet STATIC EXCLUDE_FROM_ALL
   crypter.cpp
   db.cpp
   dump.cpp
+  export.cpp
   external_signer_scriptpubkeyman.cpp
   feebumper.cpp
   fees.cpp
diff --git a/src/wallet/export.cpp b/src/wallet/export.cpp
new file mode 100644
index 0000000000..964384f09a
--- /dev/null
+++ b/src/wallet/export.cpp
@@ -0,0 +1,32 @@
+#include <wallet/export.h>
+#include <wallet/wallet.h>
+
+namespace wallet {
+util::Expected<std::vector<WalletDescInfo>, std::string> ExportDescriptors(const CWallet &wallet, bool export_private)
+{
+    AssertLockHeld(wallet.cs_wallet);
+    std::vector<WalletDescInfo> wallet_descriptors;
+    for (const auto& spk_man : wallet.GetAllScriptPubKeyMans()) {
+        const auto desc_spk_man = dynamic_cast<DescriptorScriptPubKeyMan*>(spk_man);
+        if (!desc_spk_man) {
+            return util::Unexpected{"Unexpected ScriptPubKey manager type."};
+        }
+        LOCK(desc_spk_man->cs_desc_man);
+        const auto& wallet_descriptor = desc_spk_man->GetWalletDescriptor();
+        std::string descriptor;
+        if (!Assume(desc_spk_man->GetDescriptorString(descriptor, export_private))) {
+            return util::Unexpected{"Can't get descriptor string."};
+        }
+        const bool is_range = wallet_descriptor.descriptor->IsRange();
+        wallet_descriptors.emplace_back(
+            descriptor,
+            wallet_descriptor.creation_time,
+            wallet.IsActiveScriptPubKeyMan(*desc_spk_man),
+            wallet.IsInternalScriptPubKeyMan(desc_spk_man),
+            is_range ? std::optional(std::make_pair(wallet_descriptor.range_start, wallet_descriptor.range_end)) : std::nullopt,
+            wallet_descriptor.next_index
+        );
+    }
+    return wallet_descriptors;
+}
+}
\ No newline at end of file
diff --git a/src/wallet/export.h b/src/wallet/export.h
new file mode 100644
index 0000000000..00ffd97b38
--- /dev/null
+++ b/src/wallet/export.h
@@ -0,0 +1,24 @@
+#ifndef BITCOIN_WALLET_EXPORT_H
+#define BITCOIN_WALLET_EXPORT_H
+
+#include <wallet/wallet.h>
+
+namespace wallet {
+// Struct containing all of the info from WalletDescriptor, except with the descriptor as a string,
+// and without its ID or cache.
+// Used when exporting descriptors from the wallet.
+struct WalletDescInfo {
+    std::string descriptor;
+    uint64_t creation_time;
+    bool active;
+    std::optional<bool> internal;
+    std::optional<std::pair<int64_t,int64_t>> range;
+    int64_t next_index;
+};
+
+//! Export the descriptors from the wallet so that they can be imported elsewhere
+util::Expected<std::vector<WalletDescInfo>, std::string> ExportDescriptors(const CWallet &wallet, bool export_private) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
+
+} // namespace wallet
+
+#endif // BITCOIN_WALLET_EXPORT_H
\ No newline at end of file
diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index 7a0085b2d7..5274001a1e 100644
--- a/src/wallet/rpc/backup.cpp
+++ b/src/wallet/rpc/backup.cpp
@@ -21,6 +21,7 @@
 #include <util/fs.h>
 #include <util/time.h>
 #include <util/translation.h>
+#include <wallet/export.h>
 #include <wallet/rpc/util.h>
 #include <wallet/wallet.h>
 
@@ -506,7 +507,7 @@ RPCMethod listdescriptors()
     }
 
     LOCK(wallet->cs_wallet);
-    util::Expected<std::vector<WalletDescInfo>, std::string> exported = wallet->ExportDescriptors(priv);
+    util::Expected<std::vector<WalletDescInfo>, std::string> exported = ExportDescriptors(*wallet, priv);
     if (!exported) {
         throw JSONRPCError(RPC_WALLET_ERROR, exported.error());
     }
diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp
index 9500eb510e..d7327002fa 100644
--- a/src/wallet/rpc/wallet.cpp
+++ b/src/wallet/rpc/wallet.cpp
@@ -356,7 +356,7 @@ static RPCMethod createwallet()
             {"avoid_reuse", RPCArg::Type::BOOL, RPCArg::Default{false}, "Keep track of coin reuse, and treat dirty and clean coins differently with privacy considerations in mind."},
             {"descriptors", RPCArg::Type::BOOL, RPCArg::Default{true}, "If set, must be \"true\""},
             {"load_on_startup", RPCArg::Type::BOOL, RPCArg::Optional::OMITTED, "Save wallet name to persistent settings and load on startup. True to add wallet to startup list, false to remove, null to leave unchanged."},
-            {"external_signer", RPCArg::Type::BOOL, RPCArg::Default{false}, "Use an external signer such as a hardware wallet. Requires -signer to be configured. Wallet creation will fail if keys cannot be fetched. Requires disable_private_keys and descriptors set to true."},
+            {"external_signer", RPCArg::Type::BOOL, RPCArg::Default{false}, "Use an external signer such as a hardware wallet. Requires ExportWatchOnlyWallet to be configured. Wallet creation will fail if keys cannot be fetched. Requires disable_private_keys and descriptors set to true."},
         },
         RPCResult{
             RPCResult::Type::OBJ, "", "",
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index f8011494bd..b1febf79f6 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -64,6 +64,7 @@
 #include <wallet/context.h>
 #include <wallet/crypter.h>
 #include <wallet/db.h>
+#include <wallet/export.h>
 #include <wallet/external_signer_scriptpubkeyman.h>
 #include <wallet/scriptpubkeyman.h>
 #include <wallet/transaction.h>
@@ -4600,34 +4601,6 @@ void CWallet::DisconnectChainNotifications()
     }
 }
 
-util::Expected<std::vector<WalletDescInfo>, std::string> CWallet::ExportDescriptors(bool export_private) const
-{
-    AssertLockHeld(cs_wallet);
-    std::vector<WalletDescInfo> wallet_descriptors;
-    for (const auto& spk_man : GetAllScriptPubKeyMans()) {
-        const auto desc_spk_man = dynamic_cast<DescriptorScriptPubKeyMan*>(spk_man);
-        if (!desc_spk_man) {
-            return util::Unexpected{"Unexpected ScriptPubKey manager type."};
-        }
-        LOCK(desc_spk_man->cs_desc_man);
-        const auto& wallet_descriptor = desc_spk_man->GetWalletDescriptor();
-        std::string descriptor;
-        if (!Assume(desc_spk_man->GetDescriptorString(descriptor, export_private))) {
-            return util::Unexpected{"Can't get descriptor string."};
-        }
-        const bool is_range = wallet_descriptor.descriptor->IsRange();
-        wallet_descriptors.emplace_back(
-            descriptor,
-            wallet_descriptor.creation_time,
-            IsActiveScriptPubKeyMan(*desc_spk_man),
-            IsInternalScriptPubKeyMan(desc_spk_man),
-            is_range ? std::optional(std::make_pair(wallet_descriptor.range_start, wallet_descriptor.range_end)) : std::nullopt,
-            wallet_descriptor.next_index
-        );
-    }
-    return wallet_descriptors;
-}
-
 util::Result<std::string> CWallet::ExportWatchOnlyWallet(const fs::path& destination, WalletContext& context) const
 {
     AssertLockHeld(cs_wallet);
@@ -4644,7 +4617,7 @@ util::Result<std::string> CWallet::ExportWatchOnlyWallet(const fs::path& destina
     fs::remove(destination);
 
     // Get the descriptors from this wallet
-    util::Expected<std::vector<WalletDescInfo>, std::string> exported = ExportDescriptors(/*export_private=*/false);
+    util::Expected<std::vector<WalletDescInfo>, std::string> exported = ExportDescriptors(*this, /*export_private=*/false);
     if (!exported) {
         return util::Error{Untranslated(exported.error())};
     }
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 01e8652240..b92a63f37c 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -302,18 +302,6 @@ struct CRecipient
     bool fSubtractFeeFromAmount;
 };
 
-// Struct containing all of the info from WalletDescriptor, except with the descriptor as a string,
-// and without its ID or cache.
-// Used when exporting descriptors from the wallet.
-struct WalletDescInfo {
-    std::string descriptor;
-    uint64_t creation_time;
-    bool active;
-    std::optional<bool> internal;
-    std::optional<std::pair<int64_t,int64_t>> range;
-    int64_t next_index;
-};
-
 class WalletRescanReserver; //forward declarations for ScanForWalletTransactions/RescanFromTime
 /**
  * A CWallet maintains a set of transactions and balances, and provides the ability to create new transactions.
@@ -1088,9 +1076,6 @@ public:
     //! Disconnect chain notifications and wait for all notifications to be processed
     void DisconnectChainNotifications();
 
-    //! Export the descriptors from this wallet so that they can be imported elsewhere
-    util::Expected<std::vector<WalletDescInfo>, std::string> ExportDescriptors(bool export_private) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
-
     //! Make a new watchonly wallet file containing the public descriptors from this wallet
     //! The exported watchonly wallet file will be named and placed at the path specified in 'destination'
     util::Result<std::string> ExportWatchOnlyWallet(const fs::path& destination, WalletContext& context) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

</details>

in src/wallet/wallet.cpp:4631 in f41cbe8f4e

4626 | @@ -4626,4 +4627,175 @@ util::Expected<std::vector<WalletDescInfo>, std::string> CWallet::ExportDescript
4627 |      }
4628 |      return wallet_descriptors;
4629 |  }
4630 | +
4631 | +util::Result<std::string> CWallet::ExportWatchOnlyWallet(const fs::path& destination, WalletContext& context) const

rkrux commented at 2:10 PM on May 8, 2026:

In f41cbe8f4e7b9e6edef827d7e4f78bcb1b6640d3 "wallet: Add CWallet::ExportWatchOnly"

Similar to previous comment of extracting out, ExportWatchOnlyWallet can also be present in wallet/export.cpp while not being a method of CWallet. This method uses only the public members of CWallet that a CWallet variable can easily use.

<details> <summary>Extract out ExportWatchOnlyWallet in wallet/export.cpp|h</summary>

commit a2fae2fdd7792bf7f4211e2b0d9da500b89a0dec
Author: rkrux <rkrux.connect@gmail.com>
Date:   Fri May 8 19:22:17 2026 +0530

    wallet: move out ExportWatchOnlyWallet method

diff --git a/src/wallet/export.cpp b/src/wallet/export.cpp
index 964384f09a..5db5370278 100644
--- a/src/wallet/export.cpp
+++ b/src/wallet/export.cpp
@@ -1,3 +1,6 @@
+#include <fstream>
+
+#include <wallet/context.h>
 #include <wallet/export.h>
 #include <wallet/wallet.h>
 
@@ -29,4 +32,175 @@ util::Expected<std::vector<WalletDescInfo>, std::string> ExportDescriptors(const
     }
     return wallet_descriptors;
 }
+
+util::Result<std::string> ExportWatchOnlyWallet(const CWallet &wallet, const fs::path& destination, WalletContext& context)
+{
+    AssertLockHeld(wallet.cs_wallet);
+
+    if (destination.empty()) {
+        return util::Error{_("Error: Export destination cannot be empty")};
+    }
+    if (fs::exists(destination)) {
+        return util::Error{strprintf(_("Error: Export destination '%s' already exists"), fs::PathToString(destination))};
+    }
+    if (!std::ofstream{fs::PathToString(destination)}) {
+        return util::Error{strprintf(_("Error: Could not create file '%s'"), fs::PathToString(destination))};
+    }
+    fs::remove(destination);
+
+    // Get the descriptors from this wallet
+    util::Expected<std::vector<WalletDescInfo>, std::string> exported = ExportDescriptors(wallet, /*export_private=*/false);
+    if (!exported) {
+        return util::Error{Untranslated(exported.error())};
+    }
+
+    // Setup DatabaseOptions to create a new sqlite database
+    DatabaseOptions options;
+    options.require_existing = false;
+    options.require_create = true;
+    options.require_format = DatabaseFormat::SQLITE;
+
+    // Make the wallet with the same flags as this wallet, but without private keys
+    options.create_flags = wallet.GetWalletFlags() | WALLET_FLAG_DISABLE_PRIVATE_KEYS;
+
+    // Make the watchonly wallet
+    DatabaseStatus status;
+    std::vector<bilingual_str> warnings;
+    std::string wallet_name = wallet.GetName() + "_watchonly_temp";
+    bilingual_str error;
+    std::unique_ptr<WalletDatabase> database = MakeWalletDatabase(wallet_name, options, status, error);
+    if (!database) {
+        return util::Error{strprintf(_("Wallet file creation failed: %s"), error)};
+    }
+    WalletContext empty_context;
+    empty_context.args = context.args;
+    std::shared_ptr<CWallet> watchonly_wallet = CWallet::CreateNew(empty_context, wallet_name, std::move(database), options.create_flags, error, warnings);
+    if (!watchonly_wallet) {
+        return util::Error{_("Error: Failed to create new watchonly wallet")};
+    }
+
+    // Always remove the temporary wallet files, even when returning early on error.
+    auto cleanup = interfaces::MakeCleanupHandler([&watchonly_wallet] {
+        if (!watchonly_wallet) return;
+        fs::path wallet_path = fs::PathFromString(watchonly_wallet->GetDatabase().Filename()).parent_path();
+        std::vector<fs::path> files = watchonly_wallet->GetDatabase().Files();
+        watchonly_wallet.reset();
+        for (const auto& file : files) {
+            fs::remove(file);
+        }
+        fs::remove(wallet_path);
+    });
+
+    {
+        LOCK(watchonly_wallet->cs_wallet);
+
+        // Parse the descriptors and add them to the new wallet
+        for (const WalletDescInfo& desc_info : *Assert(exported)) {
+            // Parse the descriptor
+            FlatSigningProvider dummy_keys;
+            std::string dummy_err;
+            std::vector<std::unique_ptr<Descriptor>> descs = Parse(desc_info.descriptor, dummy_keys, dummy_err, /*require_checksum=*/true);
+            assert(descs.size() == 1); // All of our descriptors should be valid, and not multipath
+            assert(dummy_keys.keys.size() == 0); // No private keys should be present in our exported descriptors
+
+            // Get the range if there is one
+            int32_t range_start = 0;
+            int32_t range_end = 0;
+            if (desc_info.range) {
+                range_start = desc_info.range->first;
+                range_end = desc_info.range->second;
+            }
+
+            WalletDescriptor w_desc(std::move(descs.at(0)), desc_info.creation_time, range_start, range_end, desc_info.next_index);
+
+            // For descriptors that cannot self expand (i.e. needs private keys or cache), retrieve the cache
+            uint256 desc_id = w_desc.id;
+            if (!w_desc.descriptor->CanSelfExpand()) {
+                DescriptorScriptPubKeyMan* desc_spkm = dynamic_cast<DescriptorScriptPubKeyMan*>(wallet.GetScriptPubKeyMan(desc_id));
+                w_desc.cache = WITH_LOCK(desc_spkm->cs_desc_man, return desc_spkm->GetWalletDescriptor().cache);
+            }
+
+            // Add to the watchonly wallet
+            if (auto spkm_res = watchonly_wallet->AddWalletDescriptor(w_desc, dummy_keys, /*label=*/"", /*internal=*/false); !spkm_res) {
+                return util::Error{util::ErrorString(spkm_res)};
+            }
+
+            // Set active spkms as active
+            if (desc_info.active) {
+                // Determine whether this descriptor is internal
+                // This is only set for active spkms
+                bool internal = false;
+                if (desc_info.internal) {
+                    internal = *desc_info.internal;
+                }
+                watchonly_wallet->AddActiveScriptPubKeyMan(desc_id, *Assert(w_desc.descriptor->GetOutputType()), internal);
+            }
+        }
+
+        // Copy locked coins that are persisted
+        for (const auto& [coin, persisted] : wallet.m_locked_coins) {
+            if (!persisted) continue;
+            watchonly_wallet->LockCoin(coin, persisted);
+        }
+
+        {
+            // Make a WalletBatch for the watchonly wallet so that everything else can be written atomically
+            WalletBatch watchonly_batch(watchonly_wallet->GetDatabase());
+            if (!watchonly_batch.TxnBegin()) {
+                return util::Error{strprintf(_("Error: database transaction cannot be executed for new watchonly wallet %s"), watchonly_wallet->GetName())};
+            }
+
+            // Copy orderPosNext
+            watchonly_batch.WriteOrderPosNext(wallet.nOrderPosNext);
+
+            // Write the best block locator to avoid rescanning on reload
+            CBlockLocator best_block_locator;
+            {
+                WalletBatch local_wallet_batch(wallet.GetDatabase());
+                if (!local_wallet_batch.ReadBestBlock(best_block_locator)) {
+                    return util::Error{_("Error: Unable to read wallet's best block locator record")};
+                }
+            }
+            if (!watchonly_batch.WriteBestBlock(best_block_locator)) {
+                return util::Error{_("Error: Unable to write watchonly wallet best block locator record")};
+            }
+
+            // Copy the transactions
+            for (const auto& [txid, wtx] : wallet.mapWallet) {
+                const CWalletTx& to_copy_wtx = wtx;
+                if (!watchonly_wallet->LoadToWallet(txid, [&](CWalletTx& ins_wtx, bool new_tx) EXCLUSIVE_LOCKS_REQUIRED(watchonly_wallet->cs_wallet) {
+                    if (!new_tx) return false;
+                    ins_wtx.SetTx(to_copy_wtx.tx);
+                    ins_wtx.CopyFrom(to_copy_wtx);
+                    return true;
+                })) {
+                    return util::Error{strprintf(_("Error: Could not add tx %s to watchonly wallet"), txid.GetHex())};
+                }
+                watchonly_batch.WriteTx(watchonly_wallet->mapWallet.at(txid));
+            }
+
+            // Copy address book
+            for (const auto& [dest, entry] : wallet.m_address_book) {
+                auto address{EncodeDestination(dest)};
+                if (entry.purpose) watchonly_batch.WritePurpose(address, PurposeToString(*entry.purpose));
+                if (entry.label) watchonly_batch.WriteName(address, *entry.label);
+                for (const auto& [id, request] : entry.receive_requests) {
+                    watchonly_batch.WriteAddressReceiveRequest(dest, id, request);
+                }
+                if (entry.previously_spent) watchonly_batch.WriteAddressPreviouslySpent(dest, true);
+            }
+
+            if (!watchonly_batch.TxnCommit()) {
+                return util::Error{_("Error: cannot commit db transaction for watchonly wallet export")};
+            }
+        }
+
+        // Make a backup of this wallet at the specified destination directory
+        if (!watchonly_wallet->BackupWallet(fs::PathToString(destination))) {
+            return util::Error{_("Error: Unable to write the exported wallet")};
+        }
+    }
+
+    return fs::PathToString(destination);
+}
 }
\ No newline at end of file
diff --git a/src/wallet/export.h b/src/wallet/export.h
index 00ffd97b38..3625d90ab7 100644
--- a/src/wallet/export.h
+++ b/src/wallet/export.h
@@ -1,6 +1,8 @@
 #ifndef BITCOIN_WALLET_EXPORT_H
 #define BITCOIN_WALLET_EXPORT_H
 
+#include <key_io.h>
+#include <wallet/context.h>
 #include <wallet/wallet.h>
 
 namespace wallet {
@@ -19,6 +21,10 @@ struct WalletDescInfo {
 //! Export the descriptors from the wallet so that they can be imported elsewhere
 util::Expected<std::vector<WalletDescInfo>, std::string> ExportDescriptors(const CWallet &wallet, bool export_private) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
 
+//! Make a new watchonly wallet file containing the public descriptors from this wallet
+//! The exported watchonly wallet file will be named and placed at the path specified in 'destination'
+util::Result<std::string> ExportWatchOnlyWallet(const CWallet &wallet, const fs::path& destination, WalletContext& context) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
+
 } // namespace wallet
 
 #endif // BITCOIN_WALLET_EXPORT_H
\ No newline at end of file
diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp
index d7327002fa..b5023421d0 100644
--- a/src/wallet/rpc/wallet.cpp
+++ b/src/wallet/rpc/wallet.cpp
@@ -12,6 +12,7 @@
 #include <univalue.h>
 #include <util/translation.h>
 #include <wallet/context.h>
+#include <wallet/export.h>
 #include <wallet/receive.h>
 #include <wallet/rpc/util.h>
 #include <wallet/rpc/wallet.h>
@@ -872,7 +873,7 @@ static RPCMethod exportwatchonlywallet()
 
             LOCK(pwallet->cs_wallet);
             pwallet->TopUpKeyPool();
-            util::Result<std::string> exported = pwallet->ExportWatchOnlyWallet(fs::PathFromString(dest), context);
+            util::Result<std::string> exported = ExportWatchOnlyWallet(*pwallet, fs::PathFromString(dest), context);
             if (!exported) {
                 throw JSONRPCError(RPC_WALLET_ERROR, util::ErrorString(exported).original);
             }
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index b1febf79f6..a573cb8a04 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4600,175 +4600,4 @@ void CWallet::DisconnectChainNotifications()
         m_chain_notifications_handler.reset();
     }
 }
-
-util::Result<std::string> CWallet::ExportWatchOnlyWallet(const fs::path& destination, WalletContext& context) const
-{
-    AssertLockHeld(cs_wallet);
-
-    if (destination.empty()) {
-        return util::Error{_("Error: Export destination cannot be empty")};
-    }
-    if (fs::exists(destination)) {
-        return util::Error{strprintf(_("Error: Export destination '%s' already exists"), fs::PathToString(destination))};
-    }
-    if (!std::ofstream{fs::PathToString(destination)}) {
-        return util::Error{strprintf(_("Error: Could not create file '%s'"), fs::PathToString(destination))};
-    }
-    fs::remove(destination);
-
-    // Get the descriptors from this wallet
-    util::Expected<std::vector<WalletDescInfo>, std::string> exported = ExportDescriptors(*this, /*export_private=*/false);
-    if (!exported) {
-        return util::Error{Untranslated(exported.error())};
-    }
-
-    // Setup DatabaseOptions to create a new sqlite database
-    DatabaseOptions options;
-    options.require_existing = false;
-    options.require_create = true;
-    options.require_format = DatabaseFormat::SQLITE;
-
-    // Make the wallet with the same flags as this wallet, but without private keys
-    options.create_flags = GetWalletFlags() | WALLET_FLAG_DISABLE_PRIVATE_KEYS;
-
-    // Make the watchonly wallet
-    DatabaseStatus status;
-    std::vector<bilingual_str> warnings;
-    std::string wallet_name = GetName() + "_watchonly_temp";
-    bilingual_str error;
-    std::unique_ptr<WalletDatabase> database = MakeWalletDatabase(wallet_name, options, status, error);
-    if (!database) {
-        return util::Error{strprintf(_("Wallet file creation failed: %s"), error)};
-    }
-    WalletContext empty_context;
-    empty_context.args = context.args;
-    std::shared_ptr<CWallet> watchonly_wallet = CWallet::CreateNew(empty_context, wallet_name, std::move(database), options.create_flags, error, warnings);
-    if (!watchonly_wallet) {
-        return util::Error{_("Error: Failed to create new watchonly wallet")};
-    }
-
-    // Always remove the temporary wallet files, even when returning early on error.
-    auto cleanup = interfaces::MakeCleanupHandler([&watchonly_wallet] {
-        if (!watchonly_wallet) return;
-        fs::path wallet_path = fs::PathFromString(watchonly_wallet->GetDatabase().Filename()).parent_path();
-        std::vector<fs::path> files = watchonly_wallet->GetDatabase().Files();
-        watchonly_wallet.reset();
-        for (const auto& file : files) {
-            fs::remove(file);
-        }
-        fs::remove(wallet_path);
-    });
-
-    {
-        LOCK(watchonly_wallet->cs_wallet);
-
-        // Parse the descriptors and add them to the new wallet
-        for (const WalletDescInfo& desc_info : *Assert(exported)) {
-            // Parse the descriptor
-            FlatSigningProvider dummy_keys;
-            std::string dummy_err;
-            std::vector<std::unique_ptr<Descriptor>> descs = Parse(desc_info.descriptor, dummy_keys, dummy_err, /*require_checksum=*/true);
-            assert(descs.size() == 1); // All of our descriptors should be valid, and not multipath
-            assert(dummy_keys.keys.size() == 0); // No private keys should be present in our exported descriptors
-
-            // Get the range if there is one
-            int32_t range_start = 0;
-            int32_t range_end = 0;
-            if (desc_info.range) {
-                range_start = desc_info.range->first;
-                range_end = desc_info.range->second;
-            }
-
-            WalletDescriptor w_desc(std::move(descs.at(0)), desc_info.creation_time, range_start, range_end, desc_info.next_index);
-
-            // For descriptors that cannot self expand (i.e. needs private keys or cache), retrieve the cache
-            uint256 desc_id = w_desc.id;
-            if (!w_desc.descriptor->CanSelfExpand()) {
-                DescriptorScriptPubKeyMan* desc_spkm = dynamic_cast<DescriptorScriptPubKeyMan*>(GetScriptPubKeyMan(desc_id));
-                w_desc.cache = WITH_LOCK(desc_spkm->cs_desc_man, return desc_spkm->GetWalletDescriptor().cache);
-            }
-
-            // Add to the watchonly wallet
-            if (auto spkm_res = watchonly_wallet->AddWalletDescriptor(w_desc, dummy_keys, /*label=*/"", /*internal=*/false); !spkm_res) {
-                return util::Error{util::ErrorString(spkm_res)};
-            }
-
-            // Set active spkms as active
-            if (desc_info.active) {
-                // Determine whether this descriptor is internal
-                // This is only set for active spkms
-                bool internal = false;
-                if (desc_info.internal) {
-                    internal = *desc_info.internal;
-                }
-                watchonly_wallet->AddActiveScriptPubKeyMan(desc_id, *Assert(w_desc.descriptor->GetOutputType()), internal);
-            }
-        }
-
-        // Copy locked coins that are persisted
-        for (const auto& [coin, persisted] : m_locked_coins) {
-            if (!persisted) continue;
-            watchonly_wallet->LockCoin(coin, persisted);
-        }
-
-        {
-            // Make a WalletBatch for the watchonly wallet so that everything else can be written atomically
-            WalletBatch watchonly_batch(watchonly_wallet->GetDatabase());
-            if (!watchonly_batch.TxnBegin()) {
-                return util::Error{strprintf(_("Error: database transaction cannot be executed for new watchonly wallet %s"), watchonly_wallet->GetName())};
-            }
-
-            // Copy orderPosNext
-            watchonly_batch.WriteOrderPosNext(nOrderPosNext);
-
-            // Write the best block locator to avoid rescanning on reload
-            CBlockLocator best_block_locator;
-            {
-                WalletBatch local_wallet_batch(GetDatabase());
-                if (!local_wallet_batch.ReadBestBlock(best_block_locator)) {
-                    return util::Error{_("Error: Unable to read wallet's best block locator record")};
-                }
-            }
-            if (!watchonly_batch.WriteBestBlock(best_block_locator)) {
-                return util::Error{_("Error: Unable to write watchonly wallet best block locator record")};
-            }
-
-            // Copy the transactions
-            for (const auto& [txid, wtx] : mapWallet) {
-                const CWalletTx& to_copy_wtx = wtx;
-                if (!watchonly_wallet->LoadToWallet(txid, [&](CWalletTx& ins_wtx, bool new_tx) EXCLUSIVE_LOCKS_REQUIRED(watchonly_wallet->cs_wallet) {
-                    if (!new_tx) return false;
-                    ins_wtx.SetTx(to_copy_wtx.tx);
-                    ins_wtx.CopyFrom(to_copy_wtx);
-                    return true;
-                })) {
-                    return util::Error{strprintf(_("Error: Could not add tx %s to watchonly wallet"), txid.GetHex())};
-                }
-                watchonly_batch.WriteTx(watchonly_wallet->mapWallet.at(txid));
-            }
-
-            // Copy address book
-            for (const auto& [dest, entry] : m_address_book) {
-                auto address{EncodeDestination(dest)};
-                if (entry.purpose) watchonly_batch.WritePurpose(address, PurposeToString(*entry.purpose));
-                if (entry.label) watchonly_batch.WriteName(address, *entry.label);
-                for (const auto& [id, request] : entry.receive_requests) {
-                    watchonly_batch.WriteAddressReceiveRequest(dest, id, request);
-                }
-                if (entry.previously_spent) watchonly_batch.WriteAddressPreviouslySpent(dest, true);
-            }
-
-            if (!watchonly_batch.TxnCommit()) {
-                return util::Error{_("Error: cannot commit db transaction for watchonly wallet export")};
-            }
-        }
-
-        // Make a backup of this wallet at the specified destination directory
-        if (!watchonly_wallet->BackupWallet(fs::PathToString(destination))) {
-            return util::Error{_("Error: Unable to write the exported wallet")};
-        }
-    }
-
-    return fs::PathToString(destination);
-}
 } // namespace wallet
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index b92a63f37c..32355ed203 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -1075,10 +1075,6 @@ public:
 
     //! Disconnect chain notifications and wait for all notifications to be processed
     void DisconnectChainNotifications();
-
-    //! Make a new watchonly wallet file containing the public descriptors from this wallet
-    //! The exported watchonly wallet file will be named and placed at the path specified in 'destination'
-    util::Result<std::string> ExportWatchOnlyWallet(const fs::path& destination, WalletContext& context) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 };
 
 /**

</details>

rkrux changes_requested

rkrux commented at 2:16 PM on May 8, 2026: contributor

Code review at fc85c444e88bfad4e16604d2273ef5c609efc33a

I think we should take this PR as an opportunity to keep splitting wallet code in multiple modules within the wallet namespace, keeping in line with #34681, #34861, #34909.

Both the suggestions I shared are done on the tip of this branch and the corresponding functional test passes.

DrahtBot requested review from rkrux on May 8, 2026