diff --git a/src/net.cpp b/src/net.cpp index 9e427b8229..eacce69110 100755 --- a/src/net.cpp +++ b/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& 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) diff --git a/src/net.h b/src/net.h index 5b2cbd658d..e6d1d9a9b2 100755 --- a/src/net.h +++ b/src/net.h @@ -32,6 +32,7 @@ #include #include #include +#include #ifndef WIN32 #include @@ -843,7 +844,8 @@ class CNode std::vector vInventoryOtherToSend; CCriticalSection cs_inventory; std::unordered_set setAskFor; - std::vector> vecAskFor; + std::unordered_set setAskForInQueue; + std::priority_queue, std::vector>, std::greater<>> queueAskFor; int64_t nNextInvSend; // Used for headers announcements - unfiltered blocks to relay // Also protected by cs_inventory diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 6cca6d6c3a..2a83066dbc 100755 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3964,11 +3964,15 @@ bool PeerLogicValidation::SendMessages(CNode* pto, std::atomic& 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& 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()); diff --git a/src/privatesend/privatesend-client.cpp b/src/privatesend/privatesend-client.cpp index 2df6ec3a81..47078c3609 100755 --- 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 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 6cbdda84c9..5272629f35 100755 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1473,6 +1473,10 @@ void CWallet::BlockConnected(const std::shared_ptr& 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& pblock, const CBlockIndex* pindexDisconnected) { @@ -1482,6 +1486,10 @@ void CWallet::BlockDisconnected(const std::shared_ptr& 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& 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& 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; diff --git a/test/functional/test_framework/mininode.py b/test/functional/test_framework/mininode.py index a15902817e..8b215aee20 100644 --- a/test/functional/test_framework/mininode.py +++ b/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,