-
Notifications
You must be signed in to change notification settings - Fork 60
Scope of queries #103
Comments
We will start by testing the initial change in the real network and take conclusions on it. With the obtained results, we aim to understand how we can improve the changed queries, as well as to create proposals for the remaining ones. |
👍 Thanks for putting this together @vasco-santos |
I spent some time today testing out queries with the latest js-ipfs and I think I discovered some issues. I was seeing some Issues1. Slow peers block.Currently all queries operate under a concurrency of ALPHA (3). This is fine, however, if we are trying to query a peer that is slow or never responds we don't move on quickly, we wait. As the spec mentions, we should remove them from consideration. If we end up with 3 bad peers at the front of the queue, the query could be stuck for a significant amount of time.
2. Query logicWith the latest logic, we start off querying ALPHA peers. Each query that returns closer peers, has those peers added to the query queue. Each time we start the next query in the queue, we check to see if we have already queried kValue peers and if we have any peers in the queue closer than those. If we have closer peers, we keep running the queue, with no priority. The spec indicates we should be querying the closest peers we're aware of. So instead of just running through the queue of peers, the running queries should be composed of ALPHA peers we have not yet queried, from the top kValue (20) closest peers we know.
ProposalAdd every unique peer found via querying to the What does this do? Currently, that 20th peer will be close to the end of the queue. This means we will end up querying every peer before that peer, even though they are further away. Ideally, we should immediately grab that peer, query it and be done. @dirkmc @vasco-santos if this sounds correct, I can work on the adjustments. |
@jacobheun I think it's a good idea to reduce the timeout, 60s seems way too long to hear back from a peer. |
Ah, I missed that. It's specific to a single Path though? Shouldn't we use a single PeerQueue to pull from? Using separate queues runs a risk of having paths finish much earlier than others. For example, if I take the 3 closest peers from my routing table and query them and two of them are no longer online, won't I lose 66% of my query power, and still have at least 19 queries to make? It seems like moving the |
As I understand it, the original Kademlia just queries until it finds
Now that I've re-read this it occurs to me that instead of starting with |
Great analysis! As @dirkmc stated, the In the disjoint paths lookup, we differentiate the queries per path as a security mechanism. But, can't we understand that a query has no more peers to query and divide the larger of the other queries and populate the empty one with half the peers? Finally, getting |
Oh yeah, dang, that will be a significant improvement. I was reading the original paper, I'll dive into the S/Kademlia paper more today. I'll work on making these updates and get a WIP PR started soon so we can discuss more there. |
In the context of improving the performance of the DHT operations @dirkmc did a great job limiting the scope of queries to k closest peers on libp2p/js-libp2p-kad-dht#97. There was a good discussion in the PR regarding where we should tweak the scope of queries, according to the type of DHT operation, in order to not compromise the proper work of the DHT and boost performance.
Queries performed
We perform queries on:
provide()
, a public API function to add self as provider of a keyput()
, a public API function, to find nodes no which to store a key/valueIn that PR, we decided to go with an initial step for limiting the scope of the first two operations.
Proposed Idea
@jacobheun proposed the following:
The text was updated successfully, but these errors were encountered: