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<const CWalletTx*, unsigned int> to CInputCoin (NicolasDorier)

Tree-SHA512: d24361fc514a0566bce1c3953d766dfe4fece79c549cb4db2600695a4ce08e85caa61b7717812618e523a2f2a1669877dad2752ed079e2ed2d27249f9bc8590e
This commit is contained in:
Wladimir J. van der Laan 2017-04-13 12:07:46 +02:00
commit 70f6f56e9d
No known key found for this signature in database
GPG Key ID: 74810B012346C9A6
5 changed files with 71 additions and 47 deletions

View File

@ -48,7 +48,7 @@ static void CoinSelection(benchmark::State& state)
addCoin(1000 * COIN, wallet, vCoins); addCoin(1000 * COIN, wallet, vCoins);
addCoin(3 * COIN, wallet, vCoins); addCoin(3 * COIN, wallet, vCoins);
std::set<std::pair<const CWalletTx*, unsigned int> > setCoinsRet; std::set<CInputCoin> setCoinsRet;
CAmount nValueRet; CAmount nValueRet;
bool success = wallet.SelectCoinsMinConf(1003 * COIN, 1, 6, 0, vCoins, setCoinsRet, nValueRet); bool success = wallet.SelectCoinsMinConf(1003 * COIN, 1, 6, 0, vCoins, setCoinsRet, nValueRet);
assert(success); assert(success);

View File

@ -23,14 +23,14 @@
int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *pWallet) int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *pWallet)
{ {
CMutableTransaction txNew(tx); CMutableTransaction txNew(tx);
std::vector<std::pair<const CWalletTx *, unsigned int>> vCoins; std::vector<CInputCoin> vCoins;
// Look up the inputs. We should have already checked that this transaction // Look up the inputs. We should have already checked that this transaction
// IsAllFromMe(ISMINE_SPENDABLE), so every input should already be in our // IsAllFromMe(ISMINE_SPENDABLE), so every input should already be in our
// wallet, with a valid index into the vout array. // wallet, with a valid index into the vout array.
for (auto& input : tx.vin) { for (auto& input : tx.vin) {
const auto mi = pWallet->mapWallet.find(input.prevout.hash); const auto mi = pWallet->mapWallet.find(input.prevout.hash);
assert(mi != pWallet->mapWallet.end() && input.prevout.n < mi->second.tx->vout.size()); 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)) { if (!pWallet->DummySignTx(txNew, vCoins)) {
// This should never happen, because IsAllFromMe(ISMINE_SPENDABLE) // This should never happen, because IsAllFromMe(ISMINE_SPENDABLE)

View File

@ -29,7 +29,7 @@ extern UniValue importmulti(const JSONRPCRequest& request);
std::vector<std::unique_ptr<CWalletTx>> wtxn; std::vector<std::unique_ptr<CWalletTx>> wtxn;
typedef std::set<std::pair<const CWalletTx*,unsigned int> > CoinSet; typedef std::set<CInputCoin> CoinSet;
BOOST_FIXTURE_TEST_SUITE(wallet_tests, WalletTestingSetup) BOOST_FIXTURE_TEST_SUITE(wallet_tests, WalletTestingSetup)

View File

@ -65,10 +65,10 @@ const uint256 CMerkleTx::ABANDON_HASH(uint256S("00000000000000000000000000000000
struct CompareValueOnly struct CompareValueOnly
{ {
bool operator()(const std::pair<CAmount, std::pair<const CWalletTx*, unsigned int> >& t1, bool operator()(const CInputCoin& t1,
const std::pair<CAmount, std::pair<const CWalletTx*, unsigned int> >& t2) const const CInputCoin& t2) const
{ {
return t1.first < t2.first; return t1.txout.nValue < t2.txout.nValue;
} }
}; };
@ -2061,7 +2061,7 @@ void CWallet::AvailableCoins(std::vector<COutput>& vCoins, bool fOnlySafe, const
} }
} }
static void ApproximateBestSubset(const std::vector<std::pair<CAmount, std::pair<const CWalletTx*,unsigned int> > >& vValue, const CAmount& nTotalLower, const CAmount& nTargetValue, static void ApproximateBestSubset(const std::vector<CInputCoin>& vValue, const CAmount& nTotalLower, const CAmount& nTargetValue,
std::vector<char>& vfBest, CAmount& nBest, int iterations = 1000) std::vector<char>& vfBest, CAmount& nBest, int iterations = 1000)
{ {
std::vector<char> vfIncluded; std::vector<char> vfIncluded;
@ -2088,7 +2088,7 @@ static void ApproximateBestSubset(const std::vector<std::pair<CAmount, std::pair
//the selection random. //the selection random.
if (nPass == 0 ? insecure_rand.rand32()&1 : !vfIncluded[i]) if (nPass == 0 ? insecure_rand.rand32()&1 : !vfIncluded[i])
{ {
nTotal += vValue[i].first; nTotal += vValue[i].txout.nValue;
vfIncluded[i] = true; vfIncluded[i] = true;
if (nTotal >= nTargetValue) if (nTotal >= nTargetValue)
{ {
@ -2098,7 +2098,7 @@ static void ApproximateBestSubset(const std::vector<std::pair<CAmount, std::pair
nBest = nTotal; nBest = nTotal;
vfBest = vfIncluded; vfBest = vfIncluded;
} }
nTotal -= vValue[i].first; nTotal -= vValue[i].txout.nValue;
vfIncluded[i] = false; vfIncluded[i] = false;
} }
} }
@ -2108,16 +2108,14 @@ static void ApproximateBestSubset(const std::vector<std::pair<CAmount, std::pair
} }
bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMine, const int nConfTheirs, const uint64_t nMaxAncestors, std::vector<COutput> vCoins, bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMine, const int nConfTheirs, const uint64_t nMaxAncestors, std::vector<COutput> vCoins,
std::set<std::pair<const CWalletTx*,unsigned int> >& setCoinsRet, CAmount& nValueRet) const std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet) const
{ {
setCoinsRet.clear(); setCoinsRet.clear();
nValueRet = 0; nValueRet = 0;
// List of values less than target // List of values less than target
std::pair<CAmount, std::pair<const CWalletTx*,unsigned int> > coinLowestLarger; boost::optional<CInputCoin> coinLowestLarger;
coinLowestLarger.first = std::numeric_limits<CAmount>::max(); std::vector<CInputCoin> vValue;
coinLowestLarger.second.first = NULL;
std::vector<std::pair<CAmount, std::pair<const CWalletTx*,unsigned int> > > vValue;
CAmount nTotalLower = 0; CAmount nTotalLower = 0;
random_shuffle(vCoins.begin(), vCoins.end(), GetRandInt); random_shuffle(vCoins.begin(), vCoins.end(), GetRandInt);
@ -2136,22 +2134,21 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin
continue; continue;
int i = output.i; int i = output.i;
CAmount n = pcoin->tx->vout[i].nValue;
std::pair<CAmount,std::pair<const CWalletTx*,unsigned int> > coin = std::make_pair(n,std::make_pair(pcoin, i)); CInputCoin coin = CInputCoin(pcoin, i);
if (n == nTargetValue) if (coin.txout.nValue == nTargetValue)
{ {
setCoinsRet.insert(coin.second); setCoinsRet.insert(coin);
nValueRet += coin.first; nValueRet += coin.txout.nValue;
return true; return true;
} }
else if (n < nTargetValue + MIN_CHANGE) else if (coin.txout.nValue < nTargetValue + MIN_CHANGE)
{ {
vValue.push_back(coin); 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; coinLowestLarger = coin;
} }
@ -2161,18 +2158,18 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin
{ {
for (unsigned int i = 0; i < vValue.size(); ++i) for (unsigned int i = 0; i < vValue.size(); ++i)
{ {
setCoinsRet.insert(vValue[i].second); setCoinsRet.insert(vValue[i]);
nValueRet += vValue[i].first; nValueRet += vValue[i].txout.nValue;
} }
return true; return true;
} }
if (nTotalLower < nTargetValue) if (nTotalLower < nTargetValue)
{ {
if (coinLowestLarger.second.first == NULL) if (!coinLowestLarger)
return false; return false;
setCoinsRet.insert(coinLowestLarger.second); setCoinsRet.insert(coinLowestLarger.get());
nValueRet += coinLowestLarger.first; nValueRet += coinLowestLarger->txout.nValue;
return true; return true;
} }
@ -2188,25 +2185,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, // 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 // or the next bigger coin is closer), return the bigger coin
if (coinLowestLarger.second.first && if (coinLowestLarger &&
((nBest != nTargetValue && nBest < nTargetValue + MIN_CHANGE) || coinLowestLarger.first <= nBest)) ((nBest != nTargetValue && nBest < nTargetValue + MIN_CHANGE) || coinLowestLarger->txout.nValue <= nBest))
{ {
setCoinsRet.insert(coinLowestLarger.second); setCoinsRet.insert(coinLowestLarger.get());
nValueRet += coinLowestLarger.first; nValueRet += coinLowestLarger->txout.nValue;
} }
else { else {
for (unsigned int i = 0; i < vValue.size(); i++) for (unsigned int i = 0; i < vValue.size(); i++)
if (vfBest[i]) if (vfBest[i])
{ {
setCoinsRet.insert(vValue[i].second); setCoinsRet.insert(vValue[i]);
nValueRet += vValue[i].first; nValueRet += vValue[i].txout.nValue;
} }
if (LogAcceptCategory(BCLog::SELECTCOINS)) { if (LogAcceptCategory(BCLog::SELECTCOINS)) {
LogPrint(BCLog::SELECTCOINS, "SelectCoins() best subset: "); LogPrint(BCLog::SELECTCOINS, "SelectCoins() best subset: ");
for (unsigned int i = 0; i < vValue.size(); i++) { for (unsigned int i = 0; i < vValue.size(); i++) {
if (vfBest[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)); LogPrint(BCLog::SELECTCOINS, "total %s\n", FormatMoney(nBest));
@ -2216,7 +2213,7 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin
return true; return true;
} }
bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAmount& nTargetValue, std::set<std::pair<const CWalletTx*,unsigned int> >& setCoinsRet, CAmount& nValueRet, const CCoinControl* coinControl) const bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAmount& nTargetValue, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CCoinControl* coinControl) const
{ {
std::vector<COutput> vCoins(vAvailableCoins); std::vector<COutput> vCoins(vAvailableCoins);
@ -2228,13 +2225,13 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
if (!out.fSpendable) if (!out.fSpendable)
continue; continue;
nValueRet += out.tx->tx->vout[out.i].nValue; 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); return (nValueRet >= nTargetValue);
} }
// calculate value from preset inputs and store them // calculate value from preset inputs and store them
std::set<std::pair<const CWalletTx*, uint32_t> > setPresetCoins; std::set<CInputCoin> setPresetCoins;
CAmount nValueFromPresetInputs = 0; CAmount nValueFromPresetInputs = 0;
std::vector<COutPoint> vPresetInputs; std::vector<COutPoint> vPresetInputs;
@ -2250,7 +2247,7 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
if (pcoin->tx->vout.size() <= outpoint.n) if (pcoin->tx->vout.size() <= outpoint.n)
return false; return false;
nValueFromPresetInputs += pcoin->tx->vout[outpoint.n].nValue; nValueFromPresetInputs += pcoin->tx->vout[outpoint.n].nValue;
setPresetCoins.insert(std::make_pair(pcoin, outpoint.n)); setPresetCoins.insert(CInputCoin(pcoin, outpoint.n));
} else } else
return false; // TODO: Allow non-wallet inputs return false; // TODO: Allow non-wallet inputs
} }
@ -2258,7 +2255,7 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
// remove preset inputs from vCoins // remove preset inputs from vCoins
for (std::vector<COutput>::iterator it = vCoins.begin(); it != vCoins.end() && coinControl && coinControl->HasSelected();) for (std::vector<COutput>::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); it = vCoins.erase(it);
else else
++it; ++it;
@ -2424,7 +2421,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
assert(txNew.nLockTime < LOCKTIME_THRESHOLD); assert(txNew.nLockTime < LOCKTIME_THRESHOLD);
{ {
std::set<std::pair<const CWalletTx*,unsigned int> > setCoins; std::set<CInputCoin> setCoins;
LOCK2(cs_main, cs_wallet); LOCK2(cs_main, cs_wallet);
{ {
std::vector<COutput> vAvailableCoins; std::vector<COutput> vAvailableCoins;
@ -2583,7 +2580,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
// behavior." // behavior."
bool rbf = coinControl ? coinControl->signalRbf : fWalletRbf; bool rbf = coinControl ? coinControl->signalRbf : fWalletRbf;
for (const auto& coin : setCoins) 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<unsigned int>::max() - (rbf ? 2 : 1))); std::numeric_limits<unsigned int>::max() - (rbf ? 2 : 1)));
// Fill in dummy signatures for fee calculation. // Fill in dummy signatures for fee calculation.
@ -2666,10 +2663,10 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
int nIn = 0; int nIn = 0;
for (const auto& coin : setCoins) for (const auto& coin : setCoins)
{ {
const CScript& scriptPubKey = coin.first->tx->vout[coin.second].scriptPubKey; const CScript& scriptPubKey = coin.txout.scriptPubKey;
SignatureData sigdata; 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"); strFailReason = _("Signing transaction failed");
return false; return false;

View File

@ -473,7 +473,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 class COutput
{ {
@ -630,7 +657,7 @@ private:
* all coins from coinControl are selected; Never select unconfirmed coins * all coins from coinControl are selected; Never select unconfirmed coins
* if they are not ours * if they are not ours
*/ */
bool SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAmount& nTargetValue, std::set<std::pair<const CWalletTx*,unsigned int> >& setCoinsRet, CAmount& nValueRet, const CCoinControl *coinControl = NULL) const; bool SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAmount& nTargetValue, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CCoinControl *coinControl = NULL) const;
CWalletDB *pwalletdbEncryption; CWalletDB *pwalletdbEncryption;
@ -782,7 +809,7 @@ public:
* completion the coin set and corresponding actual target value is * completion the coin set and corresponding actual target value is
* assembled * assembled
*/ */
bool SelectCoinsMinConf(const CAmount& nTargetValue, int nConfMine, int nConfTheirs, uint64_t nMaxAncestors, std::vector<COutput> vCoins, std::set<std::pair<const CWalletTx*,unsigned int> >& setCoinsRet, CAmount& nValueRet) const; bool SelectCoinsMinConf(const CAmount& nTargetValue, int nConfMine, int nConfTheirs, uint64_t nMaxAncestors, std::vector<COutput> vCoins, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet) const;
bool IsSpent(const uint256& hash, unsigned int n) const; bool IsSpent(const uint256& hash, unsigned int n) const;
@ -1131,7 +1158,7 @@ bool CWallet::DummySignTx(CMutableTransaction &txNew, const ContainerType &coins
int nIn = 0; int nIn = 0;
for (const auto& coin : coins) for (const auto& coin : coins)
{ {
const CScript& scriptPubKey = coin.first->tx->vout[coin.second].scriptPubKey; const CScript& scriptPubKey = coin.txout.scriptPubKey;
SignatureData sigdata; SignatureData sigdata;
if (!ProduceSignature(DummySignatureCreator(this), scriptPubKey, sigdata)) if (!ProduceSignature(DummySignatureCreator(this), scriptPubKey, sigdata))