-
Notifications
You must be signed in to change notification settings - Fork 804
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
Fix fastsync with bonsai #2372
Fix fastsync with bonsai #2372
Conversation
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, we take a big hit for restarting a bonsai fast-sync. Maybe we can revisit this in the future to see if we can do some checkpointing, but this seems like a safe workaround.
@@ -79,7 +80,9 @@ | |||
"Fast sync was requested, but cannot be enabled because the local blockchain is not empty."); | |||
return Optional.empty(); | |||
} | |||
|
|||
if (worldStateStorage instanceof BonsaiPersistedWorldState) { | |||
worldStateStorage.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we bail and restart the world state sync if there was an aborted fast sync, but resume the chain downloading? The worldstate is the bulk of the download activity right? the pivot block isn't going to be too far back in history?
If I understand correctly - this seems safe but an expensive restart.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed it is a workaround waiting to find better. There are two things we download the blocks headers and the recent state trie. The only part we reset is the recent state trie. The headers are not removed and it is a big part so we reduce the impact to the minimum. And the pivot block remains the same .
This is a fix to avoid that a person who restarts his node during a fastsync does not waste time synchronizing a corrupted node. But the typical use case is not to re-start a node during a fastsync 🧑🏭
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM karim.t2am@gmail.com
PR description
If we stop and restart a fastsync synchronization we may have missing data. The idea of this PR is to reset the worldstate while keeping the segments to avoid any data corruption
Fixed Issue(s)
Changelog