This repository has been archived by the owner on Sep 1, 2022. It is now read-only.
NetDb: refactor getting closest routers #910
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
coneiric
changed the title
coneiric
force-pushed
the
netdb-closest-routers
branch
2 times, most recently
from
11bf11c
to
e4b2882
Compare
anonimal
suggested changes
Jun 23, 2018
src/core/router/net_db/impl.cc
Outdated
| if (i < num) { | ||
| auto& ident = it.r->GetIdentHash(); | ||
| if (!excluded.count(ident)) { | ||
| while (i < num) |
There was a problem hiding this comment.
This will insert the same hash repeatedly... and any remaining elements in the set will be ignored.
There was a problem hiding this comment.
You're right, I'll change the loop to step through the whole set.
src/core/router/net_db/impl.cc
Outdated
| } | ||
| } | ||
|
|
||
| std::vector<IdentHash> res; | ||
| std::uint8_t i{}; | ||
| for (auto it : sorted) { |
There was a problem hiding this comment.
May as well const auto& while you're here.
src/core/router/net_db/impl.cc
Outdated
| @@ -978,29 +979,23 @@ std::vector<IdentHash> NetDb::GetClosestFloodfills( | |||
| IdentHash dest_key = CreateRoutingKey(destination); { | |||
| std::unique_lock<std::mutex> l(m_FloodfillsMutex); | |||
| for (auto it : m_Floodfills) { | |||
There was a problem hiding this comment.
May as well const auto& while you're here.
src/core/router/net_db/impl.cc
Outdated
| @@ -951,13 +951,14 @@ std::shared_ptr<const RouterInfo> NetDb::GetClosestFloodfill( | |||
| min_metric.SetMax(); | |||
| std::unique_lock<std::mutex> l(m_FloodfillsMutex); | |||
| for (auto it : m_Floodfills) { | |||
There was a problem hiding this comment.
May as well const auto& while you're here.
anonimal
suggested changes
Jun 23, 2018
src/core/router/net_db/impl.h
Outdated
| @@ -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); | |||
There was a problem hiding this comment.
These getters should remain const. They should only be accessing members, not mutating.
There was a problem hiding this comment.
These getters should remain const
I thought so as well, but didn't know how much work would be needed on the dispatcher to make it const as well. Will work on this.
There was a problem hiding this comment.
Not much to it:
diff --git a/src/core/util/exception.cc b/src/core/util/exception.cc
index 936dfa9f..4bc5ad23 100644
--- a/src/core/util/exception.cc
+++ b/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;
diff --git a/src/core/util/exception.h b/src/core/util/exception.h
index dc84d926..e7ae5cc1 100644
--- a/src/core/util/exception.h
+++ b/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;There was a problem hiding this comment.
Wow, really not much. Will make the changes.
src/core/router/net_db/impl.h
Outdated
|
|
||
| std::vector<IdentHash> GetClosestFloodfills( | ||
| const IdentHash& destination, | ||
| std::uint8_t num, | ||
| std::set<IdentHash>& excluded) const; | ||
| std::set<IdentHash>& excluded); |
There was a problem hiding this comment.
These getters should remain const. They should only be accessing members, not mutating.
There was a problem hiding this comment.
These getters should remain const
I thought so as well, but didn't know how much work would be needed on the dispatcher to make it const as well. Will work on this.
src/core/router/net_db/impl.h
Outdated
|
|
||
| std::shared_ptr<const RouterInfo> GetClosestNonFloodfill( | ||
| const IdentHash& destination, | ||
| const std::set<IdentHash>& excluded) const; | ||
| const std::set<IdentHash>& excluded); |
There was a problem hiding this comment.
These getters should remain const. They should only be accessing members, not mutating.
There was a problem hiding this comment.
These getters should remain const
I thought so as well, but didn't know how much work would be needed on the dispatcher to make it const as well. Will work on this.
anonimal
suggested changes
Jun 23, 2018
There was a problem hiding this comment.
These tests are a perfect example of why we simply need to test our kademlia implementation. The rest should fall in line after that's done. For the time being, if you want to keep what you have here, please condense all this copypasta into reusable functions and/or a fixture.
| { | ||
| /// @alias RouterCaps | ||
| /// @brief Capabilities alias for readability and convenience | ||
| using RouterCaps = kovri::core::RouterInfoTraits::Cap; |
There was a problem hiding this comment.
To keep in line with my branch 'bandcaps', s/RouterCaps/Cap/ (this will speed up refactoring once that's merged).
There was a problem hiding this comment.
Will do, meant to change before pushing.
I can work on this as well, and will read up on Kademlia specs.
Will do, and I originally had more of the code in the fixture. The noise from |
coneiric
force-pushed
the
netdb-closest-routers
branch
from
e4b2882
to
e072464
Compare
|
Just added the recommended changes, and updated the test fixture. If the changes look good, I'll add the issue + PR references to the commit messages. |
anonimal
suggested changes
Jun 26, 2018
There was a problem hiding this comment.
Can you please apply the following and add a "credit to anonimal" in the git log? I'm consistently having to write huge patches and am not getting credit. I'm not mad, I'm glad to work with you; my concern is that if github goes down then so does a record of all my mentoring (because we only reference PR number in git-log).
To make things easier, in the future, I should just PR to your fork for large patches like this.
Thanks in advance and for getting all this going, I appreciate it.
diff --git a/src/core/router/net_db/impl.cc b/src/core/router/net_db/impl.cc
index 686dc769..1bef84d3 100644
--- a/src/core/router/net_db/impl.cc
+++ b/src/core/router/net_db/impl.cc
@@ -975,8 +975,8 @@ std::shared_ptr<const RouterInfo> NetDb::GetClosestFloodfill(
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
{
std::vector<IdentHash> res;
try
diff --git a/src/core/router/net_db/impl.h b/src/core/router/net_db/impl.h
index c71fdfec..b9d9d19a 100644
--- a/src/core/router/net_db/impl.h
+++ b/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,
diff --git a/tests/unit_tests/core/router/net_db/impl.cc b/tests/unit_tests/core/router/net_db/impl.cc
index 7f7bd88b..232da332 100644
--- a/tests/unit_tests/core/router/net_db/impl.cc
+++ b/tests/unit_tests/core/router/net_db/impl.cc
@@ -1,5 +1,5 @@
/** //
- * Copyright (c) 2013-2018, The Kovri I2P Router Project //
+ * Copyright (c) 2015-2018, The Kovri I2P Router Project //
* //
* All rights reserved. //
* //
@@ -26,14 +26,17 @@
* 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. //
- * //
- * Parts of the project are originally copyright (c) 2013-2015 The PurpleI2P Project //
*/
#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"
@@ -44,153 +47,124 @@ namespace core = kovri::core;
struct NetDbFixture : public IdentityExFixture
{
- /// @alias RouterCap
- /// @brief Capabilities alias
- /// @details Intended for readability & user-friendliness when writing new tests
- using RouterCap = core::RouterInfoTraits::Cap;
-
- /// @brief Create identity from buffer
- /// @details Useful for getting a valid IdentHash
- void CreateIdent()
- {
- // Create valid identity
- BOOST_CHECK(
- m_Ident.FromBuffer(m_AliceIdentity.data(), m_AliceIdentity.size()));
+ using Cap = core::RouterInfoTraits::Cap;
- // Set ident hash for convenience
+ 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();
}
- /// @brief Add router to NetDb
- /// @param cap Capability to add to router
- void AddRouter(const RouterCap cap)
+ std::unique_ptr<core::RouterInfo> AddRouterInfo(Cap cap)
{
- // Create new private keys
- m_Keys = core::PrivateKeys::CreateRandomKeys(
- core::SIGNING_KEY_TYPE_EDDSA_SHA512_ED25519);
+ 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);
- // Create new router
- m_RI =
- std::make_unique<core::RouterInfo>(m_Keys, m_Points, m_Transports, cap);
+ m_NetDb->AddRouterInfo(
+ ri->GetIdentHash(), ri->GetBuffer(), ri->GetBufferLen());
- // Add router to NetDb
- BOOST_CHECK_NO_THROW(m_NetDB.AddRouterInfo(
- m_RI->GetIdentHash(), m_RI->GetBuffer(), m_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::NetDb m_NetDB;
core::IdentHash m_Hash;
- core::IdentityEx m_Ident;
- core::PrivateKeys m_Keys;
- std::set<core::IdentHash> m_Ex;
- std::unique_ptr<core::RouterInfo> m_RI;
- std::pair<bool, bool> m_Transports{true, false};
- std::vector<std::pair<std::string, std::uint16_t> > m_Points{{"127.0.0.1", 9111}};
+ std::unique_ptr<core::NetDb> m_NetDb;
};
BOOST_FIXTURE_TEST_SUITE(NetDbTests, NetDbFixture)
-BOOST_AUTO_TEST_CASE(ValidClosestFloodfill)
+// TODO(unassigned): this isn't an accurate testcase (we should rather test kademlia)
+BOOST_AUTO_TEST_CASE(ValidClosestFloodfills)
{
- // Create a valid router identity
- CreateIdent();
+ constexpr std::uint8_t count(2); // FF count
- // Add floodfill router to NetDb
- AddRouter(RouterCap::Floodfill);
+ // Add floodfills to netdb
+ std::set<std::unique_ptr<core::RouterInfo>> infos;
+ for (auto i(0); i < count; i++)
+ infos.insert(AddRouterInfo(Cap::Floodfill));
- std::shared_ptr<const core::RouterInfo> ret_ri;
+ // Get added floodfill hashes
+ std::vector<core::IdentHash> hashes;
+ for (const auto& ri : infos)
+ hashes.push_back(ri->GetIdentHash());
- // Ensure no exceptions thrown getting valid floodfill
- BOOST_CHECK_NO_THROW(ret_ri = m_NetDB.GetClosestFloodfill(m_Hash, m_Ex));
-
- // Ensure expected floodfill is returned
- BOOST_CHECK(ret_ri && ret_ri->GetIdentHash() == m_RI->GetIdentHash());
-}
+ // Get closest floodfills
+ std::vector<core::IdentHash> ffs;
+ BOOST_REQUIRE_NO_THROW(ffs = GetClosestFloodfills(infos.size()));
-BOOST_AUTO_TEST_CASE(InvalidClosestFloodfill)
-{
- // Ensure null destination throws
- BOOST_CHECK_THROW(
- m_NetDB.GetClosestFloodfill(nullptr, m_Ex), std::invalid_argument);
+ // 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());
- // Ensure zero-initialized destination throws
- BOOST_CHECK_THROW(
- m_NetDB.GetClosestFloodfill(m_Hash, m_Ex), std::invalid_argument);
+ BOOST_CHECK(ffs == hashes);
}
-BOOST_AUTO_TEST_CASE(ValidClosestFloodfills)
+BOOST_AUTO_TEST_CASE(ValidClosestFloodfill)
{
- // Create a valid router identity
- CreateIdent();
-
- // Add floodfill router to NetDb
- AddRouter(RouterCap::Floodfill);
-
- // Store the first floodfill locally
- std::unique_ptr<core::RouterInfo> flood_ri{nullptr};
- flood_ri.swap(m_RI);
-
- // Add another floodfill router to NetDb
- AddRouter(RouterCap::Floodfill);
-
- std::vector<core::IdentHash> ret_hash;
- const std::uint8_t limit = 2;
-
- // Ensure no exceptions thrown getting valid floodfill(s)
- BOOST_CHECK_NO_THROW(
- ret_hash = m_NetDB.GetClosestFloodfills(m_Hash, limit, m_Ex));
-
- // Ensure number of floodfills added are returned
- BOOST_CHECK(ret_hash.size() == limit);
+ std::unique_ptr<core::RouterInfo> ri;
+ BOOST_REQUIRE_NO_THROW(ri = AddRouterInfo(Cap::Floodfill));
- // Ensure returned ident hashes are unique
- BOOST_CHECK(ret_hash.front() != ret_hash.back());
+ std::shared_ptr<const core::RouterInfo> ff;
+ BOOST_REQUIRE_NO_THROW(ff = GetClosestFloodfill());
- // Ensure returned ident hashes match expected floodfills
- for (auto const& h : ret_hash)
- BOOST_CHECK(h == flood_ri->GetIdentHash() || h == m_RI->GetIdentHash());
-
- // Ensure limit is respected
- BOOST_CHECK(m_NetDB.GetClosestFloodfills(m_Hash, 0, m_Ex).empty());
+ BOOST_REQUIRE_EQUAL(ff->GetIdentHash(), ri->GetIdentHash());
}
-BOOST_AUTO_TEST_CASE(InvalidClosestFloodfills)
+BOOST_AUTO_TEST_CASE(ValidClosestNonFloodfill)
{
- // Ensure null destination throws
- BOOST_CHECK_THROW(
- m_NetDB.GetClosestFloodfills(nullptr, 1, m_Ex), std::invalid_argument);
+ std::unique_ptr<core::RouterInfo> ri;
+ BOOST_REQUIRE_NO_THROW(ri = AddRouterInfo(Cap::HighBandwidth));
- // Ensure zero-initialized destination throws
- BOOST_CHECK_THROW(
- m_NetDB.GetClosestFloodfills(m_Hash, 1, m_Ex), std::invalid_argument);
+ std::shared_ptr<const core::RouterInfo> ff;
+ BOOST_REQUIRE_NO_THROW(ff = GetClosestNonFloodfill());
+
+ BOOST_CHECK_EQUAL(ff->GetIdentHash(), ri->GetIdentHash());
}
-BOOST_AUTO_TEST_CASE(ValidClosestNonFloodfill)
+BOOST_AUTO_TEST_CASE(InvalidRouters)
{
- // Create a valid router identity
- CreateIdent();
+ core::IdentHash hash; // Empty hash
- // Add non-floodfill to NetDb
- AddRouter(RouterCap::HighBandwidth);
+ BOOST_CHECK_THROW(m_NetDb->GetClosestFloodfill(hash), std::exception);
+ BOOST_CHECK_THROW(m_NetDb->GetClosestFloodfill(nullptr), std::exception);
- std::shared_ptr<const core::RouterInfo> ret_ri;
+ BOOST_CHECK_THROW(m_NetDb->GetClosestFloodfills(hash, 1), std::exception);
+ BOOST_CHECK_THROW(m_NetDb->GetClosestFloodfills(nullptr, 1), std::exception);
- // Ensure no exceptions thrown getting valid non-floodfill
- BOOST_CHECK_NO_THROW(ret_ri = m_NetDB.GetClosestNonFloodfill(m_Hash, m_Ex));
-
- // Ensure expected non-floodfill is returned
- BOOST_CHECK(ret_ri && ret_ri->GetIdentHash() == m_RI->GetIdentHash());
-}
-
-BOOST_AUTO_TEST_CASE(InvalidClosestNonFloodfill)
-{
- // Ensure null destination throws
- BOOST_CHECK_THROW(
- m_NetDB.GetClosestNonFloodfill(nullptr, m_Ex), std::invalid_argument);
+ BOOST_CHECK_THROW(GetClosestFloodfills(0), std::exception);
- // Ensure zero-initialized destination throws
- BOOST_CHECK_THROW(
- m_NetDB.GetClosestNonFloodfill(m_Hash, m_Ex), std::invalid_argument);
+ BOOST_CHECK_THROW(m_NetDb->GetClosestNonFloodfill(nullptr), std::exception);
+ BOOST_CHECK_THROW(m_NetDb->GetClosestNonFloodfill(hash), std::exception);
}
BOOST_AUTO_TEST_SUITE_END()
Absolutely, my bad for not including the credit in the git log for the last patch you submitted to my other PR. Thank you for the patch. |
It is unnecessary to sort excluded routers. References monero-project#643 and monero-project#910
Adds const-correctness to getting closest router parameters, and default parameters for excluded routers. Credit to anonimal for the patch. Referencing monero-project#643 and monero-project#910
Cleaner, more useable NetDb test fixture, and more concise test cases. Credit to anonimal for the patch. References monero-project#643 and monero-project#910
coneiric
force-pushed
the
netdb-closest-routers
branch
from
e072464
to
e9d2bf4
Compare
anonimal
approved these changes
Jun 29, 2018
anonimal
added a commit
that referenced
this pull request
Jun 29, 2018
e9d2bf4 Tests: patches NetDb closest router unit-tests (oneiric) 32ebf5d NetDb: adds const-correctness + default params (oneiric) 4ea811c Test: NetDb get closest routers unit-tests (oneiric) 06d4f0b NetDb: catch exceptions getting closest routers (oneiric) bbdf271 Exception: make Dispatch const-correct (oneiric) eca1128 NetDb: add lock in getting closest non-floodfill (oneiric) 57332bb NetDb: exclude routers before sorting them (oneiric)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By submitting this pull-request, I confirm the following:
A number of refactors to getting closest routers:
CreateRoutingKeycallers.RouterInfos inGetClosestNonFloodfillto make calling the function thread-safe.Referencing #643.