Remove libp2p dependency from sc-network-sync

[ndkazu]

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.

[bkchr]

@ndkazu totally the crate libp2p should be removed as a dependency. Also the type OutboundFailure is still a libp2p type, even if you import it from sc-network. Not sure this is what @dmitry-markin wanted to achieve here.

[ndkazu]

@ndkazu totally the crate libp2p should be removed as a dependency. Also the type OutboundFailure is still a libp2p type, even if you import it from sc-network. Not sure this is what @dmitry-markin wanted to achieve here.

Thanks for the clarification, will keep looking into it then.

[ndkazu]

The main libp2p dependency in engine.rs being the request_responses::Network, I decided to add a Network2 field in the request_responses struct, and by using a CustomOutboundFailure enum which does not depend on libp2p, I make sure that engine is completely free from any libp2p dependency.
@dmitry-markin & @bkchr , could you review the PR please?

[ndkazu]

@dmitry-markin & @bkchr , only OutboundFailure in engine.rs is mentioned in the issue, but by looking at the definition of sc_network_types::PeerId I see that it is also coming from libp2p_identity: is it ok to keep this one?

[dmitry-markin]

@dmitry-markin & @bkchr , only OutboundFailure in engine.rs is mentioned in the issue, but by looking at the definition of sc_network_types::PeerId I see that it is also coming from libp2p_identity: is it ok to keep this one?

sc_network_types::PeerId is fine as it's a Substrate-specific wrapper that does not expose the underlying type, and can be updated later to wrap another networking library type. Of course, if there are other types that make impossible removing the libp2p dependency from Cargo.toml, they have to be replaced by Substrate types as well.

[dmitry-markin]

@ndkazu totally the crate libp2p should be removed as a dependency. Also the type OutboundFailure is still a libp2p type, even if you import it from sc-network. Not sure this is what @dmitry-markin wanted to achieve here.

I would say the first step would be to remove libp2p from Cargo.toml, but we also want to make sure the common sc-network code shared by both libp2p & litep2p network backends does not depend on the specific network libraries, so we can completely remove one of the libraries in the future. I would say removing libp2p dependency is higher in priority than removing litep2p dependency (and there shouldn't be any already).

[ndkazu]

@dmitry-markin & @bkchr , only OutboundFailure in engine.rs is mentioned in the issue, but by looking at the definition of sc_network_types::PeerId I see that it is also coming from libp2p_identity: is it ok to keep this one?

sc_network_types::PeerId is fine as it's a Substrate-specific wrapper that does not expose the underlying type, and can be updated later to wrap another networking library type. Of course, if there are other types that make impossible removing the libp2p dependency from Cargo.toml, they have to be replaced by Substrate types as well.

In this case, I think this PR is ready for Review.

[ndkazu]

@dmitry-markin & @bkchr , correct me if I'm wrong, but after spending time reading the code, I believe the only way to complete this PR as requested is to remove ALL libp2p dependency found in request_responses.rs. This however, is not as trivial as the issue made it seem... I am still digging...

[ndkazu]

Do not hesitate to let me know If you think I should pull the brake/re-think my approach...

[lexnv]

bot fmt

[command-bot]

@lexnv https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7366911 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 5-4e41e1d2-2cdb-4737-9f61-a17c6d154398 to cancel this command or bot cancel to cancel all commands in this pull request.

[command-bot]

@lexnv Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7366911 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7366911/artifacts/download.

[ndkazu]

Sorry, another bot fmt needed.

[ndkazu]

There is a failing CI test that seems unrelated to this PR: not sure what to do.

[dmitry-markin]

bot fmt

[command-bot]

@dmitry-markin https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7369110 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 1-76b1d14d-f53e-4b5b-9c2f-50ade48a208f to cancel this command or bot cancel to cancel all commands in this pull request.

[command-bot]

@dmitry-markin Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7369110 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7369110/artifacts/download.

[dmitry-markin]

bot fmt was a no-op, the format was fine. I only had to manually correct the EOF in prdoc in vim as it had some spaces after a newline.

The unrelated tests failing is not an issue, you can ignore them.

[dmitry-markin]

Good job here! Thanks for contributing!

Once @lexnv approves the PR, we will merge it.

[dmitry-markin]

Can you also update the PR description to reflect the latest state of PR? Can be something similar to your prdoc.

[lexnv]

LGTM! Thanks for contributing!

[ndkazu]

Shall we address issue #4859 next? 😄

[bkchr]

Shall we address issue #4859 next? 😄

Fell free :)