From 8a105ecd1aeff15f84c3883e2762bf71ad59d920 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 28 Mar 2022 15:39:25 -0400 Subject: wallet: Use CalculateMaximumSignedInputSize to indicate solvability In AvailableCoins, we need to know whether we can solve for an output. This was done by using IsSolvable, which just calls ProduceSignature and produces a dummy signature. However, we already do that in order to get the size of the input by using CalculateMaximumSignedInputSize. As this function returns -1 if ProduceSignature fails, we can just remove the use of IsSolvable and check that input_bytes is not -1 to determine the solvability of an output. --- src/wallet/spend.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index c00a2cd023..cacefffe35 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -213,7 +213,10 @@ CoinsResult AvailableCoins(const CWallet& wallet, std::unique_ptr provider = wallet.GetSolvingProvider(output.scriptPubKey); - bool solvable = provider ? IsSolvable(*provider, output.scriptPubKey) : false; + int input_bytes = CalculateMaximumSignedInputSize(output, COutPoint(), provider.get(), coinControl); + // Because CalculateMaximumSignedInputSize just uses ProduceSignature and makes a dummy signature, + // it is safe to assume that this input is solvable if input_bytes is greater -1. + bool solvable = input_bytes > -1; bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable)); // Filter by spendable outputs only @@ -243,7 +246,6 @@ CoinsResult AvailableCoins(const CWallet& wallet, type = Solver(output.scriptPubKey, return_values_unused); } - int input_bytes = CalculateMaximumSignedInputSize(output, COutPoint(), provider.get(), coinControl); COutput coin(outpoint, output, nDepth, input_bytes, spendable, solvable, safeTx, wtx.GetTxTime(), tx_from_me, feerate); switch (type) { case TxoutType::WITNESS_UNKNOWN: -- cgit v1.2.3 From 1f798fe85ba952273005f68e36ed48cfc36f4c9d Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 28 Mar 2022 16:47:59 -0400 Subject: wallet: Cache SigningProviders In order to avoid constantly re-deriving the same keys in DescriptorScriptPubKeyMan, cache the SigningProviders generated inside of GetSigningProvider. --- src/wallet/scriptpubkeyman.cpp | 17 ++++++++++++++--- src/wallet/scriptpubkeyman.h | 2 ++ 2 files changed, 16 insertions(+), 3 deletions(-) (limited to 'src') diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index bba31dfe0b..a0efc4c063 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -2075,10 +2075,21 @@ std::unique_ptr DescriptorScriptPubKeyMan::GetSigningProvid std::unique_ptr DescriptorScriptPubKeyMan::GetSigningProvider(int32_t index, bool include_private) const { AssertLockHeld(cs_desc_man); - // Get the scripts, keys, and key origins for this script + std::unique_ptr out_keys = std::make_unique(); - std::vector scripts_temp; - if (!m_wallet_descriptor.descriptor->ExpandFromCache(index, m_wallet_descriptor.cache, scripts_temp, *out_keys)) return nullptr; + + // Fetch SigningProvider from cache to avoid re-deriving + auto it = m_map_signing_providers.find(index); + if (it != m_map_signing_providers.end()) { + *out_keys = Merge(*out_keys, it->second); + } else { + // Get the scripts, keys, and key origins for this script + std::vector scripts_temp; + if (!m_wallet_descriptor.descriptor->ExpandFromCache(index, m_wallet_descriptor.cache, scripts_temp, *out_keys)) return nullptr; + + // Cache SigningProvider so we don't need to re-derive if we need this SigningProvider again + m_map_signing_providers[index] = *out_keys; + } if (HavePrivateKeys() && include_private) { FlatSigningProvider master_provider; diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index eebc05330f..d0af09463b 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -547,6 +547,8 @@ private: KeyMap GetKeys() const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); + // Cached FlatSigningProviders to avoid regenerating them each time they are needed. + mutable std::map m_map_signing_providers; // Fetch the SigningProvider for the given script and optionally include private keys std::unique_ptr GetSigningProvider(const CScript& script, bool include_private = false) const; // Fetch the SigningProvider for the given pubkey and always include private keys. This should only be called by signing code. -- cgit v1.2.3 From 97532867cf51db3e941231fbdc60f9f4fa0012a0 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 17 Sep 2019 18:56:50 -0400 Subject: Change mapTxSpends to be a std::unordered_multimap --- src/wallet/feebumper.cpp | 2 +- src/wallet/test/wallet_tests.cpp | 6 +++--- src/wallet/wallet.cpp | 36 ++++++++++++++++++++++-------------- src/wallet/wallet.h | 4 ++-- 4 files changed, 28 insertions(+), 20 deletions(-) (limited to 'src') diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index 43fdcee48d..3ae81456b6 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -21,7 +21,7 @@ namespace wallet { //! mined, or conflicts with a mined transaction. Return a feebumper::Result. static feebumper::Result PreconditionChecks(const CWallet& wallet, const CWalletTx& wtx, std::vector& errors) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) { - if (wallet.HasWalletSpend(wtx.GetHash())) { + if (wallet.HasWalletSpend(wtx.tx)) { errors.push_back(Untranslated("Transaction has descendants in the wallet")); return feebumper::Result::INVALID_PARAMETER; } diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 81183aa738..ac87800293 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -843,16 +843,16 @@ BOOST_FIXTURE_TEST_CASE(ZapSelectTx, TestChain100Setup) { auto block_hash = block_tx.GetHash(); - auto prev_hash = m_coinbase_txns[0]->GetHash(); + auto prev_tx = m_coinbase_txns[0]; LOCK(wallet->cs_wallet); - BOOST_CHECK(wallet->HasWalletSpend(prev_hash)); + BOOST_CHECK(wallet->HasWalletSpend(prev_tx)); BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_hash), 1u); std::vector vHashIn{ block_hash }, vHashOut; BOOST_CHECK_EQUAL(wallet->ZapSelectTx(vHashIn, vHashOut), DBErrors::LOAD_OK); - BOOST_CHECK(!wallet->HasWalletSpend(prev_hash)); + BOOST_CHECK(!wallet->HasWalletSpend(prev_tx)); BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_hash), 0u); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 54a3221e2d..e244b604a9 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -569,11 +569,17 @@ std::set CWallet::GetConflicts(const uint256& txid) const return result; } -bool CWallet::HasWalletSpend(const uint256& txid) const +bool CWallet::HasWalletSpend(const CTransactionRef& tx) const { AssertLockHeld(cs_wallet); - auto iter = mapTxSpends.lower_bound(COutPoint(txid, 0)); - return (iter != mapTxSpends.end() && iter->first.hash == txid); + const uint256& txid = tx->GetHash(); + for (unsigned int i = 0; i < tx->vout.size(); ++i) { + auto iter = mapTxSpends.find(COutPoint(txid, i)); + if (iter != mapTxSpends.end()) { + return true; + } + } + return false; } void CWallet::Flush() @@ -1191,12 +1197,13 @@ bool CWallet::AbandonTransaction(const uint256& hashTx) batch.WriteTx(wtx); NotifyTransactionChanged(wtx.GetHash(), CT_UPDATED); // Iterate over all its outputs, and mark transactions in the wallet that spend them abandoned too - TxSpends::const_iterator iter = mapTxSpends.lower_bound(COutPoint(now, 0)); - while (iter != mapTxSpends.end() && iter->first.hash == now) { - if (!done.count(iter->second)) { - todo.insert(iter->second); + for (unsigned int i = 0; i < wtx.tx->vout.size(); ++i) { + std::pair range = mapTxSpends.equal_range(COutPoint(now, i)); + for (TxSpends::const_iterator iter = range.first; iter != range.second; ++iter) { + if (!done.count(iter->second)) { + todo.insert(iter->second); + } } - iter++; } // If a transaction changes 'conflicted' state, that changes the balance // available of the outputs it spends. So force those to be recomputed @@ -1242,12 +1249,13 @@ void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, c wtx.MarkDirty(); batch.WriteTx(wtx); // Iterate over all its outputs, and mark transactions in the wallet that spend them conflicted too - TxSpends::const_iterator iter = mapTxSpends.lower_bound(COutPoint(now, 0)); - while (iter != mapTxSpends.end() && iter->first.hash == now) { - if (!done.count(iter->second)) { - todo.insert(iter->second); - } - iter++; + for (unsigned int i = 0; i < wtx.tx->vout.size(); ++i) { + std::pair range = mapTxSpends.equal_range(COutPoint(now, i)); + for (TxSpends::const_iterator iter = range.first; iter != range.second; ++iter) { + if (!done.count(iter->second)) { + todo.insert(iter->second); + } + } } // If a transaction changes 'conflicted' state, that changes the balance // available of the outputs it spends. So force those to be recomputed diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 15f5917040..5780749283 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -259,7 +259,7 @@ private: * detect and report conflicts (double-spends or * mutated transactions where the mutant gets mined). */ - typedef std::multimap TxSpends; + typedef std::unordered_multimap TxSpends; TxSpends mapTxSpends GUARDED_BY(cs_wallet); void AddToSpends(const COutPoint& outpoint, const uint256& wtxid, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void AddToSpends(const CWalletTx& wtx, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); @@ -708,7 +708,7 @@ public: std::set GetConflicts(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Check if a given transaction has any of its outputs spent by another transaction in the wallet - bool HasWalletSpend(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool HasWalletSpend(const CTransactionRef& tx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Flush wallet (bitdb flush) void Flush(); -- cgit v1.2.3 From 272356024db978c92112167f8d8e4cc62adad63d Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 17 Sep 2019 19:44:19 -0400 Subject: Change getWalletTxs to return a set instead of a vector For some reason, the primary consumer of getWalletTxs requires the transactions to be in hash order when it is processing them. std::map will iterate in hash order so the transactions end up in that order when placed into the vector. To ensure this order when mapWallet is no longer ordered, the vector is replaced with a set which will maintain the hash order. --- src/interfaces/wallet.h | 4 +++- src/wallet/interfaces.cpp | 7 +++---- 2 files changed, 6 insertions(+), 5 deletions(-) (limited to 'src') diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index e29fefae56..4ab416f451 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -183,7 +183,7 @@ public: virtual WalletTx getWalletTx(const uint256& txid) = 0; //! Get list of all wallet transactions. - virtual std::vector getWalletTxs() = 0; + virtual std::set getWalletTxs() = 0; //! Try to get updated status for a particular transaction, if possible without blocking. virtual bool tryGetTxStatus(const uint256& txid, @@ -395,6 +395,8 @@ struct WalletTx int64_t time; std::map value_map; bool is_coinbase; + + bool operator<(const WalletTx& a) const { return tx->GetHash() < a.tx->GetHash(); } }; //! Updated transaction status. diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 23f91d9b3a..5add128717 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -318,13 +318,12 @@ public: } return {}; } - std::vector getWalletTxs() override + std::set getWalletTxs() override { LOCK(m_wallet->cs_wallet); - std::vector result; - result.reserve(m_wallet->mapWallet.size()); + std::set result; for (const auto& entry : m_wallet->mapWallet) { - result.emplace_back(MakeWalletTx(*m_wallet, entry.second)); + result.emplace(MakeWalletTx(*m_wallet, entry.second)); } return result; } -- cgit v1.2.3 From bc886fcb31e1afa7bbf7b86bfd93e51da7076ccf Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 17 Sep 2019 17:04:43 -0400 Subject: Change mapWallet to be a std::unordered_map --- src/wallet/wallet.cpp | 10 +++++----- src/wallet/wallet.h | 4 +++- 2 files changed, 8 insertions(+), 6 deletions(-) (limited to 'src') diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e244b604a9..74650b8f3f 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -414,7 +414,7 @@ std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& b const CWalletTx* CWallet::GetWalletTx(const uint256& hash) const { AssertLockHeld(cs_wallet); - std::map::const_iterator it = mapWallet.find(hash); + const auto it = mapWallet.find(hash); if (it == mapWallet.end()) return nullptr; return &(it->second); @@ -551,7 +551,7 @@ std::set CWallet::GetConflicts(const uint256& txid) const std::set result; AssertLockHeld(cs_wallet); - std::map::const_iterator it = mapWallet.find(txid); + const auto it = mapWallet.find(txid); if (it == mapWallet.end()) return result; const CWalletTx& wtx = it->second; @@ -642,7 +642,7 @@ bool CWallet::IsSpent(const COutPoint& outpoint) const for (TxSpends::const_iterator it = range.first; it != range.second; ++it) { const uint256& wtxid = it->second; - std::map::const_iterator mit = mapWallet.find(wtxid); + const auto mit = mapWallet.find(wtxid); if (mit != mapWallet.end()) { int depth = GetTxDepthInMainChain(mit->second); if (depth > 0 || (depth == 0 && !mit->second.isAbandoned())) @@ -1372,7 +1372,7 @@ CAmount CWallet::GetDebit(const CTxIn &txin, const isminefilter& filter) const { { LOCK(cs_wallet); - std::map::const_iterator mi = mapWallet.find(txin.prevout.hash); + const auto mi = mapWallet.find(txin.prevout.hash); if (mi != mapWallet.end()) { const CWalletTx& prev = (*mi).second; @@ -1961,7 +1961,7 @@ bool CWallet::SignTransaction(CMutableTransaction& tx) const // Build coins map std::map coins; for (auto& input : tx.vin) { - std::map::const_iterator mi = mapWallet.find(input.prevout.hash); + const auto mi = mapWallet.find(input.prevout.hash); if(mi == mapWallet.end() || input.prevout.n >= mi->second.tx->vout.size()) { return false; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 5780749283..bf9a12c01b 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -37,6 +38,7 @@ #include #include #include +#include #include #include @@ -390,7 +392,7 @@ public: /** Map from txid to CWalletTx for all transactions this wallet is * interested in, including received and sent transactions. */ - std::map mapWallet GUARDED_BY(cs_wallet); + std::unordered_map mapWallet GUARDED_BY(cs_wallet); typedef std::multimap TxItems; TxItems wtxOrdered; -- cgit v1.2.3