Pipeline chain download - Checkpoints #1203
Conversation
…astSynchronizer and FullSynchronizer to FastDownloaderFactory and FullDownloaderFactory.
…wnloader-abstraction
…wnloaderFactory to FullSyncDownloader since it doesn't actually need a factory.
…pleted (SyncTargetManager would just blindly re-use it).
…-download-target-selection
…nd wire them into FastSyncChainDownloader with a currently off feature toggle.
| final BlockHeader commonAncestor, | ||
| final int checkpointTimeoutsPermitted, | ||
| final int newHeaderWaitDuration, | ||
| final TimeUnit newHeaderWaitDurationUnit) { |
There was a problem hiding this comment.
(optional) You could simplify the constructor by accepting a single Duration object rather than newHeaderWaitDuration and newHeaderWaitDurationUnit.
There was a problem hiding this comment.
True, we can't pass it on as a duration to the get call that needs it but no need to share that pain.
| if (pendingCheckpointsRequest.isPresent()) { | ||
| return getCheckpointRangeFromPendingRequest(); | ||
| } | ||
| pendingCheckpointsRequest = |
There was a problem hiding this comment.
Do we need any kind of throttling here? To slow down requests if we keep sending and failing in quick succession?
There was a problem hiding this comment.
Probably a good thing to do.
| final SynchronizerConfiguration syncConfig, | ||
| final ProtocolSchedule<?> protocolSchedule, | ||
| final EthContext ethContext, | ||
| final UnaryOperator<List<BlockHeader>> checkpointFilter, |
There was a problem hiding this comment.
(optional) You could set this as a Supplier<OptionalLong> that provides the max block number to request, then we can avoid going over the limit for the last fast sync request.
There was a problem hiding this comment.
I'm going to leave this for now but have a note to revisit. We need to ensure the last checkpoint header for fast sync is the pivot block and it sometimes needs to be explicitly inserted. We could use an Optional<BlockHeader> finalCheckpoint and include all the logic inside CheckpointHeaderFetcher but then it winds up with a bunch of fast sync specific logic mixed in. Need to play around with it but want to get something actually working first.
|
|
||
| @Override | ||
| public CompletableFuture<Void> start() { | ||
| return performDownload(); |
There was a problem hiding this comment.
You may want to use an AtomicBoolean to make sure start is only called once.
There was a problem hiding this comment.
We do tend to follow that pattern. Done.
| } | ||
|
|
||
| @Override | ||
| public synchronized void cancel() { |
There was a problem hiding this comment.
Why do you need these synchronized keywords? If you only allow cancellation once, after starting (if(started.get() && cancelled.compareAndSet(false, true)){...}), do you need to synchronize?
There was a problem hiding this comment.
We need the synchronised keyword to protect access to the currentDownloadPipeline field and to ensure that when we're cancelling, there isn't another thread starting a different pipeline. We need the creation, starting and assignment to currentDownloadPipeline to be a single atomic operation so we know cancel cancels the pipeline that's actually running.
…-download-checkpoints
…lling in a tight loop.
…-download-checkpoints
PR description
Implement an
Iteratorthat retrieves checkpoint headers and creates ranges based on those to act as the start of the chain download pipeline.Also adds the plumbing required to hook the pipeline download into Pantheon, but currently toggled off (it only gets as far as creating checkpoint ranges currently).
Builds on #1196.