Skip to content

Commit 69b42ce

Browse files
laanwjPastaPastaPasta
authored andcommitted
Merge bitcoin#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 fix error, based on 1 0 1 6 5 fix error, based on 1 0 1 6 5
1 parent d9a4dea commit 69b42ce

File tree

4 files changed

+71
-49
lines changed

4 files changed

+71
-49
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/test/wallet_tests.cpp

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

3232
std::vector<std::unique_ptr<CWalletTx>> wtxn;
3333

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

3636
BOOST_FIXTURE_TEST_SUITE(wallet_tests, WalletTestingSetup)
3737

src/wallet/wallet.cpp

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

7777
struct CompareValueOnly
7878
{
79-
bool operator()(const std::pair<CAmount, std::pair<const CWalletTx*, unsigned int> >& t1,
80-
const std::pair<CAmount, std::pair<const CWalletTx*, unsigned int> >& t2) const
79+
bool operator()(const CInputCoin& t1,
80+
const CInputCoin& t2) const
8181
{
82-
return t1.first < t2.first;
82+
return t1.txout.nValue < t2.txout.nValue;
8383
}
8484
};
8585

@@ -2601,7 +2601,7 @@ void CWallet::AvailableCoins(std::vector<COutput>& vCoins, bool fOnlySafe, const
26012601
}
26022602
}
26032603

2604-
static void ApproximateBestSubset(const std::vector<std::pair<CAmount, std::pair<const CWalletTx*,unsigned int> > >& vValue, const CAmount& nTotalLower, const CAmount& nTargetValue,
2604+
static void ApproximateBestSubset(const std::vector<CInputCoin>& vValue, const CAmount& nTotalLower, const CAmount& nTargetValue,
26052605
std::vector<char>& vfBest, CAmount& nBest, bool fUseInstantSend = false, int iterations = 1000)
26062606
{
26072607
std::vector<char> vfIncluded;
@@ -2626,7 +2626,7 @@ static void ApproximateBestSubset(const std::vector<std::pair<CAmount, std::pair
26262626
{
26272627
for (unsigned int i = 0; i < vValue.size(); i++)
26282628
{
2629-
if (fUseInstantSend && nTotal + vValue[i].first > sporkManager.GetSporkValue(SPORK_5_INSTANTSEND_MAX_VALUE)*COIN) {
2629+
if (fUseInstantSend && nTotal + vValue[i].txout.nValue > sporkManager.GetSporkValue(SPORK_5_INSTANTSEND_MAX_VALUE)*COIN) {
26302630
continue;
26312631
}
26322632
//The solver here uses a randomized algorithm,
@@ -2637,7 +2637,7 @@ static void ApproximateBestSubset(const std::vector<std::pair<CAmount, std::pair
26372637
//the selection random.
26382638
if (nPass == 0 ? insecure_rand.rand32()&1 : !vfIncluded[i])
26392639
{
2640-
nTotal += vValue[i].first;
2640+
nTotal += vValue[i].txout.nValue;
26412641
vfIncluded[i] = true;
26422642
if (nTotal >= nTargetValue)
26432643
{
@@ -2647,7 +2647,7 @@ static void ApproximateBestSubset(const std::vector<std::pair<CAmount, std::pair
26472647
nBest = nTotal;
26482648
vfBest = vfIncluded;
26492649
}
2650-
nTotal -= vValue[i].first;
2650+
nTotal -= vValue[i].txout.nValue;
26512651
vfIncluded[i] = false;
26522652
}
26532653
}
@@ -2682,7 +2682,7 @@ bool less_then_denom (const COutput& out1, const COutput& out2)
26822682
}
26832683

26842684
bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMine, const int nConfTheirs, const uint64_t nMaxAncestors, std::vector<COutput> vCoins,
2685-
std::set<std::pair<const CWalletTx*,unsigned int> >& setCoinsRet, CAmount& nValueRet, AvailableCoinsType nCoinType, bool fUseInstantSend) const
2685+
std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, AvailableCoinsType nCoinType, bool fUseInstantSend) const
26862686
{
26872687
setCoinsRet.clear();
26882688
nValueRet = 0;
@@ -2694,12 +2694,8 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin
26942694
}
26952695

26962696
// List of values less than target
2697-
std::pair<CAmount, std::pair<const CWalletTx*,unsigned int> > coinLowestLarger;
2698-
coinLowestLarger.first = fUseInstantSend
2699-
? sporkManager.GetSporkValue(SPORK_5_INSTANTSEND_MAX_VALUE)*COIN
2700-
: std::numeric_limits<CAmount>::max();
2701-
coinLowestLarger.second.first = NULL;
2702-
std::vector<std::pair<CAmount, std::pair<const CWalletTx*,unsigned int> > > vValue;
2697+
boost::optional<CInputCoin> coinLowestLarger;
2698+
std::vector<CInputCoin> vValue;
27032699
CAmount nTotalLower = 0;
27042700

27052701
random_shuffle(vCoins.begin(), vCoins.end(), GetRandInt);
@@ -2741,8 +2737,10 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin
27412737
continue;
27422738

27432739
int i = output.i;
2744-
CAmount n = pcoin->tx->vout[i].nValue;
2745-
if (tryDenom == 0 && CPrivateSend::IsDenominatedAmount(n)) continue; // we don't want denom values on first run
2740+
2741+
CInputCoin coin = CInputCoin(pcoin, i);
2742+
2743+
if (tryDenom == 0 && CPrivateSend::IsDenominatedAmount(coin.txout.nValue)) continue; // we don't want denom values on first run
27462744

27472745
if (nCoinType == ONLY_DENOMINATED) {
27482746
// Make sure it's actually anonymized
@@ -2751,20 +2749,18 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin
27512749
if (nRounds < privateSendClient.nPrivateSendRounds) continue;
27522750
}
27532751

2754-
std::pair<CAmount, std::pair<const CWalletTx*,unsigned int> > coin = std::make_pair(n, std::make_pair(pcoin, i));
2755-
2756-
if (n == nTargetValue)
2752+
if (coin.txout.nValue == nTargetValue)
27572753
{
2758-
setCoinsRet.insert(coin.second);
2759-
nValueRet += coin.first;
2754+
setCoinsRet.insert(coin);
2755+
nValueRet += coin.txout.nValue;
27602756
return true;
27612757
}
2762-
else if (n < nTargetValue + nMinChange)
2758+
else if (coin.txout.nValue < nTargetValue + nMinChange)
27632759
{
27642760
vValue.push_back(coin);
2765-
nTotalLower += n;
2761+
nTotalLower += coin.txout.nValue;
27662762
}
2767-
else if (n < coinLowestLarger.first)
2763+
else if (!coinLowestLarger || coin.txout.nValue < coinLowestLarger->txout.nValue)
27682764
{
27692765
coinLowestLarger = coin;
27702766
}
@@ -2774,15 +2770,15 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin
27742770
{
27752771
for (unsigned int i = 0; i < vValue.size(); ++i)
27762772
{
2777-
setCoinsRet.insert(vValue[i].second);
2778-
nValueRet += vValue[i].first;
2773+
setCoinsRet.insert(vValue[i]);
2774+
nValueRet += vValue[i].txout.nValue;
27792775
}
27802776
return true;
27812777
}
27822778

27832779
if (nTotalLower < nTargetValue)
27842780
{
2785-
if (coinLowestLarger.second.first == NULL) // there is no input larger than nTargetValue
2781+
if (!coinLowestLarger) // there is no input larger than nTargetValue
27862782
{
27872783
if (tryDenom == 0)
27882784
// we didn't look at denom yet, let's do it
@@ -2791,16 +2787,15 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin
27912787
// we looked at everything possible and didn't find anything, no luck
27922788
return false;
27932789
}
2794-
setCoinsRet.insert(coinLowestLarger.second);
2795-
nValueRet += coinLowestLarger.first;
2790+
setCoinsRet.insert(coinLowestLarger.get());
2791+
nValueRet += coinLowestLarger->txout.nValue;
27962792
// There is no change in PS, so we know the fee beforehand,
27972793
// can see if we exceeded the max fee and thus fail quickly.
27982794
return nCoinType == ONLY_DENOMINATED ? (nValueRet - nTargetValue <= maxTxFee) : true;
27992795
}
28002796

28012797
// nTotalLower > nTargetValue
28022798
break;
2803-
28042799
}
28052800

28062801
// Solve subset sum by stochastic approximation
@@ -2815,21 +2810,21 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin
28152810

28162811
// If we have a bigger coin and (either the stochastic approximation didn't find a good solution,
28172812
// or the next bigger coin is closer), return the bigger coin
2818-
if (coinLowestLarger.second.first &&
2819-
((nBest != nTargetValue && nBest < nTargetValue + nMinChange) || coinLowestLarger.first <= nBest))
2813+
if (coinLowestLarger &&
2814+
((nBest != nTargetValue && nBest < nTargetValue + nMinChange) || coinLowestLarger->txout.nValue <= nBest))
28202815
{
2821-
setCoinsRet.insert(coinLowestLarger.second);
2822-
nValueRet += coinLowestLarger.first;
2816+
setCoinsRet.insert(coinLowestLarger.get());
2817+
nValueRet += coinLowestLarger->txout.nValue;
28232818
}
28242819
else {
28252820
std::string s = "CWallet::SelectCoinsMinConf best subset: ";
28262821
for (unsigned int i = 0; i < vValue.size(); i++)
28272822
{
28282823
if (vfBest[i])
28292824
{
2830-
setCoinsRet.insert(vValue[i].second);
2831-
nValueRet += vValue[i].first;
2832-
s += FormatMoney(vValue[i].first) + " ";
2825+
setCoinsRet.insert(vValue[i]);
2826+
nValueRet += vValue[i].txout.nValue;
2827+
s += FormatMoney(vValue[i].txout.nValue) + " ";
28332828
}
28342829
}
28352830
LogPrint(BCLog::SELECTCOINS, "%s - total %s\n", s, FormatMoney(nBest));
@@ -2840,7 +2835,7 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin
28402835
return nCoinType == ONLY_DENOMINATED ? (nValueRet - nTargetValue <= maxTxFee) : true;
28412836
}
28422837

2843-
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, AvailableCoinsType nCoinType, bool fUseInstantSend) const
2838+
bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAmount& nTargetValue, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CCoinControl* coinControl, AvailableCoinsType nCoinType, bool fUseInstantSend) const
28442839
{
28452840
// Note: this function should never be used for "always free" tx types like dstx
28462841

@@ -2861,7 +2856,7 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
28612856
if(nRounds < privateSendClient.nPrivateSendRounds) continue;
28622857
}
28632858
nValueRet += out.tx->tx->vout[out.i].nValue;
2864-
setCoinsRet.insert(std::make_pair(out.tx, out.i));
2859+
setCoinsRet.insert(CInputCoin(out.tx, out.i));
28652860

28662861
if (!coinControl->fRequireAllInputs && nValueRet >= nTargetValue) {
28672862
// 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<COutput>& vAvailableCoins, const CAm
28732868
}
28742869

28752870
// calculate value from preset inputs and store them
2876-
std::set<std::pair<const CWalletTx*, uint32_t> > setPresetCoins;
2871+
std::set<CInputCoin> setPresetCoins;
28772872
CAmount nValueFromPresetInputs = 0;
28782873

28792874
std::vector<COutPoint> vPresetInputs;
@@ -2895,15 +2890,15 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
28952890
if (nRounds < privateSendClient.nPrivateSendRounds) continue;
28962891
}
28972892
nValueFromPresetInputs += pcoin->tx->vout[outpoint.n].nValue;
2898-
setPresetCoins.insert(std::make_pair(pcoin, outpoint.n));
2893+
setPresetCoins.insert(CInputCoin(pcoin, outpoint.n));
28992894
} else
29002895
return false; // TODO: Allow non-wallet inputs
29012896
}
29022897

29032898
// remove preset inputs from vCoins
29042899
for (std::vector<COutput>::iterator it = vCoins.begin(); it != vCoins.end() && coinControl && coinControl->HasSelected();)
29052900
{
2906-
if (setPresetCoins.count(std::make_pair(it->tx, it->i)))
2901+
if (setPresetCoins.count(CInputCoin(it->tx, it->i)))
29072902
it = vCoins.erase(it);
29082903
else
29092904
++it;
@@ -3435,7 +3430,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
34353430
assert(txNew.nLockTime < LOCKTIME_THRESHOLD);
34363431

34373432
{
3438-
std::set<std::pair<const CWalletTx*,unsigned int> > setCoins;
3433+
std::set<CInputCoin> setCoins;
34393434
std::vector<CTxDSIn> vecTxDSInTmp;
34403435
LOCK2(cs_main, cs_wallet);
34413436
{
@@ -3617,9 +3612,9 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
36173612
// nLockTime set above actually works.
36183613
vecTxDSInTmp.clear();
36193614
for (const auto& coin : setCoins) {
3620-
CTxIn txin = CTxIn(coin.first->GetHash(),coin.second,CScript(),
3615+
CTxIn txin = CTxIn(coin.outpoint,CScript(),
36213616
std::numeric_limits<unsigned int>::max()-1);
3622-
vecTxDSInTmp.push_back(CTxDSIn(txin, coin.first->tx->vout[coin.second].scriptPubKey));
3617+
vecTxDSInTmp.push_back(CTxDSIn(txin, coin.txout.scriptPubKey));
36233618
txNew.vin.push_back(txin);
36243619
}
36253620

src/wallet/wallet.h

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -518,7 +518,34 @@ class CWalletTx : public CMerkleTx
518518
};
519519

520520

521+
class CInputCoin {
522+
public:
523+
CInputCoin(const CWalletTx* walletTx, unsigned int i)
524+
{
525+
if (!walletTx)
526+
throw std::invalid_argument("walletTx should not be null");
527+
if (i >= walletTx->tx->vout.size())
528+
throw std::out_of_range("The output index is out of range");
529+
530+
outpoint = COutPoint(walletTx->GetHash(), i);
531+
txout = walletTx->tx->vout[i];
532+
}
533+
534+
COutPoint outpoint;
535+
CTxOut txout;
521536

537+
bool operator<(const CInputCoin& rhs) const {
538+
return outpoint < rhs.outpoint;
539+
}
540+
541+
bool operator!=(const CInputCoin& rhs) const {
542+
return outpoint != rhs.outpoint;
543+
}
544+
545+
bool operator==(const CInputCoin& rhs) const {
546+
return outpoint == rhs.outpoint;
547+
}
548+
};
522549

523550
class COutput
524551
{
@@ -680,7 +707,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
680707
* all coins from coinControl are selected; Never select unconfirmed coins
681708
* if they are not ours
682709
*/
683-
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, AvailableCoinsType nCoinType=ALL_COINS, bool fUseInstantSend = true) const;
710+
bool SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAmount& nTargetValue, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CCoinControl *coinControl = NULL, AvailableCoinsType nCoinType=ALL_COINS, bool fUseInstantSend = true) const;
684711

685712
CWalletDB *pwalletdbEncryption;
686713

@@ -869,7 +896,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
869896
* completion the coin set and corresponding actual target value is
870897
* assembled
871898
*/
872-
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, AvailableCoinsType nCoinType=ALL_COINS, bool fUseInstantSend = false) const;
899+
bool SelectCoinsMinConf(const CAmount& nTargetValue, int nConfMine, int nConfTheirs, uint64_t nMaxAncestors, std::vector<COutput> vCoins, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, AvailableCoinsType nCoinType=ALL_COINS, bool fUseInstantSend = false) const;
873900

874901
// Coin selection
875902
bool SelectPSInOutPairsByDenominations(int nDenom, CAmount nValueMin, CAmount nValueMax, std::vector< std::pair<CTxDSIn, CTxOut> >& vecPSInOutPairsRet);

0 commit comments

Comments
 (0)