From 9ea534ffe933acd5025e3bf01cda5bf8ab79c8a7 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 22 Dec 2023 15:25:13 -0500 Subject: [PATCH 01/32] descriptor: Add unused(KEY) descriptor unused() descriptors do not have scriptPubKeys. Instead, the wallet uses them to store keys without having any scripts to watch for. --- src/script/descriptor.cpp | 40 ++++++++++++++++++ src/script/descriptor.h | 5 ++- src/test/descriptor_tests.cpp | 61 ++++++++++++++++++++++++++++ src/wallet/test/walletload_tests.cpp | 1 + src/wallet/wallet.cpp | 4 +- 5 files changed, 108 insertions(+), 3 deletions(-) diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index c530c888c43e..bd91d5a55b3f 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -993,6 +993,8 @@ class DescriptorImpl : public Descriptor virtual std::unique_ptr Clone() const = 0; + bool HasScripts() const override { return true; } + // NOLINTNEXTLINE(misc-no-recursion) std::vector Warnings() const override { std::vector all = m_warnings; @@ -1638,6 +1640,23 @@ class RawTRDescriptor final : public DescriptorImpl } }; +/** A parsed unused(KEY) descriptor */ +class UnusedDescriptor final : public DescriptorImpl +{ +protected: + std::vector MakeScripts(const std::vector& keys, std::span scripts, FlatSigningProvider& out) const override { return {}; } +public: + UnusedDescriptor(std::unique_ptr prov) : DescriptorImpl(Vector(std::move(prov)), "unused") {} + bool IsSingleType() const final { return true; } + bool HasScripts() const override { return false; } + + std::unique_ptr Clone() const override + { + return std::make_unique(m_pubkey_args.at(0)->Clone()); + } +}; + + //////////////////////////////////////////////////////////////////////////// // Parser // //////////////////////////////////////////////////////////////////////////// @@ -2479,6 +2498,27 @@ std::vector> ParseScript(uint32_t& key_exp_index error = "Can only have rawtr at top level"; return {}; } + if (ctx == ParseScriptContext::TOP && Func("unused", expr)) { + // Check for only one expression, should not find commas, brackets, or parentheses + auto arg = Expr(expr); + if (expr.size()) { + error = strprintf("unused(): only one key expected"); + return {}; + } + auto keys = ParsePubkey(key_exp_index, arg, ctx, out, error); + if (keys.empty()) return {}; + for (auto& pubkey : keys) { + if (pubkey->IsRange()) { + error = "unused(): key cannot be ranged"; + return {}; + } + ret.emplace_back(std::make_unique(std::move(pubkey))); + } + return ret; + } else if (Func("unused", expr)) { + error = "Can only have unused at top level"; + return {}; + } if (ctx == ParseScriptContext::TOP && Func("raw", expr)) { std::string str(expr.begin(), expr.end()); if (!IsHex(str)) { diff --git a/src/script/descriptor.h b/src/script/descriptor.h index 8c8dff8a8530..48cbdec87a31 100644 --- a/src/script/descriptor.h +++ b/src/script/descriptor.h @@ -108,7 +108,7 @@ struct Descriptor { /** Convert the descriptor back to a string, undoing parsing. */ virtual std::string ToString(bool compat_format=false) const = 0; - /** Whether this descriptor will return one scriptPubKey or multiple (aka is or is not combo) */ + /** Whether this descriptor will return at most one scriptPubKey or multiple (aka is or is not combo) */ virtual bool IsSingleType() const = 0; /** Convert the descriptor to a private string. This fails if the provided provider does not have the relevant private keys. */ @@ -166,6 +166,9 @@ struct Descriptor { */ virtual void GetPubKeys(std::set& pubkeys, std::set& ext_pubs) const = 0; + /** Whether this descriptor produces any scripts with the Expand functions */ + virtual bool HasScripts() const = 0; + /** Semantic/safety warnings (includes subdescriptors). */ virtual std::vector Warnings() const = 0; }; diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp index 04794b73ba47..52d14d08d88d 100644 --- a/src/test/descriptor_tests.cpp +++ b/src/test/descriptor_tests.cpp @@ -1317,4 +1317,65 @@ BOOST_AUTO_TEST_CASE(descriptor_older_warnings) } } +void CheckSingleUnparsable(const std::string& desc, const std::string& expected_error) +{ + FlatSigningProvider keys; + std::string error; + auto parsed = Parse(desc, keys, error); + BOOST_CHECK_MESSAGE(parsed.empty(), desc); + BOOST_CHECK_EQUAL(error, expected_error); +} + +void CheckUnused(const std::string& prv, const std::string& pub) +{ + FlatSigningProvider keys_priv, keys_pub; + std::string error; + + std::unique_ptr parse_priv; + std::unique_ptr parse_pub; + parse_priv = std::move(Parse(prv, keys_priv, error).at(0)); + parse_pub = std::move(Parse(pub, keys_pub, error).at(0)); + BOOST_CHECK_MESSAGE(parse_priv, error); + BOOST_CHECK_MESSAGE(parse_pub, error); + + BOOST_CHECK(parse_priv->GetOutputType() == std::nullopt); + BOOST_CHECK(parse_pub->GetOutputType() == std::nullopt); + + // Check private keys are extracted from the private version but not the public one. + BOOST_CHECK(keys_priv.keys.size()); + BOOST_CHECK(!keys_pub.keys.size()); + + // Check that both versions serialize back to the public version. + std::string pub1 = parse_priv->ToString(); + std::string pub2 = parse_pub->ToString(); + BOOST_CHECK_MESSAGE(EqualDescriptor(pub, pub1), "Private ser: " + pub1 + " Public desc: " + pub); + BOOST_CHECK_MESSAGE(EqualDescriptor(pub, pub2), "Public ser: " + pub2 + " Public desc: " + pub); + + // Check both only have one pubkey + std::set prv_pubkeys; + std::set prv_extpubs; + parse_pub->GetPubKeys(prv_pubkeys, prv_extpubs); + BOOST_CHECK_EQUAL(prv_pubkeys.size() + prv_extpubs.size(), 1); + std::set pub_pubkeys; + std::set pub_extpubs; + parse_pub->GetPubKeys(pub_pubkeys, pub_extpubs); + BOOST_CHECK_EQUAL(pub_pubkeys.size() + pub_extpubs.size(), 1); +} + +// unused() descriptors don't produce scripts, so these need to be tested separately +BOOST_AUTO_TEST_CASE(unused_descriptor_test) +{ + CheckUnparsable("unused(L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1,5KYZdUEo39z3FPrtuX2QbbwGnNP5zTd7yyr2SC1j299sBCnWjss)", "unused(03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,04a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd5b8dec5235a0fa8722476c7709c02559e3aa73aa03918ba2d492eea75abea235)", "unused(): only one key expected"); + CheckUnparsable("wsh(unused(L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1))", "wsh(unused(03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd))", "Can only have unused at top level"); + CheckUnparsable("unused(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc/*)", "unused(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/*)", "unused(): key cannot be ranged"); + CheckUnparsable("unused()", "unused()", "No key provided"); + + // x-only keys cannot be used in unused() + CheckSingleUnparsable("unused(a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd)", "Pubkey 'a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd' is invalid"); + + CheckUnused("unused(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc)", "unused(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL)"); + CheckUnused("unused(L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1)", "unused(03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd)"); + CheckUnused("unused(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc/0h/0h/1)", "unused(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/0h/0h/1)"); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/wallet/test/walletload_tests.cpp b/src/wallet/test/walletload_tests.cpp index 0cc5a88cfdb1..6c8071e0bea0 100644 --- a/src/wallet/test/walletload_tests.cpp +++ b/src/wallet/test/walletload_tests.cpp @@ -35,6 +35,7 @@ class DummyDescriptor final : public Descriptor { std::optional MaxSatisfactionWeight(bool) const override { return {}; } std::optional MaxSatisfactionElems() const override { return {}; } void GetPubKeys(std::set& pubkeys, std::set& ext_pubs) const override {} + bool HasScripts() const override { return true; } std::vector Warnings() const override { return {}; } }; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index ed9bb8406d72..66c282e4a0b6 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3727,8 +3727,8 @@ util::Result> CWallet::AddWall } // Apply the label if necessary - // Note: we disable labels for ranged descriptors - if (!desc.descriptor->IsRange()) { + // Note: we disable labels for descriptors that are ranged or that don't produce output scripts (i.e. unused()) + if (!desc.descriptor->IsRange() && desc.descriptor->HasScripts()) { auto script_pub_keys = spk_man->GetScriptPubKeys(); if (script_pub_keys.empty()) { return util::Error{_("Could not generate scriptPubKeys (cache is empty)")}; From 05fb11d0c24c12516d29eb3eb72f3ce4430ba23e Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Thu, 4 Jan 2024 12:00:16 -0500 Subject: [PATCH 02/32] test: Simple test for importing unused(KEY) --- test/functional/wallet_importdescriptors.py | 25 +++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py index fdaeae35afd6..358b1fb6e526 100755 --- a/test/functional/wallet_importdescriptors.py +++ b/test/functional/wallet_importdescriptors.py @@ -65,6 +65,29 @@ def test_importdesc(self, req, success, error_code=None, error_message=None, war assert_equal(result[0]['error']['code'], error_code) assert_equal(result[0]['error']['message'], error_message) + def test_import_unused_key(self): + self.log.info("Test import of unused(KEY)") + self.nodes[0].createwallet(wallet_name="import_unused", blank=True) + wallet = self.nodes[0].get_wallet_rpc("import_unused") + + assert_equal(len(wallet.gethdkeys()), 0) + + xprv = "tprv8ZgxMBicQKsPeuVhWwi6wuMQGfPKi9Li5GtX35jVNknACgqe3CY4g5xgkfDDJcmtF7o1QnxWDRYw4H5P26PXq7sbcUkEqeR4fg3Kxp2tigg" + xpub = "tpubD6NzVbkrYhZ4YNXVQbNhMK1WqguFsUXceaVJKbmno2aZ3B6QfbMeraaYvnBSGpV3vxLyTTK9DYT1yoEck4XUScMzXoQ2U2oSmE2JyMedq3H" + self.test_importdesc({"desc":descsum_create(f"unused({xpub})"), + "timestamp": "now"}, + success=False, + error_code=-4, + error_message='Cannot import descriptor without private keys to a wallet with private keys enabled', + wallet=wallet) + self.test_importdesc({"timestamp": "now", "desc": descsum_create(f"unused({xprv})")}, + success=True, + wallet=wallet) + hdkeys = wallet.gethdkeys() + assert_equal(len(hdkeys), 1) + assert_equal(hdkeys[0]["xpub"], xpub) + wallet.unloadwallet() + def run_test(self): self.log.info('Setting up wallets') self.nodes[0].createwallet(wallet_name='w0', disable_private_keys=False) @@ -820,5 +843,7 @@ def run_test(self): ) + self.test_import_unused_key() + if __name__ == '__main__': ImportDescriptorsTest(__file__).main() From 7109d3aac499885927139ebd525307814cb83e68 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 22 Dec 2023 17:07:18 -0500 Subject: [PATCH 03/32] wallet: Add addhdkey RPC --- src/wallet/rpc/wallet.cpp | 77 ++++++++++++++++++++++++++++++++++++ test/functional/wallet_hd.py | 54 +++++++++++++++++++++++++ 2 files changed, 131 insertions(+) diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index c7c3f459fffa..889b40860ff6 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -845,6 +845,82 @@ static RPCHelpMan createwalletdescriptor() }; } +RPCHelpMan addhdkey() +{ + return RPCHelpMan{ + "addhdkey", + "Add a BIP 32 HD key to the wallet that can be used with 'createwalletdescriptor'\n", + { + {"hdkey", RPCArg::Type::STR, RPCArg::DefaultHint{"Automatically generated new key"}, "The BIP 32 extended private key to add. If none is provided, a randomly generated one will be added."}, + }, + RPCResult{ + RPCResult::Type::OBJ, "", "", + { + {RPCResult::Type::STR, "xpub", "The xpub of the HD key that was added to the wallet"} + }, + }, + RPCExamples{ + HelpExampleCli("addhdkey", "xprv") + HelpExampleRpc("addhdkey", "xprv") + }, + [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue + { + std::shared_ptr const wallet = GetWalletForJSONRPCRequest(request); + if (!wallet) return UniValue::VNULL; + + if (wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { + throw JSONRPCError(RPC_WALLET_ERROR, "addhdkey is not available for wallets without private keys"); + } + + EnsureWalletIsUnlocked(*wallet); + + CExtKey hdkey; + if (request.params[0].isNull()) { + CKey seed_key = GenerateRandomKey(); + hdkey.SetSeed(seed_key); + } else { + hdkey = DecodeExtKey(request.params[0].get_str()); + if (!hdkey.key.IsValid()) { + // Check if the user gave us an xpub and give a more descriptive error if so + CExtPubKey xpub = DecodeExtPubKey(request.params[0].get_str()); + if (xpub.pubkey.IsValid()) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Extended public key (xpub) provided, but extended private key (xprv) is required"); + } else { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Could not parse HD key"); + } + } + } + + LOCK(wallet->cs_wallet); + std::string desc_str = "unused(" + EncodeExtKey(hdkey) + ")"; + FlatSigningProvider keys; + std::string error; + std::vector> descs = Parse(desc_str, keys, error, false); + CHECK_NONFATAL(!descs.empty()); + WalletDescriptor w_desc(std::move(descs.at(0)), GetTime(), 0, 0, 0); + if (wallet->GetDescriptorScriptPubKeyMan(w_desc) != nullptr) { + throw JSONRPCError(RPC_WALLET_ERROR, "HD key already exists"); + } + + auto spkm = wallet->AddWalletDescriptor(w_desc, keys, /*label=*/"", /*internal=*/false); + if (!spkm) { + throw JSONRPCError(RPC_WALLET_ERROR, util::ErrorString(spkm).original); + } + + UniValue response(UniValue::VOBJ); + const DescriptorScriptPubKeyMan& desc_spkm = spkm->get(); + LOCK(desc_spkm.cs_desc_man); + std::set pubkeys; + std::set extpubs; + desc_spkm.GetWalletDescriptor().descriptor->GetPubKeys(pubkeys, extpubs); + CHECK_NONFATAL(pubkeys.size() == 0); + CHECK_NONFATAL(extpubs.size() == 1); + response.pushKV("xpub", EncodeExtPubKey(*extpubs.begin())); + + return response; + }, + }; +} + // addresses RPCHelpMan getaddressinfo(); RPCHelpMan getnewaddress(); @@ -913,6 +989,7 @@ std::span GetWalletRPCCommands() {"rawtransactions", &fundrawtransaction}, {"wallet", &abandontransaction}, {"wallet", &abortrescan}, + {"wallet", &addhdkey}, {"wallet", &backupwallet}, {"wallet", &bumpfee}, {"wallet", &psbtbumpfee}, diff --git a/test/functional/wallet_hd.py b/test/functional/wallet_hd.py index 62ad88d2d7d7..44cdb41075af 100755 --- a/test/functional/wallet_hd.py +++ b/test/functional/wallet_hd.py @@ -7,10 +7,12 @@ import shutil 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, wallet_importprivkey, + assert_raises_rpc_error, ) @@ -25,6 +27,56 @@ def set_test_params(self): def skip_test_if_missing_module(self): self.skip_if_no_wallet() + def test_addhdkey(self): + self.log.info("Test addhdkey") + def_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + self.nodes[0].createwallet("hdkey") + wallet = self.nodes[0].get_wallet_rpc("hdkey") + + assert_equal(len(wallet.gethdkeys()), 1) + + wallet.addhdkey() + xpub_info = wallet.gethdkeys() + assert_equal(len(xpub_info), 2) + for x in xpub_info: + if len(x["descriptors"]) == 1 and x["descriptors"][0]["desc"].startswith("unused("): + break + else: + assert False, "Did not find HD key with no descriptors" + + imp_xpub_info = def_wallet.gethdkeys(private=True)[0] + imp_xpub = imp_xpub_info["xpub"] + imp_xprv = imp_xpub_info["xprv"] + + assert_raises_rpc_error(-5, "Extended public key (xpub) provided, but extended private key (xprv) is required", wallet.addhdkey, imp_xpub) + add_res = wallet.addhdkey(imp_xprv) + expected_unused_desc = descsum_create(f"unused({imp_xpub})") + assert_equal(add_res["xpub"], imp_xpub) + xpub_info = wallet.gethdkeys() + assert_equal(len(xpub_info), 3) + for x in xpub_info: + if x["xpub"] == imp_xpub: + assert_equal(len(x["descriptors"]), 1) + assert_equal(x["descriptors"][0]["desc"], expected_unused_desc) + break + else: + assert False, "Added HD key was not found in wallet" + + for d in wallet.listdescriptors()["descriptors"]: + if d["desc"] == expected_unused_desc: + assert_equal(d["active"], False) + break + else: + assert False, "Added HD key's descriptor was not found in wallet" + + assert_raises_rpc_error(-4, "HD key already exists", wallet.addhdkey, imp_xprv) + + def test_addhdkey_noprivs(self): + self.log.info("Test addhdkey is not available for wallets without privkeys") + self.nodes[0].createwallet("hdkey_noprivs", disable_private_keys=True) + wallet = self.nodes[0].get_wallet_rpc("hdkey_noprivs") + assert_raises_rpc_error(-4, "addhdkey is not available for wallets without private keys", wallet.addhdkey) + def run_test(self): # Make sure we use hd, keep masterkeyid hd_fingerprint = self.nodes[1].getaddressinfo(self.nodes[1].getnewaddress())['hdmasterfingerprint'] @@ -124,6 +176,8 @@ def run_test(self): assert_equal(keypath[0:14], "m/84h/1h/0h/1/") + self.test_addhdkey() + self.test_addhdkey_noprivs() if __name__ == '__main__': WalletHDTest(__file__).main() From 8a73206cd65fd403e3749d1bbf504f86e4fbfc3d Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Thu, 4 Jan 2024 13:50:35 -0500 Subject: [PATCH 04/32] wallet, rpc: Disallow import of unused() if key already exists --- src/wallet/rpc/backup.cpp | 17 +++++++++++++++++ test/functional/wallet_importdescriptors.py | 17 +++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index 6217358c870f..d12022272b71 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -265,6 +265,23 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c } } + // If this is an unused(KEY) descriptor, check that the wallet doesn't already have other descriptors with this key + if (!parsed_desc->HasScripts()) { + // Unused descriptors must contain a single key. + // Earlier checks will have enforced that this key is either a private key when private keys are enabled, + // or that this key is a public key when private keys are disabled. + // If we can retrieve the corresponding private key from the wallet, then this key is already in the wallet + // and we should not import it. + std::set pubkeys; + std::set extpubs; + parsed_desc->GetPubKeys(pubkeys, extpubs); + std::transform(extpubs.begin(), extpubs.end(), std::inserter(pubkeys, pubkeys.begin()), [](const CExtPubKey& xpub) { return xpub.pubkey; }); + CHECK_NONFATAL(pubkeys.size() == 1); + if (wallet.GetKey(pubkeys.begin()->GetID())) { + throw JSONRPCError(RPC_WALLET_ERROR, "Cannot import an unused() descriptor when its private key is already in the wallet"); + } + } + WalletDescriptor w_desc(std::move(parsed_desc), timestamp, range_start, range_end, next_index); // Add descriptor to the wallet diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py index 358b1fb6e526..9799f9f2ab12 100755 --- a/test/functional/wallet_importdescriptors.py +++ b/test/functional/wallet_importdescriptors.py @@ -88,6 +88,22 @@ def test_import_unused_key(self): assert_equal(hdkeys[0]["xpub"], xpub) wallet.unloadwallet() + def test_import_unused_key_existing(self): + self.log.info("Test import of unused(KEY) with existing KEY") + self.nodes[0].createwallet(wallet_name="import_existing_unused") + wallet = self.nodes[0].get_wallet_rpc("import_existing_unused") + + hdkeys = wallet.gethdkeys(private=True) + assert_equal(len(hdkeys), 1) + xprv = hdkeys[0]["xprv"] + + self.test_importdesc({"timestamp": "now", "desc": descsum_create(f"unused({xprv})")}, + success=False, + error_code=-4, + error_message="Cannot import an unused() descriptor when its private key is already in the wallet", + wallet=wallet) + wallet.unloadwallet() + def run_test(self): self.log.info('Setting up wallets') self.nodes[0].createwallet(wallet_name='w0', disable_private_keys=False) @@ -844,6 +860,7 @@ def run_test(self): self.test_import_unused_key() + self.test_import_unused_key_existing() if __name__ == '__main__': ImportDescriptorsTest(__file__).main() From 1e62c536fa29f7b7188f612749179000a0d5a59c Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Thu, 4 Jan 2024 14:02:19 -0500 Subject: [PATCH 05/32] wallet, rpc: Disallow importing unused() to wallets without privkeys --- src/wallet/rpc/backup.cpp | 3 +++ test/functional/wallet_importdescriptors.py | 15 +++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index d12022272b71..f6b57264f6c4 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -267,6 +267,9 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c // If this is an unused(KEY) descriptor, check that the wallet doesn't already have other descriptors with this key if (!parsed_desc->HasScripts()) { + if (wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { + throw JSONRPCError(RPC_WALLET_ERROR, "Cannot import unused() to wallet without private keys enabled"); + } // Unused descriptors must contain a single key. // Earlier checks will have enforced that this key is either a private key when private keys are enabled, // or that this key is a public key when private keys are disabled. diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py index 9799f9f2ab12..c02824810c44 100755 --- a/test/functional/wallet_importdescriptors.py +++ b/test/functional/wallet_importdescriptors.py @@ -104,6 +104,20 @@ def test_import_unused_key_existing(self): wallet=wallet) wallet.unloadwallet() + def test_import_unused_noprivs(self): + self.log.info("Test import of unused(KEY) to wallet without privkeys") + self.nodes[0].createwallet(wallet_name="import_unused_noprivs", disable_private_keys=True) + wallet = self.nodes[0].get_wallet_rpc("import_unused_noprivs") + + xpub = "tpubD6NzVbkrYhZ4YNXVQbNhMK1WqguFsUXceaVJKbmno2aZ3B6QfbMeraaYvnBSGpV3vxLyTTK9DYT1yoEck4XUScMzXoQ2U2oSmE2JyMedq3H" + self.test_importdesc({"timestamp": "now", "desc": descsum_create(f"unused({xpub})")}, + success=False, + error_code=-4, + error_message="Cannot import unused() to wallet without private keys enabled", + wallet=wallet) + wallet.unloadwallet() + + def run_test(self): self.log.info('Setting up wallets') self.nodes[0].createwallet(wallet_name='w0', disable_private_keys=False) @@ -861,6 +875,7 @@ def run_test(self): self.test_import_unused_key() self.test_import_unused_key_existing() + self.test_import_unused_noprivs() if __name__ == '__main__': ImportDescriptorsTest(__file__).main() From 23f5529077dc984498e80b0d6f818d5d664362de Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Thu, 24 Jul 2025 14:04:39 -0700 Subject: [PATCH 06/32] doc: Release note for addhdkey --- doc/release-notes-29136.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 doc/release-notes-29136.md diff --git a/doc/release-notes-29136.md b/doc/release-notes-29136.md new file mode 100644 index 000000000000..af074a0b30ad --- /dev/null +++ b/doc/release-notes-29136.md @@ -0,0 +1,6 @@ +Wallet +------ +- A new RPC `addhdkey` is added which allows a BIP 32 extended key to be added to the wallet without + needing to import it as part of a separate descriptor. This key will not be used to produce any + output scripts unless it is explicitly imported as part of a separate descriptor independent of + the `addhdkey` RPC. From 8f7aa62662931988f3e866f0a3d03d71f2cc5c93 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 21 Jul 2025 09:45:45 +0200 Subject: [PATCH 07/32] wallet: add bip388_hmac record --- src/wallet/wallet.cpp | 4 ++++ src/wallet/wallet.h | 14 ++++++++++++++ src/wallet/walletdb.cpp | 34 ++++++++++++++++++++++++++++++++++ src/wallet/walletdb.h | 5 +++++ 4 files changed, 57 insertions(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index bc27018cd2f8..c2f45fc86912 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1793,6 +1793,10 @@ uint64_t CWallet::GetWalletFlags() const return m_wallet_flags; } +void CWallet::LoadHmacBIP388(const std::string& policy_name, const std::string& fingerprint, const std::string& hmac) { + m_bip388.push_back({.name = policy_name, .fingerprint = fingerprint, .hmac = hmac}); +} + void CWallet::MaybeUpdateBirthTime(int64_t time) { int64_t birthtime = m_birth_time.load(); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 4250acca69e9..cea5b3bb9007 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -304,6 +304,13 @@ struct CRecipient bool fSubtractFeeFromAmount; }; +/** BIP388 registered hmac */ +struct BIP388 { + std::string name; + std::string fingerprint; + std::string hmac; +}; + class WalletRescanReserver; //forward declarations for ScanForWalletTransactions/RescanFromTime /** * A CWallet maintains a set of transactions and balances, and provides the ability to create new transactions. @@ -383,6 +390,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati /** WalletFlags set on this wallet. */ std::atomic m_wallet_flags{0}; + std::vector m_bip388; + bool SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& address, const std::string& strName, const std::optional& strPurpose); //! Unsets a wallet flag and saves it to disk @@ -917,6 +926,11 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati //! Retrieve all of the wallet's flags uint64_t GetWalletFlags() const; + std::vector GetHmacs() const { return m_bip388; } + + //! Load BIP388 registered hmac + void LoadHmacBIP388(const std::string& policy_name, const std::string& fingerprint, const std::string& hmac); + /** Return wallet name for use in logs, will return "default wallet" if the wallet has no name. */ std::string LogName() const override { diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 617b8282d9dc..b051a04d08db 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -33,6 +33,7 @@ const std::string ACENTRY{"acentry"}; const std::string ACTIVEEXTERNALSPK{"activeexternalspk"}; const std::string ACTIVEINTERNALSPK{"activeinternalspk"}; const std::string BESTBLOCK_NOMERKLE{"bestblock_nomerkle"}; +const std::string BIP388_HMAC{"bip388_hmac"}; const std::string BESTBLOCK{"bestblock"}; const std::string CRYPTED_KEY{"ckey"}; const std::string CSCRIPT{"cscript"}; @@ -932,6 +933,31 @@ static DBErrors LoadDescriptorWalletRecords(CWallet* pwallet, DatabaseBatch& bat return desc_res.m_result; } +static DBErrors LoadWalletPolicyRecords(CWallet* pwallet, DatabaseBatch& batch, int last_client) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet) +{ + AssertLockHeld(pwallet->cs_wallet); + + LoadResult res = LoadRecords(pwallet, batch, DBKeys::BIP388_HMAC, + [] (CWallet* pwallet, DataStream& key, DataStream& value, std::string& err) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet) { + std::pair key_pair; + std::string hmac; + key >> key_pair; + value >> hmac; + std::string policy_name = key_pair.first; + std::string fingerprint = key_pair.second; + pwallet->LoadHmacBIP388(policy_name, fingerprint, hmac); + return DBErrors::LOAD_OK; + }); + + if (res.m_result <= DBErrors::NONCRITICAL_ERROR) { + // Only log if there are no critical errors + pwallet->WalletLogPrintf("Loaded BIP388 policy"); + } + + return res.m_result; +} + + static DBErrors LoadAddressBookRecords(CWallet* pwallet, DatabaseBatch& batch) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet) { AssertLockHeld(pwallet->cs_wallet); @@ -1146,6 +1172,9 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) // when in reality the wallet is simply too new. if (result == DBErrors::UNKNOWN_DESCRIPTOR) return result; + // Load BIP388 HMAC record + result = std::max(LoadWalletPolicyRecords(pwallet, *m_batch, last_client), result); + // Load address book result = std::max(LoadAddressBookRecords(pwallet, *m_batch), result); @@ -1263,6 +1292,11 @@ bool WalletBatch::WriteWalletFlags(const uint64_t flags) return WriteIC(DBKeys::FLAGS, flags); } +bool WalletBatch::WriteHmacBip388(const std::string& policy_name, const std::string& fingerprint, const std::string& hmac) +{ + return WriteIC(std::make_pair(DBKeys::BIP388_HMAC, std::make_pair(policy_name, fingerprint)), hmac); +} + bool WalletBatch::EraseRecords(const std::unordered_set& types) { return std::all_of(types.begin(), types.end(), [&](const std::string& type) { diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index c4466bfaef3f..06d9a4651ca0 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -61,6 +61,7 @@ extern const std::string ACTIVEEXTERNALSPK; extern const std::string ACTIVEINTERNALSPK; extern const std::string BESTBLOCK; extern const std::string BESTBLOCK_NOMERKLE; +extern const std::string BIP388_HMAC; extern const std::string CRYPTED_KEY; extern const std::string CSCRIPT; extern const std::string DEFAULTKEY; @@ -269,6 +270,10 @@ class WalletBatch bool EraseRecords(const std::unordered_set& types); bool WriteWalletFlags(uint64_t flags); + + // Write a BIP-388 policy hmac for the signer identified by fingerprint. + bool WriteHmacBip388(const std::string& policy_name, const std::string& fingerprint, const std::string& hmac); + //! Begin a new transaction bool TxnBegin(); //! Commit current transaction From d7e974496b9aeaeded74029e98e86de285f055fc Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 21 Jul 2025 09:54:33 +0200 Subject: [PATCH 08/32] wallet: add RegisterPolicy to external signer --- src/external_signer.cpp | 9 +++++++++ src/external_signer.h | 8 ++++++++ 2 files changed, 17 insertions(+) diff --git a/src/external_signer.cpp b/src/external_signer.cpp index 84d98a199062..edd50836fdd1 100644 --- a/src/external_signer.cpp +++ b/src/external_signer.cpp @@ -71,6 +71,15 @@ UniValue ExternalSigner::GetDescriptors(const int account) return RunCommandParseJSON(m_command + " --fingerprint " + m_fingerprint + NetworkArg() + " getdescriptors --account " + strprintf("%d", account)); } +UniValue ExternalSigner::RegisterPolicy(const std::string& name, const std::string& descriptor_template, const std::vector& keys_info) const +{ + std::string key_args; + for (const std::string& key_info : keys_info) { + key_args.append(" --key " + key_info); + } + return RunCommandParseJSON(m_command + " --fingerprint " + m_fingerprint + NetworkArg() + " register --name " + name + " --desc " + descriptor_template + key_args); +} + bool ExternalSigner::SignTransaction(PartiallySignedTransaction& psbtx, std::string& error) { // Serialize the PSBT diff --git a/src/external_signer.h b/src/external_signer.h index 1b36d49622e1..56de2bb85596 100644 --- a/src/external_signer.h +++ b/src/external_signer.h @@ -57,6 +57,14 @@ class ExternalSigner //! @returns see doc/external-signer.md UniValue GetDescriptors(int account); + //! Register BIP388 policy on the device. + //! Calls ` register` and passes the name, policy and key info + //! @param[in] name policy name to display on the signer + //! @param[in] descriptor_template BIP388 descriptor template + //! @param[in] keys_info key with origin for each participant + //! @returns hmac provided by the signer + UniValue RegisterPolicy(const std::string& name, const std::string& descriptor_template, const std::vector& keys_info) const; + //! Sign PartiallySignedTransaction on the device. //! Calls ` signtransaction` and passes the PSBT via stdin. //! @param[in,out] psbt PartiallySignedTransaction to be signed From 281173233ff5981f2503c32f97d77acbb5c21565 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 21 Jul 2025 10:12:48 +0200 Subject: [PATCH 09/32] wallet: add RegisterPolicy to signer SPKM --- .../external_signer_scriptpubkeyman.cpp | 20 +++++++++++++++++++ src/wallet/external_signer_scriptpubkeyman.h | 12 +++++++++++ 2 files changed, 32 insertions(+) diff --git a/src/wallet/external_signer_scriptpubkeyman.cpp b/src/wallet/external_signer_scriptpubkeyman.cpp index f66686254a45..69df202e6451 100644 --- a/src/wallet/external_signer_scriptpubkeyman.cpp +++ b/src/wallet/external_signer_scriptpubkeyman.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -107,4 +108,23 @@ std::optional ExternalSignerScriptPubKeyMan::FillPSBT(PartiallySigned if (finalize) FinalizePSBT(psbt); // This won't work in a multisig setup return {}; } + +util::Result ExternalSignerScriptPubKeyMan::RegisterPolicy(const ExternalSigner& signer, + const std::string& name, + const std::string& descriptor_template, + const std::vector& keys_info) const +{ + const UniValue& result{signer.RegisterPolicy(name, descriptor_template, keys_info)}; + + const UniValue& error = result.find_value("error"); + if (error.isStr()) return util::Error{strprintf(_("Signer returned error: %s"), error.getValStr())}; + + const UniValue& ret_hmac = result.find_value("hmac"); + if (!ret_hmac.isStr()) return util::Error{_("Signer did not return hmac")}; + const std::string hmac{ret_hmac.getValStr()}; + if (!IsHex(hmac)) return util::Error{strprintf(_("Signer return invalid hmac: %s"), hmac)}; + + return util::Result(hmac); +} + } // namespace wallet diff --git a/src/wallet/external_signer_scriptpubkeyman.h b/src/wallet/external_signer_scriptpubkeyman.h index 0ccc243c2569..7b7a88e5339f 100644 --- a/src/wallet/external_signer_scriptpubkeyman.h +++ b/src/wallet/external_signer_scriptpubkeyman.h @@ -37,6 +37,18 @@ class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan util::Result DisplayAddress(const CTxDestination& dest, const ExternalSigner& signer) const; std::optional FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, std::optional sighash_type = std::nullopt, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr, bool finalize = true) const override; + + /** + * Register BIP388 wallet policy. + * @param[in] name policy name to display on the signer + * @param[in] descriptor_template BIP388 descriptor template + * @param[in] keys_info key with origin for each participant + * @returns hmac or an error message + */ + util::Result RegisterPolicy(const ExternalSigner& signer, + const std::string& name, + const std::string& descriptor_template, + const std::vector& keys_info) const; }; } // namespace wallet #endif // BITCOIN_WALLET_EXTERNAL_SIGNER_SCRIPTPUBKEYMAN_H From f04cd6025da0c06b6b19c67915e08ac91102fa14 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 5 Aug 2025 20:49:38 +0200 Subject: [PATCH 10/32] refactor: avoid std::regex in ReplaceAll --- src/util/string.cpp | 12 ++++++++++-- src/util/string.h | 2 +- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/util/string.cpp b/src/util/string.cpp index 507d9d317182..235ab9a8e4f5 100644 --- a/src/util/string.cpp +++ b/src/util/string.cpp @@ -8,9 +8,17 @@ #include namespace util { -void ReplaceAll(std::string& in_out, const std::string& search, const std::string& substitute) +void ReplaceAll(std::string& in_out, const std::string& search, const std::string& substitute, bool regex) { if (search.empty()) return; - in_out = std::regex_replace(in_out, std::regex(search), substitute); + if (regex) { + in_out = std::regex_replace(in_out, std::regex(search), substitute); + return; + } + size_t pos{0}; + while ((pos = in_out.find(search, pos)) != std::string::npos) { + in_out.replace(pos, search.size(), substitute); + ++pos; + } } } // namespace util diff --git a/src/util/string.h b/src/util/string.h index 2578e530af51..b283ddef912f 100644 --- a/src/util/string.h +++ b/src/util/string.h @@ -94,7 +94,7 @@ struct ConstevalFormatString { consteval ConstevalFormatString(const char* str) : fmt{str} { detail::CheckNumFormatSpecifiers(fmt); } }; -void ReplaceAll(std::string& in_out, const std::string& search, const std::string& substitute); +void ReplaceAll(std::string& in_out, const std::string& search, const std::string& substitute, bool regex = true); /** Split a string on any char found in separators, returning a vector. * From 7a039675f97e2f055061ac310ad396586759c4c4 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 21 Jul 2025 10:13:29 +0200 Subject: [PATCH 11/32] wallet: add registerPolicy() --- src/interfaces/wallet.h | 3 + src/wallet/interfaces.cpp | 5 ++ src/wallet/wallet.cpp | 164 +++++++++++++++++++++++++++++++++++++- src/wallet/wallet.h | 22 +++++ 4 files changed, 191 insertions(+), 3 deletions(-) diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 40c612a4c511..fa723dbe8ee7 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -129,6 +129,9 @@ class Wallet //! Display address on external signer virtual util::Result displayAddress(const CTxDestination& dest) = 0; + //! Register BIP388 policy on external signer, store and return hmac + virtual util::Result registerPolicy(const std::optional& name) = 0; + //! Lock coin. virtual bool lockCoin(const COutPoint& output, bool write_to_db) = 0; diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 28c4d639323a..4ed47ca02669 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -237,6 +237,11 @@ class WalletImpl : public Wallet LOCK(m_wallet->cs_wallet); return m_wallet->DisplayAddress(dest); } + util::Result registerPolicy(const std::optional& name) override + { + LOCK(m_wallet->cs_wallet); + return m_wallet->RegisterPolicy(name); + } bool lockCoin(const COutPoint& output, const bool write_to_db) override { LOCK(m_wallet->cs_wallet); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c2f45fc86912..91063712083f 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -36,7 +36,9 @@ #include #include #include +#include #include