From be3ae51ece862fc072d2574c34c808315ebdbff9 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 3 Feb 2023 10:17:19 -0300 Subject: [PATCH 1/4] rpc: make importaddress compatible with descriptors wallet so it's simpler to watch for certain address/hex. --- src/script/descriptor.cpp | 4 +- src/script/descriptor.h | 5 ++ src/wallet/rpc/backup.cpp | 69 ++++++++++++++++----- test/functional/test_framework/test_node.py | 27 -------- test/functional/wallet_basic.py | 21 +++++++ test/functional/wallet_descriptor.py | 11 +++- test/functional/wallet_labels.py | 2 +- 7 files changed, 91 insertions(+), 48 deletions(-) diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index 0951504ee0..4f3daa13ef 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -143,8 +143,6 @@ std::string DescriptorChecksum(const Span& span) return ret; } -std::string AddChecksum(const std::string& str) { return str + "#" + DescriptorChecksum(str); } - //////////////////////////////////////////////////////////////////////////// // Internal representation // //////////////////////////////////////////////////////////////////////////// @@ -1746,6 +1744,8 @@ std::string GetDescriptorChecksum(const std::string& descriptor) return ret; } +std::string AddChecksum(const std::string& str) { return str + "#" + DescriptorChecksum(str); } + std::unique_ptr InferDescriptor(const CScript& script, const SigningProvider& provider) { return InferScript(script, ParseScriptContext::TOP, provider); diff --git a/src/script/descriptor.h b/src/script/descriptor.h index 39b1a37f9a..e8d0d72205 100644 --- a/src/script/descriptor.h +++ b/src/script/descriptor.h @@ -166,6 +166,11 @@ std::unique_ptr Parse(const std::string& descriptor, FlatSigningProv */ std::string GetDescriptorChecksum(const std::string& descriptor); +/** + * Simple wrapper to add the checksum at the end of the descriptor + */ +std::string AddChecksum(const std::string& str); + /** Find a descriptor for the specified `script`, using information from `provider` where possible. * * A non-ranged descriptor which only generates the specified script will be returned in all diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index 553bbfb62f..34c83f052f 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -213,6 +213,8 @@ RPCHelpMan importprivkey() }; } +static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); + RPCHelpMan importaddress() { return RPCHelpMan{"importaddress", @@ -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", { {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The Bitcoin address (or hex-encoded script)"}, {"label", RPCArg::Type::STR, RPCArg::Default{""}, "An optional label"}, @@ -247,7 +249,18 @@ RPCHelpMan importaddress() std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return UniValue::VNULL; - EnsureLegacyScriptPubKeyMan(*pwallet, true); + // Use legacy spkm only if the wallet does not support descriptors. + bool use_legacy = !pwallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS); + if (!use_legacy) { + // We don't allow mixing watch-only descriptors with spendable ones. + if (!pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { + throw JSONRPCError(RPC_WALLET_ERROR, "Cannot import address in wallet with private keys enabled. " + "Create wallet with no private keys to watch specific addresses/scripts"); + } + } else { + // In case the wallet is blank + EnsureLegacyScriptPubKeyMan(*pwallet, /*also_create=*/true); + } const std::string strLabel{LabelFromValue(request.params[1])}; @@ -273,33 +286,55 @@ RPCHelpMan importaddress() if (!request.params[3].isNull()) fP2SH = request.params[3].get_bool(); + // Import descriptor helper function + const auto& import_descriptor = [pwallet](const std::string& desc, const std::string label) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet) { + UniValue data(UniValue::VType::VOBJ); + data.pushKV("desc", AddChecksum(desc)); + if (!label.empty()) data.pushKV("label", label); + const UniValue& ret = ProcessDescriptorImport(*pwallet, data, /*timestamp=*/1); + if (ret.exists("error")) throw ret["error"]; + }; + { LOCK(pwallet->cs_wallet); - CTxDestination dest = DecodeDestination(request.params[0].get_str()); + const std::string& address = request.params[0].get_str(); + CTxDestination dest = DecodeDestination(address); if (IsValidDestination(dest)) { if (fP2SH) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Cannot use the p2sh flag with an address - use a script instead"); } - if (OutputTypeFromDestination(dest) == OutputType::BECH32M) { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Bech32m addresses cannot be imported into legacy wallets"); - } pwallet->MarkDirty(); - pwallet->ImportScriptPubKeys(strLabel, {GetScriptForDestination(dest)}, /*have_solving_data=*/false, /*apply_label=*/true, /*timestamp=*/1); - } else if (IsHex(request.params[0].get_str())) { - std::vector data(ParseHex(request.params[0].get_str())); - CScript redeem_script(data.begin(), data.end()); - - std::set scripts = {redeem_script}; - pwallet->ImportScripts(scripts, /*timestamp=*/0); - - if (fP2SH) { - scripts.insert(GetScriptForDestination(ScriptHash(redeem_script))); + if (use_legacy) { + if (OutputTypeFromDestination(dest) == OutputType::BECH32M) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Bech32m addresses cannot be imported into legacy wallets"); + } + pwallet->ImportScriptPubKeys(strLabel, {GetScriptForDestination(dest)}, /*have_solving_data=*/false, /*apply_label=*/true, /*timestamp=*/1); + } else { + import_descriptor("addr(" + address + ")", strLabel); } + } else if (IsHex(request.params[0].get_str())) { + const std::string& hex = request.params[0].get_str(); - pwallet->ImportScriptPubKeys(strLabel, scripts, /*have_solving_data=*/false, /*apply_label=*/true, /*timestamp=*/1); + if (use_legacy) { + std::vector data(ParseHex(hex)); + CScript redeem_script(data.begin(), data.end()); + std::set scripts = {redeem_script}; + pwallet->ImportScripts(scripts, /*timestamp=*/0); + if (fP2SH) { + scripts.insert(GetScriptForDestination(ScriptHash(redeem_script))); + } + pwallet->ImportScriptPubKeys(strLabel, scripts, /*have_solving_data=*/false, /*apply_label=*/true, /*timestamp=*/1); + } else { + // P2SH Not allowed. Can't detect inner P2SH function from a raw hex. + if (fP2SH) throw JSONRPCError(RPC_WALLET_ERROR, "P2SH import feature disabled for descriptors' wallet. " + "Use 'importdescriptors' to specify inner P2SH function"); + + // Import descriptors + import_descriptor("raw(" + hex + ")", strLabel); + } } else { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Bitcoin address or script"); } diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 56abe5f26a..274212a3fa 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -819,30 +819,3 @@ class RPCOverloadWrapper(): import_res = self.importdescriptors(req) if not import_res[0]['success']: raise JSONRPCException(import_res[0]['error']) - - def importaddress(self, address, label=None, rescan=None, p2sh=None): - wallet_info = self.getwalletinfo() - if 'descriptors' not in wallet_info or ('descriptors' in wallet_info and not wallet_info['descriptors']): - return self.__getattr__('importaddress')(address, label, rescan, p2sh) - is_hex = False - try: - int(address ,16) - is_hex = True - desc = descsum_create('raw(' + address + ')') - except Exception: - desc = descsum_create('addr(' + address + ')') - reqs = [{ - 'desc': desc, - 'timestamp': 0 if rescan else 'now', - 'label': label if label else '' - }] - if is_hex and p2sh: - reqs.append({ - 'desc': descsum_create('p2sh(raw(' + address + '))'), - 'timestamp': 0 if rescan else 'now', - 'label': label if label else '' - }) - import_res = self.importdescriptors(reqs) - for res in import_res: - if not res['success']: - raise JSONRPCException(res['error']) diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index 53ac01686a..0187d0a57a 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -55,6 +55,24 @@ class WalletTest(BitcoinTestFramework): def get_vsize(self, txn): return self.nodes[0].decoderawtransaction(txn)['vsize'] + def test_legacy_importaddress(self): + if self.options.descriptors: + return + + addr = self.nodes[1].getnewaddress() + self.nodes[1].sendtoaddress(addr, 10) + self.sync_mempools(self.nodes[0:2]) + + self.log.info("Test 'importaddress' on a blank, private keys disabled, wallet with no descriptors support") + self.nodes[0].createwallet(wallet_name="watch-only-legacy", disable_private_keys=False, descriptors=False, blank=True) + wallet_watch_only = self.nodes[0].get_wallet_rpc("watch-only-legacy") + wallet_watch_only.importaddress(addr) + assert_equal(wallet_watch_only.getaddressinfo(addr)['ismine'], False) + assert_equal(wallet_watch_only.getaddressinfo(addr)['iswatchonly'], True) + assert_equal(wallet_watch_only.getaddressinfo(addr)['solvable'], False) + assert_equal(wallet_watch_only.getbalances()["watchonly"]['untrusted_pending'], 10) + self.nodes[0].unloadwallet("watch-only-legacy") + def run_test(self): # Check that there's no UTXO on none of the nodes @@ -540,6 +558,9 @@ class WalletTest(BitcoinTestFramework): {"address": address_to_import}, {"spendable": True}) + # Test importaddress on a blank, private keys disabled, legacy wallet with no descriptors support + self.test_legacy_importaddress() + # Mine a block from node0 to an address from node1 coinbase_addr = self.nodes[1].getnewaddress() block_hash = self.generatetoaddress(self.nodes[0], 1, coinbase_addr, sync_fun=lambda: self.sync_all(self.nodes[0:3]))[0] diff --git a/test/functional/wallet_descriptor.py b/test/functional/wallet_descriptor.py index b0e93df36a..f22581ccab 100755 --- a/test/functional/wallet_descriptor.py +++ b/test/functional/wallet_descriptor.py @@ -115,7 +115,6 @@ class WalletDescriptorTest(BitcoinTestFramework): self.log.info("Test disabled RPCs") assert_raises_rpc_error(-4, "Only legacy wallets are supported by this command", recv_wrpc.rpc.importprivkey, "cVpF924EspNh8KjYsfhgY96mmxvT6DgdWiTYMtMjuM74hJaU5psW") assert_raises_rpc_error(-4, "Only legacy wallets are supported by this command", recv_wrpc.rpc.importpubkey, send_wrpc.getaddressinfo(send_wrpc.getnewaddress())["pubkey"]) - assert_raises_rpc_error(-4, "Only legacy wallets are supported by this command", recv_wrpc.rpc.importaddress, recv_wrpc.getnewaddress()) assert_raises_rpc_error(-4, "Only legacy wallets are supported by this command", recv_wrpc.rpc.importmulti, []) assert_raises_rpc_error(-4, "Only legacy wallets are supported by this command", recv_wrpc.rpc.addmultisigaddress, 1, [recv_wrpc.getnewaddress()]) assert_raises_rpc_error(-4, "Only legacy wallets are supported by this command", recv_wrpc.rpc.dumpprivkey, recv_wrpc.getnewaddress()) @@ -123,6 +122,16 @@ 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.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) + self.nodes[0].unloadwallet("watch-only-desc") + self.log.info("Test encryption") # Get the master fingerprint before encrypt info1 = send_wrpc.getaddressinfo(send_wrpc.getnewaddress()) diff --git a/test/functional/wallet_labels.py b/test/functional/wallet_labels.py index f074339a2b..1cc7786c94 100755 --- a/test/functional/wallet_labels.py +++ b/test/functional/wallet_labels.py @@ -212,7 +212,7 @@ class WalletLabelsTest(BitcoinTestFramework): ad = BECH32_INVALID[l] assert_raises_rpc_error( -5, - "Address is not valid" if self.options.descriptors else "Invalid Bitcoin address or script", + "Invalid Bitcoin address or script", lambda: wallet_watch_only.importaddress(label=l, rescan=False, address=ad), ) From 0c38bbd194e2314d3bf8ff3c2e50db46da7dfc38 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sat, 2 Mar 2024 13:36:02 +0000 Subject: [PATCH 2/4] QA: wallet_reindex: Use `importdescriptors` directly rather than abusing test_framework's `importaddress` wrapper `importdescriptors` is expected to initialise wallet birth time to "now", but `importaddress` initialises it to 1 instead. --- test/functional/wallet_reindex.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/test/functional/wallet_reindex.py b/test/functional/wallet_reindex.py index 5388de4b71..ad0a0c3151 100755 --- a/test/functional/wallet_reindex.py +++ b/test/functional/wallet_reindex.py @@ -8,6 +8,7 @@ import time from test_framework.blocktools import COINBASE_MATURITY +from test_framework.descriptors import descsum_create from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, @@ -52,7 +53,15 @@ class WalletReindexTest(BitcoinTestFramework): # For a descriptors wallet: Import address with timestamp=now. # For legacy wallet: There is no way of importing a script/address with a custom time. The wallet always imports it with birthtime=1. # In both cases, disable rescan to not detect the transaction. - wallet_watch_only.importaddress(wallet_addr, rescan=False) + if self.options.descriptors: + import_res = wallet_watch_only.importdescriptors([{ + 'desc': descsum_create('addr(' + wallet_addr + ')'), + 'timestamp': 'now', + }]) + assert len(import_res) == 1 + assert import_res[0]['success'] + else: + wallet_watch_only.importaddress(wallet_addr, rescan=False) assert_equal(len(wallet_watch_only.listtransactions()), 0) # Depending on the wallet type, the birth time changes. From 816d59c61a0df22e2da9c42d01d76b499e88026d Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Thu, 27 Jul 2023 18:41:51 +0000 Subject: [PATCH 3/4] Diff-minimise --- src/wallet/rpc/backup.cpp | 34 ++++++++++++--------- test/functional/test_framework/test_node.py | 27 ++++++++++++++++ 2 files changed, 46 insertions(+), 15 deletions(-) diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index 85d539e2fa..18742abedb 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -255,15 +255,15 @@ RPCHelpMan importaddress() // Use legacy spkm only if the wallet does not support descriptors. bool use_legacy = !pwallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS); - if (!use_legacy) { + if (use_legacy) { + // In case the wallet is blank + EnsureLegacyScriptPubKeyMan(*pwallet, true); + } else { // We don't allow mixing watch-only descriptors with spendable ones. if (!pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { throw JSONRPCError(RPC_WALLET_ERROR, "Cannot import address in wallet with private keys enabled. " "Create wallet with no private keys to watch specific addresses/scripts"); } - } else { - // In case the wallet is blank - EnsureLegacyScriptPubKeyMan(*pwallet, /*also_create=*/true); } const std::string strLabel{LabelFromValue(request.params[1])}; @@ -308,14 +308,15 @@ RPCHelpMan importaddress() if (fP2SH) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Cannot use the p2sh flag with an address - use a script instead"); } + if (OutputTypeFromDestination(dest) == OutputType::BECH32M) { + if (use_legacy) + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Bech32m addresses cannot be imported into legacy wallets"); + } pwallet->MarkDirty(); if (use_legacy) { - if (OutputTypeFromDestination(dest) == OutputType::BECH32M) { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Bech32m addresses cannot be imported into legacy wallets"); - } - pwallet->ImportScriptPubKeys(strLabel, {GetScriptForDestination(dest)}, /*have_solving_data=*/false, /*apply_label=*/true, /*timestamp=*/1); + pwallet->ImportScriptPubKeys(strLabel, {GetScriptForDestination(dest)}, /*have_solving_data=*/false, /*apply_label=*/true, /*timestamp=*/1); } else { import_descriptor("addr(" + address + ")", strLabel); } @@ -324,13 +325,16 @@ RPCHelpMan importaddress() if (use_legacy) { std::vector data(ParseHex(hex)); - CScript redeem_script(data.begin(), data.end()); - std::set scripts = {redeem_script}; - pwallet->ImportScripts(scripts, /*timestamp=*/0); - if (fP2SH) { - scripts.insert(GetScriptForDestination(ScriptHash(redeem_script))); - } - pwallet->ImportScriptPubKeys(strLabel, scripts, /*have_solving_data=*/false, /*apply_label=*/true, /*timestamp=*/1); + CScript redeem_script(data.begin(), data.end()); + + std::set scripts = {redeem_script}; + pwallet->ImportScripts(scripts, /*timestamp=*/0); + + if (fP2SH) { + scripts.insert(GetScriptForDestination(ScriptHash(redeem_script))); + } + + pwallet->ImportScriptPubKeys(strLabel, scripts, /*have_solving_data=*/false, /*apply_label=*/true, /*timestamp=*/1); } else { // P2SH Not allowed. Can't detect inner P2SH function from a raw hex. if (fP2SH) throw JSONRPCError(RPC_WALLET_ERROR, "P2SH import feature disabled for descriptors' wallet. " diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index d018512f3e..2c244c6291 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -950,3 +950,30 @@ class RPCOverloadWrapper(): import_res = self.importdescriptors(req) if not import_res[0]['success']: raise JSONRPCException(import_res[0]['error']) + + def _deleted_importaddress(self, address, label=None, rescan=None, p2sh=None): + wallet_info = self.getwalletinfo() + if 'descriptors' not in wallet_info or ('descriptors' in wallet_info and not wallet_info['descriptors']): + return self.__getattr__('importaddress')(address, label, rescan, p2sh) + is_hex = False + try: + int(address ,16) + is_hex = True + desc = descsum_create('raw(' + address + ')') + except Exception: + desc = descsum_create('addr(' + address + ')') + reqs = [{ + 'desc': desc, + 'timestamp': 0 if rescan else 'now', + 'label': label if label else '' + }] + if is_hex and p2sh: + reqs.append({ + 'desc': descsum_create('p2sh(raw(' + address + '))'), + 'timestamp': 0 if rescan else 'now', + 'label': label if label else '' + }) + import_res = self.importdescriptors(reqs) + for res in import_res: + if not res['success']: + raise JSONRPCException(res['error']) From f72496a06a9999f3f703d842d610f5b670d89efa Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Thu, 27 Jul 2023 18:57:20 +0000 Subject: [PATCH 4/4] Keep ProcessDescriptorImport exported for bitcoin-wallet importfromcoldcard --- src/wallet/rpc/backup.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index 18742abedb..0e7787586e 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -217,7 +217,7 @@ RPCHelpMan importprivkey() }; } -static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); +UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); RPCHelpMan importaddress() {