From 0c8f638c0836d3c89bc57af225ad13631293d756 Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Wed, 16 Oct 2019 16:10:18 +0200 Subject: [PATCH 1/4] Add missing "notfound" and "getsporks" to messagemap (#3146) --- test/functional/test_framework/mininode.py | 2 ++ 1 file changed, 2 insertions(+) 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, From a09821b48dc21332a60eb39540bdf3999a65e8c4 Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Wed, 16 Oct 2019 16:10:36 +0200 Subject: [PATCH 2/4] Replace vecAskFor with a priority queue (#3147) This avoids sorting before looping through it to figure out what to request. The assumption that sorting would be cheap when vecAskFor is already mostly sorted (only unsorted at the end) turned out to be false. In reality, ~50% of CPU time was consumed by the sort when a lot of traffic (thousands of TXs) happen. --- src/net.cpp | 21 +++++++++++---------- src/net.h | 4 +++- src/net_processing.cpp | 16 ++++++++++------ 3 files changed, 24 insertions(+), 17 deletions(-) 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()); From 55f1c883676ac6508ec7d4bc4f5bfb5a7ddb537e Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Thu, 17 Oct 2019 12:36:18 +0300 Subject: [PATCH 3/4] 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 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..9e2dbf7e44 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; } @@ -3277,8 +3285,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 +3359,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; From 934f1e704ce43747b83adfc61694d79b9aa0b806 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Thu, 17 Oct 2019 12:36:35 +0300 Subject: [PATCH 4/4] Partially revert 3061 (#3150) Turned out that this causes SelectCoinsMinConf to (unnecessary) lean towards selecting mempool txes which can cause "too long mempool chain" error even when there are more funds to spend. --- src/wallet/wallet.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 9e2dbf7e44..5272629f35 100755 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2961,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++) { @@ -2982,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;