Rework libp2p-identify #116

tomaka · 2018-01-30T11:39:25Z

Based over #115

Slightly reworks the existing code of libp2p-identify. Instead of producing an Option<IdentifyInfo>, we now produce an IdentifyOutput that contains either an IdentifyInfo or a IdentifySender that can be used to send back the information to the remote. The IdentifyInfo is no longer part of the configuration of the connection upgrade.
Adds an IdentifyTransport type that wraps around a Transport and a Peerstore. Whenever we receive an incoming connection, we dial back to the node in order to identify it and store its identity in the peerstore. Whenever we dial a node with an address of type /ipfs/..., we look inside the Peerstore for a known multiaddress. Due to this behaviour, it is strongly recommended to run the IdentifyTransport on top of a ConnectionReuse. The IdentifyTransport essentially exists to expose addresses of the form /ipfs/... instead of raw multiaddresses.
Changes the Transport trait so that dialing now also produces the multiaddress that was dialed in addition to the socket. This is because the IdentifyTransport can change the multiaddress of the dialed node to be /ipfs/....
Changes the Transport trait so that the multiaddress of an incoming client is known only after the connection upgrade. This is required because of the IdentifyTransport which changes the multiaddr of the client after identifying it.

eira-fransham

I think everything here is a nit and so I'd probably be OK with approving this if you would rather not make these changes (I'd expect you to make the changes on #117 and not here). That being said, a couple of the changes would probably make this code somewhat nicer.

eira-fransham · 2018-03-05T16:54:09Z

libp2p-identify/src/transport.rs

+					.peer(&peer_id)
+					.into_iter()
+					.flat_map(|peer| peer.addrs())
+					.collect::<Vec<_>>()


Since we take ownership of self.peerstore in this function, why can't we just do:

self.peerstore .deref() .peer(&peer_id) .into_iter() .flat_map(|peer| peer.addrs()) .into_iter() .flat_map::<_, fn(_) -> _>(|peer| peer.addrs())

and save a traversal and allocation?

The problem is that deref() borrows, therefore we would have a reference to self.peerstore stored in the closure.

Oh, right. That's obvious now. We don't have deref_move yet, ugh.

eira-fransham · 2018-03-05T17:00:32Z

libp2p-websocket/src/browser.rs

@@ -191,9 +186,9 @@ impl Transport for BrowserWsConfig {
 			});
 		};

-		Ok(open_rx.then(|result| {
+		Ok(Box::new(open_rx.then(|result| {


Why did we add a Box here if it wasn't necessary before?

This PR modifies the item produced by dial, and writing the type explicitly would become too much of a clusterfuck, therefore I changed the associated type Dial to a Box.

Personally I'd write the full type when possible (i.e. when it doesn't use closures) even if it's a clusterfuck but that's me.

After looking more at it, I think the problem is that the closure now takes ownership of original_addr, which wasn't needed before. Therefore we can either use a closure, or write a custom modifier that makes the code much more difficult to read.

eira-fransham · 2018-03-05T17:15:45Z

libp2p-identify/src/protocol.rs

+}
+
+// Turn a `Vec<u8>` into a `Multiaddr`. If something bad happens, turn it into an `IoError`.
+fn bytes_to_multiaddr(bytes: Vec<u8>) -> Result<Multiaddr, IoError> {


This should be defined inside parse_proto_msg because it's very specific to that function. Better still would be to impl Into<io::Error> for multiaddr::errors::Error and then just do Multiaddr::from_bytes(bytes)? and let the ? machinery add the into call.

eira-fransham · 2018-03-05T17:18:59Z

libp2p-identify/src/transport.rs

+use std::ops::Deref;
+use std::time::Duration;
+
+/// Implementation of `Transport`. See the crate root description.


Maybe link the crate root here. Ideally I'd move the relevant text to this docstring but mirroring the readme and crate root doc is too valuable for maintainance so just linking is enough.

eira-fransham · 2018-03-05T17:25:45Z

libp2p-identify/src/transport.rs

+
+impl<T, Pr, P> Transport for IdentifyTransport<T, Pr>
+where
+	T: Transport + Clone + 'static,          // TODO: 'static :(


For nontrivial uses of generics can we give proper names to the type parameters like we would arguments? You wouldn't get away with naming nontrivial arguments a and b, why should type parameters get a free pass? My personal choice would be impl<Trans, PStoreRef, PStore>. Also, ideally PStore would be before PStoreRef and the where bounds reordered accordingly since that makes it more readable in my opinion.

That's true, it's a bad habit I have now.

eira-fransham · 2018-03-05T17:27:39Z

libp2p-identify/src/transport.rs

+							original_addr,
+							addr_ttl,
+						)?,
+						_ => unreachable!(


I'd like if we allowed protocols to define that they only work on servers or clients or both so that we could have a seperate IdentifyServer and IdentifyClient type and didn't need hacks like this, but that's a major change and not necessary for this PR.

eira-fransham · 2018-03-05T17:31:42Z

libp2p-identify/src/transport.rs

+					Ok(transport
+						.dial(old_addr)
+						.unwrap_or_else(|_| {
+							panic!("the same multiaddr was determined to be valid earlier")


Maybe this panic/message is a sign that we should have a way to dial using an already-parsed Multiaddr, since this is always infallible if the multiaddr is already verified. It'd be nice to express that in the API instead of relying on the user to know that.

On the other hand, I don't see any other situation where we would dial the same multiaddress twice in a row, and adding such an API would add a lot of complexity.

What about the use-case of compile-time checked constant multiaddrs that are parsed by a macro? I think enforcing strings to be the canonical representation of multiaddrs at API boundaries is a poor choice and we should probably have a trait akin to ToSocketAddrs (it would be possible to have the dial call be infallible for types that can infallibly be converted to a Multiaddr using type hackery but I'd prefer if we just added an associated Error type to this trait and made type Error = ! when that lands). I don't think that's necessary for this PR though, I'll write up another PR with that in it.

eira-fransham · 2018-03-05T17:34:16Z

libp2p-swarm/src/transport.rs

@@ -384,6 +383,37 @@ where
 	}
 }

+/// > **Note**: This type is needed because of the lack of `-> impl Trait` in Rust. It can be


Is that actually true? It's an enum and I don't think impl Trait allows for anonymous sum types.

If Rust had impl Trait we could use the Either enum from the futures crate and add some modifiers to it. This custom enum is a combination of Either and these modifiers.

Maybe say that here then because as it stands this note isn't much more than a TODO and doesn't have any place in the docs.

* Implement ConnectionReuse correctly * Add some tests and fixes * Remove useless boolean in active_connections * Correctly run tests * Optimize the processing * Next incoming is now in two steps * Remove log * Fix dialing a node even if we already have a connection * Add a proper PeerId to Peerstore * Turn identify into a transport layer * Expose the dialed multiaddress * Add identified nodes to the peerstore * Allow configuring the TTL of the addresses * Split identify in two modules * Some comments and tweaks * Run rustfmt * Add test and bugfix * Fix wrong address reported when dialing * Fix websocket browser code * Ignore errors in the swarm * Fix multiplex test * Fix some style concerns * Fix concerns

Added peer discovery tests

tomaka added 9 commits January 29, 2018 15:13

Add a proper PeerId to Peerstore

a7e7b93

Turn identify into a transport layer

459a666

Expose the dialed multiaddress

9c469e9

Add identified nodes to the peerstore

7f88c0a

Allow configuring the TTL of the addresses

7061c75

Split identify in two modules

ad1e082

Some comments and tweaks

9825635

Run rustfmt

34c3d99

Add test and bugfix

c40cf3b

tomaka force-pushed the identify-rework branch from 81bc5f7 to c40cf3b Compare January 30, 2018 12:18

tomaka mentioned this pull request Jan 30, 2018

Merge of #116 and #111 #117

Merged

tomaka added 4 commits February 1, 2018 18:01

Fix wrong address reported when dialing

fdabefc

Merge remote-tracking branch 'upstream/master' into identify-rework

08aadaf

Fix websocket browser code

77b74bd

Merge remote-tracking branch 'upstream/master' into identify-rework

96dd4f5

tomaka mentioned this pull request Feb 12, 2018

Identify log #129

Merged

tomaka added 2 commits February 14, 2018 14:38

Merge remote-tracking branch 'upstream/master' into identify-rework

6189445

Support the p2p protocol in libp2p-identify

853bf77

tomaka mentioned this pull request Feb 22, 2018

IPFS nodes seem to ignore our nodes #133

Closed

eira-fransham reviewed Mar 5, 2018

View reviewed changes

tomaka added 4 commits March 6, 2018 10:27

Fix concerns

51bf9ba

Merge remote-tracking branch 'upstream/master' into identify-rework

c115e02

Fix libp2p-dns

52913d2

More concerns

028536e

eira-fransham approved these changes Mar 7, 2018

View reviewed changes

tomaka merged commit 39dde30 into libp2p:master Mar 7, 2018

tomaka deleted the identify-rework branch March 7, 2018 09:49

mxinden mentioned this pull request May 16, 2023

identify: consider dropping the protocol version libp2p/go-libp2p#2294

Closed

dkuehr pushed a commit to openmina/rust-libp2p that referenced this pull request Oct 24, 2023

Merge pull request libp2p#116 from openmina/feat/peer-discovery-tests-

18a2fa8

Added peer discovery tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework libp2p-identify #116

Rework libp2p-identify #116

tomaka commented Jan 30, 2018

eira-fransham left a comment

eira-fransham Mar 5, 2018

tomaka Mar 6, 2018

eira-fransham Mar 6, 2018

eira-fransham Mar 5, 2018

tomaka Mar 6, 2018 •

edited

Loading

eira-fransham Mar 6, 2018

tomaka Mar 6, 2018

eira-fransham Mar 5, 2018

eira-fransham Mar 5, 2018

eira-fransham Mar 5, 2018

tomaka Mar 6, 2018

eira-fransham Mar 5, 2018

eira-fransham Mar 5, 2018

tomaka Mar 6, 2018

eira-fransham Mar 7, 2018

eira-fransham Mar 5, 2018

tomaka Mar 6, 2018

eira-fransham Mar 6, 2018

Rework libp2p-identify #116

Rework libp2p-identify #116

Conversation

tomaka commented Jan 30, 2018

eira-fransham 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

tomaka Mar 6, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaka Mar 6, 2018 •

edited

Loading