-
Notifications
You must be signed in to change notification settings - Fork 732
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
Integrate litep2p into Polkadot SDK #2944
Conversation
Users of `sc-network` cannot directly depend on `libp2p::PeerId` as it won't be the only `PeerId` that will be provided. `sc-network` still re-exports `libp2p::PeerId` as `sc_network::PeerId` but any code using `PeerId` should depend on `sc-network-types::PeerId` as it's agnostic over the underlying networking backend implementation while remaining compatible with `libp2p::PeerId`.
Implement `NetworkBackend` for `NetworkWorker`, introduce more traits related to making notification/request-response protocols generic over the underlying networking backend and start using these traits throughout the codebase. `sc_network::NetworkService` is no longer provided as an object but instead as `Arc<dyn NetworkService>` which abstracts the underlying networking backend without introducing generics across the entire codebase. Notification/request-response protocol configurations are generic over the networking backend as libp2p and litep2p have slightly different ways of providing said protocols and thus require custom ways of initializing them. The network backend be selected with `--network-backend` switch
The new backend can be selected with `--network-backend litep2p`
The PR is already nicely split in three commits as per bullet points, so what about publishing it as three separate PRs to make reviewing easier? Does it make sense from the perspective of LOC in every change (i.e., if 90% of changes are in one of three commits, that likely doesn't make sense)? In the end it's up to you to decide, because rebasing/merging the ladder of three PRs could be quite tedious. |
Is there a benefit from splitting the code into three PRs compared to reviewing the three commits separately in this PR? The workload for the review is the same as far as I see it. |
A possible benefit I had in mind is that when the code is updated (i.e., the review comments are addressed), it would be instantly visible in separate PRs, but the individual commits would stay the same, so all 9700 LOC of changes should be viewed to get the understanding of what the code is after additional commits are pushed. |
I'm not pushing for this, as this would come at the cost of rebasing the PRs |
Do other people have any opinions? Would you like to see this split into multiple PRs or is it ok to review the three commits separately? |
I don't mind reviewing as a single PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... I think actually splitting this PR might be a good idea. Not necessarily because of making it easier to review, but simply because that'd probably speed up how fast we can merge it. The PeerId
commit is a good example of something that we could just merge almost immediately.
I don't mind if the review takes a bit longer because honestly I see now value in getting the first two commits into master while the third one is still in review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outstanding work! 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments (mostly nits) for the sc_network_types::PeerId
commit (the first commit).
substrate/client/network/src/litep2p/shim/notification/peerset.rs
Outdated
Show resolved
Hide resolved
[litep2p](https://github.com/altonen/litep2p) is a libp2p-compatible P2P networking library. It supports all of the features of `rust-libp2p` that are currently being utilized by Polkadot SDK. Compared to `rust-libp2p`, `litep2p` has a quite different architecture which is why the new `litep2p` network backend is only able to use a little of the existing code in `sc-network`. The design has been mainly influenced by how we'd wish to structure our networking-related code in Polkadot SDK: independent higher-levels protocols directly communicating with the network over links that support bidirectional backpressure. A good example would be `NotificationHandle`/`RequestResponseHandle` abstractions which allow, e.g., `SyncingEngine` to directly communicate with peers to announce/request blocks. I've tried running `polkadot --network-backend litep2p` with a few different peer configurations and there is a noticeable reduction in networking CPU usage. For high load (`--out-peers 200`), networking CPU usage goes down from ~110% to ~30% (80 pp) and for normal load (`--out-peers 40`), the usage goes down from ~55% to ~18% (37 pp). These should not be taken as final numbers because: a) there are still some low-hanging optimization fruits, such as enabling [receive window auto-tuning](libp2p/rust-yamux#176), integrating `Peerset` more closely with `litep2p` or improving memory usage of the WebSocket transport b) fixing bugs/instabilities that incorrectly cause `litep2p` to do less work will increase the networking CPU usage c) verification in a more diverse set of tests/conditions is needed Nevertheless, these numbers should give an early estimate for CPU usage of the new networking backend. This PR consists of three separate changes: * introduce a generic `PeerId` (wrapper around `Multihash`) so that we don't have use `NetworkService::PeerId` in every part of the code that uses a `PeerId` * introduce `NetworkBackend` trait, implement it for the libp2p network stack and make Polkadot SDK generic over `NetworkBackend` * implement `NetworkBackend` for litep2p The new library should be considered experimental which is why `rust-libp2p` will remain as the default option for the time being. This PR currently depends on the master branch of `litep2p` but I'll cut a new release for the library once all review comments have been addresses. --------- Signed-off-by: Alexandru Vasile <[email protected]> Co-authored-by: Dmitry Markin <[email protected]> Co-authored-by: Alexandru Vasile <[email protected]> Co-authored-by: Alexandru Vasile <[email protected]>
[litep2p](https://github.com/altonen/litep2p) is a libp2p-compatible P2P networking library. It supports all of the features of `rust-libp2p` that are currently being utilized by Polkadot SDK. Compared to `rust-libp2p`, `litep2p` has a quite different architecture which is why the new `litep2p` network backend is only able to use a little of the existing code in `sc-network`. The design has been mainly influenced by how we'd wish to structure our networking-related code in Polkadot SDK: independent higher-levels protocols directly communicating with the network over links that support bidirectional backpressure. A good example would be `NotificationHandle`/`RequestResponseHandle` abstractions which allow, e.g., `SyncingEngine` to directly communicate with peers to announce/request blocks. I've tried running `polkadot --network-backend litep2p` with a few different peer configurations and there is a noticeable reduction in networking CPU usage. For high load (`--out-peers 200`), networking CPU usage goes down from ~110% to ~30% (80 pp) and for normal load (`--out-peers 40`), the usage goes down from ~55% to ~18% (37 pp). These should not be taken as final numbers because: a) there are still some low-hanging optimization fruits, such as enabling [receive window auto-tuning](libp2p/rust-yamux#176), integrating `Peerset` more closely with `litep2p` or improving memory usage of the WebSocket transport b) fixing bugs/instabilities that incorrectly cause `litep2p` to do less work will increase the networking CPU usage c) verification in a more diverse set of tests/conditions is needed Nevertheless, these numbers should give an early estimate for CPU usage of the new networking backend. This PR consists of three separate changes: * introduce a generic `PeerId` (wrapper around `Multihash`) so that we don't have use `NetworkService::PeerId` in every part of the code that uses a `PeerId` * introduce `NetworkBackend` trait, implement it for the libp2p network stack and make Polkadot SDK generic over `NetworkBackend` * implement `NetworkBackend` for litep2p The new library should be considered experimental which is why `rust-libp2p` will remain as the default option for the time being. This PR currently depends on the master branch of `litep2p` but I'll cut a new release for the library once all review comments have been addresses. --------- Signed-off-by: Alexandru Vasile <[email protected]> Co-authored-by: Dmitry Markin <[email protected]> Co-authored-by: Alexandru Vasile <[email protected]> Co-authored-by: Alexandru Vasile <[email protected]>
This pull request has been mentioned on Polkadot Forum. There might be relevant details there: https://forum.polkadot.network/t/paritytech-update-for-april/7646/1 |
I understand that this is an important change, and a technological achievement indeed (all judging by the number of emoji reaction 🚀), but I want to ask for future similar changes to Please see #5, https://forum.polkadot.network/t/polkadot-parachain-omni-node-gathering-ideas-and-feedback/7823 and similar :) |
@michalkucharczyk good data point for the improvements we want to make. Networking changes should obviously not affect client code, if they are not customizing things. |
I had started locally to fix this up, but have given up. The reviewers should not have accepted it in this state, especially for an experimental feature. |
) This PR introduces custom types / substrate wrappers for `Multiaddr`, `multiaddr::Protocol`, `Multihash`, `ed25519::*` and supplementary types like errors and iterators. This is needed to unblock `libp2p` upgrade PR #1631 after #2944 was merged. `libp2p` and `litep2p` currently depend on different versions of `multiaddr` crate, and introduction of this "common ground" types is needed to support independent version upgrades of `multiaddr` and dependent crates in `libp2p` & `litep2p`. While being just convenient to not tie versions of `libp2p` & `litep2p` dependencies together, it's currently not even possible to keep `libp2p` & `litep2p` dependencies updated to the same versions as `multiaddr` in `libp2p` depends on `libp2p-identity` that we can't include as a dependency of `litep2p`, which has it's own `PeerId` type. In the future, to keep things updated on `litep2p` side, we will likely need to fork `multiaddr` and make it use `litep2p` `PeerId` as a payload of `/p2p/...` protocol. With these changes, common code in substrate uses these custom types, and `litep2p` & `libp2p` backends use corresponding libraries types.
…ritytech#4198) This PR introduces custom types / substrate wrappers for `Multiaddr`, `multiaddr::Protocol`, `Multihash`, `ed25519::*` and supplementary types like errors and iterators. This is needed to unblock `libp2p` upgrade PR paritytech#1631 after paritytech#2944 was merged. `libp2p` and `litep2p` currently depend on different versions of `multiaddr` crate, and introduction of this "common ground" types is needed to support independent version upgrades of `multiaddr` and dependent crates in `libp2p` & `litep2p`. While being just convenient to not tie versions of `libp2p` & `litep2p` dependencies together, it's currently not even possible to keep `libp2p` & `litep2p` dependencies updated to the same versions as `multiaddr` in `libp2p` depends on `libp2p-identity` that we can't include as a dependency of `litep2p`, which has it's own `PeerId` type. In the future, to keep things updated on `litep2p` side, we will likely need to fork `multiaddr` and make it use `litep2p` `PeerId` as a payload of `/p2p/...` protocol. With these changes, common code in substrate uses these custom types, and `litep2p` & `libp2p` backends use corresponding libraries types.
…ritytech#4198) This PR introduces custom types / substrate wrappers for `Multiaddr`, `multiaddr::Protocol`, `Multihash`, `ed25519::*` and supplementary types like errors and iterators. This is needed to unblock `libp2p` upgrade PR paritytech#1631 after paritytech#2944 was merged. `libp2p` and `litep2p` currently depend on different versions of `multiaddr` crate, and introduction of this "common ground" types is needed to support independent version upgrades of `multiaddr` and dependent crates in `libp2p` & `litep2p`. While being just convenient to not tie versions of `libp2p` & `litep2p` dependencies together, it's currently not even possible to keep `libp2p` & `litep2p` dependencies updated to the same versions as `multiaddr` in `libp2p` depends on `libp2p-identity` that we can't include as a dependency of `litep2p`, which has it's own `PeerId` type. In the future, to keep things updated on `litep2p` side, we will likely need to fork `multiaddr` and make it use `litep2p` `PeerId` as a payload of `/p2p/...` protocol. With these changes, common code in substrate uses these custom types, and `litep2p` & `libp2p` backends use corresponding libraries types.
- Upgrade Polkadot-sdk 1.10.0 to 1.11.0 - Update weights to reflect the new version. Notable Changes: - [Apply patch `run_with_spec`](paritytech/polkadot-sdk#3512) - [Genesis Presets](paritytech/polkadot-sdk#2714) - [Integrate LiteP2P](paritytech/polkadot-sdk#2944) - [Template Simplification](https://github.com/paritytech/polkadot-sdk/pull/3801/files) For more details, please refer to: [Release Notes](https://github.com/paritytech/polkadot-sdk/releases/tag/polkadot-v1.11.0) issue-1957
- Upgrade Polkadot-sdk 1.10.0 to 1.11.0 - Update weights to reflect the new version. Notable Changes: - [Apply patch `run_with_spec`](paritytech/polkadot-sdk#3512) - [Genesis Presets](paritytech/polkadot-sdk#2714) - [Integrate LiteP2P](paritytech/polkadot-sdk#2944) - [Template Simplification](https://github.com/paritytech/polkadot-sdk/pull/3801/files) For more details, please refer to: [Release Notes](https://github.com/paritytech/polkadot-sdk/releases/tag/polkadot-v1.11.0) issue-1957
- Upgrade Polkadot-sdk 1.10.0 to 1.11.0 - Update weights to reflect the new version. Notable Changes: - [Apply patch `run_with_spec`](paritytech/polkadot-sdk#3512) - [Genesis Presets](paritytech/polkadot-sdk#2714) - [Integrate LiteP2P](paritytech/polkadot-sdk#2944) - [Template Simplification](https://github.com/paritytech/polkadot-sdk/pull/3801/files) For more details, please refer to: [Release Notes](https://github.com/paritytech/polkadot-sdk/releases/tag/polkadot-v1.11.0) issue-1957
- Upgrade Polkadot-sdk 1.10.0 to 1.11.0 - Update weights to reflect the new version. Notable Changes: - [Apply patch `run_with_spec`](paritytech/polkadot-sdk#3512) - [Genesis Presets](paritytech/polkadot-sdk#2714) - [Integrate LiteP2P](paritytech/polkadot-sdk#2944) - [Template Simplification](https://github.com/paritytech/polkadot-sdk/pull/3801/files) For more details, please refer to: [Release Notes](https://github.com/paritytech/polkadot-sdk/releases/tag/polkadot-v1.11.0) issue-1957
- Upgrade Polkadot-sdk 1.10.0 to 1.11.0 - Update weights to reflect the new version. Notable Changes: - [Apply patch `run_with_spec`](paritytech/polkadot-sdk#3512) - [Genesis Presets](paritytech/polkadot-sdk#2714) - [Integrate LiteP2P](paritytech/polkadot-sdk#2944) - [Template Simplification](https://github.com/paritytech/polkadot-sdk/pull/3801/files) - [Remove try-runtime-cli](https://github.com/paritytech/polkadot-sdk/pull/4017/files#diff-3a3aa5e088741f8ab1ca38fbe314c7a150d18b7466426f4ad2569113017e8439) For more details, please refer to: [Release Notes](https://github.com/paritytech/polkadot-sdk/releases/tag/polkadot-v1.11.0) #1957
- Upgrade Polkadot-sdk 1.10.0 to 1.11.0 - Update weights to reflect the new version. Notable Changes: - [Apply patch `run_with_spec`](paritytech/polkadot-sdk#3512) - [Genesis Presets](paritytech/polkadot-sdk#2714) - [Integrate LiteP2P](paritytech/polkadot-sdk#2944) - [Template Simplification](https://github.com/paritytech/polkadot-sdk/pull/3801/files) - [Remove try-runtime-cli](https://github.com/paritytech/polkadot-sdk/pull/4017/files#diff-3a3aa5e088741f8ab1ca38fbe314c7a150d18b7466426f4ad2569113017e8439) For more details, please refer to: [Release Notes](https://github.com/paritytech/polkadot-sdk/releases/tag/polkadot-v1.11.0)
- Upgrade Polkadot-sdk 1.10.0 to 1.11.0 - Update weights to reflect the new version. Notable Changes: - [Apply patch `run_with_spec`](paritytech/polkadot-sdk#3512) - [Genesis Presets](paritytech/polkadot-sdk#2714) - [Integrate LiteP2P](paritytech/polkadot-sdk#2944) - [Template Simplification](https://github.com/paritytech/polkadot-sdk/pull/3801/files) - [Remove try-runtime-cli](https://github.com/paritytech/polkadot-sdk/pull/4017/files#diff-3a3aa5e088741f8ab1ca38fbe314c7a150d18b7466426f4ad2569113017e8439) For more details, please refer to: [Release Notes](https://github.com/paritytech/polkadot-sdk/releases/tag/polkadot-v1.11.0) #1957
litep2p is a libp2p-compatible P2P networking library. It supports all of the features of
rust-libp2p
that are currently being utilized by Polkadot SDK.Compared to
rust-libp2p
,litep2p
has a quite different architecture which is why the newlitep2p
network backend is only able to use a little of the existing code insc-network
. The design has been mainly influenced by how we'd wish to structure our networking-related code in Polkadot SDK: independent higher-levels protocols directly communicating with the network over links that support bidirectional backpressure. A good example would beNotificationHandle
/RequestResponseHandle
abstractions which allow, e.g.,SyncingEngine
to directly communicate with peers to announce/request blocks.I've tried running
polkadot --network-backend litep2p
with a few different peer configurations and there is a noticeable reduction in networking CPU usage. For high load (--out-peers 200
), networking CPU usage goes down from ~110% to ~30% (80 pp) and for normal load (--out-peers 40
), the usage goes down from ~55% to ~18% (37 pp).These should not be taken as final numbers because:
a) there are still some low-hanging optimization fruits, such as enabling receive window auto-tuning, integrating
Peerset
more closely withlitep2p
or improving memory usage of the WebSocket transportb) fixing bugs/instabilities that incorrectly cause
litep2p
to do less work will increase the networking CPU usagec) verification in a more diverse set of tests/conditions is needed
Nevertheless, these numbers should give an early estimate for CPU usage of the new networking backend.
This PR consists of three separate changes:
PeerId
(wrapper aroundMultihash
) so that we don't have useNetworkService::PeerId
in every part of the code that uses aPeerId
NetworkBackend
trait, implement it for the libp2p network stack and make Polkadot SDK generic overNetworkBackend
NetworkBackend
for litep2pThe new library should be considered experimental which is why
rust-libp2p
will remain as the default option for the time being. This PR currently depends on the master branch oflitep2p
but I'll cut a new release for the library once all review comments have been addresses.