Refactoring of iterative queries (`query.rs`) to improve both
correctness and performance (for larger DHTs):
Correctness:
1. Queries no longer terminate prematurely due to counting results
from peers farther from the target while results from closer
peers are still pending. (#1105).
2. Queries no longer ignore reported closer peers that are not duplicates
just because they are currently not among the `num_results` closest.
The currently `max_results` closest may contain peers marked as failed
or pending / waiting. Hence all reported closer peers that are not
duplicates must be considered candidates that may still end up
among the `num_results` closest that successfully responded.
3. Bounded parallelism based on the `active_counter` was not working
correctly, as new (not yet contacted) peers closer to the target
may be discovered at any time and thus appear in `closer_peers`
before the already active / pending peers.
4. The `Frozen` query mechanism allowed all remaining not-yet contacted
peers to be contacted, but their results were discarded, because
`inject_rpc_result` would only incorporate results while the
query is `Iterating`. The `Frozen` state has been reworked into
a `Stalled` state that implements a slightly more permissive
variant of the following from the paper / specs: "If a round of
FIND_NODEs fails to return a node any closer than the closest
already seen, the initiator resends the FIND_NODE to all of the
k closest nodes it has not already queried.". Importantly, though
not explicitly mentioned, the query can move back to `Iterating`
if it makes further progress again as a result of these requests.
The `Stalled` state thus allows (temporarily) higher parallelism
in an effort to make progress and bring the query to an end.
Performance:
1. Repeated distance calculations between the same peers and the
target is avoided.
2. Enabled by #1108, use of a more appropriate data structure (`BTreeMap`) for
the incrementally updated list of closer peers. The data structure needs
efficient lookups (to avoid duplicates) and insertions at any position,
both of which large(r) vectors are not that good at. Unscientific benchmarks
showed a ~40-60% improvement in somewhat pathological scenarios with at least
20 healthy nodes, each possibly returning a distinct list of closer 20 peers
to the requestor. A previous assumption may have been that the vector always
stays very small, but that is not the case in larger clusters: Even if the
lists of closer peers reported by the 20 contacted peers are heavily overlapping,
typically a lot more than 20 peers have to be (at least temporarily) considered
as closest peers until the query completes. See also issue (2) above.
New tests are added for:
* Query termination conditions.
* Bounded parallelism.
* Absence of duplicates.
* initial implementation of the records
* move to multihash keys
* correctly process query results
* comments and formatting
* correctly return closer_peers in query
* checking wrong peer id in test
* Apply suggestions from code review
Co-Authored-By: Roman Borschel <romanb@users.noreply.github.com>
* Fix changes from suggestions
* Send responses to PUT_VALUE requests
* Shortcut in get_value
* Update protocols/kad/src/behaviour.rs
Co-Authored-By: Roman Borschel <romanb@users.noreply.github.com>
* Revert "Update protocols/kad/src/behaviour.rs"
This reverts commit 579ce742a7f4c94587f1e1f0866d2a3a37418efb.
* Remove duplicate insertion
* Adds a record to a PUT_VALUE response
* Fix a racy put_value test
* Store value ourselves only if we are in K closest
* Abstract over storage
* Revert "Abstract over storage": bad take
This reverts commit eaebf5b6d915712eaf3b05929577fdf697f204d8.
* Abstract over records storage using hashmap as default
* Constructor for custom records
* New Record type and its traits
* Fix outdated storage name
* Fixes returning an event
* Change FindNodeReq key type to Multihash
* WriteState for a second stage of a PUT_VALUE request
* GET_VALUE should not have a record
* Refactor a match arm
* Add successes and failures counters to PutValueRes
* If value is found no need to return closer peers
* Remove a custo storage from tests
* Rename a test to get_value_not_found
* Adds a TODO to change FindNode request key to Multihash
Co-Authored-By: Roman Borschel <romanb@users.noreply.github.com>
* Move MemoryRecordStorage to record.rs
* Return a Cow-ed Record from get
* Fix incorrect GET_VALUE parsing
* Various fixes with review
* Fixes get_value_not_found
* Fix peerids names in test
* another fix
* PutValue correctly distributes values
* Simplify the test
* Check that results are actually the closest
* Reverts changes to tests
* Fix the test topology and checking the results
* Run put_value test ten times
* Adds a get_value test
* Apply suggestions from code review
Co-Authored-By: Roman Borschel <romanb@users.noreply.github.com>
* Make Record fields public
* Moves WriteState to write.rs
* A couple of minor fixes
* Another few fixes of review
* Simplify the put_value test
* Dont synchronously return an error from put_value
* Formatting fixes and comments
* Collect a bunch of results
* Take exactly as much elements as neede
* Check if the peer is still connected
* Adds a multiple GetValueResults results number test
* Unnecessary mut iterators in put_value
* Ask for num_results in get_value
* Dont allocate twice in get_value
* Dont count same errored peer multiple times
* Apply suggestions from code review
Co-Authored-By: Roman Borschel <romanb@users.noreply.github.com>
* Fix another review
* Apply suggestions from code review
Co-Authored-By: Pierre Krieger <pierre.krieger1708@gmail.com>
* Bring back FromIterator and improve a panic message
* Update protocols/kad/src/behaviour.rs
Co-Authored-By: Pierre Krieger <pierre.krieger1708@gmail.com>
* Fix status updates in KBuckets.
`KBucket::update` does currently not correctly update the `first_connected_pos`
before reinserting the node. This may result in connected nodes being considered
disconnected and thus eligible for replacement by a pending node if the bucket is
full and a new connected node is added.
Tests have been added for checking that `KBucket::update` preserves the status
and ordering of all other nodes in the bucket.
* Small test improvement.
Set an expectation for the new position, instead of taking the assigned position.
* Kademlia: Optimise iteration over closest entries.
The current implementation for finding the entries whose keys are closest
to some target key in the Kademlia routing table involves copying the
keys of all buckets into a new `Vec` which is then sorted based on the
distances to the target and turned into an iterator from which only a
small number of elements (by default 20) are drawn.
This commit introduces an iterator over buckets for finding the closest
keys to a target that visits the buckets in the optimal order, based on
the information contained in the distance bit-string representing the
distance between the local key and the target.
Correctness is tested against full-table scans.
Also included:
* Updated documentation.
* The `Entry` API was moved to the `kbucket::entry` sub-module for
ease of maintenance.
* The pending node handling has been slightly refactored in order to
bring code and documentation in agreement and clarify the semantics
a little.
* Rewrite pending node handling and add tests.
There are two issues with the current definition and use of Kademlia's
XOR metric:
1. The distance is currently equated with the bucket index, i.e.
`distance(a,b) - 1` is the index of the bucket into which either
peer is put by the other. The result is a metric that is not
unidirectional, as defined in the Kademlia paper and as implemented
in e.g. libp2p-go and libp2p-js, which is to interpret the result
of the XOR as an integer in its entirety.
2. The current `KBucketsPeerId` trait and its instances allow computing
distances between types with differing bit lengths as well as between
types that hash all inputs again (i.e. `KadHash`) and "plain" `PeerId`s
or `Multihash`es. This can result in computed distances that are either
incorrect as per the requirement of the libp2p specs that all distances
are to be computed from the XOR of the SHA256 of the input keys, or
even fall outside of the image of the metric used for the `KBucketsTable`.
In the latter case, such distances are not currently used as a bucket index
- they can only occur in the context of comparing distances for the purpose
of sorting peers - but that still seems undesirable.
These issues are addressed here as follows:
* Unidirectionality of the XOR metric is restored by keeping the "full"
integer representation of the bitwise XOR. The result is an XOR metric
as defined in the paper. This also opens the door to avoiding the
"full table scan" when searching for the keys closest to a given key -
the ideal order in which to visit the buckets can be computed with the
help of the distance bit string.
* As a simplification and to make it easy to "do the right thing", the
XOR metric is only defined on an opaque `kbucket::Key` type, partially
derived from the current `KadHash`. `KadHash` and `KBucketsPeerId`
are removed.
Although not explicitly mentioned in the paper, it seems clear that
including an entry for the requesting peer in a FIND_NODE response
never gives useful information and just occupies a result slot that may
have been better filled with another peer that the requestor may not
know about.
There is one explicit mention that this is the desired behavior
in a somewhat dated design document of another p2p framework [1]:
"The recipient of a FIND_NODE should never return a triple containing
the nodeID of the requestor."
The same reasoning supposedly applies to the libp2p-specific `GET_PROVIDERS`
request.
[1] http://xlattice.sourceforge.net/components/protocol/kademlia/specs.html#FIND_NODE
* Fix self-dialing in Kademlia.
Addresses https://github.com/libp2p/rust-libp2p/issues/341 which is the cause
for one of the observations made in https://github.com/libp2p/rust-libp2p/issues/1053.
However, the latter is not assumed to be fully addressed by these changes and
needs further investigation.
Currently, whenever a search for a key yields a response containing the initiating
peer as one of the closest peers known to the remote, the local node
would attempt to dial itself. That attempt is ignored by the Swarm, but
the Kademlia behaviour now believes it still has a query ongoing which is
always doomed to time out. That timeout delays successful completion of the query.
Hence, any query where a remote responds with the ID of the local node takes at
least as long as the `rpc_timeout` to complete, which possibly affects almost
all queries in smaller clusters where every node knows about every other.
This problem is fixed here by ensuring that Kademlia never tries to dial the local node.
Furthermore, `Discovered` events are no longer emitted for the local node
and it is not inserted into the `untrusted_addresses` from discovery, as described
in #341.
This commit also includes a change to the condition for freezing / terminating
a Kademlia query upon receiving a response. Specifically, the condition is
tightened such that it only applies if in addition to `parallelism`
consecutive responses that failed to yield a peer closer to the target, the
last response must also either not have reported any new peer or the
number of collected peers has already reached the number of desired results.
In effect, a Kademlia query now tries harder to actually return `k`
closest peers.
Tests have been refactored and expanded.
* Add another comment.
* Bump to 0.7.0
* Update CHANGELOG.md
Co-Authored-By: tomaka <pierre.krieger1708@gmail.com>
* Update for #1078
* New version of multihash and multiaddr as well
The functionality is available through `Multiaddr::replace`.
What we currently call "nat_traversal" is merley a replacement of an IP
address prefix in a `Multiaddr`, hence it can be done directly on
`Multiaddr` values instead of having to go through a `Transport`.
In addition this PR consolidates changes made to `Multiaddr` in
previous commits which resulted in lots of deprecations. It adds some
more (see below for the complete list of API changes) and removes all
deprecated functionality, requiring a minor version bump.
Here are the changes to `multiaddr` compared to the currently published
version:
1. Removed `into_bytes` (use `to_vec` instead).
2. Renamed `to_bytes` to `to_vec`.
3. Removed `from_bytes` (use the `TryFrom` impl instead).
4. Added `with_capacity`.
5. Added `len`.
6. Removed `as_slice` (use `AsRef` impl instead).
7. Removed `encapsulate` (use `push` or `with` instead).
8. Removed `decapsulate` (use `pop` instead).
9. Renamed `append` to `push`.
10. Added `with`.
11. Added `replace`.
12. Removed `ToMultiaddr` trait (use `TryFrom` instead).
* libp2p-ping improvements.
* re #950: Removes use of the `OneShotHandler`, but still sending each
ping over a new substream, as seems to be intentional since #828.
* re #842: Adds an integration test that exercises the ping behaviour through
a Swarm, requiring the RTT to be below a threshold. This requires disabling
Nagle's algorithm as it can interact badly with delayed ACKs (and has been
observed to do so in the context of the new ping example and integration test).
* re #864: Control of the inbound and outbound (sub)stream protocol upgrade
timeouts has been moved from the `NodeHandlerWrapperBuilder` to the
`ProtocolsHandler`. That may also alleviate the need for a custom timeout
on an `OutboundSubstreamRequest` as a `ProtocolsHandler` is now free to
adjust these timeouts over time.
Other changes:
* A new ping example.
* Documentation improvements.
* More documentation improvements.
* Add PingPolicy and ensure no event is dropped.
* Remove inbound_timeout/outbound_timeout.
As per review comment, the inbound timeout is now configured
as part of the `listen_protocol` and the outbound timeout as
part of the `OutboundSubstreamRequest`.
* Simplify and generalise.
Generalise `ListenProtocol` to `SubstreamProtocol`, reusing it in
the context of `ProtocolsHandlerEvent::OutboundSubstreamRequest`.
* Doc comments for SubstreamProtocol.
* Adapt to changes in master.
* Relax upper bound for ping integration test rtt.
For "slow" CI build machines?
Replace the listener and address pair returned from `Transport::listen_on` with just a listener that produces `ListenerEvent` values which include upgrades as well as address changes.
* Some k-buckets improvements
* Apply suggestions from code review
Co-Authored-By: tomaka <pierre.krieger1708@gmail.com>
* Use NonZeroUsize for the distance
* Update TODO comment
* Move QueryTarget to the behaviour
* Rework query system
* Add a few tests
* Add some Kademlia tests
* More tests
* Don't return self entry
* Fix tests
Initially I had hoped that the deprecated `#![allow(clippy)]` would no
longer be put into the generated rust files, but -- as of 2019-01-30 --
it still is (see [1] for details). Since we explicitly update the
protobuf files I decided to *manually edit the generated code* and
replace this with `#![allow(clippy:all)]`. Hopefully, by the time we do
the next upgrade, no such manual tweaking would be necessary anymore. I
think the benefit of a less polluted clippy output is worth it this
time.
[1]: https://github.com/stepancheg/rust-protobuf/pull/332
* Embed the topology in the NetworkBehaviour
* Put topologies inside of Floodsub and Kad
* Fix core tests
* Fix chat example
* More work
* Some cleanup
* Restore external addresses system
