From 08cc58236e616f9cd28c67bdabe763c2771df366 Mon Sep 17 00:00:00 2001 From: tmohay <37158202+rain-on@users.noreply.github.com> Date: Sat, 8 Dec 2018 15:17:58 +1100 Subject: [PATCH] Clique: Prevent out of turn blocks interrupt in-turn mining (#364) --- .../consensus/clique/CliqueMiningTracker.java | 44 ++++ .../CliqueMiningCoordinator.java | 39 ++- .../CliqueMiningCoordinatorTest.java | 240 ++++++++++++++++++ .../AbstractMiningCoordinator.java | 8 +- .../ethereum/blockcreation/BlockCreator.java | 1 - .../ethereum/blockcreation/BlockMiner.java | 6 + .../EthHashMiningCoordinator.java | 6 + .../AbstractMiningCoordinatorTest.java | 6 + .../controller/CliquePantheonController.java | 11 +- 9 files changed, 356 insertions(+), 5 deletions(-) create mode 100644 consensus/clique/src/main/java/tech/pegasys/pantheon/consensus/clique/CliqueMiningTracker.java create mode 100644 consensus/clique/src/test/java/tech/pegasys/pantheon/consensus/clique/blockcreation/CliqueMiningCoordinatorTest.java diff --git a/consensus/clique/src/main/java/tech/pegasys/pantheon/consensus/clique/CliqueMiningTracker.java b/consensus/clique/src/main/java/tech/pegasys/pantheon/consensus/clique/CliqueMiningTracker.java new file mode 100644 index 0000000000..811b2e2b35 --- /dev/null +++ b/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 protocolContext; + + public CliqueMiningTracker( + final Address localAddress, final ProtocolContext 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); + } +} diff --git a/consensus/clique/src/main/java/tech/pegasys/pantheon/consensus/clique/blockcreation/CliqueMiningCoordinator.java b/consensus/clique/src/main/java/tech/pegasys/pantheon/consensus/clique/blockcreation/CliqueMiningCoordinator.java index 03ead46dcc..a2266cd087 100644 --- a/consensus/clique/src/main/java/tech/pegasys/pantheon/consensus/clique/blockcreation/CliqueMiningCoordinator.java +++ b/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 { + 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; } } diff --git a/consensus/clique/src/test/java/tech/pegasys/pantheon/consensus/clique/blockcreation/CliqueMiningCoordinatorTest.java b/consensus/clique/src/test/java/tech/pegasys/pantheon/consensus/clique/blockcreation/CliqueMiningCoordinatorTest.java new file mode 100644 index 0000000000..5e0dfe4aba --- /dev/null +++ b/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
validators = Lists.newArrayList(validatorAddress, proposerAddress); + + private final BlockHeaderTestFixture headerTestFixture = new BlockHeaderTestFixture(); + + private CliqueMiningTracker miningTracker; + + @Mock private MutableBlockchain blockChain; + @Mock private ProtocolContext 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 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 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 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 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())); + } +} diff --git a/ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/AbstractMiningCoordinator.java b/ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/AbstractMiningCoordinator.java index 810d907f4f..1b477e3fcb 100644 --- a/ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/AbstractMiningCoordinator.java +++ b/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>> implements BlockAddedObserver, MiningCoordinator { + private static final Logger LOG = getLogger(); protected boolean isEnabled = false; protected volatile Optional 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); } diff --git a/ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/BlockCreator.java b/ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/BlockCreator.java index b23676d202..698dc71b7b 100644 --- a/ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/BlockCreator.java +++ b/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); } diff --git a/ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/BlockMiner.java b/ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/BlockMiner.java index e8b95574c8..41816c1d78 100644 --- a/ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/BlockMiner.java +++ b/ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/BlockMiner.java @@ -65,6 +65,7 @@ public class BlockMiner> implements Runnabl @Override public void run() { + boolean blockMined = false; while (!blockMined) { try { @@ -84,6 +85,7 @@ public class BlockMiner> 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> implements Runnabl private void notifyNewBlockListeners(final Block block) { observers.forEach(obs -> obs.blockMined(block)); } + + public BlockHeader getParentHeader() { + return parentHeader; + } } diff --git a/ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/EthHashMiningCoordinator.java b/ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/EthHashMiningCoordinator.java index 92fd685142..2de4449715 100644 --- a/ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/EthHashMiningCoordinator.java +++ b/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