Post review

consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/IbftUnsignedPrePrepareMessageData.java

@@ -19,8 +19,6 @@
 import tech.pegasys.pantheon.ethereum.rlp.RLPInput;
 import tech.pegasys.pantheon.ethereum.rlp.RLPOutput;
 
-import java.util.Objects;
-
 // NOTE: Implementation of all methods of this class is still pending. This class was added to show
 // how a PreparedCertificate is encoded and decoded inside a RoundChange message
 public class IbftUnsignedPrePrepareMessageData extends AbstractIbftUnsignedInRoundMessageData {
@@ -62,22 +60,4 @@ public Block getBlock() {
   public int getMessageType() {
     return TYPE;
   }
-
-  @Override
-  public boolean equals(final Object o) {
-    if (this == o) {
-      return true;
-    }
-    if (o == null || getClass() != o.getClass()) {
-      return false;
-    }
-    IbftUnsignedPrePrepareMessageData that = (IbftUnsignedPrePrepareMessageData) o;
-    return Objects.equals(block, that.block)
-        && Objects.equals(roundIdentifier, that.roundIdentifier);
-  }
-
-  @Override
-  public int hashCode() {
-    return Objects.hash(block);
-  }
 }

consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/MessageValidator.java

@@ -68,16 +68,22 @@ public boolean addPreprepareMessage(
       final IbftSignedMessageData<IbftUnsignedPrePrepareMessageData> msg) {
 
     if (preprepareMessage.isPresent()) {
-      return handleSubsequentPreprepareMessage(msg);
+      return handleSubsequentPreprepareMessage(preprepareMessage.get(), msg);
     }
 
-    if (!msg.getUnsignedMessageData().getRoundIdentifier().equals(roundIdentifier)) {
-      LOG.info("Invalid Preprepare message, does not match current round.");
+    if (!validatePreprepareMessage(msg)) {
       return false;
     }
 
-    if (preprepareMessage.isPresent()) {
-      LOG.info("Invalid Preprepare message, one already exists for the current round.");
+    preprepareMessage = Optional.of(msg);
+    return true;
+  }
+
+  private boolean validatePreprepareMessage(
+      final IbftSignedMessageData<IbftUnsignedPrePrepareMessageData> msg) {
+
+    if (!msg.getUnsignedMessageData().getRoundIdentifier().equals(roundIdentifier)) {
+      LOG.info("Invalid Preprepare message, does not match current round.");
       return false;
     }
 
@@ -95,23 +101,21 @@ public boolean addPreprepareMessage(
       return false;
     }
 
-    preprepareMessage = Optional.of(msg);
     return true;
   }
 
   private boolean handleSubsequentPreprepareMessage(
-      final IbftSignedMessageData<IbftUnsignedPrePrepareMessageData> msg) {
-    IbftSignedMessageData<IbftUnsignedPrePrepareMessageData> existingMsg = preprepareMessage.get();
-
-    if (!existingMsg.getSender().equals(msg.getSender())) {
+      final IbftSignedMessageData<IbftUnsignedPrePrepareMessageData> existingMsg,
+      final IbftSignedMessageData<IbftUnsignedPrePrepareMessageData> newMsg) {
+    if (!existingMsg.getSender().equals(newMsg.getSender())) {
       LOG.debug("Received subsequent invalid Preprepare message; sender differs from original.");
       return false;
     }
 
     final IbftUnsignedPrePrepareMessageData existingData = existingMsg.getUnsignedMessageData();
-    final IbftUnsignedPrePrepareMessageData newData = msg.getUnsignedMessageData();
+    final IbftUnsignedPrePrepareMessageData newData = newMsg.getUnsignedMessageData();
 
-    if (!existingData.equals(newData)) {
+    if (!preprepareMessagesAreIdentical(existingData, newData)) {
       LOG.debug("Received subsequent invalid Preprepare message; content differs from original.");
       return false;
     }
@@ -123,7 +127,7 @@ public boolean validatePrepareMessage(
       final IbftSignedMessageData<IbftUnsignedPrepareMessageData> msg) {
     final String msgType = "Prepare";
 
-    if (!isMessageInRoundFromValidatorAndMatchesPreprepareDigest(msg, msgType)) {
+    if (!isMessageForCurrentRoundFromValidatorAndPreprareMessageAvailable(msg, msgType)) {
       return false;
     }
 
@@ -139,7 +143,7 @@ public boolean validateCommmitMessage(
       final IbftSignedMessageData<IbftUnsignedCommitMessageData> msg) {
     final String msgType = "Commit";
 
-    if (!isMessageInRoundFromValidatorAndMatchesPreprepareDigest(msg, msgType)) {
+    if (!isMessageForCurrentRoundFromValidatorAndPreprareMessageAvailable(msg, msgType)) {
       return false;
     }
 
@@ -156,7 +160,7 @@ public boolean validateCommmitMessage(
     return validateDigestMatchesPreprepareBlock(msg.getUnsignedMessageData().getDigest(), msgType);
   }
 
-  private boolean isMessageInRoundFromValidatorAndMatchesPreprepareDigest(
+  private boolean isMessageForCurrentRoundFromValidatorAndPreprareMessageAvailable(
       final IbftSignedMessageData<? extends AbstractIbftUnsignedInRoundMessageData> msg,
       final String msgType) {
 
@@ -191,4 +195,10 @@ private boolean validateDigestMatchesPreprepareBlock(final Hash digest, final St
     }
     return true;
   }
+
+  private boolean preprepareMessagesAreIdentical(
+      final IbftUnsignedPrePrepareMessageData right, final IbftUnsignedPrePrepareMessageData left) {
+    return right.getBlock().getHash().equals(left.getBlock().getHash())
+        && (right.getRoundIdentifier().compareTo(left.getRoundIdentifier()) == 0);
+  }
 }