rpc: make importaddress compatible with descriptors wallet #27034

furszy · 2023-02-03T15:21:29Z

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.

DrahtBot · 2023-02-03T15:21:32Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26840 (refactor: importpubkey, importprivkey, importaddress, importmulti, and importdescriptors rpc by KolbyML)

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.

src/wallet/rpc/backup.cpp

Sjors · 2023-02-06T09:48:21Z

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.

kristapsk · 2023-02-06T09:52:27Z

Concept ACK

furszy · 2023-02-06T13:40:09Z

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.

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. (#27034 (comment)).

Sjors · 2023-02-06T14:19:41Z

src/wallet/rpc/wallet.cpp

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.

maflcko · 2023-02-20T15:15:35Z

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)

furszy · 2023-02-20T22:39:38Z

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.

Sjors

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.

src/wallet/rpc/backup.cpp

furszy · 2023-02-24T14:04:27Z

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 (comment)?

Just pushed an update diff.
Thanks for the feedback 👌🏼 .

so it's simpler to watch for certain address/hex.

so it's simpler to watch for certain address/hex. Github-Pull: bitcoin#27034 Rebased-From: be3ae51

aureleoules

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

aureleoules · 2023-10-10T22:56:49Z

src/wallet/rpc/backup.cpp

            "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",


nit: Is the /s a typo?

Suggested change

"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",

furszy · 2023-10-11T00:20:49Z

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.

DrahtBot added the RPC/REST/ZMQ label Feb 3, 2023

furszy force-pushed the 2022_rpc_importaddress_descriptors_compatible branch from 0117855 to ed94b5d Compare February 3, 2023 15:35

furszy force-pushed the 2022_rpc_importaddress_descriptors_compatible branch from ed94b5d to fa78ec8 Compare February 6, 2023 00:50

Sjors reviewed Feb 6, 2023

View reviewed changes

src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved

Sjors reviewed Feb 6, 2023

View reviewed changes

furszy force-pushed the 2022_rpc_importaddress_descriptors_compatible branch 4 times, most recently from 17fba05 to 8afc6a7 Compare February 6, 2023 15:47

This was referenced Feb 6, 2023

lint: enable E722 do not use bare except #25867

Merged

tests: Run both descriptor and legacy tests within a single test invocation #20892

Closed

furszy mentioned this pull request Feb 18, 2023

Improve porting documentation for legacy-only wallet RPCs #25363

Closed

DrahtBot mentioned this pull request Feb 19, 2023

refactor: importpubkey, importprivkey, importaddress, importmulti, and importdescriptors rpc #26840

Closed

furszy force-pushed the 2022_rpc_importaddress_descriptors_compatible branch from 8afc6a7 to c744d32 Compare February 20, 2023 21:32

DrahtBot mentioned this pull request Feb 21, 2023

Add Import to Wallet GUI bitcoin-core/gui#650

Closed

2 tasks

Sjors suggested changes Feb 21, 2023

View reviewed changes

src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved

src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved

src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Feb 22, 2023

furszy force-pushed the 2022_rpc_importaddress_descriptors_compatible branch 2 times, most recently from d3bd158 to ced5ef1 Compare February 24, 2023 13:52

DrahtBot removed the Needs rebase label Feb 24, 2023

rpc: make importaddress compatible with descriptors wallet

be3ae51

so it's simpler to watch for certain address/hex.

DrahtBot added the Needs rebase label May 1, 2023

furszy force-pushed the 2022_rpc_importaddress_descriptors_compatible branch from ced5ef1 to be3ae51 Compare May 1, 2023 12:53

DrahtBot removed the Needs rebase label May 1, 2023

DrahtBot mentioned this pull request Jul 22, 2023

wallet legacy: bugfix, disallow importing invalid scripts via importaddress #28126

Closed

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Aug 16, 2023

rpc: make importaddress compatible with descriptors wallet

c090b42

so it's simpler to watch for certain address/hex. Github-Pull: bitcoin#27034 Rebased-From: be3ae51

achow101 self-requested a review September 20, 2023 16:36

aureleoules reviewed Oct 10, 2023

View reviewed changes

DrahtBot requested a review from Sjors October 10, 2023 23:03

furszy closed this Oct 11, 2023

furszy mentioned this pull request May 25, 2024

Enable importprivkey, addmultisigaddress in descriptor wallets #30175

Open

bitcoin locked and limited conversation to collaborators Oct 10, 2024

	"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",

rpc: make importaddress compatible with descriptors wallet #27034

rpc: make importaddress compatible with descriptors wallet #27034

Uh oh!

Conversation

furszy commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Uh oh!

Sjors commented Feb 6, 2023

Uh oh!

kristapsk commented Feb 6, 2023

Uh oh!

furszy commented Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors Feb 6, 2023

Choose a reason for hiding this comment

Uh oh!

furszy Feb 6, 2023

Choose a reason for hiding this comment

Uh oh!

Sjors Feb 6, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko commented Feb 20, 2023

Uh oh!

furszy commented Feb 20, 2023

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

furszy commented Feb 24, 2023

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

aureleoules Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

furszy commented Oct 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

furszy commented Feb 3, 2023 •

edited

Loading

DrahtBot commented Feb 3, 2023 •

edited

Loading

furszy commented Feb 6, 2023 •

edited

Loading