Upgrade libp2p to version 0.53.1 #1977

jsdanielh · 2023-11-17T23:19:15Z

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 · 2023-11-17T23:34:40Z

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

jsdanielh · 2023-11-17T23:43:42Z

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

Yes, thanks for adding the reference.

hrxi

Amazing work! :)

I left a couple of comments and questions.

network-libp2p/src/behaviour.rs

network-libp2p/src/dispatch/codecs/typed.rs

network-libp2p/Cargo.toml

thomaseizinger

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

I've left a couple of comments :)

network-libp2p/src/behaviour.rs

network-libp2p/src/connection_pool/handler.rs

thomaseizinger · 2023-11-19T04:55:47Z

network-libp2p/src/connection_pool/mod.rs

+    /// Peer is banned
+    #[error("Peer is banned")]
+    BannedPeer,


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

thomaseizinger · 2023-11-19T05:00:33Z

network-libp2p/src/discovery/protocol.rs

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


You might want to use ReadyUpgrade here instead.

thomaseizinger · 2023-11-19T05:02:02Z

network-libp2p/src/dispatch/codecs/typed.rs

    type Request = IncomingRequest;
    type Response = OutgoingResponse;


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 :)

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

thomaseizinger · 2023-11-19T05:08:35Z

network-libp2p/src/network.rs

+        #[cfg(not(target_family = "wasm"))]
+        let swarm = SwarmBuilder::with_existing_identity(keypair)
+            .with_tokio()
+            .with_other_transport(|_| transport)


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.

thomaseizinger · 2023-11-19T05:09:01Z

network-libp2p/src/network.rs

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


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

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.

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.

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!

thomaseizinger · 2023-11-19T05:10:10Z

network-libp2p/tests/discovery.rs


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


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

hrxi · 2023-11-19T13:14:10Z

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

jsdanielh · 2023-11-19T22:06:38Z

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.

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 <hrrrxi@gmail.com>

jsdanielh force-pushed the jsdanielh/libp2p_upstream branch from 68e4756 to d13d4aa Compare November 17, 2023 23:30

jsdanielh requested a review from hrxi November 17, 2023 23:31

jsdanielh force-pushed the jsdanielh/libp2p_upstream branch from d13d4aa to 056ffe5 Compare November 17, 2023 23:32

hrxi mentioned this pull request Nov 17, 2023

Bump multiaddr from 0.16.0 to 0.18.1 #1946

Closed

jsdanielh force-pushed the jsdanielh/libp2p_upstream branch from 056ffe5 to 6cb3d1e Compare November 17, 2023 23:39

jsdanielh force-pushed the jsdanielh/libp2p_upstream branch from 6cb3d1e to b2cd13c Compare November 18, 2023 00:10

hrxi reviewed Nov 18, 2023

View reviewed changes

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 force-pushed the jsdanielh/libp2p_upstream branch 2 times, most recently from 66ae543 to 9dba7b0 Compare November 18, 2023 23:19

thomaseizinger reviewed Nov 19, 2023

View reviewed changes

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

jsdanielh force-pushed the jsdanielh/libp2p_upstream branch 2 times, most recently from 66d2c9b to 0868856 Compare November 20, 2023 23:08

jsdanielh force-pushed the jsdanielh/libp2p_upstream branch from 0868856 to c447dc2 Compare November 20, 2023 23:11

hrxi merged commit c447dc2 into albatross Nov 21, 2023
6 checks passed

hrxi deleted the jsdanielh/libp2p_upstream branch November 21, 2023 16:23

hrxi mentioned this pull request Nov 21, 2023

Bump tokio-metrics from 0.3.0 to 0.3.1 #1983

Closed

jsdanielh added this to the Nimiq PoS Mainnet milestone Nov 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade libp2p to version 0.53.1 #1977

Upgrade libp2p to version 0.53.1 #1977

jsdanielh commented Nov 17, 2023 •

edited by sisou

Loading

hrxi commented Nov 17, 2023

jsdanielh commented Nov 17, 2023

hrxi left a comment

thomaseizinger left a comment

thomaseizinger Nov 19, 2023

thomaseizinger Nov 19, 2023

thomaseizinger Nov 19, 2023

thomaseizinger Nov 19, 2023

thomaseizinger Nov 19, 2023

thomaseizinger Nov 19, 2023

thomaseizinger Nov 19, 2023

hrxi Nov 19, 2023

thomaseizinger Nov 19, 2023

thomaseizinger Nov 19, 2023

hrxi commented Nov 19, 2023

jsdanielh commented Nov 19, 2023

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

		type Request = IncomingRequest;
		type Response = OutgoingResponse;


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

Upgrade libp2p to version 0.53.1 #1977

Upgrade libp2p to version 0.53.1 #1977

Conversation

jsdanielh commented Nov 17, 2023 • edited by sisou Loading

Pull request checklist

hrxi commented Nov 17, 2023

jsdanielh commented Nov 17, 2023

hrxi left a comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrxi commented Nov 19, 2023

jsdanielh commented Nov 19, 2023

jsdanielh commented Nov 17, 2023 •

edited by sisou

Loading