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

Possible use of libp2p_identity::PeerId as PeerId type #73

Closed
Tracked by #140
dmitry-markin opened this issue Apr 11, 2024 · 3 comments
Closed
Tracked by #140

Possible use of libp2p_identity::PeerId as PeerId type #73

dmitry-markin opened this issue Apr 11, 2024 · 3 comments

Comments

@dmitry-markin
Copy link
Collaborator

dmitry-markin commented Apr 11, 2024

While working on paritytech/polkadot-sdk#1631 I tried upgrading litep2p to use multiaddr-0.18.1 also used by libp2p-0.52.3. The thing with multiaddr-0.18.1 is that it uses libp2p_identity::PeerId as a payload of Protocol::P2p instead of Multihash in the older versions.

So, we have three options:

  1. Make litep2p indirectly depend on libp2p-identity via multiaddr, but keep distinct PeerId types. We would need to add conversion between libp2p & litep2p PeerId types to litep2p.
  2. Completely replace all PeerId types used in litep2p & polkadot-sdk by libp2p_identity::PeerId.
  3. Introduce own Multiaddr implementation into litep2p with a layer of abstraction in polkadot-sdk like it's currently done with PeerId types.

I don't think route 3 is a good idea due to code duplication and layers of abstraction more heavy than the ones for PeerId, and a lot of code touched overall.

To proceed with paritytech/polkadot-sdk#1631 I'm going to go route 1 for now, but we should at least consider route 2 as something that would simplify things.

CC @altonen @lexnv

@altonen
Copy link
Contributor

altonen commented Apr 11, 2024

I'm no longer a maintainer of litep2p/sc-network and don't have a strong opinion on this but if I was, I wouldn't go for 1) or 2). I don't think the issue is litep2p but in Polkadot SDK because, IIUC, it's using two libraries that provide incompatible versions for some type (Multiaddr). 3) would temporarily add some extra code to Polkadot SDK (litep2p could just re-export its multiaddr) because sc-network-types would have to implement some wrapper code for libp2p and litep2p but that wrapper code could be kept very thin and only implement the parts that Polkadot SDK uses. Eventually one of the libraries will get removed from Polkadot SDK and all the wrapper code will be removed along with it. Having temporary wrapper code to work under special circumstances (two libp2p implementations) is IMO a better way to deal with the issue than introducing a new dependency that is not in the control of Parity. The reason litep2p was written was because libp2p kept breaking stuff in every release and in typical fashion stuff broke again, this time in one of its dependencies.

All that said, I think you should do whatever works best for Parity, that's why litep2p exists in the first place :)

@dmitry-markin
Copy link
Collaborator Author

@altonen Thanks for sharing your opinion! I'll give it another thought.

@dmitry-markin
Copy link
Collaborator Author

Closing as not relevant anymore after implementing paritytech/polkadot-sdk#4198.

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

2 participants