From 041620beb859e8904a039ded991452849d591a78 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Wed, 6 Nov 2019 17:14:45 +0100 Subject: [PATCH 01/12] Merge #17381: LegacyScriptPubKeyMan code cleanups 05b224a175065aee4d6d9c471722bc4503f01fdf Add missing SetupGeneration error handling in EncryptWallet (Russell Yanofsky) bfd826a675445801adec86a469040f3ceb8172ee Clean up nested scope in GetReservedDestination (Russell Yanofsky) 491a599b37f3e3a648e52aebed677ca11b0615e2 Get rid of confusing LegacyScriptPubKeyMan::TopUpKeyPool method (Russell Yanofsky) 4a0abf694ee10cf186f25a67ca35c3fce0c10874 Pass CTxDestination to ScriptPubKeyMan::GetMetadata (Russell Yanofsky) b07b07cd8779355ba1dd16e7eb4af42e0ae1c587 Add EnsureLegacyScriptPubKeyMan and use in rpcwallet.cpp (Russell Yanofsky) Pull request description: This PR implements suggested code cleanups from #17300 and #17304 review comments ACKs for top commit: Sjors: re-ACK 05b224a laanwj: Code review ACK 05b224a175065aee4d6d9c471722bc4503f01fdf Tree-SHA512: 12fd86637088515b744c028e0501c5d21a9cf9ee9c9cfd70e9cb65d44611ea5643abd5f6f101105caa5aff015d74de606f074f08af7dae8429f929d21288ab45 --- src/wallet/rpcdump.cpp | 24 +++++++----------- src/wallet/rpcwallet.cpp | 27 ++++++++++---------- src/wallet/rpcwallet.h | 4 ++- src/wallet/scriptpubkeyman.cpp | 45 +++++++++++++++++----------------- src/wallet/scriptpubkeyman.h | 6 ++--- src/wallet/wallet.cpp | 4 ++- 6 files changed, 53 insertions(+), 57 deletions(-) diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 1e0d7d4f0808..6e5209af9405 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -66,15 +66,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) { RPCHelpMan{"importprivkey", @@ -110,7 +101,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); + LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*wallet); WalletBatch batch(pwallet->GetDBHandle()); WalletRescanReserver reserver(pwallet); @@ -221,6 +212,7 @@ UniValue importaddress(const JSONRPCRequest& request) std::shared_ptr const wallet = GetWalletForJSONRPCRequest(request); if (!wallet) return NullUniValue; CWallet* const pwallet = wallet.get(); + EnsureLegacyScriptPubKeyMan(*pwallet); std::string strLabel; if (!request.params[1].isNull()) @@ -410,6 +402,7 @@ UniValue importpubkey(const JSONRPCRequest& request) std::shared_ptr const wallet = GetWalletForJSONRPCRequest(request); if (!wallet) return NullUniValue; CWallet* const pwallet = wallet.get(); + EnsureLegacyScriptPubKeyMan(*wallet); std::string strLabel; if (!request.params[1].isNull()) @@ -486,6 +479,7 @@ UniValue importwallet(const JSONRPCRequest& request) std::shared_ptr const wallet = GetWalletForJSONRPCRequest(request); if (!wallet) return NullUniValue; CWallet* const pwallet = wallet.get(); + EnsureLegacyScriptPubKeyMan(*wallet); if (pwallet->chain().havePruned()) { // Exit early and print an error. @@ -653,7 +647,7 @@ UniValue importelectrumwallet(const JSONRPCRequest& request) throw JSONRPCError(RPC_WALLET_ERROR, "Error: Private keys are disabled for this wallet"); } - LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet); + LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*wallet); LOCK(pwallet->cs_wallet); AssertLockHeld(spk_man.cs_wallet); @@ -822,7 +816,7 @@ UniValue dumpprivkey(const JSONRPCRequest& request) if (!wallet) return NullUniValue; CWallet* const pwallet = wallet.get(); - LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet); + LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*wallet); LOCK(pwallet->cs_wallet); @@ -871,7 +865,7 @@ UniValue dumphdinfo(const JSONRPCRequest& request) EnsureWalletIsUnlocked(pwallet); - LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet); + LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*wallet); CHDChain hdChainCurrent; if (!spk_man.GetHDChain(hdChainCurrent)) throw JSONRPCError(RPC_WALLET_ERROR, "This wallet is not a HD wallet."); @@ -919,7 +913,7 @@ UniValue dumpwallet(const JSONRPCRequest& request) if (!wallet) return NullUniValue; CWallet* const pwallet = wallet.get(); - LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet); + LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*wallet); LOCK(pwallet->cs_wallet); AssertLockHeld(spk_man.cs_wallet); @@ -1515,7 +1509,7 @@ UniValue importmulti(const JSONRPCRequest& mainRequest) if (!wallet) return NullUniValue; CWallet* const pwallet = wallet.get(); - GetLegacyScriptPubKeyMan(*wallet); + EnsureLegacyScriptPubKeyMan(*wallet); //Default options bool fRescan = true; diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 1c4242ac9c27..4fd16ccdc23c 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -129,6 +129,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; +} + WalletContext& EnsureWalletContext(const CoreContext& context) { auto* wallet_context = GetContext(context); @@ -995,10 +1004,7 @@ static UniValue addmultisigaddress(const JSONRPCRequest& request) if (!wallet) return NullUniValue; CWallet* const pwallet = wallet.get(); - 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); LOCK(pwallet->cs_wallet); @@ -1015,14 +1021,14 @@ 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())); } } // Construct using pay-to-script-hash: CScript inner = CreateMultisigRedeemscript(required, pubkeys); CScriptID innerID(inner); - spk_man->AddCScript(inner); + spk_man.AddCScript(inner); pwallet->SetAddressBook(innerID, label, "send"); @@ -3691,14 +3697,7 @@ UniValue getaddressinfo(const JSONRPCRequest& request) ScriptPubKeyMan* spk_man = pwallet->GetScriptPubKeyMan(); if (spk_man) { const CKeyID *key_id = std::get_if(&dest); - const CKeyMetadata* meta = nullptr; - if (key_id != nullptr && !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); CHDChain hdChainCurrent; LegacyScriptPubKeyMan* legacy_spk_man = pwallet->GetLegacyScriptPubKeyMan(); diff --git a/src/wallet/rpcwallet.h b/src/wallet/rpcwallet.h index afe43ecf7b5d..236fdf5b5677 100644 --- a/src/wallet/rpcwallet.h +++ b/src/wallet/rpcwallet.h @@ -15,6 +15,7 @@ class CRPCCommand; class CWallet; class JSONRPCRequest; +class LegacyScriptPubKeyMan; class UniValue; class CTransaction; struct PartiallySignedTransaction; @@ -32,8 +33,9 @@ Span GetWalletRPCCommands(); */ std::shared_ptr GetWalletForJSONRPCRequest(const JSONRPCRequest& request); -void EnsureWalletIsUnlocked(const CWallet*); +void EnsureWalletIsUnlocked(const CWallet *); WalletContext& EnsureWalletContext(const CoreContext& context); +LegacyScriptPubKeyMan& EnsureLegacyScriptPubKeyMan(CWallet& wallet); UniValue getaddressinfo(const JSONRPCRequest& request); UniValue signrawtransactionwithwallet(const JSONRPCRequest& request); diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 57a6dc46a17e..aafde4adcdca 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -15,7 +15,7 @@ bool LegacyScriptPubKeyMan::GetNewDestination(CTxDestination& dest, std::string& error) { error.clear(); - TopUpKeyPool(); + TopUp(); // Generate a new key that is added to wallet CPubKey new_key; @@ -265,10 +265,8 @@ bool LegacyScriptPubKeyMan::EncryptKeys(CKeyingMaterial& vMasterKeyIn) bool LegacyScriptPubKeyMan::GetReservedDestination(bool internal, int64_t& index, CKeyPool& keypool) { - { - if (!ReserveKeyFromKeyPool(index, keypool, internal)) { - return false; - } + if (!ReserveKeyFromKeyPool(index, keypool, internal)) { + return false; } return true; } @@ -283,11 +281,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(WalletBatch &batch, const CScript& script, const uint256& hashBlock) { AssertLockHeld(cs_wallet); @@ -298,7 +291,7 @@ void LegacyScriptPubKeyMan::MarkUnusedAddresses(WalletBatch &batch, const CScrip 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__); } } @@ -666,7 +659,6 @@ bool LegacyScriptPubKeyMan::CanGetAddresses(bool internal) return keypool_has_keys; } - bool LegacyScriptPubKeyMan::HavePrivateKeys() const { LOCK(cs_KeyStore); @@ -736,18 +728,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; + + const CKeyID *key_id = std::get_if(&dest); + if (key_id != nullptr && !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; } @@ -1357,15 +1355,16 @@ bool LegacyScriptPubKeyMan::NewKeyPool() m_pool_key_to_index.clear(); - if (!TopUpKeyPool()) + if (!TopUp()) { return false; + } WalletLogPrintf("LegacyScriptPubKeyMan::NewKeyPool rewrote keypool\n"); } return true; } -bool LegacyScriptPubKeyMan::TopUpKeyPool(unsigned int kpSize) +bool LegacyScriptPubKeyMan::TopUp(unsigned int kpSize) { if (!CanGenerateKeys()) { return false; @@ -1508,7 +1507,7 @@ bool LegacyScriptPubKeyMan::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& key { LOCK(cs_wallet); - TopUpKeyPool(); + TopUp(); bool fReturningInternal = fRequestedInternal; fReturningInternal &= IsHDEnabled() || m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS); diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 1e0c51cbdbaf..44dfaa4d52b2 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -173,7 +173,8 @@ class ScriptPubKeyMan 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 @@ -287,7 +288,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv int64_t GetTimeFirstKey() const override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - const CKeyMetadata* GetMetadata(uint160 id) const override; + const CKeyMetadata* GetMetadata(const CTxDestination& dest) const override; bool CanGetAddresses(bool internal = false) override; @@ -361,7 +362,6 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv bool AddKeyOrigin(const CPubKey& pubkey, const KeyOriginInfo& info); void LoadKeyPool(int64_t nIndex, const CKeyPool &keypool) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - bool TopUpKeyPool(unsigned int kpSize = 0); bool NewKeyPool(); // Seems as not used now anywhere in code // void AddKeypoolPubkey(const CPubKey& pubkey, const bool internal); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6c98ef9eacc9..7dd4b7b0177e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -672,7 +672,9 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) // if we are not using HD, generate new keypool if(m_spk_man->IsHDEnabled()) { - m_spk_man->TopUpKeyPool(); + if (!m_spk_man->TopUp()) { + return false; + } } else { m_spk_man->NewKeyPool(); From 51e643a7e3a8c9bc50a30efc9d4599f474c654ca Mon Sep 17 00:00:00 2001 From: fanquake Date: Fri, 8 Nov 2019 08:30:09 -0500 Subject: [PATCH 02/12] Merge #17354: wallet: Tidy CWallet::SetUsedDestinationState MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 0b75a7f0680d16a41043864a897470324917b1e8 wallet: Reuse existing batch in CWallet::SetUsedDestinationState (João Barbosa) 01f45dd00eb032a19d142026e4d019944192da19 wallet: Avoid recursive lock in CWallet::SetUsedDestinationState (João Barbosa) Pull request description: This PR makes 2 distinct changes around `CWallet::SetUsedDestinationState`: - 1st the recursive lock is removed and now it requires the lock to be held; - 2nd change is to support, in the best case, just a wallet database flush when transaction is added to the wallet. ACKs for top commit: achow101: ACK 0b75a7f0680d16a41043864a897470324917b1e8 MarcoFalke: ACK 0b75a7f0680d16a41043864a897470324917b1e8 ryanofsky: Code review ACK 0b75a7f0680d16a41043864a897470324917b1e8. Code changes looks fine but PR description should be updated to say what benefits of the change are. I might have missed something, but I didn't see a place where multiple batches were used previously and a single batch was used now. So the main benefit of this change appears to be removing a recursive lock? And maybe moving toward a consistent convention for passing batch instances? Tree-SHA512: abcf23a5850d29990668db20d6f624cca3e89629cc9ed003e0d05cde1b58ab2ff365034f156684ad13e55764b54c6c0c2bc7d5f96b8af7dc5e45a3be955d6b15 --- src/interfaces/wallet.cpp | 6 ++++-- src/wallet/test/wallet_tests.cpp | 7 ++++--- src/wallet/wallet.cpp | 18 +++++++++--------- src/wallet/wallet.h | 6 +++--- 4 files changed, 20 insertions(+), 17 deletions(-) diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index b48a10e6b207..660dda3f98f5 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -255,12 +255,14 @@ class WalletImpl : public Wallet bool addDestData(const CTxDestination& dest, const std::string& key, const std::string& value) override { LOCK(m_wallet->cs_wallet); - return m_wallet->AddDestData(dest, key, value); + WalletBatch batch{m_wallet->GetDatabase()}; + return m_wallet->AddDestData(batch, dest, key, value); } bool eraseDestData(const CTxDestination& dest, const std::string& key) override { LOCK(m_wallet->cs_wallet); - return m_wallet->EraseDestData(dest, key); + WalletBatch batch{m_wallet->GetDatabase()}; + return m_wallet->EraseDestData(batch, dest, key); } std::vector getDestValues(const std::string& prefix) override { diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 769e7aba1911..3d2e0c62ec88 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -401,9 +401,10 @@ BOOST_AUTO_TEST_CASE(LoadReceiveRequests) { CTxDestination dest = CKeyID(); LOCK(m_wallet.cs_wallet); - m_wallet.AddDestData(dest, "misc", "val_misc"); - m_wallet.AddDestData(dest, "rr0", "val_rr0"); - m_wallet.AddDestData(dest, "rr1", "val_rr1"); + WalletBatch batch{m_wallet.GetDatabase()}; + m_wallet.AddDestData(batch, dest, "misc", "val_misc"); + m_wallet.AddDestData(batch, dest, "rr0", "val_rr0"); + m_wallet.AddDestData(batch, dest, "rr1", "val_rr1"); auto values = m_wallet.GetDestValues("rr"); BOOST_CHECK_EQUAL(values.size(), 2U); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7dd4b7b0177e..dda2d13d826e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -778,21 +778,21 @@ void CWallet::MarkDirty() fAnonymizableTallyCachedNonDenom = false; } -void CWallet::SetSpentKeyState(const uint256& hash, unsigned int n, bool used, std::set& tx_destinations) +void CWallet::SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned int n, bool used, std::set& tx_destinations) { + AssertLockHeld(cs_wallet); const CWalletTx* srctx = GetWalletTx(hash); if (!srctx) return; CTxDestination dst; if (ExtractDestination(srctx->tx->vout[n].scriptPubKey, dst)) { if (IsMine(dst)) { - LOCK(cs_wallet); if (used && !GetDestData(dst, "used", nullptr)) { - if (AddDestData(dst, "used", "p")) { // p for "present", opposite of absent (null) + if (AddDestData(batch, dst, "used", "p")) { // p for "present", opposite of absent (null) tx_destinations.insert(dst); } } else if (!used && GetDestData(dst, "used", nullptr)) { - EraseDestData(dst, "used"); + EraseDestData(batch, dst, "used"); } } } @@ -825,7 +825,7 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose) for (const CTxIn& txin : wtxIn.tx->vin) { const COutPoint& op = txin.prevout; - SetSpentKeyState(op.hash, op.n, true, tx_destinations); + SetSpentKeyState(batch, op.hash, op.n, true, tx_destinations); } MarkDestinationsDirty(tx_destinations); @@ -4176,20 +4176,20 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx) const return nTimeSmart; } -bool CWallet::AddDestData(const CTxDestination &dest, const std::string &key, const std::string &value) +bool CWallet::AddDestData(WalletBatch& batch, const CTxDestination &dest, const std::string &key, const std::string &value) { if (std::get_if(&dest)) return false; mapAddressBook[dest].destdata.insert(std::make_pair(key, value)); - return WalletBatch(*database).WriteDestData(EncodeDestination(dest), key, value); + return batch.WriteDestData(EncodeDestination(dest), key, value); } -bool CWallet::EraseDestData(const CTxDestination &dest, const std::string &key) +bool CWallet::EraseDestData(WalletBatch& batch, const CTxDestination &dest, const std::string &key) { if (!mapAddressBook[dest].destdata.erase(key)) return false; - return WalletBatch(*database).EraseDestData(EncodeDestination(dest), key); + return batch.EraseDestData(EncodeDestination(dest), key); } void CWallet::LoadDestData(const CTxDestination &dest, const std::string &key, const std::string &value) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 8b7354f8d19e..8d9cfa0bdf2b 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -910,7 +910,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati // Whether this or any UTXO with the same CTxDestination has been spent. bool IsSpentKey(const CTxDestination& dst) const; bool IsSpentKey(const uint256& hash, unsigned int n) const; - void SetSpentKeyState(const uint256& hash, unsigned int n, bool used, std::set& tx_destinations); + void SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned int n, bool used, std::set& tx_destinations) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); std::vector GroupOutputs(const std::vector& outputs, bool single_coin) const; @@ -936,9 +936,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati bool LoadMinVersion(int nVersion) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { AssertLockHeld(cs_wallet); nWalletVersion = nVersion; nWalletMaxVersion = std::max(nWalletMaxVersion, nVersion); return true; } //! Adds a destination data tuple to the store, and saves it to disk - bool AddDestData(const CTxDestination& dest, const std::string& key, const std::string& value) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool AddDestData(WalletBatch& batch, const CTxDestination& dest, const std::string& key, const std::string& value) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Erases a destination data tuple in the store and on disk - bool EraseDestData(const CTxDestination& dest, const std::string& key) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool EraseDestData(WalletBatch& batch, const CTxDestination& dest, const std::string& key) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Adds a destination data tuple to the store, without saving it to disk void LoadDestData(const CTxDestination& dest, const std::string& key, const std::string& value) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Look up a destination data tuple in the store, return true if found false otherwise From a1d2970b31fbf1fc75ac20ce58a5bdcefd7477fc Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Sat, 23 Nov 2019 08:33:38 +1300 Subject: [PATCH 03/12] Merge #17371: Refactor: Require scriptPubKey to get wallet SigningProvider d0dab897afaac0a18aa47d3ce673a4a43a69178a Refactor: Require scriptPubKey to get wallet SigningProvider (Andrew Chow) 4b0c718f8f48c678cbe4575e9a9cf9e62a30f0da Accumulate result UniValue in SignTransaction (Andrew Chow) Pull request description: Easier to review ignoring whitespace: git log -p -n1 -w This commit does not change behavior. It passes new CScript arguments to signing functions, but the arguments aren't currently used. Split from #17261 ACKs for top commit: instagibbs: utACK https://github.com/bitcoin/bitcoin/pull/17371/commits/d0dab897afaac0a18aa47d3ce673a4a43a69178a ryanofsky: Code review ACK d0dab897afaac0a18aa47d3ce673a4a43a69178a. Thanks for the SignTransaction update. No other changes since last review Sjors: Code review ACK d0dab897afaac0a18aa47d3ce673a4a43a69178a promag: Code review ACK d0dab897afaac0a18aa47d3ce673a4a43a69178a. meshcollider: Code review ACK d0dab897afaac0a18aa47d3ce673a4a43a69178a Tree-SHA512: c3f52df20fd9d6b3b5aa65562cf5f7dce7b7f44c148b0f988f8b578fce2a28e9b7bf010f5f04bb5bf60f5272b2899f1dbbfb8aee81579c21c9cba559d1d2bb70 --- src/coinjoin/client.cpp | 4 +-- src/interfaces/wallet.cpp | 22 ++++++------ src/interfaces/wallet.h | 4 +-- src/qt/coincontroldialog.cpp | 2 +- src/qt/signverifymessagedialog.cpp | 2 +- src/wallet/psbtwallet.cpp | 25 ++++++++++++-- src/wallet/rpcwallet.cpp | 55 ++++++++++++++++++++++-------- src/wallet/wallet.cpp | 19 ++++++++--- src/wallet/wallet.h | 9 +++-- 9 files changed, 102 insertions(+), 40 deletions(-) diff --git a/src/coinjoin/client.cpp b/src/coinjoin/client.cpp index bed147b9499f..577ef82ed5b7 100644 --- a/src/coinjoin/client.cpp +++ b/src/coinjoin/client.cpp @@ -623,7 +623,7 @@ bool CCoinJoinClientSession::SignFinalTransaction(const CTransaction& finalTrans LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::%s -- Signing my input %i\n", __func__, nMyInputIndex); // TODO we're using amount=0 here but we should use the correct amount. This works because Dash ignores the amount while signing/verifying (only used in Bitcoin/Segwit) - if (!SignSignature(*mixingWallet.GetSigningProvider(), prevPubKey, finalMutableTransaction, nMyInputIndex, 0, int(SIGHASH_ALL | SIGHASH_ANYONECANPAY))) { // changes scriptSig + if (!SignSignature(*mixingWallet.GetSigningProvider(prevPubKey), prevPubKey, finalMutableTransaction, nMyInputIndex, 0, int(SIGHASH_ALL | SIGHASH_ANYONECANPAY))) { // changes scriptSig LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::%s -- Unable to sign my own transaction!\n", __func__); // not sure what to do here, it will time out...? } @@ -1544,7 +1544,7 @@ bool CCoinJoinClientSession::CreateCollateralTransaction(CMutableTransaction& tx txCollateral.vout.emplace_back(0, CScript() << OP_RETURN); } - if (!SignSignature(*mixingWallet.GetSigningProvider(), txout.scriptPubKey, txCollateral, 0, txout.nValue, SIGHASH_ALL)) { + if (!SignSignature(*mixingWallet.GetSigningProvider(txout.scriptPubKey), txout.scriptPubKey, txCollateral, 0, txout.nValue, SIGHASH_ALL)) { strReason = "Unable to sign collateral transaction!"; return false; } diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index 660dda3f98f5..8907cfae48c8 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -189,19 +189,21 @@ class WalletImpl : public Wallet std::string error; return m_wallet->GetNewDestination(label, dest, error); } - bool getPubKey(const CKeyID& address, CPubKey& pub_key) override { - auto spk_man = m_wallet->GetLegacyScriptPubKeyMan(); - if (!spk_man) { - return false; + bool getPubKey(const CScript& script, const CKeyID& address, CPubKey& pub_key) override + { + const SigningProvider* provider = m_wallet->GetSigningProvider(script); + if (provider) { + return provider->GetPubKey(address, pub_key); } - return spk_man->GetPubKey(address, pub_key); + return false; } - bool getPrivKey(const CKeyID& address, CKey& key) override { - auto spk_man = m_wallet->GetLegacyScriptPubKeyMan(); - if (!spk_man) { - return false; + bool getPrivKey(const CScript& script, const CKeyID& address, CKey& key) override + { + const SigningProvider* provider = m_wallet->GetSigningProvider(script); + if (provider) { + return provider->GetKey(address, key); } - return spk_man->GetKey(address, key); + return false; } bool isSpendable(const CScript& script) override { return m_wallet->IsMine(script) & ISMINE_SPENDABLE; } bool isSpendable(const CTxDestination& dest) override { return m_wallet->IsMine(dest) & ISMINE_SPENDABLE; } diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 768e9f9d6396..1fb5db93ee33 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -114,10 +114,10 @@ class Wallet virtual bool getNewDestination(const std::string label, CTxDestination& dest) = 0; //! Get public key. - virtual bool getPubKey(const CKeyID& address, CPubKey& pub_key) = 0; + virtual bool getPubKey(const CScript& script, const CKeyID& address, CPubKey& pub_key) = 0; //! Get private key. - virtual bool getPrivKey(const CKeyID& address, CKey& key) = 0; + virtual bool getPrivKey(const CScript& script, const CKeyID& address, CKey& key) = 0; //! Return whether wallet has private key. virtual bool isSpendable(const CScript& script) = 0; diff --git a/src/qt/coincontroldialog.cpp b/src/qt/coincontroldialog.cpp index 7395e6e3ec96..c6004d2d0572 100644 --- a/src/qt/coincontroldialog.cpp +++ b/src/qt/coincontroldialog.cpp @@ -514,7 +514,7 @@ void CoinControlDialog::updateLabels(CCoinControl& m_coin_control, WalletModel * { CPubKey pubkey; CKeyID *keyid = std::get_if(&address); - if (keyid && model->wallet().getPubKey(*keyid, pubkey)) + if (keyid && model->wallet().getPubKey(out.txout.scriptPubKey, *keyid, pubkey)) { nBytesInputs += (pubkey.IsCompressed() ? 148 : 180); } diff --git a/src/qt/signverifymessagedialog.cpp b/src/qt/signverifymessagedialog.cpp index 91c8aa1269f6..621ca31dbb8f 100644 --- a/src/qt/signverifymessagedialog.cpp +++ b/src/qt/signverifymessagedialog.cpp @@ -162,7 +162,7 @@ void SignVerifyMessageDialog::on_signMessageButton_SM_clicked() } CKey key; - if (!model->wallet().getPrivKey(*keyID, key)) + if (!model->wallet().getPrivKey(GetScriptForDestination(destination), *keyID, key)) { ui->statusLabel_SM->setStyleSheet(GUIUtil::getThemedStyleQString(GUIUtil::ThemedStyle::TS_ERROR)); ui->statusLabel_SM->setText(tr("Private key for the entered address is not available.")); diff --git a/src/wallet/psbtwallet.cpp b/src/wallet/psbtwallet.cpp index 97d34f4cb33d..f361de271e57 100644 --- a/src/wallet/psbtwallet.cpp +++ b/src/wallet/psbtwallet.cpp @@ -36,12 +36,33 @@ TransactionError FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& ps return TransactionError::SIGHASH_MISMATCH; } - complete &= SignPSBTInput(HidingSigningProvider(pwallet->GetSigningProvider(), !sign, !bip32derivs), psbtx, i, sighash_type); + // Get the scriptPubKey to know which SigningProvider to use + CScript script; + if (input.non_witness_utxo) { + script = input.non_witness_utxo->vout[txin.prevout.n].scriptPubKey; + } else { + // There's no UTXO so we can just skip this now + complete = false; + continue; + } + SignatureData sigdata; + input.FillSignatureData(sigdata); + const SigningProvider* provider = pwallet->GetSigningProvider(script, sigdata); + if (!provider) { + complete = false; + continue; + } + + complete &= SignPSBTInput(HidingSigningProvider(provider, !sign, !bip32derivs), psbtx, i, sighash_type); } // Fill in the bip32 keypaths and redeemscripts for the outputs so that hardware wallets can identify change for (unsigned int i = 0; i < psbtx.tx->vout.size(); ++i) { - UpdatePSBTOutput(HidingSigningProvider(pwallet->GetSigningProvider(), true, !bip32derivs), psbtx, i); + const CTxOut& out = psbtx.tx->vout.at(i); + const SigningProvider* provider = pwallet->GetSigningProvider(out.scriptPubKey); + if (provider) { + UpdatePSBTOutput(HidingSigningProvider(provider, true, !bip32derivs), psbtx, i); + } } return TransactionError::OK; diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 4fd16ccdc23c..418e654889df 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -598,7 +598,11 @@ static UniValue signmessage(const JSONRPCRequest& request) throw JSONRPCError(RPC_TYPE_ERROR, "Address does not refer to key"); } - const SigningProvider* provider = pwallet->GetSigningProvider(); + CScript script_pub_key = GetScriptForDestination(*keyID); + const SigningProvider* provider = pwallet->GetSigningProvider(script_pub_key); + if (!provider) { + throw JSONRPCError(RPC_WALLET_ERROR, "Private key not available"); + } CKey key; if (!provider->GetKey(*keyID, key)) { @@ -3156,12 +3160,14 @@ static UniValue listunspent(const JSONRPCRequest& request) entry.pushKV("label", i->second.name); } - const SigningProvider* provider = pwallet->GetSigningProvider(); - if (scriptPubKey.IsPayToScriptHash()) { - const CScriptID& hash = CScriptID(std::get(address)); - CScript redeemScript; - if (provider->GetCScript(hash, redeemScript)) { - entry.pushKV("redeemScript", HexStr(redeemScript)); + const SigningProvider* provider = pwallet->GetSigningProvider(scriptPubKey); + if (provider) { + if (scriptPubKey.IsPayToScriptHash()) { + const CScriptID& hash = CScriptID(std::get(address)); + CScript redeemScript; + if (provider->GetCScript(hash, redeemScript)) { + entry.pushKV("redeemScript", HexStr(redeemScript)); + } } } } @@ -3172,8 +3178,11 @@ static UniValue listunspent(const JSONRPCRequest& request) entry.pushKV("spendable", out.fSpendable); entry.pushKV("solvable", out.fSolvable); if (out.fSolvable) { - auto descriptor = InferDescriptor(scriptPubKey, *pwallet->GetLegacyScriptPubKeyMan()); - entry.pushKV("desc", descriptor->ToString()); + const SigningProvider* provider = pwallet->GetSigningProvider(scriptPubKey); + if (provider) { + auto descriptor = InferDescriptor(scriptPubKey, *provider); + entry.pushKV("desc", descriptor->ToString()); + } } if (avoid_reuse) entry.pushKV("reused", reused); entry.pushKV("safe", out.fSafe); @@ -3450,9 +3459,23 @@ UniValue signrawtransactionwithwallet(const JSONRPCRequest& request) // Parse the prevtxs array ParsePrevouts(request.params[1], nullptr, coins); + std::set providers; + for (const std::pair coin_pair : coins) { + const SigningProvider* provider = pwallet->GetSigningProvider(coin_pair.second.out.scriptPubKey); + if (provider) { + providers.insert(std::move(provider)); + } + } + if (providers.size() == 0) { + // When there are no available providers, use DUMMY_SIGNING_PROVIDER so we can check if the tx is complete + providers.insert(&DUMMY_SIGNING_PROVIDER); + } + UniValue result(UniValue::VOBJ); - SignTransaction(mtx, &*pwallet->GetLegacyScriptPubKeyMan(), coins, request.params[2], result); - return result; + for (const SigningProvider* provider : providers) { + SignTransaction(mtx, provider, coins, request.params[2], result); + } + return result; } static UniValue rescanblockchain(const JSONRPCRequest& request) @@ -3588,9 +3611,10 @@ static UniValue DescribeWalletAddress(CWallet* pwallet, const CTxDestination& de { UniValue ret(UniValue::VOBJ); UniValue detail = DescribeAddress(dest); + CScript script = GetScriptForDestination(dest); const SigningProvider* provider = nullptr; if (pwallet) { - provider = pwallet->GetSigningProvider(); + provider = pwallet->GetSigningProvider(script); } ret.pushKVs(detail); ret.pushKVs(std::visit(DescribeWalletAddressVisitor(provider), dest)); @@ -3678,11 +3702,11 @@ UniValue getaddressinfo(const JSONRPCRequest& request) CScript scriptPubKey = GetScriptForDestination(dest); ret.pushKV("scriptPubKey", HexStr(scriptPubKey)); - const SigningProvider* provider = pwallet->GetSigningProvider(); + const SigningProvider* provider = pwallet->GetSigningProvider(scriptPubKey); isminetype mine = pwallet->IsMine(dest); ret.pushKV("ismine", bool(mine & ISMINE_SPENDABLE)); - bool solvable = IsSolvable(*provider, scriptPubKey); + bool solvable = provider && IsSolvable(*provider, scriptPubKey); ret.pushKV("solvable", solvable); if (solvable) { ret.pushKV("desc", InferDescriptor(scriptPubKey, *provider)->ToString()); @@ -3694,7 +3718,8 @@ UniValue getaddressinfo(const JSONRPCRequest& request) ret.pushKV("label", pwallet->mapAddressBook[dest].name); } ret.pushKV("ischange", pwallet->IsChange(scriptPubKey)); - ScriptPubKeyMan* spk_man = pwallet->GetScriptPubKeyMan(); + + ScriptPubKeyMan* spk_man = pwallet->GetScriptPubKeyMan(scriptPubKey); if (spk_man) { const CKeyID *key_id = std::get_if(&dest); if (const CKeyMetadata* meta = spk_man->GetMetadata(dest)) { diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index dda2d13d826e..c2cefd30c655 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1630,7 +1630,11 @@ bool CWallet::DummySignInput(CTxIn &tx_in, const CTxOut &txout, bool use_max_sig const CScript& scriptPubKey = txout.scriptPubKey; SignatureData sigdata; - const SigningProvider* provider = GetSigningProvider(); + const SigningProvider* provider = GetSigningProvider(scriptPubKey); + if (!provider) { + // We don't know about this scriptpbuKey; + return false; + } if (!ProduceSignature(*provider, use_max_sig ? DUMMY_MAXIMUM_SIGNATURE_CREATOR : DUMMY_SIGNATURE_CREATOR, scriptPubKey, sigdata)) { return false; @@ -2551,7 +2555,7 @@ void CWallet::AvailableCoins(std::vector &vCoins, bool fOnlySafe, const continue; } - const SigningProvider* provider = GetSigningProvider(); + const SigningProvider* provider = GetSigningProvider(pcoin->tx->vout[i].scriptPubKey); bool solvable = provider ? IsSolvable(*provider, pcoin->tx->vout[i].scriptPubKey) : false; bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable)); @@ -3493,7 +3497,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CTransac const CScript& scriptPubKey = coin.txout.scriptPubKey; SignatureData sigdata; - const SigningProvider* provider = GetSigningProvider(); + const SigningProvider* provider = GetSigningProvider(scriptPubKey); if (!provider || !ProduceSignature(*provider, MutableTransactionSignatureCreator(&txNew, nIn, coin.txout.nValue, SIGHASH_ALL), scriptPubKey, sigdata)) { error = _("Signing transaction failed"); @@ -4980,12 +4984,17 @@ bool CWallet::Unlock(const SecureString& strWalletPassphrase, bool fForMixingOnl return false; } -ScriptPubKeyMan* CWallet::GetScriptPubKeyMan() const +ScriptPubKeyMan* CWallet::GetScriptPubKeyMan(const CScript& script) const +{ + return m_spk_man.get(); +} + +const SigningProvider* CWallet::GetSigningProvider(const CScript& script) const { return m_spk_man.get(); } -const SigningProvider* CWallet::GetSigningProvider() const +const SigningProvider* CWallet::GetSigningProvider(const CScript& script, SignatureData& sigdata) const { return m_spk_man.get(); } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 8d9cfa0bdf2b..de78590fa89e 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1275,8 +1275,13 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati m_last_block_processed = block_hash; }; - ScriptPubKeyMan* GetScriptPubKeyMan() const; - const SigningProvider* GetSigningProvider() const; + //! Get the ScriptPubKeyMan for a script + ScriptPubKeyMan* GetScriptPubKeyMan(const CScript& script) const; + + //! Get the SigningProvider for a script + const SigningProvider* GetSigningProvider(const CScript& script) const; + const SigningProvider* GetSigningProvider(const CScript& script, SignatureData& sigdata) const; + LegacyScriptPubKeyMan* GetLegacyScriptPubKeyMan() const; // Temporary LegacyScriptPubKeyMan accessors and aliases. From 9fafe3391e797b20f1fff9ce284b0f279a9b4084 Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Sat, 23 Nov 2019 09:26:13 +1300 Subject: [PATCH 04/12] Merge #17237: wallet: LearnRelatedScripts only if KeepDestination MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 3958295bc8a3787b66b0269190218a3764088f79 wallet: LearnRelatedScripts only if KeepDestination (João Barbosa) 55295fba4cbff36e9a8c3fed9c38e82ebe3c48b7 wallet: Lock address type in ReserveDestination (João Barbosa) Pull request description: Only mutates the wallet if the reserved key is kept. First commit is a refactor that makes the address type a class member. The second commit moves `LearnRelatedScripts` from `GetReservedDestination` to `KeepDestination` to avoid an unnecessary call to `AddCScript` - which in turn prevents multiple entries of the same script in the wallet DB. ACKs for top commit: achow101: Re-ACK 3958295bc8a3787b66b0269190218a3764088f79 Sjors: ACK 3958295bc8a3787b66b0269190218a3764088f79 ryanofsky: Code review ACK 3958295bc8a3787b66b0269190218a3764088f79. I like this change. The new behavior makes more sense, and the change makes the code clearer, since the current LearnRelatedScripts call is hard to understand and explain. (Personally, I'd like it if this PR were merged before #17373 or that PR was rebased on top of this one so it would be less confusing.) meshcollider: utACK 3958295bc8a3787b66b0269190218a3764088f79 Tree-SHA512: 49a5f4b022b28042ad37ea309b28378a3983cb904e234a25795b5a360356652e0f8e60f15e3e64d85094ea63af9be01812d90ccfc08ca4f1dd927fdd8566e33f --- src/wallet/wallet.h | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index de78590fa89e..77e5e0c8b086 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -150,7 +150,7 @@ class ReserveDestination : public CReserveScript { protected: //! The wallet to reserve from - CWallet* pwallet; + CWallet* const pwallet; LegacyScriptPubKeyMan* m_spk_man{nullptr}; //! The index of the address's key in the keypool @@ -164,10 +164,9 @@ class ReserveDestination : public CReserveScript public: //! Construct a ReserveDestination object. This does NOT reserve an address yet - explicit ReserveDestination(CWallet* pwalletIn) - { - pwallet = pwalletIn; - } + explicit ReserveDestination(CWallet* pwallet) + : pwallet(pwallet) + { } ReserveDestination(const ReserveDestination&) = delete; ReserveDestination& operator=(const ReserveDestination&) = delete; From 613c2877622339f567c33929bc68abe234c367c9 Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Sun, 24 Nov 2019 18:26:03 +1300 Subject: [PATCH 05/12] Merge #17518: refactor, wallet: Nuke coincontrol circular dependency 3ed5e6819a50434449d92cb96b9d8d141e8c7d2b refactor: Nuke coincontrol circular dependency (Hennadii Stepanov) Pull request description: This PR gets rid of `wallet/coincontrol` -> `wallet/wallet` -> `wallet/coincontrol` circular dependency. ACKs for top commit: Sjors: re-ACK 3ed5e6819a50434449d92cb96b9d8d141e8c7d2b meshcollider: utACK 3ed5e6819a50434449d92cb96b9d8d141e8c7d2b Tree-SHA512: 7fbceb74a9cd04157170df158d2deb979cf397df818376b478224d2423f1d8504a8688e3a9b8fc527da73e4a34ab6bc4a98be0cc2937e102a063ab2ac553e86d --- src/wallet/coincontrol.cpp | 1 - src/wallet/coincontrol.h | 3 +++ src/wallet/init.cpp | 2 ++ src/wallet/wallet.h | 2 -- test/lint/lint-circular-dependencies.sh | 1 - 5 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/wallet/coincontrol.cpp b/src/wallet/coincontrol.cpp index 7431f1fcab82..3d9a38b3ddae 100644 --- a/src/wallet/coincontrol.cpp +++ b/src/wallet/coincontrol.cpp @@ -3,7 +3,6 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include -#include #include diff --git a/src/wallet/coincontrol.h b/src/wallet/coincontrol.h index df93c6db2968..a5a79ca76d40 100644 --- a/src/wallet/coincontrol.h +++ b/src/wallet/coincontrol.h @@ -26,6 +26,9 @@ enum class CoinType MAX_COIN_TYPE = ONLY_COINJOIN_COLLATERAL, }; +//! Default for -avoidpartialspends +static constexpr bool DEFAULT_AVOIDPARTIALSPENDS = false; + /** Coin Control Features. */ class CCoinControl { diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp index 9cd9c97bdf44..612d9bc8e0a5 100644 --- a/src/wallet/init.cpp +++ b/src/wallet/init.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -17,6 +18,7 @@ #include #include #include +#include #include #include diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 77e5e0c8b086..37f10ac1aab1 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -76,8 +76,6 @@ static const CAmount WALLET_INCREMENTAL_RELAY_FEE = 5000; static const bool DEFAULT_SPEND_ZEROCONF_CHANGE = true; //! Default for -walletrejectlongchains static const bool DEFAULT_WALLET_REJECT_LONG_CHAINS = false; -//! Default for -avoidpartialspends -static const bool DEFAULT_AVOIDPARTIALSPENDS = false; //! -txconfirmtarget default static const unsigned int DEFAULT_TX_CONFIRM_TARGET = 6; static const bool DEFAULT_WALLETBROADCAST = true; diff --git a/test/lint/lint-circular-dependencies.sh b/test/lint/lint-circular-dependencies.sh index 70a20d8bb0ed..4bf98fe2ebe8 100755 --- a/test/lint/lint-circular-dependencies.sh +++ b/test/lint/lint-circular-dependencies.sh @@ -21,7 +21,6 @@ EXPECTED_CIRCULAR_DEPENDENCIES=( "txmempool -> validation -> txmempool" "wallet/fees -> wallet/wallet -> wallet/fees" "wallet/wallet -> wallet/walletdb -> wallet/wallet" - "wallet/coincontrol -> wallet/wallet -> wallet/coincontrol" "txmempool -> validation -> validationinterface -> txmempool" "wallet/scriptpubkeyman -> wallet/wallet -> wallet/scriptpubkeyman" # Dash From f798da8e92bdcf34b208056d8c515445067f75a8 Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Tue, 26 Nov 2019 21:17:36 +1300 Subject: [PATCH 06/12] Merge #17584: wallet: replace raw pointer with const reference in AddrToPubKey 1a3a256d5e0443d19757c1f1fceb9c9ede17758a wallet: replace raw pointer with const reference in AddrToPubKey (Harris) Pull request description: This PR replaces a redundant reference-to-pointer conversion in **addmultisigaddress** from *wallet/rpcwallet.cpp*. It also makes the API from *rpc/util.h* look more straightforward as **AddrToPubKey** now uses const references like other functions from there. I am not sure why there is a ref-to-ptr conversion in addmultisignatures, so I can only speculate that this is because of "historical reasons". The ref-to-ptr conversion happens here: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpcwallet.cpp#L1001 There, the address of LegacyScriptPubKeyMan& is given to AddrToPubKey. Later, in AddrToPubKey, it gets converted back to a reference, because GetKeyForDestination in rpc/util.cpp expects a const ref: https://github.com/bitcoin/bitcoin/blob/master/src/rpc/util.cpp#L140 Regards, ACKs for top commit: achow101: ACK 1a3a256d5e0443d19757c1f1fceb9c9ede17758a meshcollider: utACK 1a3a256d5e0443d19757c1f1fceb9c9ede17758a promag: Code review ACK 1a3a256d5e0443d19757c1f1fceb9c9ede17758a. hebasto: ACK 1a3a256d5e0443d19757c1f1fceb9c9ede17758a, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged. Tree-SHA512: 1a2b8ddab5694ef4c65fac69f011e38dd03a634e84a35857e13bd05ad99fe42af22ee0af6230865e3d2c725693512f3336acb055ede19c958424283e7a3856c4 --- src/rpc/util.cpp | 4 ++-- src/rpc/util.h | 2 +- src/wallet/rpcwallet.cpp | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index e5fa9c2ee32e..6688a3031695 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -189,7 +189,7 @@ CPubKey HexToPubKey(const std::string& hex_in) } // Retrieves a public key for an address from the given FillableSigningProvider -CPubKey AddrToPubKey(FillableSigningProvider* const keystore, const std::string& addr_in) +CPubKey AddrToPubKey(const FillableSigningProvider& keystore, const std::string& addr_in) { CTxDestination dest = DecodeDestination(addr_in); if (!IsValidDestination(dest)) { @@ -200,7 +200,7 @@ CPubKey AddrToPubKey(FillableSigningProvider* const keystore, const std::string& throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("%s does not refer to a key", addr_in)); } CPubKey vchPubKey; - if (!keystore->GetPubKey(*keyID, vchPubKey)) { + if (!keystore.GetPubKey(*keyID, vchPubKey)) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("no full public key for address %s", addr_in)); } if (!vchPubKey.IsFullyValid()) { diff --git a/src/rpc/util.h b/src/rpc/util.h index 3594966b434d..07af6ffb1084 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -83,7 +83,7 @@ extern std::string HelpExampleCli(const std::string& methodname, const std::stri extern std::string HelpExampleRpc(const std::string& methodname, const std::string& args); CPubKey HexToPubKey(const std::string& hex_in); -CPubKey AddrToPubKey(FillableSigningProvider* const keystore, const std::string& addr_in); +CPubKey AddrToPubKey(const FillableSigningProvider& keystore, const std::string& addr_in); CScript CreateMultisigRedeemscript(const int required, const std::vector& pubkeys); UniValue DescribeAddress(const CTxDestination& dest); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 418e654889df..b938cc54cdcf 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -1025,7 +1025,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())); } } From 4b6a09bbd30acabdf6dc409e3a927d19e5a51aea Mon Sep 17 00:00:00 2001 From: fanquake Date: Fri, 6 Dec 2019 13:37:12 -0500 Subject: [PATCH 07/12] Merge #17373: wallet: Various fixes and cleanup to keypool handling in LegacyScriptPubKeyMan and CWallet 886f1731bec4393dd342403ac34069a3a4f95eea Key pool: Fix omitted pre-split count in GetKeyPoolSize (Andrew Chow) 386a994b853bc5b3a2ed0d812673465b8ffa4849 Key pool: Change ReturnDestination interface to take address instead of key (Andrew Chow) ba41aa4969169cd73d6b4f57444ed7d8d875de10 Key pool: Move LearnRelated and GetDestination calls (Andrew Chow) 65833a74076cddf986037c6eb3b29a9b9dbe31c5 Add OutputType and CPubKey parameters to KeepDestination (Andrew Chow) 9fcf8ce7ae02bf170b9bf0c2887fd709d752cbf7 Rename Keep/ReturnKey to Keep/ReturnDestination and remove the wrapper (Andrew Chow) 596f6460f9fd8273665c8754ccd673d93a4f25f0 Key pool: Move CanGetAddresses call (Andrew Chow) Pull request description: * The `pwallet->CanGetAddresses()` call in `ReserveDestination::GetReservedDestination` to `LegacyScriptPubKeyMan::GetReservedDestination` so that the sanity check results in a failure when a `ScriptPubKeyMan` individually cannot get a destination, not when any of the `ScriptPubKeyMan`s can't. * `ScriptPubKeyMan::GetReservedDestination` is changed to return the destination so that future `ScriptPubKeyMan`s can return destinations constructed in other ways. This is implemented for `LegacyScriptPubKeyMan` by moving key-to-destination code from `CWallet` to `LegacyScriptPubKeyMan` * In order for `ScriptPubKeyMan` to be generic and work with future `ScriptPubKeyMan`s, `ScriptPubKeyMan::ReturnDestination` is changed to take a `CTxDestination` instead of a `CPubKey`. Since `LegacyScriptPubKeyMan` still deals with keys internally, a new map `m_reserved_key_to_index` is added in order to track the keypool indexes that have been reserved. * A bug is fixed in how the total keypool size is calculated as it was omitting `set_pre_split_keypool` which is a bug. Split from #17261 ACKs for top commit: ryanofsky: Code review ACK 886f1731bec4393dd342403ac34069a3a4f95eea. Only change is moving earlier fix to a better commit (same end result). promag: Code review ACK 886f1731bec4393dd342403ac34069a3a4f95eea. instagibbs: code review re-ACK https://github.com/bitcoin/bitcoin/pull/17373/commits/886f1731bec4393dd342403ac34069a3a4f95eea Sjors: Code review re-ACK 886f1731bec4393dd342403ac34069a3a4f95eea Tree-SHA512: f4be290759f63fdc920d5c02bd0d09acc4b06a5f053787d4afcd3c921b2e35d2bd97617fadae015da853dc189f559fb8d2c6e58d53e4cabfac9af151cd97ad19 --- src/wallet/scriptpubkeyman.cpp | 34 +++++++++++++++++++--------------- src/wallet/scriptpubkeyman.h | 14 +++++++------- src/wallet/wallet.cpp | 13 ++----------- src/wallet/wallet.h | 5 ++--- 4 files changed, 30 insertions(+), 36 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index aafde4adcdca..ca1bd2f2518c 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -263,24 +263,19 @@ bool LegacyScriptPubKeyMan::EncryptKeys(CKeyingMaterial& vMasterKeyIn) return true; } -bool LegacyScriptPubKeyMan::GetReservedDestination(bool internal, int64_t& index, CKeyPool& keypool) +bool LegacyScriptPubKeyMan::GetReservedDestination(bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) { + if (!CanGetAddresses(internal)) { + return false; + } + if (!ReserveKeyFromKeyPool(index, keypool, internal)) { return false; } + address = keypool.vchPubKey.GetID(); return true; } -void LegacyScriptPubKeyMan::KeepDestination(int64_t index) -{ - KeepKey(index); -} - -void LegacyScriptPubKeyMan::ReturnDestination(int64_t index, bool internal, const CPubKey& pubkey) -{ - ReturnKey(index, internal, pubkey); -} - void LegacyScriptPubKeyMan::MarkUnusedAddresses(WalletBatch &batch, const CScript& script, const uint256& hashBlock) { AssertLockHeld(cs_wallet); @@ -1447,7 +1442,7 @@ void LegacyScriptPubKeyMan::AddKeypoolPubkeyWithDB(const CPubKey& pubkey, const m_pool_key_to_index[pubkey.GetID()] = index; } -void LegacyScriptPubKeyMan::KeepKey(int64_t nIndex) +void LegacyScriptPubKeyMan::KeepDestination(int64_t nIndex) { // Remove from key pool { @@ -1457,11 +1452,16 @@ void LegacyScriptPubKeyMan::KeepKey(int64_t nIndex) --m_wallet.nKeysLeftSinceAutoBackup; if (!nWalletBackups) m_wallet.nKeysLeftSinceAutoBackup = 0; + + CPubKey pubkey; + bool have_pk = GetPubKey(m_index_to_reserved_key.at(nIndex), pubkey); + assert(have_pk); + m_index_to_reserved_key.erase(nIndex); } WalletLogPrintf("keypool keep %d\n", nIndex); } -void LegacyScriptPubKeyMan::ReturnKey(int64_t nIndex, bool fInternal, const CPubKey& pubkey) +void LegacyScriptPubKeyMan::ReturnDestination(int64_t nIndex, bool fInternal, const CTxDestination&) { // Return to key pool { @@ -1471,7 +1471,9 @@ void LegacyScriptPubKeyMan::ReturnKey(int64_t nIndex, bool fInternal, const CPub } else { setExternalKeyPool.insert(nIndex); } - m_pool_key_to_index[pubkey.GetID()] = nIndex; + CKeyID& pubkey_id = m_index_to_reserved_key.at(nIndex); + m_pool_key_to_index[pubkey_id] = nIndex; + m_index_to_reserved_key.erase(nIndex); NotifyCanGetAddressesChanged(); } WalletLogPrintf("keypool return %d\n", nIndex); @@ -1494,7 +1496,7 @@ bool LegacyScriptPubKeyMan::GetKeyFromPool(CPubKey& result, bool internal) result = GenerateNewKey(batch, 0, internal); return true; } - KeepKey(nIndex); + KeepDestination(nIndex); result = keypool.vchPubKey; } return true; @@ -1536,6 +1538,8 @@ bool LegacyScriptPubKeyMan::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& key throw std::runtime_error(std::string(__func__) + ": keypool entry invalid"); } + assert(m_index_to_reserved_key.count(nIndex) == 0); + m_index_to_reserved_key[nIndex] = keypool.vchPubKey.GetID(); m_pool_key_to_index.erase(keypool.vchPubKey.GetID()); WalletLogPrintf("keypool reserve %d\n", nIndex); } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 44dfaa4d52b2..2f2eeab3e238 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -145,9 +145,9 @@ class ScriptPubKeyMan virtual isminetype IsMine(const CScript& script) const { return ISMINE_NO; } virtual isminetype IsMine(const CTxDestination& dest) const { return ISMINE_NO; } - virtual bool GetReservedDestination(bool internal, int64_t& index, CKeyPool& keypool) { return false; } + virtual bool GetReservedDestination(bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) { return false; } virtual void KeepDestination(int64_t index) {} - virtual void ReturnDestination(int64_t index, bool internal, const CPubKey& pubkey) {} + virtual void ReturnDestination(int64_t index, bool internal, const CTxDestination& addr) {} virtual bool TopUp(unsigned int size = 0) { return false; } @@ -232,9 +232,12 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv std::set setExternalKeyPool GUARDED_BY(cs_wallet); int64_t m_max_keypool_index GUARDED_BY(cs_wallet) = 0; std::map m_pool_key_to_index; + // Tracks keypool indexes to CKeyIDs of keys that have been taken out of the keypool but may be returned to it + std::map m_index_to_reserved_key; //! Fetches a key from the keypool bool GetKeyFromPool(CPubKey &key, bool fInternal /*= false*/); + /** * Reserves a key from the keypool and sets nIndex to its index * @@ -251,9 +254,6 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv */ bool ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool fRequestedInternal); - void KeepKey(int64_t nIndex); - void ReturnKey(int64_t nIndex, bool fInternal, const CPubKey& pubkey); - public: bool GetNewDestination(CTxDestination& dest, std::string& error) override; isminetype IsMine(const CScript& script) const override; @@ -262,9 +262,9 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv //! will encrypt previously unencrypted keys bool EncryptKeys(CKeyingMaterial& vMasterKeyIn); - bool GetReservedDestination(bool internal, int64_t& index, CKeyPool& keypool) override; + bool GetReservedDestination(bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) override; void KeepDestination(int64_t index) override; - void ReturnDestination(int64_t index, bool internal, const CPubKey& pubkey) override; + void ReturnDestination(int64_t index, bool internal, const CTxDestination&) override; bool TopUp(unsigned int size = 0) override; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c2cefd30c655..05688461f0ab 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3966,21 +3966,14 @@ bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool fInte return false; } - if (!pwallet->CanGetAddresses(fInternalIn)) { - return false; - } - if (nIndex == -1) { CKeyPool keypool; - if (!m_spk_man->GetReservedDestination(fInternalIn, nIndex, keypool)) { + if (!m_spk_man->GetReservedDestination(fInternalIn, address, nIndex, keypool)) { return false; } - vchPubKey = keypool.vchPubKey; fInternal = keypool.fInternal; } - assert(vchPubKey.IsValid()); - address = vchPubKey.GetID(); dest = address; return true; } @@ -3991,17 +3984,15 @@ void ReserveDestination::KeepDestination() m_spk_man->KeepDestination(nIndex); } nIndex = -1; - vchPubKey = CPubKey(); address = CNoDestination(); } void ReserveDestination::ReturnDestination() { if (nIndex != -1) { - m_spk_man->ReturnDestination(nIndex, fInternal, vchPubKey); + m_spk_man->ReturnDestination(nIndex, fInternal, address); } nIndex = -1; - vchPubKey = CPubKey(); address = CNoDestination(); } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 37f10ac1aab1..922ad18c08c1 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -149,12 +149,11 @@ class ReserveDestination : public CReserveScript protected: //! The wallet to reserve from CWallet* const pwallet; - LegacyScriptPubKeyMan* m_spk_man{nullptr}; + //! The ScriptPubKeyMan to reserve from. Based on type when GetReservedDestination is called + ScriptPubKeyMan* m_spk_man{nullptr}; //! The index of the address's key in the keypool int64_t nIndex{-1}; - //! The public key for the address - CPubKey vchPubKey; //! The destination CTxDestination address; //! Whether this is from the internal (change output) keypool From a5a44d10e9197fdc210f288f419c27fe5e7ff310 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Sat, 17 Dec 2022 13:26:02 +0700 Subject: [PATCH 08/12] fix: add a missing condition in GetHDChain. Current implementation works until bitcoin#17369 backported because it changes logic of related IsCrypted() function not fully ompatible way --- src/wallet/scriptpubkeyman.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index ca1bd2f2518c..d46676d39de0 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1707,9 +1707,9 @@ std::set LegacyScriptPubKeyMan::GetKeys() const bool LegacyScriptPubKeyMan::GetHDChain(CHDChain& hdChainRet) const { LOCK(cs_KeyStore); - if(IsCrypted()) { + if (IsCrypted() && !cryptedHDChain.IsNull()) { hdChainRet = cryptedHDChain; - return !cryptedHDChain.IsNull(); + return true; } hdChainRet = hdChain; From f60f437a6f29e66b47151b805102d85ecb602356 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Thu, 5 Dec 2019 18:01:30 -0500 Subject: [PATCH 09/12] Merge #17369: Refactor: Move encryption code between KeyMan and Wallet 7cecf10ac32af0fca206ac5f24f482bdec88cb7d Replace LegacyScriptPubKeyMan::IsCrypted with LegacyScriptPubKeyMan::HasEncryptionKeys (Andrew Chow) bf6417142f36a2f75b3a11368bd73fe788ae1ccb Remove SetCrypted() and fUseCrypto; Change IsCrypted()'s implementation (Andrew Chow) 77a777118eaf78f10a439810d1c08d510a539aa0 Rename EncryptKeys to Encrypt and pass in the encrypted batch to use (Andrew Chow) 35f962fcf0d5107ae6a3a9348e249a9b18ff7106 Clear mapKeys before encrypting (Andrew Chow) 14b5efd66ff0afbf3bf9158a724534a9090fc7fc Move fDecryptionThoroughlyChecked from CWallet to LegacyScriptPubKeyMan (Andrew Chow) 97c0374a46943b2ed38ea24eeeff1f1568dd55b3 Move Unlock implementation to LegacyScriptPubKeyMan (Andrew Chow) e576b135d6451101d6a8219f55d80aefa216dc38 Replace LegacyScriptPubKeyMan::vMasterKey with GetDecryptionKey() (Andrew Chow) fd9d6eebc1eabb4675a118d19d38283da2dead39 Add GetEncryptionKey() and HasEncryptionKeys() to WalletStorage (Andrew Chow) Pull request description: Let wallet class handle locked/unlocked status and master key, and let keyman handle encrypting its data and determining whether there is encrypted data. There should be no change in behavior, but state is tracked differently. The fUseCrypto atomic bool is eliminated and replaced with equivalent HasEncryptionKeys checks. Split from #17261 ACKs for top commit: laanwj: ACK 7cecf10 Tree-SHA512: 95a997c366ca539abba0c0a7a0015f39d27b55220683d8d86344ff2d926db4724da67700d2c8ec2d82ed75d07404318c6cb81544af8aadeefab312167257e673 --- src/wallet/scriptpubkeyman.cpp | 121 +++++++++++++++------------------ src/wallet/scriptpubkeyman.h | 19 ++++-- src/wallet/wallet.cpp | 50 ++++++++++---- src/wallet/wallet.h | 23 ++----- 4 files changed, 111 insertions(+), 102 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index d46676d39de0..1eef41f4b22a 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -173,12 +173,11 @@ isminetype LegacyScriptPubKeyMan::IsMine(const CTxDestination& dest) const return IsMine(script); } -bool CWallet::Unlock(const CKeyingMaterial& vMasterKeyIn, bool fForMixingOnly, bool accept_no_keys) +bool LegacyScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys) { { LOCK(cs_KeyStore); - if (!SetCrypted()) - return false; + assert(mapKeys.empty()); bool keyPass = mapCryptedKeys.empty(); // Always pass when there are no encrypted keys bool keyFail = false; @@ -188,7 +187,7 @@ bool CWallet::Unlock(const CKeyingMaterial& vMasterKeyIn, bool fForMixingOnly, b const CPubKey &vchPubKey = (*mi).second.first; const std::vector &vchCryptedSecret = (*mi).second.second; CKey key; - if (!DecryptKey(vMasterKeyIn, vchCryptedSecret, vchPubKey, key)) + if (!DecryptKey(master_key, vchCryptedSecret, vchPubKey, key)) { keyFail = true; break; @@ -205,61 +204,60 @@ bool CWallet::Unlock(const CKeyingMaterial& vMasterKeyIn, bool fForMixingOnly, b if (keyFail) { return false; } - if (!keyPass && !accept_no_keys) { - if (m_spk_man == nullptr) { - return false; - } else { - AssertLockHeld(m_spk_man->cs_KeyStore); - if (m_spk_man->cryptedHDChain.IsNull()) { - return false; - } - } + if (!keyPass && !accept_no_keys && cryptedHDChain.IsNull()) { + return false; } - vMasterKey = vMasterKeyIn; - - if (m_spk_man != nullptr) { - AssertLockHeld(m_spk_man->cs_KeyStore); - if(!m_spk_man->cryptedHDChain.IsNull()) { - bool chainPass = false; - // try to decrypt seed and make sure it matches - CHDChain hdChainTmp; - if (m_spk_man->DecryptHDChain(hdChainTmp)) { - // make sure seed matches this chain - chainPass = m_spk_man->cryptedHDChain.GetID() == hdChainTmp.GetSeedHash(); - } - if (!chainPass) { - vMasterKey.clear(); - return false; - } + // This implementation is sligthly different with bitcoin's + // because function `DecryptHDChain` uses vMasterKey + m_wallet.GetEncryptionKeyMutable() = master_key; + AssertLockHeld(cs_KeyStore); + if(!cryptedHDChain.IsNull()) { + bool chainPass = false; + // try to decrypt seed and make sure it matches + CHDChain hdChainTmp; + if (DecryptHDChain(hdChainTmp)) { + // make sure seed matches this chain + chainPass = cryptedHDChain.GetID() == hdChainTmp.GetSeedHash(); + } + if (!chainPass) { + m_wallet.GetEncryptionKeyMutable().clear(); + return false; } } fDecryptionThoroughlyChecked = true; } - fOnlyMixingAllowed = fForMixingOnly; - NotifyStatusChanged(this); return true; } -bool LegacyScriptPubKeyMan::EncryptKeys(CKeyingMaterial& vMasterKeyIn) +bool LegacyScriptPubKeyMan::Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) { + AssertLockHeld(cs_wallet); LOCK(cs_KeyStore); - if (!mapCryptedKeys.empty() || IsCrypted()) + encrypted_batch = batch; + if (!mapCryptedKeys.empty()) { + encrypted_batch = nullptr; return false; + } - fUseCrypto = true; - for (const KeyMap::value_type& mKey : mapKeys) + KeyMap keys_to_encrypt; + keys_to_encrypt.swap(mapKeys); // Clear mapKeys so AddCryptedKeyInner will succeed. + for (const KeyMap::value_type& mKey : keys_to_encrypt) { const CKey &key = mKey.second; CPubKey vchPubKey = key.GetPubKey(); CKeyingMaterial vchSecret(key.begin(), key.end()); std::vector vchCryptedSecret; - if (!EncryptSecret(vMasterKeyIn, vchSecret, vchPubKey.GetHash(), vchCryptedSecret)) + if (!EncryptSecret(master_key, vchSecret, vchPubKey.GetHash(), vchCryptedSecret)) { + encrypted_batch = nullptr; return false; - if (!AddCryptedKey(vchPubKey, vchCryptedSecret)) + } + if (!AddCryptedKey(vchPubKey, vchCryptedSecret)) { + encrypted_batch = nullptr; return false; + } } - mapKeys.clear(); + encrypted_batch = nullptr; return true; } @@ -386,7 +384,7 @@ bool LegacyScriptPubKeyMan::GenerateNewHDChainEncrypted(const SecureString& secu assert(!m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)); LOCK(cs_wallet); - if (!IsCrypted()) { + if (!m_storage.HasEncryptionKeys()) { return false; } @@ -524,7 +522,7 @@ bool LegacyScriptPubKeyMan::EncryptHDChain(const CKeyingMaterial& vMasterKeyIn, { LOCK(cs_KeyStore); // should call EncryptKeys first - if (!IsCrypted()) + if (!m_storage.HasEncryptionKeys()) return false; if (!cryptedHDChain.IsNull()) @@ -582,7 +580,7 @@ bool LegacyScriptPubKeyMan::EncryptHDChain(const CKeyingMaterial& vMasterKeyIn, bool LegacyScriptPubKeyMan::DecryptHDChain(CHDChain& hdChainRet) const { LOCK(cs_KeyStore); - if (!IsCrypted()) + if (!m_storage.HasEncryptionKeys()) return true; if (cryptedHDChain.IsNull()) @@ -594,7 +592,7 @@ bool LegacyScriptPubKeyMan::DecryptHDChain(CHDChain& hdChainRet) const SecureVector vchSecureSeed; SecureVector vchSecureCryptedSeed = cryptedHDChain.GetSeed(); std::vector vchCryptedSeed(vchSecureCryptedSeed.begin(), vchSecureCryptedSeed.end()); - if (!DecryptSecret(vMasterKey, vchCryptedSeed, cryptedHDChain.GetID(), vchSecureSeed)) + if (!DecryptSecret(m_storage.GetEncryptionKey(), vchCryptedSeed, cryptedHDChain.GetID(), vchSecureSeed)) return false; hdChainRet = cryptedHDChain; @@ -616,9 +614,9 @@ bool LegacyScriptPubKeyMan::DecryptHDChain(CHDChain& hdChainRet) const std::vector vchCryptedMnemonic(vchSecureCryptedMnemonic.begin(), vchSecureCryptedMnemonic.end()); std::vector vchCryptedMnemonicPassphrase(vchSecureCryptedMnemonicPassphrase.begin(), vchSecureCryptedMnemonicPassphrase.end()); - if (!vchCryptedMnemonic.empty() && !DecryptSecret(vMasterKey, vchCryptedMnemonic, cryptedHDChain.GetID(), vchSecureMnemonic)) + if (!vchCryptedMnemonic.empty() && !DecryptSecret(m_storage.GetEncryptionKey(), vchCryptedMnemonic, cryptedHDChain.GetID(), vchSecureMnemonic)) return false; - if (!vchCryptedMnemonicPassphrase.empty() && !DecryptSecret(vMasterKey, vchCryptedMnemonicPassphrase, cryptedHDChain.GetID(), vchSecureMnemonicPassphrase)) + if (!vchCryptedMnemonicPassphrase.empty() && !DecryptSecret(m_storage.GetEncryptionKey(), vchCryptedMnemonicPassphrase, cryptedHDChain.GetID(), vchSecureMnemonicPassphrase)) return false; if (!hdChainRet.SetMnemonic(vchSecureMnemonic, vchSecureMnemonicPassphrase, false)) @@ -802,7 +800,7 @@ bool LegacyScriptPubKeyMan::AddKeyPubKeyWithDB(WalletBatch& batch, const CKey& s RemoveWatchOnly(script); } - if (!IsCrypted()) { + if (!m_storage.HasEncryptionKeys()) { return batch.WriteKey(pubkey, secret.GetPrivKey(), mapKeyMetadata[pubkey.GetID()]); @@ -843,7 +841,7 @@ void LegacyScriptPubKeyMan::LoadScriptMetadata(const CScriptID& script_id, const bool LegacyScriptPubKeyMan::AddKeyPubKeyInner(const CKey& key, const CPubKey &pubkey) { LOCK(cs_KeyStore); - if (!IsCrypted()) { + if (!m_storage.HasEncryptionKeys()) { return FillableSigningProvider::AddKeyPubKey(key, pubkey); } @@ -853,7 +851,7 @@ bool LegacyScriptPubKeyMan::AddKeyPubKeyInner(const CKey& key, const CPubKey &pu std::vector vchCryptedSecret; CKeyingMaterial vchSecret(key.begin(), key.end()); - if (!EncryptSecret(vMasterKey, vchSecret, pubkey.GetHash(), vchCryptedSecret)) { + if (!EncryptSecret(m_storage.GetEncryptionKey(), vchSecret, pubkey.GetHash(), vchCryptedSecret)) { return false; } @@ -866,7 +864,7 @@ bool LegacyScriptPubKeyMan::AddKeyPubKeyInner(const CKey& key, const CPubKey &pu bool LegacyScriptPubKeyMan::GetKeyInner(const CKeyID &address, CKey& keyOut) const { LOCK(cs_KeyStore); - if (!IsCrypted()) { + if (!m_storage.HasEncryptionKeys()) { return FillableSigningProvider::GetKey(address, keyOut); } @@ -875,7 +873,7 @@ bool LegacyScriptPubKeyMan::GetKeyInner(const CKeyID &address, CKey& keyOut) con { const CPubKey &vchPubKey = (*mi).second.first; const std::vector &vchCryptedSecret = (*mi).second.second; - return DecryptKey(vMasterKey, vchCryptedSecret, vchPubKey, keyOut); + return DecryptKey(m_storage.GetEncryptionKey(), vchCryptedSecret, vchPubKey, keyOut); } return false; } @@ -883,7 +881,7 @@ bool LegacyScriptPubKeyMan::GetKeyInner(const CKeyID &address, CKey& keyOut) con bool LegacyScriptPubKeyMan::GetPubKeyInner(const CKeyID &address, CPubKey& vchPubKeyOut) const { LOCK(cs_KeyStore); - if (!IsCrypted()) { + if (!m_storage.HasEncryptionKeys()) { if (!FillableSigningProvider::GetPubKey(address, vchPubKeyOut)) { return GetWatchPubKey(address, vchPubKeyOut); } @@ -908,7 +906,7 @@ bool LegacyScriptPubKeyMan::LoadCryptedKey(const CPubKey &vchPubKey, const std:: bool LegacyScriptPubKeyMan::HaveKeyInner(const CKeyID &address) const { LOCK(cs_KeyStore); - if (!IsCrypted()) { + if (!m_storage.HasEncryptionKeys()) { return FillableSigningProvider::HaveKey(address); } return mapCryptedKeys.count(address) > 0; @@ -917,9 +915,7 @@ bool LegacyScriptPubKeyMan::HaveKeyInner(const CKeyID &address) const bool LegacyScriptPubKeyMan::AddCryptedKeyInner(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret) { LOCK(cs_KeyStore); - if (!SetCrypted()) { - return false; - } + assert(mapKeys.empty()); mapCryptedKeys[vchPubKey.GetID()] = make_pair(vchPubKey, vchCryptedSecret); return true; @@ -1044,7 +1040,7 @@ bool LegacyScriptPubKeyMan::AddWatchOnly(const CScript& dest, int64_t nCreateTim bool LegacyScriptPubKeyMan::SetHDChain(const CHDChain& chain) { LOCK(cs_KeyStore); - if (IsCrypted()) + if (m_storage.HasEncryptionKeys()) return false; if (chain.IsCrypted()) @@ -1057,7 +1053,7 @@ bool LegacyScriptPubKeyMan::SetHDChain(const CHDChain& chain) bool LegacyScriptPubKeyMan::SetCryptedHDChain(const CHDChain& chain) { LOCK(cs_KeyStore); - if (!SetCrypted()) + if (!m_storage.HasEncryptionKeys()) return false; if (!chain.IsCrypted()) @@ -1281,7 +1277,7 @@ void LegacyScriptPubKeyMan::DeriveNewChildKey(WalletBatch &batch, CKeyMetadata& if (!hdChainCurrent.SetAccount(nAccountIndex, acc)) throw std::runtime_error(std::string(__func__) + ": SetAccount failed"); - if (IsCrypted()) { + if (m_storage.HasEncryptionKeys()) { if (!SetCryptedHDChain(batch, hdChainCurrent, false)) throw std::runtime_error(std::string(__func__) + ": SetCryptedHDChain failed"); } @@ -1693,7 +1689,7 @@ bool LegacyScriptPubKeyMan::ImportScriptPubKeys(const std::set& script_ std::set LegacyScriptPubKeyMan::GetKeys() const { LOCK(cs_KeyStore); - if (!IsCrypted()) { + if (!m_storage.HasEncryptionKeys()) { return FillableSigningProvider::GetKeys(); } std::set set_address; @@ -1707,7 +1703,7 @@ std::set LegacyScriptPubKeyMan::GetKeys() const bool LegacyScriptPubKeyMan::GetHDChain(CHDChain& hdChainRet) const { LOCK(cs_KeyStore); - if (IsCrypted() && !cryptedHDChain.IsNull()) { + if (m_storage.HasEncryptionKeys() && !cryptedHDChain.IsNull()) { hdChainRet = cryptedHDChain; return true; } @@ -1720,13 +1716,8 @@ bool LegacyScriptPubKeyMan::GetHDChain(CHDChain& hdChainRet) const LegacyScriptPubKeyMan::LegacyScriptPubKeyMan(CWallet& wallet) : ScriptPubKeyMan(wallet), m_wallet(wallet), - cs_wallet(wallet.cs_wallet), - vMasterKey(wallet.vMasterKey), - fUseCrypto(wallet.fUseCrypto), - fDecryptionThoroughlyChecked(wallet.fDecryptionThoroughlyChecked) {} + cs_wallet(wallet.cs_wallet) {} -bool LegacyScriptPubKeyMan::SetCrypted() { return m_wallet.SetCrypted(); } -bool LegacyScriptPubKeyMan::IsCrypted() const { return m_wallet.IsCrypted(); } void LegacyScriptPubKeyMan::NotifyWatchonlyChanged(bool fHaveWatchOnly) const { return m_wallet.NotifyWatchonlyChanged(fHaveWatchOnly); } void LegacyScriptPubKeyMan::NotifyCanGetAddressesChanged() const { return m_wallet.NotifyCanGetAddressesChanged(); } template void LegacyScriptPubKeyMan::WalletLogPrintf(const std::string& fmt, const Params&... parameters) const { return m_wallet.WalletLogPrintf(fmt, parameters...); } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 2f2eeab3e238..54eb2241fbce 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -29,6 +29,9 @@ class WalletStorage virtual void UnsetBlankWalletFlag(WalletBatch&) = 0; virtual bool CanSupportFeature(enum WalletFeature) const = 0; virtual void SetMinVersion(enum WalletFeature, WalletBatch* = nullptr, bool = false) = 0; + virtual const CKeyingMaterial& GetEncryptionKey() const = 0; + virtual CKeyingMaterial& GetEncryptionKeyMutable() = 0; + virtual bool HasEncryptionKeys() const = 0; virtual bool IsLocked(bool fForMixing = false) const = 0; }; @@ -145,6 +148,10 @@ class ScriptPubKeyMan virtual isminetype IsMine(const CScript& script) const { return ISMINE_NO; } virtual isminetype IsMine(const CTxDestination& dest) const { return ISMINE_NO; } + //! Check that the given decryption key is valid for this ScriptPubKeyMan, i.e. it decrypts all of the keys handled by it. + virtual bool CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys = false) { return false; } + virtual bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) { return false; } + virtual bool GetReservedDestination(bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) { return false; } virtual void KeepDestination(int64_t index) {} virtual void ReturnDestination(int64_t index, bool internal, const CTxDestination& addr) {} @@ -180,6 +187,9 @@ class ScriptPubKeyMan class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProvider { private: + //! keeps track of whether Unlock has run a thorough check before + bool fDecryptionThoroughlyChecked = false; + using WatchOnlySet = std::set; using WatchKeyMap = std::map; using HDPubKeyMap = std::map; @@ -259,8 +269,8 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv isminetype IsMine(const CScript& script) const override; isminetype IsMine(const CTxDestination& dest) const override; - //! will encrypt previously unencrypted keys - bool EncryptKeys(CKeyingMaterial& vMasterKeyIn); + bool CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys = false) override; + bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) override; bool GetReservedDestination(bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) override; void KeepDestination(int64_t index) override; @@ -421,16 +431,11 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv friend class CWallet; friend class ReserveDestination; LegacyScriptPubKeyMan(CWallet& wallet); - bool SetCrypted(); - bool IsCrypted() const; void NotifyWatchonlyChanged(bool fHaveWatchOnly) const; void NotifyCanGetAddressesChanged() const; template void WalletLogPrintf(const std::string& fmt, const Params&... parameters) const; CWallet& m_wallet; CCriticalSection& cs_wallet; - CKeyingMaterial& vMasterKey GUARDED_BY(cs_KeyStore); - std::atomic& fUseCrypto; - bool& fDecryptionThoroughlyChecked; }; #endif // BITCOIN_WALLET_SCRIPTPUBKEYMAN_H diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 05688461f0ab..d7a2648d45d4 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -610,8 +610,7 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) { LOCK(cs_wallet); mapMasterKeys[++nMasterKeyMaxID] = kMasterKey; - assert(!encrypted_batch); - encrypted_batch = new WalletBatch(*database); + WalletBatch* encrypted_batch = new WalletBatch(*database); if (!encrypted_batch->TxnBegin()) { delete encrypted_batch; encrypted_batch = nullptr; @@ -624,8 +623,7 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) if (auto spk_man = m_spk_man.get()) { spk_man->GetHDChain(hdChainCurrent); - if (!spk_man->EncryptKeys(_vMasterKey)) - { + if (!spk_man->Encrypt(_vMasterKey, encrypted_batch)) { encrypted_batch->TxnAbort(); delete encrypted_batch; encrypted_batch = nullptr; @@ -4889,15 +4887,9 @@ std::vector CWallet::GroupOutputs(const std::vector& outpu return groups; } -bool CWallet::SetCrypted() +bool CWallet::IsCrypted() const { - LOCK(cs_KeyStore); - if (fUseCrypto) - return true; - if (!mapKeys.empty()) - return false; - fUseCrypto = true; - return true; + return HasEncryptionKeys(); } // This function should be used in a different combinations to determine @@ -4931,7 +4923,7 @@ bool CWallet::IsLocked(bool fForMixing) const bool CWallet::Lock(bool fAllowMixing) { - if (!SetCrypted()) + if (!IsCrypted()) return false; if(!fAllowMixing) { @@ -4975,6 +4967,23 @@ bool CWallet::Unlock(const SecureString& strWalletPassphrase, bool fForMixingOnl return false; } +bool CWallet::Unlock(const CKeyingMaterial& vMasterKeyIn, bool fForMixingOnly, bool accept_no_keys) +{ + { + LOCK(cs_KeyStore); + if (m_spk_man) { + if (!m_spk_man->CheckDecryptionKey(vMasterKeyIn, accept_no_keys)) { + return false; + } + } else { + vMasterKey = vMasterKeyIn; + } + fOnlyMixingAllowed = fForMixingOnly; + } + NotifyStatusChanged(this); + return true; +} + ScriptPubKeyMan* CWallet::GetScriptPubKeyMan(const CScript& script) const { return m_spk_man.get(); @@ -4994,3 +5003,18 @@ LegacyScriptPubKeyMan* CWallet::GetLegacyScriptPubKeyMan() const { return m_spk_man.get(); } + +const CKeyingMaterial& CWallet::GetEncryptionKey() const +{ + return vMasterKey; +} + +CKeyingMaterial& CWallet::GetEncryptionKeyMutable() +{ + return vMasterKey; +} + +bool CWallet::HasEncryptionKeys() const +{ + return !mapMasterKeys.empty(); +} diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 922ad18c08c1..48dbae10b248 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -650,18 +650,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati private: CKeyingMaterial vMasterKey GUARDED_BY(cs_KeyStore); - //! if fUseCrypto is true, mapKeys must be empty - //! if fUseCrypto is false, vMasterKey must be empty - std::atomic fUseCrypto; - - //! keeps track of whether Unlock has run a thorough check before - bool fDecryptionThoroughlyChecked; - //! if fOnlyMixingAllowed is true, only mixing should be allowed in unlocked wallet bool fOnlyMixingAllowed; - bool SetCrypted(); - bool Unlock(const CKeyingMaterial& vMasterKeyIn, bool fForMixingOnly = false, bool accept_no_keys = false); std::atomic fAbortRescan{false}; @@ -813,9 +804,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati /** Construct wallet with specified name and database implementation. */ CWallet(interfaces::Chain* chain, const std::string& name, std::unique_ptr database) - : fUseCrypto(false), - fDecryptionThoroughlyChecked(false), - fOnlyMixingAllowed(false), + : fOnlyMixingAllowed(false), m_chain(chain), m_name(name), database(std::move(database)) @@ -826,13 +815,11 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati { // Should not have slots connected at this point. assert(NotifyUnload.empty()); - delete encrypted_batch; - encrypted_batch = nullptr; } /** Interface to assert chain access */ bool HaveChain() const { return m_chain ? true : false; } - bool IsCrypted() const { return fUseCrypto; } + bool IsCrypted() const; bool IsLocked(bool fForMixing = false) const override; bool Lock(bool fForMixing = false); @@ -1280,6 +1267,10 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati LegacyScriptPubKeyMan* GetLegacyScriptPubKeyMan() const; + const CKeyingMaterial& GetEncryptionKey() const override; + CKeyingMaterial& GetEncryptionKeyMutable() override; + bool HasEncryptionKeys() const override; + // Temporary LegacyScriptPubKeyMan accessors and aliases. friend class LegacyScriptPubKeyMan; std::unique_ptr m_spk_man = std::make_unique(*this); @@ -1290,8 +1281,6 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati LegacyScriptPubKeyMan::WatchOnlySet& setWatchOnly GUARDED_BY(cs_KeyStore) = m_spk_man->setWatchOnly; LegacyScriptPubKeyMan::WatchKeyMap& mapWatchKeys GUARDED_BY(cs_KeyStore) = m_spk_man->mapWatchKeys; LegacyScriptPubKeyMan::HDPubKeyMap& mapHdPubKeys GUARDED_BY(cs_KeyStore) = m_spk_man->mapHdPubKeys; - WalletBatch*& encrypted_batch GUARDED_BY(cs_wallet) = m_spk_man->encrypted_batch; - using CryptedKeyMap = LegacyScriptPubKeyMan::CryptedKeyMap; }; /** From ecb6c89aff8803c3f0cfdb3769c704ce23788675 Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 17 Dec 2019 11:50:25 -0500 Subject: [PATCH 10/12] Merge #17537: wallet: Cleanup and move opportunistic and superfluous TopUp()s 6e77a7b65cda1b46ce42f0c99ca91562255aeb28 keypool: Add comment about TopUp and when to use it (Andrew Chow) ea50e34b287e0da0806c1116bb55ade730e8ff6c keypool: Move opportunistic TopUps from LegacyScriptPubKeyMan to CWallet and ReserveDestination (Andrew Chow) bb2c8ce23c9d7ba8d0e5538243e07218443c85b4 keypool: Remove superfluous topup from CWallet::GetNewChangeDestination (Andrew Chow) Pull request description: * The `TopUp()` in `CWallet::GetNewChangeDestination` is unnecessary as currently m_spk_man calls TopUp further down the call stack inside LegacyScriptPubKeyMan::ReserveKeyFromKeyPool (called by LegacyScriptPubKeyMan::GetReservedDestination). This also lets us prepare for future changes with multiple ScriptPubKeyMans in the wallet. * An opportunistic `TopUp()` is moved from `LegacyScriptPubKeyMan::GetNewDestination` to `CWallet::GetNewDestination`. * Another opportunistic `TopUp()` is moved from `LegacyScriptPubKeyMan::ReserveKeyFromKeyPool` Moving opportunistic TopUps ensures that ScriptPubKeyMans will always be topped up before requesting Destinations from them as we cannot always rely on future ScriptPubKeyMan implementaions topping up internally. See also: https://github.com/bitcoin/bitcoin/pull/17373#discussion_r348598174 ACKs for top commit: instagibbs: utACK https://github.com/bitcoin/bitcoin/pull/17537/commits/6e77a7b65cda1b46ce42f0c99ca91562255aeb28 only change is slight elaboration on comment ryanofsky: Code review ACK 6e77a7b65cda1b46ce42f0c99ca91562255aeb28. Only the comment changed since my previous review. Tree-SHA512: bdfc8d303842c3fb7c3d40af7abfa6d9dac4ef71a24922bb92229674ee89bfe3113ebb46d3903ac48ef99f0a7d6eaac33282495844f2b31f91b8df55084c421f --- src/wallet/scriptpubkeyman.cpp | 3 --- src/wallet/scriptpubkeyman.h | 4 ++++ src/wallet/wallet.cpp | 5 +++-- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 1eef41f4b22a..2a8c99f38a08 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -15,7 +15,6 @@ bool LegacyScriptPubKeyMan::GetNewDestination(CTxDestination& dest, std::string& error) { error.clear(); - TopUp(); // Generate a new key that is added to wallet CPubKey new_key; @@ -1505,8 +1504,6 @@ bool LegacyScriptPubKeyMan::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& key { LOCK(cs_wallet); - TopUp(); - bool fReturningInternal = fRequestedInternal; fReturningInternal &= IsHDEnabled() || m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS); std::set& setKeyPool = fReturningInternal ? setInternalKeyPool : setExternalKeyPool; diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 54eb2241fbce..20e196cf6179 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -156,6 +156,10 @@ class ScriptPubKeyMan virtual void KeepDestination(int64_t index) {} virtual void ReturnDestination(int64_t index, bool internal, const CTxDestination& addr) {} + /** Fills internal address pool. Use within ScriptPubKeyMan implementations should be used sparingly and only + * when something from the address pool is removed, excluding GetNewDestination and GetReservedDestination. + * External wallet code is primarily responsible for topping up prior to fetching new addresses + */ virtual bool TopUp(unsigned int size = 0) { return false; } //! Mark unused addresses as being used diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index d7a2648d45d4..c6fe22b2917a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3771,6 +3771,7 @@ bool CWallet::GetNewDestination(const std::string label, CTxDestination& dest, s bool result = false; auto spk_man = m_spk_man.get(); if (spk_man) { + spk_man->TopUp(); result = spk_man->GetNewDestination(dest, error); } if (result) { @@ -3784,8 +3785,6 @@ bool CWallet::GetNewChangeDestination(CTxDestination& dest, std::string& error) { error.clear(); - m_spk_man->TopUp(); - ReserveDestination reservedest(this); if (!reservedest.GetReservedDestination(dest, true)) { error = "Error: Keypool ran out, please call keypoolrefill first"; @@ -3966,6 +3965,8 @@ bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool fInte if (nIndex == -1) { + m_spk_man->TopUp(); + CKeyPool keypool; if (!m_spk_man->GetReservedDestination(fInternalIn, address, nIndex, keypool)) { return false; From 78e25573af37e472fe0b0cb42c7b32d13b7b7ebb Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Wed, 8 Jan 2020 10:31:23 +1300 Subject: [PATCH 11/12] Merge #17621: IsUsedDestination should count any known single-key address 09502452bbbe21bb974f1de8cf53196373921ab9 IsUsedDestination should count any known single-key address (Gregory Sanders) Pull request description: This plugs the privacy leak detailed at https://github.com/bitcoin/bitcoin/issues/17605, at least for the single-key case. ACKs for top commit: meshcollider: Code Review ACK 09502452bbbe21bb974f1de8cf53196373921ab9 Tree-SHA512: e1d68281675f05072b3087171cba1df9416a69c9ccf70c72e8555e55eadda2d0fd339e5a894e3a3438ff94b9e3827fb19b8b701faade70c08756b19ff157ee0c --- src/wallet/rpcwallet.cpp | 2 +- src/wallet/wallet.cpp | 22 ++++++++++++++-------- src/wallet/wallet.h | 5 ++--- test/functional/wallet_avoidreuse.py | 2 +- 4 files changed, 18 insertions(+), 13 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index b938cc54cdcf..cde01feeda0a 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3143,7 +3143,7 @@ static UniValue listunspent(const JSONRPCRequest& request) CTxDestination address; const CScript& scriptPubKey = out.tx->tx->vout[out.i].scriptPubKey; bool fValidAddress = ExtractDestination(scriptPubKey, address); - bool reused = avoid_reuse && pwallet->IsSpentKey(address); + bool reused = avoid_reuse && pwallet->IsSpentKey(out.tx->GetHash(), out.i); if (destinations.size() && (!fValidAddress || !destinations.count(address))) continue; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c6fe22b2917a..e95434837f94 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -796,17 +796,23 @@ void CWallet::SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned } } -bool CWallet::IsSpentKey(const CTxDestination& dst) const -{ - LOCK(cs_wallet); - return IsMine(dst) && GetDestData(dst, "used", nullptr); -} - bool CWallet::IsSpentKey(const uint256& hash, unsigned int n) const { - CTxDestination dst; + AssertLockHeld(cs_wallet); const CWalletTx* srctx = GetWalletTx(hash); - return srctx && ExtractDestination(srctx->tx->vout[n].scriptPubKey, dst) && IsSpentKey(dst); + if (srctx) { + assert(srctx->tx->vout.size() > n); + LegacyScriptPubKeyMan* spk_man = GetLegacyScriptPubKeyMan(); + // When descriptor wallets arrive, these additional checks are + // likely superfluous and can be optimized out + assert(spk_man != nullptr); + for (const auto& keyid : GetAffectedKeys(srctx->tx->vout[n].scriptPubKey, *spk_man)) { + if (GetDestData(keyid, "used", nullptr)) { + return true; + } + } + } + return false; } bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 48dbae10b248..e8f38991e05e 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -890,9 +890,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati bool IsSpent(const uint256& hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - // Whether this or any UTXO with the same CTxDestination has been spent. - bool IsSpentKey(const CTxDestination& dst) const; - bool IsSpentKey(const uint256& hash, unsigned int n) const; + // Whether this or any known UTXO with the same single key has been spent. + bool IsSpentKey(const uint256& hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned int n, bool used, std::set& tx_destinations) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); std::vector GroupOutputs(const std::vector& outputs, bool single_coin) const; diff --git a/test/functional/wallet_avoidreuse.py b/test/functional/wallet_avoidreuse.py index 63fec49c3e4a..88351bc7587d 100755 --- a/test/functional/wallet_avoidreuse.py +++ b/test/functional/wallet_avoidreuse.py @@ -200,7 +200,7 @@ def test_fund_send_fund_send(self): ''' self.log.info("Test fund send fund send") - fundaddr = self.nodes[1].getnewaddress() + fundaddr = self.nodes[1].getnewaddress(label="") retaddr = self.nodes[0].getnewaddress() self.nodes[0].sendtoaddress(fundaddr, 10) From c75594a6b495c8a887dc72ac050a8b038f057773 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 4 Jan 2023 19:36:08 +0300 Subject: [PATCH 12/12] refactor: implementation of CheckDecryptionKey is unified with bitcoin's implementation Changed relationship between a functino `DecryptHDChain` and `vMasterKey` Removed unused GetEncryptionKeyMutable() --- src/wallet/scriptpubkeyman.cpp | 28 +++++++++------------------- src/wallet/scriptpubkeyman.h | 3 +-- src/wallet/wallet.cpp | 8 +------- src/wallet/wallet.h | 1 - 4 files changed, 11 insertions(+), 29 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 2a8c99f38a08..570990fc1386 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -207,20 +207,10 @@ bool LegacyScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master_key return false; } - // This implementation is sligthly different with bitcoin's - // because function `DecryptHDChain` uses vMasterKey - m_wallet.GetEncryptionKeyMutable() = master_key; - AssertLockHeld(cs_KeyStore); if(!cryptedHDChain.IsNull()) { - bool chainPass = false; // try to decrypt seed and make sure it matches CHDChain hdChainTmp; - if (DecryptHDChain(hdChainTmp)) { - // make sure seed matches this chain - chainPass = cryptedHDChain.GetID() == hdChainTmp.GetSeedHash(); - } - if (!chainPass) { - m_wallet.GetEncryptionKeyMutable().clear(); + if (!DecryptHDChain(master_key, hdChainTmp) || (cryptedHDChain.GetID() != hdChainTmp.GetSeedHash())) { return false; } } @@ -314,7 +304,7 @@ void LegacyScriptPubKeyMan::UpgradeKeyMetadata() CHDChain hdChainCurrent; if (!GetHDChain(hdChainCurrent)) throw std::runtime_error(std::string(__func__) + ": GetHDChain failed"); - if (!DecryptHDChain(hdChainCurrent)) + if (!DecryptHDChain(m_storage.GetEncryptionKey(), hdChainCurrent)) throw std::runtime_error(std::string(__func__) + ": DecryptHDChain failed"); CExtKey masterKey; @@ -505,7 +495,7 @@ bool LegacyScriptPubKeyMan::GetDecryptedHDChain(CHDChain& hdChainRet) return false; } - if (!DecryptHDChain(hdChainTmp)) + if (!DecryptHDChain(m_storage.GetEncryptionKey(), hdChainTmp)) return false; // make sure seed matches this chain @@ -576,7 +566,7 @@ bool LegacyScriptPubKeyMan::EncryptHDChain(const CKeyingMaterial& vMasterKeyIn, return true; } -bool LegacyScriptPubKeyMan::DecryptHDChain(CHDChain& hdChainRet) const +bool LegacyScriptPubKeyMan::DecryptHDChain(const CKeyingMaterial& vMasterKeyIn, CHDChain& hdChainRet) const { LOCK(cs_KeyStore); if (!m_storage.HasEncryptionKeys()) @@ -591,7 +581,7 @@ bool LegacyScriptPubKeyMan::DecryptHDChain(CHDChain& hdChainRet) const SecureVector vchSecureSeed; SecureVector vchSecureCryptedSeed = cryptedHDChain.GetSeed(); std::vector vchCryptedSeed(vchSecureCryptedSeed.begin(), vchSecureCryptedSeed.end()); - if (!DecryptSecret(m_storage.GetEncryptionKey(), vchCryptedSeed, cryptedHDChain.GetID(), vchSecureSeed)) + if (!DecryptSecret(vMasterKeyIn, vchCryptedSeed, cryptedHDChain.GetID(), vchSecureSeed)) return false; hdChainRet = cryptedHDChain; @@ -613,9 +603,9 @@ bool LegacyScriptPubKeyMan::DecryptHDChain(CHDChain& hdChainRet) const std::vector vchCryptedMnemonic(vchSecureCryptedMnemonic.begin(), vchSecureCryptedMnemonic.end()); std::vector vchCryptedMnemonicPassphrase(vchSecureCryptedMnemonicPassphrase.begin(), vchSecureCryptedMnemonicPassphrase.end()); - if (!vchCryptedMnemonic.empty() && !DecryptSecret(m_storage.GetEncryptionKey(), vchCryptedMnemonic, cryptedHDChain.GetID(), vchSecureMnemonic)) + if (!vchCryptedMnemonic.empty() && !DecryptSecret(vMasterKeyIn, vchCryptedMnemonic, cryptedHDChain.GetID(), vchSecureMnemonic)) return false; - if (!vchCryptedMnemonicPassphrase.empty() && !DecryptSecret(m_storage.GetEncryptionKey(), vchCryptedMnemonicPassphrase, cryptedHDChain.GetID(), vchSecureMnemonicPassphrase)) + if (!vchCryptedMnemonicPassphrase.empty() && !DecryptSecret(vMasterKeyIn, vchCryptedMnemonicPassphrase, cryptedHDChain.GetID(), vchSecureMnemonicPassphrase)) return false; if (!hdChainRet.SetMnemonic(vchSecureMnemonic, vchSecureMnemonicPassphrase, false)) @@ -1117,7 +1107,7 @@ bool LegacyScriptPubKeyMan::GetKey(const CKeyID &address, CKey& keyOut) const CHDChain hdChainCurrent; if (!GetHDChain(hdChainCurrent)) throw std::runtime_error(std::string(__func__) + ": GetHDChain failed"); - if (!DecryptHDChain(hdChainCurrent)) + if (!DecryptHDChain(m_storage.GetEncryptionKey(), hdChainCurrent)) throw std::runtime_error(std::string(__func__) + ": DecryptHDChain failed"); // make sure seed matches this chain if (hdChainCurrent.GetID() != hdChainCurrent.GetSeedHash()) @@ -1229,7 +1219,7 @@ void LegacyScriptPubKeyMan::DeriveNewChildKey(WalletBatch &batch, CKeyMetadata& throw std::runtime_error(std::string(__func__) + ": GetHDChain failed"); } - if (!DecryptHDChain(hdChainTmp)) + if (!DecryptHDChain(m_storage.GetEncryptionKey(), hdChainTmp)) throw std::runtime_error(std::string(__func__) + ": DecryptHDChain failed"); // make sure seed matches this chain if (hdChainTmp.GetID() != hdChainTmp.GetSeedHash()) diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 20e196cf6179..6f9c0a18712e 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -30,7 +30,6 @@ class WalletStorage virtual bool CanSupportFeature(enum WalletFeature) const = 0; virtual void SetMinVersion(enum WalletFeature, WalletBatch* = nullptr, bool = false) = 0; virtual const CKeyingMaterial& GetEncryptionKey() const = 0; - virtual CKeyingMaterial& GetEncryptionKeyMutable() = 0; virtual bool HasEncryptionKeys() const = 0; virtual bool IsLocked(bool fForMixing = false) const = 0; }; @@ -399,7 +398,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv */ bool EncryptHDChain(const CKeyingMaterial& vMasterKeyIn, const CHDChain& chain = CHDChain()); - bool DecryptHDChain(CHDChain& hdChainRet) const; + bool DecryptHDChain(const CKeyingMaterial& vMasterKeyIn, CHDChain& hdChainRet) const; bool SetHDChain(const CHDChain& chain); bool GetHDChain(CHDChain& hdChainRet) const; bool SetCryptedHDChain(const CHDChain& chain); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e95434837f94..35c5081462ec 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4982,9 +4982,8 @@ bool CWallet::Unlock(const CKeyingMaterial& vMasterKeyIn, bool fForMixingOnly, b if (!m_spk_man->CheckDecryptionKey(vMasterKeyIn, accept_no_keys)) { return false; } - } else { - vMasterKey = vMasterKeyIn; } + vMasterKey = vMasterKeyIn; fOnlyMixingAllowed = fForMixingOnly; } NotifyStatusChanged(this); @@ -5016,11 +5015,6 @@ const CKeyingMaterial& CWallet::GetEncryptionKey() const return vMasterKey; } -CKeyingMaterial& CWallet::GetEncryptionKeyMutable() -{ - return vMasterKey; -} - bool CWallet::HasEncryptionKeys() const { return !mapMasterKeys.empty(); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index e8f38991e05e..e6d30f699162 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1267,7 +1267,6 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati LegacyScriptPubKeyMan* GetLegacyScriptPubKeyMan() const; const CKeyingMaterial& GetEncryptionKey() const override; - CKeyingMaterial& GetEncryptionKeyMutable() override; bool HasEncryptionKeys() const override; // Temporary LegacyScriptPubKeyMan accessors and aliases.