This repository has been archived by the owner on Oct 28, 2021. It is now read-only.
Fix failing Discovery tests #5581
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
Codecov Report
@@ Coverage Diff @@
## master #5581 +/- ##
==========================================
+ Coverage 62.15% 62.22% +0.07%
==========================================
Files 347 347
Lines 29110 29136 +26
Branches 3303 3295 -8
==========================================
+ Hits 18092 18130 +38
+ Misses 9837 9826 -11
+ Partials 1181 1180 -1 |
|
Also first I tried another approach using |
gumb0
commented
Apr 30, 2019
| for (auto const& nodeWeakPtr : bucket.nodes) | ||
| if (auto node = nodeWeakPtr.lock()) | ||
| { | ||
| nodesByDistanceToTarget.emplace(distance(_target, node->id()), node); |
There was a problem hiding this comment.
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.
gumb0
commented
Apr 30, 2019
| BOOST_REQUIRE_EQUAL(nearest.front()->id(), nodeTableHost.testNodes.front().first); | ||
| } | ||
|
|
||
| unsigned xorDistance(h256 const& _h1, h256 const& _h2) |
There was a problem hiding this comment.
I will move this function to NodeTable implementation in the next PR.
…ering test. It needs many nodes to fill the bucket.
…tries. Instead maintain a set of NodeEntries not larger than s_bucketSize
|
Rebased. |
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.
Fixes
network/nettests failures mentioned in #5544The reason for some of the network test failires was incorrect implementation of
NodeTable::nearestNodeEntriesmethod (finding the nodes from the node table closest to some target node)The old implementation tried to utilize the ordering of the nodes in the node table (they are ordered in buckets by distance to the host node) to efficiently find the nodes closest to target. It first looked for the bucket where the target node would fall, and then assumed the nodes in that bucket and around would be close to the target. However I don't think the idea was completely correct, and the implementation definitely was buggy and difficult to understand.
I ended up rewriting it in another, I think simpler, manner using
std::multisetordered by the distance to target and keeping only 16 closest nodes.Go-ethereum uses the similar apporach of iterating over all nodes and putting them into a data structure which keeps only 16 nodes closest to target.
https://github.com/ethereum/go-ethereum/blob/4c90efdf57ce87edf0d855c8cec10525875a6ab1/p2p/discover/table.go#L521-L536
https://github.com/ethereum/go-ethereum/blob/4c90efdf57ce87edf0d855c8cec10525875a6ab1/p2p/discover/table.go#L757-L781
Parity uses another, much trickier (and presumably more efficient) approach, but I decided not to complicate it like this. Also it's very different from our original approach.
https://github.com/paritytech/parity-ethereum/blob/b30b54e446981dc57835b74d550b40b36385742a/util/network-devp2p/src/discovery.rs#L422-L471