rpc: make importaddress compatible with descriptors wallet #27034
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
0117855 to
ed94b5d
Compare
ed94b5d to
fa78ec8
Compare
|
Concept ACK on reviving |
|
Concept ACK |
Actually, the comment that I wrote there is wrong, not the code. Will fix it. |
src/wallet/rpc/wallet.cpp
Outdated
| @@ -59,6 +59,7 @@ static RPCHelpMan getwalletinfo() | |||
| }, /*skip_type_check=*/true}, | |||
| {RPCResult::Type::BOOL, "descriptors", "whether this wallet uses descriptors for scriptPubKey management"}, | |||
| {RPCResult::Type::BOOL, "external_signer", "whether this wallet is configured to use an external signer such as a hardware wallet"}, | |||
| {RPCResult::Type::BOOL, "has_legacy", "whether this wallet contain a legacy scriptPubKey manager or not"}, | |||
There was a problem hiding this comment.
I can see how it's useful in a test, but otherwise it's too much of an implementation detail, and potentially confusing.
There was a problem hiding this comment.
I don't think that differs much from the returned 'descriptors' flag. The information might be useful for debugging users' issues.
But.. I'm not so strong for it neither. Flag removed.
There was a problem hiding this comment.
It sounds like almost a mix between descriptors and !blank, but whether or not a legacy wallet contains a (in memory) LegacySPKM should not matter to the user. I didn't even know we don't create an empty one in all cases.
17fba05 to
8afc6a7
Compare
|
https://cirrus-ci.com/task/5389792813252608?logs=ci#L3465 |
8afc6a7 to
c744d32
Compare
Pushed. Thanks @MarcoFalke. |
d3bd158 to
ced5ef1
Compare
You mean that there are other situations where a legacy spkm should be created? Just pushed an update diff. |
so it's simpler to watch for certain address/hex.
ced5ef1 to
be3ae51
Compare
so it's simpler to watch for certain address/hex. Github-Pull: bitcoin#27034 Rebased-From: be3ae51
There was a problem hiding this comment.
ACK be3ae51
I reviewed the code and it looks good to me. I added a test below to cover the missing test coverage if you want to pick it up (see https://corecheck.dev/bitcoin/bitcoin/pulls/27034).
diff --git a/test/functional/wallet_descriptor.py b/test/functional/wallet_descriptor.py
index f22581ccab..921602da1e 100755
--- a/test/functional/wallet_descriptor.py
+++ b/test/functional/wallet_descriptor.py
@@ -122,16 +122,24 @@ class WalletDescriptorTest(BitcoinTestFramework):
assert_raises_rpc_error(-4, "Only legacy wallets are supported by this command", recv_wrpc.rpc.importwallet, 'wallet.dump')
assert_raises_rpc_error(-4, "Only legacy wallets are supported by this command", recv_wrpc.rpc.sethdseed)
- # Test importaddress
- self.log.info("Test watch-only descriptor wallet")
+ self.log.info("Test importaddress on watch-only descriptor wallet")
self.nodes[0].createwallet(wallet_name="watch-only-desc", disable_private_keys=True, descriptors=True, blank=True)
wallet_watch_only = self.nodes[0].get_wallet_rpc("watch-only-desc")
wallet_watch_only.importaddress(addr)
assert_equal(wallet_watch_only.getaddressinfo(addr)['ismine'], True)
assert_equal(wallet_watch_only.getaddressinfo(addr)['solvable'], False)
assert_equal(wallet_watch_only.getbalances()["mine"]['untrusted_pending'], 10)
+ raw_script = "a91430ca314f85d5036bc09c8e3bdaf68814008b6ee887"
+ assert_raises_rpc_error(-4, "P2SH import feature disabled for descriptors' wallet. Use 'importdescriptors' to specify inner P2SH function", wallet_watch_only.importaddress, raw_script, p2sh=True)
+ wallet_watch_only.importaddress(raw_script)
self.nodes[0].unloadwallet("watch-only-desc")
+ self.log.info("Test importaddress on descriptor wallet")
+ self.nodes[0].createwallet(wallet_name="non-watch-only-desc", descriptors=True)
+ wallet_non_watch_only = self.nodes[0].get_wallet_rpc("non-watch-only-desc")
+ assert_raises_rpc_error(-4, "Cannot import address in wallet with private keys enabled. Create wallet with no private keys to watch specific addresses/scripts", wallet_non_watch_only.importaddress, addr)
+ self.nodes[0].unloadwallet("non-watch-only-desc")
+
self.log.info("Test encryption")
# Get the master fingerprint before encrypt
info1 = send_wrpc.getaddressinfo(send_wrpc.getnewaddress())| @@ -226,7 +228,7 @@ RPCHelpMan importaddress() | |||
| "\nNote: If you import a non-standard raw script in hex form, outputs sending to it will be treated\n" | |||
| "as change, and not show up in many RPCs.\n" | |||
| "Note: Use \"getwalletinfo\" to query the scanning progress.\n" | |||
| "Note: This command is only compatible with legacy wallets. Use \"importdescriptors\" for descriptor wallets.\n", | |||
| "Note: For descriptor wallets, this command will create new descriptor/s, and only works if the wallet has private keys disabled.\n", | |||
There was a problem hiding this comment.
nit: Is the /s a typo?
| "Note: For descriptor wallets, this command will create new descriptor/s, and only works if the wallet has private keys disabled.\n", | |
| "Note: For descriptor wallets, this command will create new descriptors, and only works if the wallet has private keys disabled.\n", |
|
Thanks for the review @aureleoules! I think that will close the PR so we can remove this RPC command within the legacy wallet removal path. |
Made it as part of reviewing other PRs; so it's simpler to watch certain address/hex in descriptor wallets (without having to go through
importdescriptorsnuances).basically importing the standalone address as a
addr(ADDR)descriptor and the raw hex as araw(HEX)descriptor.Note:
As we don't allow mixing watch-only descriptors with spendable ones, the previous behavior is retained
if the user try to import an address into a wallet with private keys enabled.