Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src/query/peers/closest: Consider alpha peers at initialization #1536

Merged
merged 16 commits into from
Apr 17, 2020

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented Apr 1, 2020

To allign with the Kademlia paper this commit makes the
ClosestPeersIter consider the closest alpha peers at initialization,
instead of num_results (which is equal to the k-value).

Aligning with the paper also applies aligning with the libp2p Golang
implementation as well as the xlattice specification.

Instead of initializing the iterator with num_results amount of nodes,
discarding the remaining, initialize the iterator with all provided
peers.

This patch allows the following scenario:

> Given an iterator with the configured goal of 1 result and an initial
set of peers of 2, one would expect the iterator to try the second peer
in case the first fails.

There is a downside to this patch. Say the iterator is initialized with
100 peers. Each peer is doomed to fail. In this case the iterator will
try each peer resulting in many connection attempts.

While the previous state is a safeguard against the scenario above, the
same could happen when the iterator is configured with num_result of 10,
the 9 first peers return 100 peers, each of them being doomed to fail,
thus the iterator would again attempt to make 100 connections only to
fail overall.


I cam across this issue looking into the following suggestion in #1473 (comment):

> It is probably not necessary to split the initial closest peers among the d iterators, instead letting all of them start out with the same list of initial peers. Then the "fairness" w.r.t. which iterator gets to query which peer will be determined by the composite disjoint iterator, without needing to also think about a "fair" initial split. That no peer is queried by multiple iterators is also still guaranteed by the parent disjoint iterator, of course.

Would it make sense to introduce a MAX_CONSECUTIVE_FAILURE constant to safeguard against many unsuccessful connection attempts?

Given an iterator with the configured goal of 1 result and an initial
set of peers of 2, one would expect the iterator to try the second peer
in case the first fails.
Instead of initializing the iterator with `num_results` amount of nodes,
discarding the remaining, initialize the iterator with all provided
peers.

This patch allows the following scenario:

> Given an iterator with the configured goal of 1 result and an initial
set of peers of 2, one would expect the iterator to try the second peer
in case the first fails.

There is a downside to this patch. Say the iterator is initialized with
100 peers. Each peer is doomed to fail. In this case the iterator will
try each peer resulting in many connection attempts.

While the previous state is a safeguard against the scenario above, the
same could happen when the iterator is configured with num_result of 10,
the 9 first peers return 100 peers, each of them being doomed to fail,
thus the iterator would again attempt to make 100 connections only to
fail overall.
@mxinden mxinden requested a review from romanb April 1, 2020 10:15
@romanb
Copy link
Contributor

romanb commented Apr 1, 2020

To be honest, this is a point that my understanding has never been entirely clear on. From the wording of the Kademlia paper as well as other sources, the lookup is supposed to start with the \alpha closest nodes to the target from the local buckets and this initial list is only extended by replies, meaning no further nodes are drawn from the local buckets. Consequently, if all the \alpha initial nodes are not reachable, the query simply fails early. At least that is how I read it. I cannot remember why I essentially chose to take k instead, probably because I feared \alpha would be too low. I suppose taking \alpha would also solve the problem with which you motivate the PR, since it is then independent from num_results (which only defaults to k but can be changed). I'm really not sure, but I hardly think that potentially iterating through the entire routing table, i.e. considering all nodes in the local buckets as potential initial nodes for any query, is what is supposed to be done. I'm more leaning towards following the existing paper(s) and documentation to the letter and using \alpha instead. It also seems to be what go-libp2p-kad is doing. What do you think?

@mxinden
Copy link
Member Author

mxinden commented Apr 1, 2020

I'm really not sure, but I hardly think that potentially iterating through the entire routing table, i.e. considering all nodes in the local buckets as potential initial nodes for any query, is what is supposed to be done.

Agreed. This should either be limited by only takeing X amount of nodes at the start, or restricting the amount of failures to a constant maximum.


I suppose taking \alpha would also solve the problem with which you motivate the PR, since it is then independent from num_results (which only defaults to k but can be changed).

This would not solve my initial issue. With the suggestion in #1473 (comment) in mind consider the following scenario:

Say we want to use 4 disjoint paths. Thus we would have one DisjointClosestPeersIter and 4 ClosestPeersIter. Each ClosestPeersIter would take the same first best \alpha (3 by default) nodes as their initial set.

  • On the first DisjointClosestPeersIter::next call the first ClosestPeersIter would return the best peer to query.

  • On the second DisjointClosestPeersIter::next call the second ClosestPeersIter would return the best peer to query. Given that this peer was already queried by the first ClosestPeersIter ClosestPeersIter::on_failure is called. The second ClosestPeersIter then suggests to query the second best node.

  • ...

  • On the forth DisjointClosestPeersIter::next call the forth ClosestPeersIter would return the best peer to query. Given that this peer was already queried by the first ClosestPeersIter ClosestPeersIter::on_failure is called. The forth ClosestPeersIter would then return the second best peer. Given that this peer was already queried by the second ClosestPeersIter ClosestPeersIter::on_failure is called. The forth ClosestPeersIter would then return the third best peer. Given that this peer was already queried by the third ClosestPeersIter ClosestPeersIter::on_failure is called.

At this point the forth ClosestPeersIter is out of peers.

Moving forward I see the following options:

  • Configure each ClosestPeersIter with a different initial set of peers, thus not following the suggestion in protocols/kad: Implement S-Kademlia's lookup over disjoint paths v2 #1473 (comment). (Which would be a bummer as (1) the suggestion is clean and (2) might lead to performance improvements, as the first ClosestPeersIter might not need the second best peer, thus one of the others can use it.)

  • Configure ClosestPeersIter to take k initial peers instead of num_results.

  • Configure ClosestPeersIter to take some X initial peers, introduce a MAX_FAILURES constant (e.g. equal to \alpha), and introduce a new state Ignored for peers which is set when a ClosestPeersIter tries to query a node which was already queried by a different ClosestPeersIter.

What do you think @romanb?

@romanb
Copy link
Contributor

romanb commented Apr 1, 2020

I suppose taking \alpha would also solve the problem with which you motivate the PR, since it is then independent from num_results (which only defaults to k but can be changed).

This would not solve my initial issue. With the suggestion in #1473 (comment) in mind consider the following scenario:

Say we want to use 4 disjoint paths. Thus we would have one DisjointClosestPeersIter and 4 ClosestPeersIter. Each ClosestPeersIter would take the same first best \alpha (3 by default) nodes as their initial set. [..]

I admit that I already assumed that we would always take d = \alpha, as I (or rather Ximin) suggested on the other PR. Thus there would always be the same number of peers in the initial list as there are ClosestPeersIters in the disjoint iterator. Is your scenario still an issue then or do you have objections to that suggestion?

@mxinden
Copy link
Member Author

mxinden commented Apr 1, 2020

Thinking about this I find the following path forward most suitable:

  • A ClosestPeersIter considers all peers passed to it through its constructors (new, with_config).

  • Within query.rs on initialization of a ClosestPeersIter only the best \alpha peers are passed in.

  • With protocols/kad: Implement S-Kademlia's lookup over disjoint paths v2 #1473 a DisjointClosestPeersIter initializes each ClosestPeersIter with the same best \alpha * num_disjoint_paths peers.

@mxinden
Copy link
Member Author

mxinden commented Apr 1, 2020

@romanb and I discussed the following via chat:

I admit that I already assumed that we would always take d = \alpha, as I (or rather Ximin) suggested on the other PR. Thus there would always be the same number of peers in the initial list as there are ClosestPeersIters in the disjoint iterator. Is your scenario still an issue then or do you have objections to that suggestion?

Say the number of disjoint paths and alpha is 3. Thus all of the ClosestPeersIter consider the same initial set of 3 peers. Say a single of those 3 peers is unreachable. Thus one of the ClosestPeersIter would not get a single response, thus is Finished. Thereby a single non-available peer would force us to explore only 2 disjoint paths.

This would be solved by the suggestion above using the same set of \alpha * num_disjoint_paths best peers for all ClosestPeersIter of a DisjointClosestPeersIter (where \alpha might be equal to num_disjoint_paths).

@romanb
Copy link
Contributor

romanb commented Apr 1, 2020

@romanb and I discussed the following via chat:

I admit that I already assumed that we would always take d = \alpha, as I (or rather Ximin) suggested on the other PR. Thus there would always be the same number of peers in the initial list as there are ClosestPeersIters in the disjoint iterator. Is your scenario still an issue then or do you have objections to that suggestion?

Say the number of disjoint paths and alpha is 3. Thus all of the ClosestPeersIter consider the same initial set of 3 peers. Say a single of those 3 peers is unreachable. Thus one of the ClosestPeersIter would not get a single response, thus is Finished. Thereby a single non-available peer would force us to explore only 2 disjoint paths.

True, your last suggestion sounds good to me then, though it may then actually still be preferable to partition the initial list of peers to make sure each iterator gets at least \alpha peers to try from that list.

To allign with the Kademlia paper this commit makes the
`ClosestPeersIter` consider the closest alpha peers at initialization,
instead of `num_results` (which is equal to the k-value).

Aligning with the paper also applies aligning with the libp2p Golang
implementation as well as the xlattice specification.
@mxinden mxinden changed the title src/query/peers/closest: Consider all peers at initialization src/query/peers/closest: Consider alpha peers at initialization Apr 1, 2020
@mxinden
Copy link
Member Author

mxinden commented Apr 1, 2020

I have reverted my initial commit 4fb9366 which considered all peers passed at initialization.

I have added dd87819 which makes ClosestPeersIter choose \alpha peers at initialization instead of num_results. This would be inline with #1536 (comment).

This pull request still includes a regression test. I would suggest removing it as it adds more noise than being actually helpful.

True, your last suggestion sounds good to me then, though it may then actually still be preferable to partition the initial list of peers to make sure each iterator gets at least \alpha peers to try from that list.

This I would address in #1473.

@romanb what do you think?

@romanb
Copy link
Contributor

romanb commented Apr 2, 2020

Sounds good, the tests seem to be stalling though (judging from CI)?

@mxinden mxinden closed this Apr 3, 2020
@mxinden mxinden reopened this Apr 3, 2020
@mxinden
Copy link
Member Author

mxinden commented Apr 8, 2020

Sounds good, the tests seem to be stalling though (judging from CI)?

I went down quite a rabbit hole to understand the failures. As far as I can tell my changes to to behaviour/tests.rs retain the status quo reasonably well. In order to understand the reasoning behind the large amount of changes, each commit has a detailed description.

I think it is easiest to review the new commits one by one with each commit message in mind:

  • protocols/kad/src/behaviour/test: Enable building single nodes b8f85fc

  • protocols/kad/behaviour/test: Fix bootstrap test initialization 584aaef

  • protocols/kad/behaviour/test: Fix put_record and get_provider ab134d7

@romanb would you mind taking another look?

Introduces the `build_node` function to build a single not connected
node. Along the way replace the notion of a `port_base` with returning
the actual `Multiaddr` of the node.
When looking for the closest node to a key, Kademlia considers
ALPHA_VALUE nodes to query at initialization. If `num_groups` is larger
than ALPHA_VALUE the remaining locally known nodes will not be
considered. Given that no other node is aware of them other than node 1,
they would be lost entirely. To prevent the above restrict `num_groups`
to be equal or smaller than ALPHA_VALUE.
In the past, when trying to find the closest nodes to a key, Kademlia
would consider `num_result` amount of nodes to query out of all the
nodes it is aware of.

Both the `put_record` and the `get_provider` tests initialized their
swarms in the same way. The tests took the replication factor to use as
an input. The number of results to get was equal to the replication
factor. The amount of swarms to start was twice the replication factor.
Nodes would be grouped in two groups a replication factor nodes. The
first node would be aware of all of the nodes in the first group. The
last node of the first group would be aware of all the nodes in the
second group.

By coincidence (I assume) these numbers played together very well. At
initialization the first node would consider `num_results` amount of
peers (see first paragraph). It would then contact each of them. As the
first node is aware of the last node of the first group which in turn is
aware of all nodes in the second group, the first node would eventually
discover all nodes.

Recently the amount of nodes Kademlia considers at initialization when
looking for the nodes closest to a key was changed to only consider
ALPHA nodes.

With this in mind playing through the test setup above again would
result in (1) `replication_factor - ALPHA` nodes being entirely lost as
the first node would never consider them and (2) the first node probably
never contacting the last node out of the first group and thus not
discovering any nodes of the second group.

To keep the multi hop discovery in place while not basing ones test
setup on the lucky assumption of Kademlia considering replication factor
amount of nodes at initialization, this patch alters the two tests:

Build a fully connected set of nodes and one addition node (the first
node). Connect the first node to a single node of the fully connected
set (simulating a boot node). Continue as done previously.
@mxinden
Copy link
Member Author

mxinden commented Apr 14, 2020

@romanb friendly ping. Would you mind taking another look?

@mxinden
Copy link
Member Author

mxinden commented Apr 14, 2020

Thanks for the quick feedback! Would you mind taking another look @romanb?

@romanb
Copy link
Contributor

romanb commented Apr 14, 2020

Regarding ab134d7, is it really necessary to create a fully connected graph? Wouldn't it suffice to ensure num_group <= \alpha (which I think should be done in the build_ function(s)). It is certainly another good test case, but a fully-connected graph is naturally not a very realistic scenario. I'm generally OK with it, just wondering, because one motivation for grouping the connectivity as done before is to exercise the iterative queries a bit more: With a fully connected graph, the alpha initial nodes will all return the same peers, from which all but these alpha nodes are successfully queried (again returning the same sets of peers) and the query finishes. In other words, I think the "result list" of the query in such a scenario is fixed already with the first response and won't need to change.

@mxinden
Copy link
Member Author

mxinden commented Apr 14, 2020

Wouldn't it suffice to ensure num_group <= \alpha?

Problem would be that put_record and add_provider vary the replication_factor which in turn determines num_results.

Given replication_factor = 6 num_total would be 12. num_group should normally be 6, but given the requirement of num_group <= \alpha we set it to 3. Imagine the following directed graph:

V = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12}

E = {{1, 11}, {1, 10}, {1, 9}, {11, 8}, {11, 7}, {11, 6}, {8, 5}, {8, 4}, {8, 3}, {5, 2}, {5, 12}}

If we want to store the key 13 with a replication_factor of 6 we would expect it to be stored at nodes {12, 11, 10, 9, 8, 7} as they are closest to the key.

A put_record starting from node 1 would initialize its ClosestPeersIter with nodes {11, 10, 9}. Querying 11 would reveal {8, 7, 6}. Querying 10 or 9 reveals nothing. Querying 8 reveals {5, 4, 3}. Querying 7 or 6 reveals nothing. At this point the ClosestPeersIter knows nodes {11, 10, 9, 8, 7, 6, 5, 4, 3}. Given that the first 6 (== num_results) nodes were successfully contacted it terminates. The value would then be stored at {11, 10, 9, 8, 7, 6} instead of {12, 11, 10, 9, 8, 7}.

@romanb does the above make sense?

@romanb
Copy link
Contributor

romanb commented Apr 14, 2020

I see, thanks for the example. Fully connected or not, I forgot that these tests rely on always finding the closest nodes to the key out of all nodes, and that there are intentionally more nodes than the replication factor (but not more than K_VALUE to avoid potential bucket overflow). Hence this setup absolutely makes sense.

ps. I think ab134d7 should talk about add_provider, not get_provider in the commit message. I was confused for a moment.

Copy link
Contributor

@romanb romanb left a comment

Choose a reason for hiding this comment

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

This looks good to me, a nice improvement to the tests in particular. However, I'm still a bit on the fence w.r.t. the use of \alpha. It seems that go-libp2p-kad-dht now even changed their behaviour to what we are currently doing (almost). In light of that, for reasons of trying to provide analogous behaviour in rust-libp2p and to keep backward-compatibility, maybe we should just use .take(K_VALUE)? It is still a good idea not to use the configured num_results, as is done right now. The test improvements would remain valid, of course. What do you think?

@mxinden
Copy link
Member Author

mxinden commented Apr 14, 2020

I am fine with both \alpha as well as K_VALUE. I don't see a way how either could violate correctness. The more information we can give a ClosestPeersIter the better, thus let's go with K_VALUE.

While preparing a commit I noticed termination_and_parallelism to fail when taking K_VALUE nodes at initialization. This boils down to at_capacity setting num_results parallelism for a stalled iterator even when num_results is smaller than parallelism. #1548 fixes it.

Very happy we have all these Quickcheck tests.

@mxinden
Copy link
Member Author

mxinden commented Apr 16, 2020

#1548 is merged and 4c0c60f makes ClosestPeersIter consider K_VALUE nodes at initialization.

@romanb could you give this another review?

@@ -695,6 +695,32 @@ mod tests {
}

#[test]
fn try_all_provided_peers_on_failure() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Will make this a prop test as well.

@mxinden mxinden merged commit 9f981a4 into libp2p:master Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants