[PAN-2325] Fix IndexOutOfBoundsException in DetermineCommonAncestorTask #1038
Conversation
Remove generic param from DetermineCommonAncestorTask.
| @@ -220,7 +220,7 @@ public int getWorldStateMaxRequestsWithoutProgress() { | |||
| private Range<Long> blockPropagationRange = Range.closed(-10L, 30L); | |||
| private long downloaderChangeTargetThresholdByHeight = 20L; | |||
| private UInt256 downloaderChangeTargetThresholdByTd = UInt256.of(1_000_000_000L); | |||
| private int downloaderHeaderRequestSize = 10; | |||
| private int downloaderHeaderRequestSize = 200; | |||
There was a problem hiding this comment.
Somewhat unrelated, but I see no reason not to bump up this value.
This will mean we request larger chunks of headers when running DetermineCommonAncestorTask. We'll also request a larger number of checkpoint headers when downloading.
| public void shouldFailIfPeerDisconnects() { | ||
| final Block block = blockDataGenerator.nextBlock(localBlockchain.getChainHeadBlock()); | ||
| localBlockchain.appendBlock(block, blockDataGenerator.receipts(block)); | ||
| public void shouldHandleEmptyResponses() { |
There was a problem hiding this comment.
Added this test to validate the bug fix.
| @@ -57,57 +58,34 @@ | |||
|
|
|||
| public class DetermineCommonAncestorTaskTest { | |||
There was a problem hiding this comment.
This file looks pretty messy, but in summary: 1 test was added for the bug fix, 2 tests for undefined behavior were removed, the generic param on this task was removed, and otherwise there's just a little bit of cleanup / reorganization.
| final WorldStateArchive worldStateArchive = createInMemoryWorldStateArchive(); | ||
| ethProtocolManager = EthProtocolManagerTestUtil.create(localBlockchain, worldStateArchive); | ||
| ethContext = ethProtocolManager.ethContext(); | ||
| protocolContext = new ProtocolContext<>(localBlockchain, worldStateArchive, null); | ||
| } | ||
|
|
||
| @Test | ||
| public void shouldThrowExceptionNoCommonBlock() { |
| @@ -184,104 +160,24 @@ public void shouldCorrectlyCalculateSkipIntervalAndCount() { | |||
| assertThat(skipInterval).isEqualTo(9); | |||
| } | |||
|
|
|||
| @Test | |||
| public void shouldGracefullyHandleExecutionsForNoCommonAncestor() { | |||
| @@ -142,8 +148,13 @@ static int calculateCount(final double range, final int skipInterval) { | |||
|
|
|||
| private CompletableFuture<Void> processHeaders( | |||
| final AbstractPeerTask.PeerTaskResult<List<BlockHeader>> headersResult) { | |||
| final CompletableFuture<Void> processingResult = CompletableFuture.completedFuture(null); | |||
There was a problem hiding this comment.
optional nit: do we need to hold a variable for this? we can call CompletableFuture.completedFuture(null) in both returns and never use it more than once. Feels like too much normalization.
There was a problem hiding this comment.
Yeah, I think it does end up being a bit confusing. I'll clean this up.
PR description
Fix issue where
DetermineCommonAncestorTaskcould throw anIndexOutOfBoundsExceptionwhen receiving an empty headers response.Additionally, clean up tests a bit by deduplicating some setup code. Remove tests that were validating undefined behavior (what happens when we try to find an ancestor with a peer on a different chain altogether). Remove generic param from
DetermineCommonAncestorTask. Add a few utility methods.