Update RoundChangeManager correctly create its message validator (#450)

The RoundChangeManager was incorrectly creating its internal
RoundChangeMessageValidator with "quorum" number of Prepare messages
required - however, given the ProposalMessage in the Prepared-
Certificate represents a valid node.
Signed-off-by: Adrian Sutton <adrian.sutton@consensys.net>

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

@@ -89,7 +89,7 @@ public class RoundChangeManager {
     this.quorumSize = IbftHelpers.calculateRequiredValidatorQuorum(validators.size());
     this.roundChangeMessageValidator =
         new RoundChangeMessageValidator(
-            messageValidityFactory, validators, quorumSize, sequenceNumber);
+            messageValidityFactory, validators, quorumSize - 1, sequenceNumber);
   }
 
   /**

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

@@ -99,7 +99,7 @@ public class MessageValidator {
     final Block proposedBlock = msg.getPayload().getBlock();
     if (!headerValidator.validateHeader(
         proposedBlock.getHeader(), parentHeader, protocolContext, FULL)) {
-      LOG.info("Invalid Prepare message, block did not pass header validation.");
+      LOG.info("Invalid Proposal message, block did not pass header validation.");
       return false;
     }
 

consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/TestHelpers.java

@@ -12,18 +12,16 @@
  */
 package tech.pegasys.pantheon.consensus.ibft;
 
-import static java.util.Collections.singletonList;
-
 import tech.pegasys.pantheon.ethereum.core.Address;
 import tech.pegasys.pantheon.ethereum.core.Block;
 import tech.pegasys.pantheon.ethereum.core.BlockDataGenerator;
 import tech.pegasys.pantheon.ethereum.core.BlockDataGenerator.BlockOptions;
 import tech.pegasys.pantheon.util.bytes.BytesValue;
 
+import java.util.Collections;
+import java.util.List;
 import java.util.Optional;
 
-import com.google.common.collect.Lists;
-
 public class TestHelpers {
 
   public static ConsensusRoundIdentifier createFrom(
@@ -32,14 +30,14 @@ public class TestHelpers {
         initial.getSequenceNumber() + offSetSequence, initial.getRoundNumber() + offSetRound);
   }
 
-  public static Block createProposalBlock() {
+  public static Block createProposalBlock(final List<Address> validators, final int round) {
     final BytesValue extraData =
         new IbftExtraData(
                 BytesValue.wrap(new byte[32]),
-                Lists.newArrayList(),
+                Collections.emptyList(),
                 Optional.empty(),
-                0,
-                singletonList(Address.fromHexString(String.format("%020d", 1))))
+                round,
+                validators)
             .encode();
     final BlockOptions blockOptions =
         BlockOptions.create()

consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/NewRoundPayloadTest.java

@@ -12,12 +12,14 @@
  */
 package tech.pegasys.pantheon.consensus.ibft.ibftmessagedata;
 
+import static java.util.Collections.singletonList;
 import static org.assertj.core.api.Assertions.assertThat;
 
 import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier;
 import tech.pegasys.pantheon.consensus.ibft.TestHelpers;
 import tech.pegasys.pantheon.consensus.ibft.ibftmessage.IbftV2;
 import tech.pegasys.pantheon.crypto.SECP256K1.Signature;
+import tech.pegasys.pantheon.ethereum.core.AddressHelpers;
 import tech.pegasys.pantheon.ethereum.core.Block;
 import tech.pegasys.pantheon.ethereum.core.Hash;
 import tech.pegasys.pantheon.ethereum.rlp.BytesValueRLPOutput;
@@ -32,12 +34,14 @@ import org.assertj.core.util.Lists;
 import org.junit.Test;
 
 public class NewRoundPayloadTest {
+
   private static final ConsensusRoundIdentifier ROUND_IDENTIFIER =
       new ConsensusRoundIdentifier(0x1234567890ABCDEFL, 0xFEDCBA98);
 
   @Test
   public void roundTripRlpWithNoRoundChangePayloads() {
-    final Block block = TestHelpers.createProposalBlock();
+    final Block block =
+        TestHelpers.createProposalBlock(singletonList(AddressHelpers.ofValue(1)), 0);
     final ProposalPayload proposalPayload = new ProposalPayload(ROUND_IDENTIFIER, block);
     final Signature signature = Signature.create(BigInteger.ONE, BigInteger.TEN, (byte) 0);
     final SignedData<ProposalPayload> proposalPayloadSignedData =
@@ -60,7 +64,8 @@ public class NewRoundPayloadTest {
 
   @Test
   public void roundTripRlpWithRoundChangePayloads() {
-    final Block block = TestHelpers.createProposalBlock();
+    final Block block =
+        TestHelpers.createProposalBlock(singletonList(AddressHelpers.ofValue(1)), 0);
     final ProposalPayload proposalPayload = new ProposalPayload(ROUND_IDENTIFIER, block);
     final Signature signature = Signature.create(BigInteger.ONE, BigInteger.TEN, (byte) 0);
     final SignedData<ProposalPayload> signedProposal = SignedData.from(proposalPayload, signature);

consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/PreparedCertificateTest.java

@@ -12,11 +12,13 @@
  */
 package tech.pegasys.pantheon.consensus.ibft.ibftmessagedata;
 
+import static java.util.Collections.singletonList;
 import static org.assertj.core.api.Assertions.assertThat;
 
 import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier;
 import tech.pegasys.pantheon.consensus.ibft.TestHelpers;
 import tech.pegasys.pantheon.crypto.SECP256K1.Signature;
+import tech.pegasys.pantheon.ethereum.core.AddressHelpers;
 import tech.pegasys.pantheon.ethereum.core.Block;
 import tech.pegasys.pantheon.ethereum.core.Hash;
 import tech.pegasys.pantheon.ethereum.rlp.BytesValueRLPOutput;
@@ -31,6 +33,7 @@ import org.assertj.core.util.Lists;
 import org.junit.Test;
 
 public class PreparedCertificateTest {
+
   private static final ConsensusRoundIdentifier ROUND_IDENTIFIER =
       new ConsensusRoundIdentifier(0x1234567890ABCDEFL, 0xFEDCBA98);
 
@@ -74,7 +77,8 @@ public class PreparedCertificateTest {
   }
 
   private SignedData<ProposalPayload> signedProposal() {
-    final Block block = TestHelpers.createProposalBlock();
+    final Block block =
+        TestHelpers.createProposalBlock(singletonList(AddressHelpers.ofValue(1)), 0);
     final ProposalPayload proposalPayload = new ProposalPayload(ROUND_IDENTIFIER, block);
     final Signature signature = Signature.create(BigInteger.ONE, BigInteger.TEN, (byte) 0);
     return SignedData.from(proposalPayload, signature);

consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/ProposalPayloadTest.java

@@ -12,11 +12,13 @@
  */
 package tech.pegasys.pantheon.consensus.ibft.ibftmessagedata;
 
+import static java.util.Collections.singletonList;
 import static org.assertj.core.api.Assertions.assertThat;
 
 import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier;
 import tech.pegasys.pantheon.consensus.ibft.TestHelpers;
 import tech.pegasys.pantheon.consensus.ibft.ibftmessage.IbftV2;
+import tech.pegasys.pantheon.ethereum.core.AddressHelpers;
 import tech.pegasys.pantheon.ethereum.core.Block;
 import tech.pegasys.pantheon.ethereum.rlp.BytesValueRLPOutput;
 import tech.pegasys.pantheon.ethereum.rlp.RLP;
@@ -25,12 +27,14 @@ import tech.pegasys.pantheon.ethereum.rlp.RLPInput;
 import org.junit.Test;
 
 public class ProposalPayloadTest {
+
   private static final ConsensusRoundIdentifier ROUND_IDENTIFIER =
       new ConsensusRoundIdentifier(0x1234567890ABCDEFL, 0xFEDCBA98);
 
   @Test
   public void roundTripRlp() {
-    final Block block = TestHelpers.createProposalBlock();
+    final Block block =
+        TestHelpers.createProposalBlock(singletonList(AddressHelpers.ofValue(1)), 0);
     final ProposalPayload expectedProposalPayload = new ProposalPayload(ROUND_IDENTIFIER, block);
     final BytesValueRLPOutput rlpOut = new BytesValueRLPOutput();
     expectedProposalPayload.writeTo(rlpOut);

consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/RoundChangeCertificateTest.java

@@ -12,12 +12,14 @@
  */
 package tech.pegasys.pantheon.consensus.ibft.ibftmessagedata;
 
+import static java.util.Collections.singletonList;
 import static org.assertj.core.api.Assertions.assertThat;
 
 import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier;
 import tech.pegasys.pantheon.consensus.ibft.TestHelpers;
 import tech.pegasys.pantheon.consensus.ibft.ibftmessage.IbftV2;
 import tech.pegasys.pantheon.crypto.SECP256K1.Signature;
+import tech.pegasys.pantheon.ethereum.core.AddressHelpers;
 import tech.pegasys.pantheon.ethereum.core.Block;
 import tech.pegasys.pantheon.ethereum.core.Hash;
 import tech.pegasys.pantheon.ethereum.rlp.BytesValueRLPOutput;
@@ -51,7 +53,8 @@ public class RoundChangeCertificateTest {
 
   @Test
   public void rlpRoundTripWithPreparedCertificate() {
-    final Block block = TestHelpers.createProposalBlock();
+    final Block block =
+        TestHelpers.createProposalBlock(singletonList(AddressHelpers.ofValue(1)), 0);
     final ProposalPayload proposalPayload = new ProposalPayload(ROUND_IDENTIFIER, block);
     final Signature signature = Signature.create(BigInteger.ONE, BigInteger.TEN, (byte) 0);
     SignedData<ProposalPayload> signedProposal = SignedData.from(proposalPayload, signature);

consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/RoundChangePayloadTest.java

@@ -12,6 +12,7 @@
  */
 package tech.pegasys.pantheon.consensus.ibft.ibftmessagedata;
 
+import static java.util.Collections.singletonList;
 import static java.util.Optional.empty;
 import static org.assertj.core.api.Assertions.assertThat;
 
@@ -19,6 +20,7 @@ import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier;
 import tech.pegasys.pantheon.consensus.ibft.TestHelpers;
 import tech.pegasys.pantheon.consensus.ibft.ibftmessage.IbftV2;
 import tech.pegasys.pantheon.crypto.SECP256K1.Signature;
+import tech.pegasys.pantheon.ethereum.core.AddressHelpers;
 import tech.pegasys.pantheon.ethereum.core.Block;
 import tech.pegasys.pantheon.ethereum.core.Hash;
 import tech.pegasys.pantheon.ethereum.rlp.BytesValueRLPOutput;
@@ -33,6 +35,7 @@ import org.assertj.core.util.Lists;
 import org.junit.Test;
 
 public class RoundChangePayloadTest {
+
   private static final ConsensusRoundIdentifier ROUND_IDENTIFIER =
       new ConsensusRoundIdentifier(0x1234567890ABCDEFL, 0xFEDCBA98);
   private static final Signature SIGNATURE =
@@ -95,7 +98,8 @@ public class RoundChangePayloadTest {
   }
 
   private SignedData<ProposalPayload> signedProposal() {
-    final Block block = TestHelpers.createProposalBlock();
+    final Block block =
+        TestHelpers.createProposalBlock(singletonList(AddressHelpers.ofValue(1)), 0);
     final ProposalPayload proposalPayload = new ProposalPayload(ROUND_IDENTIFIER, block);
     final Signature signature = Signature.create(BigInteger.ONE, BigInteger.TEN, (byte) 0);
     return SignedData.from(proposalPayload, signature);

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

@@ -13,11 +13,17 @@
 package tech.pegasys.pantheon.consensus.ibft.statemachine;
 
 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.when;
 
 import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier;
 import tech.pegasys.pantheon.consensus.ibft.IbftContext;
+import tech.pegasys.pantheon.consensus.ibft.TestHelpers;
 import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.MessageFactory;
+import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.PreparePayload;
+import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.PreparedCertificate;
+import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.ProposalPayload;
 import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.RoundChangePayload;
 import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.SignedData;
 import tech.pegasys.pantheon.consensus.ibft.validation.MessageValidator;
@@ -25,15 +31,19 @@ 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.BlockHeader;
 import tech.pegasys.pantheon.ethereum.core.Util;
 import tech.pegasys.pantheon.ethereum.db.WorldStateArchive;
 import tech.pegasys.pantheon.ethereum.mainnet.BlockHeaderValidator;
 
+import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
+import java.util.stream.Collectors;
 
+import com.google.common.base.Preconditions;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
 import org.junit.Before;
@@ -51,10 +61,10 @@ public class RoundChangeManagerTest {
   private final ConsensusRoundIdentifier ri1 = new ConsensusRoundIdentifier(2, 1);
   private final ConsensusRoundIdentifier ri2 = new ConsensusRoundIdentifier(2, 2);
   private final ConsensusRoundIdentifier ri3 = new ConsensusRoundIdentifier(2, 3);
+  private final List<Address> validators = Lists.newArrayList();
 
   @Before
   public void setup() {
-    List<Address> validators = Lists.newArrayList();
 
     validators.add(Util.publicKeyToAddress(proposerKey.getPublicKey()));
     validators.add(Util.publicKeyToAddress(validator1Key.getPublicKey()));
@@ -67,6 +77,7 @@ public class RoundChangeManagerTest {
     @SuppressWarnings("unchecked")
     BlockHeaderValidator<IbftContext> headerValidator =
         (BlockHeaderValidator<IbftContext>) mock(BlockHeaderValidator.class);
+    when(headerValidator.validateHeader(any(), any(), any(), any())).thenReturn(true);
     BlockHeader parentHeader = mock(BlockHeader.class);
 
     Map<ConsensusRoundIdentifier, MessageValidator> messageValidators = Maps.newHashMap();
@@ -110,6 +121,35 @@ public class RoundChangeManagerTest {
     return messageFactory.createSignedRoundChangePayload(round, Optional.empty());
   }
 
+  private SignedData<RoundChangePayload> makeRoundChangeMessageWithPreparedCert(
+      final KeyPair key,
+      final ConsensusRoundIdentifier round,
+      final List<KeyPair> prepareProviders) {
+    Preconditions.checkArgument(!prepareProviders.contains(key));
+
+    final MessageFactory messageFactory = new MessageFactory(key);
+
+    final ConsensusRoundIdentifier proposalRound = TestHelpers.createFrom(round, 0, -1);
+    final Block block = TestHelpers.createProposalBlock(validators, proposalRound.getRoundNumber());
+    // Proposal must come from an earlier round.
+    final SignedData<ProposalPayload> proposal =
+        messageFactory.createSignedProposalPayload(proposalRound, block);
+
+    final List<SignedData<PreparePayload>> preparePayloads =
+        prepareProviders
+            .stream()
+            .map(
+                k -> {
+                  final MessageFactory prepareFactory = new MessageFactory(k);
+                  return prepareFactory.createSignedPreparePayload(proposalRound, block.getHash());
+                })
+            .collect(Collectors.toList());
+
+    final PreparedCertificate cert = new PreparedCertificate(proposal, preparePayloads);
+
+    return messageFactory.createSignedRoundChangePayload(round, Optional.of(cert));
+  }
+
   @Test
   public void rejectsInvalidRoundChangeMessage() {
     SignedData<RoundChangePayload> roundChangeData = makeRoundChangeMessage(nonValidatorKey, ri1);
@@ -193,4 +233,22 @@ public class RoundChangeManagerTest {
         .isEqualTo(Optional.empty());
     assertThat(manager.roundChangeCache.get(ri2).receivedMessages.size()).isEqualTo(2);
   }
+
+  @Test
+  public void roundChangeMessagesWithPreparedCertificateMustHaveSufficientPrepareMessages() {
+    // Specifically, prepareMessage count is ONE LESS than the calculated quorum size (as the
+    // proposal acts as the extra msg).
+    // There are 3 validators, therefore, should only need 2 prepare message to be acceptable.
+
+    // These tests are run at ri2, such that validators can be found for past round at ri1.
+    SignedData<RoundChangePayload> roundChangeData =
+        makeRoundChangeMessageWithPreparedCert(proposerKey, ri2, Collections.emptyList());
+    assertThat(manager.appendRoundChangeMessage(roundChangeData)).isEmpty();
+    assertThat(manager.roundChangeCache.get(ri2)).isNull();
+
+    roundChangeData =
+        makeRoundChangeMessageWithPreparedCert(proposerKey, ri2, Lists.newArrayList(validator1Key));
+    assertThat(manager.appendRoundChangeMessage(roundChangeData)).isEmpty();
+    assertThat(manager.roundChangeCache.get(ri2).receivedMessages.size()).isEqualTo(1);
+  }
 }