Clique: Prevent out of turn blocks interrupt in-turn mining (#364)

consensus/clique/src/main/java/tech/pegasys/pantheon/consensus/clique/CliqueMiningTracker.java

@@ -0,0 +1,44 @@
+/*
+ * Copyright 2018 ConsenSys AG.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
+ * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ */
+package tech.pegasys.pantheon.consensus.clique;
+
+import tech.pegasys.pantheon.ethereum.ProtocolContext;
+import tech.pegasys.pantheon.ethereum.core.Address;
+import tech.pegasys.pantheon.ethereum.core.BlockHeader;
+
+public class CliqueMiningTracker {
+
+  private final Address localAddress;
+  private final ProtocolContext<CliqueContext> protocolContext;
+
+  public CliqueMiningTracker(
+      final Address localAddress, final ProtocolContext<CliqueContext> protocolContext) {
+    this.localAddress = localAddress;
+    this.protocolContext = protocolContext;
+  }
+
+  public boolean isProposerAfter(final BlockHeader header) {
+    final Address nextProposer =
+        CliqueHelpers.getProposerForBlockAfter(
+            header, protocolContext.getConsensusState().getVoteTallyCache());
+    return localAddress.equals(nextProposer);
+  }
+
+  public boolean canMakeBlockNextRound(final BlockHeader header) {
+    return CliqueHelpers.addressIsAllowedToProduceNextBlock(localAddress, protocolContext, header);
+  }
+
+  public boolean blockCreatedLocally(final BlockHeader header) {
+    return CliqueHelpers.getProposerOfBlock(header).equals(localAddress);
+  }
+}

consensus/clique/src/main/java/tech/pegasys/pantheon/consensus/clique/blockcreation/CliqueMiningCoordinator.java

@@ -13,15 +13,52 @@
 package tech.pegasys.pantheon.consensus.clique.blockcreation;
 
 import tech.pegasys.pantheon.consensus.clique.CliqueContext;
+import tech.pegasys.pantheon.consensus.clique.CliqueMiningTracker;
 import tech.pegasys.pantheon.ethereum.blockcreation.AbstractMiningCoordinator;
 import tech.pegasys.pantheon.ethereum.chain.Blockchain;
+import tech.pegasys.pantheon.ethereum.core.BlockHeader;
 import tech.pegasys.pantheon.ethereum.eth.sync.state.SyncState;
 
 public class CliqueMiningCoordinator
     extends AbstractMiningCoordinator<CliqueContext, CliqueBlockMiner> {
 
+  private final CliqueMiningTracker miningTracker;
+
   public CliqueMiningCoordinator(
-      final Blockchain blockchain, final CliqueMinerExecutor executor, final SyncState syncState) {
+      final Blockchain blockchain,
+      final CliqueMinerExecutor executor,
+      final SyncState syncState,
+      final CliqueMiningTracker miningTracker) {
     super(blockchain, executor, syncState);
+    this.miningTracker = miningTracker;
+  }
+
+  @Override
+  protected boolean newChainHeadInvalidatesMiningOperation(final BlockHeader newChainHeadHeader) {
+    if (!currentRunningMiner.isPresent()) {
+      return true;
+    }
+
+    if (miningTracker.blockCreatedLocally(newChainHeadHeader)) {
+      return true;
+    }
+
+    return networkBlockBetterThanCurrentMiner(newChainHeadHeader);
+  }
+
+  private boolean networkBlockBetterThanCurrentMiner(final BlockHeader newChainHeadHeader) {
+    final BlockHeader parentHeader = currentRunningMiner.get().getParentHeader();
+    final long currentMinerTargetHeight = parentHeader.getNumber() + 1;
+    if (currentMinerTargetHeight < newChainHeadHeader.getNumber()) {
+      return true;
+    }
+
+    final boolean nodeIsMining = miningTracker.canMakeBlockNextRound(parentHeader);
+    final boolean nodeIsInTurn = miningTracker.isProposerAfter(parentHeader);
+
+    if (nodeIsMining && nodeIsInTurn) {
+      return false;
+    }
+    return true;
   }
 }

consensus/clique/src/test/java/tech/pegasys/pantheon/consensus/clique/blockcreation/CliqueMiningCoordinatorTest.java

@@ -0,0 +1,240 @@
+/*
+ * Copyright 2018 ConsenSys AG.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
+ * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ */
+package tech.pegasys.pantheon.consensus.clique.blockcreation;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.reset;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+import static tech.pegasys.pantheon.ethereum.core.InMemoryStorageProvider.createInMemoryBlockchain;
+
+import tech.pegasys.pantheon.consensus.clique.CliqueContext;
+import tech.pegasys.pantheon.consensus.clique.CliqueMiningTracker;
+import tech.pegasys.pantheon.consensus.clique.TestHelpers;
+import tech.pegasys.pantheon.consensus.clique.VoteTallyCache;
+import tech.pegasys.pantheon.consensus.common.VoteTally;
+import tech.pegasys.pantheon.crypto.SECP256K1.KeyPair;
+import tech.pegasys.pantheon.ethereum.ProtocolContext;
+import tech.pegasys.pantheon.ethereum.chain.MutableBlockchain;
+import tech.pegasys.pantheon.ethereum.core.Address;
+import tech.pegasys.pantheon.ethereum.core.Block;
+import tech.pegasys.pantheon.ethereum.core.BlockBody;
+import tech.pegasys.pantheon.ethereum.core.BlockHeader;
+import tech.pegasys.pantheon.ethereum.core.BlockHeaderTestFixture;
+import tech.pegasys.pantheon.ethereum.core.Hash;
+import tech.pegasys.pantheon.ethereum.core.Util;
+import tech.pegasys.pantheon.ethereum.eth.sync.state.SyncState;
+
+import java.util.List;
+
+import org.assertj.core.util.Lists;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+@RunWith(MockitoJUnitRunner.class)
+public class CliqueMiningCoordinatorTest {
+
+  private final KeyPair proposerKeys = KeyPair.generate();
+  private final KeyPair validatorKeys = KeyPair.generate();
+  private final Address proposerAddress = Util.publicKeyToAddress(proposerKeys.getPublicKey());
+  private final Address validatorAddress = Util.publicKeyToAddress(validatorKeys.getPublicKey());
+
+  private final List<Address> validators = Lists.newArrayList(validatorAddress, proposerAddress);
+
+  private final BlockHeaderTestFixture headerTestFixture = new BlockHeaderTestFixture();
+
+  private CliqueMiningTracker miningTracker;
+
+  @Mock private MutableBlockchain blockChain;
+  @Mock private ProtocolContext<CliqueContext> protocolContext;
+  @Mock private CliqueMinerExecutor minerExecutor;
+  @Mock private CliqueBlockMiner blockMiner;
+  @Mock private SyncState syncState;
+  @Mock private VoteTallyCache voteTallyCache;
+
+  @Before
+  public void setup() {
+
+    headerTestFixture.number(1);
+    Block genesisBlock = createEmptyBlock(0, Hash.ZERO, proposerKeys); // not normally signed but ok
+    blockChain = createInMemoryBlockchain(genesisBlock);
+
+    final VoteTally voteTally = mock(VoteTally.class);
+    when(voteTally.getValidators()).thenReturn(validators);
+    when(voteTallyCache.getVoteTallyAfterBlock(any())).thenReturn(voteTally);
+    final CliqueContext cliqueContext = new CliqueContext(voteTallyCache, null, null);
+
+    when(protocolContext.getConsensusState()).thenReturn(cliqueContext);
+    when(protocolContext.getBlockchain()).thenReturn(blockChain);
+    when(minerExecutor.startAsyncMining(any(), any())).thenReturn(blockMiner);
+    when(syncState.isInSync()).thenReturn(true);
+
+    miningTracker = new CliqueMiningTracker(proposerAddress, protocolContext);
+  }
+
+  @Test
+  public void outOfTurnBlockImportedDoesNotInterruptInTurnMiningOperation() {
+    // As the head of the blockChain is 0 (which effectively doesn't have a signer, all validators
+    // are able to propose.
+
+    when(blockMiner.getParentHeader()).thenReturn(blockChain.getChainHeadHeader());
+
+    // Note also - validators is an hard-ordered LIST, thus in-turn will follow said list - block_1
+    // should be created by proposer.
+    final CliqueMiningCoordinator coordinator =
+        new CliqueMiningCoordinator(blockChain, minerExecutor, syncState, miningTracker);
+
+    coordinator.enable();
+
+    verify(minerExecutor, times(1)).startAsyncMining(any(), any());
+
+    reset(minerExecutor);
+
+    final Block importedBlock = createEmptyBlock(1, blockChain.getChainHeadHash(), validatorKeys);
+
+    blockChain.appendBlock(importedBlock, Lists.emptyList());
+
+    // The minerExecutor should not be invoked as the mining operation was conducted by an in-turn
+    // validator, and the created block came from an out-turn validator.
+    verify(minerExecutor, never()).startAsyncMining(any(), any());
+  }
+
+  @Test
+  public void outOfTurnBlockImportedAtHigherLevelInterruptsMiningOperation() {
+    // As the head of the blockChain is 1 (which effectively doesn't have a signer, all validators
+    // are able to propose.
+    when(blockMiner.getParentHeader()).thenReturn(blockChain.getChainHeadHeader());
+
+    // Note also - validators is an hard-ordered LIST, thus in-turn will follow said list - block_1
+    // should be created by proposer.
+    final CliqueMiningCoordinator coordinator =
+        new CliqueMiningCoordinator(blockChain, minerExecutor, syncState, miningTracker);
+
+    coordinator.enable();
+
+    verify(minerExecutor, times(1)).startAsyncMining(any(), any());
+
+    reset(minerExecutor);
+    when(minerExecutor.startAsyncMining(any(), any())).thenReturn(blockMiner);
+
+    final Block importedBlock = createEmptyBlock(2, blockChain.getChainHeadHash(), validatorKeys);
+
+    blockChain.appendBlock(importedBlock, Lists.emptyList());
+
+    // The minerExecutor should not be invoked as the mining operation was conducted by an in-turn
+    // validator, and the created block came from an out-turn validator.
+    ArgumentCaptor<BlockHeader> varArgs = ArgumentCaptor.forClass(BlockHeader.class);
+    verify(minerExecutor, times(1)).startAsyncMining(any(), varArgs.capture());
+    assertThat(varArgs.getValue()).isEqualTo(blockChain.getChainHeadHeader());
+  }
+
+  @Test
+  public void outOfTurnBlockImportedInterruptsOutOfTurnMiningOperation() {
+    blockChain.appendBlock(
+        createEmptyBlock(1, blockChain.getChainHeadHash(), validatorKeys), Lists.emptyList());
+
+    when(blockMiner.getParentHeader()).thenReturn(blockChain.getChainHeadHeader());
+
+    // Note also - validators is an hard-ordered LIST, thus in-turn will follow said list - block_2
+    // should be created by 'validator', thus Proposer is out-of-turn.
+    final CliqueMiningCoordinator coordinator =
+        new CliqueMiningCoordinator(blockChain, minerExecutor, syncState, miningTracker);
+
+    coordinator.enable();
+
+    verify(minerExecutor, times(1)).startAsyncMining(any(), any());
+
+    reset(minerExecutor);
+    when(minerExecutor.startAsyncMining(any(), any())).thenReturn(blockMiner);
+
+    final Block importedBlock = createEmptyBlock(2, blockChain.getChainHeadHash(), validatorKeys);
+
+    blockChain.appendBlock(importedBlock, Lists.emptyList());
+
+    // The minerExecutor should not be invoked as the mining operation was conducted by an in-turn
+    // validator, and the created block came from an out-turn validator.
+    ArgumentCaptor<BlockHeader> varArgs = ArgumentCaptor.forClass(BlockHeader.class);
+    verify(minerExecutor, times(1)).startAsyncMining(any(), varArgs.capture());
+    assertThat(varArgs.getValue()).isEqualTo(blockChain.getChainHeadHeader());
+  }
+
+  @Test
+  public void outOfTurnBlockImportedInterruptsNonRunningMiner() {
+    blockChain.appendBlock(
+        createEmptyBlock(1, blockChain.getChainHeadHash(), proposerKeys), Lists.emptyList());
+
+    when(blockMiner.getParentHeader()).thenReturn(blockChain.getChainHeadHeader());
+
+    // Note also - validators is an hard-ordered LIST, thus in-turn will follow said list - block_2
+    // should be created by 'validator', thus Proposer is out-of-turn.
+    final CliqueMiningCoordinator coordinator =
+        new CliqueMiningCoordinator(blockChain, minerExecutor, syncState, miningTracker);
+
+    coordinator.enable();
+
+    verify(minerExecutor, times(1)).startAsyncMining(any(), any());
+
+    reset(minerExecutor);
+    when(minerExecutor.startAsyncMining(any(), any())).thenReturn(blockMiner);
+
+    final Block importedBlock = createEmptyBlock(2, blockChain.getChainHeadHash(), validatorKeys);
+
+    blockChain.appendBlock(importedBlock, Lists.emptyList());
+
+    // The minerExecutor should not be invoked as the mining operation was conducted by an in-turn
+    // validator, and the created block came from an out-turn validator.
+    ArgumentCaptor<BlockHeader> varArgs = ArgumentCaptor.forClass(BlockHeader.class);
+    verify(minerExecutor, times(1)).startAsyncMining(any(), varArgs.capture());
+    assertThat(varArgs.getValue()).isEqualTo(blockChain.getChainHeadHeader());
+  }
+
+  @Test
+  public void locallyGeneratedBlockInvalidatesMiningEvenIfInTurn() {
+    // Note also - validators is an hard-ordered LIST, thus in-turn will follow said list - block_1
+    // should be created by Proposer, and thus will be in-turn.
+    final CliqueMiningCoordinator coordinator =
+        new CliqueMiningCoordinator(blockChain, minerExecutor, syncState, miningTracker);
+
+    coordinator.enable();
+
+    verify(minerExecutor, times(1)).startAsyncMining(any(), any());
+
+    reset(minerExecutor);
+    when(minerExecutor.startAsyncMining(any(), any())).thenReturn(blockMiner);
+
+    final Block importedBlock = createEmptyBlock(1, blockChain.getChainHeadHash(), proposerKeys);
+    blockChain.appendBlock(importedBlock, Lists.emptyList());
+
+    // The minerExecutor should not be invoked as the mining operation was conducted by an in-turn
+    // validator, and the created block came from an out-turn validator.
+    ArgumentCaptor<BlockHeader> varArgs = ArgumentCaptor.forClass(BlockHeader.class);
+    verify(minerExecutor, times(1)).startAsyncMining(any(), varArgs.capture());
+    assertThat(varArgs.getValue()).isEqualTo(blockChain.getChainHeadHeader());
+  }
+
+  private Block createEmptyBlock(
+      final long blockNumber, final Hash parentHash, final KeyPair signer) {
+    headerTestFixture.number(blockNumber).parentHash(parentHash);
+    final BlockHeader header =
+        TestHelpers.createCliqueSignedBlockHeader(headerTestFixture, signer, validators);
+    return new Block(header, new BlockBody(Lists.emptyList(), Lists.emptyList()));
+  }
+}

ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/AbstractMiningCoordinator.java

@@ -31,6 +31,7 @@ import org.apache.logging.log4j.Logger;
 public abstract class AbstractMiningCoordinator<
         C, M extends BlockMiner<C, ? extends AbstractBlockCreator<C>>>
     implements BlockAddedObserver, MiningCoordinator {
+
   private static final Logger LOG = getLogger();
   protected boolean isEnabled = false;
   protected volatile Optional<M> currentRunningMiner = Optional.empty();
@@ -95,7 +96,9 @@ public abstract class AbstractMiningCoordinator<
   @Override
   public void onBlockAdded(final BlockAddedEvent event, final Blockchain blockchain) {
     synchronized (this) {
-      if (isEnabled && event.isNewCanonicalHead()) {
+      if (isEnabled
+          && event.isNewCanonicalHead()
+          && newChainHeadInvalidatesMiningOperation(event.getBlock().getHeader())) {
         haltCurrentMiningOperation();
         if (syncState.isInSync()) {
           startAsyncMiningOperation();
@@ -135,4 +138,7 @@ public abstract class AbstractMiningCoordinator<
   public void setExtraData(final BytesValue extraData) {
     executor.setExtraData(extraData);
   }
+
+  protected abstract boolean newChainHeadInvalidatesMiningOperation(
+      final BlockHeader newChainHeadHeader);
 }

ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/BlockCreator.java

@@ -15,6 +15,5 @@ package tech.pegasys.pantheon.ethereum.blockcreation;
 import tech.pegasys.pantheon.ethereum.core.Block;
 
 public interface BlockCreator {
-
   Block createBlock(final long timestamp);
 }

ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/BlockMiner.java

@@ -65,6 +65,7 @@ public class BlockMiner<C, M extends AbstractBlockCreator<C>> implements Runnabl
 
   @Override
   public void run() {
+
     boolean blockMined = false;
     while (!blockMined) {
       try {
@@ -84,6 +85,7 @@ public class BlockMiner<C, M extends AbstractBlockCreator<C>> implements Runnabl
     // Ensure the block is allowed to be mined - i.e. the timestamp on the new block is sufficiently
     // ahead of the parent, and still within allowable clock tolerance.
     LOG.trace("Started a mining operation.");
+
     final long newBlockTimestamp = scheduler.waitUntilNextBlockCanBeMined(parentHeader);
 
     LOG.trace("Mining a new block with timestamp {}", newBlockTimestamp);
@@ -112,4 +114,8 @@ public class BlockMiner<C, M extends AbstractBlockCreator<C>> implements Runnabl
   private void notifyNewBlockListeners(final Block block) {
     observers.forEach(obs -> obs.blockMined(block));
   }
+
+  public BlockHeader getParentHeader() {
+    return parentHeader;
+  }
 }

ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/EthHashMiningCoordinator.java

@@ -15,6 +15,7 @@ package tech.pegasys.pantheon.ethereum.blockcreation;
 import tech.pegasys.pantheon.ethereum.chain.BlockAddedObserver;
 import tech.pegasys.pantheon.ethereum.chain.Blockchain;
 import tech.pegasys.pantheon.ethereum.core.Address;
+import tech.pegasys.pantheon.ethereum.core.BlockHeader;
 import tech.pegasys.pantheon.ethereum.eth.sync.state.SyncState;
 import tech.pegasys.pantheon.ethereum.mainnet.EthHashSolution;
 import tech.pegasys.pantheon.ethereum.mainnet.EthHashSolverInputs;
@@ -81,4 +82,9 @@ public class EthHashMiningCoordinator extends AbstractMiningCoordinator<Void, Et
         });
     currentRunningMiner = Optional.empty();
   }
+
+  @Override
+  protected boolean newChainHeadInvalidatesMiningOperation(final BlockHeader newChainHeadHeader) {
+    return true;
+  }
 }

ethereum/blockcreation/src/test/java/tech/pegasys/pantheon/ethereum/blockcreation/AbstractMiningCoordinatorTest.java

@@ -24,6 +24,7 @@ import tech.pegasys.pantheon.ethereum.chain.BlockAddedEvent;
 import tech.pegasys.pantheon.ethereum.chain.Blockchain;
 import tech.pegasys.pantheon.ethereum.core.Block;
 import tech.pegasys.pantheon.ethereum.core.BlockBody;
+import tech.pegasys.pantheon.ethereum.core.BlockHeader;
 import tech.pegasys.pantheon.ethereum.core.BlockHeaderTestFixture;
 import tech.pegasys.pantheon.ethereum.eth.sync.state.SyncState;
 
@@ -134,5 +135,10 @@ public class AbstractMiningCoordinatorTest {
         final SyncState syncState) {
       super(blockchain, executor, syncState);
     }
+
+    @Override
+    public boolean newChainHeadInvalidatesMiningOperation(final BlockHeader newChainHeadHeader) {
+      return true;
+    }
   }
 }

pantheon/src/main/java/tech/pegasys/pantheon/controller/CliquePantheonController.java

@@ -18,6 +18,7 @@ import tech.pegasys.pantheon.config.CliqueConfigOptions;
 import tech.pegasys.pantheon.config.GenesisConfigFile;
 import tech.pegasys.pantheon.consensus.clique.CliqueBlockInterface;
 import tech.pegasys.pantheon.consensus.clique.CliqueContext;
+import tech.pegasys.pantheon.consensus.clique.CliqueMiningTracker;
 import tech.pegasys.pantheon.consensus.clique.CliqueProtocolSchedule;
 import tech.pegasys.pantheon.consensus.clique.VoteTallyCache;
 import tech.pegasys.pantheon.consensus.clique.blockcreation.CliqueBlockScheduler;
@@ -32,6 +33,7 @@ import tech.pegasys.pantheon.ethereum.ProtocolContext;
 import tech.pegasys.pantheon.ethereum.blockcreation.MiningCoordinator;
 import tech.pegasys.pantheon.ethereum.chain.GenesisState;
 import tech.pegasys.pantheon.ethereum.chain.MutableBlockchain;
+import tech.pegasys.pantheon.ethereum.core.Address;
 import tech.pegasys.pantheon.ethereum.core.Hash;
 import tech.pegasys.pantheon.ethereum.core.MiningParameters;
 import tech.pegasys.pantheon.ethereum.core.Synchronizer;
@@ -108,6 +110,7 @@ public class CliquePantheonController implements PantheonController<CliqueContex
       final int networkId,
       final KeyPair nodeKeys,
       final MetricsSystem metricsSystem) {
+    final Address localAddress = Util.publicKeyToAddress(nodeKeys.getPublicKey());
     final CliqueConfigOptions cliqueConfig =
         genesisConfig.getConfigOptions().getCliqueConfigOptions();
     final long blocksPerEpoch = cliqueConfig.getEpochLength();
@@ -176,11 +179,15 @@ public class CliquePantheonController implements PantheonController<CliqueContex
             new CliqueBlockScheduler(
                 Clock.systemUTC(),
                 protocolContext.getConsensusState().getVoteTallyCache(),
-                Util.publicKeyToAddress(nodeKeys.getPublicKey()),
+                localAddress,
                 secondsBetweenBlocks),
             epochManger);
     final CliqueMiningCoordinator miningCoordinator =
-        new CliqueMiningCoordinator(blockchain, miningExecutor, syncState);
+        new CliqueMiningCoordinator(
+            blockchain,
+            miningExecutor,
+            syncState,
+            new CliqueMiningTracker(localAddress, protocolContext));
     miningCoordinator.addMinedBlockObserver(ethProtocolManager);
 
     // Clique mining is implicitly enabled.