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

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

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

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

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32489.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	shahsb, Sjors, vicjuma, Eunovo, rkrux, stringintech, enirox001
Stale ACK	pablomartin4btc, ryanofsky

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33094 (refactor: unify container presence checks by l0rinc)
#33043 ([POC] wallet: Enable non-electronic (paper-based) wallet backup with codex32 by w0xlt)
#33034 (wallet: Store transactions in a separate sqlite table by achow101)
#32895 (wallet: Prepare for future upgrades by recording versions of last client to open and decrypt by achow101)
#32861 (Have createwalletdescriptor auto-detect an unused(KEY) by Sjors)
#32784 (wallet: derivehdkey RPC to get xpub at arbitrary path by Sjors)
#32652 (wallet: add codex32 argument to addhdkey by roconnor-blockstream)
#32471 (wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key by Eunovo)
#29136 (wallet: addhdkey RPC to add just keys to wallets via new unused(KEY) descriptor by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Wallet on May 13, 2025

achow101 force-pushed on May 13, 2025

DrahtBot added the label CI failed on May 13, 2025

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

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

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

achow101 force-pushed on May 13, 2025

achow101 force-pushed on May 14, 2025

DrahtBot removed the label CI failed on May 14, 2025

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

Concept ACK

DrahtBot added the label Needs rebase on May 16, 2025

achow101 force-pushed on May 16, 2025

DrahtBot removed the label Needs rebase on May 16, 2025

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

Concept ACK

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

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

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

DrahtBot added the label Needs rebase on May 19, 2025

achow101 force-pushed on May 20, 2025

DrahtBot removed the label Needs rebase on May 20, 2025

DrahtBot added the label Needs rebase on May 21, 2025

achow101 force-pushed on May 21, 2025

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

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

Rebased, ready for review

DrahtBot removed the label Needs rebase on May 21, 2025

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

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

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

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

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

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

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

Also split into #32593

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

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

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

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

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

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

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

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

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

Added a test.

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

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

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

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

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

This PR does that.

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

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

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

re: #32489 (review)

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

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

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

Some initial observations on the early commits inline…

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

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

achow101 force-pushed on May 22, 2025

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

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

achow101 marked this as a draft on May 22, 2025

achow101 force-pushed on May 22, 2025

achow101 force-pushed on May 23, 2025

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

Split the LockCoin commit into #3259,

#32593

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

Concept ACK

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

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

achow101 force-pushed on Jun 2, 2025

achow101 force-pushed on Jun 4, 2025

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

achow101 force-pushed on Jun 14, 2025

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

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

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

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

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

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

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

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

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

Done

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

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

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

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

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

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

Done

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

Concept ACK

Left some comments.

achow101 force-pushed on Jun 17, 2025

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

DrahtBot added the label Needs rebase on Jul 1, 2025

achow101 force-pushed on Jul 1, 2025

DrahtBot removed the label Needs rebase on Jul 1, 2025

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

achow101 force-pushed on Jul 24, 2025

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

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

All prerequisite PRs merged, ready for review.

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

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

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

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

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

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

Yes, removed.

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

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

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

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

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

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

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

re: #32489 (review)

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

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

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

light cr ACK e6ac2015a852caa0f50e44becf9cd4b7592c48e7

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

I’ll continue reviewing and will test it soon.

Left a couple of comments.

DrahtBot requested review from Eunovo on Jul 25, 2025

DrahtBot requested review from Sjors on Jul 25, 2025

DrahtBot requested review from shahsb on Jul 25, 2025

DrahtBot requested review from rkrux on Jul 25, 2025

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

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

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

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

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

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

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

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

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

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

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

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

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

Done

DrahtBot added the label Needs rebase on Jul 31, 2025

achow101 force-pushed on Jul 31, 2025

DrahtBot removed the label Needs rebase on Aug 1, 2025

DrahtBot added the label CI failed on Aug 1, 2025

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

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

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

achow101 force-pushed on Aug 1, 2025

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

ead56a5bfd

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.

1170ade3a6

achow101 force-pushed on Aug 2, 2025

DrahtBot removed the label CI failed on Aug 2, 2025

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

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

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

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

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

ryanofsky added the label Review club on Aug 4, 2025

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

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

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

nit:

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

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

Done

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

re-ACK 84d3bc68cc4d23ab9f7a91a955cd0e26ffb1b6e4

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

We’d need release notes for this new RPC.

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

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

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

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

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

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

Done

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

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

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

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

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

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

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

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

Done as suggested

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

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

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

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

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

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

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

Done

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

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

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

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

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

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

I’ll leave that for a followup

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

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

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

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

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

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

Done

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

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

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

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

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

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

Done

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

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

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

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

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

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

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

Done

ryanofsky approved

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

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

in src/wallet/wallet.cpp:4552 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:4527 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

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.

3a7a46bf5f

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.

400feb914e

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.

cc1ce4993e

achow101 force-pushed on Aug 14, 2025

wallet, rpc: Add exportwatchonlywallet RPC a429bb7cd0

test: Test for exportwatchonlywallet 4cc7be4ed9

achow101 force-pushed on Aug 14, 2025

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

Code Coverage & Benchmarks

Reviews

Conflicts

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