Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Syncing strategy refactoring #5469

Merged

Conversation

nazar-pc
Copy link
Contributor

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 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 #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.

@dmitry-markin dmitry-markin added the T0-node This PR/Issue is related to the topic “node”. label Aug 29, 2024
Copy link
Contributor

@dmitry-markin dmitry-markin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for excellent per-commit review experience!

@dmitry-markin dmitry-markin requested a review from a team August 29, 2024 14:39
@nazar-pc
Copy link
Contributor Author

nazar-pc commented Sep 9, 2024

@lexnv 🙏

…refactoring

# Conflicts:
#	substrate/client/network/sync/src/strategy/chain_sync.rs
@nazar-pc
Copy link
Contributor Author

Resolved merge conflict after #5635

Copy link
Contributor

@lexnv lexnv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Nice job here 🙏

@dmitry-markin dmitry-markin added this pull request to the merge queue Sep 10, 2024
Merged via the queue into paritytech:master with commit 1f1f20a Sep 10, 2024
203 of 205 checks passed
@nazar-pc nazar-pc deleted the syncing-strategy-refactoring branch September 10, 2024 15:08
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 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)
nazar-pc added a commit to autonomys/polkadot-sdk that referenced this pull request Sep 27, 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.

(cherry picked from commit 1f1f20a)
nazar-pc added a commit to autonomys/polkadot-sdk that referenced this pull request Sep 27, 2024
# 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants