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

Support block gap created by fast sync #5703

Merged
merged 10 commits into from
Nov 20, 2024

Conversation

liuchengxu
Copy link
Contributor

This is part 2 of #5406 (comment), properly handling the block gap generated during fast sync.

Although #5406 remains unresolved due to the known issues in #5663, I decided to open up this PR earlier than later to speed up the overall progress. I've tested the fast sync locally with this PR, and it appears to be functioning well. (I was doing a fast sync from a discontinued archive node locally, thus the issue highlighted in #5663 (comment) was bypassed exactly.)

Once the edge cases in #5663 are addressed, we can move forward by removing the body attribute from the LightState block request and complete the work on #5406. The changes in this PR are incremental, so reviewing commit by commit should provide the best clarity.

cc @dmitry-markin

Copy link
Contributor

@dmitry-markin dmitry-markin left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, but do you think we should support converting already present "missing body" gap into "missing header and body" gap if detect it?

substrate/client/db/src/lib.rs Outdated Show resolved Hide resolved
@dmitry-markin dmitry-markin added the T0-node This PR/Issue is related to the topic “node”. label Sep 18, 2024
@liuchengxu
Copy link
Contributor Author

I don't believe supporting this conversion is necessary unless we encounter a specific use case that justifies it. Currently, the conceptual model is straightforward: gaps are caused either by warp sync or fast sync, but not both simultaneously. From my point of view, warp sync is typically the preferred method, with fast sync used only when warp sync is unsupported by the chain. Users are generally expected to choose one sync strategy, rather than a mix of the two. Adding support for converting gaps between the two could introduce unnecessary complexity without clear benefits.

@liuchengxu
Copy link
Contributor Author

Ping @bkchr In addition, some insights in terms of the edge case in #5663 (comment) would also be appreciated.

@liuchengxu
Copy link
Contributor Author

Could you have a look at this? @bkchr

prdoc/pr_5703.prdoc Outdated Show resolved Hide resolved
@bkchr bkchr enabled auto-merge November 19, 2024 22:11
@bkchr bkchr added this pull request to the merge queue Nov 19, 2024
Merged via the queue into paritytech:master with commit ce20d0a Nov 20, 2024
193 of 198 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants