search

paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.network/
1.66k stars 594 forks source link

Get rid of `libp2p` dependency in `sc-network-sync` #4858

Open dmitry-markin opened 1 month ago

dmitry-markin commented 1 month ago

libp2p is currently pulled in as a dependency of sc-network-sync due to the use of libp2p::request_response::OutboundFailure in substrate/client/network/sync/src/engine.rs. This should be refactored/replaced by a network backend agnostic type.

cenwadike commented 3 weeks ago

First time attempting to contribute to polkadot SDK, I'd like to give this a shot

cenwadike commented 3 weeks ago

Hello @dmitry-markin, I'd like to know what defines an agnostic type.

ndkazu commented 2 weeks ago

Hello @dmitry-markin, I'd like to know what defines an agnostic type.

This is coming from the sc_network documentation:

Substrate’s networking protocol is based upon libp2p. It is at the moment not possible and not planned to permit using something else than the libp2p network stack and the rust-libp2p library. However the libp2p framework is very flexible and the rust-libp2p library could be extended to support a wider range of protocols than what is offered by libp2p.

The dependency to 1 type of network library makes it non-network backend agnostic, so by removing the libp2p dependency from sc_network_sync (which is the subject here), you make it able to use more than one type of network library (or stack...the manual says stack) --> network backend agnostic. Feel free to correct me if I didn't get it right

ndkazu commented 2 weeks ago

@cenwadike , I would suggest to ~replace~ use libp2p::request_response::OutboundFailure; ~~ by~~ use sc_network::OutboundFailure; or use sc_network::request_responses::OutboundFailure ~in~ [substrate/client/network/sync/src/engine.rs](https://github.com/paritytech/polkadot-sdk/blob/ b301218db8785c6d425ca9a9ef90daa80780f2ce/substrate/client/network/sync/src/engine.rs#L1276) ~This way, when it will be ok to use other protocols than libp2p in substrate, sc_network_sync will be ready...thinking out loud, so feel free to correct me~ Ok after the details given in the PR by @bkchr , I think the best way to do this is to replace OutBoundFailure by a generic term→Need to work in substrate/client/network/src/request_responses.rs as well. A bit more involved than what I first said.

ndkazu commented 2 weeks ago

Ok, created a PR . It would be nice to link the issue to the PR...

ndkazu commented 1 week ago

@dmitry-markin , I have PR ready to be reviewed here: https://github.com/paritytech/polkadot-sdk/pull/4974 The main libp2p dependency in engine.rs being the request_responses::Network field, I decided to add a request_responses::Network2 field. Then, by using a CustomOutboundFailure enum which does not depend on libp2p, I make sure that engine is completely free from any libp2p dependency. Now, looking at issue #4859 I am wondering if there is more...