Skip to content
This repository has been archived by the owner on Oct 23, 2022. It is now read-only.

Upgrade libp2p 0.19.0 #169

Merged
merged 32 commits into from
May 19, 2020
Merged

Upgrade libp2p 0.19.0 #169

merged 32 commits into from
May 19, 2020

Conversation

koivunej
Copy link
Collaborator

@koivunej koivunej commented May 8, 2020

Fixes #168. Mostly just the api (rename) changes, however in src/p2p/swarm.rs the multiple connections per peerid were a bit more tricky. I initially implemented by panicing on another connection, then added a test case, then wrangled the bookkeeping to work. The test case required the listening address modifying API which is something we've needed either way.

This was originally upgrading to 0.18.0|1 but 0.19 was released in the meantime so I upgraded directly to that while searching for a solution "why doesn't connection closing to banned peer invoke networkbehaviour methods". It didn't have them but I should file a bug as soon as I can confirm it.

No failures on conformance.

TODO:

  • need to forward all NetworkBehaviour methods on pubsub at least
  • that windows test failure is interesting, need to investigate
  • add better message for the assert where CI windows fails

@koivunej koivunej marked this pull request as draft May 8, 2020 14:36
@koivunej
Copy link
Collaborator Author

I've managed to install a compatible compilation environment for the root crate in Windows by:

  1. install rustup with stable-gnu (stable-msvc linker didn't work, not sure why)
  2. install msys2 by installer per https://github.com/rust-lang/rust#mingw
  3. install git, vim, and gcc with pacman -S
  4. cargo test works

At least clear_on_drop dependency requires the gcc, so installing just the toolchain with rustup is not enough. Following the https://github.com/rust-lang/rust#mingw way of installing gcc through mingw-w64-x86_64-gcc did not work for me.

We should have a guide for "hacking on windows".

@koivunej

This comment has been minimized.

background here is the almost always happening failures on ci-windows, which don't
seem to be locally reproducable.
@koivunej koivunej marked this pull request as ready for review May 15, 2020 12:42
@koivunej

This comment has been minimized.

Cargo.toml Outdated Show resolved Hide resolved
src/p2p/swarm.rs Outdated Show resolved Hide resolved
src/p2p/swarm.rs Outdated Show resolved Hide resolved
Joonas Koivunen added 2 commits May 18, 2020 15:41
perhaps roundtrip_times is more readable.
peer will be removed from connected_peers through the networkbehaviour
callbacks.
src/p2p/swarm.rs Outdated Show resolved Hide resolved
@koivunej koivunej changed the title Upgrade libp2p 0.18.1 Upgrade libp2p 0.19.0 May 19, 2020
Joonas Koivunen added 2 commits May 19, 2020 14:26
the initial implementation didn't actually wait for anything other than
asking the swarm to start listening, but now the await will complete
only after a listening address has actually been bound. this required
some nasty matching on the multiaddr protocol parts which will hopefully
ever be used by test code but ... it works now, at least a bit better.
@koivunej
Copy link
Collaborator Author

Marked my few comments outdated as I just couldn't see the reason for the windows CI failures which went away by modifying the assert message and came back in the 0.19 update. The underlying issue was that the adding of listening addresses is not immediate and I tried to cheat on the first pass of add_listening_address resolving it right after the call to swarm completed. With 2dac944 this should now be fixed until the next release at least :)

src/lib.rs Outdated Show resolved Hide resolved
Joonas Koivunen added 2 commits May 19, 2020 15:11
this needed to be implemented in a `&mut self` method not to get a
double borrow issue with direct `self: Pin<&mut Self>` usage and is
prettier than getting the splittable `&mut self` in the giant match.
@koivunej koivunej requested a review from ljedrz May 19, 2020 12:17
let target = libp2p::build_multiaddr!(Ip4([127, 0, 0, 1]), Tcp(0u16));

let first = node.add_listening_address(target.clone());
let second = node.add_listening_address(target.clone());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

extra clone

@koivunej
Copy link
Collaborator Author

The macos tests are hanging now twice in row; need to wait a timeout. If this is ok, I'll remove the additional clone and merge unless the macos hang can be avoided. It should timeout in ~45min and give access to the output.

match self.connected_peers.get_mut(&peer_id) {
None => {
debug!(" requeueing send event to {}", peer_id.to_base58());
debug!(" requeueing send event to {}", peer_id);
// FIXME: I wonder if this should be
Copy link
Member

Choose a reason for hiding this comment

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

I'd make this FIXME a bit more descriptive

Copy link
Member

@ljedrz ljedrz left a comment

Choose a reason for hiding this comment

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

LGTM

@koivunej
Copy link
Collaborator Author

This PR is quickly spiralling out of control as I couldn't remember once again how much there is to do with the listening addresses :) If there are any more functionality which should be added regarding the "added for testing" feature let's handle those on separate issues and PRs.

@koivunej
Copy link
Collaborator Author

At least the macos issue cleared up, waiting for windows and ignoring the doctest failure as transient.

@koivunej koivunej merged commit 7799d88 into rs-ipfs:master May 19, 2020
@koivunej koivunej deleted the upgrade_libp2p_0_18_1 branch September 24, 2020 12:56
@koivunej koivunej restored the upgrade_libp2p_0_18_1 branch September 24, 2020 12:56
@koivunej koivunej deleted the upgrade_libp2p_0_18_1 branch September 24, 2020 12:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

libp2p upgrade to 0.18.x
2 participants