Syncing strategy refactoring (part 2) #5666
Conversation
|
@dmitry-markin @lexnv another one for you folks |
|
Impeccable! Once again thanks for excellent work! |
…refactoring-part-2 # Conflicts: # polkadot/node/service/src/lib.rs
| let request = WarpProofRequest { begin }; | ||
|
|
||
| let Some(protocol_name) = self.protocol_name.clone() else { | ||
| warn!( |
There was a problem hiding this comment.
nit: Wouldn't this generate too much noise?
There was a problem hiding this comment.
It will not, this method is not supposed to be called when there is no need for warp requests, that would basically be an implementation bug. This code effectively migrated from substrate/client/network/sync/src/engine.rs (see the change at the bottom of the file). Should be clear if looking at individual commits.
It is just that the invariants for warp sync are a bit strange, which causes this property to be optional (I only did mechanical refactoring, I didn't change the algorithm).
|
What else is left here? I have another set of changes that depend on this for review already 🙂 |
43cd6fd
| /// Metrics. | ||
| pub metrics: NotificationMetrics, | ||
| } | ||
|
|
||
| /// Build the network service, the network status sinks and an RPC sender. | ||
| pub fn build_network<TBl, TNet, TExPool, TImpQu, TCl>( | ||
| params: BuildNetworkParams<TBl, TNet, TExPool, TImpQu, TCl>, | ||
| pub fn build_network<Block, Net, TxPool, IQ, Client>( |
There was a problem hiding this comment.
While I'm late to the party (sorry).
Before this goes into any stable release or whatever, can we please revert this?
Basically we should not expose build_polkadot_syncing_strategy . Instead we should do this internally in this build_network function and then expose some build_network_advanced or whatever for @nazar-pc that takes then the syncing strategy. This way there is no breaking change to the outside for developers. (that only have a node and not any custom syncing stuff)
The end goal should be that all the node/service changes to all the nodes in this repo are reverted.
@lexnv or @dmitry-markin can you please take care of this?
There was a problem hiding this comment.
#5737 actually extracted the whole syncing strategy out of build_network and exposed build_default_syncing_engine function to construct the typical setup.
The fact that build_network was not just building the network, but also syncing engine, syncing strategies and hardcoding all the protocol was a long standing problem IMO. It was also taking (and still does) the whole sc_service::config::Configuration, most of which it doesn't use, which is also a problem (though I didn't address it in any of my changes). It makes using Substrate as a library quite painful for those who need to customize it the way we do at Subspace.
My goal would be for build_network to be simplified to little more than Net::new() call. The fact that it unconditionally adds a whole bunch of the protocols is annoying and undesirable.
Having build_network_advanced is an option, but not a long-term solution the way I imagine it. Happy to push it into #5737 if that is something you feel strongly about.
There was a problem hiding this comment.
I guess the gist of it is that build_network should build the network, not syncing engines and other stuff it is coupled with right now.
There was a problem hiding this comment.
I guess the gist of it is that
build_networkshould build the network, not syncing engines and other stuff it is coupled with right now.
I mean I'm not against this and I support this. Just about how it is done. I mean just keep the function do what it was doing all the time and then provide some extended way.
There was a problem hiding this comment.
Reverted build_network's API to before this PR and added build_network_advanced instead in #5737
…_network` API before paritytech#5666
# Description Follow-up to paritytech#5469 and mostly covering paritytech#5333. The primary change here is that syncing strategy is no longer created inside of syncing engine, instead syncing strategy is an argument of syncing engine, more specifically it is an argument to `build_network` that most downstream users will use. This also extracts addition of request-response protocols outside of network construction, making sure they are physically not present when they don't need to be (imagine syncing strategy that uses none of Substrate's protocols in its implementation for example). This technically allows to completely replace syncing strategy with whatever strategy chain might need. There will be at least one follow-up PR that will simplify `SyncingStrategy` trait and other public interfaces to remove mentions of block/state/warp sync requests, replacing them with generic APIs, such that strategies where warp sync is not applicable don't have to provide dummy method implementations, etc. ## Integration Downstream projects will have to write a bit of boilerplate calling `build_polkadot_syncing_strategy` function to create previously default syncing strategy. ## Review Notes Please review PR through individual commits rather than the final diff, it will be easier that way. The changes are mostly just moving code around one step at a time. # Checklist * [x] My PR includes a detailed description as outlined in the "Description" and its two subsections above. * [x] My PR follows the [labeling requirements]( https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process ) of this project (at minimum one label for `T` required) * External contributors: ask maintainers to put the right label on your PR. * [x] I have made corresponding changes to the documentation (if applicable) (cherry picked from commit 43cd6fd)
Description
Follow-up to #5469 and mostly covering #5333.
The primary change here is that syncing strategy is no longer created inside of syncing engine, instead syncing strategy is an argument of syncing engine, more specifically it is an argument to
build_networkthat most downstream users will use. This also extracts addition of request-response protocols outside of network construction, making sure they are physically not present when they don't need to be (imagine syncing strategy that uses none of Substrate's protocols in its implementation for example).This technically allows to completely replace syncing strategy with whatever strategy chain might need.
There will be at least one follow-up PR that will simplify
SyncingStrategytrait and other public interfaces to remove mentions of block/state/warp sync requests, replacing them with generic APIs, such that strategies where warp sync is not applicable don't have to provide dummy method implementations, etc.Integration
Downstream projects will have to write a bit of boilerplate calling
build_polkadot_syncing_strategyfunction to create previously default syncing strategy.Review Notes
Please review PR through individual commits rather than the final diff, it will be easier that way. The changes are mostly just moving code around one step at a time.
Checklist
Trequired)