Backport SelectCoinsMinConf fix, queueAskFor, messagemap edit, SelectCoinsGroupedByAddresses fixes

src/net.cpp

@@ -3221,11 +3221,11 @@ CNode::~CNode()
 
 void CNode::AskFor(const CInv& inv, int64_t doubleRequestDelay)
 {
-    if (vecAskFor.size() > MAPASKFOR_MAX_SZ || setAskFor.size() > SETASKFOR_MAX_SZ) {
+    if (queueAskFor.size() > MAPASKFOR_MAX_SZ || setAskFor.size() > SETASKFOR_MAX_SZ) {
         int64_t nNow = GetTime();
         if(nNow - nLastWarningTime > WARNING_INTERVAL) {
             LogPrintf("CNode::AskFor -- WARNING: inventory message dropped: vecAskFor.size = %d, setAskFor.size = %d, MAPASKFOR_MAX_SZ = %d, SETASKFOR_MAX_SZ = %d, nSkipped = %d, peer=%d\n",
-                      vecAskFor.size(), setAskFor.size(), MAPASKFOR_MAX_SZ, SETASKFOR_MAX_SZ, nNumWarningsSkipped, id);
+                      queueAskFor.size(), setAskFor.size(), MAPASKFOR_MAX_SZ, SETASKFOR_MAX_SZ, nNumWarningsSkipped, id);
             nLastWarningTime = nNow;
             nNumWarningsSkipped = 0;
         }
@@ -3235,10 +3235,10 @@ void CNode::AskFor(const CInv& inv, int64_t doubleRequestDelay)
         return;
     }
     // a peer may not have multiple non-responded queue positions for a single inv item
-    if (!setAskFor.insert(inv.hash).second)
+    if (!setAskFor.emplace(inv.hash).second)
         return;
 
-    // We're using vecAskFor as a priority queue,
+    // We're using queueAskFor as a priority queue,
     // the key is the earliest time the request can be sent
     int64_t nRequestTime;
     auto it = mapAlreadyAskedFor.find(inv.hash);
@@ -3262,16 +3262,17 @@ void CNode::AskFor(const CInv& inv, int64_t doubleRequestDelay)
         mapAlreadyAskedFor.update(it, nRequestTime);
     else
         mapAlreadyAskedFor.insert(std::make_pair(inv.hash, nRequestTime));
-    vecAskFor.emplace_back(nRequestTime, inv);
+
+    queueAskFor.emplace(nRequestTime, inv);
+    setAskForInQueue.emplace(inv.hash);
 }
 
 void CNode::RemoveAskFor(const uint256& hash)
 {
-    if (setAskFor.erase(hash)) {
-        vecAskFor.erase(std::remove_if(vecAskFor.begin(), vecAskFor.end(), [&](const std::pair<int64_t, CInv>& item) {
-            return item.second.hash == hash;
-        }), vecAskFor.end());
-    }
+    setAskFor.erase(hash);
+    // we don't really remove it from queueAskFor as it would be too expensive to rebuild the heap
+    // instead, we're ignoring the entry later as it won't be found in setAskForInQueue anymore
+    setAskForInQueue.erase(hash);
 }
 
 bool CConnman::NodeFullyConnected(const CNode* pnode)

src/net.h

@@ -32,6 +32,7 @@
 #include <memory>
 #include <condition_variable>
 #include <unordered_set>
+#include <queue>
 
 #ifndef WIN32
 #include <arpa/inet.h>
@@ -843,7 +844,8 @@ class CNode
     std::vector<CInv> vInventoryOtherToSend;
     CCriticalSection cs_inventory;
     std::unordered_set<uint256, StaticSaltedHasher> setAskFor;
-    std::vector<std::pair<int64_t, CInv>> vecAskFor;
+    std::unordered_set<uint256, StaticSaltedHasher> setAskForInQueue;
+    std::priority_queue<std::pair<int64_t, CInv>, std::vector<std::pair<int64_t, CInv>>, std::greater<>> queueAskFor;
     int64_t nNextInvSend;
     // Used for headers announcements - unfiltered blocks to relay
     // Also protected by cs_inventory

src/net_processing.cpp

@@ -3964,11 +3964,15 @@ bool PeerLogicValidation::SendMessages(CNode* pto, std::atomic<bool>& interruptM
         //
         // Message: getdata (non-blocks)
         //
-        std::sort(pto->vecAskFor.begin(), pto->vecAskFor.end());
-        auto it = pto->vecAskFor.begin();
-        while (it != pto->vecAskFor.end() && it->first <= nNow)
+        while (!pto->queueAskFor.empty() && pto->queueAskFor.top().first <= nNow)
         {
-            const CInv& inv = it->second;
+            const CInv& inv = pto->queueAskFor.top().second;
+            auto jt = pto->setAskForInQueue.find(inv.hash);
+            if (jt == pto->setAskForInQueue.end()) {
+                pto->queueAskFor.pop();
+                continue;
+            }
+
             if (!AlreadyHave(inv))
             {
                 LogPrint(BCLog::NET, "SendMessages -- GETDATA -- requesting inv = %s peer=%d\n", inv.ToString(), pto->GetId());
@@ -3984,9 +3988,9 @@ bool PeerLogicValidation::SendMessages(CNode* pto, std::atomic<bool>& interruptM
                 LogPrint(BCLog::NET, "SendMessages -- GETDATA -- already have inv = %s peer=%d\n", inv.ToString(), pto->GetId());
                 pto->setAskFor.erase(inv.hash);
             }
-            ++it;
+            pto->queueAskFor.pop();
+            pto->setAskForInQueue.erase(jt);
         }
-        pto->vecAskFor.erase(pto->vecAskFor.begin(), it);
         if (!vGetData.empty()) {
             connman->PushMessage(pto, msgMaker.Make(NetMsgType::GETDATA, vGetData));
             LogPrint(BCLog::NET, "SendMessages -- GETDATA -- pushed size = %lu peer=%d\n", vGetData.size(), pto->GetId());

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;
     }

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;
 }
 
 
@@ -2953,8 +2961,6 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin
         std::sort(vCoins.begin(), vCoins.end(), less_then_denom);
     }
 
-    int nMaxChainLength = std::min(gArgs.GetArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT), gArgs.GetArg("-limitdescendantcount", DEFAULT_DESCENDANT_LIMIT));
-
     // try to find nondenom first to prevent unneeded spending of mixed coins
     for (unsigned int tryDenom = tryDenomStart; tryDenom < 2; tryDenom++)
     {
@@ -2974,7 +2980,7 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMin
             if (output.nDepth < (pcoin->IsFromMe(ISMINE_ALL) ? nConfMine : nConfTheirs) && !fLockedByIS)
                 continue;
 
-            if (!mempool.TransactionWithinChainLimit(pcoin->GetHash(), fLockedByIS ? nMaxChainLength : nMaxAncestors))
+            if (!mempool.TransactionWithinChainLimit(pcoin->GetHash(), nMaxAncestors))
                 continue;
 
             int i = output.i;
@@ -3277,8 +3283,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 +3357,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;

test/functional/test_framework/mininode.py

@@ -1702,8 +1702,10 @@ class NodeConn(asyncore.dispatcher):
         b"mnlistdiff": msg_mnlistdiff,
         b"clsig": msg_clsig,
         b"islock": msg_islock,
+        b"notfound": None,
         b"senddsq": None,
         b"qsendrecsigs": None,
+        b"getsporks": None,
         b"spork": None,
         b"govsync": None,
         b"qfcommit": None,