Limit setAskFor and retire requested entries only when a getdata returns.

gmaxwell · furszy · commit 74d0482c2297 · 2021-05-08T12:51:31.000-03:00
The setAskFor duplicate elimination was too eager and removed entries
 when we still had no getdata response, allowing the peer to keep
 INVing and not responding.
diff --git a/src/net.cpp b/src/net.cpp
@@ -2487,9 +2487,9 @@ CNode::~CNode()
 
 void CNode::AskFor(const CInv& inv)
 {
-    if (mapAskFor.size() > MAPASKFOR_MAX_SZ)
+    if (mapAskFor.size() > MAPASKFOR_MAX_SZ || setAskFor.size() > SETASKFOR_MAX_SZ)
         return;
-    // a peer may not occupy multiple positions in an inv's request queue
+    // a peer may not have multiple non-responded queue positions for a single inv item
     if (!setAskFor.insert(inv.hash).second)
         return;
 
diff --git a/src/net.h b/src/net.h
@@ -69,6 +69,8 @@ static const bool DEFAULT_UPNP = false;
 #endif
 /** The maximum number of entries in mapAskFor */
 static const size_t MAPASKFOR_MAX_SZ = MAX_INV_SZ;
+/** The maximum number of entries in setAskFor (larger due to getdata latency)*/
+static const size_t SETASKFOR_MAX_SZ = 2 * MAX_INV_SZ;
 /** The maximum number of peer connections to maintain. */
 static const unsigned int DEFAULT_MAX_PEER_CONNECTIONS = 125;
 /** Disconnected peers are added to setOffsetDisconnectedPeers only if node has less than ENOUGH_CONNECTIONS */
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
@@ -1508,6 +1508,7 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR
         bool fMissingInputs = false;
         CValidationState state;
 
+        pfrom->setAskFor.erase(inv.hash);
         mapAlreadyAskedFor.erase(inv);
 
         if (ptx->ContainsZerocoins()) {
@@ -2370,8 +2371,10 @@ bool SendMessages(CNode* pto, CConnman& connman, std::atomic<bool>& interruptMsg
                     connman.PushMessage(pto, msgMaker.Make(NetMsgType::GETDATA, vGetData));
                     vGetData.clear();
                 }
+            } else {
+                //If we're not going to ask, don't expect a response.
+                pto->setAskFor.erase(inv.hash);
             }
-            pto->setAskFor.erase(inv.hash);
             pto->mapAskFor.erase(pto->mapAskFor.begin());
         }
         if (!vGetData.empty())

Original file line number	Diff line number	Diff line change
`@@ -1508,6 +1508,7 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR`
`1508`	`1508`	`bool fMissingInputs = false;`
`1509`	`1509`	`CValidationState state;`
`1510`	`1510`
	`1511`	`+ pfrom->setAskFor.erase(inv.hash);`
`1511`	`1512`	`mapAlreadyAskedFor.erase(inv);`
`1512`	`1513`
`1513`	`1514`	`if (ptx->ContainsZerocoins()) {`
`@@ -2370,8 +2371,10 @@ bool SendMessages(CNode* pto, CConnman& connman, std::atomic<bool>& interruptMsg`
`2370`	`2371`	`connman.PushMessage(pto, msgMaker.Make(NetMsgType::GETDATA, vGetData));`
`2371`	`2372`	`vGetData.clear();`
`2372`	`2373`	`}`
	`2374`	`+ } else {`
	`2375`	`+ //If we're not going to ask, don't expect a response.`
	`2376`	`+ pto->setAskFor.erase(inv.hash);`
`2373`	`2377`	`}`
`2374`		`- pto->setAskFor.erase(inv.hash);`
`2375`	`2378`	`pto->mapAskFor.erase(pto->mapAskFor.begin());`
`2376`	`2379`	`}`
`2377`	`2380`	`if (!vGetData.empty())`