Skip to content
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 libp2p to version 0.53.1 #1977

Merged
merged 1 commit into from
Nov 21, 2023
Merged

Upgrade libp2p to version 0.53.1 #1977

merged 1 commit into from
Nov 21, 2023

Conversation

jsdanielh
Copy link
Member

@jsdanielh jsdanielh commented Nov 17, 2023

Upgrade libp2p to version 0.53.1 using a forked version for now due to the lack of support of workers in the websocket-websys transport.
This also modifies most of the network code to use the adapted libp2p naming standard.

Pull request checklist

  • All tests pass. The project builds and runs.
  • I have resolved any merge conflicts.
  • I have resolved all clippy and rustfmt warnings.

@hrxi
Copy link
Member

hrxi commented Nov 17, 2023

Depending on libp2p/rust-libp2p#4889 to get rid of our fork, I guess.

@jsdanielh
Copy link
Member Author

Depending on libp2p/rust-libp2p#4889 to get rid of our fork, I guess.

Yes, thanks for adding the reference.

Copy link
Member

@hrxi hrxi left a comment

Choose a reason for hiding this comment

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

Amazing work! :)

I left a couple of comments and questions.

network-libp2p/src/behaviour.rs Show resolved Hide resolved
network-libp2p/src/dispatch/codecs/typed.rs Show resolved Hide resolved
network-libp2p/src/dispatch/codecs/typed.rs Outdated Show resolved Hide resolved
network-libp2p/Cargo.toml Show resolved Hide resolved
@jsdanielh jsdanielh force-pushed the jsdanielh/libp2p_upstream branch 2 times, most recently from 66ae543 to 9dba7b0 Compare November 18, 2023 23:19
Copy link

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

rust-libp2p maintainer here. Upgrade looks clean, well done.

I've left a couple of comments :)

network-libp2p/src/behaviour.rs Show resolved Hide resolved
network-libp2p/src/behaviour.rs Outdated Show resolved Hide resolved
network-libp2p/src/connection_pool/handler.rs Outdated Show resolved Hide resolved
Comment on lines +12 to +14
/// Peer is banned
#[error("Peer is banned")]
BannedPeer,

Choose a reason for hiding this comment

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

FYI, there is also a module that supports peer banning: https://docs.rs/libp2p/latest/libp2p/allow_block_list/index.html

@@ -86,7 +86,7 @@ pub enum DiscoveryMessage {
pub struct DiscoveryProtocol;

Choose a reason for hiding this comment

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

You might want to use ReadyUpgrade here instead.

Comment on lines 118 to 119
type Request = IncomingRequest;
type Response = OutgoingResponse;

Choose a reason for hiding this comment

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

Whilst certainly possible, this trait was designed with the idea in mind that you'd use typed messages here and not just (length-prefixed) bytes :)

Choose a reason for hiding this comment

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

Sending bytes back and forth over a "generic" request-response protocol kind of negates the idea of using protocols for multiplexing because you need another, nested layer that allows you to differentiate, which message got sent to you.

YMMV but it might be easier to have multiple instances of libp2p-request-response, all with a different codec and more specific protocols that only sent specific messages back and forth (or perhaps only a single message-type per protocol).

network-libp2p/src/network.rs Outdated Show resolved Hide resolved
#[cfg(not(target_family = "wasm"))]
let swarm = SwarmBuilder::with_existing_identity(keypair)
.with_tokio()
.with_other_transport(|_| transport)

Choose a reason for hiding this comment

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

A big benefit of the new SwarmBuilder is that it enforces at compile-time, that you compose transports together correctly. For example, DNS needs to wrap all other transports (except relay).

By using with_other_transport, you are not leveraging any of this. I would suggest to refactor this into two functions, one of them for wasm and the other one non-wasm and use the entire SwarmBuilder in there, without any further conditionally-compiled code.

"Sent Ping and received response to/from peer",
);
Ok(duration) => {
log::trace!(?event.peer, ?duration, "Successful ping from peer");

Choose a reason for hiding this comment

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

FYI: libp2p-ping already has such a log on debug level.

Choose a reason for hiding this comment

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

Also, version v0.53 is now using tracing instead of log so if you are wondering, why you aren't seeing any messages, it is because you need to set up a tracing-subscriber.

Copy link
Member

Choose a reason for hiding this comment

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

We're also using tracing, actually, we just renamed it to log so as to not touch every file.

FYI: libp2p-ping already has such a log on debug level.

Since we cap other crates at the info! level usually and sometimes increase the logging level for our crates only, this can still make sense.

Choose a reason for hiding this comment

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

We're also using tracing, actually, we just renamed it to log so as to not touch every file.

FYI: libp2p-ping already has such a log on debug level.

Since we cap other crates at the info! level usually and sometimes increase the logging level for our crates only, this can still make sense.

Of course! I just mentioned it so you know you could achieve the same thing with configuration instead of writing the code yourself :)

Each connection also has a span that adds context such as the remote's PeerId and address: https://github.com/libp2p/rust-libp2p/blob/0ceb6580941c010edff2a5689ada2e9e33c773cb/swarm/src/connection/pool.rs#L534

Hence, all logs from within the ConnectionHandler are automatically scoped to this :)

We've only recently transitioned to this and I'd be very curious to learn how helpful it is to users!


let mut swarm = Swarm::with_threadpool_executor(transport, behaviour, peer_id);
let mut swarm = SwarmBuilder::with_existing_identity(keypair)

Choose a reason for hiding this comment

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

There is also libp2p-swarm-test which allows for quick, in-memory testing of Swarms.

@hrxi
Copy link
Member

hrxi commented Nov 19, 2023

Thanks for taking a look at this PR, @thomaseizinger.

@jsdanielh jsdanielh force-pushed the jsdanielh/libp2p_upstream branch 2 times, most recently from dca4e37 to 00ac607 Compare November 19, 2023 22:06
@jsdanielh
Copy link
Member Author

rust-libp2p maintainer here. Upgrade looks clean, well done.

I've left a couple of comments :)

Hey, thank you very much for taking the time to review it and kudos for the recent improvements in rust-libp2p.
I already started implementing some of your suggestions.

@jsdanielh jsdanielh force-pushed the jsdanielh/libp2p_upstream branch 2 times, most recently from 66d2c9b to 0868856 Compare November 20, 2023 23:08
Upgrade libp2p to version 0.53.1 using a forked version for now due
to the lack of support of workers in the `websocket-websys`
transport.
This also modifies most of the network code to use the adopted libp2p
naming standard.

Co-authored-by: hrxi <[email protected]>
@hrxi hrxi merged commit c447dc2 into albatross Nov 21, 2023
6 checks passed
@hrxi hrxi deleted the jsdanielh/libp2p_upstream branch November 21, 2023 16:23
@jsdanielh jsdanielh added this to the Nimiq PoS Mainnet milestone Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants