Skip to content
This repository has been archived by the owner on Jul 21, 2023. It is now read-only.

fix: re-enable ensuring queries run along disjoint paths #371

Merged
merged 9 commits into from
Sep 29, 2022
Merged

fix: re-enable ensuring queries run along disjoint paths #371

merged 9 commits into from
Sep 29, 2022

Conversation

zeroxbt
Copy link
Contributor

@zeroxbt zeroxbt commented Sep 7, 2022

I noticed that during the getClosestPeers operation, the query paths might overlap and the same nodes will be queried multiple times.
There is no need to query a node if it has been queried during the execution of another query path, so the peersSeen set should be shared among different query paths belonging to the same query.

Copy link
Member

@wemeetagain wemeetagain left a comment

Choose a reason for hiding this comment

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

LGTM

@achingbrain should also have a look

@@ -235,8 +235,6 @@ export class PeerRouting implements Initializable {
}

for await (const event of this.queryManager.run(key, tablePeers, getCloserPeersQuery, options)) {
yield event
Copy link
Member

@achingbrain achingbrain Sep 12, 2022

Choose a reason for hiding this comment

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

Why has this been removed? Yielding intermediate events is useful for query diagnostics and tracing.

Copy link
Contributor Author

@zeroxbt zeroxbt Sep 12, 2022

Choose a reason for hiding this comment

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

From my understanding, getClosestPeers should return only kBucketSize closest peers. This implementation currently also returns multiple "in between" peers instead coming from events other than "FINAL_PEER".
I removed this so that only final peers are returned. It could be there is a misunderstanding on my part. If that's the case, I can add it back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would expect this function to yield the kBucketSize sorted peers, but it's yielding a lot more than expected (not all are FINAL_PEER), and not necessarily sorted (because it merges events yielded by both lan and wan dhts).

@achingbrain
Copy link
Member

This feature is called disjoint paths - it's in the S/Kademlia paper and this impl used to support it.

Notably the go KAD-DHT implementation does not have this feature which is why it was removed from this repo, though I notice the rust implementation does support it so there's no harm in re-adding it here, and is certainly worth doing if it makes the queries run faster.

Thanks for opening this!

src/query/manager.ts Outdated Show resolved Hide resolved
@achingbrain
Copy link
Member

The final thing to do to get this in is to write some tests.

For an example you can see how we create predictable network topologies in test/query.spec.js and run a query which records the peers it passed through to get to the final value.

You should be able to validate in a test that queries stop when passing through the same node twice.

@mpetrunic mpetrunic added the need/maintainers-input Needs input from the current maintainer(s) label Sep 20, 2022
test/query.spec.ts Outdated Show resolved Hide resolved
src/query/manager.ts Outdated Show resolved Hide resolved
@achingbrain achingbrain changed the title make query path peersSeen shared by multiple query paths fix: re-enable ensuring queries run along disjoint paths Sep 29, 2022
@achingbrain achingbrain merged commit 5ae4440 into libp2p:master Sep 29, 2022
@achingbrain
Copy link
Member

Thanks!

github-actions bot pushed a commit that referenced this pull request Sep 29, 2022
## [3.0.6](v3.0.5...v3.0.6) (2022-09-29)

### Bug Fixes

* re-enable ensuring queries run along disjoint paths ([#371](#371)) ([5ae4440](5ae4440))

### Trivial Changes

* remove IRC badge from readme ([#374](#374)) ([e48754f](e48754f))
@github-actions
Copy link

🎉 This PR is included in version 3.0.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
need/maintainers-input Needs input from the current maintainer(s) released
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants