This repository has been archived by the owner on Sep 26, 2019. It is now read-only.
Store pivot block header #836
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Allow fast sync to resume if chain head is not genesis but a fast sync is still in progress. If present, use the stored pivot block header when restarting. Extract the fast sync logic out of DefaultSynchronizer into it's own class.
mbaxter
approved these changes
Feb 12, 2019
| @@ -126,6 +131,19 @@ private void waitForAnyPeer(final CompletableFuture<Void> result) { | |||
| } | |||
|
|
|||
| public CompletableFuture<FastSyncState> selectPivotBlock() { | |||
| return loadPivotBlockFromStorage().orElseGet(this::selectPivotBlockFromPeers); | |||
There was a problem hiding this comment.
(optional / discussion) What about loading this from storage in the constructor? Otherwise, successive calls to this method may always return the same value, which you may not want or may be unexpected. Also thinking it might be nice to move the reading / writing of this to the FastSyncState class with something like FastSyncState.create(dataDir) that loads or creates a new state.
There was a problem hiding this comment.
yeah I think there are definitely some improvements along these lines. The changes have wound up being reasonable large though so I'm going to merge this and put up a follow-up PR so we can review them and discuss more clearly.
…izer-checks-fast-sync-allowed
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR description
When the pivot block header has been downloaded, store it to disk. Currently this is always removed on shutdown because the world state downloader can't currently handle resuming. However, once that is complete and we stop deleting the stored header on shutdown:
FastSyncActionswill use that same pivot block when restartingNote that the validation which disabled fast sync when the chain wasn't empty has been moved out of
SynchronizerConfigurationinto the newFastSynchronizer. The validation therefore happens later than previously and theEthProtocolManagerwill requireEth63if fast sync is requested, even if it is disallowed due to the chain not being empty. Given how rareEth62-only clients are these days and that we expect to be able to findEth63clients on the first run when fast sync is used this seems like a reasonable trade off. The code is now simpler and the decision to use fast sync is all in one place now.The introduction of
FastSynchronizermoves a lot of fast-sync specific code out ofDefaultSynchronizer, keeping it simple and providing better isolation of concerns.