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

Improve sync from DSN #1793

Merged
merged 7 commits into from
Aug 11, 2023
Merged

Improve sync from DSN #1793

merged 7 commits into from
Aug 11, 2023

Conversation

nazar-pc
Copy link
Member

There were several issues with sync from DSN, I'm surprised it worked at all 🙈

So first I removed initial sync from DSN mode. It doesn't seem to be required anymore since we can discover when node is not synced in runtime just fine.

Then turned out while block_number - best_block_number >= QUEUED_BLOCKS_LIMIT.into() { was buggy since neither best_block_number was updated nor blocks were imported in between, essentially making this an infinite loop in case blocks are small enough to accumulate more than 2048 blocks in one segment.

Then turned out node was showing 0 peers on Substrate level due to pruned peers responding with no blocks when requested, but when we skip requests we can maintain connections, so I decided to start with paused sync mode and wait for networking to come alive and kick in DSN sync before anything else.

Last commit adds concurrency for piece retrieval, allowing to pull pieces at over 100 Mbps speed easily, making everything move much faster.

As the result sync from DSN should be much less broken than before.

Code contributor checklist:

@nazar-pc nazar-pc requested a review from rg3l3dr as a code owner August 10, 2023 12:11
@nazar-pc nazar-pc marked this pull request as draft August 10, 2023 12:28
@nazar-pc nazar-pc force-pushed the improve-sync-from-dsn branch from beea722 to a1076dc Compare August 10, 2023 13:35
@nazar-pc
Copy link
Member Author

Fixed a bit tricky concurrency issue around ordered acquisition of permits in previous revision, now seems to work fine

@nazar-pc nazar-pc marked this pull request as ready for review August 10, 2023 13:36
crates/subspace-service/src/dsn/import_blocks.rs Outdated Show resolved Hide resolved
@@ -319,15 +256,15 @@ where
downloaded_blocks += 1;

if downloaded_blocks % 1000 == 0 {
info!("Imported block {} from DSN", block_number);
Copy link
Contributor

Choose a reason for hiding this comment

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

It was an indicator of the importing progress. Can we add a similar message somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

You'll see usual sync speed reporting, this is on my machine with sync from DSN:

2023-08-10 17:25:19.419  INFO tokio-runtime-worker substrate: [Consensus] ⚙️  Syncing 12.4 bps, target=#59922 (6 peers), best: #50026 (0x4a90…cf38), finalized #9199 (0xa7b7…02b7), ⬇ 3.8kiB/s ⬆ 83 B/s    

@nazar-pc
Copy link
Member Author

Found that it still pulls unnecessary segment header under certain circumstances.

@nazar-pc nazar-pc requested review from shamil-gadelshin and removed request for shamil-gadelshin August 10, 2023 15:40
@nazar-pc nazar-pc merged commit 0663bdc into main Aug 11, 2023
@nazar-pc nazar-pc deleted the improve-sync-from-dsn branch August 11, 2023 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants