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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

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.

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.

2 participants