Move syncing code from `sc-network-common` to `sc-network-sync` by dmitry-markin · Pull Request #1912 · paritytech/polkadot-sdk

dmitry-markin · 2023-10-17T12:42:05Z

This PR moves syncing-related code from sc-network-common to sc-network-sync.

Unfortunately, some parts are tightly integrated with networking, so they were left in sc-network-common for now:

SyncMode in common/src/sync.rs (used in NetworkConfiguration).
BlockAnnouncesHandshake, BlockRequest, BlockResponse, etc. in common/src/sync/message.rs (used in src/protocol.rs and src/protocol/message.rs).

More substantial refactoring is needed to decouple syncing and networking completely, including getting rid of the hardcoded sync protocol.

Release notes

Move syncing-related code from sc-network-common to sc-network-sync. Delete ChainSync trait as it's never used (the only implementation is accessed directly from SyncingEngine and exposes a lot of public methods that are not part of the trait). Some new trait(s) for syncing will likely be introduced as part of Sync 2.0 refactoring to represent syncing strategies.

Builds upon #1736.

altonen

I don't think this should be silent. There are at least two external teams that are working on their own syncing implementations so this will likely affect them. At least the removal of ChainSync is something that should be mentioned, with a note that there will be some kind of new ChainSync trait in the near future.

bkchr

Looking good. Just a little bit confused why some types are being made public?

bkchr · 2023-10-29T18:04:45Z

substrate/client/network/sync/src/lib.rs

 /// Action that [`engine::SyncingEngine`] should perform after reporting imported blocks with
 /// [`ChainSync::on_blocks_processed`].
-enum BlockRequestAction<B: BlockT> {
+pub enum BlockRequestAction<B: BlockT> {


Why do we need to make this public?

It's consumed by SyncingEngine, that owns ChainSync.

But isn't this in the same crate?

Technically, SyncingEngine is in the same root module as ChainSync, because ChainSync is defined in lib.rs. So pub is not needed at all (even as pub(crate)). Semantically, these types are public for SyncingEngine, so I feel inclined to highlight this with either pub or pub(crate).

pub(crate) is probably a better option, but may be it's also worth moving ChainSync to a separate chain_sync.rs file.

@bkchr would it be better to move ChainSync and its types to a separate file and use pub(crate) for types consumed by SyncingEngine?

@bkchr would it be better to move ChainSync and its types to a separate file and use pub(crate) for types consumed by SyncingEngine?

Done. PR looks scary now due to moved code, but overall situation with ChainSync should have improved.

@bkchr @altonen Could you have a look if moving ChainSync to a separate file worth it: 2511cfc?

pub(crate) is probably a better option, but may be it's also worth moving ChainSync to a separate chain_sync.rs file.

Yeah sorry for these dumb questions :P I should have realized that the module is private to the crate. Pub is fine by me here.

I was thinking this reordering could've been done in another PR but whatever. SyncingEngine should be moved to sync/src/lib.rs but that can be done in another PR.

Why SyncingEngine should be moved to the root module and not kept in it's own module, may be reexporting it in the root module?

bkchr · 2023-10-29T18:07:18Z

with a note that there will be some kind of new ChainSync trait in the near future.

What is the status of completely getting rid off sync in the networking crate? Because ideally there should not exist a chain sync trait at all.

dmitry-markin · 2023-10-30T08:32:56Z

with a note that there will be some kind of new ChainSync trait in the near future.

What is the status of completely getting rid off sync in the networking crate? Because ideally there should not exist a chain sync trait at all.

There won't be ChainSync trait in networking for sure. There may be some new traits for syncing in sc-network-sync though.

bkchr · 2023-10-30T08:40:24Z

There may be some new traits for syncing in sc-network-sync though.

Do we really need to make sync itself generic? I don't think so? As this is some self contained implementation.

altonen · 2023-10-30T08:44:46Z

There may be some new traits for syncing in sc-network-sync though.

Do we really need to make sync itself generic? I don't think so? As this is some self contained implementation.

That is not the current plan at least. We are most likely going to introduce some traits for the strategies that we discussed earlier, simply to make the code easier to maintain and plug additional syncing implementations without major headache.

bkchr · 2023-10-30T09:10:58Z

Okay :)

bkchr

Moving to its own file sounds reasonable!

bkchr · 2023-10-30T16:11:15Z

substrate/client/network/sync/src/chain_sync.rs

+}
+
+#[cfg(test)]
+mod test {


If you are at it, why not also move them into their own file?

…tytech#1912) This PR moves syncing-related code from `sc-network-common` to `sc-network-sync`. Unfortunately, some parts are tightly integrated with networking, so they were left in `sc-network-common` for now: 1. `SyncMode` in `common/src/sync.rs` (used in `NetworkConfiguration`). 2. `BlockAnnouncesHandshake`, `BlockRequest`, `BlockResponse`, etc. in `common/src/sync/message.rs` (used in `src/protocol.rs` and `src/protocol/message.rs`). More substantial refactoring is needed to decouple syncing and networking completely, including getting rid of the hardcoded sync protocol. ## Release notes Move syncing-related code from `sc-network-common` to `sc-network-sync`. Delete `ChainSync` trait as it's never used (the only implementation is accessed directly from `SyncingEngine` and exposes a lot of public methods that are not part of the trait). Some new trait(s) for syncing will likely be introduced as part of Sync 2.0 refactoring to represent syncing strategies.

Try to move syncing stuff from sc-network-common to sc-network-sync

b93fb1a

dmitry-markin added R0-no-crate-publish-required The change does not require any crates to be re-published. T0-node This PR/Issue is related to the topic “node”. labels Oct 17, 2023

dmitry-markin requested a review from altonen October 17, 2023 12:46

dmitry-markin added 6 commits October 18, 2023 15:19

Get rid of ChainSync trait

cad9895

Update visibility and rustdocs

a4fa0d7

Merge remote-tracking branch 'origin/master' into dm-get-rid-common-sync

cf62530

minor: comment

6097fee

minor: grammar

888f35c

minor: rustdoc

080883e

dmitry-markin changed the title ~~[draft] Move syncing code from sc-network-common to sc-network-sync~~ Move syncing code from sc-network-common to sc-network-sync Oct 18, 2023

dmitry-markin marked this pull request as ready for review October 18, 2023 14:26

dmitry-markin requested a review from andresilva as a code owner October 18, 2023 14:26

altonen approved these changes Oct 20, 2023

View reviewed changes

dmitry-markin removed the R0-no-crate-publish-required The change does not require any crates to be re-published. label Oct 20, 2023

dmitry-markin requested a review from bkchr October 23, 2023 14:29

bkchr approved these changes Oct 29, 2023

View reviewed changes

dmitry-markin added 2 commits October 30, 2023 15:45

Merge remote-tracking branch 'origin/master' into dm-get-rid-common-sync

250020b

Move ChainSync to separate file, update visibility

2511cfc

bkchr approved these changes Oct 30, 2023

View reviewed changes

dmitry-markin added 4 commits October 31, 2023 10:57

Make ChainSync public

ecdeb2c

Move ChainSync tests to separate file

d3b291a

Merge remote-tracking branch 'origin/master' into dm-get-rid-common-sync

8bfe0fa

Merge remote-tracking branch 'origin/master' into dm-get-rid-common-sync

62561fa

dmitry-markin merged commit 1cd6acd into master Nov 1, 2023

dmitry-markin deleted the dm-get-rid-common-sync branch November 1, 2023 13:10

github-actions bot mentioned this pull request Feb 19, 2024

Update substrate/polkadot/cumulus from v1.3.0 to v1.6.0 moondance-labs/tanssi#419

Closed

Ali-Usama mentioned this pull request Mar 4, 2024

Why is ExtendedPeerInfo private in release-polkadot-v1.6.0? #3556

Closed

github-actions bot mentioned this pull request Mar 13, 2024

Update polkadot-sdk from v1.3.0 to v1.7.2 moonbeam-foundation/moonbeam#2703

Closed

dmitry-markin mentioned this pull request Nov 27, 2024

Convert SyncingEngine::run to use tokio::select! instead of polling #2132

Merged

Conversation

dmitry-markin commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release notes

Uh oh!

altonen left a comment

Choose a reason for hiding this comment

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitry-markin Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkchr commented Oct 29, 2023

Uh oh!

dmitry-markin commented Oct 30, 2023

Uh oh!

bkchr commented Oct 30, 2023

Uh oh!

altonen commented Oct 30, 2023

Uh oh!

bkchr commented Oct 30, 2023

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dmitry-markin commented Oct 17, 2023 •

edited

Loading

dmitry-markin Oct 30, 2023 •

edited

Loading