Ability to pause Substrate sync on the fly

[nazar-pc]

This builds on https://github.com/paritytech/substrate/pull/14465 with the goal to be able to pause Substrate's built-in sync mechanism.

There is not that much flexibility to Substrate's sync right now, so this was my next best idea that is relatively simple.

In Subspace we have a separate sync mechanism for archival history that leverages distributed storage network of our protocol and Substrate's sync is only used to sync the tip of the chain.

The issue is that due to nodes being pruned, requests done by Substrate's sync mechanism are doomed to fail and result in frequent disconnections and decrease of peer reputation. We avoid that by pausing Substrate sync completely until we are close enough to the tip for Substrate sync to succeed.

Here is an example of how it is used in our protocol right now (not enabled by default for now, but we're working on it): https://github.com/subspace/subspace/pull/1602/files?file-filters%5B%5D=.rs&show-viewed-files=true#diff-8d08a90676fed94fe41260405e5c5ef3bba7f1774bfbc252c8329dd25d2f75f2R194-R214

[altonen]

Why is it necessary to upstream this to Substrate?

[nazar-pc]

Why is it necessary to upstream this to Substrate?

This is a fundamental concept that would require us to maintain a fork downstream. Every time we find that Substrate is not flexible enough we're trying to contribute towards making Substrate better upstream such that we have one less reason to fork it.

[bkchr]

Yeah, we should not upstream this. Can you not just create a wrapper around the normal sync engine and in this wrapper you add support for pausing it?

[nazar-pc]

Yeah, we should not upstream this. Can you not just create a wrapper around the normal sync engine and in this wrapper you add support for pausing it?

I'm afraid it is not modular enough even remotely to achieve this. ChainSync is public, but it is constructed unconditionally and internally in SyncingEngine and relevant methods on ChainSync are private anyway (just called from fn poll).

[altonen]

But this PR is basically asking us to maintain a feature we don't need so that a downstream project doesn't have to maintain a fork. I understand that the syncing refactoring is something that people are looking forward to but sadly there are other projects that have higher priority right now and at this time I'd refrain from adding any new features to sc-network-sync until the refactoring is done.

[bkchr]

I'm afraid it is not modular enough even remotely to achieve this. ChainSync is public, but it is constructed unconditionally and internally in SyncingEngine and relevant methods on ChainSync are private anyway (just called from fn poll).

Yeah, I was just answering from my head. Now, after looking into the code, you should be able to wrap SyncingService as this only implements public traits and is the communication between the networking and the engine.

[nazar-pc]

Now, after looking into the code, you should be able to wrap SyncingService as this only implements public traits and is the communication between the networking and the engine.

The issue is that ChainSync initiates those outgoing requests internally. I want it to not do that and don't see how wrapping of SyncingService would help.

Either way, I don't necessarily expect 100% of PRs to land upstream. If this is not welcome, consider it a feature request with real-world example of what people need/want to be able to do with Substrate, maybe it'll have influence on sync refactoring somehow.

[bkchr]

You could also "mock" the other side: https://github.com/paritytech/substrate/blob/3c71aa160b8ee594aecf924c66dcadbb8b8c8cdc/client/network/sync/src/lib.rs#L1403

The syncing would probably then start disconnecting these other nodes etc.

But yeah ty for the input, I will close this pr for now.