Fix failing Discovery tests

CHANGELOG.md

@@ -10,6 +10,7 @@
 - Changed: [#5568](https://github.com/ethereum/aleth/pull/5568) Improve rlpx handshake log messages and create new rlpx log channel.
 - Changed: [#5576](https://github.com/ethereum/aleth/pull/5576) Moved sstore_combinations and static_Call50000_sha256 tests to stTimeConsuming test suite. (testeth runs them only with `--all` flag)
 - Fixed: [#5562](https://github.com/ethereum/aleth/pull/5562) Don't send header request messages to peers that haven't sent us Status yet.
+- Fixed: [#5581](https://github.com/ethereum/aleth/pull/5581) Fixed finding neighbour nodes in Discovery.
 
 ## [1.6.0] - 2019-04-16
 

libp2p/NodeTable.cpp

@@ -203,6 +203,7 @@ void NodeTable::doDiscoveryRound(
     if (!m_socket->isOpen())
         return;
 
+    // send s_alpha FindNode packets to nodes we know, closest to target
     auto const nearestNodes = nearestNodeEntries(_node);
     auto newTriedCount = 0;
     for (auto const& nodeEntry : nearestNodes)
@@ -262,57 +263,29 @@ void NodeTable::doDiscoveryRound(
         });
 }
 
-vector<shared_ptr<NodeEntry>> NodeTable::nearestNodeEntries(NodeID _target)
+vector<shared_ptr<NodeEntry>> NodeTable::nearestNodeEntries(NodeID const& _target)
 {
-    // send s_alpha FindNode packets to nodes we know, closest to target
-    static unsigned lastBin = s_bins - 1;
-    unsigned head = distance(m_hostNodeID, _target);
-    unsigned tail = head == 0 ? lastBin : (head - 1) % s_bins;
-
-    map<unsigned, list<shared_ptr<NodeEntry>>> found;
+    auto const distanceToTargetLess = [](pair<int, shared_ptr<NodeEntry>> const& _node1,
+                                          pair<int, shared_ptr<NodeEntry>> const& _node2) {
+        return _node1.first < _node2.first;
+    };
+
+    std::multiset<pair<int, shared_ptr<NodeEntry>>, decltype(distanceToTargetLess)>
+        nodesByDistanceToTarget(distanceToTargetLess);
+    for (auto const& bucket : m_buckets)
+        for (auto const& nodeWeakPtr : bucket.nodes)
+            if (auto node = nodeWeakPtr.lock())
+            {
+                nodesByDistanceToTarget.emplace(distance(_target, node->id()), node);
 
-    // if d is 0, then we roll look forward, if last, we reverse, else, spread from d
-    if (head > 1 && tail != lastBin)
-        while (head != tail && head < s_bins)
-        {
-            Guard l(x_state);
-            for (auto const& n : m_buckets[head].nodes)
-                if (auto p = n.lock())
-                    found[distance(_target, p->id())].push_back(p);
-
-            if (tail)
-                for (auto const& n : m_buckets[tail].nodes)
-                    if (auto p = n.lock())
-                        found[distance(_target, p->id())].push_back(p);
-
-            head++;
-            if (tail)
-                tail--;
-        }
-    else if (head < 2)
-        while (head < s_bins)
-        {
-            Guard l(x_state);
-            for (auto const& n : m_buckets[head].nodes)
-                if (auto p = n.lock())
-                    found[distance(_target, p->id())].push_back(p);
-            head++;
-        }
-    else
-        while (tail > 0)
-        {
-            Guard l(x_state);
-            for (auto const& n : m_buckets[tail].nodes)
-                if (auto p = n.lock())
-                    found[distance(_target, p->id())].push_back(p);
-            tail--;
-        }
+                if (nodesByDistanceToTarget.size() > s_bucketSize)
+                    nodesByDistanceToTarget.erase(--nodesByDistanceToTarget.end());
+            }
 
     vector<shared_ptr<NodeEntry>> ret;
-    for (auto& nodes : found)
-        for (auto const& n : nodes.second)
-            if (ret.size() < s_bucketSize && n->endpoint() && isAllowedEndpoint(n->endpoint()))
-                ret.push_back(n);
+    for (auto& distanceAndNode : nodesByDistanceToTarget)
+        ret.emplace_back(move(distanceAndNode.second));
+
     return ret;
 }
 

libp2p/NodeTable.h

@@ -253,8 +253,8 @@ class NodeTable : UDPSocketEvents
     void doDiscoveryRound(NodeID _target, unsigned _round,
         std::shared_ptr<std::set<std::shared_ptr<NodeEntry>>> _tried);
 
-    /// Returns nodes from node table which are closest to target.
-    std::vector<std::shared_ptr<NodeEntry>> nearestNodeEntries(NodeID _target);
+    /// Returns s_bucketSize nodes from node table which are closest to target.
+    std::vector<std::shared_ptr<NodeEntry>> nearestNodeEntries(NodeID const& _target);
 
     /// Asynchronously drops _leastSeen node if it doesn't reply and adds _replacement node,
     /// otherwise _replacement is thrown away.

test/unittests/libp2p/net.cpp

@@ -54,7 +54,7 @@ struct TestNodeTable: public NodeTable
     static vector<pair<Public, uint16_t>> createTestNodes(unsigned _count)
     {
         vector<pair<Public, uint16_t>> ret;
-        asserts(_count <= 1000);
+        asserts(_count <= 2000);
 
         ret.clear();
         for (unsigned i = 0; i < _count; i++)
@@ -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;
 };
 
 /**
@@ -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)
+{
+    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
@@ -829,7 +917,7 @@ BOOST_AUTO_TEST_CASE(unexpectedFindNode)
 
 BOOST_AUTO_TEST_CASE(evictionWithOldNodeAnswering)
 {
-    TestNodeTableHost nodeTableHost(1000);
+    TestNodeTableHost nodeTableHost(2000);
     auto& nodeTable = nodeTableHost.nodeTable;
 
     // socket receiving PING