Extract warp sync strategy from ChainSync#2467
Conversation
altonen
left a comment
altonen
left a comment
There was a problem hiding this comment.
Very nice work! I left a bunch of comments but apart from state sync still partly being in ChainSync and the inability to share BlocksCollection between strategies (which can be addressed in a follow-up), I think it's looking good
| let warp_sync_progress = self.gap_sync.as_ref().map(|gap_sync| WarpSyncProgress { | ||
| phase: WarpSyncPhase::DownloadingBlocks(gap_sync.best_queued_number), | ||
| total_bytes: 0, | ||
| }); |
There was a problem hiding this comment.
I don't think this belongs here but I understand that in practice it might be harder to refactor out. We need to find a way to share BlocksCollection/peer download states and share them between strategies. This would be needed to run Sync 1.0 and 2.0 in parallel.
There was a problem hiding this comment.
No sharing of BlockCollection yet as it's not needed to run GapSync in parallel with ChainSync (it has its own instance of BlockCollection), but here is a draft PR with the initial implementation of sharing of peers between the strategies: #2814.
I don't think time should be invested now into reviewing it as a lot could change, but hopefully it shows the general idea of what a follow-up PR may look like.
Co-authored-by: Aaro Altonen <[email protected]>
Another thing that should be shared between the strategies running in parallel similarly to |
I don't think it's a good idea. All strategies should have the same view of connected peers. We can't have two simultaneous in-flight request to any peer so their states have to be synchronized across strategies. If we introduce some kind of peer + blocks collection which is shared between strategies using |
| #[must_use] | ||
| pub fn actions(&mut self) -> Box<dyn Iterator<Item = SyncingAction<B>>> { | ||
| match self { | ||
| SyncingStrategy::WarpSyncStrategy(strategy) => |
There was a problem hiding this comment.
Nit: Just for readability, maybe a good place for an Into implementation? But just a matter of taste.
There was a problem hiding this comment.
Good point. I'll do it in a follow-up PR #2814 to not complicate rebasing though)
|
lgtm, left some nits. |
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
| use warp::{EncodedProof, WarpProofRequest, WarpSync, WarpSyncAction, WarpSyncConfig}; | ||
|
|
||
| /// Corresponding `ChainSync` mode. | ||
| fn chain_sync_mode(sync_mode: SyncMode) -> ChainSyncMode { |
There was a problem hiding this comment.
Is this function really necessary? It reverted paritytech/substrate#14465 and the fact that there are two types again makes it painful to backport paritytech/substrate#14482 (whose goal is to allow combining Substrate's sync with a custom sync mechanism we have in Subspace).
Can this be replaced with a method on SyncMode that returns true for both SyncMode::Full and SyncMode::Warp? Or match enum against two possible variants explicitly.
There was a problem hiding this comment.
What this and upcoming PRs aim to achieve is to clearly separate syncing strategies like Warp, State, ChainSync, etc. Introducing an atomic variable accessible by ChainSync that refers to warp sync would be going into the opposite direction.
On the other hand, once the refactoring is over, it should be possible to plug custom sync as a separate strategy and switch to ChainSync only when the initial sync is over, without the need to pause ChainSync. May be this will suite you?
There was a problem hiding this comment.
Yes, that sounds like exactly what I need. Not sure if it will be flexible enough, but definitely sounds promising.
Extract `WarpSync` (and `StateSync` as part of warp sync) from `ChainSync` as independent syncing strategy called by `SyncingEngine`. Introduce `SyncingStrategy` enum as a proxy between `SyncingEngine` and specific syncing strategies. ## Limitations Gap sync is kept in `ChainSync` for now because it shares the same set of peers as block syncing implementation in `ChainSync`. Extraction of a common context responsible for peer management in syncing strategies able to run in parallel is planned for a follow-up PR. ## Further improvements A possibility of conversion of `SyncingStartegy` into a trait should be evaluated. The main stopper for this is that different strategies need to communicate different actions to `SyncingEngine` and respond to different events / provide different APIs (e.g., requesting justifications is only possible via `ChainSync` and not through `WarpSync`; `SendWarpProofRequest` action is only relevant to `WarpSync`, etc.) --------- Co-authored-by: Aaro Altonen <[email protected]>
6 participants
Extract
WarpSync(andStateSyncas part of warp sync) fromChainSyncas independent syncing strategy called bySyncingEngine. IntroduceSyncingStrategyenum as a proxy betweenSyncingEngineand specific syncing strategies.Limitations
Gap sync is kept in
ChainSyncfor now because it shares the same set of peers as block syncing implementation inChainSync. Extraction of a common context responsible for peer management in syncing strategies able to run in parallel is planned for a follow-up PR.Further improvements
A possibility of conversion of
SyncingStartegyinto a trait should be evaluated. The main stopper for this is that different strategies need to communicate different actions toSyncingEngineand respond to different events / provide different APIs (e.g., requesting justifications is only possible viaChainSyncand not throughWarpSync;SendWarpProofRequestaction is only relevant toWarpSync, etc.)Builds upon #2180.