-
Notifications
You must be signed in to change notification settings - Fork 668
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
Why request block body for fast sync? #5406
Comments
After looking into the original PR of fast sync paritytech/substrate#8884, I found this comment paritytech/substrate#8884 (comment), so it was intentionally to request the body in fast sync. However, this is a mismatch between the implementation and spec IMHO. The existing documentation always mentions downloading the header chain, but the block history is downloaded silently. While it may not be a big problem for the chain without a huge block history, for a project like subcoin, it matters about downloading hundreds of MiB or hundreds of GiB data before downloading the state. We should fix this mismatch in the implementation, what do you think? @arkpar |
Indeed, fast sync is supposed to download and verify the header chain first, then the state, then block bodies in the background. |
Great that we're on the same page! Any insights on the implementation? If I read the code correctly, the gap sync will be started automatically to download the block history once the block gap is detected in |
Right, the backgroudn block body download should utilize the same mechanism as the warp sync. The gap in this case is in block bodies, rather than whole block. The |
Going through |
So I guess we need to either change the logic of detecting the block gap within |
Yes, the first approach is better. Worth checking where else the block gap is used and if it won't break anything. |
We need a formal definition of a block gap before implementation. The concept of a block gap has evolved. Initially, it could only be created during warp sync; now, it can be created by both warp sync and fast sync. Warp sync creates a block gap by first downloading the finality proof and state at a specific height and then proceeding to download the block history. In contrast, fast sync downloads the header chain and state at a specific height before downloading the block history. Block Gap Creation and Updates
Block Gap Check
The logic for decreasing a block gap is consistent across both warp sync and fast sync, but fast sync can increase the gap during the header chain download. Implementation Plan
The current BlockGap is represented as pub enum BlockGapType {
// Gap created by warp sync.
MissingHeaderAndBody,
// Gap created by fast sync.
MissingBody,
}
pub struct BlockGap<N> {
start: N,
end: N,
gap_type: BlockGapType,
}
The primary changes will occur in I've implemented a working prototype locally based on the ideas above, and it appears to be functioning well. I would appreciate it if you could review the changes and provide feedback in case I missed something important. @arkpar @dmitry-markin |
@liuchengxu sure, feel free to publish your local changes as a PR / draft PR. |
Please have a look at #5540 before I send a new PR @dmitry-markin . |
There are basically three commits in this PR. Since all these commits essentially have no logical changes, I packed them into one PR. Review by per-commit is recommended. - The first commit avoids unnecessarily updating the block gap storage when the value remains unchanged, as discovered when I worked on #5406. - The second commit is purely about format string style changes but deletes ~10 lines of code, which slightly helps me look into this file :P - The third commit is added to avoid the unnecessary block gap update in `BlockchainDb`. --------- Co-authored-by: Davide Galassi <davxy@datawok.net>
Previously, block gaps could only be created by warp sync, but with upcoming changes (paritytech#5406), block gaps will also be generated by fast sync, thus `BlockGapType` is needed. This refactor converts the existing `(NumberFor<Block>, NumberFor<Block>)` into a dedicated `BlockGap<NumberFor<Block>>` struct. This change is purely structural and does not alter existing logic, but it lays the groundwork for future changes.
Previously, block gaps could only be created by warp sync, but block gaps will also be generated by fast sync once #5406 is fixed. This PR is part 1 of the detailed implementation plan in #5406 (comment): refactor `BlockGap`. This refactor converts the existing `(NumberFor<Block>, NumberFor<Block>)` into a dedicated `BlockGap<NumberFor<Block>>` struct. This change is purely structural and does not alter existing logic, but lays the groundwork for the follow-up PR. The compatibility concern caused by the new structure is addressed in the second commit. cc @dmitry-markin --------- Co-authored-by: Bastian Köcher <git@kchr.de>
This PR addresses an issue where state sync may fail to start if the conditions required for its initiation are not met when a finalized block notification is received. `pending_state_sync_attempt` is introduced to trigger the state sync later when the conditions are satisfied. This issue was spotted when I worked on #5406, specifically, `queue_blocks` was not empty when the finalized block notification was received, and then the state sync was stalled. cc @dmitry-markin --------- Co-authored-by: Dmitry Markin <dmitry@markin.tech> Co-authored-by: Bastian Köcher <git@kchr.de>
This PR addresses an issue where state sync may fail to start if the conditions required for its initiation are not met when a finalized block notification is received. `pending_state_sync_attempt` is introduced to trigger the state sync later when the conditions are satisfied. This issue was spotted when I worked on paritytech#5406, specifically, `queue_blocks` was not empty when the finalized block notification was received, and then the state sync was stalled. cc @dmitry-markin --------- Co-authored-by: Dmitry Markin <dmitry@markin.tech> Co-authored-by: Bastian Köcher <git@kchr.de> (cherry picked from commit 8236718)
Is there an existing issue?
Experiencing problems? Have you tried our Stack Exchange first?
Description of bug
If I read correctly, the docs https://github.com/paritytech/polkadot-sdk/blob/3fe22d17c4904c5ae016034b6ec2c335464b4165/substrate/client/network/README.md#fast-sync indicates that the fast sync is expected only to download and verify the header chain, and the subsequent state snapshot.
But the inline docs are contradictory, saying Downloading blocks:
polkadot-sdk/substrate/client/cli/src/arg_enums.rs
Lines 275 to 278 in 3fe22d1
and the block body is indeed requested for
--sync fast
and--sync fast-unsafe
.polkadot-sdk/substrate/client/network/sync/src/strategy/chain_sync.rs
Lines 1220 to 1225 in 3fe22d1
I'm looking for a sync mode that only downloads the headers and the state snapshot. I initially assumed that the --sync fast/fast-unsafe options would fulfill this requirement. This looks like a bug to me, could you please clarify this confusion?
Steps to reproduce
No response
The text was updated successfully, but these errors were encountered: