wallet: Add exportwatchonlywallet RPC to export a watchonly version of a wallet
#32489
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32489. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. |
achow101
force-pushed
the
export-watchonly-wallet
branch
from
dc39a64 to
30ebc97
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
achow101
force-pushed
the
export-watchonly-wallet
branch
2 times, most recently
from
211dbe3 to
9fd93da
Compare
achow101
force-pushed
the
export-watchonly-wallet
branch
from
9fd93da to
4ef9315
Compare
achow101
force-pushed
the
export-watchonly-wallet
branch
from
4ef9315 to
def9594
Compare
|
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 |
achow101
force-pushed
the
export-watchonly-wallet
branch
from
def9594 to
d41059b
Compare
achow101
force-pushed
the
export-watchonly-wallet
branch
from
0f54714 to
6fafa9d
Compare
Sjors
left a comment
Sjors
left a comment
There was a problem hiding this comment.
Reviewed first commit, will continue later.
| { | ||
| return std::make_unique<ConstPubkeyProvider>(m_expr_index, m_pubkey, m_xonly); | ||
| } | ||
| bool CanSelfExpand() const final { return true; } |
There was a problem hiding this comment.
In a3be2d8 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;There was a problem hiding this comment.
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
left a comment
Sjors
left a comment
There was a problem hiding this comment.
Reviewed 6fafa9dd48a814a929efb31b746dce343a3c96dc and tested it, though not with hardened derivation.
| // Delete the watchonly wallet now that it has been exported to the desired location | ||
| fs::path watchonly_path = fs::PathFromString(watchonly_wallet->GetDatabase().Filename()).parent_path(); | ||
| watchonly_wallet.reset(); | ||
| fs::remove_all(watchonly_path); |
There was a problem hiding this comment.
After #33032 this intermediate watch-only wallet could in-memory.
| LOCK(watchonly_wallet->cs_wallet); | ||
|
|
||
| // Parse the descriptors and add them to the new wallet | ||
| for (const WalletDescInfo& desc_info : *exported) { |
There was a problem hiding this comment.
In a453ea6ff06797224adbb1dd839299fe3d8636ae wallet: Add CWallet::ExportWatchOnly: nit *Assert(exported)
|
|
||
| // 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()) { |
There was a problem hiding this comment.
In a453ea6ff06797224adbb1dd839299fe3d8636ae wallet: Add CWallet::ExportWatchOnly: if you take my earlier suggestion #32489 (comment) for having this return false for non-ranged descriptors, you'll need to add w_desc.descriptor->IsRange() &&
| 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think that is unnecessary, and unlikely to happen.
achow101
force-pushed
the
export-watchonly-wallet
branch
2 times, most recently
from
e3f7111 to
7822a56
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
achow101
force-pushed
the
export-watchonly-wallet
branch
from
7822a56 to
8649a53
Compare
achow101
force-pushed
the
export-watchonly-wallet
branch
from
8649a53 to
fdd55b8
Compare
CanSelfExpand() reports whether a descriptor can be expanded without needing any caches or private keys to be provided by the caller of Expand().
If a descriptor does not need any caches or private keys in order to expand, then CanGetAddresses() should return true for that descriptor.
If a new WalletDescriptor is provided to us with a cache, write the cache to disk as well.
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.
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.
achow101
force-pushed
the
export-watchonly-wallet
branch
from
fdd55b8 to
5d744c1
Compare
14 participants
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
exportwatchonlywalletwhich 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.