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

Kademlia: Speed-up the record fetching #13081

Merged
merged 1 commit into from
Jan 5, 2023
Merged

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Jan 5, 2023

Before libp2p 0.50.0 we used a quorum of one to fetch records from the DHT. In the pr that upgraded to libp2p 0.50.0 we accidentally changed this behavior. This pr brings back the old behavior of using a qorum of one and thus, a faster discovery. After finding the first value, we directly finish the query. There was also another behavior change in libp2p, they stopped automatic caching on remote nodes. This pr also brings back the remote caching on nodes that are nearest to the key from our point of view of the network.

The pr that changed the behavior in libp2p: libp2p/rust-libp2p#2712

Before libp2p 0.50.0 we used a quorum of one to fetch records from the DHT. In the pr that upgraded
to libp2p 0.50.0 we accidentally changed this behavior. This pr brings back the old behavior of
using a qorum of one and thus, a faster discovery. After finding the first value, we directly finish
the query. There was also another behavior change in libp2p, they stopped automatic caching on
remote nodes. This pr also brings back the remote caching on nodes that are nearest to the key from
our point of view of the network.

The pr that changed the behavior in libp2p: libp2p/rust-libp2p#2712
@bkchr bkchr added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jan 5, 2023
Copy link
Contributor

@dmitry-markin dmitry-markin left a comment

Choose a reason for hiding this comment

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

LGTM!

client/network/src/discovery.rs Show resolved Hide resolved
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

I dont know the code but it fixes the zombienet-tests-misc-paritydb test in 30s here: paritytech/polkadot#6018

@bkchr bkchr merged commit 934d42a into master Jan 5, 2023
@bkchr bkchr deleted the bkchr-speed-up-kademlia branch January 5, 2023 21:11
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

thanks for fixing these 🙏

Ok(GetRecordOk::FinishedWithNoAdditionalRecord {
cache_candidates,
}) => {
if cache_candidates.is_empty() {
Copy link
Contributor

Choose a reason for hiding this comment

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

@bkchr A bit late to this party, but should we not call self.records_to_publish.remove(&id) here?
I think if cache_candidates is empty, we could still have added something to records_to_publish for this query id which would not be cleared then.

Copy link
Member Author

Choose a reason for hiding this comment

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

🙈

Copy link
Member Author

Choose a reason for hiding this comment

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

ltfschoen pushed a commit to ltfschoen/substrate that referenced this pull request Feb 22, 2023
Before libp2p 0.50.0 we used a quorum of one to fetch records from the DHT. In the pr that upgraded
to libp2p 0.50.0 we accidentally changed this behavior. This pr brings back the old behavior of
using a qorum of one and thus, a faster discovery. After finding the first value, we directly finish
the query. There was also another behavior change in libp2p, they stopped automatic caching on
remote nodes. This pr also brings back the remote caching on nodes that are nearest to the key from
our point of view of the network.

The pr that changed the behavior in libp2p: libp2p/rust-libp2p#2712
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
Before libp2p 0.50.0 we used a quorum of one to fetch records from the DHT. In the pr that upgraded
to libp2p 0.50.0 we accidentally changed this behavior. This pr brings back the old behavior of
using a qorum of one and thus, a faster discovery. After finding the first value, we directly finish
the query. There was also another behavior change in libp2p, they stopped automatic caching on
remote nodes. This pr also brings back the remote caching on nodes that are nearest to the key from
our point of view of the network.

The pr that changed the behavior in libp2p: libp2p/rust-libp2p#2712
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants