NetDb: refactor getting closest routers

src/core/router/net_db/impl.cc

@@ -946,82 +946,109 @@ std::shared_ptr<const RouterInfo> NetDb::GetClosestFloodfill(
     const IdentHash& destination,
     const std::set<IdentHash>& excluded) const {
   std::shared_ptr<const RouterInfo> r;
-  XORMetric min_metric;
-  IdentHash dest_key = CreateRoutingKey(destination);
-  min_metric.SetMax();
-  std::unique_lock<std::mutex> l(m_FloodfillsMutex);
-  for (auto it : m_Floodfills) {
-    if (!it->IsUnreachable()) {
-      XORMetric m = dest_key ^ it->GetIdentHash();
-      if (m < min_metric && !excluded.count(it->GetIdentHash())) {
-        min_metric = m;
-        r = it;
-      }
+  try
+   {
+      XORMetric min_metric;
+      IdentHash dest_key = CreateRoutingKey(destination);
+      min_metric.SetMax();
+      std::unique_lock<std::mutex> l(m_FloodfillsMutex);
+      for (const auto& it : m_Floodfills)
+        {
+          if (!it->IsUnreachable() && !excluded.count(it->GetIdentHash()))
+            {
+              XORMetric m = dest_key ^ it->GetIdentHash();
+              if (m < min_metric)
+                {
+                  min_metric = m;
+                  r = it;
+                }
+            }
+        }
+    }
+  catch (...)
+    {
+      m_Exception.Dispatch(__func__);
+      throw;
     }
-  }
   return r;
 }
 
 std::vector<IdentHash> NetDb::GetClosestFloodfills(
     const IdentHash& destination,
-    std::uint8_t num,
-    std::set<IdentHash>& excluded) const
+    const std::uint8_t num,
+    const std::set<IdentHash>& excluded) const
 {
-  struct Sorted {
-    std::shared_ptr<const RouterInfo> r;
-    XORMetric metric;
-    bool operator<(const Sorted& other) const {
-      return metric < other.metric;
-    }
-  };
-  std::set<Sorted> sorted;
-  IdentHash dest_key = CreateRoutingKey(destination); {
-    std::unique_lock<std::mutex> l(m_FloodfillsMutex);
-    for (auto it : m_Floodfills) {
-      if (!it->IsUnreachable()) {
-        XORMetric m = dest_key ^ it->GetIdentHash();
-        if (sorted.size() < num) {
-          sorted.insert({it, m});
-        } else if (m < sorted.rbegin()->metric) {
-          sorted.insert({it, m});
-          sorted.erase(std::prev(sorted.end()));
+  std::vector<IdentHash> res;
+  try
+    {
+      IdentHash dest_key = CreateRoutingKey(destination);
+      struct Sorted
+      {
+        std::shared_ptr<const RouterInfo> r;
+        XORMetric metric;
+        bool operator<(const Sorted& other) const
+        {
+          return metric < other.metric;
         }
+      };
+      std::set<Sorted> sorted;
+      {
+        std::unique_lock<std::mutex> l(m_FloodfillsMutex);
+        for (const auto& it : m_Floodfills)
+          {
+            if (!it->IsUnreachable() && !excluded.count(it->GetIdentHash()))
+              {
+                XORMetric m = dest_key ^ it->GetIdentHash();
+                sorted.insert({it, m});
+              }
+          }
       }
+
+      std::uint8_t i{};
+      for (auto it = sorted.begin(); it != sorted.end() && i < num; ++it)
+        {
+          const auto& ident = it->r->GetIdentHash();
+          res.push_back(ident);
+          ++i;
+        }
     }
-  }
-  std::vector<IdentHash> res;
-  std::uint8_t i{};
-  for (auto it : sorted) {
-    if (i < num) {
-      auto& ident = it.r->GetIdentHash();
-      if (!excluded.count(ident)) {
-        res.push_back(ident);
-        i++;
-      }
-    } else {
-      break;
+  catch (...)
+    {
+      m_Exception.Dispatch(__func__);
+      throw;
     }
-  }
   return res;
 }
 
 std::shared_ptr<const RouterInfo> NetDb::GetClosestNonFloodfill(
     const IdentHash& destination,
     const std::set<IdentHash>& excluded) const {
   std::shared_ptr<const RouterInfo> r;
-  XORMetric min_metric;
-  IdentHash dest_key = CreateRoutingKey(destination);
-  min_metric.SetMax();
-  // must be called from NetDb thread only
-  for (auto it : m_RouterInfos) {
-    if (!it.second->HasCap(RouterInfo::Cap::Floodfill)) {
-      XORMetric m = dest_key ^ it.first;
-      if (m < min_metric && !excluded.count(it.first)) {
-        min_metric = m;
-        r = it.second;
-      }
+  try
+    {
+      XORMetric min_metric;
+      IdentHash dest_key = CreateRoutingKey(destination);
+      min_metric.SetMax();
+      std::unique_lock<std::mutex> l(m_RouterInfosMutex);
+      for (const auto& it : m_RouterInfos)
+        {
+          if (!it.second->HasCap(RouterInfo::Cap::Floodfill)
+              && !excluded.count(it.first))
+            {
+              XORMetric m = dest_key ^ it.first;
+              if (m < min_metric)
+                {
+                  min_metric = m;
+                  r = it.second;
+                }
+            }
+        }
+    }
+  catch (...)
+    {
+      m_Exception.Dispatch(__func__);
+      throw;
     }
-  }
   return r;
 }
 

src/core/router/net_db/impl.h

@@ -197,16 +197,16 @@ class NetDb : public NetDbTraits {
 
   std::shared_ptr<const RouterInfo> GetClosestFloodfill(
       const IdentHash& destination,
-      const std::set<IdentHash>& excluded) const;
+      const std::set<IdentHash>& excluded = std::set<IdentHash>()) const;
 
   std::vector<IdentHash> GetClosestFloodfills(
       const IdentHash& destination,
-      std::uint8_t num,
-      std::set<IdentHash>& excluded) const;
+      const std::uint8_t num,
+      const std::set<IdentHash>& excluded = std::set<IdentHash>()) const;
 
   std::shared_ptr<const RouterInfo> GetClosestNonFloodfill(
       const IdentHash& destination,
-      const std::set<IdentHash>& excluded) const;
+      const std::set<IdentHash>& excluded = std::set<IdentHash>()) const;
 
   void SetUnreachable(
       const IdentHash& ident,

src/core/util/exception.cc

@@ -45,7 +45,7 @@ namespace core {
 Exception::Exception(const char* message) : m_Message(message) {}
 
 // TODO(anonimal): exception error codes to replace strings?
-void Exception::Dispatch(const char* message)
+void Exception::Dispatch(const char* message) const
 {
   // Message to log
   std::string log;

src/core/util/exception.h

@@ -49,7 +49,7 @@ class Exception final {
   /// @brief Exception class dispatcher
   /// @details Set optional exception message, concats messages, adds trivial formatting
   /// @param message String message to log for exception
-  void Dispatch(const char* message = "");
+  void Dispatch(const char* message = "") const;
 
  private:
   std::string m_Message;

tests/unit_tests/CMakeLists.txt

@@ -18,6 +18,7 @@ add_executable(kovri-tests
   core/crypto/rand.cc
   core/crypto/util/x509.cc
   core/router/identity.cc
+  core/router/net_db/impl.cc
   core/router/transports/ssu/packet.cc
   core/util/byte_stream.cc
   core/util/config.cc

tests/unit_tests/core/router/net_db/impl.cc

@@ -0,0 +1,170 @@
+/**                                                                                           //
+ * Copyright (c) 2015-2018, The Kovri I2P Router Project                                      //
+ *                                                                                            //
+ * All rights reserved.                                                                       //
+ *                                                                                            //
+ * Redistribution and use in source and binary forms, with or without modification, are       //
+ * permitted provided that the following conditions are met:                                  //
+ *                                                                                            //
+ * 1. Redistributions of source code must retain the above copyright notice, this list of     //
+ *    conditions and the following disclaimer.                                                //
+ *                                                                                            //
+ * 2. Redistributions in binary form must reproduce the above copyright notice, this list     //
+ *    of conditions and the following disclaimer in the documentation and/or other            //
+ *    materials provided with the distribution.                                               //
+ *                                                                                            //
+ * 3. Neither the name of the copyright holder nor the names of its contributors may be       //
+ *    used to endorse or promote products derived from this software without specific         //
+ *    prior written permission.                                                               //
+ *                                                                                            //
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY        //
+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF    //
+ * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL     //
+ * THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,       //
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,               //
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS    //
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,          //
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF    //
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.               //
+ */
+
+#define BOOST_TEST_DYN_LINK
+
+#include <boost/test/unit_test.hpp>
+
+#include <memory>
+#include <set>
+#include <utility>
+#include <vector>
+
+#include "core/router/identity.h"
+#include "core/router/info.h"
+#include "core/router/net_db/impl.h"
+
+#include "tests/unit_tests/core/router/identity.h"
+
+namespace core = kovri::core;
+
+struct NetDbFixture : public IdentityExFixture
+{
+  using Cap = core::RouterInfoTraits::Cap;
+
+  NetDbFixture() : m_NetDb(std::make_unique<core::NetDb>())
+  {
+    // Use Alice's data from IdentityEx fixture
+    core::IdentityEx m_Ident;
+    m_Ident.FromBuffer(m_AliceIdentity.data(), m_AliceIdentity.size());
+    m_Hash = m_Ident.GetIdentHash();
+  }
+
+  std::unique_ptr<core::RouterInfo> AddRouterInfo(Cap cap)
+  {
+    auto ri = std::make_unique<core::RouterInfo>(
+        core::PrivateKeys::CreateRandomKeys(
+            core::SIGNING_KEY_TYPE_EDDSA_SHA512_ED25519),
+        std::vector<std::pair<std::string, std::uint16_t>>{{"127.0.0.1", 9111}},
+        std::make_pair(true, false),
+        cap);
+
+    m_NetDb->AddRouterInfo(
+        ri->GetIdentHash(), ri->GetBuffer(), ri->GetBufferLen());
+
+    return ri;
+  }
+
+  std::shared_ptr<const core::RouterInfo> GetClosestFloodfill()
+  {
+    auto ff = m_NetDb->GetClosestFloodfill(m_Hash);
+    if (!ff)
+      throw std::runtime_error("no floodfill available");
+    return ff;
+  }
+
+  std::vector<core::IdentHash> GetClosestFloodfills(const std::uint8_t count)
+  {
+    auto ffs = m_NetDb->GetClosestFloodfills(m_Hash, count);
+    if (ffs.empty())
+      throw std::runtime_error("no floodfills available");
+    return ffs;
+  }
+
+  std::shared_ptr<const core::RouterInfo> GetClosestNonFloodfill()
+  {
+    auto ri = m_NetDb->GetClosestNonFloodfill(m_Hash);
+    if (!ri)
+      throw std::runtime_error("no routers available");
+    return ri;
+  }
+
+  core::IdentHash m_Hash;
+  std::unique_ptr<core::NetDb> m_NetDb;
+};
+
+BOOST_FIXTURE_TEST_SUITE(NetDbTests, NetDbFixture)
+
+// TODO(unassigned): this isn't an accurate testcase (we should rather test kademlia)
+BOOST_AUTO_TEST_CASE(ValidClosestFloodfills)
+{
+  constexpr std::uint8_t count(2);  // FF count
+
+  // Add floodfills to netdb
+  std::set<std::unique_ptr<core::RouterInfo>> infos;
+  for (auto i(0); i < count; i++)
+    infos.insert(AddRouterInfo(Cap::Floodfill));
+
+  // Get added floodfill hashes
+  std::vector<core::IdentHash> hashes;
+  for (const auto& ri : infos)
+    hashes.push_back(ri->GetIdentHash());
+
+  // Get closest floodfills
+  std::vector<core::IdentHash> ffs;
+  BOOST_REQUIRE_NO_THROW(ffs = GetClosestFloodfills(infos.size()));
+
+  // Floodfill hashes should match added router hashes
+  // TODO(unassigned): this should change once we include the kademlia test
+  std::sort(ffs.begin(), ffs.end());
+  std::sort(hashes.begin(), hashes.end());
+
+  BOOST_CHECK(ffs == hashes);
+}
+
+BOOST_AUTO_TEST_CASE(ValidClosestFloodfill)
+{
+  std::unique_ptr<core::RouterInfo> ri;
+  BOOST_REQUIRE_NO_THROW(ri = AddRouterInfo(Cap::Floodfill));
+
+  std::shared_ptr<const core::RouterInfo> ff;
+  BOOST_REQUIRE_NO_THROW(ff = GetClosestFloodfill());
+
+  BOOST_REQUIRE_EQUAL(ff->GetIdentHash(), ri->GetIdentHash());
+}
+
+BOOST_AUTO_TEST_CASE(ValidClosestNonFloodfill)
+{
+  std::unique_ptr<core::RouterInfo> ri;
+  BOOST_REQUIRE_NO_THROW(ri = AddRouterInfo(Cap::HighBandwidth));
+
+  std::shared_ptr<const core::RouterInfo> ff;
+  BOOST_REQUIRE_NO_THROW(ff = GetClosestNonFloodfill());
+
+  BOOST_CHECK_EQUAL(ff->GetIdentHash(), ri->GetIdentHash());
+}
+
+BOOST_AUTO_TEST_CASE(InvalidRouters)
+{
+  core::IdentHash hash;  // Empty hash
+
+  BOOST_CHECK_THROW(m_NetDb->GetClosestFloodfill(hash), std::exception);
+  BOOST_CHECK_THROW(m_NetDb->GetClosestFloodfill(nullptr), std::exception);
+
+  BOOST_CHECK_THROW(m_NetDb->GetClosestFloodfills(hash, 1), std::exception);
+  BOOST_CHECK_THROW(m_NetDb->GetClosestFloodfills(nullptr, 1), std::exception);
+
+  BOOST_CHECK_THROW(GetClosestFloodfills(0), std::exception);
+
+  BOOST_CHECK_THROW(m_NetDb->GetClosestNonFloodfill(nullptr), std::exception);
+  BOOST_CHECK_THROW(m_NetDb->GetClosestNonFloodfill(hash), std::exception);
+}
+
+BOOST_AUTO_TEST_SUITE_END()