dht: unify bootstrapSearch and refill method

Since bootstrapSearch was rewritten to use the node cache, it made it clear that
refill should be rewritten as such. However, then bootstrapSearch becomes
irrelevant.

An enhancement to refill is that it will let insertNode review its whole list
of nodes and then let it insert nodes up to SEARCH_NODES nodes. The previous
method would not garantee this behavior since, most of the times, it would pass
less than SEARCH_NODES nodes to insertNode.

include/opendht/dht.h

@@ -494,7 +494,15 @@ private:
     void announce(const InfoHash& id, sa_family_t af, std::shared_ptr<Value> value, DoneCallback callback, time_point created=time_point::max(), bool permanent = false);
     size_t listenTo(const InfoHash& id, sa_family_t af, GetCallback cb, Value::Filter f = Value::AllFilter(), const std::shared_ptr<Query>& q = {});
 
-    void bootstrapSearch(Search& sr);
+    /**
+     * Refill the search with good nodes if possible.
+     *
+     * @param sr  The search to refill.
+     *
+     * @return the number inserted nodes.
+     */
+    unsigned refill(Search& sr);
+
     Search *findSearch(unsigned short tid, sa_family_t af);
     void expireSearches();
 

include/opendht/network_engine.h

@@ -362,14 +362,14 @@ public:
 
     void blacklistNode(const std::shared_ptr<Node>& n);
 
-    std::vector<std::shared_ptr<Node>> getCachedNodes(const InfoHash& id, sa_family_t sa_f);
+    std::vector<std::shared_ptr<Node>> getCachedNodes(const InfoHash& id, sa_family_t sa_f, size_t count) {
+        return cache.getCachedNodes(id, sa_f, count);
+    }
 
 private:
     /***************
      *  Constants  *
      ***************/
-    /* Total number of node needed from cache */
-    static constexpr unsigned MAX_CACHE_NODE {14};
     static constexpr long unsigned MAX_REQUESTS_PER_SEC {1600};
     /* the length of a node info buffer in ipv4 format */
     static const constexpr size_t NODE4_INFO_BUF_LEN {26};

include/opendht/node_cache.h

@@ -29,7 +29,7 @@ namespace dht {
 struct NodeCache {
     std::shared_ptr<Node> getNode(const InfoHash& id, sa_family_t family);
     std::shared_ptr<Node> getNode(const InfoHash& id, const sockaddr* sa, socklen_t sa_len, time_point now, bool confirmed);
-    std::vector<std::shared_ptr<Node>> getCachedNodes(const InfoHash& id, sa_family_t sa_f, size_t total);
+    std::vector<std::shared_ptr<Node>> getCachedNodes(const InfoHash& id, sa_family_t sa_f, size_t count);
 
     /**
      * Reset the connectivity state of every node,

src/dht.cpp

@@ -329,7 +329,6 @@ struct Dht::Search {
      * @returns true if the node was not present and added to the search
      */
     bool insertNode(const std::shared_ptr<Node>& n, time_point now, const Blob& token={});
-    unsigned insertBucket(const Bucket&, time_point now);
 
     SearchNode* getNode(const std::shared_ptr<Node>& n) {
         auto srn = std::find_if(nodes.begin(), nodes.end(), [&](SearchNode& sn) {
@@ -395,8 +394,6 @@ struct Dht::Search {
 
     bool removeExpiredNode(time_point now);
 
-    unsigned refill(const RoutingTable&, time_point now);
-
     std::vector<std::shared_ptr<Node>> getNodes() const;
 
     void clear() {
@@ -844,13 +841,8 @@ Dht::searchStep(std::shared_ptr<Search> sr)
             sr->id.toString().c_str(), sr->af == AF_INET ? '4' : '6', sr->currentlySolicitedNodeCount());
     sr->step_time = now;
 
-    if (sr->refill_time + Node::NODE_EXPIRE_TIME < now and sr->nodes.size()-sr->getNumberOfBadNodes() < SEARCH_NODES) {
-        if (auto added = sr->refill(sr->af == AF_INET ? buckets : buckets6, now)) {
-            sr->refill_time = now;
-            DHT_LOG.DEBUG("[search %s IPv%c] refilled with %u nodes",
-                    sr->id.toString().c_str(), (sr->af == AF_INET) ? '4' : '6', added);
-        }
-    }
+    if (sr->refill_time + Node::NODE_EXPIRE_TIME < now and sr->nodes.size()-sr->getNumberOfBadNodes() < SEARCH_NODES)
+        refill(*sr);
 
     /* Check if the first TARGET_NODES (8) live nodes have replied. */
     if (sr->isSynced(now)) {
@@ -1028,18 +1020,6 @@ Dht::searchStep(std::shared_ptr<Search> sr)
         scheduler.edit(sr->nextSearchStep, sr->getNextStepTime(types, now));
 }
 
-/* Insert the contents of a bucket into a search structure. */
-unsigned
-Dht::Search::insertBucket(const Bucket& b, time_point now)
-{
-    unsigned inserted = 0;
-    for (auto& n : b.nodes) {
-        if (not n->isExpired() and insertNode(n, now))
-            inserted++;
-    }
-    return inserted;
-}
-
 bool
 Dht::Search::isSynced(time_point now) const
 {
@@ -1230,43 +1210,30 @@ Dht::Search::getNextStepTime(const std::map<ValueType::Id, ValueType>& types, ti
     return next_step;
 }
 
-void
-Dht::bootstrapSearch(Dht::Search& sr)
-{
-    const auto& now = scheduler.time();
-    const auto& list = network_engine.getCachedNodes(sr.id, sr.af);
-    if ( list.empty() ) {
-        DHT_LOG.ERR("No node found from cache");
-        return;
-    }
+unsigned Dht::refill(Dht::Search& sr) {
+    auto now = scheduler.time();
+    /* we search for up to SEARCH_NODES good nodes. */
+    auto cached_nodes = network_engine.getCachedNodes(sr.id, sr.af, SEARCH_NODES);
 
-    for ( auto& i : list)
-        sr.insertNode(i, now);
-    sr.refill_time = now;
-}
-
-unsigned
-Dht::Search::refill(const RoutingTable& r, time_point now) {
-    if (r.isEmpty() or r.front().af != af)
+    if (cached_nodes.empty()) {
+        DHT_LOG.ERR("[search %s IPv%c] no nodes from cache while refilling search",
+                sr.id.toString().c_str(), (sr.af == AF_INET) ? '4' : '6');
         return 0;
-    unsigned added = 0;
-    auto num_bad_nodes = getNumberOfBadNodes();
-    auto b = r.findBucket(id);
-    auto n = b;
-    while (nodes.size()-num_bad_nodes < SEARCH_NODES && (std::next(n) != r.end() || b != r.begin())) {
-        if (std::next(n) != r.end()) {
-            added += insertBucket(*std::next(n), now);
-            n = std::next(n);
-        }
-        if (b != r.begin()) {
-            added += insertBucket(*std::prev(b), now);
-            b = std::prev(b);
-        }
     }
 
-    return added;
+    unsigned inserted = 0;
+    for (auto& i : cached_nodes) {
+        /* try to insert the nodes. Search::insertNode will know how many to insert. */
+        if (sr.insertNode(i, now))
+            ++inserted;
+    }
+    DHT_LOG.DEBUG("[search %s IPv%c] refilled search with %u nodes from node cache",
+            sr.id.toString().c_str(), (sr.af == AF_INET) ? '4' : '6', inserted);
+    sr.refill_time = now;
+    return inserted;
 }
 
+
 /* Start a search. */
 std::shared_ptr<Dht::Search>
 Dht::search(const InfoHash& id, sa_family_t af, GetCallback gcb, QueryCallback qcb, DoneCallback dcb, Value::Filter f, Query q)
@@ -1324,7 +1291,7 @@ Dht::search(const InfoHash& id, sa_family_t af, GetCallback gcb, QueryCallback q
         ));
     }
 
-    bootstrapSearch(*sr);
+    refill(*sr);
     if (sr->nextSearchStep)
         scheduler.edit(sr->nextSearchStep, sr->getNextStepTime(types, scheduler.time()));
     else

src/network_engine.cpp

@@ -275,11 +275,6 @@ NetworkEngine::isNodeBlacklisted(const sockaddr *sa, socklen_t salen) const
     return false;
 }
 
-std::vector<std::shared_ptr<Node>>
-NetworkEngine::getCachedNodes(const InfoHash& id, sa_family_t sa_f) {
-    return cache.getCachedNodes(id, sa_f, MAX_CACHE_NODE);
-}
-
 void
 NetworkEngine::processMessage(const uint8_t *buf, size_t buflen, const sockaddr* from, socklen_t fromlen)
 {

src/node_cache.cpp

@@ -34,19 +34,19 @@ NodeCache::getNode(const InfoHash& id, const sockaddr* sa, socklen_t sa_len, tim
 }
 
 std::vector<std::shared_ptr<Node>>
-NodeCache::getCachedNodes(const InfoHash& id, sa_family_t sa_f, size_t total) {
+NodeCache::getCachedNodes(const InfoHash& id, sa_family_t sa_f, size_t count) {
     const auto& c = (sa_f == AF_INET ? cache_4 : cache_6);
     auto it_p = c.lower_bound(id),
          it_n = it_p;
 
     std::vector<std::shared_ptr<Node>> nodes;
-    nodes.reserve(std::min(c.size(), total));
+    nodes.reserve(std::min(c.size(), count));
     NodeMap::const_iterator it;
 
     if (it_p != c.begin()) /* Create 2 separate iterator if we could */
         --it_p;
 
-    while (nodes.size() < total and (it_n != c.end() or it_p != c.end())) {
+    while (nodes.size() < count and (it_n != c.end() or it_p != c.end())) {
         /* If one of the iterator is at the end, then take the other one
            If they are both in middle of somewhere comapre both and take
            the closest to the id. */