Overhaul of coin selection for mixing (#1364)

* Overhaul of coin selection for mixing

DoAutomaticDenominating logic should be:
- check pre-conditions,
- check denominations and collaterals,
- try using existing queue,
- try creating new queue.

Currently coins are selected too early and conditions are not quite right.

This is partially due to the fact that we no longer merge old inputs
and thus we are no longer able to calculate thresholds correctly using
SelectCoinsDark. To do this in a proper way we should use balances i.e.
GetAnonymizableBalance etc. Another issue is that we should take fee into
account when we calculate such balancies and when we select coins we should
ask for a correct denom, not just the smallest one as a minimum value.

And finally there are two bugs.
SelectCoinsGrouppedByAddresses: shouldn't push items smaller than
the smallest denom into resulting vector.
SelectCoinsDark: should allow small inputs in where "small" is defined
by nValueMin, not by some arbitrary amount.

* apply fee assumption for non-denoms only

* fix

* remove const

src/darksend.cpp

@@ -1382,79 +1382,46 @@ bool CDarksendPool::DoAutomaticDenominating(bool fDryRun)
         return false;
     }
 
-    // ** find the coins we'll use
+    CAmount nValueMin = vecPrivateSendDenominations.back();
-    std::vector<CTxIn> vecTxIn;
-    CAmount nValueMin = CENT;
-    CAmount nValueIn = 0;
 
-    CAmount nOnlyDenominatedBalance;
-    CAmount nBalanceNeedsDenominated;
-
-    CAmount nLowestDenom = vecPrivateSendDenominations.back();
     // if there are no confirmed DS collateral inputs yet
     if(!pwalletMain->HasCollateralInputs()) {
         // should have some additional amount for them
-        nLowestDenom += PRIVATESEND_COLLATERAL*4;
+        nValueMin += PRIVATESEND_COLLATERAL*4;
     }
 
-    CAmount nBalanceNeedsAnonymized = pwalletMain->GetNeedsToBeAnonymizedBalance(nLowestDenom);
+    // including denoms but applying some restrictions
+    CAmount nBalanceNeedsAnonymized = pwalletMain->GetNeedsToBeAnonymizedBalance(nValueMin);
 
     // anonymizable balance is way too small
-    if(nBalanceNeedsAnonymized < nLowestDenom) {
+    if(nBalanceNeedsAnonymized < nValueMin) {
         LogPrintf("CDarksendPool::DoAutomaticDenominating -- Not enough funds to anonymize\n");
         strAutoDenomResult = _("Not enough funds to anonymize.");
         return false;
     }
 
-    LogPrint("privatesend", "CDarksendPool::DoAutomaticDenominating -- nLowestDenom: %f, nBalanceNeedsAnonymized: %f\n", (float)nLowestDenom/COIN, (float)nBalanceNeedsAnonymized/COIN);
+    // excluding denoms
+    CAmount nBalanceAnonimizableNonDenom = pwalletMain->GetAnonymizableBalance(true);
+    // denoms
+    CAmount nBalanceDenominatedConf = pwalletMain->GetDenominatedBalance();
+    CAmount nBalanceDenominatedUnconf = pwalletMain->GetDenominatedBalance(true);
+    CAmount nBalanceDenominated = nBalanceDenominatedConf + nBalanceDenominatedUnconf;
 
-    // select coins that should be given to the pool
+    LogPrint("privatesend", "CDarksendPool::DoAutomaticDenominating -- nValueMin: %f, nBalanceNeedsAnonymized: %f, nBalanceAnonimizableNonDenom: %f, nBalanceDenominatedConf: %f, nBalanceDenominatedUnconf: %f, nBalanceDenominated: %f\n",
-    if(!pwalletMain->SelectCoinsDark(nValueMin, nBalanceNeedsAnonymized, vecTxIn, nValueIn, 0, nPrivateSendRounds))
+            (float)nValueMin/COIN,
-    {
+            (float)nBalanceNeedsAnonymized/COIN,
-        if(pwalletMain->SelectCoinsDark(nValueMin, 9999999*COIN, vecTxIn, nValueIn, -2, 0))
+            (float)nBalanceAnonimizableNonDenom/COIN,
-        {
+            (float)nBalanceDenominatedConf/COIN,
-            nOnlyDenominatedBalance = pwalletMain->GetDenominatedBalance(true) + pwalletMain->GetDenominatedBalance() - pwalletMain->GetAnonymizedBalance();
+            (float)nBalanceDenominatedUnconf/COIN,
-            nBalanceNeedsDenominated = nBalanceNeedsAnonymized - nOnlyDenominatedBalance;
+            (float)nBalanceDenominated/COIN);
-
-            if(nBalanceNeedsDenominated > nValueIn) nBalanceNeedsDenominated = nValueIn;
-
-            LogPrint("privatesend", "CDarksendPool::DoAutomaticDenominating -- `SelectCoinsDark` (%f - (%f + %f - %f = %f) ) = %f\n",
-                            (float)nBalanceNeedsAnonymized/COIN,
-                            (float)pwalletMain->GetDenominatedBalance(true)/COIN,
-                            (float)pwalletMain->GetDenominatedBalance()/COIN,
-                            (float)pwalletMain->GetAnonymizedBalance()/COIN,
-                            (float)nOnlyDenominatedBalance/COIN,
-                            (float)nBalanceNeedsDenominated/COIN);
-
-            if(nBalanceNeedsDenominated < nLowestDenom) { // most likely we are just waiting for denoms to confirm
-                LogPrintf("CDarksendPool::DoAutomaticDenominating -- No funds detected in need of denominating\n");
-                strAutoDenomResult = _("No funds detected in need of denominating.");
-                return false;
-            }
-            if(!fDryRun) return CreateDenominated();
-
-            return true;
-        } else {
-            LogPrintf("CDarksendPool::DoAutomaticDenominating -- Can't denominate (no compatible inputs left)\n");
-            strAutoDenomResult = _("Can't denominate: no compatible inputs left.");
-            return false;
-        }
-    }
 
     if(fDryRun) return true;
 
-    nOnlyDenominatedBalance = pwalletMain->GetDenominatedBalance(true) + pwalletMain->GetDenominatedBalance() - pwalletMain->GetAnonymizedBalance();
+    // Check if we have should create more denominated inputs i.e.
-    nBalanceNeedsDenominated = nBalanceNeedsAnonymized - nOnlyDenominatedBalance;
+    // there are funds to denominate and denominated balance does not exceed
-    LogPrint("privatesend", "CDarksendPool::DoAutomaticDenominating -- 'nBalanceNeedsDenominated > 0' (%f - (%f + %f - %f = %f) ) = %f\n",
+    // max amount to mix yet.
-                    (float)nBalanceNeedsAnonymized/COIN,
+    if(nBalanceAnonimizableNonDenom >= nValueMin + PRIVATESEND_COLLATERAL && nBalanceDenominated < nPrivateSendAmount*COIN)
-                    (float)pwalletMain->GetDenominatedBalance(true)/COIN,
+        return CreateDenominated();
-                    (float)pwalletMain->GetDenominatedBalance()/COIN,
-                    (float)pwalletMain->GetAnonymizedBalance()/COIN,
-                    (float)nOnlyDenominatedBalance/COIN,
-                    (float)nBalanceNeedsDenominated/COIN);
-
-    //check if we have should create more denominated inputs
-    if(nBalanceNeedsDenominated > 0) return CreateDenominated();
 
     //check if we have the collateral sized inputs
     if(!pwalletMain->HasCollateralInputs())
@@ -1470,7 +1437,8 @@ bool CDarksendPool::DoAutomaticDenominating(bool fDryRun)
     UnlockCoins();
     SetNull();
 
-    if(!fPrivateSendMultiSession && pwalletMain->GetDenominatedBalance(true) > 0) { //get denominated unconfirmed inputs
+    // should be no unconfirmed denoms in non-multi-session mode
+    if(!fPrivateSendMultiSession && nBalanceDenominatedUnconf > 0) {
         LogPrintf("CDarksendPool::DoAutomaticDenominating -- Found unconfirmed denominated outputs, will wait till they confirm to continue.\n");
         strAutoDenomResult = _("Found unconfirmed denominated outputs, will wait till they confirm to continue.");
         return false;
@@ -1525,8 +1493,11 @@ bool CDarksendPool::DoAutomaticDenominating(bool fDryRun)
 
             if(pmn->nProtocolVersion < MIN_PRIVATESEND_PEER_PROTO_VERSION) continue;
 
-            // incompatible denom
+            std::vector<int> vecBits;
-            if(dsq.nDenom >= (1 << vecPrivateSendDenominations.size())) continue;
+            if(!GetDenominationsBits(dsq.nDenom, vecBits)) {
+                // incompatible denom
+                continue;
+            }
 
             // mixing rate limit i.e. nLastDsq check should already pass in DSQUEUE ProcessMessage
             // in order for dsq to get into vecDarksendQueue, so we should be safe to mix already,
@@ -1534,11 +1505,13 @@ bool CDarksendPool::DoAutomaticDenominating(bool fDryRun)
 
             LogPrint("privatesend", "CDarksendPool::DoAutomaticDenominating -- found valid queue: %s\n", dsq.ToString());
 
+            CAmount nValueInTmp = 0;
             std::vector<CTxIn> vecTxInTmp;
             std::vector<COutput> vCoinsTmp;
-            // Try to match their denominations if possible
+
-            if(!pwalletMain->SelectCoinsByDenominations(dsq.nDenom, nValueMin, nBalanceNeedsAnonymized, vecTxInTmp, vCoinsTmp, nValueIn, 0, nPrivateSendRounds)) {
+            // Try to match their denominations if possible, select at least 1 denominations
-                LogPrintf("CDarksendPool::DoAutomaticDenominating -- Couldn't match denominations %d (%s)\n", dsq.nDenom, GetDenominationsToString(dsq.nDenom));
+            if(!pwalletMain->SelectCoinsByDenominations(dsq.nDenom, vecPrivateSendDenominations[vecBits.front()], nBalanceNeedsAnonymized, vecTxInTmp, vCoinsTmp, nValueInTmp, 0, nPrivateSendRounds)) {
+                LogPrintf("CDarksendPool::DoAutomaticDenominating -- Couldn't match denominations %d %d (%s)\n", vecBits.front(), dsq.nDenom, GetDenominationsToString(dsq.nDenom));
                 continue;
             }
 
@@ -1587,6 +1560,16 @@ bool CDarksendPool::DoAutomaticDenominating(bool fDryRun)
 
     int nTries = 0;
 
+    // ** find the coins we'll use
+    std::vector<CTxIn> vecTxIn;
+    CAmount nValueInTmp = 0;
+    if(!pwalletMain->SelectCoinsDark(nValueMin, nBalanceNeedsAnonymized, vecTxIn, nValueInTmp, 0, nPrivateSendRounds)) {
+        // this should never happen
+        LogPrintf("CDarksendPool::DoAutomaticDenominating -- Can't mix: no compatible inputs found!\n");
+        strAutoDenomResult = _("Can't mix: no compatible inputs found!");
+        return false;
+    }
+
     // otherwise, try one randomly
     while(nTries < 10) {
         CMasternode* pmn = mnodeman.FindRandomNotInVec(vecMasternodesUsed, MIN_PRIVATESEND_PEER_PROTO_VERSION);
@@ -1709,7 +1692,12 @@ bool CDarksendPool::PrepareDenominate(int nMinRounds, int nMaxRounds, std::strin
 
         if nMinRounds >= 0 it means only denominated inputs are going in and coming out
     */
-    bool fSelected = pwalletMain->SelectCoinsByDenominations(nSessionDenom, vecPrivateSendDenominations.back(), PRIVATESEND_POOL_MAX, vecTxIn, vCoins, nValueIn, nMinRounds, nMaxRounds);
+    std::vector<int> vecBits;
+    if (!GetDenominationsBits(nSessionDenom, vecBits)) {
+        strErrorRet = "Incorrect session denom";
+        return false;
+    }
+    bool fSelected = pwalletMain->SelectCoinsByDenominations(nSessionDenom, vecPrivateSendDenominations[vecBits.front()], PRIVATESEND_POOL_MAX, vecTxIn, vCoins, nValueIn, nMinRounds, nMaxRounds);
     if (nMinRounds >= 0 && !fSelected) {
         strErrorRet = "Can't select current denominated inputs";
         return false;
@@ -1731,11 +1719,6 @@ bool CDarksendPool::PrepareDenominate(int nMinRounds, int nMaxRounds, std::strin
     // initially shuffled in CWallet::SelectCoinsByDenominations already.
     int nStep = 0;
     int nStepsMax = 5 + GetRandInt(5);
-    std::vector<int> vecBits;
-    if (!GetDenominationsBits(nSessionDenom, vecBits)) {
-        strErrorRet = "Incorrect session denom";
-        return false;
-    }
 
     while (nStep < nStepsMax) {
         BOOST_FOREACH(int nBit, vecBits) {

src/wallet/wallet.cpp

@@ -1856,18 +1856,20 @@ CAmount CWallet::GetBalance() const
     return nTotal;
 }
 
-CAmount CWallet::GetAnonymizableBalance() const
+CAmount CWallet::GetAnonymizableBalance(bool fSkipDenominated) const
 {
     if(fLiteMode) return 0;
 
     std::vector<CompactTallyItem> vecTally;
-    if(!SelectCoinsGrouppedByAddresses(vecTally, false)) return 0;
+    if(!SelectCoinsGrouppedByAddresses(vecTally, fSkipDenominated)) return 0;
 
     CAmount nTotal = 0;
 
     BOOST_FOREACH(CompactTallyItem& item, vecTally) {
-        // try to anonymize all denoms and anything greater than sum of 10 smallest denoms
+        bool fIsDenominated = IsDenominatedAmount(item.nAmount);
-        if(IsDenominatedAmount(item.nAmount) || item.nAmount >= vecPrivateSendDenominations.back() * 10)
+        if(fSkipDenominated && fIsDenominated) continue;
+        // assume that the fee to create denoms be PRIVATESEND_COLLATERAL at max
+        if(item.nAmount >= vecPrivateSendDenominations.back() + (fIsDenominated ? 0 : PRIVATESEND_COLLATERAL))
             nTotal += item.nAmount;
     }
 
@@ -2559,12 +2561,12 @@ bool CWallet::SelectCoinsGrouppedByAddresses(std::vector<CompactTallyItem>& vecT
         if(fSkipDenominated && fAnonymizableTallyCachedNonDenom) {
             vecTallyRet = vecAnonymizableTallyCachedNonDenom;
             LogPrint("selectcoins", "SelectCoinsGrouppedByAddresses - using cache for non-denom inputs\n");
-            return true;
+            return vecTallyRet.size() > 0;
         }
         if(!fSkipDenominated && fAnonymizableTallyCached) {
             vecTallyRet = vecAnonymizableTallyCached;
             LogPrint("selectcoins", "SelectCoinsGrouppedByAddresses - using cache for all inputs\n");
-            return true;
+            return vecTallyRet.size() > 0;
         }
     }
 
@@ -2605,13 +2607,12 @@ bool CWallet::SelectCoinsGrouppedByAddresses(std::vector<CompactTallyItem>& vecT
         }
     }
 
-    // we found nothing
-    if(mapTally.size() == 0) return false;
-
     // construct resulting vector
     vecTallyRet.clear();
-    BOOST_FOREACH(const PAIRTYPE(CBitcoinAddress, CompactTallyItem)& item, mapTally)
+    BOOST_FOREACH(const PAIRTYPE(CBitcoinAddress, CompactTallyItem)& item, mapTally) {
+        if(fAnonymizable && item.second.nAmount < vecPrivateSendDenominations.back()) continue;
         vecTallyRet.push_back(item.second);
+    }
 
     // order by amounts per address, from smallest to largest
     sort(vecTallyRet.rbegin(), vecTallyRet.rend(), CompareByAmount());
@@ -2633,7 +2634,7 @@ bool CWallet::SelectCoinsGrouppedByAddresses(std::vector<CompactTallyItem>& vecT
         strMessage += strprintf("  %s %f\n", item.address.ToString().c_str(), float(item.nAmount)/COIN);
     LogPrint("selectcoins", "%s", strMessage);
 
-    return true;
+    return vecTallyRet.size() > 0;
 }
 
 bool CWallet::SelectCoinsDark(CAmount nValueMin, CAmount nValueMax, std::vector<CTxIn>& vecTxInRet, CAmount& nValueRet, int nPrivateSendRoundsMin, int nPrivateSendRoundsMax) const
@@ -2651,8 +2652,8 @@ bool CWallet::SelectCoinsDark(CAmount nValueMin, CAmount nValueMax, std::vector<
 
     BOOST_FOREACH(const COutput& out, vCoins)
     {
-        //do not allow inputs less than 1 CENT
+        //do not allow inputs less than 1/10th of minimum value
-        if(out.tx->vout[out.i].nValue < CENT) continue;
+        if(out.tx->vout[out.i].nValue < nValueMin/10) continue;
         //do not allow collaterals to be selected
         if(IsCollateralAmount(out.tx->vout[out.i].nValue)) continue;
         if(fMasterNode && out.tx->vout[out.i].nValue == 1000*COIN) continue; //masternode input
@@ -2670,10 +2671,7 @@ bool CWallet::SelectCoinsDark(CAmount nValueMin, CAmount nValueMax, std::vector<
         }
     }
 
-    // if it's more than min, we're good to return
+    return nValueRet >= nValueMin;
-    if(nValueRet >= nValueMin) return true;
-
-    return false;
 }
 
 bool CWallet::GetCollateralTxIn(CTxIn& txinRet, CAmount& nValueRet) const

src/wallet/wallet.h

@@ -741,7 +741,7 @@ public:
     CAmount GetUnconfirmedWatchOnlyBalance() const;
     CAmount GetImmatureWatchOnlyBalance() const;
 
-    CAmount GetAnonymizableBalance() const;
+    CAmount GetAnonymizableBalance(bool fSkipDenominated = false) const;
     CAmount GetAnonymizedBalance() const;
     double GetAverageAnonymizedRounds() const;
     CAmount GetNormalizedAnonymizedBalance() const;