-
Notifications
You must be signed in to change notification settings - Fork 257
upgrade to libp2p 0.56.0 #3737
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
upgrade to libp2p 0.56.0 #3737
Conversation
idle connection timout is no longer 0 which was instant earlier but instead changed to 10 seconds libp2p/rust-libp2p@f4edafb updated tests to ensure atleast 10 seconds has passed before we check for peer connections
🛡️ Immunefi PR ReviewsWe noticed that your project isn't set up for automatic code reviews. If you'd like this PR reviewed by the Immunefi team, you can request it manually using the link below: Once submitted, we'll take care of assigning a reviewer and follow up here. |
nazar-pc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me overall
| async fn test_connection_breaks_after_timeout_without_reservation() { | ||
| let connection_timeout = Duration::from_millis(300); | ||
| let long_delay = Duration::from_millis(1000); | ||
| let long_delay = Duration::from_millis(12000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why though? Seems excessive for tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default earlier was 0 secs and delay did not matter. Recent changed made it a default 10 seconds.
This is configurable if we build the Swarm manually but the tests uses swarm builder where default is set through macro without configurability. So just make it more than 10 seconds rather than reconstructing the swarm builder for tests
|
@nazar-pc going to merge this once CI gives green assuming you have no objections |
This PR upgrades our libp2p fork from 0.54.2 to 0.56.0.
Libp2p fork changes(https://github.com/autonomys/rust-libp2p/commits/subspace-v10/)
Inbound stream timeouts
We recently patched our fork to have inbound streams to have timeout like outbound.
Our fork has following patches for this
Upstream PR libp2p/rust-libp2p#6009 is still unmerged as we need to confirm if the fix worked. Will handle this to get the PR merged and we dont need to pull our changes in next upgrade hopefully.
BandwidthSinks
Unfortunately, polkadot-sdk is not yet migrated to 0.56.0 and still at 0.54.1.
Reason being substrate uses
with_bandwidth_loggingto measure the overall bandwidth usage distinct between inbound and outbound. There was deprecation notice and this was mentioned by Nazar in the libp2p upgrade pr paritytech/polkadot-sdk#6248With 0.56.0, the entire bandwith module is removed. Migrating the same on substrate side would like a bit more changes and felt out of scope for our fork. So I instead reverted the changes into our fork.
Once substrate upgrades to 0.56.0, we would not need this change. Either we upgrade substrate with latest libp2p ourselves or wait for community.
The changes on our end is quiet straight forward. Main change is coming from this libp2p/rust-libp2p#5938
As usual, this PR will be draft until I confirm the changes are compatible with chronos and mainnet.
Code contributor checklist: