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

Update libp2p #485

Closed
wants to merge 3 commits into from
Closed

Conversation

matthiasbeyer
Copy link

Update libp2p to 0.40.0 and then to 0.41.0

This is my take on #483 - as I am still learning how everything works (and this seems like a non-trivial change), I'd love to get some guidance on this PR!

Checklist (can be deleted from PR description once items are checked)

  • New code is “linted” i.e. code formatting via rustfmt and language idioms via clippy
  • There are no extraneous changes like formatting, line reordering, etc. Keep the patch sizes small!
  • There are functional and/or unit tests written, and they are passing
  • There is suitable documentation. In our case, this means:
    • Each command has a usage example and API specification
    • Rustdoc tests are passing on all code-level comments
    • Differences between Rust’s IPFS implementation and the Go or JS implementations are explained
  • Additions to CHANGELOG.md files

Signed-off-by: Matthias Beyer <[email protected]>
Signed-off-by: Matthias Beyer <[email protected]>
@koivunej
Copy link
Collaborator

Thanks for looking into this! I'll try to find time to fix the build, and I'll continue approving this. Please log if you are hitting any difficult spots, that'd be good to know for me at least.

@matthiasbeyer
Copy link
Author

So I fail to understand the issue here.

The failures are missing implementations of NetworkBehaviour for Void, but I don't understand how to resolve these, because I don't understand what these NetworkBehaviour things are and what changed in libp2p from 0.39.1 to 0.40.0 that makes the code no longer compile.

I would like to learn, though, so maybe you can explain or guide me on my journey?

@koivunej
Copy link
Collaborator

I haven't yet looked what has been going on. From your mention of Void my knee-jerk guess is that it has been replaced by something like std::convert::Infallible but this is just a guess. Looking into this now. The rust-libp2p does have changelogs but they are quite difficult to follow due to their multiple files nature.

@matthiasbeyer
Copy link
Author

Yes, and the complexity of the API is so high that I completely fail to understand how everything works, especially given that I'm new to the whole topic. As said: I'd like to learn everything, though... so if you're okay with guiding me and effectively delaying this whole process (because I am learning the whole thing vs someone doing this who already knows what to do), I'd love to get this done and become more proficient during the process! 😆

@koivunej
Copy link
Collaborator

I did try to look and the error message does look confusing nor did I see quickly anything from the changelogs. I need to jump on a call and such but I did notice you started the upgrade from the main/root/ipfs library crate. It might be easier (or it has been easier in the past) to first handle bitswap/ in isolation, while preparing for bigger changes. Did not yet test if it would be applicable here.

@koivunej
Copy link
Collaborator

Ok having now tried upgrading just bitswap/ it is definitely more clear set of changes I didn't yet find over at rust-libp2p side. For this "workflow" I:

  1. edit the bitswap/Cargo.toml
  2. only do cargo check --all-targets under the bitswap/ directory.

After fixing up bitswap/ it's more a matter of making sure the ipfs crate uses the same version of libp2p-{core,swarm} -- there are sometimes discrepencies, like this time around libp2p-core has changelog entry for released 0.30.1 but that is not found on crates.io, only 0.30.0 is. Sometimes new features could had also been split from the old ones or there could had been more drastic dependency changes.

Sadly most of the libp2p upgrades are quite a lot of work if one hasn't been following every notification from the repository, which I've long since stopped, since it was quite overwhelming.

However, we did try to add test coverage for the features we got working at one or another point, so once the rustc is satisfied you should be able to discover through the tests if any of our behavioural expectations have changed. This time around the road to that point just seems longer than usual.

Please continue logging your progress and I'll try to find some time during the holidays to also look into this and helping you.

@mxinden
Copy link

mxinden commented Dec 25, 2021

👋 rust-libp2p maintainer here. Feel free to tag me with concrete questions related to rust-libp2p and the recent breaking changes.

@matthiasbeyer
Copy link
Author

So I tried to update the /bitswap subtree starting fresh from master here.

The current error I get is:

the trait `ProtocolsHandler` is not implemented for `BitswapEvent

Which I do not understand... the BitswapEvent is the NetworkBehaviour::OutEvent type. Why does this need to implement ProtocolsHandler at all? Isn't it just an event object?
Also, a bit counterintuitive: The type bound is only made effective via the poll() function, not via the traits associated type itself...

Maybe someone can educate me!? 😄

@mxinden
Copy link

mxinden commented Dec 29, 2021

The type parameters of NetworkBehaviourAction changed. The first parameter is now the NetworkBehaviour::OutEvent, the second parameter is now the NetworkBehaviour::ProtocolsHandler.

    events: VecDeque<NetworkBehaviourAction<Message, BitswapEvent>>,

should be

    events: VecDeque<NetworkBehaviourAction<BitswapEvent, OneShotHandler<BitswapConfig, Message, MessageWrapper>>>,

https://github.com/matthiasbeyer/rust-ipfs/blob/ed0328bd031cde30f96d17c2ae8a4b3041d7e211/bitswap/src/behaviour.rs#L91

        -> Poll<NetworkBehaviourAction<<<Self::ProtocolsHandler as IntoProtocolsHandler>::Handler as ProtocolsHandler>::InEvent, Self::OutEvent>>

should be

        -> Poll<NetworkBehaviourAction<Self::OutEvent, Self::ProtocolsHandler>>

https://github.com/matthiasbeyer/rust-ipfs/blob/ed0328bd031cde30f96d17c2ae8a4b3041d7e211/bitswap/src/behaviour.rs#L293

Does that help @matthiasbeyer?

@matthiasbeyer
Copy link
Author

matthiasbeyer commented Dec 29, 2021

It did!
Next step is the changed NetworkBehaviourAction::Dial interface, that changed as well. I assume that the handler that needs to be added to that object is some kind of callback mechanism. I don't know how to construct one properly though.

Can you please tell me in a few short sentences what I am implementing here, maybe I understand what to do myself (no hurry though)!

My current code:


We can also switch to email or another communication channel if you want, so we do not spam this issue here! 😆

@mxinden
Copy link

mxinden commented Dec 29, 2021

I don't know how to construct one properly though.

For now you can just call Bitswap::new_handler.

I assume that the handler that needs to be added to that object is some kind of callback mechanism.

A handler implements the ProtocolsHandler trait. It handles all substreams of a specific protocol (e.g. bitswap) on a single connection.

We can also switch to email or another communication channel if you want, so we do not spam this issue here! laughing

I prefer this issue as (a) others can follow along and help and (b) it might be helpful for others.

@matthiasbeyer
Copy link
Author

Ah, saw your comment too late and figured out a way to construct a new NetworkBehaviourAction::Dial, and pushed it to the branch linked above. Would be nice to get some comment on whether that is the right way! 😃

The remaining bits is just some issue in the error type I will take care of myself.

@rand0m-cloud
Copy link
Contributor

rand0m-cloud commented Mar 6, 2022

Hello, I've recently started work on updating the crate to use libp2p v0.43.0 due to a build error when I try cargo build -p ipfs-http

error[E0282]: type annotations needed
   --> /home/xxx/.cargo/registry/src/github.com-1ecc6299db9ec823/libp2p-noise-0.32.0/src/protocol/x25519.rs:221:45
    |
221 |         curve25519_sk.copy_from_slice(&hash.as_ref()[..32]);
    |                                        -----^^^^^^--
    |                                        |    |
    |                                        |    cannot infer type for type parameter `T` declared on the trait `AsRef`
    |                                        this method call resolves to `&T`
    |
    = note: type must be known at this point

For more information about this error, try `rustc --explain E0282`.

I got through all the type renames and then hit this error like I assume you have:

error[E0277]: the trait bound `Void: ConnectionHandler` is not satisfied
   --> src/p2p/pubsub.rs:305:15
    |
305 |     ) -> Poll<PubsubNetworkBehaviourAction> {
    |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `ConnectionHandler` is not implemented for `Void`
    |
    = note: required because of the requirements on the impl of `IntoConnectionHandler` for `Void`

I scrubbed through the documentation and found that IntoConnectionHandler and ConnectionHandler added a trait bound of Send which void::Void curiously doesn't implement. I don't know how to proceed now because I guess we need to send an upstream patch to void to add Sync + Send to void::Void. At the same time, I would like to know why void::Void was in use over the () type.

Sorry if this is unnecessary, @mxinden any thoughts?

EDIT: I have submitted a patch to void
EDIT: why did I think void::Void didn't have a Send + Sync auto implementation.

@mxinden
Copy link

mxinden commented Mar 9, 2022

At the same time, I would like to know why void::Void was in use over the () type.

() is the unit type, thus can take on a single value, namely (). Void is the uninhabited type, i.e. it does not have any values and is thus impossible to instantiate. With Void we can make states impossible at compile time.

@koivunej
Copy link
Collaborator

koivunej commented Apr 1, 2022

Thanks for your efforts @matthiasbeyer. Closing this now that #499 has been merged.

@koivunej koivunej closed this Apr 1, 2022
@matthiasbeyer matthiasbeyer deleted the update-libp2p branch April 1, 2022 14:14
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.

4 participants