Syncing strategy refactoring (part 3) #5737

nazar-pc · 2024-09-17T13:14:46Z

Description

This is a continuation of #5666 that finally fixes #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 #5 (comment)

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

My PR includes a detailed description as outlined in the "Description" and its two subsections above.
My PR follows the labeling requirements of this project (at minimum one label for T required)
- External contributors: ask maintainers to put the right label on your PR.
I have made corresponding changes to the documentation (if applicable)

…trategies

…n_warp_proof_response()` with `SyncingAction::SendGenericRequest` and `SyncingStrategy::on_generic_response()`

nazar-pc · 2024-09-17T13:17:16Z

@dmitry-markin @lexnv this is probably the last one in a series, I'm reasonably happy with the achieved result and will try to rebase our downstream changes on top of these new APIs without modifying Substrate itself.

nazar-pc · 2024-09-17T13:23:06Z

@ggwpez you commented on CI stuff before, so maybe you can help. Apparently Rust version used for formatting has changed, but since it just says nightly in the workflow and I find exact version used in .github/workflows/reusable-preflight.yml, I no longer know which version am I even supposed to use to do the formatting locally. I used nightly-2024-04-10 before, but apparently it is the wrong version now.

Request: can preflight workflow print the versions maybe for reference? It prints some stuff, but nothing actually useful IMO.

UPD: I was applying formatting in a different subdirectory, but the request is still relevant.

…gAction::StartRequest`

…mpl block

…ock_response()` with `SyncingAction::StartRequest` and `SyncingStrategy::on_generic_response()`

… we already effectively more common `Arc<dyn Link<B>>` in many places

…uild_default_syncing_engine` to make construction of the default syncing engine a bit easier

…_network` API before paritytech#5666

…sc_service::Configuration`

…refactoring-part-3 # Conflicts: # substrate/client/network/sync/src/engine.rs

nazar-pc · 2024-09-20T00:36:43Z

Addressed Bastian's comment from #5666 and resolved conflicts with master.

ggwpez · 2024-09-20T22:00:27Z

Yea am very much in favour of printing all versions at the beginning of CI jobs, (Rust, rustup, cargo, clippy, fmt, nextest) to easily debug stuff.
I can add some prints myself when i see it the next time, otherwise i think @AndWeHaveAPlan was working on this.

ggwpez · 2024-09-20T22:00:30Z

bot fmt

command-bot · 2024-09-20T22:00:35Z

@ggwpez https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7399653 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 6-6aed9a84-d20a-4977-aa73-1e1cdd7a7e1c to cancel this command or bot cancel to cancel all commands in this pull request.

nazar-pc · 2024-09-20T22:01:59Z

It is already all formatted properly, just took a few attempts 🙂

command-bot · 2024-09-20T22:04:04Z

@ggwpez Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7399653 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7399653/artifacts/download.

nazar-pc · 2024-09-26T07:24:37Z

Friendly ping here, would be nice to merge this before merge conflicts happen. It shouldn't be as scary as the diff might suggest.

dmitry-markin

The changes look good and indeed finally allow custom syncing strategies use with substrate. This is what was originally planned when separate syncing strategies were introduced. Thank you for implementing this!

templates/solochain/node/Cargo.toml

substrate/client/service/src/builder.rs

lexnv · 2024-10-21T13:51:25Z

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

 					}
+					if remove_obsolete {


Should we remove pending responses first, then check if our peers contain the peer_id?

I think we were removing the pending_responses before send_block_request checked if this is a known peer

Above branch is never ever supposed to happen (see debug_assert!(false)), so I don't think it makes any difference either way whether we remove pending responses before crashing or not.

Also removed redundant comment below in latest push.

lexnv · 2024-10-21T13:54:47Z

substrate/client/network/sync/src/strategy/state.rs

+				self.protocol_name.clone(),
+				request.encode_to_vec(),
+				tx,
+				IfDisconnected::ImmediateError,


dq: Should we try to establish connection instead of immediate errors?

Would you think it is beneficial for strategies to provide this option in their actions?

This is the same as it was before, so I didn't change the behavior here and so far there was no need to do this.

Maybe the assumption is that we only send requests to connected peers, so if someone disconnected we just error instead of attempting to dial them?

lexnv

LGTM! Thanks for contributing!

…: true`

…refactoring-part-3 # Conflicts: # substrate/client/network/sync/src/strategy/chain_sync.rs

nazar-pc

Also resolved a minor merge conflict in imports after #5686

nazar-pc · 2024-10-22T17:29:56Z

substrate/client/network/sync/src/strategy/state.rs

+				self.protocol_name.clone(),
+				request.encode_to_vec(),
+				tx,
+				IfDisconnected::ImmediateError,


This is the same as it was before, so I didn't change the behavior here and so far there was no need to do this.

Maybe the assumption is that we only send requests to connected peers, so if someone disconnected we just error instead of attempting to dial them?

nazar-pc · 2024-10-22T17:31:24Z

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

 					}
+					if remove_obsolete {


Above branch is never ever supposed to happen (see debug_assert!(false)), so I don't think it makes any difference either way whether we remove pending responses before crashing or not.

Also removed redundant comment below in latest push.

…refactoring-part-3

nazar-pc · 2024-11-05T05:18:31Z

Merged master and CI should pass now, let me know if there is anything else I can do to push this across the finish line

dmitry-markin · 2024-11-05T06:50:48Z

I will run some final tests, and, if everything fine, we can merge the PR.

nazar-pc · 2024-11-05T07:02:13Z

We're running a fork with these changes for over a month now without issues

EgorPopelyaev · 2024-11-06T16:28:33Z

@dmitry-markin How does this PR look like, is it fine to be merged?

dmitry-markin · 2024-11-07T08:01:25Z

@dmitry-markin How does this PR look like, is it fine to be merged?

I am testing it on Versi to make sure everything is good. Code-wise it is fine. In any case, let's stabilize it in master first skipping stable2412.

@nazar-pc you are using a fork anyway and not need it in the polkadot-sdk release, aren't you?

nazar-pc · 2024-11-07T08:24:35Z

I was really-really trying to upstream out patches wherever possible, it is kind of a pain to rebase everything on each release. I was hoping that opening PR in September will make it into December build, I'd be disappointed if it didn't. There isn't really anything crazy in this PR, mostly just moving things around, I'd say the risk of regressions is low and will be evident immediately.

We just launched Mainnet yesterday (already 1239 consensus nodes) with this included after more than a month of testnets, there was zero complaints so far about anything that would be potentially related to this for what it is worth.

dmitry-markin · 2024-11-07T08:42:38Z

Then I got it wrong, sorry @nazar-pc. Let's see what we can do regrading the December release.

nazar-pc · 2024-11-07T11:04:33Z

Thanks everyone!

nazar-pc added 5 commits September 17, 2024 15:25

Use SyncingAction everywhere instead of custom enums in different s…

0515f8f

…trategies

Make strategy key opaque instead of having a hardcoded list of variants

743161c

Rename SyncingConfig to PolkadotSyncingStrategyConfig

e393d30

Move PolkadotSyncingStrategy into its own module

6b5acc4

Replace SyncingAction::SendWarpProofRequest and `SyncingStrategy::o…

a312e09

…n_warp_proof_response()` with `SyncingAction::SendGenericRequest` and `SyncingStrategy::on_generic_response()`

nazar-pc force-pushed the syncing-strategy-refactoring-part-3 branch 2 times, most recently from 938b988 to 6012b67 Compare September 17, 2024 13:32

nazar-pc added 9 commits September 17, 2024 17:44

Replace state sync request/response handling with generic one

72908d3

Replace SyncingAction::SendGenericRequest with more generic `Syncin…

c727dc1

…gAction::StartRequest`

Move block downloader instantiation outside the network instantiation

6bf9b7f

Move ChainSync::on_block_response implementation out of the trait i…

76b2dcf

…mpl block

Pass protocol name down to generic response handler

a1a0863

Replace SyncingAction::SendBlockRequest and `SyncingStrategy::on_bl…

d2ab786

…ock_response()` with `SyncingAction::StartRequest` and `SyncingStrategy::on_generic_response()`

Improve import queue API, removing the need for Box<dyn Link<B>> if…

0ae9dc0

… we already effectively more common `Arc<dyn Link<B>>` in many places

Remove syncing engine construction from build_network and expose `b…

a26bbfa

…uild_default_syncing_engine` to make construction of the default syncing engine a bit easier

Add prdoc

53b34d0

nazar-pc force-pushed the syncing-strategy-refactoring-part-3 branch from 6012b67 to 53b34d0 Compare September 17, 2024 14:45

dmitry-markin added the T0-node This PR/Issue is related to the topic “node”. label Sep 17, 2024

dmitry-markin self-requested a review September 17, 2024 14:50

nazar-pc mentioned this pull request Sep 17, 2024

Syncing strategy refactoring (part 2) #5666

Merged

3 tasks

nazar-pc added 3 commits September 20, 2024 03:16

Rename build_network to build_network_advanced and restore `build…

0ac0ddf

…_network` API before paritytech#5666

Make build_network_advanced arguments lower-level and not require `…

8388a81

…sc_service::Configuration`

Merge remote-tracking branch 'upstream/master' into syncing-strategy-…

243af75

…refactoring-part-3 # Conflicts: # substrate/client/network/sync/src/engine.rs

Fix toml formatting

63dbf0f

nazar-pc added a commit to autonomys/polkadot-sdk that referenced this pull request Sep 27, 2024

Syncing strategy refactoring (part 3) (paritytech#5737)

17d9b40

nazar-pc mentioned this pull request Sep 30, 2024

Upgrade Substrate to stable2409 autonomys/subspace#3078

Merged

1 task

dmitry-markin approved these changes Oct 2, 2024

View reviewed changes

templates/solochain/node/Cargo.toml Outdated Show resolved Hide resolved

dmitry-markin reviewed Oct 2, 2024

View reviewed changes

templates/solochain/node/Cargo.toml Outdated Show resolved Hide resolved

dmitry-markin requested a review from bkchr October 2, 2024 17:14

dmitry-markin requested a review from skunert October 15, 2024 15:54

lexnv reviewed Oct 21, 2024

View reviewed changes

substrate/client/service/src/builder.rs Outdated Show resolved Hide resolved

lexnv reviewed Oct 21, 2024

View reviewed changes

lexnv approved these changes Oct 21, 2024

View reviewed changes

nazar-pc added 3 commits October 22, 2024 20:17

Change name of generics for consistency

7f4ce2c

Remove redundant comment (present in places that set `remove_obsolete…

87be088

…: true`

Merge remote-tracking branch 'upstream/master' into syncing-strategy-…

27ee43c

…refactoring-part-3 # Conflicts: # substrate/client/network/sync/src/strategy/chain_sync.rs

nazar-pc commented Oct 22, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/master' into syncing-strategy-…

dfa87f4

…refactoring-part-3

dmitry-markin added this pull request to the merge queue Nov 7, 2024

Merged via the queue into paritytech:master with commit 12d9052 Nov 7, 2024
194 of 196 checks passed

nazar-pc deleted the syncing-strategy-refactoring-part-3 branch November 7, 2024 11:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Syncing strategy refactoring (part 3) #5737

Syncing strategy refactoring (part 3) #5737

nazar-pc commented Sep 17, 2024 •

edited

Loading

nazar-pc commented Sep 17, 2024

nazar-pc commented Sep 17, 2024 •

edited

Loading

nazar-pc commented Sep 20, 2024

ggwpez commented Sep 20, 2024

ggwpez commented Sep 20, 2024

command-bot bot commented Sep 20, 2024 •

edited

Loading

nazar-pc commented Sep 20, 2024

command-bot bot commented Sep 20, 2024

nazar-pc commented Sep 26, 2024

dmitry-markin left a comment

lexnv Oct 21, 2024

nazar-pc Oct 22, 2024

lexnv Oct 21, 2024

nazar-pc Oct 22, 2024

lexnv left a comment

nazar-pc left a comment

nazar-pc Oct 22, 2024

nazar-pc Oct 22, 2024

nazar-pc commented Nov 5, 2024

dmitry-markin commented Nov 5, 2024

nazar-pc commented Nov 5, 2024

EgorPopelyaev commented Nov 6, 2024

dmitry-markin commented Nov 7, 2024 •

edited

Loading

nazar-pc commented Nov 7, 2024

dmitry-markin commented Nov 7, 2024

nazar-pc commented Nov 7, 2024

Syncing strategy refactoring (part 3) #5737

Syncing strategy refactoring (part 3) #5737

Conversation

nazar-pc commented Sep 17, 2024 • edited Loading

Description

Integration

Review Notes

Checklist

nazar-pc commented Sep 17, 2024

nazar-pc commented Sep 17, 2024 • edited Loading

nazar-pc commented Sep 20, 2024

ggwpez commented Sep 20, 2024

ggwpez commented Sep 20, 2024

command-bot bot commented Sep 20, 2024 • edited Loading

nazar-pc commented Sep 20, 2024

command-bot bot commented Sep 20, 2024

nazar-pc commented Sep 26, 2024

dmitry-markin left a comment

Choose a reason for hiding this comment

lexnv Oct 21, 2024

Choose a reason for hiding this comment

nazar-pc Oct 22, 2024

Choose a reason for hiding this comment

lexnv Oct 21, 2024

Choose a reason for hiding this comment

nazar-pc Oct 22, 2024

Choose a reason for hiding this comment

lexnv left a comment

Choose a reason for hiding this comment

nazar-pc left a comment

Choose a reason for hiding this comment

nazar-pc Oct 22, 2024

Choose a reason for hiding this comment

nazar-pc Oct 22, 2024

Choose a reason for hiding this comment

nazar-pc commented Nov 5, 2024

dmitry-markin commented Nov 5, 2024

nazar-pc commented Nov 5, 2024

EgorPopelyaev commented Nov 6, 2024

dmitry-markin commented Nov 7, 2024 • edited Loading

nazar-pc commented Nov 7, 2024

dmitry-markin commented Nov 7, 2024

nazar-pc commented Nov 7, 2024

nazar-pc commented Sep 17, 2024 •

edited

Loading

nazar-pc commented Sep 17, 2024 •

edited

Loading

command-bot bot commented Sep 20, 2024 •

edited

Loading

dmitry-markin commented Nov 7, 2024 •

edited

Loading