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

Allow dialing multiaddr containing a peer id #1462

Closed
rklaehn opened this issue Feb 18, 2020 · 4 comments · Fixed by #1931
Closed

Allow dialing multiaddr containing a peer id #1462

rklaehn opened this issue Feb 18, 2020 · 4 comments · Fixed by #1931

Comments

@rklaehn
Copy link
Contributor

rklaehn commented Feb 18, 2020

One minor difference that makes interop between rust-libp2p and go-libp2p difficult is that while in go-libp2p it is possible and default to dial to a multiaddr that contains a peer id, in rust-libp2p you can only dial to a multiaddr that contains just host/port.

Allow rust-libp2p to optionally dial full multiaddrs like e.g. /ip4/192.168.178.20/tcp/4001/ipfs/QmaBUojUuCTMXTRw7yYqNscKZ7caAjFqGFJP3hqv3UH6Br.

Related:
#471

@rklaehn
Copy link
Contributor Author

rklaehn commented Feb 18, 2020

I looked around a bit how this could be implemented. The place where a multiaddr is being parsed and split into individual components is e.g. in the tcp transport, multiaddr_to_socketaddr. It would be easy enough to extend this method to extract an optional peer id.

You could also add this peer id to the TcpConfig struct that is generated in that macro.

But then what? The place where we get the peer id from the remote is higher in the transport stack. Any recommendations how to get the peer id from the multiaddr to the place it can be validated against without having to extend the transport trait in an ugly way?

@tomaka
Copy link
Member

tomaka commented Feb 19, 2020

It's not the role of the TCP transport to care about what PeerId is expected.

The problem to me is that I see this feature as strictly inferior to what we have now, where you explicitly pass a strongly-typed PeerId by value.
Putting the PeerId inside of the Multiaddr opens the gate to all sorts of weird corner cases.

Is there a situation in which multiaddresses with peerids are used in the other libp2p implementations apart from CLI purposes?

@rklaehn
Copy link
Contributor Author

rklaehn commented Feb 19, 2020

Not sure about go-libp2p, but ipfs can not even dial a multiaddr that does not contain a peer id...

It's not the role of the TCP transport to care about what PeerId is expected.

Well, that is basically my problem. I could hack the peer id into the TcpConfig and then grab it from a higher layer where you learn the peer id of the remote end, but it seems weird because it does not really seem to belong there. Why should the TcpTransport even know what a PeerId is...

Now what you would probably want to do if you wanted to support more complex multiaddrs would be that the tcp transport just grabs the front of the multiaddr and leaves the rest (could be a peer id, or a circuit relay address, or whatever) for the higher layers to figure out.

E.g. you got

/ip4/127.0.0.1/tcp/4001/p2p/QmRdjvsyoNjA2ZfAQBtQ7A2m5NmtSXLgxE55Brn1AUjZ1v/p2p-circuit/p2p/QmWiTQoWhJZDxikxHEbjmRiQT4FMgzZS98gfPNJ9GWWuZ8

the TCP transport grabs the part that it understands, so /ip4/127.0.0.1/tcp/4001, and leaves the rest for the higher layers. I think that is how it works in go-ipfs, but I would have to look it up.

@tomaka
Copy link
Member

tomaka commented Feb 19, 2020

Not sure about go-libp2p, but ipfs can not even dial a multiaddr that does not contain a peer id...

rust-libp2p also very strongly encourages you to pass a PeerId, except that we pass it explicitly by value in a strongly-typed fashion, rather than by a string appended at the end of the multiaddr.

That's why I mean by "I think our approach is superior".

We could however of course add functions that split a multiaddr between what's before and after the /p2p/. Substrate for instance does that.

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 a pull request may close this issue.

2 participants