Prevent the block being mined twice (#576)

Testing indicated that if quorum-1 commit messages is received prior
to the proposal, the proposed block will be imported twice.
Theoretically this is handled safely by the BlockImporter, however
is clearly an error in the IbftRound code.

This commit updates IbftRound such that the import can only occur
once.
Signed-off-by: Adrian Sutton <adrian.sutton@consensys.net>

consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftBlockHeightManager.java

@@ -236,7 +236,7 @@ public class IbftBlockHeightManager {
       if (messageAge == FUTURE_ROUND) {
         startNewRound(payload.getRoundIdentifier().getRoundNumber());
       }
-      currentRound.handleProposalMessage(payload.getProposalPayload());
+      currentRound.handleProposalFromNewRound(signedPayload);
     }
   }
 

consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftRound.java

@@ -23,6 +23,7 @@ import tech.pegasys.pantheon.consensus.ibft.blockcreation.IbftBlockCreator;
 import tech.pegasys.pantheon.consensus.ibft.network.IbftMessageTransmitter;
 import tech.pegasys.pantheon.consensus.ibft.payload.CommitPayload;
 import tech.pegasys.pantheon.consensus.ibft.payload.MessageFactory;
+import tech.pegasys.pantheon.consensus.ibft.payload.NewRoundPayload;
 import tech.pegasys.pantheon.consensus.ibft.payload.PreparePayload;
 import tech.pegasys.pantheon.consensus.ibft.payload.PreparedCertificate;
 import tech.pegasys.pantheon.consensus.ibft.payload.ProposalPayload;
@@ -136,8 +137,26 @@ public class IbftRound {
 
   public void handleProposalMessage(final SignedData<ProposalPayload> msg) {
     LOG.info("Handling a Proposal message.");
+
+    if (getRoundIdentifier().getRoundNumber() != 0) {
+      LOG.info("Illegally received a Proposal message when not in Round 0.");
+      return;
+    }
+    actionReceivedProposal(msg);
+  }
+
+  public void handleProposalFromNewRound(final SignedData<NewRoundPayload> msg) {
+    LOG.info("Handling a New Round Proposal.");
+
+    if (getRoundIdentifier().getRoundNumber() == 0) {
+      LOG.info("Illegally received a NewRound message when in Round 0.");
+      return;
+    }
+    actionReceivedProposal(msg.getPayload().getProposalPayload());
+  }
+
+  private void actionReceivedProposal(final SignedData<ProposalPayload> msg) {
     final Block block = msg.getPayload().getBlock();
-    final boolean wasCommitted = roundState.isCommitted();
 
     if (updateStateWithProposedBlock(msg)) {
       LOG.info("Sending prepare message.");
@@ -147,10 +166,6 @@ public class IbftRound {
               roundState.getRoundIdentifier(), block.getHash());
       peerIsPrepared(localPrepareMessage);
     }
-
-    if (wasCommitted != roundState.isCommitted()) {
-      importBlockToChain();
-    }
   }
 
   public void handlePrepareMessage(final SignedData<PreparePayload> msg) {
@@ -169,6 +184,7 @@ public class IbftRound {
 
   private boolean updateStateWithProposedBlock(final SignedData<ProposalPayload> msg) {
     final boolean wasPrepared = roundState.isPrepared();
+    final boolean wasCommitted = roundState.isCommitted();
     final boolean blockAccepted = roundState.setProposedBlock(msg);
     if (blockAccepted) {
       // There are times handling a proposed block is enough to enter prepared.
@@ -177,6 +193,10 @@ public class IbftRound {
         final Block block = roundState.getProposedBlock().get();
         transmitter.multicastCommit(getRoundIdentifier(), block.getHash(), createCommitSeal(block));
       }
+      if (wasCommitted != roundState.isCommitted()) {
+        importBlockToChain();
+      }
+
       final SignedData<CommitPayload> localCommitMessage =
           messageFactory.createSignedCommitPayload(
               roundState.getRoundIdentifier(),

consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftRoundTest.java

@@ -67,7 +67,7 @@ import org.mockito.junit.MockitoJUnitRunner;
 public class IbftRoundTest {
 
   private final KeyPair localNodeKeys = KeyPair.generate();
-  private final ConsensusRoundIdentifier roundIdentifier = new ConsensusRoundIdentifier(1, 1);
+  private final ConsensusRoundIdentifier roundIdentifier = new ConsensusRoundIdentifier(1, 0);
   private final MessageFactory messageFactory = new MessageFactory(localNodeKeys);
   private final Subscribers<MinedBlockObserver> subscribers = new Subscribers<>();
   private ProtocolContext<IbftContext> protocolContext;
@@ -375,4 +375,55 @@ public class IbftRoundTest {
     round.createAndSendProposalMessage(15);
     verify(minedBlockObserver).blockMined(any());
   }
+
+  @Test
+  public void blockIsOnlyImportedOnceWhenCommitsAreReceivedBeforeProposal() {
+    final ConsensusRoundIdentifier roundIdentifier = new ConsensusRoundIdentifier(1, 0);
+    final int QUORUM_SIZE = 2;
+    final RoundState roundState = new RoundState(roundIdentifier, QUORUM_SIZE, messageValidator);
+    final IbftRound round =
+        new IbftRound(
+            roundState,
+            blockCreator,
+            protocolContext,
+            blockImporter,
+            subscribers,
+            localNodeKeys,
+            messageFactory,
+            transmitter);
+
+    round.handleCommitMessage(
+        messageFactory.createSignedCommitPayload(
+            roundIdentifier, proposedBlock.getHash(), remoteCommitSeal));
+
+    round.handleProposalMessage(
+        messageFactory.createSignedProposalPayload(roundIdentifier, proposedBlock));
+
+    verify(blockImporter, times(1)).importBlock(any(), any(), any());
+  }
+
+  @Test
+  public void blockIsImportedOnlyOnceIfQuorumCommitsAreReceivedPriorToProposal() {
+    final int QUORUM_SIZE = 1;
+    final RoundState roundState = new RoundState(roundIdentifier, QUORUM_SIZE, messageValidator);
+    final IbftRound round =
+        new IbftRound(
+            roundState,
+            blockCreator,
+            protocolContext,
+            blockImporter,
+            subscribers,
+            localNodeKeys,
+            messageFactory,
+            transmitter);
+
+    round.handleCommitMessage(
+        messageFactory.createSignedCommitPayload(
+            roundIdentifier, proposedBlock.getHash(), remoteCommitSeal));
+
+    round.handleProposalMessage(
+        messageFactory.createSignedProposalPayload(roundIdentifier, proposedBlock));
+
+    verify(blockImporter, times(1)).importBlock(any(), any(), any());
+  }
 }