Skip to content
This repository has been archived by the owner on Oct 28, 2021. It is now read-only.

Fix failing Discovery tests #5581

Merged
merged 5 commits into from
Apr 30, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Tests for NodeTable::nearestNodeEntries
  • Loading branch information
gumb0 committed Apr 30, 2019
commit 8f13098071613ef879b086d5fa7cbb2bf5cecc74
4 changes: 2 additions & 2 deletions libp2p/NodeTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ vector<shared_ptr<NodeEntry>> NodeTable::nearestNodeEntries(NodeID const& _targe
return _node1.first < _node2.first;
};

std::set<pair<int, shared_ptr<NodeEntry>>, decltype(distanceToTargetLess)>
std::multiset<pair<int, shared_ptr<NodeEntry>>, decltype(distanceToTargetLess)>
nodesByDistanceToTarget(distanceToTargetLess);
for (auto const& bucket : m_buckets)
for (auto const& nodeWeakPtr : bucket.nodes)
Expand All @@ -279,7 +279,7 @@ vector<shared_ptr<NodeEntry>> NodeTable::nearestNodeEntries(NodeID const& _targe
nodesByDistanceToTarget.emplace(distance(_target, node->id()), node);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is currently very inefficient, because it calculates sha3(m_hostNodeID) many times in the loop and sha3(node->id()) every time nearestNodeEntries is called.

I will optimize it in the next PR by calculating hashes only once and making distance function get hashes as input.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


if (nodesByDistanceToTarget.size() > s_bucketSize)
nodesByDistanceToTarget.erase(nodesByDistanceToTarget.rbegin().base());
nodesByDistanceToTarget.erase(--nodesByDistanceToTarget.end());
}

vector<shared_ptr<NodeEntry>> ret;
Expand Down
94 changes: 91 additions & 3 deletions test/unittests/libp2p/net.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -210,13 +210,14 @@ struct TestNodeTable: public NodeTable
concurrent_queue<bytes> packetsReceived;


using NodeTable::m_hostNodeID;
using NodeTable::m_allowLocalDiscovery;
using NodeTable::m_hostNodeEndpoint;
using NodeTable::m_hostNodeID;
using NodeTable::m_socket;
using NodeTable::nearestNodeEntries;
using NodeTable::nodeEntry;
using NodeTable::noteActiveNode;
using NodeTable::setRequestTimeToLive;
using NodeTable::nodeEntry;
using NodeTable::m_allowLocalDiscovery;
};

/**
Expand Down Expand Up @@ -550,6 +551,93 @@ BOOST_AUTO_TEST_CASE(noteActiveNodeEvictsTheNodeWhenBucketIsFull)
BOOST_CHECK_EQUAL(evicted->replacementNodeEntry->id(), newNodeId);
}

BOOST_AUTO_TEST_CASE(nearestNodeEntriesOneNode)
{
TestNodeTableHost nodeTableHost(1);
nodeTableHost.populate(1);

auto& nodeTable = nodeTableHost.nodeTable;
vector<shared_ptr<NodeEntry>> const nearest = nodeTable->nearestNodeEntries(NodeID::random());

BOOST_REQUIRE_EQUAL(nearest.size(), 1);
BOOST_REQUIRE_EQUAL(nearest.front()->id(), nodeTableHost.testNodes.front().first);
}

unsigned xorDistance(h256 const& _h1, h256 const& _h2)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will move this function to NodeTable implementation in the next PR.

{
u256 d = _h1 ^ _h2;
unsigned ret = 0;
while (d >>= 1)
++ret;
return ret;
};

BOOST_AUTO_TEST_CASE(nearestNodeEntriesOneDistantNode)
{
// specific case that was failing - one node in bucket #252, target corresponding to bucket #253
unique_ptr<TestNodeTableHost> nodeTableHost;
do
{
nodeTableHost.reset(new TestNodeTableHost(1));
nodeTableHost->populate(1);
} while (nodeTableHost->nodeTable->bucketSize(252) != 1);

auto& nodeTable = nodeTableHost->nodeTable;

h256 const hostNodeIDHash = sha3(nodeTable->m_hostNodeID);

NodeID target = NodeID::random();
while (xorDistance(hostNodeIDHash, sha3(target)) != 254)
target = NodeID::random();

vector<shared_ptr<NodeEntry>> const nearest = nodeTable->nearestNodeEntries(target);

BOOST_REQUIRE_EQUAL(nearest.size(), 1);
BOOST_REQUIRE_EQUAL(nearest.front()->id(), nodeTableHost->testNodes.front().first);
}

BOOST_AUTO_TEST_CASE(nearestNodeEntriesManyNodes)
{
unsigned constexpr nodeCount = 128;
TestNodeTableHost nodeTableHost(nodeCount);
nodeTableHost.populate(nodeCount);

auto& nodeTable = nodeTableHost.nodeTable;

NodeID const target = NodeID::random();
vector<shared_ptr<NodeEntry>> nearest = nodeTable->nearestNodeEntries(target);

BOOST_REQUIRE_EQUAL(nearest.size(), 16);

// get all nodes sorted by distance to target
list<NodeEntry> const allNodeEntries = nodeTable->snapshot();
h256 const targetNodeIDHash = sha3(target);
vector<pair<unsigned, NodeID>> nodesByDistanceToTarget;
for (auto const& nodeEntry : allNodeEntries)
{
NodeID const& nodeID = nodeEntry.id();
nodesByDistanceToTarget.emplace_back(xorDistance(targetNodeIDHash, sha3(nodeID)), nodeID);
}
// stable sort to keep them in the order as they are in buckets
// (the same order they are iterated in nearestNodeEntries implementation)
std::stable_sort(nodesByDistanceToTarget.begin(), nodesByDistanceToTarget.end(),
[](pair<unsigned, NodeID> const& _n1, pair<unsigned, NodeID> const& _n2) {
return _n1.first < _n2.first;
});
// get 16 with lowest distance
std::vector<NodeID> expectedNearestIDs;
std::transform(nodesByDistanceToTarget.begin(), nodesByDistanceToTarget.begin() + 16,
std::back_inserter(expectedNearestIDs),
[](pair<unsigned, NodeID> const& _n) { return _n.second; });


vector<NodeID> nearestIDs;
for (auto const& nodeEntry : nearest)
nearestIDs.push_back(nodeEntry->id());

BOOST_REQUIRE(nearestIDs == expectedNearestIDs);
}

BOOST_AUTO_TEST_CASE(unexpectedPong)
{
// NodeTable receiving PONG
Expand Down