From 52cf68f7ffa1dd3a6850606a75c32b77b1308c4b Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 29 Oct 2019 12:20:19 -0400 Subject: Refactor: Add GetLegacyScriptPubKeyMan helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Suggested by João Barbosa https://github.com/bitcoin/bitcoin/pull/17260#discussion_r339505236 --- src/wallet/rpcdump.cpp | 72 +++++++++++++++++++++----------------------------- 1 file changed, 30 insertions(+), 42 deletions(-) (limited to 'src') diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 0eef0502de..f7353ebbbb 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -87,6 +87,15 @@ static void RescanWallet(CWallet& wallet, const WalletRescanReserver& reserver, } } +static LegacyScriptPubKeyMan& GetLegacyScriptPubKeyMan(CWallet& wallet) +{ + LegacyScriptPubKeyMan* spk_man = wallet.GetLegacyScriptPubKeyMan(); + if (!spk_man) { + throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command"); + } + return *spk_man; +} + UniValue importprivkey(const JSONRPCRequest& request) { std::shared_ptr const wallet = GetWalletForJSONRPCRequest(request); @@ -125,10 +134,7 @@ UniValue importprivkey(const JSONRPCRequest& request) throw JSONRPCError(RPC_WALLET_ERROR, "Cannot import private keys to a wallet with private keys disabled"); } - LegacyScriptPubKeyMan* spk_man = pwallet->GetLegacyScriptPubKeyMan(); - if (!spk_man) { - throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command"); - } + LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet); WalletRescanReserver reserver(pwallet); bool fRescan = true; @@ -256,10 +262,7 @@ UniValue importaddress(const JSONRPCRequest& request) }, }.Check(request); - LegacyScriptPubKeyMan* spk_man = pwallet->GetLegacyScriptPubKeyMan(); - if (!spk_man) { - throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command"); - } + LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*pwallet); std::string strLabel; if (!request.params[1].isNull()) @@ -462,10 +465,7 @@ UniValue importpubkey(const JSONRPCRequest& request) }, }.Check(request); - LegacyScriptPubKeyMan* spk_man = pwallet->GetLegacyScriptPubKeyMan(); - if (!spk_man) { - throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command"); - } + LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet); std::string strLabel; if (!request.params[1].isNull()) @@ -549,10 +549,7 @@ UniValue importwallet(const JSONRPCRequest& request) }, }.Check(request); - LegacyScriptPubKeyMan* spk_man = pwallet->GetLegacyScriptPubKeyMan(); - if (!spk_man) { - throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command"); - } + LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet); if (pwallet->chain().havePruned()) { // Exit early and print an error. @@ -711,10 +708,7 @@ UniValue dumpprivkey(const JSONRPCRequest& request) }, }.Check(request); - LegacyScriptPubKeyMan* spk_man = pwallet->GetLegacyScriptPubKeyMan(); - if (!spk_man) { - throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command"); - } + LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet); auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); @@ -726,12 +720,12 @@ UniValue dumpprivkey(const JSONRPCRequest& request) if (!IsValidDestination(dest)) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Bitcoin address"); } - auto keyid = GetKeyForDestination(*spk_man, dest); + auto keyid = GetKeyForDestination(spk_man, dest); if (keyid.IsNull()) { throw JSONRPCError(RPC_TYPE_ERROR, "Address does not refer to a key"); } CKey vchSecret; - if (!spk_man->GetKey(keyid, vchSecret)) { + if (!spk_man.GetKey(keyid, vchSecret)) { throw JSONRPCError(RPC_WALLET_ERROR, "Private key for address " + strAddress + " is not known"); } return EncodeSecret(vchSecret); @@ -765,14 +759,11 @@ UniValue dumpwallet(const JSONRPCRequest& request) }, }.Check(request); - LegacyScriptPubKeyMan* spk_man = pwallet->GetLegacyScriptPubKeyMan(); - if (!spk_man) { - throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command"); - } + LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet); auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); - AssertLockHeld(spk_man->cs_wallet); + AssertLockHeld(spk_man.cs_wallet); EnsureWalletIsUnlocked(pwallet); @@ -794,10 +785,10 @@ UniValue dumpwallet(const JSONRPCRequest& request) throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot open wallet dump file"); std::map mapKeyBirth; - const std::map& mapKeyPool = spk_man->GetAllReserveKeys(); + const std::map& mapKeyPool = spk_man.GetAllReserveKeys(); pwallet->GetKeyBirthTimes(*locked_chain, mapKeyBirth); - std::set scripts = spk_man->GetCScripts(); + std::set scripts = spk_man.GetCScripts(); // sort time/key pairs std::vector > vKeyBirth; @@ -816,11 +807,11 @@ UniValue dumpwallet(const JSONRPCRequest& request) file << "\n"; // add the base58check encoded extended master if the wallet uses HD - CKeyID seed_id = spk_man->GetHDChain().seed_id; + CKeyID seed_id = spk_man.GetHDChain().seed_id; if (!seed_id.IsNull()) { CKey seed; - if (spk_man->GetKey(seed_id, seed)) { + if (spk_man.GetKey(seed_id, seed)) { CExtKey masterKey; masterKey.SetSeed(seed.begin(), seed.size()); @@ -833,20 +824,20 @@ UniValue dumpwallet(const JSONRPCRequest& request) std::string strAddr; std::string strLabel; CKey key; - if (spk_man->GetKey(keyid, key)) { + if (spk_man.GetKey(keyid, key)) { file << strprintf("%s %s ", EncodeSecret(key), strTime); - if (GetWalletAddressesForKey(spk_man, pwallet, keyid, strAddr, strLabel)) { + if (GetWalletAddressesForKey(&spk_man, pwallet, keyid, strAddr, strLabel)) { file << strprintf("label=%s", strLabel); } else if (keyid == seed_id) { file << "hdseed=1"; } else if (mapKeyPool.count(keyid)) { file << "reserve=1"; - } else if (spk_man->mapKeyMetadata[keyid].hdKeypath == "s") { + } else if (spk_man.mapKeyMetadata[keyid].hdKeypath == "s") { file << "inactivehdseed=1"; } else { file << "change=1"; } - file << strprintf(" # addr=%s%s\n", strAddr, (spk_man->mapKeyMetadata[keyid].has_key_origin ? " hdkeypath="+WriteHDKeypath(spk_man->mapKeyMetadata[keyid].key_origin.path) : "")); + file << strprintf(" # addr=%s%s\n", strAddr, (spk_man.mapKeyMetadata[keyid].has_key_origin ? " hdkeypath="+WriteHDKeypath(spk_man.mapKeyMetadata[keyid].key_origin.path) : "")); } } file << "\n"; @@ -855,11 +846,11 @@ UniValue dumpwallet(const JSONRPCRequest& request) std::string create_time = "0"; std::string address = EncodeDestination(ScriptHash(scriptid)); // get birth times for scripts with metadata - auto it = spk_man->m_script_metadata.find(scriptid); - if (it != spk_man->m_script_metadata.end()) { + auto it = spk_man.m_script_metadata.find(scriptid); + if (it != spk_man.m_script_metadata.end()) { create_time = FormatISO8601DateTime(it->second.nCreateTime); } - if(spk_man->GetCScript(scriptid, script)) { + if(spk_man.GetCScript(scriptid, script)) { file << strprintf("%s %s script=1", HexStr(script.begin(), script.end()), create_time); file << strprintf(" # addr=%s\n", address); } @@ -1355,10 +1346,7 @@ UniValue importmulti(const JSONRPCRequest& mainRequest) RPCTypeCheck(mainRequest.params, {UniValue::VARR, UniValue::VOBJ}); - LegacyScriptPubKeyMan* spk_man = pwallet->GetLegacyScriptPubKeyMan(); - if (!spk_man) { - throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command"); - } + LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet); const UniValue& requests = mainRequest.params[0]; -- cgit v1.2.3 From 628d11b2ba0f1ba715c2358373e603a56d9f69ff Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 29 Oct 2019 12:21:57 -0400 Subject: Add back mistakenly removed AssertLockHeld Suggestion from MarcoFalke https://github.com/bitcoin/bitcoin/pull/17260#discussion_r340029481 --- src/wallet/scriptpubkeyman.cpp | 2 ++ 1 file changed, 2 insertions(+) (limited to 'src') diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index c13fddfaf3..259bfcd76d 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -386,6 +386,8 @@ bool LegacyScriptPubKeyMan::AddKeyPubKey(const CKey& secret, const CPubKey &pubk bool LegacyScriptPubKeyMan::AddKeyPubKeyWithDB(WalletBatch& batch, const CKey& secret, const CPubKey& pubkey) { + AssertLockHeld(cs_wallet); + // Make sure we aren't adding private keys to private key disabled wallets assert(!m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)); -- cgit v1.2.3 From 2632b1f12466f970b667b42f462a6503df88e128 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 29 Oct 2019 12:23:47 -0400 Subject: doc: Clarify WalletStorage / Wallet relation Suggested by MarcoFalke https://github.com/bitcoin/bitcoin/pull/17260#discussion_r340031507 --- src/wallet/scriptpubkeyman.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 55184098b7..16a5c9b979 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -20,7 +20,7 @@ enum class OutputType; // It provides access to things that are part of the entire wallet and not specific to a ScriptPubKeyMan such as // wallet flags, wallet version, encryption keys, encryption status, and the database itself. This allows a // ScriptPubKeyMan to have callbacks into CWallet without causing a circular dependency. -// WalletStorage should be the same for all ScriptPubKeyMans. +// WalletStorage should be the same for all ScriptPubKeyMans of a wallet. class WalletStorage { public: -- cgit v1.2.3 From 4b28a05f080de8acefaaa74f1204829995611d9e Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 29 Oct 2019 12:24:34 -0400 Subject: Fix misplaced AssertLockHeld Suggestion from MarcoFalke https://github.com/bitcoin/bitcoin/pull/17260#discussion_r340033021 --- src/wallet/wallet.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 4b1adfb38f..7ffdcd1368 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -249,8 +249,8 @@ const CWalletTx* CWallet::GetWalletTx(const uint256& hash) const void CWallet::UpgradeKeyMetadata() { - AssertLockHeld(m_spk_man->cs_wallet); if (m_spk_man) { + AssertLockHeld(m_spk_man->cs_wallet); m_spk_man->UpgradeKeyMetadata(); } } -- cgit v1.2.3 From 53fe0b70adeffe4cb94e6fa18a9abbdf674a2cd0 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 29 Oct 2019 12:25:28 -0400 Subject: Fix missing strFailReason in CreateTransaction Suggested by MarcoFalke https://github.com/bitcoin/bitcoin/pull/17260#discussion_r340036269 --- src/wallet/wallet.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) (limited to 'src') diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7ffdcd1368..069ae57878 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2783,11 +2783,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std SignatureData sigdata; const SigningProvider* provider = GetSigningProvider(); - if (!provider) { - return false; - } - - if (!ProduceSignature(*provider, MutableTransactionSignatureCreator(&txNew, nIn, coin.txout.nValue, SIGHASH_ALL), scriptPubKey, sigdata)) + if (!provider || !ProduceSignature(*provider, MutableTransactionSignatureCreator(&txNew, nIn, coin.txout.nValue, SIGHASH_ALL), scriptPubKey, sigdata)) { strFailReason = _("Signing transaction failed").translated; return false; -- cgit v1.2.3