From 2bf8c1107b00ab88810b96257067efaa81841b60 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Fri, 26 Jul 2019 15:19:13 -0400 Subject: [PATCH] Merge #16301: Use CWallet::Import* functions in all import* RPCs 40ad2f6a58228c72c655e3061a19a63640419378 Have importwallet use ImportPrivKeys and ImportScripts (Andrew Chow) 78941da5baf6244c7c54e86cf8ce3e09ce60c239 Optionally allow ImportScripts to set script creation timestamp (Andrew Chow) 94bf156f391759420465b2ff8c44f5f150246c7f Have importaddress use ImportScripts and ImportScriptPubKeys (Andrew Chow) a00d1e5ec5eb019f8bbeb060a2b09e341d360fe5 Have importpubkey use CWallet's ImportScriptPubKeys and ImportPubKeys functions (Andrew Chow) c6a827424711333f6f66cf5f9d79e0e6884769de Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys (Andrew Chow) fae7a5befd0b8746d84a6fde575e5b4ea46cb3c4 Log when an import is being skipped because we already have it (Andrew Chow) ab28e31c9563bd2cd1e4a088ffd2479517dc83f2 Change ImportScriptPubKeys' internal to apply_label (Andrew Chow) Pull request description: #15741 introduced `ImportPrivKeys`, `ImportPubKeys`, `ImportScripts`, and `ImportScriptPubKeys` in `CWallet` which are used by `importmulti`. This PR changes the remaining `import*` RPCs (`importaddress`, `importprivkey`, `importpubkey`, and `importwallet`) to use these functions as well instead of directly adding the imported items to the wallet. ACKs for top commit: MarcoFalke: ACK 40ad2f6a58228c72c655e3061a19a63640419378 (checked that behavior changes are mentioned in the commit body) ryanofsky: utACK 40ad2f6a58228c72c655e3061a19a63640419378. Only change since last review is a tweaked commit message (mentioning label update in importpubkey commit) Sjors: ACK 40ad2f6a5. Those extra tests also pass. Tree-SHA512: 910e3bbe20b6f8809a47b7293775db234125615d886c7fd99c194f4cdf00c765eb1e24b1799260f1213b98c88f9bbe696796f36087c182925e567d44e9194c98 --- src/wallet/rpcdump.cpp | 105 ++++++++++----------------------- src/wallet/scriptpubkeyman.cpp | 35 +++++++++-- src/wallet/scriptpubkeyman.h | 2 +- src/wallet/wallet.cpp | 4 +- src/wallet/wallet.h | 4 +- 5 files changed, 66 insertions(+), 84 deletions(-) diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index e27fd41e52..4979355ad2 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -156,18 +156,10 @@ UniValue importprivkey(const JSONRPCRequest& request) pwallet->SetAddressBook(vchAddress, strLabel, "receive"); } - // Don't throw error in case a key is already there - if (spk_man.HaveKey(vchAddress)) { - return NullUniValue; - } - - // whenever a key is imported, we need to scan the whole chain - spk_man.UpdateTimeFirstKey(1); - spk_man.mapKeyMetadata[vchAddress].nCreateTime = 1; - if (!spk_man.AddKeyPubKeyWithDB(batch, key, pubkey)) { + // Use timestamp of 1 to scan the whole chain + if (!spk_man.ImportPrivKeys({{vchAddress, key}}, 1)) { throw JSONRPCError(RPC_WALLET_ERROR, "Error adding key to wallet"); } - } } if (fRescan) { @@ -201,48 +193,6 @@ UniValue abortrescan(const JSONRPCRequest& request) return true; } -static void ImportAddress(CWallet*, const CTxDestination& dest, const std::string& strLabel); -static void ImportScript(CWallet * const pwallet, const CScript& script, const std::string& strLabel, bool isRedeemScript) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet) -{ - WalletBatch batch(pwallet->GetDBHandle()); - - if (!isRedeemScript && pwallet->IsMine(script) == ISMINE_SPENDABLE) { - throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains the private key for this address or script"); - } - - pwallet->MarkDirty(); - - LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*pwallet); - - AssertLockHeld(spk_man.cs_wallet); - - if (!spk_man.HaveWatchOnly(script) && !spk_man.AddWatchOnlyWithDB(batch, script, 0 /* nCreateTime */)) { - throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet"); - } - - if (isRedeemScript) { - const CScriptID id(script); - if (!spk_man.HaveCScript(id) && !spk_man.AddCScriptWithDB(batch, script)) { - throw JSONRPCError(RPC_WALLET_ERROR, "Error adding p2sh redeemScript to wallet"); - } - ImportAddress(pwallet, id, strLabel); - } else { - CTxDestination destination; - if (ExtractDestination(script, destination)) { - pwallet->SetAddressBook(destination, strLabel, "receive"); - } - } -} - -static void ImportAddress(CWallet * const pwallet, const CTxDestination& dest, const std::string& strLabel) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet) -{ - CScript script = GetScriptForDestination(dest); - ImportScript(pwallet, script, strLabel, false); - // add to address book or update label - if (IsValidDestination(dest)) - pwallet->SetAddressBook(dest, strLabel, "receive"); -} - UniValue importaddress(const JSONRPCRequest& request) { RPCHelpMan{"importaddress", @@ -308,10 +258,22 @@ UniValue importaddress(const JSONRPCRequest& request) if (fP2SH) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Cannot use the p2sh flag with an address - use a script instead"); } - ImportAddress(pwallet, dest, strLabel); + + pwallet->MarkDirty(); + + pwallet->ImportScriptPubKeys(strLabel, {GetScriptForDestination(dest)}, false /* have_solving_data */, true /* apply_label */, 1 /* timestamp */); } else if (IsHex(request.params[0].get_str())) { std::vector data(ParseHex(request.params[0].get_str())); - ImportScript(pwallet, CScript(data.begin(), data.end()), strLabel, fP2SH); + CScript redeem_script(data.begin(), data.end()); + + std::set scripts = {redeem_script}; + pwallet->ImportScripts(scripts, 0 /* timestamp */); + + if (fP2SH) { + scripts.insert(GetScriptForDestination(CScriptID(redeem_script))); + } + + pwallet->ImportScriptPubKeys(strLabel, scripts, false /* have_solving_data */, true /* apply_label */, 1 /* timestamp */); } else { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Dash address or script"); } @@ -482,8 +444,14 @@ UniValue importpubkey(const JSONRPCRequest& request) { LOCK(pwallet->cs_wallet); - ImportAddress(pwallet, pubKey.GetID(), strLabel); - ImportScript(pwallet, GetScriptForRawPubKey(pubKey), strLabel, false); + std::set script_pub_keys; + script_pub_keys.insert(GetScriptForDestination(pubKey.GetID())); + + pwallet->MarkDirty(); + + pwallet->ImportScriptPubKeys(strLabel, script_pub_keys, true /* have_solving_data */, true /* apply_label */, 1 /* timestamp */); + + pwallet->ImportPubKeys({pubKey.GetID()}, {{pubKey.GetID(), pubKey}} , {} /* key_origins */, false /* add_keypool */, false /* internal */, 1 /* timestamp */); } if (fRescan) { @@ -528,7 +496,8 @@ UniValue importwallet(const JSONRPCRequest& request) throw JSONRPCError(RPC_WALLET_ERROR, "Importing wallets is disabled when blocks are pruned"); } - WalletBatch batch(pwallet->GetDBHandle()); + // KNST +// WalletBatch batch(pwallet->GetDBHandle()); WalletRescanReserver reserver(pwallet); if (!reserver.reserve()) { throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait."); @@ -618,18 +587,15 @@ UniValue importwallet(const JSONRPCRequest& request) CPubKey pubkey = key.GetPubKey(); CHECK_NONFATAL(key.VerifyPubKey(pubkey)); CKeyID keyid = pubkey.GetID(); - if (spk_man->HaveKey(keyid)) { - pwallet->WalletLogPrintf("Skipping import of %s (key already present)\n", EncodeDestination(keyid)); - continue; - } pwallet->WalletLogPrintf("Importing %s...\n", EncodeDestination(keyid)); - if (!spk_man->AddKeyPubKeyWithDB(batch, key, pubkey)) { + if (!pwallet->ImportPrivKeys({{keyid, key}}, time)) { + pwallet->WalletLogPrintf("Error importing key for %s\n", EncodeDestination(keyid)); fGood = false; continue; } - spk_man->mapKeyMetadata[keyid].nCreateTime = time; if (has_label) pwallet->SetAddressBook(keyid, label, "receive"); + nTimeBegin = std::min(nTimeBegin, time); progress++; } @@ -637,24 +603,17 @@ UniValue importwallet(const JSONRPCRequest& request) pwallet->chain().showProgress("", std::max(50, std::min(75, (int)((progress / total) * 100) + 50)), false); const CScript& script = script_pair.first; int64_t time = script_pair.second; - CScriptID id(script); - if (spk_man->HaveCScript(id)) { - pwallet->WalletLogPrintf("Skipping import of %s (script already present)\n", HexStr(script)); - continue; - } - if(!spk_man->AddCScriptWithDB(batch, script)) { + if (!pwallet->ImportScripts({script}, time)) { pwallet->WalletLogPrintf("Error importing script %s\n", HexStr(script)); fGood = false; continue; } if (time > 0) { - spk_man->m_script_metadata[id].nCreateTime = time; nTimeBegin = std::min(nTimeBegin, time); } progress++; } pwallet->chain().showProgress("", 100, false); // hide progress dialog in GUI - spk_man->UpdateTimeFirstKey(nTimeBegin); } } pwallet->chain().showProgress("", 100, false); // hide progress dialog in GUI @@ -1406,7 +1365,7 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con // All good, time to import pwallet->MarkDirty(); - if (!pwallet->ImportScripts(import_data.import_scripts)) { + if (!pwallet->ImportScripts(import_data.import_scripts, timestamp)) { throw JSONRPCError(RPC_WALLET_ERROR, "Error adding script to wallet"); } if (!pwallet->ImportPrivKeys(privkey_map, timestamp)) { @@ -1415,7 +1374,7 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con if (!pwallet->ImportPubKeys(ordered_pubkeys, pubkey_map, import_data.key_origins, add_keypool, internal, timestamp)) { throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet"); } - if (!pwallet->ImportScriptPubKeys(label, script_pub_keys, have_solving_data, internal, timestamp)) { + if (!pwallet->ImportScriptPubKeys(label, script_pub_keys, have_solving_data, !internal, timestamp)) { throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet"); } diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 39b6518887..2f4e21e45d 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1502,14 +1502,27 @@ bool LegacyScriptPubKeyMan::AddCScriptWithDB(WalletBatch& batch, const CScript& return false; } -bool LegacyScriptPubKeyMan::ImportScripts(const std::set scripts) +bool LegacyScriptPubKeyMan::ImportScripts(const std::set scripts, int64_t timestamp) { WalletBatch batch(m_storage.GetDatabase()); for (const auto& entry : scripts) { - if (!HaveCScript(CScriptID(entry)) && !AddCScriptWithDB(batch, entry)) { + CScriptID id(entry); + if (HaveCScript(id)) { + WalletLogPrintf("Already have script %s, skipping\n", HexStr(entry)); + continue; + } + if (!AddCScriptWithDB(batch, entry)) { return false; } + + if (timestamp > 0) { + m_script_metadata[CScriptID(entry)].nCreateTime = timestamp; + } } + if (timestamp > 0) { + UpdateTimeFirstKey(timestamp); + } + return true; } @@ -1522,8 +1535,13 @@ bool LegacyScriptPubKeyMan::ImportPrivKeys(const std::map& privkey const CKeyID& id = entry.first; assert(key.VerifyPubKey(pubkey)); mapKeyMetadata[id].nCreateTime = timestamp; + // Skip if we already have the key + if (HaveKey(id)) { + WalletLogPrintf("Already have key with pubkey %s, skipping\n", HexStr(pubkey)); + continue; + } // If the private key is not present in the wallet, insert it. - if (!HaveKey(id) && !AddKeyPubKeyWithDB(batch, key, pubkey)) { + if (!AddKeyPubKeyWithDB(batch, key, pubkey)) { return false; } UpdateTimeFirstKey(timestamp); @@ -1541,7 +1559,12 @@ bool LegacyScriptPubKeyMan::ImportPubKeys(const std::vector& ordered_pub } const CPubKey& pubkey = entry->second; CPubKey temp; - if (!GetPubKey(id, temp) && !AddWatchOnlyWithDB(batch, GetScriptForRawPubKey(pubkey), timestamp)) { + if (GetPubKey(id, temp)) { + // Already have pubkey, skipping + WalletLogPrintf("Already have pubkey %s, skipping\n", HexStr(temp)); + continue; + } + if (!AddWatchOnlyWithDB(batch, GetScriptForRawPubKey(pubkey), timestamp)) { return false; } mapKeyMetadata[id].nCreateTime = timestamp; @@ -1557,7 +1580,7 @@ bool LegacyScriptPubKeyMan::ImportPubKeys(const std::vector& ordered_pub return true; } -bool LegacyScriptPubKeyMan::ImportScriptPubKeys(const std::string& label, const std::set& script_pub_keys, const bool have_solving_data, const bool internal, const int64_t timestamp) +bool LegacyScriptPubKeyMan::ImportScriptPubKeys(const std::string& label, const std::set& script_pub_keys, const bool have_solving_data, const bool apply_label, const int64_t timestamp) { WalletBatch batch(m_storage.GetDatabase()); for (const CScript& script : script_pub_keys) { @@ -1568,7 +1591,7 @@ bool LegacyScriptPubKeyMan::ImportScriptPubKeys(const std::string& label, const } CTxDestination dest; ExtractDestination(script, dest); - if (!internal && IsValidDestination(dest)) { + if (apply_label && IsValidDestination(dest)) { m_wallet.SetAddressBookWithDB(batch, dest, label, "receive"); } } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 8fdf4beac2..7e3255c434 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -258,7 +258,7 @@ public: //! Fetches a pubkey from mapWatchKeys if it exists there bool GetWatchPubKey(const CKeyID &address, CPubKey &pubkey_out) const; - bool ImportScripts(const std::set scripts) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool ImportScripts(const std::set scripts, int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool ImportPrivKeys(const std::map& privkey_map, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool ImportPubKeys(const std::vector& ordered_pubkeys, const std::map& pubkey_map, const std::map>& key_origins, const bool add_keypool, const bool internal, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool ImportScriptPubKeys(const std::string& label, const std::set& script_pub_keys, const bool have_solving_data, const bool apply_label, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 91f101a304..c2601a7292 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1651,14 +1651,14 @@ bool CWallet::DummySignTx(CMutableTransaction &txNew, const std::vector return true; } -bool CWallet::ImportScripts(const std::set scripts) +bool CWallet::ImportScripts(const std::set scripts, int64_t timestamp) { auto spk_man = GetLegacyScriptPubKeyMan(); if (!spk_man) { return false; } AssertLockHeld(spk_man->cs_wallet); - return spk_man->ImportScripts(scripts); + return spk_man->ImportScripts(scripts, timestamp); } bool CWallet::ImportPrivKeys(const std::map& privkey_map, const int64_t timestamp) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index ea34e16e4b..bfb5fed353 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1035,10 +1035,10 @@ public: bool DummySignTx(CMutableTransaction &txNew, const std::vector &txouts, bool use_max_sig = false) const; bool DummySignInput(CTxIn &tx_in, const CTxOut &txout, bool use_max_sig = false) const; - bool ImportScripts(const std::set scripts) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool ImportScripts(const std::set scripts, int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool ImportPrivKeys(const std::map& privkey_map, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool ImportPubKeys(const std::vector& ordered_pubkeys, const std::map& pubkey_map, const std::map>& key_origins, const bool add_keypool, const bool internal, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - bool ImportScriptPubKeys(const std::string& label, const std::set& script_pub_keys, const bool have_solving_data, const bool internal, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool ImportScriptPubKeys(const std::string& label, const std::set& script_pub_keys, const bool have_solving_data, const bool apply_label, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); CFeeRate m_pay_tx_fee{DEFAULT_PAY_TX_FEE}; unsigned int m_confirm_target{DEFAULT_TX_CONFIRM_TARGET};