From bf68ebc1cd555f791103f81adc9111e0e55c8003 Mon Sep 17 00:00:00 2001 From: S3RK <1466284+S3RK@users.noreply.github.com> Date: Mon, 28 Jun 2021 21:37:37 +0200 Subject: wallet: allow to import same descriptor twice --- src/wallet/rpcdump.cpp | 5 +- src/wallet/scriptpubkeyman.cpp | 40 +++++++++++++++ src/wallet/scriptpubkeyman.h | 2 + src/wallet/wallet.cpp | 48 +++++------------- test/functional/wallet_importdescriptors.py | 78 ++++++++++++++++++++++++----- 5 files changed, 122 insertions(+), 51 deletions(-) diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 35649ab02c..3ae36131cc 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -1566,9 +1566,8 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c // Check if the wallet already contains the descriptor auto existing_spk_manager = wallet.GetDescriptorScriptPubKeyMan(w_desc); if (existing_spk_manager) { - LOCK(existing_spk_manager->cs_desc_man); - if (range_start > existing_spk_manager->GetWalletDescriptor().range_start) { - throw JSONRPCError(RPC_INVALID_PARAMS, strprintf("range_start can only decrease; current range = [%d,%d]", existing_spk_manager->GetWalletDescriptor().range_start, existing_spk_manager->GetWalletDescriptor().range_end)); + if (!existing_spk_manager->CanUpdateToWalletDescriptor(w_desc, error)) { + throw JSONRPCError(RPC_INVALID_PARAMETER, error); } } diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 44c3912544..8b397fa1f3 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1875,6 +1875,12 @@ bool DescriptorScriptPubKeyMan::AddDescriptorKeyWithDB(WalletBatch& batch, const AssertLockHeld(cs_desc_man); assert(!m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)); + // Check if provided key already exists + if (m_map_keys.find(pubkey.GetID()) != m_map_keys.end() || + m_map_crypted_keys.find(pubkey.GetID()) != m_map_crypted_keys.end()) { + return true; + } + if (m_storage.HasEncryptionKeys()) { if (m_storage.IsLocked()) { return false; @@ -2302,3 +2308,37 @@ bool DescriptorScriptPubKeyMan::GetDescriptorString(std::string& out, bool priv) return m_wallet_descriptor.descriptor->ToNormalizedString(provider, out, priv); } + +void DescriptorScriptPubKeyMan::UpdateWalletDescriptor(WalletDescriptor& descriptor) +{ + LOCK(cs_desc_man); + std::string error; + if (!CanUpdateToWalletDescriptor(descriptor, error)) { + throw std::runtime_error(std::string(__func__) + ": " + error); + } + + m_map_pubkeys.clear(); + m_map_script_pub_keys.clear(); + m_max_cached_index = -1; + m_wallet_descriptor = descriptor; +} + +bool DescriptorScriptPubKeyMan::CanUpdateToWalletDescriptor(const WalletDescriptor& descriptor, std::string& error) +{ + LOCK(cs_desc_man); + if (!HasWalletDescriptor(descriptor)) { + error = "can only update matching descriptor"; + return false; + } + + if (descriptor.range_start > m_wallet_descriptor.range_start || + descriptor.range_end < m_wallet_descriptor.range_end) { + // Use inclusive range for error + error = strprintf("new range must include current range = [%d,%d]", + m_wallet_descriptor.range_start, + m_wallet_descriptor.range_end - 1); + return false; + } + + return true; +} diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index b2ca354b0a..f2d1d87d55 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -624,6 +624,8 @@ public: bool AddCryptedKey(const CKeyID& key_id, const CPubKey& pubkey, const std::vector& crypted_key); bool HasWalletDescriptor(const WalletDescriptor& desc) const; + void UpdateWalletDescriptor(WalletDescriptor& descriptor); + bool CanUpdateToWalletDescriptor(const WalletDescriptor& descriptor, std::string& error); void AddDescriptorKey(const CKey& key, const CPubKey &pubkey); void WriteDescriptor(); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c2586b60b8..8750f508a7 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3192,44 +3192,26 @@ ScriptPubKeyMan* CWallet::AddWalletDescriptor(WalletDescriptor& desc, const Flat } LOCK(cs_wallet); - auto new_spk_man = std::unique_ptr(new DescriptorScriptPubKeyMan(*this, desc)); - - // If we already have this descriptor, remove it from the maps but add the existing cache to desc - auto old_spk_man = GetDescriptorScriptPubKeyMan(desc); - if (old_spk_man) { + auto spk_man = GetDescriptorScriptPubKeyMan(desc); + if (spk_man) { WalletLogPrintf("Update existing descriptor: %s\n", desc.descriptor->ToString()); + spk_man->UpdateWalletDescriptor(desc); + } else { + auto new_spk_man = std::unique_ptr(new DescriptorScriptPubKeyMan(*this, desc)); + spk_man = new_spk_man.get(); - { - LOCK(old_spk_man->cs_desc_man); - new_spk_man->SetCache(old_spk_man->GetWalletDescriptor().cache); - } - - // Remove from maps of active spkMans - auto old_spk_man_id = old_spk_man->GetID(); - for (bool internal : {false, true}) { - for (OutputType t : OUTPUT_TYPES) { - auto active_spk_man = GetScriptPubKeyMan(t, internal); - if (active_spk_man && active_spk_man->GetID() == old_spk_man_id) { - if (internal) { - m_internal_spk_managers.erase(t); - } else { - m_external_spk_managers.erase(t); - } - break; - } - } - } - m_spk_managers.erase(old_spk_man_id); + // Save the descriptor to memory + m_spk_managers[new_spk_man->GetID()] = std::move(new_spk_man); } // Add the private keys to the descriptor for (const auto& entry : signing_provider.keys) { const CKey& key = entry.second; - new_spk_man->AddDescriptorKey(key, key.GetPubKey()); + spk_man->AddDescriptorKey(key, key.GetPubKey()); } // Top up key pool, the manager will generate new scriptPubKeys internally - if (!new_spk_man->TopUp()) { + if (!spk_man->TopUp()) { WalletLogPrintf("Could not top up scriptPubKeys\n"); return nullptr; } @@ -3237,7 +3219,7 @@ ScriptPubKeyMan* CWallet::AddWalletDescriptor(WalletDescriptor& desc, const Flat // Apply the label if necessary // Note: we disable labels for ranged descriptors if (!desc.descriptor->IsRange()) { - auto script_pub_keys = new_spk_man->GetScriptPubKeys(); + auto script_pub_keys = spk_man->GetScriptPubKeys(); if (script_pub_keys.empty()) { WalletLogPrintf("Could not generate scriptPubKeys (cache is empty)\n"); return nullptr; @@ -3249,12 +3231,8 @@ ScriptPubKeyMan* CWallet::AddWalletDescriptor(WalletDescriptor& desc, const Flat } } - // Save the descriptor to memory - auto ret = new_spk_man.get(); - m_spk_managers[new_spk_man->GetID()] = std::move(new_spk_man); - // Save the descriptor to DB - ret->WriteDescriptor(); + spk_man->WriteDescriptor(); - return ret; + return spk_man; } diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py index a2da16e5a3..bf1c8fb2e4 100755 --- a/test/functional/wallet_importdescriptors.py +++ b/test/functional/wallet_importdescriptors.py @@ -89,10 +89,10 @@ class ImportDescriptorsTest(BitcoinTestFramework): # # Test importing of a P2PKH descriptor key = get_generate_key() self.log.info("Should import a p2pkh descriptor") - self.test_importdesc({"desc": descsum_create("pkh(" + key.pubkey + ")"), - "timestamp": "now", - "label": "Descriptor import test"}, - success=True) + import_request = {"desc": descsum_create("pkh(" + key.pubkey + ")"), + "timestamp": "now", + "label": "Descriptor import test"} + self.test_importdesc(import_request, success=True) test_address(w1, key.p2pkh_addr, solvable=True, @@ -100,6 +100,9 @@ class ImportDescriptorsTest(BitcoinTestFramework): labels=["Descriptor import test"]) assert_equal(w1.getwalletinfo()['keypoolsize'], 0) + self.log.info("Test can import same descriptor with public key twice") + self.test_importdesc(import_request, success=True) + self.log.info("Internal addresses cannot have labels") self.test_importdesc({"desc": descsum_create("pkh(" + key.pubkey + ")"), "timestamp": "now", @@ -305,7 +308,7 @@ class ImportDescriptorsTest(BitcoinTestFramework): # Check active=False default self.log.info('Check imported descriptors are not active by default') - self.test_importdesc({'desc': descsum_create('pkh([12345678/0h/0h]' + xpub + '/*)'), + self.test_importdesc({'desc': descsum_create('pkh([12345678/1h]' + xpub + '/*)'), 'range' : [0, 2], 'timestamp': 'now', 'internal': True @@ -322,6 +325,10 @@ class ImportDescriptorsTest(BitcoinTestFramework): "timestamp": "now"}, success=True, wallet=wpriv) + + self.log.info('Test can import same descriptor with private key twice') + self.test_importdesc({"desc": descsum_create(desc), "timestamp": "now"}, success=True, wallet=wpriv) + test_address(wpriv, address, solvable=True, @@ -339,14 +346,25 @@ class ImportDescriptorsTest(BitcoinTestFramework): wmulti_priv = self.nodes[1].get_wallet_rpc("wmulti_priv") assert_equal(wmulti_priv.getwalletinfo()['keypoolsize'], 0) - self.test_importdesc({"desc":"wsh(multi(2,tprv8ZgxMBicQKsPevADjDCWsa6DfhkVXicu8NQUzfibwX2MexVwW4tCec5mXdCW8kJwkzBRRmAay1KZya4WsehVvjTGVW6JLqiqd8DdZ4xSg52/84h/0h/0h/*,tprv8ZgxMBicQKsPdSNWUhDiwTScDr6JfkZuLshTRwzvZGnMSnGikV6jxpmdDkC3YRc4T3GD6Nvg9uv6hQg73RVv1EiTXDZwxVbsLugVHU8B1aq/84h/0h/0h/*,tprv8ZgxMBicQKsPeonDt8Ka2mrQmHa61hQ5FQCsvWBTpSNzBFgM58cV2EuXNAHF14VawVpznnme3SuTbA62sGriwWyKifJmXntfNeK7zeqMCj1/84h/0h/0h/*))#m2sr93jn", + xprv1 = 'tprv8ZgxMBicQKsPevADjDCWsa6DfhkVXicu8NQUzfibwX2MexVwW4tCec5mXdCW8kJwkzBRRmAay1KZya4WsehVvjTGVW6JLqiqd8DdZ4xSg52' + acc_xpub1 = 'tpubDCJtdt5dgJpdhW4MtaVYDhG4T4tF6jcLR1PxL43q9pq1mxvXgMS9Mzw1HnXG15vxUGQJMMSqCQHMTy3F1eW5VkgVroWzchsPD5BUojrcWs8' # /84'/0'/0' + chg_xpub1 = 'tpubDCXqdwWZcszwqYJSnZp8eARkxGJfHAk23KDxbztV4BbschfaTfYLTcSkSJ3TN64dRqwa1rnFUScsYormKkGqNbbPwkorQimVevXjxzUV9Gf' # /84'/1'/0' + xprv2 = 'tprv8ZgxMBicQKsPdSNWUhDiwTScDr6JfkZuLshTRwzvZGnMSnGikV6jxpmdDkC3YRc4T3GD6Nvg9uv6hQg73RVv1EiTXDZwxVbsLugVHU8B1aq' + acc_xprv2 = 'tprv8gVCsmRAxVSxyUpsL13Y7ZEWBFPWbgS5E2MmFVNGuANrknvmmn2vWnmHvU8AwEFYzR2ji6EeZLSCLVacsYkvor3Pcb5JY5FGcevqTwYvdYx' + acc_xpub2 = 'tpubDDBF2BTR6s8drwrfDei8WxtckGuSm1cyoKxYY1QaKSBFbHBYQArWhHPA6eJrzZej6nfHGLSURYSLHr7GuYch8aY5n61tGqgn8b4cXrMuoPH' + chg_xpub2 = 'tpubDCYfZY2ceyHzYzMMVPt9MNeiqtQ2T7Uyp9QSFwYXh8Vi9iJFYXcuphJaGXfF3jUQJi5Y3GMNXvM11gaL4txzZgNGK22BFAwMXynnzv4z2Jh' + xprv3 = 'tprv8ZgxMBicQKsPeonDt8Ka2mrQmHa61hQ5FQCsvWBTpSNzBFgM58cV2EuXNAHF14VawVpznnme3SuTbA62sGriwWyKifJmXntfNeK7zeqMCj1' + acc_xpub3 = 'tpubDCsWoW1kuQB9kG5MXewHqkbjPtqPueRnXju7uM2NK7y3JYb2ajAZ9EiuZXNNuE4661RAfriBWhL8UsnAPpk8zrKKnZw1Ug7X4oHgMdZiU4E' + chg_xpub3 = 'tpubDC6UGqnsQStngYuGD4MKsMy7eD1Yg9NTJfPdvjdG2JE5oZ7EsSL3WHg4Gsw2pR5K39ZwJ46M1wZayhedVdQtMGaUhq5S23PH6fnENK3V1sb' + + self.test_importdesc({"desc":"wsh(multi(2," + xprv1 + "/84h/0h/0h/*," + xprv2 + "/84h/0h/0h/*," + xprv3 + "/84h/0h/0h/*))#m2sr93jn", "active": True, "range": 1000, "next_index": 0, "timestamp": "now"}, success=True, wallet=wmulti_priv) - self.test_importdesc({"desc":"wsh(multi(2,tprv8ZgxMBicQKsPevADjDCWsa6DfhkVXicu8NQUzfibwX2MexVwW4tCec5mXdCW8kJwkzBRRmAay1KZya4WsehVvjTGVW6JLqiqd8DdZ4xSg52/84h/1h/0h/*,tprv8ZgxMBicQKsPdSNWUhDiwTScDr6JfkZuLshTRwzvZGnMSnGikV6jxpmdDkC3YRc4T3GD6Nvg9uv6hQg73RVv1EiTXDZwxVbsLugVHU8B1aq/84h/1h/0h/*,tprv8ZgxMBicQKsPeonDt8Ka2mrQmHa61hQ5FQCsvWBTpSNzBFgM58cV2EuXNAHF14VawVpznnme3SuTbA62sGriwWyKifJmXntfNeK7zeqMCj1/84h/1h/0h/*))#q3sztvx5", + self.test_importdesc({"desc":"wsh(multi(2," + xprv1 + "/84h/1h/0h/*," + xprv2 + "/84h/1h/0h/*," + xprv3 + "/84h/1h/0h/*))#q3sztvx5", "active": True, "internal" : True, "range": 1000, @@ -374,14 +392,14 @@ class ImportDescriptorsTest(BitcoinTestFramework): wmulti_pub = self.nodes[1].get_wallet_rpc("wmulti_pub") assert_equal(wmulti_pub.getwalletinfo()['keypoolsize'], 0) - self.test_importdesc({"desc":"wsh(multi(2,[7b2d0242/84h/0h/0h]tpubDCJtdt5dgJpdhW4MtaVYDhG4T4tF6jcLR1PxL43q9pq1mxvXgMS9Mzw1HnXG15vxUGQJMMSqCQHMTy3F1eW5VkgVroWzchsPD5BUojrcWs8/*,[59b09cd6/84h/0h/0h]tpubDDBF2BTR6s8drwrfDei8WxtckGuSm1cyoKxYY1QaKSBFbHBYQArWhHPA6eJrzZej6nfHGLSURYSLHr7GuYch8aY5n61tGqgn8b4cXrMuoPH/*,[e81a0532/84h/0h/0h]tpubDCsWoW1kuQB9kG5MXewHqkbjPtqPueRnXju7uM2NK7y3JYb2ajAZ9EiuZXNNuE4661RAfriBWhL8UsnAPpk8zrKKnZw1Ug7X4oHgMdZiU4E/*))#tsry0s5e", + self.test_importdesc({"desc":"wsh(multi(2,[7b2d0242/84h/0h/0h]" + acc_xpub1 + "/*,[59b09cd6/84h/0h/0h]" + acc_xpub2 + "/*,[e81a0532/84h/0h/0h]" + acc_xpub3 +"/*))#tsry0s5e", "active": True, "range": 1000, "next_index": 0, "timestamp": "now"}, success=True, wallet=wmulti_pub) - self.test_importdesc({"desc":"wsh(multi(2,[7b2d0242/84h/1h/0h]tpubDCXqdwWZcszwqYJSnZp8eARkxGJfHAk23KDxbztV4BbschfaTfYLTcSkSJ3TN64dRqwa1rnFUScsYormKkGqNbbPwkorQimVevXjxzUV9Gf/*,[59b09cd6/84h/1h/0h]tpubDCYfZY2ceyHzYzMMVPt9MNeiqtQ2T7Uyp9QSFwYXh8Vi9iJFYXcuphJaGXfF3jUQJi5Y3GMNXvM11gaL4txzZgNGK22BFAwMXynnzv4z2Jh/*,[e81a0532/84h/1h/0h]tpubDC6UGqnsQStngYuGD4MKsMy7eD1Yg9NTJfPdvjdG2JE5oZ7EsSL3WHg4Gsw2pR5K39ZwJ46M1wZayhedVdQtMGaUhq5S23PH6fnENK3V1sb/*))#c08a2rzv", + self.test_importdesc({"desc":"wsh(multi(2,[7b2d0242/84h/1h/0h]" + chg_xpub1 + "/*,[59b09cd6/84h/1h/0h]" + chg_xpub2 + "/*,[e81a0532/84h/1h/0h]" + chg_xpub3 + "/*))#c08a2rzv", "active": True, "internal" : True, "range": 1000, @@ -396,8 +414,15 @@ class ImportDescriptorsTest(BitcoinTestFramework): change_addr = wmulti_pub.getrawchangeaddress('bech32') assert_equal(change_addr, 'bcrt1qt9uhe3a9hnq7vajl7a094z4s3crm9ttf8zw3f5v9gr2nyd7e3lnsy44n8e') assert_equal(wmulti_pub.getwalletinfo()['keypoolsize'], 999) + + # generate some utxos for next tests txid = w0.sendtoaddress(addr, 10) vout = find_vout_for_address(self.nodes[0], txid, addr) + + addr2 = wmulti_pub.getnewaddress('', 'bech32') + txid2 = w0.sendtoaddress(addr2, 10) + vout2 = find_vout_for_address(self.nodes[0], txid2, addr2) + self.nodes[0].generate(6) self.sync_all() assert_equal(wmulti_pub.getbalance(), wmulti_priv.getbalance()) @@ -411,14 +436,14 @@ class ImportDescriptorsTest(BitcoinTestFramework): wmulti_priv1 = self.nodes[1].get_wallet_rpc("wmulti_priv1") res = wmulti_priv1.importdescriptors([ { - "desc": descsum_create("wsh(multi(2,tprv8ZgxMBicQKsPevADjDCWsa6DfhkVXicu8NQUzfibwX2MexVwW4tCec5mXdCW8kJwkzBRRmAay1KZya4WsehVvjTGVW6JLqiqd8DdZ4xSg52/84h/0h/0h/*,[59b09cd6/84h/0h/0h]tpubDDBF2BTR6s8drwrfDei8WxtckGuSm1cyoKxYY1QaKSBFbHBYQArWhHPA6eJrzZej6nfHGLSURYSLHr7GuYch8aY5n61tGqgn8b4cXrMuoPH/*,[e81a0532/84h/0h/0h]tpubDCsWoW1kuQB9kG5MXewHqkbjPtqPueRnXju7uM2NK7y3JYb2ajAZ9EiuZXNNuE4661RAfriBWhL8UsnAPpk8zrKKnZw1Ug7X4oHgMdZiU4E/*))"), + "desc": descsum_create("wsh(multi(2," + xprv1 + "/84h/0h/0h/*,[59b09cd6/84h/0h/0h]" + acc_xpub2 + "/*,[e81a0532/84h/0h/0h]" + acc_xpub3 + "/*))"), "active": True, "range": 1000, "next_index": 0, "timestamp": "now" }, { - "desc": descsum_create("wsh(multi(2,tprv8ZgxMBicQKsPevADjDCWsa6DfhkVXicu8NQUzfibwX2MexVwW4tCec5mXdCW8kJwkzBRRmAay1KZya4WsehVvjTGVW6JLqiqd8DdZ4xSg52/84h/1h/0h/*,[59b09cd6/84h/1h/0h]tpubDCYfZY2ceyHzYzMMVPt9MNeiqtQ2T7Uyp9QSFwYXh8Vi9iJFYXcuphJaGXfF3jUQJi5Y3GMNXvM11gaL4txzZgNGK22BFAwMXynnzv4z2Jh/*,[e81a0532/84h/1h/0h]tpubDC6UGqnsQStngYuGD4MKsMy7eD1Yg9NTJfPdvjdG2JE5oZ7EsSL3WHg4Gsw2pR5K39ZwJ46M1wZayhedVdQtMGaUhq5S23PH6fnENK3V1sb/*))"), + "desc": descsum_create("wsh(multi(2," + xprv1 + "/84h/1h/0h/*,[59b09cd6/84h/1h/0h]" + chg_xpub2 + "/*,[e81a0532/84h/1h/0h]" + chg_xpub3 + "/*))"), "active": True, "internal" : True, "range": 1000, @@ -434,14 +459,14 @@ class ImportDescriptorsTest(BitcoinTestFramework): wmulti_priv2 = self.nodes[1].get_wallet_rpc('wmulti_priv2') res = wmulti_priv2.importdescriptors([ { - "desc": descsum_create("wsh(multi(2,[7b2d0242/84h/0h/0h]tpubDCJtdt5dgJpdhW4MtaVYDhG4T4tF6jcLR1PxL43q9pq1mxvXgMS9Mzw1HnXG15vxUGQJMMSqCQHMTy3F1eW5VkgVroWzchsPD5BUojrcWs8/*,tprv8ZgxMBicQKsPdSNWUhDiwTScDr6JfkZuLshTRwzvZGnMSnGikV6jxpmdDkC3YRc4T3GD6Nvg9uv6hQg73RVv1EiTXDZwxVbsLugVHU8B1aq/84h/0h/0h/*,[e81a0532/84h/0h/0h]tpubDCsWoW1kuQB9kG5MXewHqkbjPtqPueRnXju7uM2NK7y3JYb2ajAZ9EiuZXNNuE4661RAfriBWhL8UsnAPpk8zrKKnZw1Ug7X4oHgMdZiU4E/*))"), + "desc": descsum_create("wsh(multi(2,[7b2d0242/84h/0h/0h]" + acc_xpub1 + "/*," + xprv2 + "/84h/0h/0h/*,[e81a0532/84h/0h/0h]" + acc_xpub3 + "/*))"), "active": True, "range": 1000, "next_index": 0, "timestamp": "now" }, { - "desc": descsum_create("wsh(multi(2,[7b2d0242/84h/1h/0h]tpubDCXqdwWZcszwqYJSnZp8eARkxGJfHAk23KDxbztV4BbschfaTfYLTcSkSJ3TN64dRqwa1rnFUScsYormKkGqNbbPwkorQimVevXjxzUV9Gf/*,tprv8ZgxMBicQKsPdSNWUhDiwTScDr6JfkZuLshTRwzvZGnMSnGikV6jxpmdDkC3YRc4T3GD6Nvg9uv6hQg73RVv1EiTXDZwxVbsLugVHU8B1aq/84h/1h/0h/*,[e81a0532/84h/1h/0h]tpubDC6UGqnsQStngYuGD4MKsMy7eD1Yg9NTJfPdvjdG2JE5oZ7EsSL3WHg4Gsw2pR5K39ZwJ46M1wZayhedVdQtMGaUhq5S23PH6fnENK3V1sb/*))"), + "desc": descsum_create("wsh(multi(2,[7b2d0242/84h/1h/0h]" + chg_xpub1 + "/*," + xprv2 + "/84h/1h/0h/*,[e81a0532/84h/1h/0h]" + chg_xpub3 + "/*))"), "active": True, "internal" : True, "range": 1000, @@ -531,6 +556,33 @@ class ImportDescriptorsTest(BitcoinTestFramework): ) + self.log.info("Amending multisig with new private keys") + self.nodes[1].createwallet(wallet_name="wmulti_priv3", descriptors=True) + wmulti_priv3 = self.nodes[1].get_wallet_rpc("wmulti_priv3") + res = wmulti_priv3.importdescriptors([ + { + "desc": descsum_create("wsh(multi(2," + xprv1 + "/84h/0h/0h/*,[59b09cd6/84h/0h/0h]" + acc_xpub2 + "/*,[e81a0532/84h/0h/0h]" + acc_xpub3 + "/*))"), + "active": True, + "range": 1000, + "next_index": 0, + "timestamp": "now" + }]) + assert_equal(res[0]['success'], True) + res = wmulti_priv3.importdescriptors([ + { + "desc": descsum_create("wsh(multi(2," + xprv1 + "/84h/0h/0h/*,[59b09cd6/84h/0h/0h]" + acc_xprv2 + "/*,[e81a0532/84h/0h/0h]" + acc_xpub3 + "/*))"), + "active": True, + "range": 1000, + "next_index": 0, + "timestamp": "now" + }]) + assert_equal(res[0]['success'], True) + + rawtx = self.nodes[1].createrawtransaction([{'txid': txid2, 'vout': vout2}], {w0.getnewaddress(): 9.999}) + tx = wmulti_priv3.signrawtransactionwithwallet(rawtx) + assert_equal(tx['complete'], True) + self.nodes[1].sendrawtransaction(tx['hex']) + self.log.info("Combo descriptors cannot be active") self.test_importdesc({"desc": descsum_create("combo(tpubDCJtdt5dgJpdhW4MtaVYDhG4T4tF6jcLR1PxL43q9pq1mxvXgMS9Mzw1HnXG15vxUGQJMMSqCQHMTy3F1eW5VkgVroWzchsPD5BUojrcWs8/*)"), "active": True, -- cgit v1.2.3 From f1b7db14748d9ee04735b4968366d33bc89aea23 Mon Sep 17 00:00:00 2001 From: S3RK <1466284+S3RK@users.noreply.github.com> Date: Mon, 28 Jun 2021 21:37:44 +0200 Subject: wallet: don't mute exceptions in importdescriptors --- src/wallet/rpcdump.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 3ae36131cc..5f0d88288f 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -1590,10 +1590,6 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c } catch (const UniValue& e) { result.pushKV("success", UniValue(false)); result.pushKV("error", e); - } catch (...) { - result.pushKV("success", UniValue(false)); - - result.pushKV("error", JSONRPCError(RPC_MISC_ERROR, "Missing required fields")); } if (warnings.size()) result.pushKV("warnings", warnings); return result; -- cgit v1.2.3 From 586f1d53d60880ea2873d860f95e3390016620d1 Mon Sep 17 00:00:00 2001 From: S3RK <1466284+S3RK@users.noreply.github.com> Date: Mon, 28 Jun 2021 21:37:47 +0200 Subject: wallet: maintain SPK consistency on internal flag change --- src/wallet/wallet.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8750f508a7..5ae490d5cc 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3153,12 +3153,21 @@ void CWallet::AddActiveScriptPubKeyMan(uint256 id, OutputType type, bool interna void CWallet::LoadActiveScriptPubKeyMan(uint256 id, OutputType type, bool internal) { + // Activating ScriptPubKeyManager for a given output and change type is incompatible with legacy wallets. + // Legacy wallets have only one ScriptPubKeyManager and it's active for all output and change types. + Assert(IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)); + WalletLogPrintf("Setting spkMan to active: id = %s, type = %d, internal = %d\n", id.ToString(), static_cast(type), static_cast(internal)); auto& spk_mans = internal ? m_internal_spk_managers : m_external_spk_managers; + auto& spk_mans_other = internal ? m_external_spk_managers : m_internal_spk_managers; auto spk_man = m_spk_managers.at(id).get(); spk_man->SetInternal(internal); spk_mans[type] = spk_man; + if (spk_mans_other[type] == spk_man) { + spk_mans_other[type] = nullptr; + } + NotifyCanGetAddressesChanged(); } -- cgit v1.2.3 From 6737d9655bcf527afbd85d610d805a2d0fd28c4f Mon Sep 17 00:00:00 2001 From: S3RK <1466284+S3RK@users.noreply.github.com> Date: Mon, 28 Jun 2021 21:37:50 +0200 Subject: test: wallet importdescriptors update existing --- test/functional/wallet_importdescriptors.py | 66 ++++++++++++++++++++++++++--- 1 file changed, 61 insertions(+), 5 deletions(-) diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py index bf1c8fb2e4..adde375e54 100755 --- a/test/functional/wallet_importdescriptors.py +++ b/test/functional/wallet_importdescriptors.py @@ -79,7 +79,6 @@ class ImportDescriptorsTest(BitcoinTestFramework): # RPC importdescriptors ----------------------------------------------- # # Test import fails if no descriptor present - key = get_generate_key() self.log.info("Import should fail if a descriptor is not provided") self.test_importdesc({"timestamp": "now"}, success=False, @@ -103,11 +102,12 @@ class ImportDescriptorsTest(BitcoinTestFramework): self.log.info("Test can import same descriptor with public key twice") self.test_importdesc(import_request, success=True) + self.log.info("Test can update descriptor label") + self.test_importdesc({**import_request, "label": "Updated label"}, success=True) + test_address(w1, key.p2pkh_addr, solvable=True, ismine=True, labels=["Updated label"]) + self.log.info("Internal addresses cannot have labels") - self.test_importdesc({"desc": descsum_create("pkh(" + key.pubkey + ")"), - "timestamp": "now", - "internal": True, - "label": "Descriptor import test"}, + self.test_importdesc({**import_request, "internal": True}, success=False, error_code=-8, error_message="Internal addresses should not have a label") @@ -255,6 +255,39 @@ class ImportDescriptorsTest(BitcoinTestFramework): self.test_importdesc({"desc": descsum_create(desc), "timestamp": "now", "range": [0, 1000001]}, success=False, error_code=-8, error_message='Range is too large') + self.log.info("Verify we can only extend descriptor's range") + range_request = {"desc": descsum_create(desc), "timestamp": "now", "range": [5, 10], 'active': True} + self.test_importdesc(range_request, wallet=wpriv, success=True) + assert_equal(wpriv.getwalletinfo()['keypoolsize'], 6) + self.test_importdesc({**range_request, "range": [0, 10]}, wallet=wpriv, success=True) + assert_equal(wpriv.getwalletinfo()['keypoolsize'], 11) + self.test_importdesc({**range_request, "range": [0, 20]}, wallet=wpriv, success=True) + assert_equal(wpriv.getwalletinfo()['keypoolsize'], 21) + # Can keep range the same + self.test_importdesc({**range_request, "range": [0, 20]}, wallet=wpriv, success=True) + assert_equal(wpriv.getwalletinfo()['keypoolsize'], 21) + + self.test_importdesc({**range_request, "range": [5, 10]}, wallet=wpriv, success=False, + error_code=-8, error_message='new range must include current range = [0,20]') + self.test_importdesc({**range_request, "range": [0, 10]}, wallet=wpriv, success=False, + error_code=-8, error_message='new range must include current range = [0,20]') + self.test_importdesc({**range_request, "range": [5, 20]}, wallet=wpriv, success=False, + error_code=-8, error_message='new range must include current range = [0,20]') + assert_equal(wpriv.getwalletinfo()['keypoolsize'], 21) + + self.log.info("Check we can change descriptor internal flag") + self.test_importdesc({**range_request, "range": [0, 20], "internal": True}, wallet=wpriv, success=True) + assert_equal(wpriv.getwalletinfo()['keypoolsize'], 0) + assert_raises_rpc_error(-4, 'This wallet has no available keys', wpriv.getnewaddress, '', 'p2sh-segwit') + assert_equal(wpriv.getwalletinfo()['keypoolsize_hd_internal'], 21) + wpriv.getrawchangeaddress('p2sh-segwit') + + self.test_importdesc({**range_request, "range": [0, 20], "internal": False}, wallet=wpriv, success=True) + assert_equal(wpriv.getwalletinfo()['keypoolsize'], 21) + wpriv.getnewaddress('', 'p2sh-segwit') + assert_equal(wpriv.getwalletinfo()['keypoolsize_hd_internal'], 0) + assert_raises_rpc_error(-4, 'This wallet has no available keys', wpriv.getrawchangeaddress, 'p2sh-segwit') + # Make sure ranged imports import keys in order w1 = self.nodes[1].get_wallet_rpc('w1') self.log.info('Key ranges should be imported in order') @@ -306,6 +339,18 @@ class ImportDescriptorsTest(BitcoinTestFramework): w1.keypoolrefill() assert_equal(w1.getwalletinfo()['keypoolsize'], 5 * 3) + self.log.info("Check we can change next_index") + # go back and forth with next_index + for i in [4, 0, 2, 1, 3]: + self.test_importdesc({'desc': descsum_create('wpkh([80002067/0h/0h]' + xpub + '/*)'), + 'active': True, + 'range': [0, 9], + 'next_index': i, + 'timestamp': 'now' + }, + success=True) + assert_equal(w1.getnewaddress('', 'bech32'), addresses[i]) + # Check active=False default self.log.info('Check imported descriptors are not active by default') self.test_importdesc({'desc': descsum_create('pkh([12345678/1h]' + xpub + '/*)'), @@ -316,6 +361,17 @@ class ImportDescriptorsTest(BitcoinTestFramework): success=True) assert_raises_rpc_error(-4, 'This wallet has no available keys', w1.getrawchangeaddress, 'legacy') + self.log.info('Check can activate inactive descriptor') + self.test_importdesc({'desc': descsum_create('pkh([12345678]' + xpub + '/*)'), + 'range': [0, 5], + 'active': True, + 'timestamp': 'now', + 'internal': True + }, + success=True) + address = w1.getrawchangeaddress('legacy') + assert_equal(address, "mpA2Wh9dvZT7yfELq1UnrUmAoc5qCkMetg") + # # Test importing a descriptor containing a WIF private key wif_priv = "cTe1f5rdT8A8DFgVWTjyPwACsDPJM9ff4QngFxUixCSvvbg1x6sh" address = "2MuhcG52uHPknxDgmGPsV18jSHFBnnRgjPg" -- cgit v1.2.3 From 3efaf83c75cd8dc2fa084537b8ed6715fb58c04d Mon Sep 17 00:00:00 2001 From: S3RK <1466284+S3RK@users.noreply.github.com> Date: Mon, 28 Jun 2021 21:37:53 +0200 Subject: wallet: deactivate descriptor --- src/wallet/rpcdump.cpp | 4 ++++ src/wallet/wallet.cpp | 17 +++++++++++++++++ src/wallet/wallet.h | 6 ++++++ src/wallet/walletdb.cpp | 6 ++++++ src/wallet/walletdb.h | 1 + test/functional/wallet_importdescriptors.py | 15 +++++++++++++++ 6 files changed, 49 insertions(+) diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 5f0d88288f..7de12c3a94 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -1584,6 +1584,10 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c } else { wallet.AddActiveScriptPubKeyMan(spk_manager->GetID(), *w_desc.descriptor->GetOutputType(), internal); } + } else { + if (w_desc.descriptor->GetOutputType()) { + wallet.DeactivateScriptPubKeyMan(spk_manager->GetID(), *w_desc.descriptor->GetOutputType(), internal); + } } result.pushKV("success", UniValue(true)); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 5ae490d5cc..521708e69c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3171,6 +3171,23 @@ void CWallet::LoadActiveScriptPubKeyMan(uint256 id, OutputType type, bool intern NotifyCanGetAddressesChanged(); } +void CWallet::DeactivateScriptPubKeyMan(uint256 id, OutputType type, bool internal) +{ + auto spk_man = GetScriptPubKeyMan(type, internal); + if (spk_man != nullptr && spk_man->GetID() == id) { + WalletLogPrintf("Deactivate spkMan: id = %s, type = %d, internal = %d\n", id.ToString(), static_cast(type), static_cast(internal)); + WalletBatch batch(GetDatabase()); + if (!batch.EraseActiveScriptPubKeyMan(static_cast(type), internal)) { + throw std::runtime_error(std::string(__func__) + ": erasing active ScriptPubKeyMan id failed"); + } + + auto& spk_mans = internal ? m_internal_spk_managers : m_external_spk_managers; + spk_mans[type] = nullptr; + } + + NotifyCanGetAddressesChanged(); +} + bool CWallet::IsLegacy() const { if (m_internal_spk_managers.count(OutputType::LEGACY) == 0) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index b63938c5f1..001b94047a 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -895,6 +895,12 @@ public: //! @param[in] internal Whether this ScriptPubKeyMan provides change addresses void LoadActiveScriptPubKeyMan(uint256 id, OutputType type, bool internal); + //! Remove specified ScriptPubKeyMan from set of active SPK managers. Writes the change to the wallet file. + //! @param[in] id The unique id for the ScriptPubKeyMan + //! @param[in] type The OutputType this ScriptPubKeyMan provides addresses for + //! @param[in] internal Whether this ScriptPubKeyMan provides change addresses + void DeactivateScriptPubKeyMan(uint256 id, OutputType type, bool internal); + //! Create new DescriptorScriptPubKeyMans and add them to the wallet void SetupDescriptorScriptPubKeyMans() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 24d5351945..203fca8dd6 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -209,6 +209,12 @@ bool WalletBatch::WriteActiveScriptPubKeyMan(uint8_t type, const uint256& id, bo return WriteIC(make_pair(key, type), id); } +bool WalletBatch::EraseActiveScriptPubKeyMan(uint8_t type, bool internal) +{ + const std::string key{internal ? DBKeys::ACTIVEINTERNALSPK : DBKeys::ACTIVEEXTERNALSPK}; + return EraseIC(make_pair(key, type)); +} + bool WalletBatch::WriteDescriptorKey(const uint256& desc_id, const CPubKey& pubkey, const CPrivKey& privkey) { // hash pubkey/privkey to accelerate wallet load diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index e7b2d7d780..d740aaadb3 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -253,6 +253,7 @@ public: bool EraseDestData(const std::string &address, const std::string &key); bool WriteActiveScriptPubKeyMan(uint8_t type, const uint256& id, bool internal); + bool EraseActiveScriptPubKeyMan(uint8_t type, bool internal); DBErrors LoadWallet(CWallet* pwallet); DBErrors FindWalletTx(std::vector& vTxHash, std::list& vWtx); diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py index adde375e54..262175c789 100755 --- a/test/functional/wallet_importdescriptors.py +++ b/test/functional/wallet_importdescriptors.py @@ -372,6 +372,21 @@ class ImportDescriptorsTest(BitcoinTestFramework): address = w1.getrawchangeaddress('legacy') assert_equal(address, "mpA2Wh9dvZT7yfELq1UnrUmAoc5qCkMetg") + self.log.info('Check can deactivate active descriptor') + self.test_importdesc({'desc': descsum_create('pkh([12345678]' + xpub + '/*)'), + 'range': [0, 5], + 'active': False, + 'timestamp': 'now', + 'internal': True + }, + success=True) + assert_raises_rpc_error(-4, 'This wallet has no available keys', w1.getrawchangeaddress, 'legacy') + + self.log.info('Verify activation state is persistent') + w1.unloadwallet() + self.nodes[1].loadwallet('w1') + assert_raises_rpc_error(-4, 'This wallet has no available keys', w1.getrawchangeaddress, 'legacy') + # # Test importing a descriptor containing a WIF private key wif_priv = "cTe1f5rdT8A8DFgVWTjyPwACsDPJM9ff4QngFxUixCSvvbg1x6sh" address = "2MuhcG52uHPknxDgmGPsV18jSHFBnnRgjPg" -- cgit v1.2.3