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

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

Closed
dmitry-markin opened this issue Jun 21, 2024 · 6 comments · Fixed by #4974
Closed

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

dmitry-markin opened this issue Jun 21, 2024 · 6 comments · Fixed by #4974
Labels
C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. I4-refactor Code needs refactoring. T0-node This PR/Issue is related to the topic “node”.

Comments

@dmitry-markin
Copy link
Contributor

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.

@dmitry-markin dmitry-markin added I4-refactor Code needs refactoring. T0-node This PR/Issue is related to the topic “node”. labels Jun 21, 2024
@bkchr bkchr added the C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. label Jun 23, 2024
@cenwadike
Copy link

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

@cenwadike
Copy link

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

@ndkazu
Copy link
Contributor

ndkazu commented Jul 8, 2024

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
Copy link
Contributor

ndkazu commented Jul 8, 2024

@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/
b301218/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
Copy link
Contributor

ndkazu commented Jul 8, 2024

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

@ndkazu
Copy link
Contributor

ndkazu commented Jul 11, 2024

@dmitry-markin , I have PR ready to be reviewed here:
#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...

@dmitry-markin dmitry-markin linked a pull request Sep 18, 2024 that will close this issue
github-merge-queue bot pushed a commit that referenced this issue Sep 18, 2024
## Issue
#4858

## Description
This PR removes `libp2p::request_response::OutboundFailure` from
`substrate/client/network/sync/src/engine.rs`. This way, the dependency
with the library `libp2p` is removed from `sc-network-sync`.

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: command-bot <>
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
Co-authored-by: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. I4-refactor Code needs refactoring. T0-node This PR/Issue is related to the topic “node”.
Projects
Status: Blocked ⛔️
Development

Successfully merging a pull request may close this issue.

4 participants