Skip to content

Add a timeout to inbound libp2p-kad substreams#29

Closed
teor2345 wants to merge 1 commit intosubspace-v9from
kad-in-subst-timeout
Closed

Add a timeout to inbound libp2p-kad substreams#29
teor2345 wants to merge 1 commit intosubspace-v9from
kad-in-subst-timeout

Conversation

@teor2345
Copy link
Copy Markdown

This PR upgrades the libp2p version in our fork to autonomys/rust-libp2p#2

It adds an inbound substream timeout to the kad protocol, which matches the outbound substream timeout. This prevents "substream limit exceeded" errors under load, caused by the outbound side timing out, but the inbound side keeping on waiting.

See that PR for full details.

This PR is marked as draft, because the git commit hash will change when we merge the PR in our fork.

@teor2345 teor2345 requested review from nazar-pc and vedhavyas April 23, 2025 04:42
@teor2345 teor2345 self-assigned this Apr 23, 2025
@teor2345 teor2345 added the bug Something isn't working label Apr 23, 2025
@nazar-pc
Copy link
Copy Markdown
Member

This PR is unnecessary, IIRC libp2p version is already overridden in https://github.com/autonomys/subspace and we don't really use this repo outside of that. This is only needed if there are breaking changes to libp2p API, but looks like there were none here.

@teor2345
Copy link
Copy Markdown
Author

This PR is unnecessary, IIRC libp2p version is already overridden in https://github.com/autonomys/subspace and we don't really use this repo outside of that. This is only needed if there are breaking changes to libp2p API, but looks like there were none here.

Only libp2p-identity is overridden in Subspace, but this fix is in libp2p-kad:
https://github.com/autonomys/subspace/blob/a77c8aa8f057af7deab9bbf4a05e5b7c1cfbe19b/Cargo.toml#L416

Subspace needs the libp2p versions to be identical, so the PeerId types from our direct and indirect dependencies are the same Rust type.

We can override all the libp2p crates in Subspace in autonomys/subspace#3492 . But last time I tried to do that, you said it was the wrong thing to do.

@nazar-pc
Copy link
Copy Markdown
Member

Only libp2p-identity is overridden in Subspace

Then I believe it should be possible to add another override for libp2p too. You'll probably only need to do this for the libp2p umbrella crate. Either way it will be clear from lock file changes if it worked or not.

But last time I tried to do that, you said it was the wrong thing to do

It was probably a related, but different situation. We override frontier for the same reason, for example. This is much more productive than maintaining a fork as long as we don't do breaking changes to Substrate itself (as far as frontier libraries are concerned of course).

@teor2345
Copy link
Copy Markdown
Author

Only libp2p-identity is overridden in Subspace

Then I believe it should be possible to add another override for libp2p too. You'll probably only need to do this for the libp2p umbrella crate. Either way it will be clear from lock file changes if it worked or not.

3 crates need to be patched, and the patch only works because cargo considers https://github.com/subspace/rust-libp2p and https://github.com/autonomys/rust-libp2p to be different repositories.

You usually can't patch one crate version with another version, you have to change the crate fork/source as well.

@teor2345 teor2345 closed this Apr 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants