Network sync refactoring (part 6) #11940
Conversation
…ove unnecessary dependency
… from `NetworkService` that are already provided by trait impls
…work-common` and de-duplicate methods on `NetworkService`
…-network-common` and de-duplicate methods on `NetworkService`
…-network-common` and de-duplicate methods in `NetworkService`
…NetworkStateInfo` impl
nazar-pc
force-pushed
the
network-sync-refactoring-part-6
branch
from
6263608 to
5caf558
Compare
|
Fixed documentation references |
|
@TriplEight I guess you already noticed this:
It isn't indeed because CI didn't pass there, but it is physically mergeable (no conflicts against |
|
Looks good here. |
There was a problem hiding this comment.
I'm approving this as it looks good. However, we should stop here. There is no need to have Substrate being completely opaque to the underlying network. This mainly just introduces complexity that we don't really need.
The task should really be to make networking agnostic that there exists a syncing protocol. It should not need to care. The networking crate should only provide ways to register protocols and ensure to drive these protocols. One of these protocols is then syncing.
| /// Need a better solution to decide authorized_only, but now just use reserved_only flag for | ||
| /// prototyping. |
| fn add_reserved_peer(&self, peer: String) -> Result<(), String>; | ||
|
|
||
| /// Removes a `PeerId` from the list of reserved peers. | ||
| fn remove_reserved_peer(&self, peer_id: PeerId); | ||
|
|
||
| /// Sets the reserved set of a protocol to the given set of peers. | ||
| /// | ||
| /// Each `Multiaddr` must end with a `/p2p/` component containing the `PeerId`. It can also | ||
| /// consist of only `/p2p/<peerid>`. | ||
| /// | ||
| /// The node will start establishing/accepting connections and substreams to/from peers in this | ||
| /// set, if it doesn't have any substream open with them yet. | ||
| /// | ||
| /// Note however, if a call to this function results in less peers on the reserved set, they | ||
| /// will not necessarily get disconnected (depending on available free slots in the peer set). | ||
| /// If you want to also disconnect those removed peers, you will have to call | ||
| /// `remove_from_peers_set` on those in addition to updating the reserved set. You can omit | ||
| /// this step if the peer set is in reserved only mode. | ||
| /// | ||
| /// Returns an `Err` if one of the given addresses is invalid or contains an | ||
| /// invalid peer ID (which includes the local peer ID). | ||
| fn set_reserved_peers( | ||
| &self, | ||
| protocol: Cow<'static, str>, | ||
| peers: HashSet<Multiaddr>, | ||
| ) -> Result<(), String>; | ||
|
|
||
| /// Add peers to a peer set. | ||
| /// | ||
| /// Each `Multiaddr` must end with a `/p2p/` component containing the `PeerId`. It can also | ||
| /// consist of only `/p2p/<peerid>`. | ||
| /// | ||
| /// Returns an `Err` if one of the given addresses is invalid or contains an | ||
| /// invalid peer ID (which includes the local peer ID). | ||
| fn add_peers_to_reserved_set( | ||
| &self, | ||
| protocol: Cow<'static, str>, | ||
| peers: HashSet<Multiaddr>, | ||
| ) -> Result<(), String>; | ||
|
|
||
| /// Remove peers from a peer set. | ||
| fn remove_peers_from_reserved_set(&self, protocol: Cow<'static, str>, peers: Vec<PeerId>); |
There was a problem hiding this comment.
Most of these functions are basically duplicated, just with some small differences in their behavior. I think, merging them and controlling the exact behavior with an enum as arguments sounds better?
There was a problem hiding this comment.
Not sure, having separate names is more convenient IMO. The primary goal in this PR is to not change API if at all possible, just moving things around.
client/network/common/src/service.rs
Outdated
| /// Returns the number of peers we're connected to. | ||
| fn num_connected(&self) -> usize; |
There was a problem hiding this comment.
| /// Returns the number of peers we're connected to. | |
| fn num_connected(&self) -> usize; | |
| /// Returns the number of peers in the sync peer set we're connected to. | |
| fn sync_num_connected(&self) -> usize; |
| /// NOTE: Traits can't consume itself, but calling this method second time will return an error. | ||
| fn send(&mut self, notification: Vec<u8>) -> Result<(), NotificationSenderError>; |
There was a problem hiding this comment.
There was a problem hiding this comment.
You can define it, but can't call it later as trait object: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f01675569f20b3c7c6bc96cc0e3f0565
| async fn ready(&self) | ||
| -> Result<Box<dyn NotificationSenderReady + '_>, NotificationSenderError>; |
There was a problem hiding this comment.
Why not have an associated type as return value?
There was a problem hiding this comment.
I believe it either doesn't work at all or becomes tricky with trait object when we want to use it.
There was a problem hiding this comment.
Then we don't need any trait object? And then we also can make the function consume the object. Sounds like a win win to me :D
There was a problem hiding this comment.
The challenge is that both before and after this PR various places use trait objects of networking traits (to store them as properties of other structs for instance). Having everything generic throughout is too invasive change with almost unbounded scope unfortunately.
|
@nazar-pc and also please provide your polkadot address for a tip :) |
I have just one small upcoming PR where explicit
Understood.
✔️ |
This should be fine, especially if you have some need for this :) Just wanted to highlight that we should stop here 😬 |
|
/tip medium |
|
@bkchr A medium tip was successfully submitted for nazar-pc (1vSxzbyz2cJREAuVWjhXUT1ds8vBzoxn2w4asNpusQKwjJd on polkadot). https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/treasury/tips |
bkchr
added
A6-seemsok
B0-silent
|
bot merge |
* Trivial networking changes for Substrate PR paritytech/substrate#11940 * update lockfile for {"substrate"} Co-authored-by: parity-processbot <>
* Trivial networking changes for Substrate PR paritytech/substrate#11940 * Apply formatting rules * update lockfile for {"polkadot", "substrate"} Co-authored-by: parity-processbot <>
* Trivial networking changes for Substrate PR paritytech/substrate#11940 * Apply formatting rules * update lockfile for {"polkadot", "substrate"} Co-authored-by: parity-processbot <>
* changes to read json spec in test binary
* add zombienet tests
* fmt
* use {{COL_IMAGE}} and clean config
* add comment and use relay image from env
* use test-parachain image from pr
* fix warns
* fix warns
* fmt
* typo
* fix ci to use zombienet image
* fix spawn nodes for test
* reorg test
* add within to test
* remove check for full node collators is up
* add tests for pov, mirate solo to para, sync blocks
* bump zombienet image
* add job dep with artifacts
* add sleep for test
* fix after merge
* fmt
* bump zombienet version
* changes from clap
* use base/shared params
* fmt
* debug ci
* add upgrade test
* update js test for debug
* less debug in test
* print assertion
* fix upgrade test
* Collator key only needed if we run as collator
* [Fix] Benchmark build artifact folder creation (#1518)
* Trivial networking changes for Substrate PR #11940 (#1486)
* Trivial networking changes for Substrate PR paritytech/substrate#11940
* Apply formatting rules
* update lockfile for {"polkadot", "substrate"}
Co-authored-by: parity-processbot <>
* bump zombienet version
* update network def for test
* typo
Co-authored-by: Sebastian Kunert <[email protected]>
Co-authored-by: Roman Useinov <[email protected]>
Co-authored-by: Nazar Mokrynskyi <[email protected]>
* Extract `NetworkKVProvider` trait in `sc-authority-discovery` and remove unnecessary dependency * Extract `NetworkSyncForkRequest` trait in `sc-finality-grandpa` * Relax requirements on `SyncOracle` trait, remove extra native methods from `NetworkService` that are already provided by trait impls * Move `NetworkSigner` trait from `sc-authority-discovery` into `sc-network-common` and de-duplicate methods on `NetworkService` * Move `NetworkKVProvider` trait from `sc-authority-discovery` into `sc-network-common` and de-duplicate methods on `NetworkService` * Minimize `sc-authority-discovery` dependency on `sc-network` * Move `NetworkSyncForkRequest` trait from `sc-finality-grandpa` to `sc-network-common` and de-duplicate methods in `NetworkService` * Extract `NetworkStatusProvider` trait and de-duplicate methods on `NetworkService` * Extract `NetworkPeers` trait and de-duplicate methods on `NetworkService` * Extract `NetworkEventStream` trait and de-duplicate methods on `NetworkService` * Move more methods from `NetworkService` into `NetworkPeers` trait * Move `NetworkStateInfo` trait into `sc-network-common` * Extract `NetworkNotification` trait and de-duplicate methods on `NetworkService` * Extract `NetworkRequest` trait and de-duplicate methods on `NetworkService` * Remove `NetworkService::local_peer_id()`, it is already provided by `NetworkStateInfo` impl * Extract `NetworkTransaction` trait and de-duplicate methods on `NetworkService` * Extract `NetworkBlock` trait and de-duplicate methods on `NetworkService` * Remove dependencies on `NetworkService` from most of the methods of `sc-service` * Address simple review comments
* Extract `NetworkKVProvider` trait in `sc-authority-discovery` and remove unnecessary dependency * Extract `NetworkSyncForkRequest` trait in `sc-finality-grandpa` * Relax requirements on `SyncOracle` trait, remove extra native methods from `NetworkService` that are already provided by trait impls * Move `NetworkSigner` trait from `sc-authority-discovery` into `sc-network-common` and de-duplicate methods on `NetworkService` * Move `NetworkKVProvider` trait from `sc-authority-discovery` into `sc-network-common` and de-duplicate methods on `NetworkService` * Minimize `sc-authority-discovery` dependency on `sc-network` * Move `NetworkSyncForkRequest` trait from `sc-finality-grandpa` to `sc-network-common` and de-duplicate methods in `NetworkService` * Extract `NetworkStatusProvider` trait and de-duplicate methods on `NetworkService` * Extract `NetworkPeers` trait and de-duplicate methods on `NetworkService` * Extract `NetworkEventStream` trait and de-duplicate methods on `NetworkService` * Move more methods from `NetworkService` into `NetworkPeers` trait * Move `NetworkStateInfo` trait into `sc-network-common` * Extract `NetworkNotification` trait and de-duplicate methods on `NetworkService` * Extract `NetworkRequest` trait and de-duplicate methods on `NetworkService` * Remove `NetworkService::local_peer_id()`, it is already provided by `NetworkStateInfo` impl * Extract `NetworkTransaction` trait and de-duplicate methods on `NetworkService` * Extract `NetworkBlock` trait and de-duplicate methods on `NetworkService` * Remove dependencies on `NetworkService` from most of the methods of `sc-service` * Address simple review comments
Yet another one related to #8686
This time I noticed that there are a few traits in the codebase that somewhat match
NetworkServiceand are implemented for it. AlsoNetworkServiceitself has way too many methods for all kinds of things that are not used in most cases.What I did then is sliced
NetworkServiceinto many logical traits and made interfaces generic in crates that previously depended this specific implementation.Next PR will remove
sc-networkfrom dependencies of most crates, making Substrate a framework with pluggable networking, not just consensus. API provided by new traits is the same as before and may need to be generalized/cleaned up in the future, but first decoupling of the rest of Substrate fromsc-networkis needed.Reviewing commit after commit is recommended. There is no business logic changes here, just moving things around a bit. PR is big, I can cut it in half at any commit if it helps, but all commits are of the same type here. Hopefully trait collection and naming makes sense.
Polkadot companion: paritytech/polkadot#5841
Cumulus companion: paritytech/cumulus#1486
Polkadot address: 1vSxzbyz2cJREAuVWjhXUT1ds8vBzoxn2w4asNpusQKwjJd