Syncing strategy refactoring #5469
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 tasks
dmitry-markin
approved these changes
Aug 29, 2024
|
@lexnv 🙏 |
…refactoring # Conflicts: # substrate/client/network/sync/src/strategy/chain_sync.rs
|
Resolved merge conflict after #5635 |
Merged
via the queue into
paritytech:master
with commit Sep 10, 2024
1f1f20a
203 of 205 checks passed
mordamax
pushed a commit
to paritytech-stg/polkadot-sdk
that referenced
this pull request
Sep 10, 2024
This is a step towards paritytech#5333 The commits with code moving (but no other changes) and actual changes are separated for easier review. Essentially this results in `SyncingStrategy` trait replacing struct (which is renamed to `PolkadotSyncingStrategy`, open for better name suggestions). Technically it is already possible to replace `PolkadotSyncingStrategy<B, Client>` with `Box<dyn SyncingStrategy<B>` in syncing engine, but I decided to postpone such change until we actually have an ability to customize it. It should also be possible to swap `PolkadotSyncingStrategy` with just `ChainSync` that only supports regular full sync from genesis (it also implements `SyncingStrategy` trait). While extracted trait still has a lot of non-generic stuff in it like exposed knowledge of warp sync and `StrategyKey` with hardcoded set of options, I believe this is a step in the right direction and will address those in upcoming PRs. With paritytech#5431 that landed earlier warp sync configuration is more straightforward, but there are still numerous things interleaved that will take some time to abstract away nicely and expose in network config for developers. For now this is an internal change even though data structures are technically public and require major version bump.
3 tasks
mordamax
pushed a commit
to paritytech-stg/polkadot-sdk
that referenced
this pull request
Sep 10, 2024
This is a step towards paritytech#5333 The commits with code moving (but no other changes) and actual changes are separated for easier review. Essentially this results in `SyncingStrategy` trait replacing struct (which is renamed to `PolkadotSyncingStrategy`, open for better name suggestions). Technically it is already possible to replace `PolkadotSyncingStrategy<B, Client>` with `Box<dyn SyncingStrategy<B>` in syncing engine, but I decided to postpone such change until we actually have an ability to customize it. It should also be possible to swap `PolkadotSyncingStrategy` with just `ChainSync` that only supports regular full sync from genesis (it also implements `SyncingStrategy` trait). While extracted trait still has a lot of non-generic stuff in it like exposed knowledge of warp sync and `StrategyKey` with hardcoded set of options, I believe this is a step in the right direction and will address those in upcoming PRs. With paritytech#5431 that landed earlier warp sync configuration is more straightforward, but there are still numerous things interleaved that will take some time to abstract away nicely and expose in network config for developers. For now this is an internal change even though data structures are technically public and require major version bump.
mordamax
pushed a commit
to paritytech-stg/polkadot-sdk
that referenced
this pull request
Sep 10, 2024
This is a step towards paritytech#5333 The commits with code moving (but no other changes) and actual changes are separated for easier review. Essentially this results in `SyncingStrategy` trait replacing struct (which is renamed to `PolkadotSyncingStrategy`, open for better name suggestions). Technically it is already possible to replace `PolkadotSyncingStrategy<B, Client>` with `Box<dyn SyncingStrategy<B>` in syncing engine, but I decided to postpone such change until we actually have an ability to customize it. It should also be possible to swap `PolkadotSyncingStrategy` with just `ChainSync` that only supports regular full sync from genesis (it also implements `SyncingStrategy` trait). While extracted trait still has a lot of non-generic stuff in it like exposed knowledge of warp sync and `StrategyKey` with hardcoded set of options, I believe this is a step in the right direction and will address those in upcoming PRs. With paritytech#5431 that landed earlier warp sync configuration is more straightforward, but there are still numerous things interleaved that will take some time to abstract away nicely and expose in network config for developers. For now this is an internal change even though data structures are technically public and require major version bump.
mordamax
pushed a commit
to paritytech-stg/polkadot-sdk
that referenced
this pull request
Sep 10, 2024
This is a step towards paritytech#5333 The commits with code moving (but no other changes) and actual changes are separated for easier review. Essentially this results in `SyncingStrategy` trait replacing struct (which is renamed to `PolkadotSyncingStrategy`, open for better name suggestions). Technically it is already possible to replace `PolkadotSyncingStrategy<B, Client>` with `Box<dyn SyncingStrategy<B>` in syncing engine, but I decided to postpone such change until we actually have an ability to customize it. It should also be possible to swap `PolkadotSyncingStrategy` with just `ChainSync` that only supports regular full sync from genesis (it also implements `SyncingStrategy` trait). While extracted trait still has a lot of non-generic stuff in it like exposed knowledge of warp sync and `StrategyKey` with hardcoded set of options, I believe this is a step in the right direction and will address those in upcoming PRs. With paritytech#5431 that landed earlier warp sync configuration is more straightforward, but there are still numerous things interleaved that will take some time to abstract away nicely and expose in network config for developers. For now this is an internal change even though data structures are technically public and require major version bump.
mordamax
pushed a commit
to paritytech-stg/polkadot-sdk
that referenced
this pull request
Sep 10, 2024
This is a step towards paritytech#5333 The commits with code moving (but no other changes) and actual changes are separated for easier review. Essentially this results in `SyncingStrategy` trait replacing struct (which is renamed to `PolkadotSyncingStrategy`, open for better name suggestions). Technically it is already possible to replace `PolkadotSyncingStrategy<B, Client>` with `Box<dyn SyncingStrategy<B>` in syncing engine, but I decided to postpone such change until we actually have an ability to customize it. It should also be possible to swap `PolkadotSyncingStrategy` with just `ChainSync` that only supports regular full sync from genesis (it also implements `SyncingStrategy` trait). While extracted trait still has a lot of non-generic stuff in it like exposed knowledge of warp sync and `StrategyKey` with hardcoded set of options, I believe this is a step in the right direction and will address those in upcoming PRs. With paritytech#5431 that landed earlier warp sync configuration is more straightforward, but there are still numerous things interleaved that will take some time to abstract away nicely and expose in network config for developers. For now this is an internal change even though data structures are technically public and require major version bump.
mordamax
pushed a commit
to paritytech-stg/polkadot-sdk
that referenced
this pull request
Sep 10, 2024
This is a step towards paritytech#5333 The commits with code moving (but no other changes) and actual changes are separated for easier review. Essentially this results in `SyncingStrategy` trait replacing struct (which is renamed to `PolkadotSyncingStrategy`, open for better name suggestions). Technically it is already possible to replace `PolkadotSyncingStrategy<B, Client>` with `Box<dyn SyncingStrategy<B>` in syncing engine, but I decided to postpone such change until we actually have an ability to customize it. It should also be possible to swap `PolkadotSyncingStrategy` with just `ChainSync` that only supports regular full sync from genesis (it also implements `SyncingStrategy` trait). While extracted trait still has a lot of non-generic stuff in it like exposed knowledge of warp sync and `StrategyKey` with hardcoded set of options, I believe this is a step in the right direction and will address those in upcoming PRs. With paritytech#5431 that landed earlier warp sync configuration is more straightforward, but there are still numerous things interleaved that will take some time to abstract away nicely and expose in network config for developers. For now this is an internal change even though data structures are technically public and require major version bump.
mordamax
pushed a commit
to paritytech-stg/polkadot-sdk
that referenced
this pull request
Sep 11, 2024
This is a step towards paritytech#5333 The commits with code moving (but no other changes) and actual changes are separated for easier review. Essentially this results in `SyncingStrategy` trait replacing struct (which is renamed to `PolkadotSyncingStrategy`, open for better name suggestions). Technically it is already possible to replace `PolkadotSyncingStrategy<B, Client>` with `Box<dyn SyncingStrategy<B>` in syncing engine, but I decided to postpone such change until we actually have an ability to customize it. It should also be possible to swap `PolkadotSyncingStrategy` with just `ChainSync` that only supports regular full sync from genesis (it also implements `SyncingStrategy` trait). While extracted trait still has a lot of non-generic stuff in it like exposed knowledge of warp sync and `StrategyKey` with hardcoded set of options, I believe this is a step in the right direction and will address those in upcoming PRs. With paritytech#5431 that landed earlier warp sync configuration is more straightforward, but there are still numerous things interleaved that will take some time to abstract away nicely and expose in network config for developers. For now this is an internal change even though data structures are technically public and require major version bump.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 17, 2024
# 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_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)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a step towards #5333
The commits with code moving (but no other changes) and actual changes are separated for easier review.
Essentially this results in
SyncingStrategytrait replacing struct (which is renamed toPolkadotSyncingStrategy, open for better name suggestions). Technically it is already possible to replacePolkadotSyncingStrategy<B, Client>withBox<dyn SyncingStrategy<B>in syncing engine, but I decided to postpone such change until we actually have an ability to customize it. It should also be possible to swapPolkadotSyncingStrategywith justChainSyncthat only supports regular full sync from genesis (it also implementsSyncingStrategytrait).While extracted trait still has a lot of non-generic stuff in it like exposed knowledge of warp sync and
StrategyKeywith hardcoded set of options, I believe this is a step in the right direction and will address those in upcoming PRs.With #5431 that landed earlier warp sync configuration is more straightforward, but there are still numerous things interleaved that will take some time to abstract away nicely and expose in network config for developers. For now this is an internal change even though data structures are technically public and require major version bump.