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

/p2p/... suffix found when calling libp2p_kad::Kademlia::addresses_of_peer #2280

Closed
tomaka opened this issue Oct 7, 2021 · 3 comments
Closed

Comments

@tomaka
Copy link
Member

tomaka commented Oct 7, 2021

I unfortunately can't find the corresponding PR, but since libp2p-core 0.28.0 we now pass a /p2p/... suffix when calling Transport::dial.

Unfortunately, this leads libp2p-kad to insert addresses with /p2p/ in its routing table, here:

ConnectedPoint::Dialer { address } => Some(address),

In the code base of Substrate, this leads to these addresses with /p2p/ being propagated around and poisoning a bit everything.

I would suggest to strongly clarify everywhere a multiaddress is used whether it has or has not this /p2p/ (for example ConnectedPoint::Dialing have it but not ConnectedPoint::Listener), or better: to pass the PeerId as a separate parameter to dial, in order to have a guarantee that this /p2p/ isn't found anywhere.

@tomaka
Copy link
Member Author

tomaka commented Oct 7, 2021

I would add that in my opinion it doesn't make sense for ConnectedPoint::Dialing to contain a multiaddr with a /p2p/ suffix, as this information is already always given by the context. You never have a ConnectedPoint alone, you always have one associated with a PeerId. This redundant /p2p/... just brings confusion.

@mxinden
Copy link
Member

mxinden commented Oct 9, 2021

I unfortunately can't find the corresponding PR

I think you are referring to #1931.

@thomaseizinger
Copy link
Contributor

Closing as stale. If it is still an issue for users, they should open another issue.

I am of the opinion that because a Multiaddr allows for /p2p, we can statically guarantee anyway that it is not present so downstream code just needs to be prepared to handle its presence.

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

No branches or pull requests

3 participants