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-authority-discovery #5842

Open
wants to merge 131 commits into
base: master
Choose a base branch
from

Conversation

ndkazu
Copy link
Contributor

@ndkazu ndkazu commented Sep 26, 2024

Issue

#4859

Description

This PR removes libp2p types in authority-discovery, and replace them with network backend agnostic types from sc-network-types.
The sc-network interface is therefore updated accordingly.

@ndkazu
Copy link
Contributor Author

ndkazu commented Nov 5, 2024

@dmitry-markin , what about litep2p ? keep it or remove/replace it?
I see the following import:
litep2p::protocol::libp2p::bitswap::{ BitswapEvent, BitswapHandle, BlockPresenceType, Config, ResponseType, WantType, };

@dmitry-markin
Copy link
Contributor

bot fmt

@command-bot
Copy link

command-bot bot commented Nov 8, 2024

@dmitry-markin https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7724737 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 65-3e7512e2-ac5c-4e3f-a7b8-50230bc1b3da to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Nov 8, 2024

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

@dmitry-markin
Copy link
Contributor

@dmitry-markin , what about litep2p ? keep it or remove/replace it? I see the following import: litep2p::protocol::libp2p::bitswap::{ BitswapEvent, BitswapHandle, BlockPresenceType, Config, ResponseType, WantType, };

Nothing wrong with this. This line means we are importing libp2p bitswap protocol implementation from litep2p.

Copy link
Contributor

@dmitry-markin dmitry-markin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some minor nits, otherwise looks good!

substrate/client/authority-discovery/src/worker.rs Outdated Show resolved Hide resolved
substrate/client/authority-discovery/src/worker/tests.rs Outdated Show resolved Hide resolved
substrate/client/network/src/litep2p/mod.rs Outdated Show resolved Hide resolved
@@ -1455,17 +1455,17 @@ where
fn handle_worker_message(&mut self, msg: ServiceToWorkerMsg) {
match msg {
ServiceToWorkerMsg::GetValue(key) =>
self.network_service.behaviour_mut().get_value(key),
self.network_service.behaviour_mut().get_value(key.to_vec().into()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just key.into() here and below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say that it's because service.rs in general, and network_service in particular, are still dependant on libp2p.

substrate/client/network/types/src/kad.rs Outdated Show resolved Hide resolved
substrate/client/network/types/src/kad.rs Outdated Show resolved Hide resolved
substrate/client/network/types/src/kad.rs Outdated Show resolved Hide resolved
substrate/client/network/types/src/kad.rs Outdated Show resolved Hide resolved
substrate/client/network/types/src/kad.rs Outdated Show resolved Hide resolved
substrate/client/network/types/src/kad.rs Outdated Show resolved Hide resolved
@@ -864,7 +864,7 @@ impl<B: BlockT + 'static, H: ExHashT> NetworkBackend<B, H> for Litep2pNetworkBac
);

self.event_streams.send(Event::Dht(
DhtEvent::ValuePut(libp2p::kad::RecordKey::new(&key))
DhtEvent::ValuePut(sc_network_types::kad::Key::new(&key.to_vec()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't the following work to avoid copying?

Suggested change
DhtEvent::ValuePut(sc_network_types::kad::Key::new(&key.to_vec()))
DhtEvent::ValuePut(sc_network_types::kad::Key::from(key.to_vec()))

Also below.

Copy link
Contributor

@dmitry-markin dmitry-markin Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better: we can implement From for sc_network_types::kad::Key to convert it directly from litep2p key, and then the line will become:

DhtEvent::ValuePut(key.into())

Copy link
Contributor Author

@ndkazu ndkazu Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better: we can implement From for sc_network_types::kad::Key to convert it directly from litep2p key, and then the line will become:

DhtEvent::ValuePut(key.into())

From was already implemented for sc_network_types::kad::Key 😃 so now we have DhtEvent::ValuePut(key.into()). I'm guessing the task in the next issue, will be to remove libp2p from behaviour.rs and service.rs ? 👀

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure we can remove libp2p from behavior.rs and service.rs since they are specific to libp2p architecture (they implement the behavior and work with swarm events)

@ndkazu
Copy link
Contributor Author

ndkazu commented Nov 12, 2024

Good to merge? 👀

substrate/client/network/src/litep2p/mod.rs Outdated Show resolved Hide resolved
substrate/client/network/src/litep2p/mod.rs Outdated Show resolved Hide resolved
@dmitry-markin
Copy link
Contributor

Good to merge? 👀

Let me have a look why zombienet tests are failing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants