From a6c678944b67fff740f2084963fa98e5cc2e818f Mon Sep 17 00:00:00 2001 From: Abdelhamid Bakhta <45264458+abdelhamidbakhta@users.noreply.github.com> Date: Wed, 19 Jun 2019 21:46:37 +0200 Subject: [PATCH] [PAN-2630] Synchronizer should disconnect the sync target peer on invalid block data (#1578) * [PAN-2630] Synchronizer should disconnect the sync target peer on invalid block data - check if headers are sequential - if sequential, check if they form a chain, if not, disconnect the peer * update behaviour and add tests - change checks order - add log message - write tests * disconnect sync target when InvalidBlockException * remove power mockito - remove powermockito - write tests in subclasses of the `AbstractGetHeadersFromPeerTaskTest` * fix Exception check bug * fix PR discussion first pass - add assertion to check when the peer is disconnected - add assertion to check the peer has not been disonnected - undo remove final on class - remove sync target field * remove spy invocation * spotlessApply * shouldDisconnectPeerIfInvalidBlockException Signed-off-by: Adrian Sutton --- ethereum/eth/build.gradle | 1 - .../task/AbstractGetHeadersFromPeerTask.java | 20 ++++- .../eth/sync/PipelineChainDownloader.java | 11 +++ .../GetHeadersFromPeerByHashTaskTest.java | 75 +++++++++++++++++++ .../GetHeadersFromPeerByNumberTaskTest.java | 57 ++++++++++++++ .../eth/sync/PipelineChainDownloaderTest.java | 20 +++++ 6 files changed, 179 insertions(+), 5 deletions(-) diff --git a/ethereum/eth/build.gradle b/ethereum/eth/build.gradle index 7d8309a5a8..ca0881d113 100644 --- a/ethereum/eth/build.gradle +++ b/ethereum/eth/build.gradle @@ -55,7 +55,6 @@ dependencies { testImplementation 'org.assertj:assertj-core' testImplementation 'org.awaitility:awaitility' testImplementation 'org.mockito:mockito-core' - jmhImplementation project(path: ':ethereum:core', configuration: 'testSupportArtifacts') integrationTestImplementation project(path: ':config', configuration: 'testSupportArtifacts') diff --git a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/manager/task/AbstractGetHeadersFromPeerTask.java b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/manager/task/AbstractGetHeadersFromPeerTask.java index 253e684d3f..4529c4826c 100644 --- a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/manager/task/AbstractGetHeadersFromPeerTask.java +++ b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/manager/task/AbstractGetHeadersFromPeerTask.java @@ -21,6 +21,7 @@ import tech.pegasys.pantheon.ethereum.eth.messages.BlockHeadersMessage; import tech.pegasys.pantheon.ethereum.eth.messages.EthPV62; import tech.pegasys.pantheon.ethereum.mainnet.ProtocolSchedule; import tech.pegasys.pantheon.ethereum.p2p.rlpx.wire.MessageData; +import tech.pegasys.pantheon.ethereum.p2p.rlpx.wire.messages.DisconnectMessage; import tech.pegasys.pantheon.metrics.MetricsSystem; import java.util.ArrayList; @@ -86,16 +87,27 @@ public abstract class AbstractGetHeadersFromPeerTask final List headersList = new ArrayList<>(); headersList.add(firstHeader); - long prevNumber = firstHeader.getNumber(); - + BlockHeader prevBlockHeader = firstHeader; final int expectedDelta = reverse ? -(skip + 1) : (skip + 1); for (int i = 1; i < headers.size(); i++) { final BlockHeader header = headers.get(i); - if (header.getNumber() != prevNumber + expectedDelta) { + if (header.getNumber() != prevBlockHeader.getNumber() + expectedDelta) { // Skip doesn't match, this isn't our data return Optional.empty(); } - prevNumber = header.getNumber(); + // if headers are supposed to be sequential check if a chain is formed + if (Math.abs(expectedDelta) == 1) { + final BlockHeader parent = reverse ? header : prevBlockHeader; + final BlockHeader child = reverse ? prevBlockHeader : header; + if (!parent.getHash().equals(child.getParentHash())) { + LOG.debug( + "Sequential headers must form a chain through hashes, disconnecting peer: {}", + peer.toString()); + peer.disconnect(DisconnectMessage.DisconnectReason.BREACH_OF_PROTOCOL); + return Optional.empty(); + } + } + prevBlockHeader = header; headersList.add(header); } diff --git a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/PipelineChainDownloader.java b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/PipelineChainDownloader.java index 6e1cc4379c..ff01ccc43b 100644 --- a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/PipelineChainDownloader.java +++ b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/PipelineChainDownloader.java @@ -21,6 +21,7 @@ import tech.pegasys.pantheon.ethereum.eth.manager.exceptions.EthTaskException; import tech.pegasys.pantheon.ethereum.eth.sync.state.SyncState; import tech.pegasys.pantheon.ethereum.eth.sync.state.SyncTarget; import tech.pegasys.pantheon.ethereum.eth.sync.tasks.exceptions.InvalidBlockException; +import tech.pegasys.pantheon.ethereum.p2p.rlpx.wire.messages.DisconnectMessage; import tech.pegasys.pantheon.metrics.Counter; import tech.pegasys.pantheon.metrics.LabelledMetric; import tech.pegasys.pantheon.metrics.MetricsSystem; @@ -121,6 +122,16 @@ public class PipelineChainDownloader implements ChainDownloader { // Allowing the normal looping logic to retry after a brief delay. return scheduler.scheduleFutureTask(() -> completedFuture(null), PAUSE_AFTER_ERROR_DURATION); } + if (ExceptionUtils.rootCause(error) instanceof InvalidBlockException) { + syncState + .syncTarget() + .ifPresent( + syncTarget -> + syncTarget + .peer() + .disconnect(DisconnectMessage.DisconnectReason.BREACH_OF_PROTOCOL)); + } + logDownloadFailure("Chain download failed.", error); // Propagate the error out, terminating this chain download. return completedExceptionally(error); diff --git a/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/manager/task/GetHeadersFromPeerByHashTaskTest.java b/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/manager/task/GetHeadersFromPeerByHashTaskTest.java index 78e835adbc..4eb2a1512d 100644 --- a/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/manager/task/GetHeadersFromPeerByHashTaskTest.java +++ b/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/manager/task/GetHeadersFromPeerByHashTaskTest.java @@ -13,23 +13,43 @@ package tech.pegasys.pantheon.ethereum.eth.manager.task; import static org.assertj.core.api.Assertions.assertThat; +import static org.hamcrest.collection.IsCollectionWithSize.hasSize; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static tech.pegasys.pantheon.ethereum.vm.TestBlockchain.generateTestBlockHash; import tech.pegasys.pantheon.ethereum.core.BlockHeader; +import tech.pegasys.pantheon.ethereum.core.BlockHeaderTestFixture; +import tech.pegasys.pantheon.ethereum.eth.manager.EthPeer; import tech.pegasys.pantheon.ethereum.eth.manager.EthProtocolManagerTestUtil; import tech.pegasys.pantheon.ethereum.eth.manager.RespondingEthPeer; import tech.pegasys.pantheon.ethereum.eth.manager.RespondingEthPeer.Responder; import tech.pegasys.pantheon.ethereum.eth.manager.ethtaskutils.PeerMessageTaskTest; import tech.pegasys.pantheon.ethereum.eth.manager.task.AbstractPeerTask.PeerTaskResult; +import tech.pegasys.pantheon.ethereum.eth.messages.BlockHeadersMessage; +import tech.pegasys.pantheon.ethereum.p2p.rlpx.wire.messages.DisconnectMessage; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; +import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; +import org.hamcrest.MatcherAssert; import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; +@RunWith(MockitoJUnitRunner.class) public class GetHeadersFromPeerByHashTaskTest extends PeerMessageTaskTest> { + @Mock private EthPeer peerMock; @Override protected void assertPartialResultMatchesExpectation( @@ -124,4 +144,59 @@ public class GetHeadersFromPeerByHashTaskTest extends PeerMessageTaskTest headers = Arrays.asList(block1, block2); + + final AbstractGetHeadersFromPeerTask task = + new GetHeadersFromPeerByHashTask( + protocolSchedule, + ethContext, + block1.getHash(), + startNumber, + 2, + 0, + false, + metricsSystem); + Optional> optionalBlockHeaders = + task.processResponse(false, BlockHeadersMessage.create(headers), peerMock); + assertNotNull(optionalBlockHeaders); + assertTrue(optionalBlockHeaders.isPresent()); + List blockHeaders = optionalBlockHeaders.get(); + MatcherAssert.assertThat(blockHeaders, hasSize(2)); + verify(peerMock, times(0)).disconnect(any()); + } + + @Test + public void checkThatSequentialHeadersNotFormingAChainFails() { + final int startNumber = 1; + + final BlockHeader block1 = + new BlockHeaderTestFixture().number(1).parentHash(generateTestBlockHash(0)).buildHeader(); + final BlockHeader block2 = + new BlockHeaderTestFixture().number(2).parentHash(generateTestBlockHash(1)).buildHeader(); + final List headers = Arrays.asList(block1, block2); + final AbstractGetHeadersFromPeerTask task = + new GetHeadersFromPeerByHashTask( + protocolSchedule, + ethContext, + block1.getHash(), + startNumber, + 2, + 0, + false, + metricsSystem); + Optional> optionalBlockHeaders = + task.processResponse(false, BlockHeadersMessage.create(headers), peerMock); + assertNotNull(optionalBlockHeaders); + assertFalse(optionalBlockHeaders.isPresent()); + verify(peerMock).disconnect(DisconnectMessage.DisconnectReason.BREACH_OF_PROTOCOL); + } } diff --git a/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/manager/task/GetHeadersFromPeerByNumberTaskTest.java b/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/manager/task/GetHeadersFromPeerByNumberTaskTest.java index 6aa945add5..f47db0899e 100644 --- a/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/manager/task/GetHeadersFromPeerByNumberTaskTest.java +++ b/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/manager/task/GetHeadersFromPeerByNumberTaskTest.java @@ -13,21 +13,41 @@ package tech.pegasys.pantheon.ethereum.eth.manager.task; import static org.assertj.core.api.Assertions.assertThat; +import static org.hamcrest.collection.IsCollectionWithSize.hasSize; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static tech.pegasys.pantheon.ethereum.vm.TestBlockchain.generateTestBlockHash; import tech.pegasys.pantheon.ethereum.core.BlockHeader; +import tech.pegasys.pantheon.ethereum.core.BlockHeaderTestFixture; +import tech.pegasys.pantheon.ethereum.eth.manager.EthPeer; import tech.pegasys.pantheon.ethereum.eth.manager.EthProtocolManagerTestUtil; import tech.pegasys.pantheon.ethereum.eth.manager.RespondingEthPeer; import tech.pegasys.pantheon.ethereum.eth.manager.ethtaskutils.PeerMessageTaskTest; +import tech.pegasys.pantheon.ethereum.eth.messages.BlockHeadersMessage; +import tech.pegasys.pantheon.ethereum.p2p.rlpx.wire.messages.DisconnectMessage; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; +import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; +import org.hamcrest.MatcherAssert; import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; +@RunWith(MockitoJUnitRunner.class) public class GetHeadersFromPeerByNumberTaskTest extends PeerMessageTaskTest> { + @Mock private EthPeer peerMock; @Override protected void assertPartialResultMatchesExpectation( @@ -111,4 +131,41 @@ public class GetHeadersFromPeerByNumberTaskTest extends PeerMessageTaskTest headers = Arrays.asList(block1, block2); + final AbstractGetHeadersFromPeerTask task = + new GetHeadersFromPeerByNumberTask( + protocolSchedule, ethContext, block1.getNumber(), 2, 0, false, metricsSystem); + Optional> optionalBlockHeaders = + task.processResponse(false, BlockHeadersMessage.create(headers), peerMock); + assertNotNull(optionalBlockHeaders); + assertTrue(optionalBlockHeaders.isPresent()); + List blockHeaders = optionalBlockHeaders.get(); + MatcherAssert.assertThat(blockHeaders, hasSize(2)); + verify(peerMock, times(0)).disconnect(any()); + } + + @Test + public void checkThatSequentialHeadersNotFormingAChainFails() { + + final BlockHeader block1 = + new BlockHeaderTestFixture().number(1).parentHash(generateTestBlockHash(0)).buildHeader(); + final BlockHeader block2 = + new BlockHeaderTestFixture().number(2).parentHash(generateTestBlockHash(1)).buildHeader(); + final List headers = Arrays.asList(block1, block2); + final AbstractGetHeadersFromPeerTask task = + new GetHeadersFromPeerByNumberTask( + protocolSchedule, ethContext, block1.getNumber(), 2, 0, false, metricsSystem); + Optional> optionalBlockHeaders = + task.processResponse(false, BlockHeadersMessage.create(headers), peerMock); + assertNotNull(optionalBlockHeaders); + assertFalse(optionalBlockHeaders.isPresent()); + verify(peerMock).disconnect(DisconnectMessage.DisconnectReason.BREACH_OF_PROTOCOL); + } } diff --git a/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/PipelineChainDownloaderTest.java b/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/PipelineChainDownloaderTest.java index c509153d73..9219369b6a 100644 --- a/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/PipelineChainDownloaderTest.java +++ b/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/PipelineChainDownloaderTest.java @@ -31,6 +31,8 @@ import tech.pegasys.pantheon.ethereum.eth.manager.EthPeer; import tech.pegasys.pantheon.ethereum.eth.manager.EthScheduler; import tech.pegasys.pantheon.ethereum.eth.sync.state.SyncState; import tech.pegasys.pantheon.ethereum.eth.sync.state.SyncTarget; +import tech.pegasys.pantheon.ethereum.eth.sync.tasks.exceptions.InvalidBlockException; +import tech.pegasys.pantheon.ethereum.p2p.rlpx.wire.messages.DisconnectMessage; import tech.pegasys.pantheon.metrics.noop.NoOpMetricsSystem; import tech.pegasys.pantheon.services.pipeline.Pipeline; @@ -44,6 +46,7 @@ import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Mock; +import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; @RunWith(MockitoJUnitRunner.class) @@ -251,6 +254,23 @@ public class PipelineChainDownloaderTest { assertCancelled(result); } + @Test + public void shouldDisconnectPeerIfInvalidBlockException() { + final CompletableFuture selectTargetFuture = new CompletableFuture<>(); + when(syncTargetManager.shouldContinueDownloading()).thenReturn(false); + when(syncTargetManager.findSyncTarget(Optional.empty())) + .thenReturn(selectTargetFuture) + .thenReturn(new CompletableFuture<>()); + final EthPeer ethPeer = Mockito.mock(EthPeer.class); + final BlockHeader commonAncestor = Mockito.mock(BlockHeader.class); + final SyncTarget target = new SyncTarget(ethPeer, commonAncestor); + when(syncState.syncTarget()).thenReturn(Optional.of(target)); + chainDownloader.start(); + verify(syncTargetManager).findSyncTarget(Optional.empty()); + selectTargetFuture.completeExceptionally(new InvalidBlockException("", 1, null)); + verify(ethPeer).disconnect(DisconnectMessage.DisconnectReason.BREACH_OF_PROTOCOL); + } + private CompletableFuture expectPipelineStarted(final SyncTarget syncTarget) { return expectPipelineStarted(syncTarget, downloadPipeline); }