From 69b42ce6711c1a9ea35c69d89aaead02ff0336a8 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 13 Apr 2017 12:07:46 +0200 Subject: [PATCH] Merge #10165: [Wallet] Refactoring by using CInputCoin instead of std::pair c37e32a [Wallet] Prevent CInputCoin to be in a null state (NicolasDorier) f597dcb [Wallet] Simplify code using CInputCoin (NicolasDorier) e78bc45 [Wallet] Decouple CInputCoin from CWalletTx (NicolasDorier) fd44ac1 [Wallet] Rename std::pair to CInputCoin (NicolasDorier) Tree-SHA512: d24361fc514a0566bce1c3953d766dfe4fece79c549cb4db2600695a4ce08e85caa61b7717812618e523a2f2a1669877dad2752ed079e2ed2d27249f9bc8590e fix error, based on 1 0 1 6 5 fix error, based on 1 0 1 6 5 --- src/bench/coin_selection.cpp | 2 +- src/wallet/test/wallet_tests.cpp | 2 +- src/wallet/wallet.cpp | 85 +++++++++++++++----------------- src/wallet/wallet.h | 31 +++++++++++- 4 files changed, 71 insertions(+), 49 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index e866afe40..ec6e7a2c1 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -48,7 +48,7 @@ static void CoinSelection(benchmark::State& state) addCoin(1000 * COIN, wallet, vCoins); addCoin(3 * COIN, wallet, vCoins); - std::set > setCoinsRet; + std::set setCoinsRet; CAmount nValueRet; bool success = wallet.SelectCoinsMinConf(1003 * COIN, 1, 6, 0, vCoins, setCoinsRet, nValueRet); assert(success); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index c23614eef..140d4e369 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -31,7 +31,7 @@ extern UniValue importwallet(const JSONRPCRequest& request); std::vector> wtxn; -typedef std::set > CoinSet; +typedef std::set CoinSet; BOOST_FIXTURE_TEST_SUITE(wallet_tests, WalletTestingSetup) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c5e918fb3..d240c90f8 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -76,10 +76,10 @@ const uint256 CMerkleTx::ABANDON_HASH(uint256S("00000000000000000000000000000000 struct CompareValueOnly { - bool operator()(const std::pair >& t1, - const std::pair >& t2) const + bool operator()(const CInputCoin& t1, + const CInputCoin& t2) const { - return t1.first < t2.first; + return t1.txout.nValue < t2.txout.nValue; } }; @@ -2601,7 +2601,7 @@ void CWallet::AvailableCoins(std::vector& vCoins, bool fOnlySafe, const } } -static void ApproximateBestSubset(const std::vector > >& vValue, const CAmount& nTotalLower, const CAmount& nTargetValue, +static void ApproximateBestSubset(const std::vector& vValue, const CAmount& nTotalLower, const CAmount& nTargetValue, std::vector& vfBest, CAmount& nBest, bool fUseInstantSend = false, int iterations = 1000) { std::vector vfIncluded; @@ -2626,7 +2626,7 @@ static void ApproximateBestSubset(const std::vector sporkManager.GetSporkValue(SPORK_5_INSTANTSEND_MAX_VALUE)*COIN) { + if (fUseInstantSend && nTotal + vValue[i].txout.nValue > sporkManager.GetSporkValue(SPORK_5_INSTANTSEND_MAX_VALUE)*COIN) { continue; } //The solver here uses a randomized algorithm, @@ -2637,7 +2637,7 @@ static void ApproximateBestSubset(const std::vector= nTargetValue) { @@ -2647,7 +2647,7 @@ static void ApproximateBestSubset(const std::vector vCoins, - std::set >& setCoinsRet, CAmount& nValueRet, AvailableCoinsType nCoinType, bool fUseInstantSend) const + std::set& setCoinsRet, CAmount& nValueRet, AvailableCoinsType nCoinType, bool fUseInstantSend) const { setCoinsRet.clear(); nValueRet = 0; @@ -2694,12 +2694,8 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin } // List of values less than target - std::pair > coinLowestLarger; - coinLowestLarger.first = fUseInstantSend - ? sporkManager.GetSporkValue(SPORK_5_INSTANTSEND_MAX_VALUE)*COIN - : std::numeric_limits::max(); - coinLowestLarger.second.first = NULL; - std::vector > > vValue; + boost::optional coinLowestLarger; + std::vector vValue; CAmount nTotalLower = 0; random_shuffle(vCoins.begin(), vCoins.end(), GetRandInt); @@ -2741,8 +2737,10 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin continue; int i = output.i; - CAmount n = pcoin->tx->vout[i].nValue; - if (tryDenom == 0 && CPrivateSend::IsDenominatedAmount(n)) continue; // we don't want denom values on first run + + CInputCoin coin = CInputCoin(pcoin, i); + + if (tryDenom == 0 && CPrivateSend::IsDenominatedAmount(coin.txout.nValue)) continue; // we don't want denom values on first run if (nCoinType == ONLY_DENOMINATED) { // Make sure it's actually anonymized @@ -2751,20 +2749,18 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin if (nRounds < privateSendClient.nPrivateSendRounds) continue; } - std::pair > coin = std::make_pair(n, std::make_pair(pcoin, i)); - - if (n == nTargetValue) + if (coin.txout.nValue == nTargetValue) { - setCoinsRet.insert(coin.second); - nValueRet += coin.first; + setCoinsRet.insert(coin); + nValueRet += coin.txout.nValue; return true; } - else if (n < nTargetValue + nMinChange) + else if (coin.txout.nValue < nTargetValue + nMinChange) { vValue.push_back(coin); - nTotalLower += n; + nTotalLower += coin.txout.nValue; } - else if (n < coinLowestLarger.first) + else if (!coinLowestLarger || coin.txout.nValue < coinLowestLarger->txout.nValue) { coinLowestLarger = coin; } @@ -2774,15 +2770,15 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin { for (unsigned int i = 0; i < vValue.size(); ++i) { - setCoinsRet.insert(vValue[i].second); - nValueRet += vValue[i].first; + setCoinsRet.insert(vValue[i]); + nValueRet += vValue[i].txout.nValue; } return true; } if (nTotalLower < nTargetValue) { - if (coinLowestLarger.second.first == NULL) // there is no input larger than nTargetValue + if (!coinLowestLarger) // there is no input larger than nTargetValue { if (tryDenom == 0) // we didn't look at denom yet, let's do it @@ -2791,8 +2787,8 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin // we looked at everything possible and didn't find anything, no luck return false; } - setCoinsRet.insert(coinLowestLarger.second); - nValueRet += coinLowestLarger.first; + setCoinsRet.insert(coinLowestLarger.get()); + nValueRet += coinLowestLarger->txout.nValue; // There is no change in PS, so we know the fee beforehand, // can see if we exceeded the max fee and thus fail quickly. return nCoinType == ONLY_DENOMINATED ? (nValueRet - nTargetValue <= maxTxFee) : true; @@ -2800,7 +2796,6 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin // nTotalLower > nTargetValue break; - } // Solve subset sum by stochastic approximation @@ -2815,11 +2810,11 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin // If we have a bigger coin and (either the stochastic approximation didn't find a good solution, // or the next bigger coin is closer), return the bigger coin - if (coinLowestLarger.second.first && - ((nBest != nTargetValue && nBest < nTargetValue + nMinChange) || coinLowestLarger.first <= nBest)) + if (coinLowestLarger && + ((nBest != nTargetValue && nBest < nTargetValue + nMinChange) || coinLowestLarger->txout.nValue <= nBest)) { - setCoinsRet.insert(coinLowestLarger.second); - nValueRet += coinLowestLarger.first; + setCoinsRet.insert(coinLowestLarger.get()); + nValueRet += coinLowestLarger->txout.nValue; } else { std::string s = "CWallet::SelectCoinsMinConf best subset: "; @@ -2827,9 +2822,9 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin { if (vfBest[i]) { - setCoinsRet.insert(vValue[i].second); - nValueRet += vValue[i].first; - s += FormatMoney(vValue[i].first) + " "; + setCoinsRet.insert(vValue[i]); + nValueRet += vValue[i].txout.nValue; + s += FormatMoney(vValue[i].txout.nValue) + " "; } } LogPrint(BCLog::SELECTCOINS, "%s - total %s\n", s, FormatMoney(nBest)); @@ -2840,7 +2835,7 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin return nCoinType == ONLY_DENOMINATED ? (nValueRet - nTargetValue <= maxTxFee) : true; } -bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAmount& nTargetValue, std::set >& setCoinsRet, CAmount& nValueRet, const CCoinControl* coinControl, AvailableCoinsType nCoinType, bool fUseInstantSend) const +bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAmount& nTargetValue, std::set& setCoinsRet, CAmount& nValueRet, const CCoinControl* coinControl, AvailableCoinsType nCoinType, bool fUseInstantSend) const { // Note: this function should never be used for "always free" tx types like dstx @@ -2861,7 +2856,7 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm if(nRounds < privateSendClient.nPrivateSendRounds) continue; } nValueRet += out.tx->tx->vout[out.i].nValue; - setCoinsRet.insert(std::make_pair(out.tx, out.i)); + setCoinsRet.insert(CInputCoin(out.tx, out.i)); if (!coinControl->fRequireAllInputs && nValueRet >= nTargetValue) { // stop when we added at least one input and enough inputs to have at least nTargetValue funds @@ -2873,7 +2868,7 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm } // calculate value from preset inputs and store them - std::set > setPresetCoins; + std::set setPresetCoins; CAmount nValueFromPresetInputs = 0; std::vector vPresetInputs; @@ -2895,7 +2890,7 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm if (nRounds < privateSendClient.nPrivateSendRounds) continue; } nValueFromPresetInputs += pcoin->tx->vout[outpoint.n].nValue; - setPresetCoins.insert(std::make_pair(pcoin, outpoint.n)); + setPresetCoins.insert(CInputCoin(pcoin, outpoint.n)); } else return false; // TODO: Allow non-wallet inputs } @@ -2903,7 +2898,7 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm // remove preset inputs from vCoins for (std::vector::iterator it = vCoins.begin(); it != vCoins.end() && coinControl && coinControl->HasSelected();) { - if (setPresetCoins.count(std::make_pair(it->tx, it->i))) + if (setPresetCoins.count(CInputCoin(it->tx, it->i))) it = vCoins.erase(it); else ++it; @@ -3435,7 +3430,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT assert(txNew.nLockTime < LOCKTIME_THRESHOLD); { - std::set > setCoins; + std::set setCoins; std::vector vecTxDSInTmp; LOCK2(cs_main, cs_wallet); { @@ -3617,9 +3612,9 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT // nLockTime set above actually works. vecTxDSInTmp.clear(); for (const auto& coin : setCoins) { - CTxIn txin = CTxIn(coin.first->GetHash(),coin.second,CScript(), + CTxIn txin = CTxIn(coin.outpoint,CScript(), std::numeric_limits::max()-1); - vecTxDSInTmp.push_back(CTxDSIn(txin, coin.first->tx->vout[coin.second].scriptPubKey)); + vecTxDSInTmp.push_back(CTxDSIn(txin, coin.txout.scriptPubKey)); txNew.vin.push_back(txin); } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 8d1654f41..aee2961a4 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -518,7 +518,34 @@ public: }; +class CInputCoin { +public: + CInputCoin(const CWalletTx* walletTx, unsigned int i) + { + if (!walletTx) + throw std::invalid_argument("walletTx should not be null"); + if (i >= walletTx->tx->vout.size()) + throw std::out_of_range("The output index is out of range"); + outpoint = COutPoint(walletTx->GetHash(), i); + txout = walletTx->tx->vout[i]; + } + + COutPoint outpoint; + CTxOut txout; + + bool operator<(const CInputCoin& rhs) const { + return outpoint < rhs.outpoint; + } + + bool operator!=(const CInputCoin& rhs) const { + return outpoint != rhs.outpoint; + } + + bool operator==(const CInputCoin& rhs) const { + return outpoint == rhs.outpoint; + } +}; class COutput { @@ -680,7 +707,7 @@ private: * all coins from coinControl are selected; Never select unconfirmed coins * if they are not ours */ - bool SelectCoins(const std::vector& vAvailableCoins, const CAmount& nTargetValue, std::set >& setCoinsRet, CAmount& nValueRet, const CCoinControl *coinControl = NULL, AvailableCoinsType nCoinType=ALL_COINS, bool fUseInstantSend = true) const; + bool SelectCoins(const std::vector& vAvailableCoins, const CAmount& nTargetValue, std::set& setCoinsRet, CAmount& nValueRet, const CCoinControl *coinControl = NULL, AvailableCoinsType nCoinType=ALL_COINS, bool fUseInstantSend = true) const; CWalletDB *pwalletdbEncryption; @@ -869,7 +896,7 @@ public: * completion the coin set and corresponding actual target value is * assembled */ - bool SelectCoinsMinConf(const CAmount& nTargetValue, int nConfMine, int nConfTheirs, uint64_t nMaxAncestors, std::vector vCoins, std::set >& setCoinsRet, CAmount& nValueRet, AvailableCoinsType nCoinType=ALL_COINS, bool fUseInstantSend = false) const; + bool SelectCoinsMinConf(const CAmount& nTargetValue, int nConfMine, int nConfTheirs, uint64_t nMaxAncestors, std::vector vCoins, std::set& setCoinsRet, CAmount& nValueRet, AvailableCoinsType nCoinType=ALL_COINS, bool fUseInstantSend = false) const; // Coin selection bool SelectPSInOutPairsByDenominations(int nDenom, CAmount nValueMin, CAmount nValueMax, std::vector< std::pair >& vecPSInOutPairsRet);