From 4b6af8f2c1522a732abd6a836bec530b68fb43fa Mon Sep 17 00:00:00 2001
From: UdjinM6 <UdjinM6@users.noreply.github.com>
Date: Thu, 17 Oct 2019 12:36:18 +0300
Subject: [PATCH] Few fixes related to SelectCoinsGroupedByAddresses (#3144)

* Fix cache usage in SelectCoinsGroupedByAddresses

* Reset cache flags in SelectCoinsGroupedByAddresses when a block is (dis)connected

* MakeCollateralAmounts should call SelectCoinsGroupedByAddresses with a limited number of inputs
---
 src/privatesend/privatesend-client.cpp |  6 +++++-
 src/wallet/wallet.cpp                  | 18 ++++++++++++++----
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/src/privatesend/privatesend-client.cpp b/src/privatesend/privatesend-client.cpp
index 19721214f..f93a88dba 100644
--- a/src/privatesend/privatesend-client.cpp
+++ b/src/privatesend/privatesend-client.cpp
@@ -1367,8 +1367,12 @@ bool CPrivateSendClientSession::MakeCollateralAmounts(CConnman& connman)
 {
     if (!privateSendClient.fEnablePrivateSend || !privateSendClient.fPrivateSendRunning) return false;
 
+    // NOTE: We do not allow txes larger than 100kB, so we have to limit number of inputs here.
+    // We still want to consume a lot of inputs to avoid creating only smaller denoms though.
+    // Knowing that each CTxIn is at least 148b big, 400 inputs should take 400 x ~148b = ~60kB.
+    // This still leaves more than enough room for another data of typical MakeCollateralAmounts tx.
     std::vector<CompactTallyItem> vecTally;
-    if (!vpwallets[0]->SelectCoinsGroupedByAddresses(vecTally, false, false)) {
+    if (!vpwallets[0]->SelectCoinsGroupedByAddresses(vecTally, false, false, true, 400)) {
         LogPrint(BCLog::PRIVATESEND, "CPrivateSendClientSession::MakeCollateralAmounts -- SelectCoinsGroupedByAddresses can't find any inputs!\n");
         return false;
     }
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 075b0eb8e..c3f346f22 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1473,6 +1473,10 @@ void CWallet::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const
     }
 
     hashPrevBestCoinbase = pblock->vtx[0]->GetHash();
+
+    // reset cache to make sure no longer immature coins are included
+    fAnonymizableTallyCached = false;
+    fAnonymizableTallyCachedNonDenom = false;
 }
 
 void CWallet::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexDisconnected) {
@@ -1482,6 +1486,10 @@ void CWallet::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, con
         // NOTE: do NOT pass pindex here
         SyncTransaction(ptx);
     }
+
+    // reset cache to make sure no longer mature coins are excluded
+    fAnonymizableTallyCached = false;
+    fAnonymizableTallyCachedNonDenom = false;
 }
 
 
@@ -3277,8 +3285,9 @@ bool CWallet::SelectCoinsGroupedByAddresses(std::vector<CompactTallyItem>& vecTa
 
     isminefilter filter = ISMINE_SPENDABLE;
 
-    // try to use cache for already confirmed anonymizable inputs, no cache should be used when the limit is specified
-    if(nMaxOupointsPerAddress != -1 && fAnonymizable && fSkipUnconfirmed) {
+    // Try using the cache for already confirmed anonymizable inputs.
+    // This should only be used if nMaxOupointsPerAddress was NOT specified.
+    if(nMaxOupointsPerAddress == -1 && fAnonymizable && fSkipUnconfirmed) {
         if(fSkipDenominated && fAnonymizableTallyCachedNonDenom) {
             vecTallyRet = vecAnonymizableTallyCachedNonDenom;
             LogPrint(BCLog::SELECTCOINS, "SelectCoinsGroupedByAddresses - using cache for non-denom inputs %d\n", vecTallyRet.size());
@@ -3350,8 +3359,9 @@ bool CWallet::SelectCoinsGroupedByAddresses(std::vector<CompactTallyItem>& vecTa
         vecTallyRet.push_back(item.second);
     }
 
-    // cache already confirmed anonymizable entries for later use, no cache should be saved when the limit is specified
-    if(nMaxOupointsPerAddress != -1 && fAnonymizable && fSkipUnconfirmed) {
+    // Cache already confirmed anonymizable entries for later use.
+    // This should only be used if nMaxOupointsPerAddress was NOT specified.
+    if(nMaxOupointsPerAddress == -1 && fAnonymizable && fSkipUnconfirmed) {
         if(fSkipDenominated) {
             vecAnonymizableTallyCachedNonDenom = vecTallyRet;
             fAnonymizableTallyCachedNonDenom = true;