Skip to content
Snippets Groups Projects
  • Nazar Mokrynskyi's avatar
    Syncing strategy refactoring (part 3) (#5737) · 12d90524
    Nazar Mokrynskyi authored
    # Description
    
    This is a continuation of
    https://github.com/paritytech/polkadot-sdk/pull/5666 that finally fixes
    https://github.com/paritytech/polkadot-sdk/issues/5333.
    
    This should allow developers to create custom syncing strategies or even
    the whole syncing engine if they so desire. It also moved syncing engine
    creation and addition of corresponding protocol outside
    `build_network_advanced` method, which is something Bastian expressed as
    desired in
    https://github.com/paritytech/polkadot-sdk/issues/5#issuecomment-1700816458
    
    Here I replaced strategy-specific types and methods in `SyncingStrategy`
    trait with generic ones. Specifically `SyncingAction` is now used by all
    strategies instead of strategy-specific types with conversions.
    `StrategyKey` was an enum with a fixed set of options and now replaced
    with an opaque type that strategies create privately and send to upper
    layers as an opaque type. Requests and responses are now handled in a
    generic way regardless of the strategy, which reduced and simplified
    strategy API.
    
    `PolkadotSyncingStrategy` now lives in its dedicated module (had to edit
    .gitignore for this) like other strategies.
    
    `build_network_advanced` takes generic `SyncingService` as an argument
    alongside with a few other low-level types (that can probably be
    extracted in the future as well) without any notion of specifics of the
    way syncing is actually done. All the protocol and tasks are created
    outside and not a part of the network anymore. It still adds a bunch of
    protocols like for light client and some others that should eventually
    be restructured making `build_network_advanced` just building generic
    network and not application-specific protocols handling.
    
    ## Integration
    
    Just like https://github.com/paritytech/polkadot-sdk/pull/5666
    introduced `build_polkadot_syncing_strategy`, this PR introduces
    `build_default_block_downloader`, but for convenience and to avoid
    typical boilerplate a simpler high-level function
    `build_default_syncing_engine` is added that will take care of creating
    typical block downloader, syncing strategy and syncing engine, which is
    what most users will be using going forward. `build_network` towards the
    end of the PR was renamed to `build_network_advanced` and
    `build_network`'s API was reverted to
    pre-https://github.com/paritytech/polkadot-sdk/pull/5666, so most users
    will not see much of a difference during upgrade unless they opt-in to
    use new API.
    
    ## Review Notes
    
    For `StrategyKey` I was thinking about using something like private type
    and then storing `TypeId` inside instead of a static string in it, let
    me know if that would preferred.
    
    The biggest change happened to requests that different strategies make
    and how their responses are handled. The most annoying thing here is
    that block response decoding, in contrast to all other responses, is
    dependent on request. This meant request had to be sent throughout the
    system. While originally `Response` was `Vec<u8>`, I didn't want to
    re-encode/decode request and response just to fit into that API, so I
    ended up with `Box<dyn Any + Send>`. This allows responses to be truly
    generic and each strategy will know how to downcast it back to the
    concrete type when handling the response.
    
    Import queue refactoring was needed to move `SyncingEngine` construction
    out of `build_network` that awkwardly implemented for `SyncingService`,
    but due to `&mut self` wasn't usable on `Arc<SyncingService>` for no
    good reason. `Arc<SyncingService>` itself is of course useless, but
    refactoring to replace it with just `SyncingService` was unfortunately
    rejected in https://github.com/paritytech/polkadot-sdk/pull/5454
    
    As usual I recommend to review this PR as a series of commits instead of
    as the final diff, it'll make more sense that way.
    
    # 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)
    Unverified
    12d90524
Code owners
Assign users and groups as approvers for specific file changes. Learn more.
.gitignore 578 B