Merge #906: Improve CreateDenominated and MakeCollateralAmounts

ca207f6 Improve CreateDenominated and MakeCollateralAmounts:
- should use funds only from the same receiving address
- should denominate while has funds but has not enough denoms
- slightly refactored

20d3a23 Add few more improvements to CreateDenominated and MakeCollateralAmounts:
- return change to the origin address to use it in future splits
- fix a bug with incorrect overshoot
- more debug (optional/errors only) and fixed var names

src/darksend.cpp

@@ -14,6 +14,7 @@
 #include "instantx.h"
 #include "txmempool.h"
 #include "ui_interface.h"
+#include "coincontrol.h"
 #include <boost/algorithm/string/replace.hpp>
 #include <boost/filesystem.hpp>
 #include <boost/filesystem/fstream.hpp>
@@ -1076,6 +1077,7 @@ bool CDarksendPool::SignaturesComplete(){
             if(!s.fHasSig) return false;
         }
     }
+
     return true;
 }
 
@@ -1457,35 +1459,21 @@ bool CDarksendPool::DoAutomaticDenominating(bool fDryRun)
         // should have some additional amount for them
         nLowestDenom += DARKSEND_COLLATERAL*4;
 
-    CAmount nBalanceNeedsAnonymized = nAnonymizeDashAmount*COIN - pwalletMain->GetAnonymizedBalance();
+    CAmount nBalanceNeedsAnonymized = pwalletMain->GetNeedsToBeAnonymizedBalance(nLowestDenom);
-
-    // if balanceNeedsAnonymized is more than pool max, take the pool max
-    if(nBalanceNeedsAnonymized > DARKSEND_POOL_MAX) nBalanceNeedsAnonymized = DARKSEND_POOL_MAX;
-
-    // try to overshoot target DS balance up to nLowestDenom
-    nBalanceNeedsAnonymized += nLowestDenom;
-
-    CAmount nAnonymizableBalance = pwalletMain->GetAnonymizableBalance();
 
     // anonymizable balance is way too small
-    if(nAnonymizableBalance < nLowestDenom)
+    if(nBalanceNeedsAnonymized < nLowestDenom)
     {
-        LogPrintf("DoAutomaticDenominating : No funds detected in need of denominating \n");
+        LogPrintf("DoAutomaticDenominating : Not enough funds to anonymize\n");
-        strAutoDenomResult = _("No funds detected in need of denominating.");
+        strAutoDenomResult = _("Not enough funds to anonymize.");
         return false;
     }
 
-    // not enough funds to anonymze amount we want, try the max we can
-    if(nBalanceNeedsAnonymized > nAnonymizableBalance) nBalanceNeedsAnonymized = nAnonymizableBalance;
-
     LogPrint("privatesend", "DoAutomaticDenominating : nLowestDenom=%d, nBalanceNeedsAnonymized=%d\n", nLowestDenom, nBalanceNeedsAnonymized);
 
     // select coins that should be given to the pool
     if (!pwalletMain->SelectCoinsDark(nValueMin, nBalanceNeedsAnonymized, vCoins, nValueIn, 0, nPrivateSendRounds))
     {
-        nValueIn = 0;
-        vCoins.clear();
-
         if (pwalletMain->SelectCoinsDark(nValueMin, 9999999*COIN, vCoins, nValueIn, -2, 0))
         {
             nOnlyDenominatedBalance = pwalletMain->GetDenominatedBalance(true) + pwalletMain->GetDenominatedBalance() - pwalletMain->GetAnonymizedBalance();
@@ -1501,8 +1489,12 @@ bool CDarksendPool::DoAutomaticDenominating(bool fDryRun)
                             (float)nOnlyDenominatedBalance/COIN,
                             (float)nBalanceNeedsDenominated/COIN);
 
-            if(nBalanceNeedsDenominated < nLowestDenom) return false; // most likely we just waiting for denoms to confirm
+            if(nBalanceNeedsDenominated < nLowestDenom) { // most likely we are just waiting for denoms to confirm
-            if(!fDryRun) return CreateDenominated(nBalanceNeedsDenominated);
+                LogPrintf("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 {
@@ -1526,7 +1518,7 @@ bool CDarksendPool::DoAutomaticDenominating(bool fDryRun)
                     (float)nBalanceNeedsDenominated/COIN);
 
     //check if we have should create more denominated inputs
-    if(nBalanceNeedsDenominated > nOnlyDenominatedBalance) return CreateDenominated(nBalanceNeedsDenominated);
+    if(nBalanceNeedsDenominated > 0) return CreateDenominated();
 
     //check if we have the collateral sized inputs
     if(!pwalletMain->HasCollateralInputs()) return !pwalletMain->HasCollateralInputs(false) && MakeCollateralAmounts();
@@ -1702,8 +1694,11 @@ bool CDarksendPool::PrepareDarksendDenominate()
     // Try to use only inputs with the same number of rounds starting from lowest number of rounds possible
     for(int i = 0; i < nPrivateSendRounds; i++) {
         strError = pwalletMain->PrepareDarksendDenominate(i, i+1);
-        LogPrintf("DoAutomaticDenominating : Running PrivateSend denominate for %d rounds. Return '%s'\n", i, strError);
+        if(strError == "") {
-        if(strError == "") return true;
+            LogPrintf("PrepareDenominate : Running PrivateSend denominate for %d rounds, success\n", i);
+            return true;
+        }
+        LogPrintf("PrepareDenominate : Running PrivateSend denominate for %d rounds. Return '%s'\n", i, strError);
     }
 
     // We failed? That's strange but let's just make final attempt and try to mix everything
@@ -1717,15 +1712,32 @@ bool CDarksendPool::PrepareDarksendDenominate()
     return false;
 }
 
-// Split up large inputs or create fee sized inputs
+// Create collaterals by looping through inputs grouped by addresses
 bool CDarksendPool::MakeCollateralAmounts()
+{
+    std::vector<CompactTallyItem> vecTally;
+    if(!pwalletMain->SelectCoinsGrouppedByAddresses(vecTally)) {
+        LogPrint("privatesend", "CWallet::MakeCollateralAmounts() - SelectCoinsGrouppedByAddresses can't find any inputs!\n");
+        return false;
+    }
+
+    BOOST_FOREACH(CompactTallyItem& item, vecTally) {
+        if(!MakeCollateralAmounts(item)) continue;
+        return true;
+    }
+
+    LogPrintf("CWallet::MakeCollateralAmounts() - failed!\n");
+    return false;
+}
+
+// Split up large inputs or create fee sized inputs
+bool CDarksendPool::MakeCollateralAmounts(const CompactTallyItem& tallyItem)
 {
     CWalletTx wtx;
     CAmount nFeeRet = 0;
     int nChangePosRet = -1;
     std::string strFail = "";
-    vector< CRecipient > vecSend;
+    std::vector<CRecipient> vecSend;
-    CCoinControl *coinControl = NULL;
 
     // make our collateral address
     CReserveKey reservekeyCollateral(pwalletMain);
@@ -1739,16 +1751,25 @@ bool CDarksendPool::MakeCollateralAmounts()
 
     vecSend.push_back((CRecipient){scriptCollateral, DARKSEND_COLLATERAL*4, false});
 
-    // try to use non-denominated and not mn-like funds
+    // try to use non-denominated and not mn-like funds first, select them explicitly
-    bool success = pwalletMain->CreateTransaction(vecSend, wtx, reservekeyChange,
+    CCoinControl coinControl;
-            nFeeRet, nChangePosRet, strFail, coinControl, true, ONLY_NONDENOMINATED_NOT1000IFMN);
+    coinControl.fAllowOtherInputs = false;
-    if(!success){
+    coinControl.fAllowWatchOnly = false;
+    // send change to the same address so that we were able create more denoms out of it later
+    coinControl.destChange = tallyItem.address.Get();
+    BOOST_FOREACH(const CTxIn& txin, tallyItem.vecTxIn)
+        coinControl.Select(txin.prevout);
+
+    bool fSuccess = pwalletMain->CreateTransaction(vecSend, wtx, reservekeyChange,
+            nFeeRet, nChangePosRet, strFail, &coinControl, true, ONLY_NONDENOMINATED_NOT1000IFMN);
+    if(!fSuccess) {
         // if we failed (most likeky not enough funds), try to use all coins instead -
         // MN-like funds should not be touched in any case and we can't mix denominated without collaterals anyway
         LogPrintf("MakeCollateralAmounts: ONLY_NONDENOMINATED_NOT1000IFMN Error - %s\n", strFail);
-        success = pwalletMain->CreateTransaction(vecSend, wtx, reservekeyChange,
+        CCoinControl *coinControlNull = NULL;
-                nFeeRet, nChangePosRet, strFail, coinControl, true, ONLY_NOT1000IFMN);
+        fSuccess = pwalletMain->CreateTransaction(vecSend, wtx, reservekeyChange,
-        if(!success){
+                nFeeRet, nChangePosRet, strFail, coinControlNull, true, ONLY_NOT1000IFMN);
+        if(!fSuccess) {
             LogPrintf("MakeCollateralAmounts: ONLY_NOT1000IFMN Error - %s\n", strFail);
             reservekeyCollateral.ReturnKey();
             return false;
@@ -1770,23 +1791,34 @@ bool CDarksendPool::MakeCollateralAmounts()
     return true;
 }
 
-// Create denominations
+// Create denominations by looping through inputs grouped by addresses
-bool CDarksendPool::CreateDenominated(CAmount nTotalValue)
+bool CDarksendPool::CreateDenominated()
 {
-    CWalletTx wtx;
+    std::vector<CompactTallyItem> vecTally;
-    CAmount nFeeRet = 0;
+    if(!pwalletMain->SelectCoinsGrouppedByAddresses(vecTally)) {
-    int nChangePosRet = -1;
+        LogPrint("privatesend", "CWallet::CreateDenominated() - SelectCoinsGrouppedByAddresses can't find any inputs!\n");
-    std::string strFail = "";
+        return false;
-    vector< CRecipient > vecSend;
+    }
-    CAmount nValueLeft = nTotalValue;
+
+    BOOST_FOREACH(CompactTallyItem& item, vecTally) {
+        if(!CreateDenominated(item)) continue;
+        return true;
+    }
+
+    LogPrintf("CWallet::CreateDenominated() - failed!\n");
+    return false;
+}
+
+// Create denominations
+bool CDarksendPool::CreateDenominated(const CompactTallyItem& tallyItem)
+{
+    std::vector<CRecipient> vecSend;
+    CAmount nValueLeft = tallyItem.nAmount;
+    nValueLeft -= DARKSEND_COLLATERAL; // leave some room for fees
 
     LogPrintf("CreateDenominated0 %d\n", nValueLeft);
     // make our collateral address
     CReserveKey reservekeyCollateral(pwalletMain);
-    // make our change address
-    CReserveKey reservekeyChange(pwalletMain);
-    // make our denom addresses
-    CReserveKey reservekeyDenom(pwalletMain);
 
     CScript scriptCollateral;
     CPubKey vchPubKey;
@@ -1794,6 +1826,7 @@ bool CDarksendPool::CreateDenominated(CAmount nTotalValue)
     scriptCollateral = GetScriptForDestination(vchPubKey.GetID());
 
     // ****** Add collateral outputs ************ /
+
     if(!pwalletMain->HasCollateralInputs()) {
         vecSend.push_back((CRecipient){scriptCollateral, DARKSEND_COLLATERAL*4, false});
         nValueLeft -= DARKSEND_COLLATERAL*4;
@@ -1801,6 +1834,9 @@ bool CDarksendPool::CreateDenominated(CAmount nTotalValue)
 
     // ****** Add denoms ************ /
 
+    // make our denom addresses
+    CReserveKey reservekeyDenom(pwalletMain);
+
     // try few times - skipping smallest denoms first if there are too much already, if failed - use them
     int nOutputsTotal = 0;
     bool fSkip = true;
@@ -1855,10 +1891,24 @@ bool CDarksendPool::CreateDenominated(CAmount nTotalValue)
 
     // if we have anything left over, it will be automatically send back as change - there is no need to send it manually
 
-    CCoinControl *coinControl=NULL;
+    CCoinControl coinControl;
-    bool success = pwalletMain->CreateTransaction(vecSend, wtx, reservekeyChange,
+    coinControl.fAllowOtherInputs = false;
-            nFeeRet, nChangePosRet, strFail, coinControl, true, ONLY_NONDENOMINATED_NOT1000IFMN);
+    coinControl.fAllowWatchOnly = false;
-    if(!success){
+    // send change to the same address so that we were able create more denoms out of it later
+    coinControl.destChange = tallyItem.address.Get();
+    BOOST_FOREACH(const CTxIn& txin, tallyItem.vecTxIn)
+        coinControl.Select(txin.prevout);
+
+    CWalletTx wtx;
+    CAmount nFeeRet = 0;
+    int nChangePosRet = -1;
+    std::string strFail = "";
+    // make our change address
+    CReserveKey reservekeyChange(pwalletMain);
+
+    bool fSuccess = pwalletMain->CreateTransaction(vecSend, wtx, reservekeyChange,
+            nFeeRet, nChangePosRet, strFail, &coinControl, true, ONLY_NONDENOMINATED_NOT1000IFMN);
+    if(!fSuccess) {
         LogPrintf("CreateDenominated: Error - %s\n", strFail);
         // TODO: return reservekeyDenom here
         reservekeyCollateral.ReturnKey();
@@ -1868,12 +1918,13 @@ bool CDarksendPool::CreateDenominated(CAmount nTotalValue)
     // TODO: keep reservekeyDenom here
     reservekeyCollateral.KeepKey();
 
-    // use the same cachedLastSuccess as for DS mixinx to prevent race
+    if(!pwalletMain->CommitTransaction(wtx, reservekeyChange)) {
-    if(pwalletMain->CommitTransaction(wtx, reservekeyChange))
-        cachedLastSuccess = pCurrentBlockIndex->nHeight;
-    else
         LogPrintf("CreateDenominated: CommitTransaction failed!\n");
+        return false;
+    }
 
+    // use the same cachedLastSuccess as for DS mixing to prevent race
+    cachedLastSuccess = pCurrentBlockIndex->nHeight;
     LogPrintf("CreateDenominated: tx %s\n", wtx.GetHash().GetHex());
 
     return true;
@@ -1891,7 +1942,7 @@ bool CDarksendPool::IsCompatibleWithEntries(std::vector<CTxOut>& vout)
     return true;
 }
 
-bool CDarksendPool::IsCompatibleWithSession(CAmount nDenom, CTransaction txCollateral, int& errorID)
+bool CDarksendPool::IsCompatibleWithSession(int nDenom, CTransaction txCollateral, int& errorID)
 {
     if(nDenom == 0) return false;
 

src/darksend.h

@@ -472,7 +472,7 @@ public:
     bool IsCompatibleWithEntries(std::vector<CTxOut>& vout);
 
     /// Is this amount compatible with other client in the pool?
-    bool IsCompatibleWithSession(CAmount nAmount, CTransaction txCollateral, int &errorID);
+    bool IsCompatibleWithSession(int nDenom, CTransaction txCollateral, int &errorID);
 
     /// Passively run Darksend in the background according to the configuration in settings
     bool DoAutomaticDenominating(bool fDryRun=false);
@@ -514,7 +514,11 @@ public:
 
     /// Split up large inputs or make fee sized inputs
     bool MakeCollateralAmounts();
-    bool CreateDenominated(CAmount nTotalValue);
+    bool MakeCollateralAmounts(const CompactTallyItem& tallyItem);
+    /// Create denominations
+    bool CreateDenominated();
+    bool CreateDenominated(const CompactTallyItem& tallyItem);
+
 
     /// Get the denominations for a list of outputs (returns a bitshifted integer)
     int GetDenominations(const std::vector<CTxOut>& vout, bool fSingleRandomDenom = false);

src/wallet/wallet.cpp

@@ -2014,6 +2014,30 @@ CAmount CWallet::GetNormalizedAnonymizedBalance() const
     return nTotal;
 }
 
+CAmount CWallet::GetNeedsToBeAnonymizedBalance(CAmount nMinBalance) const
+{
+    if(fLiteMode) return 0;
+
+    CAmount nAnonymizedBalance = GetAnonymizedBalance();
+    CAmount nNeedsToAnonymizeBalance = nAnonymizeDashAmount*COIN - nAnonymizedBalance;
+
+    // try to overshoot target DS balance up to nMinBalance
+    nNeedsToAnonymizeBalance += nMinBalance;
+
+    CAmount nAnonymizableBalance = GetAnonymizableBalance();
+
+    // anonymizable balance is way too small
+    if(nAnonymizableBalance < nMinBalance) return 0;
+
+    // not enough funds to anonymze amount we want, try the max we can
+    if(nNeedsToAnonymizeBalance > nAnonymizableBalance) nNeedsToAnonymizeBalance = nAnonymizableBalance;
+
+    // we should never exceed the pool max
+    if(nNeedsToAnonymizeBalance > DARKSEND_POOL_MAX) nNeedsToAnonymizeBalance = DARKSEND_POOL_MAX;
+
+    return nNeedsToAnonymizeBalance;
+}
+
 CAmount CWallet::GetDenominatedBalance(bool unconfirmed) const
 {
     if(fLiteMode) return 0;
@@ -2585,6 +2609,70 @@ bool CWallet::SelectCoinsByDenominations(int nDenom, CAmount nValueMin, CAmount
     return (nValueRet >= nValueMin && fFound100 && fFound10 && fFound1 && fFoundDot1);
 }
 
+struct CompareByAmount
+{
+    bool operator()(const CompactTallyItem& t1,
+                    const CompactTallyItem& t2) const
+    {
+        return t1.nAmount > t2.nAmount;
+    }
+};
+
+bool CWallet::SelectCoinsGrouppedByAddresses(std::vector<CompactTallyItem>& vecTallyRet, bool fSkipDenominated)
+{
+    LOCK2(cs_main, cs_wallet);
+
+    int nMinDepth = 1;
+    isminefilter filter = ISMINE_SPENDABLE;
+
+    // Tally
+    map<CBitcoinAddress, CompactTallyItem> mapTally;
+    for (map<uint256, CWalletTx>::iterator it = mapWallet.begin(); it != mapWallet.end(); ++it) {
+        const CWalletTx& wtx = (*it).second;
+
+        if(!CheckFinalTx(wtx)) continue;
+        if(wtx.GetDepthInMainChain(false) < nMinDepth) continue;
+        if(wtx.IsCoinBase() && wtx.GetBlocksToMaturity() > 0) continue;
+
+        for (unsigned int i = 0; i < wtx.vout.size(); i++) {
+            CTxDestination address;
+            if (!ExtractDestination(wtx.vout[i].scriptPubKey, address)) continue;
+
+            isminefilter mine = ::IsMine(*this, address);
+            if(!(mine & filter)) continue;
+
+            if(IsSpent(wtx.GetHash(), i) || IsLockedCoin(wtx.GetHash(), i)) continue;
+            if(IsCollateralAmount(wtx.vout[i].nValue)) continue;
+            if(fMasterNode && wtx.vout[i].nValue == 1000*COIN) continue;
+            if(fSkipDenominated && IsDenominatedAmount(wtx.vout[i].nValue)) continue;
+
+            CompactTallyItem& item = mapTally[address];
+            item.address = address;
+            item.nAmount += wtx.vout[i].nValue;
+            item.vecTxIn.push_back(CTxIn(wtx.GetHash(), i));
+        }
+    }
+
+    // we found nothing
+    if(mapTally.size() == 0) return false;
+
+    // construct resulting vector
+    BOOST_FOREACH(const PAIRTYPE(CBitcoinAddress, CompactTallyItem)& item, mapTally) {
+        vecTallyRet.push_back(item.second);
+    }
+
+    // order by amounts per address, from smallest to largest
+    sort(vecTallyRet.rbegin(), vecTallyRet.rend(), CompareByAmount());
+
+    // debug
+    std::string strMessage = "SelectCoinsGrouppedByAddresses - vecTallyRet:\n";
+    BOOST_FOREACH(CompactTallyItem& item, vecTallyRet)
+        strMessage += strprintf("  %s %f\n", item.address.ToString().c_str(), float(item.nAmount)/COIN);
+    LogPrint("selectcoins", "%s", strMessage);
+
+    return true;
+}
+
 bool CWallet::SelectCoinsDark(CAmount nValueMin, CAmount nValueMax, std::vector<CTxIn>& setCoinsRet, CAmount& nValueRet, int nPrivateSendRoundsMin, int nPrivateSendRoundsMax) const
 {
     CCoinControl *coinControl=NULL;

src/wallet/wallet.h

@@ -8,6 +8,7 @@
 #define BITCOIN_WALLET_WALLET_H
 
 #include "amount.h"
+#include "base58.h"
 #include "streams.h"
 #include "tinyformat.h"
 #include "ui_interface.h"
@@ -99,6 +100,16 @@ enum AvailableCoinsType
     ONLY_1000 = 5 // find masternode outputs including locked ones (use with caution)
 };
 
+struct CompactTallyItem
+{
+    CBitcoinAddress address;
+    CAmount nAmount;
+    std::vector<CTxIn> vecTxIn;
+    CompactTallyItem()
+    {
+        nAmount = 0;
+    }
+};
 
 /** A key pool entry */
 class CKeyPool
@@ -636,6 +647,7 @@ public:
     bool SelectCoinsByDenominations(int nDenom, CAmount nValueMin, CAmount nValueMax, std::vector<CTxIn>& vCoinsRet, std::vector<COutput>& vCoinsRet2, CAmount& nValueRet, int nPrivateSendRoundsMin, int nPrivateSendRoundsMax);
     bool SelectCoinsCollateral(std::vector<CTxIn>& setCoinsRet, CAmount& nValueRet) const ;
     bool SelectCoinsDark(CAmount nValueMin, CAmount nValueMax, std::vector<CTxIn>& setCoinsRet, CAmount& nValueRet, int nPrivateSendRoundsMin, int nPrivateSendRoundsMax) const;
+    bool SelectCoinsGrouppedByAddresses(std::vector<CompactTallyItem>& vecTallyRet, bool fSkipDenominated = true);
 
     /// Get 1000DASH input that can be used for the Masternode
     bool GetMasternodeVinAndKeys(CTxIn& vinRet, CPubKey& pubKeyRet, CKey& keyRet, std::string strTxHash = "", std::string strOutputIndex = "");
@@ -734,6 +746,7 @@ public:
     CAmount GetAnonymizedBalance() const;
     double GetAverageAnonymizedRounds() const;
     CAmount GetNormalizedAnonymizedBalance() const;
+    CAmount GetNeedsToBeAnonymizedBalance(CAmount nMinBalance = 0) const;
     CAmount GetDenominatedBalance(bool unconfirmed=false) const;
 
     bool GetBudgetSystemCollateralTX(CTransaction& tx, uint256 hash, bool useIX);