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

pull achow101 wants to merge 11 commits into bitcoin:master from achow101:export-watchonly-wallet changing 18 files +700 −93
  1. achow101 commented at 10:22 pm on May 13, 2025: member

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

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

    Depends on #32593 and #32597

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

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #bitcoin-core/gui/872 (Menu action to export a watchonly wallet by achow101)
    • #32724 (Musig2 tests by w0xlt)
    • #32652 (wallet: add codex32 argument to addhdkey by roconnor-blockstream)
    • #32471 (wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key by Eunovo)
    • #31244 (descriptors: MuSig2 by achow101)
    • #29675 (wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys by achow101)
    • #29136 (wallet: addhdkey RPC to add just keys to wallets via new unused(KEY) descriptor by achow101)
    • #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
    • #27286 (wallet: Keep track of the wallet’s own transaction outputs in memory by achow101)

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

  3. DrahtBot added the label Wallet on May 13, 2025
  4. achow101 force-pushed on May 13, 2025
  5. DrahtBot added the label CI failed on May 13, 2025
  6. 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.

  7. achow101 force-pushed on May 13, 2025
  8. achow101 force-pushed on May 13, 2025
  9. achow101 force-pushed on May 14, 2025
  10. DrahtBot removed the label CI failed on May 14, 2025
  11. shahsb commented at 2:30 pm on May 15, 2025: none
    Concept ACK
  12. DrahtBot added the label Needs rebase on May 16, 2025
  13. achow101 force-pushed on May 16, 2025
  14. DrahtBot removed the label Needs rebase on May 16, 2025
  15. 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).

  16. DrahtBot added the label Needs rebase on May 19, 2025
  17. achow101 force-pushed on May 20, 2025
  18. DrahtBot removed the label Needs rebase on May 20, 2025
  19. DrahtBot added the label Needs rebase on May 21, 2025
  20. achow101 force-pushed on May 21, 2025
  21. achow101 marked this as ready for review on May 21, 2025
  22. achow101 commented at 5:54 pm on May 21, 2025: member
    Rebased, ready for review
  23. DrahtBot removed the label Needs rebase on May 21, 2025
  24. 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

  25. 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.
  26. 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.

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

  28. achow101 force-pushed on May 22, 2025
  29. 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
  30. achow101 marked this as a draft on May 22, 2025
  31. achow101 force-pushed on May 22, 2025
  32. achow101 force-pushed on May 23, 2025
  33. Sjors commented at 7:52 am on May 23, 2025: member

    Split the LockCoin commit into #3259,

    #32593

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

  35. fanquake referenced this in commit 4b1d48a686 on May 30, 2025
  36. achow101 force-pushed on Jun 2, 2025
  37. achow101 force-pushed on Jun 4, 2025
  38. glozow referenced this in commit 287cd04a32 on Jun 13, 2025
  39. wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC 
    If the locked coin needs to be persisted to the wallet database,
insteead of having the RPC figure out when to create a WalletBatch and
having LockCoin's behavior depend on it, have LockCoin take whether to
persist as a parameter so it makes the batch.

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

Keeping track of whether a locked coin was persisted is also useful
information for future PRs.
    923013a16a
  40. wallet: Set upgraded descriptor cache flag for newly created wallets 
    Although WalletBatch::LoadWallet performs the descriptor cache upgrade,
because new wallets do not have the descriptor flag set yet, the upgrade
does not run and set the flag.

Since new wallets will always being using the upgraded cache, there's no
reason to wait to set the flag, so set it when the wallet flags are
being initialized for new wallets.
    9c62aaf31d
  41. wallet: Add GetWalletFlags 93fc4832bb
  42. wallet, rpc: Output wallet flags in getwalletinfo 2e017a0853
  43. 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().
    ab0be3f980
  44. 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.
    87b37364c9
  45. 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.
    45d3a6c66b
  46. 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.
    4021cc7ddf
  47. 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.
    f61fbfb724
  48. wallet, rpc: Add exportwatchonlywallet RPC 2deef5e576
  49. achow101 force-pushed on Jun 14, 2025
  50. 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.
  51. 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
  52. 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
  53. Eunovo commented at 12:20 pm on June 17, 2025: contributor

    Concept ACK

    Left some comments.

  54. test: Test for exportwatchonlywallet 3010b614ac
  55. achow101 force-pushed on Jun 17, 2025


achow101 DrahtBot shahsb Sjors vicjuma Eunovo

Labels
Wallet

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-06-18 06:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me