Implement safer version of CWallet::PrepareDarksendDenominate:

Make outputs by looping through denominations: try to add every needed denomination, repeat up to 5-10 times.
This way we can be pretty sure that it should have at least one of each needed denomination.
We also do not care about full amount as long as we have right denominations, just pass what we found.

src/wallet.cpp

@@ -2318,6 +2318,7 @@ string CWallet::PrepareDarksendDenominate(int minRounds, int maxRounds)
 
     // ** find the coins we'll use
     std::vector<CTxIn> vCoins;
+    std::vector<CTxIn> vCoinsResult;
     std::vector<COutput> vCoins2;
     int64_t nValueIn = 0;
     CReserveKey reservekey(this);
@@ -2329,58 +2330,31 @@ string CWallet::PrepareDarksendDenominate(int minRounds, int maxRounds)
     */
     if(minRounds >= 0){
         if (!SelectCoinsByDenominations(darkSendPool.sessionDenom, 0.1*COIN, DARKSEND_POOL_MAX, vCoins, vCoins2, nValueIn, minRounds, maxRounds))
-            return _("Insufficient funds");
+            return _("Error: can't select current denominated inputs");
     }
 
-    // calculate total value out
+    LogPrintf("PrepareDarksendDenominate - preparing darksend denominate . Got: %d \n", nValueIn);
-    int64_t nTotalValue = GetTotalValue(vCoins);
-    LogPrintf("PrepareDarksendDenominate - preparing darksend denominate . Got: %d \n", nTotalValue);
 
     BOOST_FOREACH(CTxIn v, vCoins) 
         LockCoin(v.prevout);
 
-    // denominate our funds
+    int64_t nValueLeft = nValueIn;
-    int64_t nValueLeft = nTotalValue;
     std::vector<CTxOut> vOut;
-    std::vector<int64_t> vDenoms;
 
     /*
         TODO: Front load with needed denominations (e.g. .1, 1 )
     */
 
-    /*
+    // Make outputs by looping through denominations: try to add every needed denomination, repeat up to 5-10 times.
-        Add all denominations once
+    // This way we can be pretty sure that it should have at least one of each needed denomination.
+    // NOTE: No need to randomize order of inputs because they were
+    // initially shuffled in CWallet::SelectCoinsByDenominations already.
+    int nStep = 0;
+    int nStepsMax = 5 + GetRandInt(5);
+    while(nStep < nStepsMax) {
 
-        The beginning of the list is front loaded with each possible
+        BOOST_FOREACH(int64_t v, darkSendDenominations){
-        denomination in random order. This means we'll at least get 1
+            // only use the ones that are approved
-        of each that is required as outputs.
-    */
-    BOOST_FOREACH(int64_t d, darkSendDenominations){
-        vDenoms.push_back(d);
-        vDenoms.push_back(d);
-    }
-
-    //randomize the order of these denominations
-    std::random_shuffle (vDenoms.begin(), vDenoms.end());
-
-    /*
-        Build a long list of denominations
-
-        Next we'll build a long random list of denominations to add.
-        Eventually as the algorithm goes through these it'll find the ones
-        it nees to get exact change.
-    */
-    for(int i = 0; i <= 500; i++)
-        BOOST_FOREACH(int64_t d, darkSendDenominations)
-            vDenoms.push_back(d);
-
-    //randomize the order of inputs we get back
-    std::random_shuffle (vDenoms.begin() + (int)darkSendDenominations.size() + 1, vDenoms.end());
-
-    // Make outputs by looping through denominations randomly
-
-    BOOST_REVERSE_FOREACH(int64_t v, vDenoms){
-        //only use the ones that are approved
             bool fAccepted = false;
             if((darkSendPool.sessionDenom & (1 << 0))      && v == ((100*COIN) +100000)) {fAccepted = true;}
             else if((darkSendPool.sessionDenom & (1 << 1)) && v == ((10*COIN)  +10000)) {fAccepted = true;}
@@ -2388,70 +2362,63 @@ string CWallet::PrepareDarksendDenominate(int minRounds, int maxRounds)
             else if((darkSendPool.sessionDenom & (1 << 3)) && v == ((.1*COIN)  +100)) {fAccepted = true;}
             if(!fAccepted) continue;
 
-        int nOutputs = 0;
+            // try to add it
+            if(nValueLeft - v >= 0) {
+                // Note: this relies on a fact that both vectors MUST have same size
+                std::vector<CTxIn>::iterator it = vCoins.begin();
+                std::vector<COutput>::iterator it2 = vCoins2.begin();
+                while(it2 != vCoins2.end()) {
+                    // we have matching inputs
+                    if((*it2).tx->vout[(*it2).i].nValue == v) {
+                        // add new input in resulting vector
+                        vCoinsResult.push_back(*it);
+                        // remove corresponting items from initial vectors
+                        vCoins.erase(it);
+                        vCoins2.erase(it2);
 
-        // add each output up to 10 times until it can't be added again
-        while(nValueLeft - v >= 0 && nOutputs <= 10) {
                         CScript scriptChange;
                         CPubKey vchPubKey;
-            //use a unique change address
+                        // use a unique change address
                         assert(reservekey.GetReservedKey(vchPubKey)); // should never fail, as we just unlocked
                         scriptChange = GetScriptForDestination(vchPubKey.GetID());
                         reservekey.KeepKey();
 
+                        // add new output
                         CTxOut o(v, scriptChange);
                         vOut.push_back(o);
 
-            //increment outputs and subtract denomination amount
+                        // subtract denomination amount
-            nOutputs++;
                         nValueLeft -= v;
+
+                        break;
                     }
+                    ++it;
+                    ++it2;
+                }
+            }
+        }
+
+        nStep++;
 
         if(nValueLeft == 0) break;
     }
 
-    //back up mode , incase we couldn't successfully make the outputs for some reason
+    // unlock unused coins
-    if(vOut.size() > 40 || darkSendPool.GetDenominations(vOut) != darkSendPool.sessionDenom || nValueLeft != 0){
+    BOOST_FOREACH(CTxIn v, vCoins)
-        vOut.clear();
+        UnlockCoin(v.prevout);
-        nValueLeft = nTotalValue;
-
-        // Make outputs by looping through denominations, from small to large
-
-        BOOST_FOREACH(const COutput& out, vCoins2){
-            CScript scriptChange;
-            CPubKey vchPubKey;
-            //use a unique change address
-            assert(reservekey.GetReservedKey(vchPubKey)); // should never fail, as we just unlocked
-            scriptChange = GetScriptForDestination(vchPubKey.GetID());
-            reservekey.KeepKey();
-
-            CTxOut o(out.tx->vout[out.i].nValue, scriptChange);
-            vOut.push_back(o);
-
-            //increment outputs and subtract denomination amount
-            nValueLeft -= out.tx->vout[out.i].nValue;
-
-            if(nValueLeft == 0) break;
-        }
-    }
 
     if(darkSendPool.GetDenominations(vOut) != darkSendPool.sessionDenom) {
-        BOOST_FOREACH(CTxIn v, vCoins)
+        // unlock used coins on failure
+        BOOST_FOREACH(CTxIn v, vCoinsResult)
             UnlockCoin(v.prevout);
         return "Error: can't make current denominated outputs";
     }
 
-    // we don't support change at all
+    // randomize the output order
-    if(nValueLeft != 0) {
-        BOOST_FOREACH(CTxIn v, vCoins)
-            UnlockCoin(v.prevout);
-        return "Error: change left-over in pool. Must use denominations only";
-    }
-
-    //randomize the output order
     std::random_shuffle (vOut.begin(), vOut.end());
 
-    darkSendPool.SendDarksendDenominate(vCoins, vOut, nValueIn);
+    // We also do not care about full amount as long as we have right denominations, just pass what we found
+    darkSendPool.SendDarksendDenominate(vCoinsResult, vOut, nValueIn - nValueLeft);
 
     return "";
 }