From b07b07cd8779355ba1dd16e7eb4af42e0ae1c587 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 5 Nov 2019 10:13:43 -0500 Subject: Add EnsureLegacyScriptPubKeyMan and use in rpcwallet.cpp This also fixes unused variable warnings in rpcdump.cpp --- src/wallet/rpcdump.cpp | 23 +++++++---------------- src/wallet/rpcwallet.cpp | 33 ++++++++++++++++++--------------- src/wallet/rpcwallet.h | 2 ++ 3 files changed, 27 insertions(+), 31 deletions(-) (limited to 'src') diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index da4da4d9e0..6995974f08 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -87,15 +87,6 @@ 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); @@ -134,7 +125,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 = GetLegacyScriptPubKeyMan(*wallet); + EnsureLegacyScriptPubKeyMan(*wallet); WalletRescanReserver reserver(pwallet); bool fRescan = true; @@ -262,7 +253,7 @@ UniValue importaddress(const JSONRPCRequest& request) }, }.Check(request); - LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*pwallet); + EnsureLegacyScriptPubKeyMan(*pwallet); std::string strLabel; if (!request.params[1].isNull()) @@ -465,7 +456,7 @@ UniValue importpubkey(const JSONRPCRequest& request) }, }.Check(request); - LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet); + EnsureLegacyScriptPubKeyMan(*wallet); std::string strLabel; if (!request.params[1].isNull()) @@ -549,7 +540,7 @@ UniValue importwallet(const JSONRPCRequest& request) }, }.Check(request); - LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet); + EnsureLegacyScriptPubKeyMan(*wallet); if (pwallet->chain().havePruned()) { // Exit early and print an error. @@ -708,7 +699,7 @@ UniValue dumpprivkey(const JSONRPCRequest& request) }, }.Check(request); - LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet); + LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*wallet); auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); @@ -759,7 +750,7 @@ UniValue dumpwallet(const JSONRPCRequest& request) }, }.Check(request); - LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet); + LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*wallet); auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); @@ -1346,7 +1337,7 @@ UniValue importmulti(const JSONRPCRequest& mainRequest) RPCTypeCheck(mainRequest.params, {UniValue::VARR, UniValue::VOBJ}); - LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet); + EnsureLegacyScriptPubKeyMan(*wallet); const UniValue& requests = mainRequest.params[0]; diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 7f998ab450..7b39f3aecb 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -124,6 +124,15 @@ void EnsureWalletIsUnlocked(const CWallet* pwallet) } } +LegacyScriptPubKeyMan& EnsureLegacyScriptPubKeyMan(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; +} + static void WalletTxToJSON(interfaces::Chain& chain, interfaces::Chain::Lock& locked_chain, const CWalletTx& wtx, UniValue& entry) { int confirms = wtx.GetDepthInMainChain(locked_chain); @@ -966,10 +975,7 @@ static UniValue addmultisigaddress(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 = EnsureLegacyScriptPubKeyMan(*pwallet); auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); @@ -987,7 +993,7 @@ static UniValue addmultisigaddress(const JSONRPCRequest& request) if (IsHex(keys_or_addrs[i].get_str()) && (keys_or_addrs[i].get_str().length() == 66 || keys_or_addrs[i].get_str().length() == 130)) { pubkeys.push_back(HexToPubKey(keys_or_addrs[i].get_str())); } else { - pubkeys.push_back(AddrToPubKey(spk_man, keys_or_addrs[i].get_str())); + pubkeys.push_back(AddrToPubKey(&spk_man, keys_or_addrs[i].get_str())); } } @@ -1000,7 +1006,7 @@ static UniValue addmultisigaddress(const JSONRPCRequest& request) // Construct using pay-to-script-hash: CScript inner; - CTxDestination dest = AddAndGetMultisigDestination(required, pubkeys, output_type, *spk_man, inner); + CTxDestination dest = AddAndGetMultisigDestination(required, pubkeys, output_type, spk_man, inner); pwallet->SetAddressBook(dest, label, "send"); UniValue result(UniValue::VOBJ); @@ -3933,10 +3939,7 @@ UniValue sethdseed(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 = EnsureLegacyScriptPubKeyMan(*pwallet); if (pwallet->chain().isInitialBlockDownload()) { throw JSONRPCError(RPC_CLIENT_IN_INITIAL_DOWNLOAD, "Cannot set a new HD seed while still in Initial Block Download"); @@ -3963,22 +3966,22 @@ UniValue sethdseed(const JSONRPCRequest& request) CPubKey master_pub_key; if (request.params[1].isNull()) { - master_pub_key = spk_man->GenerateNewSeed(); + master_pub_key = spk_man.GenerateNewSeed(); } else { CKey key = DecodeSecret(request.params[1].get_str()); if (!key.IsValid()) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key"); } - if (HaveKey(*spk_man, key)) { + if (HaveKey(spk_man, key)) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Already have this key (either as an HD seed or as a loose private key)"); } - master_pub_key = spk_man->DeriveNewSeed(key); + master_pub_key = spk_man.DeriveNewSeed(key); } - spk_man->SetHDSeed(master_pub_key); - if (flush_key_pool) spk_man->NewKeyPool(); + spk_man.SetHDSeed(master_pub_key); + if (flush_key_pool) spk_man.NewKeyPool(); return NullUniValue; } diff --git a/src/wallet/rpcwallet.h b/src/wallet/rpcwallet.h index 31d3f7a5f9..becca455f6 100644 --- a/src/wallet/rpcwallet.h +++ b/src/wallet/rpcwallet.h @@ -12,6 +12,7 @@ class CRPCTable; class CWallet; class JSONRPCRequest; +class LegacyScriptPubKeyMan; class UniValue; struct PartiallySignedTransaction; class CTransaction; @@ -40,6 +41,7 @@ std::shared_ptr GetWalletForJSONRPCRequest(const JSONRPCRequest& reques std::string HelpRequiringPassphrase(const CWallet*); void EnsureWalletIsUnlocked(const CWallet*); bool EnsureWalletIsAvailable(const CWallet*, bool avoidException); +LegacyScriptPubKeyMan& EnsureLegacyScriptPubKeyMan(CWallet& wallet); UniValue getaddressinfo(const JSONRPCRequest& request); UniValue signrawtransactionwithwallet(const JSONRPCRequest& request); -- cgit v1.2.3 From 4a0abf694ee10cf186f25a67ca35c3fce0c10874 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 5 Nov 2019 10:36:55 -0500 Subject: Pass CTxDestination to ScriptPubKeyMan::GetMetadata Pass CTxDestination instead of more ambiguous uint160 hash value. This is more type safe and more efficient since it avoids doing map lookups that will always fail and were not done previously before a18edd7b383d667b15b6d4b87aa3a055a9fa5051 from https://github.com/bitcoin/bitcoin/pull/17304 Change suggested by Andrew Chow in https://github.com/bitcoin/bitcoin/pull/17304#discussion_r340345745 and https://github.com/bitcoin/bitcoin/pull/17381#issuecomment-549994944 --- src/wallet/rpcwallet.cpp | 10 +--------- src/wallet/scriptpubkeyman.cpp | 22 ++++++++++++++-------- src/wallet/scriptpubkeyman.h | 5 +++-- 3 files changed, 18 insertions(+), 19 deletions(-) (limited to 'src') diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 7b39f3aecb..6098a08292 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3765,15 +3765,7 @@ UniValue getaddressinfo(const JSONRPCRequest& request) ScriptPubKeyMan* spk_man = pwallet->GetScriptPubKeyMan(); if (spk_man) { - CKeyID key_id = GetKeyForDestination(*provider, dest); - const CKeyMetadata* meta = nullptr; - if (!key_id.IsNull()) { - meta = spk_man->GetMetadata(key_id); - } - if (!meta) { - meta = spk_man->GetMetadata(CScriptID(scriptPubKey)); - } - if (meta) { + if (const CKeyMetadata* meta = spk_man->GetMetadata(dest)) { ret.pushKV("timestamp", meta->nCreateTime); if (meta->has_key_origin) { ret.pushKV("hdkeypath", WriteHDKeypath(meta->key_origin.path)); diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index bb13db11ba..48fe1843a3 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -476,18 +476,24 @@ int64_t LegacyScriptPubKeyMan::GetTimeFirstKey() const return nTimeFirstKey; } -const CKeyMetadata* LegacyScriptPubKeyMan::GetMetadata(uint160 id) const +const CKeyMetadata* LegacyScriptPubKeyMan::GetMetadata(const CTxDestination& dest) const { AssertLockHeld(cs_wallet); - auto it = mapKeyMetadata.find(CKeyID(id)); - if (it != mapKeyMetadata.end()) { - return &it->second; - } else { - auto it2 = m_script_metadata.find(CScriptID(id)); - if (it2 != m_script_metadata.end()) { - return &it2->second; + + CKeyID key_id = GetKeyForDestination(*this, dest); + if (!key_id.IsNull()) { + auto it = mapKeyMetadata.find(key_id); + if (it != mapKeyMetadata.end()) { + return &it->second; } } + + CScript scriptPubKey = GetScriptForDestination(dest); + auto it = m_script_metadata.find(CScriptID(scriptPubKey)); + if (it != m_script_metadata.end()) { + return &it->second; + } + return nullptr; } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 0dbf98ee94..55117db5c1 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -186,7 +186,8 @@ public: virtual int64_t GetTimeFirstKey() const { return 0; } - virtual const CKeyMetadata* GetMetadata(uint160 id) const { return nullptr; } + //! Return address metadata + virtual const CKeyMetadata* GetMetadata(const CTxDestination& dest) const { return nullptr; } }; class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProvider @@ -302,7 +303,7 @@ public: int64_t GetTimeFirstKey() const override; - const CKeyMetadata* GetMetadata(uint160 id) const override; + const CKeyMetadata* GetMetadata(const CTxDestination& dest) const override; bool CanGetAddresses(bool internal = false) override; -- cgit v1.2.3 From 491a599b37f3e3a648e52aebed677ca11b0615e2 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 5 Nov 2019 10:43:36 -0500 Subject: Get rid of confusing LegacyScriptPubKeyMan::TopUpKeyPool method Previous discussion https://github.com/bitcoin/bitcoin/pull/17304#discussion_r340307903 --- src/wallet/scriptpubkeyman.cpp | 17 ++++++----------- src/wallet/scriptpubkeyman.h | 1 - 2 files changed, 6 insertions(+), 12 deletions(-) (limited to 'src') diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 48fe1843a3..ce7cf36924 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -14,7 +14,7 @@ bool LegacyScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error) { error.clear(); - TopUpKeyPool(); + TopUp(); // Generate a new key that is added to wallet CPubKey new_key; @@ -282,11 +282,6 @@ void LegacyScriptPubKeyMan::ReturnDestination(int64_t index, bool internal, cons ReturnKey(index, internal, pubkey); } -bool LegacyScriptPubKeyMan::TopUp(unsigned int size) -{ - return TopUpKeyPool(size); -} - void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script) { AssertLockHeld(cs_wallet); @@ -297,7 +292,7 @@ void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script) WalletLogPrintf("%s: Detected a used keypool key, mark all keypool key up to this key as used\n", __func__); MarkReserveKeysAsUsed(mi->second); - if (!TopUpKeyPool()) { + if (!TopUp()) { WalletLogPrintf("%s: Topping up keypool failed (locked wallet)\n", __func__); } } @@ -401,7 +396,7 @@ bool LegacyScriptPubKeyMan::Upgrade(int prev_version, std::string& error) } // Regenerate the keypool if upgraded to HD if (hd_upgrade) { - if (!TopUpKeyPool()) { + if (!TopUp()) { error = _("Unable to generate keys").translated; return false; } @@ -1029,7 +1024,7 @@ bool LegacyScriptPubKeyMan::NewKeyPool() m_pool_key_to_index.clear(); - if (!TopUpKeyPool()) { + if (!TopUp()) { return false; } WalletLogPrintf("LegacyScriptPubKeyMan::NewKeyPool rewrote keypool\n"); @@ -1037,7 +1032,7 @@ bool LegacyScriptPubKeyMan::NewKeyPool() return true; } -bool LegacyScriptPubKeyMan::TopUpKeyPool(unsigned int kpSize) +bool LegacyScriptPubKeyMan::TopUp(unsigned int kpSize) { if (!CanGenerateKeys()) { return false; @@ -1154,7 +1149,7 @@ bool LegacyScriptPubKeyMan::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& key { LOCK(cs_wallet); - TopUpKeyPool(); + TopUp(); bool fReturningInternal = fRequestedInternal; fReturningInternal &= (IsHDEnabled() && m_storage.CanSupportFeature(FEATURE_HD_SPLIT)) || m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS); diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 55117db5c1..4f17156792 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -356,7 +356,6 @@ public: //! Load a keypool entry void LoadKeyPool(int64_t nIndex, const CKeyPool &keypool) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - bool TopUpKeyPool(unsigned int kpSize = 0); bool NewKeyPool(); void MarkPreSplitKeys() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); -- cgit v1.2.3 From bfd826a675445801adec86a469040f3ceb8172ee Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 5 Nov 2019 10:47:07 -0500 Subject: Clean up nested scope in GetReservedDestination Suggested https://github.com/bitcoin/bitcoin/pull/17304#discussion_r341194391 by Gregory Sanders Reason for keeping the `return true` `return false` verbosity is that more code will be added after the ReserveKeyFromKeyPool() call before returning. --- src/wallet/scriptpubkeyman.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'src') diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index ce7cf36924..3eaaf3786c 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -264,10 +264,8 @@ bool LegacyScriptPubKeyMan::EncryptKeys(CKeyingMaterial& vMasterKeyIn) bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool) { - { - if (!ReserveKeyFromKeyPool(index, keypool, internal)) { - return false; - } + if (!ReserveKeyFromKeyPool(index, keypool, internal)) { + return false; } return true; } -- cgit v1.2.3 From 05b224a175065aee4d6d9c471722bc4503f01fdf Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 5 Nov 2019 10:53:07 -0500 Subject: Add missing SetupGeneration error handling in EncryptWallet Suggested https://github.com/bitcoin/bitcoin/pull/17304#discussion_r341286026 by me --- src/wallet/wallet.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 0b7dc256ad..cdcb65e3c0 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -572,7 +572,9 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) // if we are using HD, replace the HD seed with a new one if (auto spk_man = m_spk_man.get()) { if (spk_man->IsHDEnabled()) { - spk_man->SetupGeneration(true); + if (!spk_man->SetupGeneration(true)) { + return false; + } } } Lock(); -- cgit v1.2.3