NewRoundMessageValidator ignores Round Number when comparing blocks (#523)

* NewRoundMessageValidator ignores Round Number when comparing blocks

NewRoundMessageValidator previously ensured the block in the newest
PreparedCertificate was the same as that in the Proposal by comparing
block hashes.
However, the hash-function for received IBFT blocks includes round
number - thus the blocks will never be deemed 'equal'.

As such, the blocks must be compared using a hash function which
ignores the round number - i.e. the OnChain function.

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

@@ -16,6 +16,7 @@ import static tech.pegasys.pantheon.consensus.ibft.IbftHelpers.findLatestPrepare
 import static tech.pegasys.pantheon.consensus.ibft.IbftHelpers.prepareMessageCountForQuorum;
 
 import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier;
+import tech.pegasys.pantheon.consensus.ibft.IbftBlockHashing;
 import tech.pegasys.pantheon.consensus.ibft.blockcreation.ProposerSelector;
 import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.NewRoundPayload;
 import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.PreparedCertificate;
@@ -25,6 +26,7 @@ import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.RoundChangePayload;
 import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.SignedData;
 import tech.pegasys.pantheon.consensus.ibft.validation.RoundChangeMessageValidator.MessageValidatorForHeightFactory;
 import tech.pegasys.pantheon.ethereum.core.Address;
+import tech.pegasys.pantheon.ethereum.core.Hash;
 
 import java.util.Collection;
 import java.util.Optional;
@@ -77,15 +79,11 @@ public class NewRoundMessageValidator {
       return false;
     }
 
-    final SignedData<ProposalPayload> proposalMessage = payload.getProposalPayload();
+    final SignedData<ProposalPayload> proposalPayload = payload.getProposalPayload();
-
+    final MessageValidator proposalValidator =
-    if (!msg.getSender().equals(proposalMessage.getSender())) {
+        messageValidatorFactory.createAt(rootRoundIdentifier);
-      LOG.info("Invalid NewRound message, embedded Proposal message not signed by sender.");
+    if (!proposalValidator.addSignedProposalPayload(proposalPayload)) {
-      return false;
+      LOG.info("Invalid NewRound message, embedded proposal failed validation");
-    }
-
-    if (!proposalMessage.getPayload().getRoundIdentifier().equals(rootRoundIdentifier)) {
-      LOG.info("Invalid NewRound message, embedded Proposal has mismatched round.");
       return false;
     }
 
@@ -152,13 +150,22 @@ public class NewRoundMessageValidator {
           "No round change messages have a preparedCertificate, any valid block may be proposed.");
       return true;
     }
-    if (!latestPreparedCertificate
+
-        .get()
+    // Get the hash of the block in latest prepareCert, not including the Round field.
-        .getProposalPayload()
+    final Hash roundAgnosticBlockHashPreparedCert =
-        .getPayload()
+        IbftBlockHashing.calculateHashOfIbftBlockOnChain(
-        .getBlock()
+            latestPreparedCertificate
-        .getHash()
+                .get()
-        .equals(payload.getProposalPayload().getPayload().getBlock().getHash())) {
+                .getProposalPayload()
+                .getPayload()
+                .getBlock()
+                .getHeader());
+
+    final Hash roundAgnosticBlockHashProposal =
+        IbftBlockHashing.calculateHashOfIbftBlockOnChain(
+            payload.getProposalPayload().getPayload().getBlock().getHeader());
+
+    if (!roundAgnosticBlockHashPreparedCert.equals(roundAgnosticBlockHashProposal)) {
       LOG.info(
           "Invalid NewRound message, block in latest RoundChange does not match proposed block.");
       return false;

consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/validation/NewRoundMessageValidatorTest.java

@@ -31,7 +31,6 @@ import tech.pegasys.pantheon.consensus.ibft.validation.RoundChangeMessageValidat
 import tech.pegasys.pantheon.crypto.SECP256K1.KeyPair;
 import tech.pegasys.pantheon.ethereum.core.Address;
 import tech.pegasys.pantheon.ethereum.core.Block;
-import tech.pegasys.pantheon.ethereum.core.Hash;
 import tech.pegasys.pantheon.ethereum.core.Util;
 
 import java.util.Collections;
@@ -49,7 +48,6 @@ public class NewRoundMessageValidatorTest {
   private final KeyPair otherValidatorKey = KeyPair.generate();
   private final MessageFactory proposerMessageFactory = new MessageFactory(proposerKey);
   private final MessageFactory validatorMessageFactory = new MessageFactory(validatorKey);
-  private final MessageFactory otherValidatorMessageFactory = new MessageFactory(otherValidatorKey);
   private final Address proposerAddress = Util.publicKeyToAddress(proposerKey.getPublicKey());
   private final List<Address> validators = Lists.newArrayList();
   private final long chainHeight = 2;
@@ -57,16 +55,14 @@ public class NewRoundMessageValidatorTest {
       new ConsensusRoundIdentifier(chainHeight, 4);
   private NewRoundMessageValidator validator;
 
-  private final Block proposedBlock = mock(Block.class);
   private final ProposerSelector proposerSelector = mock(ProposerSelector.class);
   private final MessageValidatorForHeightFactory validatorFactory =
       mock(MessageValidatorForHeightFactory.class);
   private final MessageValidator messageValidator = mock(MessageValidator.class);
 
-  private final SignedData<NewRoundPayload> validMsg =
+  private Block proposedBlock;
-      createValidNewRoundMessageSignedBy(proposerKey);
+  private SignedData<NewRoundPayload> validMsg;
-  private final NewRoundPayload.Builder msgBuilder =
+  private NewRoundPayload.Builder msgBuilder;
-      NewRoundPayload.Builder.fromExisting(validMsg.getPayload());
 
   @Before
   public void setup() {
@@ -74,14 +70,16 @@ public class NewRoundMessageValidatorTest {
     validators.add(Util.publicKeyToAddress(validatorKey.getPublicKey()));
     validators.add(Util.publicKeyToAddress(otherValidatorKey.getPublicKey()));
 
+    proposedBlock = TestHelpers.createProposalBlock(validators, roundIdentifier.getRoundNumber());
+    validMsg = createValidNewRoundMessageSignedBy(proposerKey);
+    msgBuilder = NewRoundPayload.Builder.fromExisting(validMsg.getPayload());
+
     when(proposerSelector.selectProposerForRound(any())).thenReturn(proposerAddress);
 
     when(validatorFactory.createAt(any())).thenReturn(messageValidator);
     when(messageValidator.addSignedProposalPayload(any())).thenReturn(true);
     when(messageValidator.validatePrepareMessage(any())).thenReturn(true);
 
-    when(proposedBlock.getHash()).thenReturn(Hash.fromHexStringLenient("1"));
-
     validator =
         new NewRoundMessageValidator(
             validators, proposerSelector, validatorFactory, 1, chainHeight);
@@ -147,16 +145,6 @@ public class NewRoundMessageValidatorTest {
     assertThat(validator.validateNewRoundMessage(inValidMsg)).isFalse();
   }
 
-  @Test
-  public void newRoundContainingProposalFromDifferentValidatorFails() {
-    msgBuilder.setProposalPayload(
-        validatorMessageFactory.createSignedProposalPayload(roundIdentifier, proposedBlock));
-
-    final SignedData<NewRoundPayload> inValidMsg = signPayload(msgBuilder.build(), proposerKey);
-
-    assertThat(validator.validateNewRoundMessage(inValidMsg)).isFalse();
-  }
-
   @Test
   public void newRoundWithEmptyRoundChangeCertificateFails() {
     msgBuilder.setRoundChangeCertificate(new RoundChangeCertificate(Collections.emptyList()));
@@ -166,23 +154,14 @@ public class NewRoundMessageValidatorTest {
     assertThat(validator.validateNewRoundMessage(inValidMsg)).isFalse();
   }
 
-  @Test
-  public void newRoundWithMismatchOfRoundIdentifierToProposalMessageFails() {
-    final ConsensusRoundIdentifier mismatchedRound = TestHelpers.createFrom(roundIdentifier, 0, -1);
-
-    msgBuilder.setProposalPayload(
-        proposerMessageFactory.createSignedProposalPayload(mismatchedRound, proposedBlock));
-
-    final SignedData<NewRoundPayload> inValidMsg = signPayload(msgBuilder.build(), proposerKey);
-
-    assertThat(validator.validateNewRoundMessage(inValidMsg)).isFalse();
-  }
-
   @Test
   public void newRoundWithProposalNotMatchingLatestRoundChangeFails() {
     final ConsensusRoundIdentifier preparedRound = TestHelpers.createFrom(roundIdentifier, 0, -1);
-    final Block prevProposedBlock = mock(Block.class);
+    // The previous block has been constructed with less validators, so is thus not identical
-    when(prevProposedBlock.getHash()).thenReturn(Hash.fromHexStringLenient("2"));
+    // to the block in the new proposal (so should fail).
+    final Block prevProposedBlock =
+        TestHelpers.createProposalBlock(validators.subList(0, 1), roundIdentifier.getRoundNumber());
+
     final PreparedCertificate misMatchedPreparedCertificate =
         new PreparedCertificate(
             proposerMessageFactory.createSignedProposalPayload(preparedRound, prevProposedBlock),
@@ -289,4 +268,25 @@ public class NewRoundMessageValidatorTest {
 
     assertThat(validator.validateNewRoundMessage(msg)).isTrue();
   }
+
+  @Test
+  public void embeddedProposalFailsValidation() {
+    when(messageValidator.addSignedProposalPayload(any())).thenReturn(false, true);
+
+    final SignedData<ProposalPayload> proposal =
+        proposerMessageFactory.createSignedProposalPayload(roundIdentifier, proposedBlock);
+
+    final SignedData<NewRoundPayload> msg =
+        proposerMessageFactory.createSignedNewRoundPayload(
+            roundIdentifier,
+            new RoundChangeCertificate(
+                Lists.newArrayList(
+                    proposerMessageFactory.createSignedRoundChangePayload(
+                        roundIdentifier, Optional.empty()),
+                    validatorMessageFactory.createSignedRoundChangePayload(
+                        roundIdentifier, Optional.empty()))),
+            proposal);
+
+    assertThat(validator.validateNewRoundMessage(msg)).isFalse();
+  }
 }