From fd44ac1e8b75f6f83cc0fea20ae721de163ff9cc Mon Sep 17 00:00:00 2001 From: NicolasDorier Date: Fri, 7 Apr 2017 09:38:33 +0000 Subject: [PATCH 1/4] [Wallet] Rename std::pair to CInputCoin --- src/bench/coin_selection.cpp | 2 +- src/wallet/feebumper.cpp | 2 +- src/wallet/test/wallet_tests.cpp | 2 +- src/wallet/wallet.cpp | 20 ++++++++++---------- src/wallet/wallet.h | 6 +++--- 5 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index 06882f151..42891f345 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/feebumper.cpp b/src/wallet/feebumper.cpp index fe3871a91..7201d17b0 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -23,7 +23,7 @@ int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *pWallet) { CMutableTransaction txNew(tx); - std::vector> vCoins; + std::vector vCoins; // Look up the inputs. We should have already checked that this transaction // IsAllFromMe(ISMINE_SPENDABLE), so every input should already be in our // wallet, with a valid index into the vout array. diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 67e5e9022..033536192 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -29,7 +29,7 @@ extern UniValue importmulti(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 d449f83a8..982293aca 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -65,8 +65,8 @@ const uint256 CMerkleTx::ABANDON_HASH(uint256S("00000000000000000000000000000000 struct CompareValueOnly { - bool operator()(const std::pair >& t1, - const std::pair >& t2) const + bool operator()(const std::pair& t1, + const std::pair& t2) const { return t1.first < t2.first; } @@ -2032,7 +2032,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, int iterations = 1000) { std::vector vfIncluded; @@ -2079,16 +2079,16 @@ static void ApproximateBestSubset(const std::vector vCoins, - std::set >& setCoinsRet, CAmount& nValueRet) const + std::set& setCoinsRet, CAmount& nValueRet) const { setCoinsRet.clear(); nValueRet = 0; // List of values less than target - std::pair > coinLowestLarger; + std::pair coinLowestLarger; coinLowestLarger.first = std::numeric_limits::max(); coinLowestLarger.second.first = NULL; - std::vector > > vValue; + std::vector > vValue; CAmount nTotalLower = 0; random_shuffle(vCoins.begin(), vCoins.end(), GetRandInt); @@ -2109,7 +2109,7 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin int i = output.i; CAmount n = pcoin->tx->vout[i].nValue; - std::pair > coin = std::make_pair(n,std::make_pair(pcoin, i)); + std::pair coin = std::make_pair(n,std::make_pair(pcoin, i)); if (n == nTargetValue) { @@ -2187,7 +2187,7 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin return true; } -bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAmount& nTargetValue, std::set >& setCoinsRet, CAmount& nValueRet, const CCoinControl* coinControl) const +bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAmount& nTargetValue, std::set& setCoinsRet, CAmount& nValueRet, const CCoinControl* coinControl) const { std::vector vCoins(vAvailableCoins); @@ -2205,7 +2205,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; @@ -2395,7 +2395,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT assert(txNew.nLockTime < LOCKTIME_THRESHOLD); { - std::set > setCoins; + std::set setCoins; LOCK2(cs_main, cs_wallet); { std::vector vAvailableCoins; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index c714ddd09..a882cec77 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -475,7 +475,7 @@ public: }; - +using CInputCoin = std::pair; class COutput { @@ -632,7 +632,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) const; + bool SelectCoins(const std::vector& vAvailableCoins, const CAmount& nTargetValue, std::set& setCoinsRet, CAmount& nValueRet, const CCoinControl *coinControl = NULL) const; CWalletDB *pwalletdbEncryption; @@ -780,7 +780,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) const; + bool SelectCoinsMinConf(const CAmount& nTargetValue, int nConfMine, int nConfTheirs, uint64_t nMaxAncestors, std::vector vCoins, std::set& setCoinsRet, CAmount& nValueRet) const; bool IsSpent(const uint256& hash, unsigned int n) const; From e78bc45810c0834ad47cffcdfb52f0a57bfbba2d Mon Sep 17 00:00:00 2001 From: NicolasDorier Date: Fri, 7 Apr 2017 09:57:06 +0000 Subject: [PATCH 2/4] [Wallet] Decouple CInputCoin from CWalletTx --- src/wallet/feebumper.cpp | 2 +- src/wallet/wallet.cpp | 19 +++++++++---------- src/wallet/wallet.h | 35 +++++++++++++++++++++++++++++++++-- 3 files changed, 43 insertions(+), 13 deletions(-) diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index 7201d17b0..6b030935f 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -30,7 +30,7 @@ int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *pWal for (auto& input : tx.vin) { const auto mi = pWallet->mapWallet.find(input.prevout.hash); assert(mi != pWallet->mapWallet.end() && input.prevout.n < mi->second.tx->vout.size()); - vCoins.emplace_back(&(mi->second), input.prevout.n); + vCoins.emplace_back(CInputCoin(&(mi->second), input.prevout.n)); } if (!pWallet->DummySignTx(txNew, vCoins)) { // This should never happen, because IsAllFromMe(ISMINE_SPENDABLE) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 982293aca..2b12ece8d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2087,7 +2087,6 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin // List of values less than target std::pair coinLowestLarger; coinLowestLarger.first = std::numeric_limits::max(); - coinLowestLarger.second.first = NULL; std::vector > vValue; CAmount nTotalLower = 0; @@ -2109,7 +2108,7 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin int i = output.i; CAmount n = pcoin->tx->vout[i].nValue; - std::pair coin = std::make_pair(n,std::make_pair(pcoin, i)); + std::pair coin = std::make_pair(n, CInputCoin(pcoin, i)); if (n == nTargetValue) { @@ -2140,7 +2139,7 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin if (nTotalLower < nTargetValue) { - if (coinLowestLarger.second.first == NULL) + if (coinLowestLarger.second.IsNull()) return false; setCoinsRet.insert(coinLowestLarger.second); nValueRet += coinLowestLarger.first; @@ -2159,7 +2158,7 @@ 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 && + if (coinLowestLarger.second.IsNull() && ((nBest != nTargetValue && nBest < nTargetValue + MIN_CHANGE) || coinLowestLarger.first <= nBest)) { setCoinsRet.insert(coinLowestLarger.second); @@ -2199,7 +2198,7 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm if (!out.fSpendable) 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)); } return (nValueRet >= nTargetValue); } @@ -2221,7 +2220,7 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm if (pcoin->tx->vout.size() <= outpoint.n) return false; 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 } @@ -2229,7 +2228,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; @@ -2554,7 +2553,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT // behavior." bool rbf = coinControl ? coinControl->signalRbf : fWalletRbf; for (const auto& coin : setCoins) - txNew.vin.push_back(CTxIn(coin.first->GetHash(),coin.second,CScript(), + txNew.vin.push_back(CTxIn(coin.outpoint,CScript(), std::numeric_limits::max() - (rbf ? 2 : 1))); // Fill in dummy signatures for fee calculation. @@ -2637,10 +2636,10 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT int nIn = 0; for (const auto& coin : setCoins) { - const CScript& scriptPubKey = coin.first->tx->vout[coin.second].scriptPubKey; + const CScript& scriptPubKey = coin.txout.scriptPubKey; SignatureData sigdata; - if (!ProduceSignature(TransactionSignatureCreator(this, &txNewConst, nIn, coin.first->tx->vout[coin.second].nValue, SIGHASH_ALL), scriptPubKey, sigdata)) + if (!ProduceSignature(TransactionSignatureCreator(this, &txNewConst, nIn, coin.txout.nValue, SIGHASH_ALL), scriptPubKey, sigdata)) { strFailReason = _("Signing transaction failed"); return false; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index a882cec77..d43bd4b59 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -475,7 +475,38 @@ public: }; -using CInputCoin = std::pair; +class CInputCoin { +public: + CInputCoin() + { + } + CInputCoin(const CWalletTx* walletTx, unsigned int i) + { + if (walletTx != nullptr && i < walletTx->tx->vout.size()) + { + outpoint = COutPoint(walletTx->GetHash(), i); + txout = walletTx->tx->vout[i]; + } + } + bool IsNull() const + { + return outpoint.IsNull() && txout.IsNull(); + } + 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 { @@ -1132,7 +1163,7 @@ bool CWallet::DummySignTx(CMutableTransaction &txNew, const ContainerType &coins int nIn = 0; for (const auto& coin : coins) { - const CScript& scriptPubKey = coin.first->tx->vout[coin.second].scriptPubKey; + const CScript& scriptPubKey = coin.txout.scriptPubKey; SignatureData sigdata; if (!ProduceSignature(DummySignatureCreator(this), scriptPubKey, sigdata)) From f597dcb7c875b51abe9a4cfdcb22dd5099cdbf7f Mon Sep 17 00:00:00 2001 From: NicolasDorier Date: Fri, 7 Apr 2017 10:04:41 +0000 Subject: [PATCH 3/4] [Wallet] Simplify code using CInputCoin --- src/wallet/wallet.cpp | 56 +++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 29 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 2b12ece8d..540352c7d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -65,10 +65,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; } }; @@ -2032,7 +2032,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, int iterations = 1000) { std::vector vfIncluded; @@ -2059,7 +2059,7 @@ static void ApproximateBestSubset(const std::vector= nTargetValue) { @@ -2069,7 +2069,7 @@ static void ApproximateBestSubset(const std::vector coinLowestLarger; - coinLowestLarger.first = std::numeric_limits::max(); - std::vector > vValue; + CInputCoin coinLowestLarger; + std::vector vValue; CAmount nTotalLower = 0; random_shuffle(vCoins.begin(), vCoins.end(), GetRandInt); @@ -2106,22 +2105,21 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin continue; int i = output.i; - CAmount n = pcoin->tx->vout[i].nValue; - std::pair coin = std::make_pair(n, CInputCoin(pcoin, i)); + CInputCoin coin = CInputCoin(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 + MIN_CHANGE) + else if (coin.txout.nValue < nTargetValue + MIN_CHANGE) { vValue.push_back(coin); - nTotalLower += n; + nTotalLower += coin.txout.nValue; } - else if (n < coinLowestLarger.first) + else if (coinLowestLarger.IsNull() || coin.txout.nValue < coinLowestLarger.txout.nValue) { coinLowestLarger = coin; } @@ -2131,18 +2129,18 @@ 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.IsNull()) + if (coinLowestLarger.IsNull()) return false; - setCoinsRet.insert(coinLowestLarger.second); - nValueRet += coinLowestLarger.first; + setCoinsRet.insert(coinLowestLarger); + nValueRet += coinLowestLarger.txout.nValue; return true; } @@ -2158,25 +2156,25 @@ 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.IsNull() && - ((nBest != nTargetValue && nBest < nTargetValue + MIN_CHANGE) || coinLowestLarger.first <= nBest)) + if (!coinLowestLarger.IsNull() && + ((nBest != nTargetValue && nBest < nTargetValue + MIN_CHANGE) || coinLowestLarger.txout.nValue <= nBest)) { - setCoinsRet.insert(coinLowestLarger.second); - nValueRet += coinLowestLarger.first; + setCoinsRet.insert(coinLowestLarger); + nValueRet += coinLowestLarger.txout.nValue; } else { for (unsigned int i = 0; i < vValue.size(); i++) if (vfBest[i]) { - setCoinsRet.insert(vValue[i].second); - nValueRet += vValue[i].first; + setCoinsRet.insert(vValue[i]); + nValueRet += vValue[i].txout.nValue; } if (LogAcceptCategory(BCLog::SELECTCOINS)) { LogPrint(BCLog::SELECTCOINS, "SelectCoins() best subset: "); for (unsigned int i = 0; i < vValue.size(); i++) { if (vfBest[i]) { - LogPrint(BCLog::SELECTCOINS, "%s ", FormatMoney(vValue[i].first)); + LogPrint(BCLog::SELECTCOINS, "%s ", FormatMoney(vValue[i].txout.nValue)); } } LogPrint(BCLog::SELECTCOINS, "total %s\n", FormatMoney(nBest)); From c37e32af0d2f8723f89c5304d41a4a41d4d34ea3 Mon Sep 17 00:00:00 2001 From: NicolasDorier Date: Sat, 8 Apr 2017 03:41:27 +0000 Subject: [PATCH 4/4] [Wallet] Prevent CInputCoin to be in a null state --- src/wallet/wallet.cpp | 18 +++++++++--------- src/wallet/wallet.h | 20 ++++++++------------ 2 files changed, 17 insertions(+), 21 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 540352c7d..ff135fb05 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2085,7 +2085,7 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin nValueRet = 0; // List of values less than target - CInputCoin coinLowestLarger; + boost::optional coinLowestLarger; std::vector vValue; CAmount nTotalLower = 0; @@ -2119,7 +2119,7 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin vValue.push_back(coin); nTotalLower += coin.txout.nValue; } - else if (coinLowestLarger.IsNull() || coin.txout.nValue < coinLowestLarger.txout.nValue) + else if (!coinLowestLarger || coin.txout.nValue < coinLowestLarger->txout.nValue) { coinLowestLarger = coin; } @@ -2137,10 +2137,10 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin if (nTotalLower < nTargetValue) { - if (coinLowestLarger.IsNull()) + if (!coinLowestLarger) return false; - setCoinsRet.insert(coinLowestLarger); - nValueRet += coinLowestLarger.txout.nValue; + setCoinsRet.insert(coinLowestLarger.get()); + nValueRet += coinLowestLarger->txout.nValue; return true; } @@ -2156,11 +2156,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.IsNull() && - ((nBest != nTargetValue && nBest < nTargetValue + MIN_CHANGE) || coinLowestLarger.txout.nValue <= nBest)) + if (coinLowestLarger && + ((nBest != nTargetValue && nBest < nTargetValue + MIN_CHANGE) || coinLowestLarger->txout.nValue <= nBest)) { - setCoinsRet.insert(coinLowestLarger); - nValueRet += coinLowestLarger.txout.nValue; + setCoinsRet.insert(coinLowestLarger.get()); + nValueRet += coinLowestLarger->txout.nValue; } else { for (unsigned int i = 0; i < vValue.size(); i++) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index d43bd4b59..b95d3de52 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -477,21 +477,17 @@ public: class CInputCoin { public: - CInputCoin() - { - } CInputCoin(const CWalletTx* walletTx, unsigned int i) { - if (walletTx != nullptr && i < walletTx->tx->vout.size()) - { - outpoint = COutPoint(walletTx->GetHash(), i); - txout = walletTx->tx->vout[i]; - } - } - bool IsNull() const - { - return outpoint.IsNull() && txout.IsNull(); + 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;