From 4ce235ef8f9a9dddc52d7ab60c8f71bda1d38873 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 22 Apr 2022 17:31:28 -0300 Subject: [PATCH 1/9] wallet: return 'CoinsResult' struct in `AvailableCoins` Instead of accepting a `vCoins` reference that is cleared at the beginning of the method. Note: This new struct, down the commits line, will contain other `AvailableCoins` useful results. --- src/wallet/rpc/coins.cpp | 2 +- src/wallet/rpc/spend.cpp | 4 +--- src/wallet/spend.cpp | 39 ++++++++++++++++---------------- src/wallet/spend.h | 9 +++++--- src/wallet/test/wallet_tests.cpp | 8 ++----- 5 files changed, 30 insertions(+), 32 deletions(-) diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp index 2649fa586c..5efb61c3bd 100644 --- a/src/wallet/rpc/coins.cpp +++ b/src/wallet/rpc/coins.cpp @@ -638,7 +638,7 @@ RPCHelpMan listunspent() cctl.m_max_depth = nMaxDepth; cctl.m_include_unsafe_inputs = include_unsafe; LOCK(pwallet->cs_wallet); - AvailableCoinsListUnspent(*pwallet, vecOutputs, &cctl, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount); + vecOutputs = AvailableCoinsListUnspent(*pwallet, &cctl, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount).coins; } LOCK(pwallet->cs_wallet); diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index d1a0ba50f6..98e180e4a1 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -1380,7 +1380,6 @@ RPCHelpMan sendall() CMutableTransaction rawTx{ConstructTransaction(options["inputs"], recipient_key_value_pairs, options["locktime"], rbf)}; LOCK(pwallet->cs_wallet); - std::vector all_the_utxos; CAmount total_input_value(0); bool send_max{options.exists("send_max") ? options["send_max"].get_bool() : false}; @@ -1398,8 +1397,7 @@ RPCHelpMan sendall() total_input_value += tx->tx->vout[input.prevout.n].nValue; } } else { - AvailableCoins(*pwallet, all_the_utxos, &coin_control, fee_rate, /*nMinimumAmount=*/0); - for (const COutput& output : all_the_utxos) { + for (const COutput& output : AvailableCoins(*pwallet, &coin_control, fee_rate, /*nMinimumAmount=*/0).coins) { CHECK_NONFATAL(output.input_bytes > 0); if (send_max && fee_rate.GetFee(output.input_bytes) > output.txout.nValue) { continue; diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index fe36cfcc6b..79c8f064c0 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -84,11 +84,17 @@ TxSize CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *walle return CalculateMaximumSignedTxSize(tx, wallet, txouts, coin_control); } -void AvailableCoins(const CWallet& wallet, std::vector& vCoins, const CCoinControl* coinControl, std::optional feerate, const CAmount& nMinimumAmount, const CAmount& nMaximumAmount, const CAmount& nMinimumSumAmount, const uint64_t nMaximumCount) +CoinsResult AvailableCoins(const CWallet& wallet, + const CCoinControl* coinControl, + std::optional feerate, + const CAmount& nMinimumAmount, + const CAmount& nMaximumAmount, + const CAmount& nMinimumSumAmount, + const uint64_t nMaximumCount) { AssertLockHeld(wallet.cs_wallet); - vCoins.clear(); + CoinsResult result; CAmount nTotal = 0; // Either the WALLET_FLAG_AVOID_REUSE flag is not set (in which case we always allow), or we default to avoiding, and only in the case where // a coin control object is provided, and has the avoid address reuse flag set to false, do we allow already used addresses @@ -191,29 +197,30 @@ void AvailableCoins(const CWallet& wallet, std::vector& vCoins, const C bool solvable = provider ? IsSolvable(*provider, wtx.tx->vout[i].scriptPubKey) : false; bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable)); int input_bytes = GetTxSpendSize(wallet, wtx, i, (coinControl && coinControl->fAllowWatchOnly)); - - vCoins.emplace_back(COutPoint(wtx.GetHash(), i), wtx.tx->vout.at(i), nDepth, input_bytes, spendable, solvable, safeTx, wtx.GetTxTime(), tx_from_me, feerate); + result.coins.emplace_back(COutPoint(wtx.GetHash(), i), wtx.tx->vout.at(i), nDepth, input_bytes, spendable, solvable, safeTx, wtx.GetTxTime(), tx_from_me, feerate); // Checks the sum amount of all UTXO's. if (nMinimumSumAmount != MAX_MONEY) { nTotal += wtx.tx->vout[i].nValue; if (nTotal >= nMinimumSumAmount) { - return; + return result; } } // Checks the maximum number of UTXO's. - if (nMaximumCount > 0 && vCoins.size() >= nMaximumCount) { - return; + if (nMaximumCount > 0 && result.coins.size() >= nMaximumCount) { + return result; } } } + + return result; } -void AvailableCoinsListUnspent(const CWallet& wallet, std::vector& vCoins, const CCoinControl* coinControl, const CAmount& nMinimumAmount, const CAmount& nMaximumAmount, const CAmount& nMinimumSumAmount, const uint64_t nMaximumCount) +CoinsResult AvailableCoinsListUnspent(const CWallet& wallet, const CCoinControl* coinControl, const CAmount& nMinimumAmount, const CAmount& nMaximumAmount, const CAmount& nMinimumSumAmount, const uint64_t nMaximumCount) { - AvailableCoins(wallet, vCoins, coinControl, /*feerate=*/ std::nullopt, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount); + return AvailableCoins(wallet, coinControl, /*feerate=*/ std::nullopt, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount); } CAmount GetAvailableBalance(const CWallet& wallet, const CCoinControl* coinControl) @@ -221,9 +228,7 @@ CAmount GetAvailableBalance(const CWallet& wallet, const CCoinControl* coinContr LOCK(wallet.cs_wallet); CAmount balance = 0; - std::vector vCoins; - AvailableCoinsListUnspent(wallet, vCoins, coinControl); - for (const COutput& out : vCoins) { + for (const COutput& out : AvailableCoinsListUnspent(wallet, coinControl).coins) { if (out.spendable) { balance += out.txout.nValue; } @@ -260,11 +265,8 @@ std::map> ListCoins(const CWallet& wallet) AssertLockHeld(wallet.cs_wallet); std::map> result; - std::vector availableCoins; - AvailableCoinsListUnspent(wallet, availableCoins); - - for (const COutput& coin : availableCoins) { + for (const COutput& coin : AvailableCoinsListUnspent(wallet).coins) { CTxDestination address; if ((coin.spendable || (wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && coin.solvable)) && ExtractDestination(FindNonChangeParentOutput(wallet, coin.outpoint).scriptPubKey, address)) { @@ -791,11 +793,10 @@ static std::optional CreateTransactionInternal( CAmount selection_target = recipients_sum + not_input_fees; // Get available coins - std::vector vAvailableCoins; - AvailableCoins(wallet, vAvailableCoins, &coin_control, coin_selection_params.m_effective_feerate, 1, MAX_MONEY, MAX_MONEY, 0); + auto res_available_coins = AvailableCoins(wallet, &coin_control, coin_selection_params.m_effective_feerate, 1, MAX_MONEY, MAX_MONEY, 0); // Choose coins to use - std::optional result = SelectCoins(wallet, vAvailableCoins, /*nTargetValue=*/selection_target, coin_control, coin_selection_params); + std::optional result = SelectCoins(wallet, res_available_coins.coins, /*nTargetValue=*/selection_target, coin_control, coin_selection_params); if (!result) { error = _("Insufficient funds"); return std::nullopt; diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 988058a25a..dde2523eb7 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -34,16 +34,19 @@ struct TxSize { TxSize CalculateMaximumSignedTxSize(const CTransaction& tx, const CWallet* wallet, const std::vector& txouts, const CCoinControl* coin_control = nullptr); TxSize CalculateMaximumSignedTxSize(const CTransaction& tx, const CWallet* wallet, const CCoinControl* coin_control = nullptr) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet); +struct CoinsResult { + std::vector coins; +}; /** - * populate vCoins with vector of available COutputs. + * Return vector of available COutputs. */ -void AvailableCoins(const CWallet& wallet, std::vector& vCoins, const CCoinControl* coinControl = nullptr, std::optional feerate = std::nullopt, const CAmount& nMinimumAmount = 1, const CAmount& nMaximumAmount = MAX_MONEY, const CAmount& nMinimumSumAmount = MAX_MONEY, const uint64_t nMaximumCount = 0) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); +CoinsResult AvailableCoins(const CWallet& wallet, const CCoinControl* coinControl = nullptr, std::optional feerate = std::nullopt, const CAmount& nMinimumAmount = 1, const CAmount& nMaximumAmount = MAX_MONEY, const CAmount& nMinimumSumAmount = MAX_MONEY, const uint64_t nMaximumCount = 0) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); /** * Wrapper function for AvailableCoins which skips the `feerate` parameter. Use this function * to list all available coins (e.g. listunspent RPC) while not intending to fund a transaction. */ -void AvailableCoinsListUnspent(const CWallet& wallet, std::vector& vCoins, const CCoinControl* coinControl = nullptr, const CAmount& nMinimumAmount = 1, const CAmount& nMaximumAmount = MAX_MONEY, const CAmount& nMinimumSumAmount = MAX_MONEY, const uint64_t nMaximumCount = 0) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); +CoinsResult AvailableCoinsListUnspent(const CWallet& wallet, const CCoinControl* coinControl = nullptr, const CAmount& nMinimumAmount = 1, const CAmount& nMaximumAmount = MAX_MONEY, const CAmount& nMinimumSumAmount = MAX_MONEY, const uint64_t nMaximumCount = 0) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); CAmount GetAvailableBalance(const CWallet& wallet, const CCoinControl* coinControl = nullptr); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 70863f5464..0c03b2f4a2 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -583,9 +583,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoinsTest, ListCoinsTestingSetup) // Lock both coins. Confirm number of available coins drops to 0. { LOCK(wallet->cs_wallet); - std::vector available; - AvailableCoinsListUnspent(*wallet, available); - BOOST_CHECK_EQUAL(available.size(), 2U); + BOOST_CHECK_EQUAL(AvailableCoinsListUnspent(*wallet).coins.size(), 2U); } for (const auto& group : list) { for (const auto& coin : group.second) { @@ -595,9 +593,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoinsTest, ListCoinsTestingSetup) } { LOCK(wallet->cs_wallet); - std::vector available; - AvailableCoinsListUnspent(*wallet, available); - BOOST_CHECK_EQUAL(available.size(), 0U); + BOOST_CHECK_EQUAL(AvailableCoinsListUnspent(*wallet).coins.size(), 0U); } // Confirm ListCoins still returns same result as before, despite coins // being locked. From 9472ca0a65396206b3078bddf98f4c1807be2d82 Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 27 Apr 2022 10:34:19 -0300 Subject: [PATCH 2/9] wallet: AvailableCoins, don't call 'wtx.tx->vout[i]' multiple times --- src/wallet/spend.cpp | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 79c8f064c0..78d3ad4b88 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -165,24 +165,27 @@ CoinsResult AvailableCoins(const CWallet& wallet, bool tx_from_me = CachedTxIsFromMe(wallet, wtx, ISMINE_ALL); for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) { + const CTxOut& output = wtx.tx->vout[i]; + const COutPoint outpoint(wtxid, i); + // Only consider selected coins if add_inputs is false - if (coinControl && !coinControl->m_add_inputs && !coinControl->IsSelected(COutPoint(entry.first, i))) { + if (coinControl && !coinControl->m_add_inputs && !coinControl->IsSelected(outpoint)) { continue; } - if (wtx.tx->vout[i].nValue < nMinimumAmount || wtx.tx->vout[i].nValue > nMaximumAmount) + if (output.nValue < nMinimumAmount || output.nValue > nMaximumAmount) continue; - if (coinControl && coinControl->HasSelected() && !coinControl->fAllowOtherInputs && !coinControl->IsSelected(COutPoint(entry.first, i))) + if (coinControl && coinControl->HasSelected() && !coinControl->fAllowOtherInputs && !coinControl->IsSelected(outpoint)) continue; - if (wallet.IsLockedCoin(entry.first, i)) + if (wallet.IsLockedCoin(wtxid, i)) continue; if (wallet.IsSpent(wtxid, i)) continue; - isminetype mine = wallet.IsMine(wtx.tx->vout[i]); + isminetype mine = wallet.IsMine(output); if (mine == ISMINE_NO) { continue; @@ -192,16 +195,16 @@ CoinsResult AvailableCoins(const CWallet& wallet, continue; } - std::unique_ptr provider = wallet.GetSolvingProvider(wtx.tx->vout[i].scriptPubKey); + std::unique_ptr provider = wallet.GetSolvingProvider(output.scriptPubKey); - bool solvable = provider ? IsSolvable(*provider, wtx.tx->vout[i].scriptPubKey) : false; + bool solvable = provider ? IsSolvable(*provider, output.scriptPubKey) : false; bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable)); int input_bytes = GetTxSpendSize(wallet, wtx, i, (coinControl && coinControl->fAllowWatchOnly)); - result.coins.emplace_back(COutPoint(wtx.GetHash(), i), wtx.tx->vout.at(i), nDepth, input_bytes, spendable, solvable, safeTx, wtx.GetTxTime(), tx_from_me, feerate); + result.coins.emplace_back(outpoint, output, nDepth, input_bytes, spendable, solvable, safeTx, wtx.GetTxTime(), tx_from_me, feerate); // Checks the sum amount of all UTXO's. if (nMinimumSumAmount != MAX_MONEY) { - nTotal += wtx.tx->vout[i].nValue; + nTotal += output.nValue; if (nTotal >= nMinimumSumAmount) { return result; From 91902b77202fc636edb3db587cb6e87d9fb9b60a Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 27 Apr 2022 10:37:50 -0300 Subject: [PATCH 3/9] wallet: IsLockedCoin, 'COutPoint' arg instead of (hash, index) --- src/wallet/interfaces.cpp | 2 +- src/wallet/rpc/coins.cpp | 2 +- src/wallet/spend.cpp | 2 +- src/wallet/wallet.cpp | 6 ++---- src/wallet/wallet.h | 2 +- 5 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index e1203817e0..897c198664 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -245,7 +245,7 @@ public: bool isLockedCoin(const COutPoint& output) override { LOCK(m_wallet->cs_wallet); - return m_wallet->IsLockedCoin(output.hash, output.n); + return m_wallet->IsLockedCoin(output); } void listLockedCoins(std::vector& outputs) override { diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp index 5efb61c3bd..d9ecbc661e 100644 --- a/src/wallet/rpc/coins.cpp +++ b/src/wallet/rpc/coins.cpp @@ -345,7 +345,7 @@ RPCHelpMan lockunspent() throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, expected unspent output"); } - const bool is_locked = pwallet->IsLockedCoin(outpt.hash, outpt.n); + const bool is_locked = pwallet->IsLockedCoin(outpt); if (fUnlock && !is_locked) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, expected locked output"); diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 78d3ad4b88..14edfecdca 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -179,7 +179,7 @@ CoinsResult AvailableCoins(const CWallet& wallet, if (coinControl && coinControl->HasSelected() && !coinControl->fAllowOtherInputs && !coinControl->IsSelected(outpoint)) continue; - if (wallet.IsLockedCoin(wtxid, i)) + if (wallet.IsLockedCoin(outpoint)) continue; if (wallet.IsSpent(wtxid, i)) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6c333c709b..19650ed166 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2449,12 +2449,10 @@ bool CWallet::UnlockAllCoins() return success; } -bool CWallet::IsLockedCoin(uint256 hash, unsigned int n) const +bool CWallet::IsLockedCoin(const COutPoint& output) const { AssertLockHeld(cs_wallet); - COutPoint outpt(hash, n); - - return (setLockedCoins.count(outpt) > 0); + return setLockedCoins.count(output) > 0; } void CWallet::ListLockedCoins(std::vector& vOutpts) const diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 7da601c3b7..bd4a4b2846 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -450,7 +450,7 @@ public: /** Display address on an external signer. Returns false if external signer support is not compiled */ bool DisplayAddress(const CTxDestination& dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - bool IsLockedCoin(uint256 hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool IsLockedCoin(const COutPoint& output) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool LockCoin(const COutPoint& output, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool UnlockCoin(const COutPoint& output, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool UnlockAllCoins() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); From a06fa94ff81e2bccef0316ea5ec4eca0f4de5071 Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 27 Apr 2022 10:52:30 -0300 Subject: [PATCH 4/9] wallet: IsSpent, 'COutPoint' arg instead of (hash, index) --- src/wallet/interfaces.cpp | 4 ++-- src/wallet/receive.cpp | 15 +++++++-------- src/wallet/rpc/coins.cpp | 2 +- src/wallet/rpc/spend.cpp | 2 +- src/wallet/spend.cpp | 2 +- src/wallet/wallet.cpp | 6 ++---- src/wallet/wallet.h | 2 +- 7 files changed, 15 insertions(+), 18 deletions(-) diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 897c198664..f54b2c83d2 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -110,7 +110,7 @@ WalletTxOut MakeWalletTxOut(const CWallet& wallet, result.txout = wtx.tx->vout[n]; result.time = wtx.GetTxTime(); result.depth_in_main_chain = depth; - result.is_spent = wallet.IsSpent(wtx.GetHash(), n); + result.is_spent = wallet.IsSpent(COutPoint(wtx.GetHash(), n)); return result; } @@ -121,7 +121,7 @@ WalletTxOut MakeWalletTxOut(const CWallet& wallet, result.txout = output.txout; result.time = output.time; result.depth_in_main_chain = output.depth; - result.is_spent = wallet.IsSpent(output.outpoint.hash, output.outpoint.n); + result.is_spent = wallet.IsSpent(output.outpoint); return result; } diff --git a/src/wallet/receive.cpp b/src/wallet/receive.cpp index 8cce07b921..39d4574def 100644 --- a/src/wallet/receive.cpp +++ b/src/wallet/receive.cpp @@ -204,9 +204,8 @@ CAmount CachedTxGetAvailableCredit(const CWallet& wallet, const CWalletTx& wtx, bool allow_used_addresses = (filter & ISMINE_USED) || !wallet.IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE); CAmount nCredit = 0; uint256 hashTx = wtx.GetHash(); - for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) - { - if (!wallet.IsSpent(hashTx, i) && (allow_used_addresses || !wallet.IsSpentKey(hashTx, i))) { + for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) { + if (!wallet.IsSpent(COutPoint(hashTx, i)) && (allow_used_addresses || !wallet.IsSpentKey(hashTx, i))) { const CTxOut &txout = wtx.tx->vout[i]; nCredit += OutputGetCredit(wallet, txout, filter); if (!MoneyRange(nCredit)) @@ -371,15 +370,15 @@ std::map GetAddressBalances(const CWallet& wallet) if (nDepth < (CachedTxIsFromMe(wallet, wtx, ISMINE_ALL) ? 0 : 1)) continue; - for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) - { + for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) { + const auto& output = wtx.tx->vout[i]; CTxDestination addr; - if (!wallet.IsMine(wtx.tx->vout[i])) + if (!wallet.IsMine(output)) continue; - if(!ExtractDestination(wtx.tx->vout[i].scriptPubKey, addr)) + if(!ExtractDestination(output.scriptPubKey, addr)) continue; - CAmount n = wallet.IsSpent(walletEntry.first, i) ? 0 : wtx.tx->vout[i].nValue; + CAmount n = wallet.IsSpent(COutPoint(walletEntry.first, i)) ? 0 : output.nValue; balances[addr] += n; } } diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp index d9ecbc661e..3eea03ff78 100644 --- a/src/wallet/rpc/coins.cpp +++ b/src/wallet/rpc/coins.cpp @@ -341,7 +341,7 @@ RPCHelpMan lockunspent() throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, vout index out of bounds"); } - if (pwallet->IsSpent(outpt.hash, outpt.n)) { + if (pwallet->IsSpent(outpt)) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, expected unspent output"); } diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index 98e180e4a1..9627111a06 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -1387,7 +1387,7 @@ RPCHelpMan sendall() throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot combine send_max with specific inputs."); } else if (options.exists("inputs")) { for (const CTxIn& input : rawTx.vin) { - if (pwallet->IsSpent(input.prevout.hash, input.prevout.n)) { + if (pwallet->IsSpent(input.prevout)) { throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Input not available. UTXO (%s:%d) was already spent.", input.prevout.hash.ToString(), input.prevout.n)); } const CWalletTx* tx{pwallet->GetWalletTx(input.prevout.hash)}; diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 14edfecdca..d178358048 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -182,7 +182,7 @@ CoinsResult AvailableCoins(const CWallet& wallet, if (wallet.IsLockedCoin(outpoint)) continue; - if (wallet.IsSpent(wtxid, i)) + if (wallet.IsSpent(outpoint)) continue; isminetype mine = wallet.IsMine(output); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 19650ed166..de5f191520 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -629,14 +629,12 @@ void CWallet::SyncMetaData(std::pair ran * Outpoint is spent if any non-conflicted transaction * spends it: */ -bool CWallet::IsSpent(const uint256& hash, unsigned int n) const +bool CWallet::IsSpent(const COutPoint& outpoint) const { - const COutPoint outpoint(hash, n); std::pair range; range = mapTxSpends.equal_range(outpoint); - for (TxSpends::const_iterator it = range.first; it != range.second; ++it) - { + for (TxSpends::const_iterator it = range.first; it != range.second; ++it) { const uint256& wtxid = it->second; std::map::const_iterator mit = mapWallet.find(wtxid); if (mit != mapWallet.end()) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index bd4a4b2846..7969b59f4a 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -441,7 +441,7 @@ public: //! check whether we support the named feature bool CanSupportFeature(enum WalletFeature wf) const override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { AssertLockHeld(cs_wallet); return IsFeatureSupported(nWalletVersion, wf); } - bool IsSpent(const uint256& hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool IsSpent(const COutPoint& outpoint) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); // 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); From 3d8a2822570e3cf4d1bc4f9d59b5dcb0145920ad Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 27 Apr 2022 11:01:35 -0300 Subject: [PATCH 5/9] wallet: decouple IsSpentKey(scriptPubKey) from IsSpentKey(hash, n) This will be used in a follow-up commit to prevent extra 'GetWalletTx' lookups if the function caller already have the wtx and can just provide the scriptPubKey directly. --- src/wallet/wallet.cpp | 55 +++++++++++++++++++++++-------------------- src/wallet/wallet.h | 1 + 2 files changed, 31 insertions(+), 25 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index de5f191520..57abc6fb19 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -910,31 +910,36 @@ bool CWallet::IsSpentKey(const uint256& hash, unsigned int n) const { AssertLockHeld(cs_wallet); const CWalletTx* srctx = GetWalletTx(hash); - if (srctx) { - assert(srctx->tx->vout.size() > n); - CTxDestination dest; - if (!ExtractDestination(srctx->tx->vout[n].scriptPubKey, dest)) { - return false; - } - if (IsAddressUsed(dest)) { - return true; - } - if (IsLegacy()) { - LegacyScriptPubKeyMan* spk_man = GetLegacyScriptPubKeyMan(); - assert(spk_man != nullptr); - for (const auto& keyid : GetAffectedKeys(srctx->tx->vout[n].scriptPubKey, *spk_man)) { - WitnessV0KeyHash wpkh_dest(keyid); - if (IsAddressUsed(wpkh_dest)) { - return true; - } - ScriptHash sh_wpkh_dest(GetScriptForDestination(wpkh_dest)); - if (IsAddressUsed(sh_wpkh_dest)) { - return true; - } - PKHash pkh_dest(keyid); - if (IsAddressUsed(pkh_dest)) { - return true; - } + if (!srctx) return false; + assert(srctx->tx->vout.size() > n); + return IsSpentKey(srctx->tx->vout[n].scriptPubKey); +} + +bool CWallet::IsSpentKey(const CScript& scriptPubKey) const +{ + AssertLockHeld(cs_wallet); + CTxDestination dest; + if (!ExtractDestination(scriptPubKey, dest)) { + return false; + } + if (IsAddressUsed(dest)) { + return true; + } + if (IsLegacy()) { + LegacyScriptPubKeyMan* spk_man = GetLegacyScriptPubKeyMan(); + assert(spk_man != nullptr); + for (const auto& keyid : GetAffectedKeys(scriptPubKey, *spk_man)) { + WitnessV0KeyHash wpkh_dest(keyid); + if (IsAddressUsed(wpkh_dest)) { + return true; + } + ScriptHash sh_wpkh_dest(GetScriptForDestination(wpkh_dest)); + if (IsAddressUsed(sh_wpkh_dest)) { + return true; + } + PKHash pkh_dest(keyid); + if (IsAddressUsed(pkh_dest)) { + return true; } } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 7969b59f4a..61b59e7895 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -445,6 +445,7 @@ public: // 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); + bool IsSpentKey(const CScript& scriptPubKey) 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); /** Display address on an external signer. Returns false if external signer support is not compiled */ From 4b83bf8dbcf6b8b1c1293575391e90ac7e21b0e0 Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 27 Apr 2022 11:04:31 -0300 Subject: [PATCH 6/9] wallet: avoid extra IsSpentKey -> GetWalletTx lookups --- src/wallet/receive.cpp | 4 ++-- src/wallet/rpc/coins.cpp | 2 +- src/wallet/spend.cpp | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/wallet/receive.cpp b/src/wallet/receive.cpp index 39d4574def..8de4017371 100644 --- a/src/wallet/receive.cpp +++ b/src/wallet/receive.cpp @@ -205,8 +205,8 @@ CAmount CachedTxGetAvailableCredit(const CWallet& wallet, const CWalletTx& wtx, CAmount nCredit = 0; uint256 hashTx = wtx.GetHash(); for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) { - if (!wallet.IsSpent(COutPoint(hashTx, i)) && (allow_used_addresses || !wallet.IsSpentKey(hashTx, i))) { - const CTxOut &txout = wtx.tx->vout[i]; + const CTxOut& txout = wtx.tx->vout[i]; + if (!wallet.IsSpent(COutPoint(hashTx, i)) && (allow_used_addresses || !wallet.IsSpentKey(txout.scriptPubKey))) { nCredit += OutputGetCredit(wallet, txout, filter); if (!MoneyRange(nCredit)) throw std::runtime_error(std::string(__func__) + " : value out of range"); diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp index 3eea03ff78..ad59cc94ff 100644 --- a/src/wallet/rpc/coins.cpp +++ b/src/wallet/rpc/coins.cpp @@ -649,7 +649,7 @@ RPCHelpMan listunspent() CTxDestination address; const CScript& scriptPubKey = out.txout.scriptPubKey; bool fValidAddress = ExtractDestination(scriptPubKey, address); - bool reused = avoid_reuse && pwallet->IsSpentKey(out.outpoint.hash, out.outpoint.n); + bool reused = avoid_reuse && pwallet->IsSpentKey(scriptPubKey); if (destinations.size() && (!fValidAddress || !destinations.count(address))) continue; diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index d178358048..c598f2c925 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -191,7 +191,7 @@ CoinsResult AvailableCoins(const CWallet& wallet, continue; } - if (!allow_used_addresses && wallet.IsSpentKey(wtxid, i)) { + if (!allow_used_addresses && wallet.IsSpentKey(output.scriptPubKey)) { continue; } From cdf185ccfb2085e5a4bf82d833392d74b748aeff Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 27 Apr 2022 11:08:53 -0300 Subject: [PATCH 7/9] wallet: remove unused IsSpentKey(hash, index) method --- src/wallet/wallet.cpp | 9 --------- src/wallet/wallet.h | 3 +-- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 57abc6fb19..679fedca50 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -906,15 +906,6 @@ void CWallet::SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned } } -bool CWallet::IsSpentKey(const uint256& hash, unsigned int n) const -{ - AssertLockHeld(cs_wallet); - const CWalletTx* srctx = GetWalletTx(hash); - if (!srctx) return false; - assert(srctx->tx->vout.size() > n); - return IsSpentKey(srctx->tx->vout[n].scriptPubKey); -} - bool CWallet::IsSpentKey(const CScript& scriptPubKey) const { AssertLockHeld(cs_wallet); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 61b59e7895..a75b6981e1 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -443,8 +443,7 @@ public: bool IsSpent(const COutPoint& outpoint) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - // 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); + // Whether this or any known scriptPubKey with the same single key has been spent. bool IsSpentKey(const CScript& scriptPubKey) 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); From 162d4ad10f28c5fa38551d69ce9b296ab3933c77 Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 2 Jun 2022 14:45:04 -0300 Subject: [PATCH 8/9] wallet: add 'only_spendable' filter to AvailableCoins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We are skipping the non-spendable coins that appear in vCoins ('AvailableCoins' result) later, in several parts of the CreateTransaction and GetBalance flows: GetAvailableBalance (1) gets all the available coins calling AvailableCoins and, right away, walk through the entire vector, skipping the non-spendable coins, to calculate the total balance. Inside CreateTransactionInternal —> SelectCoins(vCoins,...), we have several calls to AttemptSelection which, on each of them internally, we call twice to GroupOutputs which internally has two for-loops over the entire vCoins vector that skip the non-spendable coins. So, Purpose is not add the non-spendable coins into the AvailableCoins result (vCoins) in the first place for the processes that aren’t using them at all, so we don’t waste resources skipping them later so many times. Note: this speedup is for all the processes that call to CreateTransaction and GetBalance* internally. --- src/wallet/spend.cpp | 17 ++++++++++++++--- src/wallet/spend.h | 10 +++++++++- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index c598f2c925..63e04c7a9b 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -90,7 +90,8 @@ CoinsResult AvailableCoins(const CWallet& wallet, const CAmount& nMinimumAmount, const CAmount& nMaximumAmount, const CAmount& nMinimumSumAmount, - const uint64_t nMaximumCount) + const uint64_t nMaximumCount, + bool only_spendable) { AssertLockHeld(wallet.cs_wallet); @@ -199,6 +200,10 @@ CoinsResult AvailableCoins(const CWallet& wallet, bool solvable = provider ? IsSolvable(*provider, output.scriptPubKey) : false; bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable)); + + // Filter by spendable outputs only + if (!spendable && only_spendable) continue; + int input_bytes = GetTxSpendSize(wallet, wtx, i, (coinControl && coinControl->fAllowWatchOnly)); result.coins.emplace_back(outpoint, output, nDepth, input_bytes, spendable, solvable, safeTx, wtx.GetTxTime(), tx_from_me, feerate); @@ -223,7 +228,7 @@ CoinsResult AvailableCoins(const CWallet& wallet, CoinsResult AvailableCoinsListUnspent(const CWallet& wallet, const CCoinControl* coinControl, const CAmount& nMinimumAmount, const CAmount& nMaximumAmount, const CAmount& nMinimumSumAmount, const uint64_t nMaximumCount) { - return AvailableCoins(wallet, coinControl, /*feerate=*/ std::nullopt, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount); + return AvailableCoins(wallet, coinControl, /*feerate=*/ std::nullopt, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount, /*only_spendable=*/false); } CAmount GetAvailableBalance(const CWallet& wallet, const CCoinControl* coinControl) @@ -796,7 +801,13 @@ static std::optional CreateTransactionInternal( CAmount selection_target = recipients_sum + not_input_fees; // Get available coins - auto res_available_coins = AvailableCoins(wallet, &coin_control, coin_selection_params.m_effective_feerate, 1, MAX_MONEY, MAX_MONEY, 0); + auto res_available_coins = AvailableCoins(wallet, + &coin_control, + coin_selection_params.m_effective_feerate, + 1, /*nMinimumAmount*/ + MAX_MONEY, /*nMaximumAmount*/ + MAX_MONEY, /*nMinimumSumAmount*/ + 0); /*nMaximumCount*/ // Choose coins to use std::optional result = SelectCoins(wallet, res_available_coins.coins, /*nTargetValue=*/selection_target, coin_control, coin_selection_params); diff --git a/src/wallet/spend.h b/src/wallet/spend.h index dde2523eb7..84628517b0 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -39,8 +39,16 @@ struct CoinsResult { }; /** * Return vector of available COutputs. + * By default, returns only the spendable coins. */ -CoinsResult AvailableCoins(const CWallet& wallet, const CCoinControl* coinControl = nullptr, std::optional feerate = std::nullopt, const CAmount& nMinimumAmount = 1, const CAmount& nMaximumAmount = MAX_MONEY, const CAmount& nMinimumSumAmount = MAX_MONEY, const uint64_t nMaximumCount = 0) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); +CoinsResult AvailableCoins(const CWallet& wallet, + const CCoinControl* coinControl = nullptr, + std::optional feerate = std::nullopt, + const CAmount& nMinimumAmount = 1, + const CAmount& nMaximumAmount = MAX_MONEY, + const CAmount& nMinimumSumAmount = MAX_MONEY, + const uint64_t nMaximumCount = 0, + bool only_spendable = true) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); /** * Wrapper function for AvailableCoins which skips the `feerate` parameter. Use this function From fd5c996d1609e6f88769f6f3ef0c322e3435b3aa Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 27 Apr 2022 11:15:09 -0300 Subject: [PATCH 9/9] wallet: GetAvailableBalance, remove double walk-through every available coin Filtering `AvailableCoins` by spendable outputs only and using the retrieved total_amount. --- src/wallet/spend.cpp | 22 ++++++++++------------ src/wallet/spend.h | 2 ++ 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 63e04c7a9b..1428a8e217 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -96,7 +96,6 @@ CoinsResult AvailableCoins(const CWallet& wallet, AssertLockHeld(wallet.cs_wallet); CoinsResult result; - CAmount nTotal = 0; // Either the WALLET_FLAG_AVOID_REUSE flag is not set (in which case we always allow), or we default to avoiding, and only in the case where // a coin control object is provided, and has the avoid address reuse flag set to false, do we allow already used addresses bool allow_used_addresses = !wallet.IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE) || (coinControl && !coinControl->m_avoid_address_reuse); @@ -206,12 +205,11 @@ CoinsResult AvailableCoins(const CWallet& wallet, int input_bytes = GetTxSpendSize(wallet, wtx, i, (coinControl && coinControl->fAllowWatchOnly)); result.coins.emplace_back(outpoint, output, nDepth, input_bytes, spendable, solvable, safeTx, wtx.GetTxTime(), tx_from_me, feerate); + result.total_amount += output.nValue; // Checks the sum amount of all UTXO's. if (nMinimumSumAmount != MAX_MONEY) { - nTotal += output.nValue; - - if (nTotal >= nMinimumSumAmount) { + if (result.total_amount >= nMinimumSumAmount) { return result; } } @@ -234,14 +232,14 @@ CoinsResult AvailableCoinsListUnspent(const CWallet& wallet, const CCoinControl* CAmount GetAvailableBalance(const CWallet& wallet, const CCoinControl* coinControl) { LOCK(wallet.cs_wallet); - - CAmount balance = 0; - for (const COutput& out : AvailableCoinsListUnspent(wallet, coinControl).coins) { - if (out.spendable) { - balance += out.txout.nValue; - } - } - return balance; + return AvailableCoins(wallet, + coinControl, + std::nullopt, /*feerate=*/ + 1, /*nMinimumAmount*/ + MAX_MONEY, /*nMaximumAmount*/ + MAX_MONEY, /*nMinimumSumAmount*/ + 0 /*nMaximumCount*/ + ).total_amount; } const CTxOut& FindNonChangeParentOutput(const CWallet& wallet, const CTransaction& tx, int output) diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 84628517b0..cba42d6fae 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -36,6 +36,8 @@ TxSize CalculateMaximumSignedTxSize(const CTransaction& tx, const CWallet* walle struct CoinsResult { std::vector coins; + // Sum of all the coins amounts + CAmount total_amount{0}; }; /** * Return vector of available COutputs.