diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index f39471b871..161372784b 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -676,18 +676,24 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup) BOOST_CHECK_EQUAL(list.begin()->second.size(), 2); // Lock both coins. Confirm number of available coins drops to 0. - std::vector available; - wallet->AvailableCoins(available); - BOOST_CHECK_EQUAL(available.size(), 2); + { + LOCK2(cs_main, wallet->cs_wallet); + std::vector available; + wallet->AvailableCoins(available); + BOOST_CHECK_EQUAL(available.size(), 2); + } for (const auto& group : list) { for (const auto& coin : group.second) { LOCK(wallet->cs_wallet); wallet->LockCoin(COutPoint(coin.tx->GetHash(), coin.i)); } } - wallet->AvailableCoins(available); - BOOST_CHECK_EQUAL(available.size(), 0); - + { + LOCK2(cs_main, wallet->cs_wallet); + std::vector available; + wallet->AvailableCoins(available); + BOOST_CHECK_EQUAL(available.size(), 0); + } // Confirm ListCoins still returns same result as before, despite coins // being locked. list = wallet->ListCoins(); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 1b3a3ee50f..7f36aefeaf 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2198,111 +2198,109 @@ CAmount CWallet::GetAvailableBalance(const CCoinControl* coinControl) const void CWallet::AvailableCoins(std::vector &vCoins, bool fOnlySafe, const CCoinControl *coinControl, const CAmount &nMinimumAmount, const CAmount &nMaximumAmount, const CAmount &nMinimumSumAmount, const uint64_t nMaximumCount, const int nMinDepth, const int nMaxDepth) const { + AssertLockHeld(cs_main); + AssertLockHeld(cs_wallet); + vCoins.clear(); + CAmount nTotal = 0; + for (const auto& entry : mapWallet) { - LOCK2(cs_main, cs_wallet); + const uint256& wtxid = entry.first; + const CWalletTx* pcoin = &entry.second; - CAmount nTotal = 0; + if (!CheckFinalTx(*pcoin->tx)) + continue; - for (const auto& entry : mapWallet) - { - const uint256& wtxid = entry.first; - const CWalletTx* pcoin = &entry.second; + if (pcoin->IsCoinBase() && pcoin->GetBlocksToMaturity() > 0) + continue; - if (!CheckFinalTx(*pcoin->tx)) + int nDepth = pcoin->GetDepthInMainChain(); + if (nDepth < 0) + continue; + + // We should not consider coins which aren't at least in our mempool + // It's possible for these to be conflicted via ancestors which we may never be able to detect + if (nDepth == 0 && !pcoin->InMempool()) + continue; + + bool safeTx = pcoin->IsTrusted(); + + // We should not consider coins from transactions that are replacing + // other transactions. + // + // Example: There is a transaction A which is replaced by bumpfee + // transaction B. In this case, we want to prevent creation of + // a transaction B' which spends an output of B. + // + // Reason: If transaction A were initially confirmed, transactions B + // and B' would no longer be valid, so the user would have to create + // a new transaction C to replace B'. However, in the case of a + // one-block reorg, transactions B' and C might BOTH be accepted, + // when the user only wanted one of them. Specifically, there could + // be a 1-block reorg away from the chain where transactions A and C + // were accepted to another chain where B, B', and C were all + // accepted. + if (nDepth == 0 && pcoin->mapValue.count("replaces_txid")) { + safeTx = false; + } + + // Similarly, we should not consider coins from transactions that + // have been replaced. In the example above, we would want to prevent + // creation of a transaction A' spending an output of A, because if + // transaction B were initially confirmed, conflicting with A and + // A', we wouldn't want to the user to create a transaction D + // intending to replace A', but potentially resulting in a scenario + // where A, A', and D could all be accepted (instead of just B and + // D, or just A and A' like the user would want). + if (nDepth == 0 && pcoin->mapValue.count("replaced_by_txid")) { + safeTx = false; + } + + if (fOnlySafe && !safeTx) { + continue; + } + + if (nDepth < nMinDepth || nDepth > nMaxDepth) + continue; + + for (unsigned int i = 0; i < pcoin->tx->vout.size(); i++) { + if (pcoin->tx->vout[i].nValue < nMinimumAmount || pcoin->tx->vout[i].nValue > nMaximumAmount) continue; - if (pcoin->IsCoinBase() && pcoin->GetBlocksToMaturity() > 0) + if (coinControl && coinControl->HasSelected() && !coinControl->fAllowOtherInputs && !coinControl->IsSelected(COutPoint(entry.first, i))) continue; - int nDepth = pcoin->GetDepthInMainChain(); - if (nDepth < 0) + if (IsLockedCoin(entry.first, i)) continue; - // We should not consider coins which aren't at least in our mempool - // It's possible for these to be conflicted via ancestors which we may never be able to detect - if (nDepth == 0 && !pcoin->InMempool()) + if (IsSpent(wtxid, i)) continue; - bool safeTx = pcoin->IsTrusted(); + isminetype mine = IsMine(pcoin->tx->vout[i]); - // We should not consider coins from transactions that are replacing - // other transactions. - // - // Example: There is a transaction A which is replaced by bumpfee - // transaction B. In this case, we want to prevent creation of - // a transaction B' which spends an output of B. - // - // Reason: If transaction A were initially confirmed, transactions B - // and B' would no longer be valid, so the user would have to create - // a new transaction C to replace B'. However, in the case of a - // one-block reorg, transactions B' and C might BOTH be accepted, - // when the user only wanted one of them. Specifically, there could - // be a 1-block reorg away from the chain where transactions A and C - // were accepted to another chain where B, B', and C were all - // accepted. - if (nDepth == 0 && pcoin->mapValue.count("replaces_txid")) { - safeTx = false; - } - - // Similarly, we should not consider coins from transactions that - // have been replaced. In the example above, we would want to prevent - // creation of a transaction A' spending an output of A, because if - // transaction B were initially confirmed, conflicting with A and - // A', we wouldn't want to the user to create a transaction D - // intending to replace A', but potentially resulting in a scenario - // where A, A', and D could all be accepted (instead of just B and - // D, or just A and A' like the user would want). - if (nDepth == 0 && pcoin->mapValue.count("replaced_by_txid")) { - safeTx = false; - } - - if (fOnlySafe && !safeTx) { + if (mine == ISMINE_NO) { continue; } - if (nDepth < nMinDepth || nDepth > nMaxDepth) - continue; + bool fSpendableIn = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (coinControl && coinControl->fAllowWatchOnly && (mine & ISMINE_WATCH_SOLVABLE) != ISMINE_NO); + bool fSolvableIn = (mine & (ISMINE_SPENDABLE | ISMINE_WATCH_SOLVABLE)) != ISMINE_NO; - for (unsigned int i = 0; i < pcoin->tx->vout.size(); i++) { - if (pcoin->tx->vout[i].nValue < nMinimumAmount || pcoin->tx->vout[i].nValue > nMaximumAmount) - continue; + vCoins.push_back(COutput(pcoin, i, nDepth, fSpendableIn, fSolvableIn, safeTx)); - if (coinControl && coinControl->HasSelected() && !coinControl->fAllowOtherInputs && !coinControl->IsSelected(COutPoint(entry.first, i))) - continue; + // Checks the sum amount of all UTXO's. + if (nMinimumSumAmount != MAX_MONEY) { + nTotal += pcoin->tx->vout[i].nValue; - if (IsLockedCoin(entry.first, i)) - continue; - - if (IsSpent(wtxid, i)) - continue; - - isminetype mine = IsMine(pcoin->tx->vout[i]); - - if (mine == ISMINE_NO) { - continue; - } - - bool fSpendableIn = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (coinControl && coinControl->fAllowWatchOnly && (mine & ISMINE_WATCH_SOLVABLE) != ISMINE_NO); - bool fSolvableIn = (mine & (ISMINE_SPENDABLE | ISMINE_WATCH_SOLVABLE)) != ISMINE_NO; - - vCoins.push_back(COutput(pcoin, i, nDepth, fSpendableIn, fSolvableIn, safeTx)); - - // Checks the sum amount of all UTXO's. - if (nMinimumSumAmount != MAX_MONEY) { - nTotal += pcoin->tx->vout[i].nValue; - - if (nTotal >= nMinimumSumAmount) { - return; - } - } - - // Checks the maximum number of UTXO's. - if (nMaximumCount > 0 && vCoins.size() >= nMaximumCount) { + if (nTotal >= nMinimumSumAmount) { return; } } + + // Checks the maximum number of UTXO's. + if (nMaximumCount > 0 && vCoins.size() >= nMaximumCount) { + return; + } } } } @@ -2320,11 +2318,11 @@ std::map> CWallet::ListCoins() const // avoid adding some extra complexity to the Qt code. std::map> result; - std::vector availableCoins; - AvailableCoins(availableCoins); LOCK2(cs_main, cs_wallet); + AvailableCoins(availableCoins); + for (auto& coin : availableCoins) { CTxDestination address; if (coin.fSpendable &&