Skip to content

Commit 70f6f56

Browse files
laanwjlaanwj
committed
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
2 parents c9ff4f8 + c37e32a commit 70f6f56

File tree

5 files changed

+71
-47
lines changed

5 files changed

+71
-47
lines changed

‎src/bench/coin_selection.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ static void CoinSelection(benchmark::State& state)
4848
addCoin(1000 * COIN, wallet, vCoins);
4949
addCoin(3 * COIN, wallet, vCoins);
5050

51-
std::set<std::pair<const CWalletTx*, unsigned int> > setCoinsRet;
51+
std::set<CInputCoin> setCoinsRet;
5252
CAmount nValueRet;
5353
bool success = wallet.SelectCoinsMinConf(1003 * COIN, 1, 6, 0, vCoins, setCoinsRet, nValueRet);
5454
assert(success);

‎src/wallet/feebumper.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,14 @@
2323
int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *pWallet)
2424
{
2525
CMutableTransaction txNew(tx);
26-
std::vector<std::pair<const CWalletTx *, unsigned int>> vCoins;
26+
std::vector<CInputCoin> vCoins;
2727
// Look up the inputs. We should have already checked that this transaction
2828
// IsAllFromMe(ISMINE_SPENDABLE), so every input should already be in our
2929
// wallet, with a valid index into the vout array.
3030
for (auto& input : tx.vin) {
3131
const auto mi = pWallet->mapWallet.find(input.prevout.hash);
3232
assert(mi != pWallet->mapWallet.end() && input.prevout.n < mi->second.tx->vout.size());
33-
vCoins.emplace_back(&(mi->second), input.prevout.n);
33+
vCoins.emplace_back(CInputCoin(&(mi->second), input.prevout.n));
3434
}
3535
if (!pWallet->DummySignTx(txNew, vCoins)) {
3636
// This should never happen, because IsAllFromMe(ISMINE_SPENDABLE)

‎src/wallet/test/wallet_tests.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ extern UniValue importmulti(const JSONRPCRequest& request);
2929

3030
std::vector<std::unique_ptr<CWalletTx>> wtxn;
3131

32-
typedef std::set<std::pair<const CWalletTx*,unsigned int> > CoinSet;
32+
typedef std::set<CInputCoin> CoinSet;
3333

3434
BOOST_FIXTURE_TEST_SUITE(wallet_tests, WalletTestingSetup)
3535

‎src/wallet/wallet.cpp

Lines changed: 37 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -65,10 +65,10 @@ const uint256 CMerkleTx::ABANDON_HASH(uint256S("00000000000000000000000000000000
6565

6666
struct CompareValueOnly
6767
{
68-
bool operator()(const std::pair<CAmount, std::pair<const CWalletTx*, unsigned int> >& t1,
69-
const std::pair<CAmount, std::pair<const CWalletTx*, unsigned int> >& t2) const
68+
bool operator()(const CInputCoin& t1,
69+
const CInputCoin& t2) const
7070
{
71-
return t1.first < t2.first;
71+
return t1.txout.nValue < t2.txout.nValue;
7272
}
7373
};
7474

@@ -2061,7 +2061,7 @@ void CWallet::AvailableCoins(std::vector<COutput>& vCoins, bool fOnlySafe, const
20612061
}
20622062
}
20632063

2064-
static void ApproximateBestSubset(const std::vector<std::pair<CAmount, std::pair<const CWalletTx*,unsigned int> > >& vValue, const CAmount& nTotalLower, const CAmount& nTargetValue,
2064+
static void ApproximateBestSubset(const std::vector<CInputCoin>& vValue, const CAmount& nTotalLower, const CAmount& nTargetValue,
20652065
std::vector<char>& vfBest, CAmount& nBest, int iterations = 1000)
20662066
{
20672067
std::vector<char> vfIncluded;
@@ -2088,7 +2088,7 @@ static void ApproximateBestSubset(const std::vector<std::pair<CAmount, std::pair
20882088
//the selection random.
20892089
if (nPass == 0 ? insecure_rand.rand32()&1 : !vfIncluded[i])
20902090
{
2091-
nTotal += vValue[i].first;
2091+
nTotal += vValue[i].txout.nValue;
20922092
vfIncluded[i] = true;
20932093
if (nTotal >= nTargetValue)
20942094
{
@@ -2098,7 +2098,7 @@ static void ApproximateBestSubset(const std::vector<std::pair<CAmount, std::pair
20982098
nBest = nTotal;
20992099
vfBest = vfIncluded;
21002100
}
2101-
nTotal -= vValue[i].first;
2101+
nTotal -= vValue[i].txout.nValue;
21022102
vfIncluded[i] = false;
21032103
}
21042104
}
@@ -2108,16 +2108,14 @@ static void ApproximateBestSubset(const std::vector<std::pair<CAmount, std::pair
21082108
}
21092109

21102110
bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMine, const int nConfTheirs, const uint64_t nMaxAncestors, std::vector<COutput> vCoins,
2111-
std::set<std::pair<const CWalletTx*,unsigned int> >& setCoinsRet, CAmount& nValueRet) const
2111+
std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet) const
21122112
{
21132113
setCoinsRet.clear();
21142114
nValueRet = 0;
21152115

21162116
// List of values less than target
2117-
std::pair<CAmount, std::pair<const CWalletTx*,unsigned int> > coinLowestLarger;
2118-
coinLowestLarger.first = std::numeric_limits<CAmount>::max();
2119-
coinLowestLarger.second.first = NULL;
2120-
std::vector<std::pair<CAmount, std::pair<const CWalletTx*,unsigned int> > > vValue;
2117+
boost::optional<CInputCoin> coinLowestLarger;
2118+
std::vector<CInputCoin> vValue;
21212119
CAmount nTotalLower = 0;
21222120

21232121
random_shuffle(vCoins.begin(), vCoins.end(), GetRandInt);
@@ -2136,22 +2134,21 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin
21362134
continue;
21372135

21382136
int i = output.i;
2139-
CAmount n = pcoin->tx->vout[i].nValue;
21402137

2141-
std::pair<CAmount,std::pair<const CWalletTx*,unsigned int> > coin = std::make_pair(n,std::make_pair(pcoin, i));
2138+
CInputCoin coin = CInputCoin(pcoin, i);
21422139

2143-
if (n == nTargetValue)
2140+
if (coin.txout.nValue == nTargetValue)
21442141
{
2145-
setCoinsRet.insert(coin.second);
2146-
nValueRet += coin.first;
2142+
setCoinsRet.insert(coin);
2143+
nValueRet += coin.txout.nValue;
21472144
return true;
21482145
}
2149-
else if (n < nTargetValue + MIN_CHANGE)
2146+
else if (coin.txout.nValue < nTargetValue + MIN_CHANGE)
21502147
{
21512148
vValue.push_back(coin);
2152-
nTotalLower += n;
2149+
nTotalLower += coin.txout.nValue;
21532150
}
2154-
else if (n < coinLowestLarger.first)
2151+
else if (!coinLowestLarger || coin.txout.nValue < coinLowestLarger->txout.nValue)
21552152
{
21562153
coinLowestLarger = coin;
21572154
}
@@ -2161,18 +2158,18 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin
21612158
{
21622159
for (unsigned int i = 0; i < vValue.size(); ++i)
21632160
{
2164-
setCoinsRet.insert(vValue[i].second);
2165-
nValueRet += vValue[i].first;
2161+
setCoinsRet.insert(vValue[i]);
2162+
nValueRet += vValue[i].txout.nValue;
21662163
}
21672164
return true;
21682165
}
21692166

21702167
if (nTotalLower < nTargetValue)
21712168
{
2172-
if (coinLowestLarger.second.first == NULL)
2169+
if (!coinLowestLarger)
21732170
return false;
2174-
setCoinsRet.insert(coinLowestLarger.second);
2175-
nValueRet += coinLowestLarger.first;
2171+
setCoinsRet.insert(coinLowestLarger.get());
2172+
nValueRet += coinLowestLarger->txout.nValue;
21762173
return true;
21772174
}
21782175

@@ -2188,25 +2185,25 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin
21882185

21892186
// If we have a bigger coin and (either the stochastic approximation didn't find a good solution,
21902187
// or the next bigger coin is closer), return the bigger coin
2191-
if (coinLowestLarger.second.first &&
2192-
((nBest != nTargetValue && nBest < nTargetValue + MIN_CHANGE) || coinLowestLarger.first <= nBest))
2188+
if (coinLowestLarger &&
2189+
((nBest != nTargetValue && nBest < nTargetValue + MIN_CHANGE) || coinLowestLarger->txout.nValue <= nBest))
21932190
{
2194-
setCoinsRet.insert(coinLowestLarger.second);
2195-
nValueRet += coinLowestLarger.first;
2191+
setCoinsRet.insert(coinLowestLarger.get());
2192+
nValueRet += coinLowestLarger->txout.nValue;
21962193
}
21972194
else {
21982195
for (unsigned int i = 0; i < vValue.size(); i++)
21992196
if (vfBest[i])
22002197
{
2201-
setCoinsRet.insert(vValue[i].second);
2202-
nValueRet += vValue[i].first;
2198+
setCoinsRet.insert(vValue[i]);
2199+
nValueRet += vValue[i].txout.nValue;
22032200
}
22042201

22052202
if (LogAcceptCategory(BCLog::SELECTCOINS)) {
22062203
LogPrint(BCLog::SELECTCOINS, "SelectCoins() best subset: ");
22072204
for (unsigned int i = 0; i < vValue.size(); i++) {
22082205
if (vfBest[i]) {
2209-
LogPrint(BCLog::SELECTCOINS, "%s ", FormatMoney(vValue[i].first));
2206+
LogPrint(BCLog::SELECTCOINS, "%s ", FormatMoney(vValue[i].txout.nValue));
22102207
}
22112208
}
22122209
LogPrint(BCLog::SELECTCOINS, "total %s\n", FormatMoney(nBest));
@@ -2216,7 +2213,7 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin
22162213
return true;
22172214
}
22182215

2219-
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
2216+
bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAmount& nTargetValue, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CCoinControl* coinControl) const
22202217
{
22212218
std::vector<COutput> vCoins(vAvailableCoins);
22222219

@@ -2228,13 +2225,13 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
22282225
if (!out.fSpendable)
22292226
continue;
22302227
nValueRet += out.tx->tx->vout[out.i].nValue;
2231-
setCoinsRet.insert(std::make_pair(out.tx, out.i));
2228+
setCoinsRet.insert(CInputCoin(out.tx, out.i));
22322229
}
22332230
return (nValueRet >= nTargetValue);
22342231
}
22352232

22362233
// calculate value from preset inputs and store them
2237-
std::set<std::pair<const CWalletTx*, uint32_t> > setPresetCoins;
2234+
std::set<CInputCoin> setPresetCoins;
22382235
CAmount nValueFromPresetInputs = 0;
22392236

22402237
std::vector<COutPoint> vPresetInputs;
@@ -2250,15 +2247,15 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
22502247
if (pcoin->tx->vout.size() <= outpoint.n)
22512248
return false;
22522249
nValueFromPresetInputs += pcoin->tx->vout[outpoint.n].nValue;
2253-
setPresetCoins.insert(std::make_pair(pcoin, outpoint.n));
2250+
setPresetCoins.insert(CInputCoin(pcoin, outpoint.n));
22542251
} else
22552252
return false; // TODO: Allow non-wallet inputs
22562253
}
22572254

22582255
// remove preset inputs from vCoins
22592256
for (std::vector<COutput>::iterator it = vCoins.begin(); it != vCoins.end() && coinControl && coinControl->HasSelected();)
22602257
{
2261-
if (setPresetCoins.count(std::make_pair(it->tx, it->i)))
2258+
if (setPresetCoins.count(CInputCoin(it->tx, it->i)))
22622259
it = vCoins.erase(it);
22632260
else
22642261
++it;
@@ -2424,7 +2421,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
24242421
assert(txNew.nLockTime < LOCKTIME_THRESHOLD);
24252422

24262423
{
2427-
std::set<std::pair<const CWalletTx*,unsigned int> > setCoins;
2424+
std::set<CInputCoin> setCoins;
24282425
LOCK2(cs_main, cs_wallet);
24292426
{
24302427
std::vector<COutput> vAvailableCoins;
@@ -2583,7 +2580,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
25832580
// behavior."
25842581
bool rbf = coinControl ? coinControl->signalRbf : fWalletRbf;
25852582
for (const auto& coin : setCoins)
2586-
txNew.vin.push_back(CTxIn(coin.first->GetHash(),coin.second,CScript(),
2583+
txNew.vin.push_back(CTxIn(coin.outpoint,CScript(),
25872584
std::numeric_limits<unsigned int>::max() - (rbf ? 2 : 1)));
25882585

25892586
// Fill in dummy signatures for fee calculation.
@@ -2666,10 +2663,10 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
26662663
int nIn = 0;
26672664
for (const auto& coin : setCoins)
26682665
{
2669-
const CScript& scriptPubKey = coin.first->tx->vout[coin.second].scriptPubKey;
2666+
const CScript& scriptPubKey = coin.txout.scriptPubKey;
26702667
SignatureData sigdata;
26712668

2672-
if (!ProduceSignature(TransactionSignatureCreator(this, &txNewConst, nIn, coin.first->tx->vout[coin.second].nValue, SIGHASH_ALL), scriptPubKey, sigdata))
2669+
if (!ProduceSignature(TransactionSignatureCreator(this, &txNewConst, nIn, coin.txout.nValue, SIGHASH_ALL), scriptPubKey, sigdata))
26732670
{
26742671
strFailReason = _("Signing transaction failed");
26752672
return false;

‎src/wallet/wallet.h

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -473,7 +473,34 @@ class CWalletTx : public CMerkleTx
473473
};
474474

475475

476+
class CInputCoin {
477+
public:
478+
CInputCoin(const CWalletTx* walletTx, unsigned int i)
479+
{
480+
if (!walletTx)
481+
throw std::invalid_argument("walletTx should not be null");
482+
if (i >= walletTx->tx->vout.size())
483+
throw std::out_of_range("The output index is out of range");
484+
485+
outpoint = COutPoint(walletTx->GetHash(), i);
486+
txout = walletTx->tx->vout[i];
487+
}
488+
489+
COutPoint outpoint;
490+
CTxOut txout;
476491

492+
bool operator<(const CInputCoin& rhs) const {
493+
return outpoint < rhs.outpoint;
494+
}
495+
496+
bool operator!=(const CInputCoin& rhs) const {
497+
return outpoint != rhs.outpoint;
498+
}
499+
500+
bool operator==(const CInputCoin& rhs) const {
501+
return outpoint == rhs.outpoint;
502+
}
503+
};
477504

478505
class COutput
479506
{
@@ -630,7 +657,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
630657
* all coins from coinControl are selected; Never select unconfirmed coins
631658
* if they are not ours
632659
*/
633-
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;
660+
bool SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAmount& nTargetValue, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CCoinControl *coinControl = NULL) const;
634661

635662
CWalletDB *pwalletdbEncryption;
636663

@@ -782,7 +809,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
782809
* completion the coin set and corresponding actual target value is
783810
* assembled
784811
*/
785-
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;
812+
bool SelectCoinsMinConf(const CAmount& nTargetValue, int nConfMine, int nConfTheirs, uint64_t nMaxAncestors, std::vector<COutput> vCoins, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet) const;
786813

787814
bool IsSpent(const uint256& hash, unsigned int n) const;
788815

@@ -1131,7 +1158,7 @@ bool CWallet::DummySignTx(CMutableTransaction &txNew, const ContainerType &coins
11311158
int nIn = 0;
11321159
for (const auto& coin : coins)
11331160
{
1134-
const CScript& scriptPubKey = coin.first->tx->vout[coin.second].scriptPubKey;
1161+
const CScript& scriptPubKey = coin.txout.scriptPubKey;
11351162
SignatureData sigdata;
11361163

11371164
if (!ProduceSignature(DummySignatureCreator(this), scriptPubKey, sigdata))

0 commit comments

Comments
 (0)