Full domain chain snap sync (v2). #3196

shamil-gadelshin · 2024-10-30T15:29:53Z

This PR introduces snap sync for the domain chain (#3026).

It supersedes the previous PR with squashed review commits into the original code to enable clean review (requested by @nazar-pc here)

The main changes from the original PR:

the last confirmed domain block execution receipt protocol was moved to domain network from consensus network
the last confirmed domain block execution receipt sync algorithm was changed to support possible change due to network delays
SubspaceSyncOracle was modified to support domain snap sync
SnapSyncOrchestrator was simplified significantly
consensus and domain chains synchronization was refactored from SnapSyncOrchestrator to block import notifications

Code contributor checklist:

I have read, understood and followed contributing guide

teor2345

Can you merge the main branch into this branch and resolve conflicts?
Otherwise we’re going to have to review this PR again before it merges.

(Anyone who wants to compare with the original PR, or review without the merge, can do that up to the last current commit, which is 21ee3e0.)

nazar-pc · 2024-10-31T01:04:34Z

Better rebase since no one reviewed it yet

- add custom `build-network` method - add last domain block execution receipt protocol (moved from consensus chain)

teor2345

Looks good to me. I also confirmed it’s essentially the same as your previous PR, here’s the compare view:
full-domain-snap-sync2...full-domain-snap-sync

Sorry to bring this up late in the process, but there’s no test code or test changes here. I understand that syncing code can be hard to test using unit tests. What’s the test plan for this feature?

Oh and it looks like there’s a minor error handling conflict, can you resolve that using a merge?

teor2345 · 2024-10-31T20:57:13Z

domains/service/src/network.rs

+            client.clone(),
+            num_peer_hint,
+        );
+        spawn_handle.spawn(


Why aren’t any of the tasks in this function essential?

It's a copy of build_network() code from Substrate. The logic seems to be that any network protocol task failure shouldn't bring the whole process down.

Ok, so I think I’m missing some context here.

What’s the impact on Subspace if each of these tasks fails?

last-domain-execution-receipt-request-handler

block-request-handler

syncing

Let’s assume a failure on most or all of the nodes on the network, because that could easily happen in the logs and we wouldn’t know about it. (Because the node won’t go down, and most people don’t read logs.)

nazar-pc

This will require more work to structure it in a more streamlined way.
There is still too much interleaving and coupling between components that should not know about each other existence.
From consensus knowing about domain sync to domain operator depending on the whole consensus chain service.

I see you have updated MMR stuff, that should have been one PR.
Then I see you want to reuse some sync implementation components from consensus in domains, the first step there would be to move those components into their own crate (long overdue, something like sc-subspace-sync, probably one PR) and then probably moving some generic stuff (like sc-subspace-sync-common, either second PR or together with the previous one).
I understand that simply making things public where they are and adding a dependency is quick and easy, but with this approach we will quickly get into recursive dependencies and there should be some reasonable hierarchy to dependencies.

After sync is a separate crate that doesn't depend on quite literally the whole subspace node implementation (minus CLI), this PR can be rebased, then we can get rid of domain_snap_sync_finished, SnapSyncOrchestrator, ConsensusChainSyncParams and I think it will be okay on consensus side.

nazar-pc · 2024-10-31T20:56:31Z

crates/sc-consensus-subspace/src/slot_worker.rs

@@ -106,6 +107,11 @@ where
        // (default state), it also accounts for DSN sync
        (!self.force_authoring && self.inner.is_major_syncing())
            || self.pause_sync.load(Ordering::Acquire)
+            || self


This oracle is about consensus chain sync. It shouldn't know or care about domain sync state.

Hmm, I was under impression that you suggested the very same idea as Ning which I implemented. What did I miss?

My understanding was that this was needed in domains, in which case you create a domain sync oracle with necessary custom logic there, but consensus sync oracle doesn't need to be modified

crates/subspace-node/src/commands/run.rs

nazar-pc · 2024-10-31T21:03:48Z

crates/subspace-service/src/domains/snap_sync_orchestrator.rs

+use tracing::debug;
+
+/// Synchronizes consensus and domain chain snap sync.
+pub struct SnapSyncOrchestrator {


I thought we agreed that this orchestrator is not needed, yet it still exists. It should be removed as unnecessary.

We agreed that we need to provide only two shared variables an atomic bool for the sync oracle and a receiver for the target block. The rest was removed. The current orchestrator is just a container for those variables that are packed together most of the time. It is just more convenient to pass those variables in the same struct even if it is possible to remove it completely.

There is also a channel in it. I would have expected to remove this orchestrator completely, have some config data structure in domains if necessary and generally not have methods like unblock_consensus_snap_sync that need to be called in strategic moments.

nazar-pc · 2024-10-31T21:05:16Z

domains/service/Cargo.toml

@@ -63,8 +65,10 @@ sp-subspace-mmr = { version = "0.1.0", path = "../../crates/sp-subspace-mmr" }
 sp-transaction-pool = { git = "https://github.com/subspace/polkadot-sdk", rev = "5871818e1d736f1843eb9078f886290695165c42" }
 subspace-core-primitives = { version = "0.1.0", path = "../../crates/subspace-core-primitives" }
 subspace-runtime-primitives = { version = "0.1.0", path = "../../crates/subspace-runtime-primitives" }
+subspace-service = { version = "0.1.0", path = "../../crates/subspace-service" }


This is very problematic. You add a gigantic crate as a dependency to another gigantic crate. And the only purpose is to access ConsensusChainSyncParams, which most likely doesn't belong to subspace_service in the first place.

I can get rid of this dependency by moving the struct. However, I would need to add sc-consensus-subspace dependency to domain-operator (block import streams dependency in the struct). Is it better?

Not really, domains should not rely on Subspace consensus code either

crates/subspace-node/src/commands/run.rs

nazar-pc · 2024-10-31T21:09:01Z

domains/client/domain-operator/Cargo.toml

@@ -39,6 +44,7 @@ sp-trie = { git = "https://github.com/subspace/polkadot-sdk", rev = "5871818e1d7
 sp-weights = { git = "https://github.com/subspace/polkadot-sdk", rev = "5871818e1d736f1843eb9078f886290695165c42" }
 subspace-core-primitives = { version = "0.1.0", path = "../../../crates/subspace-core-primitives" }
 subspace-runtime-primitives = { version = "0.1.0", path = "../../../crates/subspace-runtime-primitives" }
+subspace-service = { version = "0.1.0", default-features = false, path = "../../../crates/subspace-service" }


Ouch, this is way more problematic than the other one. This is a gigantic dependency added to a lower-level crate, which will significantly worsen compile times and will serialize compilation order even more.

domain-operator hosts domain snap sync feature it requires both mmr_sync and consensus snap sync features from subspace_service. There are at least two ways to mitigate the existing dependency: either create a separate crate for sync features (both subspace_service and domain_operator will depend on it) or introduce several traits and wrapping structures to pass incapsulated functions to domains (domain_operator contains traits which are used by subspace_node to pass wrapping structures to domains). Either way, it seems more complicated than keeping an existing dependency. Do you see a simpler way or do you think the new complexity is justified from a dependency graph perspective?

I certainly expect a separate crate (or several) for sync, we'll have even more code once we upgrade our sync implementation to implement custom SyncStrategy after latest Substrate upgrade. But ideally operator wouldn't depend on consensus sync either, but rather on some shared crate with common primitives/utilities.

nazar-pc · 2024-10-31T21:15:24Z

crates/subspace-service/src/domains.rs


-impl<Block, Client, NR> LastDomainBlockInfoReceiver<Block, Client, NR>
+/// Provides parameters for domain snap sync synchronization with the consensus chain snap sync.
+pub struct ConsensusChainSyncParams<Block, CNR>


Everything included in this struct is already public and can be accessed in subspace-node, this struct should not exist in subspace-service, it doesn't belong here.

I answered above: the question remains about the dependency for block import notification stream.

Stream is just a stream. Domains already use some of the streams that are derived from consensus logic, yet they don't depend (I hope) on consensus crate for this.

shamil-gadelshin · 2024-11-01T10:55:24Z

@teor2345

Looks good to me. I also confirmed it’s essentially the same as your previous PR, here’s the compare view:
full-domain-snap-sync2...full-domain-snap-sync

I don't think the comparison is correct, for example: SubspaceSyncOracle is the same in both branches but it showed as different.

Sorry to bring this up late in the process, but there’s no test code or test changes here. I understand that syncing code can be hard to test using unit tests. What’s the test plan for this feature?
Oh and it looks like there’s a minor error handling conflict, can you resolve that using a merge?

We test such code manually and this is the reason why I will merge the main in the end - the testing stand requires a non-trivial setup each time.

teor2345

Maybe these tasks should be essential?

teor2345 · 2024-11-03T21:44:51Z

domains/service/src/network.rs

+            client.clone(),
+            num_peer_hint,
+        );
+        spawn_handle.spawn(


Ok, so I think I’m missing some context here.

What’s the impact on Subspace if each of these tasks fails?

last-domain-execution-receipt-request-handler

block-request-handler

syncing

Let’s assume a failure on most or all of the nodes on the network, because that could easily happen in the logs and we wouldn’t know about it. (Because the node won’t go down, and most people don’t read logs.)

shamil-gadelshin · 2024-11-06T14:32:19Z

What’s the impact on Subspace if each of these tasks fails?

last-domain-execution-receipt-request-handler

block-request-handler

syncing
Let’s assume a failure on most or all of the nodes on the network, because that could easily happen in the logs and we wouldn’t know about it. (Because the node won’t go down, and most people don’t read logs.)

Failure of any of these tasks is not catastrophic for the working node after the domain sync. However, total network failure will result in failed domain snap sync attempts (full sync will be possible though).

shamil-gadelshin · 2024-11-06T14:39:30Z

The last commit batch contains changes related to PR comments and offline discussion with Nazar:

new separate crate for SubspaceSyncEngine for consensus and domain snap sync
remove subspace-service dependencies from domain service and operator crates
refactor block notification streams to remove dependency from sc-consensus-subspace
move snap sync structures from subspace-service to domain-client-operator: SnapSyncOrchestrator and ConsensusChainSyncParams
remove MMR-sync from domain snap-sync because it was integrated into consensus snap-sync

teor2345 · 2024-11-06T23:13:07Z

What’s the impact on Subspace if each of these tasks fails?

last-domain-execution-receipt-request-handler

block-request-handler

syncing
Let’s assume a failure on most or all of the nodes on the network, because that could easily happen in the logs and we wouldn’t know about it. (Because the node won’t go down, and most people don’t read logs.)

Failure of any of these tasks is not catastrophic for the working node after the domain sync. However, total network failure will result in failed domain snap sync attempts (full sync will be possible though).

Ok, so I guess the overall question is:
If any of these tasks fail across the Subspace network, do we want them to exit (and hopefully restart)?
Or do we want them to keep on going, and no-one is able to snap sync?

Do we have tests that make sure all these tasks run properly, and don’t quickly error out?

teor2345

Thanks, this looks a bit clearer.

domains/client/domain-operator/src/snap_sync.rs

teor2345

Thanks, looks good to me.

I’m happy to leave the essential task question to another ticket or PR, but we’ll need to resolve conflicts for this to merge.

domains/client/domain-operator/src/lib.rs

crates/subspace-service/Cargo.toml

nazar-pc · 2024-11-08T07:30:38Z

shared/subspace-sync/Cargo.toml

@@ -0,0 +1,24 @@
+[package]
+name = "subspace-sync"


Suggested change

name = "subspace-sync"

name = "sc-subspace-sync-common"

domains/client/domain-operator/src/snap_sync.rs

nazar-pc · 2024-11-08T07:34:50Z

crates/subspace-node/src/commands/run/domain.rs

+    let consensus_network_sync_oracle_wrapper = Arc::new(ConsensusChainSyncOracleWrapper::new(
+        consensus_network_sync_oracle,
+        consensus_chain_sync_params
+            .as_ref()
+            .map(|params| params.snap_sync_orchestrator.domain_snap_sync_finished()),
+    ));


Still don't really understand why consensus syncing oracle needs to check if domain snap sync hash finished...

shamil-gadelshin · 2024-11-08T15:32:40Z

The last commit batch contains:

merge commit with main
MMR target block fix
new subspace sync renaming

shamil-gadelshin self-assigned this Oct 30, 2024

shamil-gadelshin requested review from NingLin-P, nazar-pc and rg3l3dr as code owners October 30, 2024 15:29

shamil-gadelshin mentioned this pull request Oct 30, 2024

Full domain chain snap sync. #3115

Closed

1 task

teor2345 reviewed Oct 30, 2024

View reviewed changes

shamil-gadelshin added 7 commits October 31, 2024 15:17

Introduce snap sync orchestrator.

4e0dad5

Remove last confirmed domain block receipt handlers.

d13b281

Add domain snap sync algorithm.

d30ddaf

Update MMR-sync

64eebe9

Update consensus snap-sync

415812d

Modify domain networking stack.

d6b9932

- add custom `build-network` method - add last domain block execution receipt protocol (moved from consensus chain)

Update domain service (domain snap-sync integration)

3bb35bd

shamil-gadelshin force-pushed the full-domain-snap-sync2 branch from 21ee3e0 to cc6572a Compare October 31, 2024 11:45

teor2345 previously approved these changes Oct 31, 2024

View reviewed changes

nazar-pc requested changes Oct 31, 2024

View reviewed changes

teor2345 reviewed Nov 3, 2024

View reviewed changes

shamil-gadelshin dismissed teor2345’s stale review via cb158d1 November 6, 2024 14:33

shamil-gadelshin requested a review from nazar-pc November 6, 2024 14:33

teor2345 reviewed Nov 6, 2024

View reviewed changes

domains/client/domain-operator/src/snap_sync.rs Outdated Show resolved Hide resolved

domains/client/domain-operator/src/snap_sync.rs Outdated Show resolved Hide resolved

shamil-gadelshin requested a review from teor2345 November 7, 2024 09:50

teor2345 reviewed Nov 7, 2024

View reviewed changes

nazar-pc reviewed Nov 8, 2024

View reviewed changes

shamil-gadelshin added 2 commits November 8, 2024 11:30

Update domain operator (domain snap-sync integration)

4a9c99b

Update subspace-service (domain snap-sync integration)

13ace29

shamil-gadelshin added 8 commits November 8, 2024 11:30

Modify SubspaceSyncOracle to add domain snap sync.

df83db2

Update subspace-node (domain snap-sync integration)

740b01d

Update test dependencies (domain snap-sync integration)

1a729f4

Extract syncing engine impl into separate crate subspace-sync

54a7c25

Update sync oracle for domain snap sync.

109003e

Remove MMR-sync from domain snap-sync

a7a4d7c

Move snap-sync structures to domains.

6fe0083

Refactor domain snap-sync

959a1a9

nazar-pc force-pushed the full-domain-snap-sync2 branch from b9fbdc3 to 959a1a9 Compare November 8, 2024 09:31

nazar-pc and others added 4 commits November 8, 2024 11:46

Rename sync oracle

5d1313e

Merge branch 'main' into full-domain-snap-sync2

ef322c0

Change MMR-sync target block.

88e4c3d

Rename subspace-sync -> sc-subspace-sync-common

df317ce

shamil-gadelshin requested a review from nazar-pc November 8, 2024 15:34

nazar-pc approved these changes Nov 8, 2024

View reviewed changes

shamil-gadelshin added this pull request to the merge queue Nov 8, 2024

Merged via the queue into main with commit f26446d Nov 8, 2024
10 checks passed

shamil-gadelshin deleted the full-domain-snap-sync2 branch November 8, 2024 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Full domain chain snap sync (v2). #3196

Full domain chain snap sync (v2). #3196

shamil-gadelshin commented Oct 30, 2024

teor2345 left a comment

nazar-pc commented Oct 31, 2024

teor2345 left a comment

teor2345 Oct 31, 2024

shamil-gadelshin Nov 1, 2024

teor2345 Nov 3, 2024

nazar-pc left a comment

nazar-pc Oct 31, 2024

shamil-gadelshin Nov 1, 2024

nazar-pc Nov 1, 2024

nazar-pc Oct 31, 2024

shamil-gadelshin Nov 1, 2024

nazar-pc Nov 1, 2024

nazar-pc Oct 31, 2024

shamil-gadelshin Nov 1, 2024

nazar-pc Nov 1, 2024

nazar-pc Oct 31, 2024

shamil-gadelshin Nov 1, 2024

nazar-pc Nov 1, 2024

nazar-pc Oct 31, 2024

shamil-gadelshin Nov 1, 2024

nazar-pc Nov 1, 2024

shamil-gadelshin commented Nov 1, 2024

teor2345 left a comment

teor2345 Nov 3, 2024

shamil-gadelshin commented Nov 6, 2024

shamil-gadelshin commented Nov 6, 2024

teor2345 commented Nov 6, 2024

teor2345 left a comment

teor2345 left a comment

nazar-pc Nov 8, 2024 •

edited

Loading

nazar-pc Nov 8, 2024

shamil-gadelshin commented Nov 8, 2024

Full domain chain snap sync (v2). #3196

Full domain chain snap sync (v2). #3196

Conversation

shamil-gadelshin commented Oct 30, 2024

Code contributor checklist:

teor2345 left a comment

Choose a reason for hiding this comment

nazar-pc commented Oct 31, 2024

teor2345 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazar-pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamil-gadelshin commented Nov 1, 2024

teor2345 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamil-gadelshin commented Nov 6, 2024

shamil-gadelshin commented Nov 6, 2024

teor2345 commented Nov 6, 2024

teor2345 left a comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

nazar-pc Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamil-gadelshin commented Nov 8, 2024

nazar-pc Nov 8, 2024 •

edited

Loading