Kademlia: Refactor iterative queries.

[romanb]

This is a suggested refactoring of iterative queries (query.rs). It addresses the following issues:

1. Queries no longer terminate prematurely due to counting results from peers farther from the target while results from closer peers are still pending. (Kademlia query termination isn't properly implemented #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 num_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. Moreover, the Frozen state was a terminal state. 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.

Other Changes

• Repeated distance calculations between the same peers and the target is avoided.

• Enabled by Kademlia: Correct XOR metric. #1108, a more appropriate data structure (BTreeMap keyed by Distance) for the incrementally updated list of closer peers is used. Besides resulting in a simpler implementation, it yields better performance for queries in larger DHTs. The data structure needs efficient lookups (to avoid duplicates) and insertions at any position, both of which larger(r) vectors are not that good at. Some unscientific comparisons showed a ~40-60% improvement in somewhat pathological scenarios with at least 20 healthy nodes, each 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 DHTs: 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 candidates for the closest peers until the query completes. See also issue (2) above. Even in the case where the list stays close to 20 peers throughout, it still requires a lookup for every single reported closer peer to avoid duplicates.

• Cancellation of still pending RPCs upon timeout has been removed. Besides only canceling RPCs that have not yet been sent - a rare case after the timeout elapsed - the implementation actually removed all pending RPCs for a peer, regardless of the query they belong(ed) to. Meaning that a timeout in one query could cancel a pending RPC of a concurrent, more recently started query to the same peer. It is according to the Kademlia paper / specs that a peer that does not respond will not stop the query from terminating, but the query still accepts / incorporates delayed results that come in while the query is still running. This has been realised by further distinguishing between QueryPeerState::Failed (permanent) and QueryPeerState::Unresponsive (timeout).

New Tests

New tests have been added for:

• Query termination conditions.
• Bounded parallelism.
• Absence of duplicates.

[tomaka]

What is the relationship between this PR and #1144?
Am I right to think that #1144 should go in first?

[romanb]

What is the relationship between this PR and #1144?
Am I right to think that #1144 should go in first?

Yes, that was my intention.

[montekki]

@tomaka @romanb I think that at least #1155 should go in before #1144 , shouldn't it?

[romanb]

@tomaka @romanb I think that at least #1155 should go in before #1144 , shouldn't it?

Of course, that's why I extracted it into a separate PR.

[AgeManning]

on_sucess -> on_success

[tomaka]

I wanted to looked more into details, but keep delaying this.
I took a glance a while ago, and looked good.