[NC-2138] Implement chain download for fast sync #690
Conversation
…ce FastSyncChainDownloader that imports the chain only up to the pivot block.
| return requestAdditionalCheckpointHeaders(lastHeader, syncTarget) | ||
| .handle( | ||
| (headers, t) -> { | ||
| t = ExceptionUtils.rootCause(t); |
There was a problem hiding this comment.
I think we should use Guava's Throwables.getRootCause instead, it has cause loop detection.
There was a problem hiding this comment.
Of course our throwable accepts a null param and returns null in that case...
There was a problem hiding this comment.
Seems like a good idea to delegate to Guava's implementation but the null handling is really very useful given the API that CompletableFuture uses often gives us null exceptions to deal with: #692
| .thenApply( | ||
| result -> { | ||
| if (result.size() != 1 || !result.get(0).equals(pivotBlockHeader)) { | ||
| bestPeer.disconnect(DisconnectReason.USELESS_PEER); |
There was a problem hiding this comment.
I wonder if we should be disconnecting here if !result.get(0).equals(pivotBlockHeader). This peer could still provide useful data.
There was a problem hiding this comment.
Yeah I went back and forward on it. If we don't disconnect we'll need to maintain a sync target blacklist to avoid continuously selecting the same peer on a different chain. But ultimately I'm ok with disconnecting because while we might be useful data from a peer it's seems like a pretty strong indicator that something is wrong with a peer that's forked 500 blocks back in disagreement with the majority of our other peers.
|
|
||
| private CompletableFuture<Optional<EthPeer>> confirmPivotBlockHeader(final EthPeer bestPeer) { | ||
| final RetryingGetHeaderFromPeerByNumberTask task = | ||
| RetryingGetHeaderFromPeerByNumberTask.forPivotBlock( |
There was a problem hiding this comment.
nit - forPivotBlock should probably be called forSingleNumber to correspond to the methods on GetHeadersFromPeerByNumberTask
| } | ||
|
|
||
| @Test | ||
| public void shouldNotDuplicatePivotBlockAsCheckpoint() { |
There was a problem hiding this comment.
Might be worth adding a few more edge cases here - when the common ancestor is at the block prior to the pivot, or at the pivot etc.
PR description
Implements a
FastSyncChainDownloaderand hooks it intoFastSyncActionsso that the headers and bodies for the chain up to the pivot point are downloaded. It deliberately avoids downloading blocks that are beyond the pivot block.Receipts aren't yet downloaded and we're still performing full validation on all headers rather than light validation.