Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Limited mapAlreadyAskedFor #2423

Merged
merged 3 commits into from

7 participants

@TheBlueMatt

Based on #2409 (though I dont think it needs it).
Just puts a limit on how large mapAlreadyAskedFor can grow.

@sipa
Owner

@TheBlueMatt Can you drop the #2409 included code, or switch to its latest version? They should combine cleanly anyway.

@TheBlueMatt

No longer depends on #2409

@rebroad

How about separating askedfor blocks and askedfor transactions? That way, one growing large won't affect downloads of the other.

@TheBlueMatt

@rebroad meh, that just sounds like useless duplication...in that case, anyone will just send block invs so that you re-request blocks more often, sounds like a better way to let an attacker be more targeted.

TheBlueMatt added some commits
@TheBlueMatt TheBlueMatt Add a limitedmap class similar to mruset 5ffc299
@TheBlueMatt TheBlueMatt Revert "Actually use mapAlreadyAskedFor."
This reverts commit 643160f.

Turns out this commit was useless after a more careful reading of
CNode::AskFor
eb59c4c
@TheBlueMatt TheBlueMatt Move mapAlreadyAskedFor to limitedmap
This will result in re-requesting invs if we are under heavy inv
load, however as long as we get no more than 16,000 invs in two
minutes, this should have no effect on runtime behavior.
b5afda6
@TheBlueMatt

OK, switched to MAX_INV_SZ for the map's limit...Im not a huge fan of such a big number, but since its already a protocol rule it makes more sense to use that.

@gmaxwell
Owner

This has passed a couple days of in-valgrind production testing, but I think the new limited map needs some unit tests. OTOH, I'd like this merged ASAP for previously discussed reasons.

@sipa
Owner

If my understanding of how maps are represented at runtime in STL is correct, this should mean at most ~6.2MB of memory for 50000 invs (with 8-byte pointers, with 4-byte pointers is 4.4MB).

@jgarzik
Owner

@gmaxwell what are the previously discussed reasons? I don't see them in this pull.

@sipa
Owner

ACK, but unit tests for the map would be nice indeed. Has been running for several days for me without problems (together with #2419 #2418 #2422 #2420 #2409)

@BitcoinPullTester

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/b5afda67f2846ddc6554304acc1567796733725b for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@gavinandresen gavinandresen merged commit aaf47ea into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 1, 2013
  1. @TheBlueMatt
  2. @TheBlueMatt

    Revert "Actually use mapAlreadyAskedFor."

    TheBlueMatt authored
    This reverts commit 643160f.
    
    Turns out this commit was useless after a more careful reading of
    CNode::AskFor
  3. @TheBlueMatt

    Move mapAlreadyAskedFor to limitedmap

    TheBlueMatt authored
    This will result in re-requesting invs if we are under heavy inv
    load, however as long as we get no more than 16,000 invs in two
    minutes, this should have no effect on runtime behavior.
This page is out of date. Refresh to see the latest.
Showing with 113 additions and 4 deletions.
  1. +100 −0 src/limitedmap.h
  2. +0 −1  src/main.cpp
  3. +1 −1  src/net.cpp
  4. +12 −2 src/net.h
View
100 src/limitedmap.h
@@ -0,0 +1,100 @@
+// Copyright (c) 2012 The Bitcoin developers
+// Distributed under the MIT/X11 software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+#ifndef BITCOIN_LIMITEDMAP_H
+#define BITCOIN_LIMITEDMAP_H
+
+#include <map>
+#include <deque>
+
+/** STL-like map container that only keeps the N elements with the highest value. */
+template <typename K, typename V> class limitedmap
+{
+public:
+ typedef K key_type;
+ typedef V mapped_type;
+ typedef std::pair<const key_type, mapped_type> value_type;
+ typedef typename std::map<K, V>::const_iterator const_iterator;
+ typedef typename std::map<K, V>::size_type size_type;
+
+protected:
+ std::map<K, V> map;
+ typedef typename std::map<K, V>::iterator iterator;
+ std::multimap<V, iterator> rmap;
+ typedef typename std::multimap<V, iterator>::iterator rmap_iterator;
+ size_type nMaxSize;
+
+public:
+ limitedmap(size_type nMaxSizeIn = 0) { nMaxSize = nMaxSizeIn; }
+ const_iterator begin() const { return map.begin(); }
+ const_iterator end() const { return map.end(); }
+ size_type size() const { return map.size(); }
+ bool empty() const { return map.empty(); }
+ const_iterator find(const key_type& k) const { return map.find(k); }
+ size_type count(const key_type& k) const { return map.count(k); }
+ void insert(const value_type& x)
+ {
+ std::pair<iterator, bool> ret = map.insert(x);
+ if (ret.second)
+ {
+ if (nMaxSize && map.size() == nMaxSize)
+ {
+ map.erase(rmap.begin()->second);
+ rmap.erase(rmap.begin());
+ }
+ rmap.insert(make_pair(x.second, ret.first));
+ }
+ return;
+ }
+ void erase(const key_type& k)
+ {
+ iterator itTarget = map.find(k);
+ if (itTarget == map.end())
+ return;
+ std::pair<rmap_iterator, rmap_iterator> itPair = rmap.equal_range(itTarget->second);
+ for (rmap_iterator it = itPair.first; it != itPair.second; ++it)
+ if (it->second == itTarget)
+ {
+ rmap.erase(it);
+ map.erase(itTarget);
+ return;
+ }
+ // Shouldn't ever get here
+ assert(0); //TODO remove me
+ map.erase(itTarget);
+ }
+ void update(const_iterator itIn, const mapped_type& v)
+ {
+ //TODO: When we switch to C++11, use map.erase(itIn, itIn) to get the non-const iterator
+ iterator itTarget = map.find(itIn->first);
+ if (itTarget == map.end())
+ return;
+ std::pair<rmap_iterator, rmap_iterator> itPair = rmap.equal_range(itTarget->second);
+ for (rmap_iterator it = itPair.first; it != itPair.second; ++it)
+ if (it->second == itTarget)
+ {
+ rmap.erase(it);
+ itTarget->second = v;
+ rmap.insert(make_pair(v, itTarget));
+ return;
+ }
+ // Shouldn't ever get here
+ assert(0); //TODO remove me
+ itTarget->second = v;
+ rmap.insert(make_pair(v, itTarget));
+ }
+ size_type max_size() const { return nMaxSize; }
+ size_type max_size(size_type s)
+ {
+ if (s)
+ while (map.size() > s)
+ {
+ map.erase(rmap.begin()->second);
+ rmap.erase(rmap.begin());
+ }
+ nMaxSize = s;
+ return nMaxSize;
+ }
+};
+
+#endif
View
1  src/main.cpp
@@ -3994,7 +3994,6 @@ bool SendMessages(CNode* pto, bool fSendTrickle)
pto->PushMessage("getdata", vGetData);
vGetData.clear();
}
- mapAlreadyAskedFor[inv] = nNow;
}
pto->mapAskFor.erase(pto->mapAskFor.begin());
}
View
2  src/net.cpp
@@ -63,7 +63,7 @@ CCriticalSection cs_vNodes;
map<CInv, CDataStream> mapRelay;
deque<pair<int64, CInv> > vRelayExpiration;
CCriticalSection cs_mapRelay;
-map<CInv, int64> mapAlreadyAskedFor;
+limitedmap<CInv, int64> mapAlreadyAskedFor(MAX_INV_SZ);
static deque<string> vOneShots;
CCriticalSection cs_vOneShots;
View
14 src/net.h
@@ -15,6 +15,7 @@
#endif
#include "mruset.h"
+#include "limitedmap.h"
#include "netbase.h"
#include "protocol.h"
#include "addrman.h"
@@ -100,7 +101,7 @@ extern CCriticalSection cs_vNodes;
extern std::map<CInv, CDataStream> mapRelay;
extern std::deque<std::pair<int64, CInv> > vRelayExpiration;
extern CCriticalSection cs_mapRelay;
-extern std::map<CInv, int64> mapAlreadyAskedFor;
+extern limitedmap<CInv, int64> mapAlreadyAskedFor;
extern std::vector<std::string> vAddedNodes;
extern CCriticalSection cs_vAddedNodes;
@@ -367,7 +368,12 @@ class CNode
{
// We're using mapAskFor as a priority queue,
// the key is the earliest time the request can be sent
- int64& nRequestTime = mapAlreadyAskedFor[inv];
+ int64 nRequestTime;
+ limitedmap<CInv, int64>::const_iterator it = mapAlreadyAskedFor.find(inv);
+ if (it != mapAlreadyAskedFor.end())
+ nRequestTime = it->second;
+ else
+ nRequestTime = 0;
if (fDebugNet)
printf("askfor %s %"PRI64d" (%s)\n", inv.ToString().c_str(), nRequestTime, DateTimeStrFormat("%H:%M:%S", nRequestTime/1000000).c_str());
@@ -380,6 +386,10 @@ class CNode
// Each retry is 2 minutes after the last
nRequestTime = std::max(nRequestTime + 2 * 60 * 1000000, nNow);
+ if (it != mapAlreadyAskedFor.end())
+ mapAlreadyAskedFor.update(it, nRequestTime);
+ else
+ mapAlreadyAskedFor.insert(std::make_pair(inv, nRequestTime));
mapAskFor.insert(std::make_pair(nRequestTime, inv));
}
Something went wrong with that request. Please try again.