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 importdescriptors nuances).
basically importing the standalone address as a addr(ADDR) descriptor and the raw hex as a raw(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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | aureleoules |
| Concept ACK | Sjors, kristapsk |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, 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.
249 | @@ -247,7 +250,15 @@ RPCHelpMan importaddress() 250 | std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request); 251 | if (!pwallet) return UniValue::VNULL; 252 | 253 | - EnsureLegacyScriptPubKeyMan(*pwallet, true); 254 | + bool has_legacy = pwallet->GetLegacyScriptPubKeyMan(); 255 | + if (!has_legacy) { 256 | + // As we don't allow mixing watch-only descriptors with spendable ones, 257 | + // continue creating a legacy spkm if that is the case
Let's not create a legacy spkm inside a descriptor wallet. Just fail instead, and tell the user to use importdescriptors for that use case.
This is actually to retain the current behavior, it does not create an hybrid wallet; the legacy spkm creation only happens on blank non-descriptors wallet.
The internal EnsureLegacyScriptPubKeyMan workflow uses SetupLegacyScriptPubKeyMan which fails if the wallet contains any spkm or the WALLET_FLAG_DESCRIPTORS wallet flag.
Ah, that's worth mentioning in the comment, and maybe also covering with a test.
yeah 👍🏼
Pushed test coverage for it.
Concept ACK on reviving importaddress for descriptor wallets for some use cases. But it should continue to fail for anything that doesn't work, and not create a hybrid legacy wallet.
Concept ACK
Concept ACK on reviving
importaddressfor descriptor wallets for some use cases. But it should continue to fail for anything that doesn't work, and not create a hybrid legacy wallet.
Actually, the comment that I wrote there is wrong, not the code. Will fix it. The same previous behavior is retained; the flow does not create an hybrid wallet; the legacy spkm creation only happens on blank non-descriptors wallets. (https://github.com/bitcoin/bitcoin/pull/27034#discussion_r1097385816).
58 | @@ -59,6 +59,7 @@ static RPCHelpMan getwalletinfo() 59 | }, /*skip_type_check=*/true}, 60 | {RPCResult::Type::BOOL, "descriptors", "whether this wallet uses descriptors for scriptPubKey management"}, 61 | {RPCResult::Type::BOOL, "external_signer", "whether this wallet is configured to use an external signer such as a hardware wallet"}, 62 | + {RPCResult::Type::BOOL, "has_legacy", "whether this wallet contain a legacy scriptPubKey manager or not"},
I can see how it's useful in a test, but otherwise it's too much of an implementation detail, and potentially confusing.
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.
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.
https://cirrus-ci.com/task/5389792813252608?logs=ci#L3465
self.test_legacy_importaddress()
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/wallet_basic.py", line 72, in test_legacy_importaddress
assert_equal(wallet_watch_only.getbalances()["watchonly"]['untrusted_pending'], 10)
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/util.py", line 56, in assert_equal
raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(0E-8 == 10)
https://cirrus-ci.com/task/5389792813252608?logs=ci#L3465
self.test_legacy_importaddress() File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/wallet_basic.py", line 72, in test_legacy_importaddress assert_equal(wallet_watch_only.getbalances()["watchonly"]['untrusted_pending'], 10) File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/util.py", line 56, in assert_equal raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args)) AssertionError: not(0E-8 == 10)
Pushed. Thanks @MarcoFalke.
The issue was related to the trickle mechanism and the test network topology (lack of sync between mempools). Nothing new really, a sync_mempool call after sending the tx fixed it.
226 | @@ -225,7 +227,8 @@ RPCHelpMan importaddress() 227 | "\nNote: If you import a non-standard raw script in hex form, outputs sending to it will be treated\n" 228 | "as change, and not show up in many RPCs.\n" 229 | "Note: Use \"getwalletinfo\" to query the scanning progress.\n" 230 | - "Note: This command is only compatible with legacy wallets. Use \"importdescriptors\" with \"addr(X)\" for descriptor wallets.\n", 231 | + "Note: This command will create a new descriptor/s if the wallet has private keys disabled and has no legacy scriptPubKey manager."
Suggested alternative wording that avoids "scriptPubKey manager".
For descriptor wallets, this command only works if it has private keys disabled.
It will create a new descriptor/s. Otherwise, use \"importdescriptors\" instead.
pushed without the last part, mainly because the user cannot import watch-only descriptors on a private keys enabled wallet. So, importdescriptors wouldn't help anyway.
226 | @@ -225,7 +227,8 @@ RPCHelpMan importaddress() 227 | "\nNote: If you import a non-standard raw script in hex form, outputs sending to it will be treated\n" 228 | "as change, and not show up in many RPCs.\n" 229 | "Note: Use \"getwalletinfo\" to query the scanning progress.\n" 230 | - "Note: This command is only compatible with legacy wallets. Use \"importdescriptors\" with \"addr(X)\" for descriptor wallets.\n", 231 | + "Note: This command will create a new descriptor/s if the wallet has private keys disabled and has no legacy scriptPubKey manager." 232 | + "For any other case, the legacy scriptPubKey manager will be used\n",
This sentence can be dropped with my suggestion.
255 | + if (!use_legacy) { 256 | + // As we don't allow mixing watch-only descriptors with spendable ones, 257 | + // ensure legacy spkm if wallet is blank and descriptors are not supported. 258 | + // (blank check is done inside 'EnsureLegacyScriptPubKeyMan()') 259 | + if (!pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { 260 | + EnsureLegacyScriptPubKeyMan(*pwallet, /*also_create=*/true);
I'm confused: !use_legacy means we have a descriptor wallet. We should never create a LegacyScriptPubKeyMan in that case. I don't understand what this if block is trying to achieve, but I think it should throw JSONRPCError(RPC_WALLET_ERROR, "Use 'importdescriptors' instead"
yeah ok. No legacy spkm can be created here because EnsureLegacyScriptPubKeyMan internally calls to SetupLegacyScriptPubKeyMan which returns early if the WALLET_FLAG_DESCRIPTORS is set.
I thought that we had a release version that upgraded the legacy wallet into a descriptors wallet, letting the user import descriptors while the legacy spkm is available. But.. seems that we never allowed it, at least not officially.
Good, thanks.
upgraded the legacy wallet into a descriptors wallet
I think it's best to have only one way to go from a legacy wallet to a descriptor wallet (the migration RPC), so in the long run it's easier to keep testing this stuff.
upgraded the legacy wallet into a descriptors wallet
I think it's best to have only one way to go from a legacy wallet to a descriptor wallet (the migration RPC), so in the long run it's easier to keep testing this stuff.
yeah, agree.
the legacy spkm creation only happens on blank non-descriptors wallets
I don't think this is true, but maybe I'm reading the code wrong.
the legacy spkm creation only happens on blank non-descriptors wallets
I don't think this is true, but maybe I'm reading the code wrong.
You mean that there are other situations where a legacy spkm should be created? Or you were referring to #27034 (review)?
Just pushed an update diff. Thanks for the feedback 👌🏼 .
so it's simpler to watch for certain address/hex.
227 | @@ -226,7 +228,7 @@ RPCHelpMan importaddress() 228 | "\nNote: If you import a non-standard raw script in hex form, outputs sending to it will be treated\n" 229 | "as change, and not show up in many RPCs.\n" 230 | "Note: Use \"getwalletinfo\" to query the scanning progress.\n" 231 | - "Note: This command is only compatible with legacy wallets. Use \"importdescriptors\" for descriptor wallets.\n", 232 | + "Note: For descriptor wallets, this command will create new descriptor/s, and only works if the wallet has private keys disabled.\n",
nit: Is the /s a typo?
"Note: For descriptor wallets, this command will create new descriptors, and only works if the wallet has private keys disabled.\n",
ACK be3ae51ece862fc072d2574c34c808315ebdbff9 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())
Thanks for the review @aureleoules! https://corecheck.dev/bitcoin/bitcoin/pulls/27034 works great ❤️.
I think that will close the PR so we can remove this RPC command within the legacy wallet removal path. Happy to re-open it if there is more interest on it.
