Ibft Int. testing - Round Change (#537)

Integration tests have been added to determine the IBFT components
response to the reception of RoundChange messages.

Through the course of creating these tests the following changes were
made to the RoundChangeManager:
* Stores RoundChangeMessages in the order of arrival
* Discards subsequent RoundChange messages from the same node,
  targeting a specific round.

Signed-off-by: Adrian Sutton <adrian.sutton@consensys.net>

consensus/ibft/src/integration-test/java/tech/pegasys/pantheon/consensus/ibft/support/MessageReceptionHelpers.java

@@ -13,6 +13,7 @@
 package tech.pegasys.pantheon.consensus.ibft.support;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Fail.fail;
 
 import tech.pegasys.pantheon.consensus.ibft.ibftmessage.CommitMessageData;
 import tech.pegasys.pantheon.consensus.ibft.ibftmessage.IbftV2;
@@ -48,29 +49,38 @@ public class MessageReceptionHelpers {
           n -> {
             final List<MessageData> rxMsgs = n.getReceivedMessages();
             final MessageData rxMsgData = rxMsgs.get(index);
-            assertThat(msgMatchesExpected(rxMsgData, msg)).isTrue();
+            messageMatchesExpected(rxMsgData, msg);
           });
     }
     allPeers.forEach(p -> p.clearReceivedMessages());
   }
 
-  public static boolean msgMatchesExpected(
-      final MessageData actual, final SignedData<? extends Payload> expected) {
-    final Payload expectedPayload = expected.getPayload();
+  public static void messageMatchesExpected(
+      final MessageData actual, final SignedData<? extends Payload> signedExpectedPayload) {
+    final Payload expectedPayload = signedExpectedPayload.getPayload();
+    SignedData<?> actualSignedPayload = null;
 
     switch (expectedPayload.getMessageType()) {
       case IbftV2.PROPOSAL:
-        return ProposalMessageData.fromMessageData(actual).decode().equals(expected);
+        actualSignedPayload = ProposalMessageData.fromMessageData(actual).decode();
+        break;
       case IbftV2.PREPARE:
-        return PrepareMessageData.fromMessageData(actual).decode().equals(expected);
+        actualSignedPayload = PrepareMessageData.fromMessageData(actual).decode();
+        break;
       case IbftV2.COMMIT:
-        return CommitMessageData.fromMessageData(actual).decode().equals(expected);
+        actualSignedPayload = CommitMessageData.fromMessageData(actual).decode();
+        break;
       case IbftV2.NEW_ROUND:
-        return NewRoundMessageData.fromMessageData(actual).decode().equals(expected);
+        actualSignedPayload = NewRoundMessageData.fromMessageData(actual).decode();
+        break;
       case IbftV2.ROUND_CHANGE:
-        return RoundChangeMessageData.fromMessageData(actual).decode().equals(expected);
+        actualSignedPayload = RoundChangeMessageData.fromMessageData(actual).decode();
+        break;
       default:
-        return false;
+        fail("Illegal IBFTV2 message type.");
+        break;
     }
+    assertThat(signedExpectedPayload)
+        .isEqualToComparingFieldByFieldRecursively(actualSignedPayload);
   }
 }

consensus/ibft/src/integration-test/java/tech/pegasys/pantheon/consensus/ibft/support/RoundSpecificNodeRoles.java

@@ -13,6 +13,7 @@
 package tech.pegasys.pantheon.consensus.ibft.support;
 
 import java.util.Collection;
+import java.util.Collections;
 import java.util.List;
 
 public class RoundSpecificNodeRoles {
@@ -35,11 +36,11 @@ public class RoundSpecificNodeRoles {
   }
 
   public Collection<ValidatorPeer> getAllPeers() {
-    return peers;
+    return Collections.unmodifiableCollection(peers);
   }
 
   public List<ValidatorPeer> getNonProposingPeers() {
-    return nonProposingPeers;
+    return Collections.unmodifiableList(nonProposingPeers);
   }
 
   public ValidatorPeer getNonProposingPeer(final int index) {

consensus/ibft/src/integration-test/java/tech/pegasys/pantheon/consensus/ibft/support/TestContext.java

@@ -35,7 +35,7 @@ NetworkLayout and RoundSpecificNodeRoles concepts.
  */
 public class TestContext {
 
-  private Map<Address, ValidatorPeer> remotePeers;
+  private final Map<Address, ValidatorPeer> remotePeers;
   private final MutableBlockchain blockchain;
   private final IbftController controller;
   private final IbftFinalState finalState;

consensus/ibft/src/integration-test/java/tech/pegasys/pantheon/consensus/ibft/support/ValidatorPeer.java

@@ -53,7 +53,7 @@ public class ValidatorPeer {
   private final KeyPair nodeKeys;
   private final MessageFactory messageFactory;
   private final PeerConnection peerConnection = new StubbedPeerConnection();
-  private List<MessageData> receivedMessages = Lists.newArrayList();
+  private final List<MessageData> receivedMessages = Lists.newArrayList();
 
   private final IbftController localNodeController;
 

consensus/ibft/src/integration-test/java/tech/pegasys/pantheon/consensus/ibft/tests/RoundChangeTest.java

@@ -0,0 +1,321 @@
+/*
+ * Copyright 2019 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.ibft.tests;
+
+import static java.util.Optional.empty;
+import static tech.pegasys.pantheon.consensus.ibft.support.MessageReceptionHelpers.assertPeersReceivedExactly;
+import static tech.pegasys.pantheon.consensus.ibft.support.MessageReceptionHelpers.assertPeersReceivedNoMessages;
+
+import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier;
+import tech.pegasys.pantheon.consensus.ibft.IbftHelpers;
+import tech.pegasys.pantheon.consensus.ibft.ibftevent.RoundExpiry;
+import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.MessageFactory;
+import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.NewRoundPayload;
+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.RoundChangeCertificate;
+import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.RoundChangePayload;
+import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.SignedData;
+import tech.pegasys.pantheon.consensus.ibft.support.RoundSpecificNodeRoles;
+import tech.pegasys.pantheon.consensus.ibft.support.TestContext;
+import tech.pegasys.pantheon.consensus.ibft.support.TestContextFactory;
+import tech.pegasys.pantheon.consensus.ibft.support.ValidatorPeer;
+import tech.pegasys.pantheon.ethereum.core.Block;
+
+import java.time.Clock;
+import java.time.Instant;
+import java.time.ZoneId;
+import java.util.List;
+import java.util.Optional;
+import java.util.stream.Collectors;
+
+import com.google.common.collect.Lists;
+import org.junit.Before;
+import org.junit.Test;
+
+public class RoundChangeTest {
+
+  private final long blockTimeStamp = 100;
+  private final Clock fixedClock =
+      Clock.fixed(Instant.ofEpochSecond(blockTimeStamp), ZoneId.systemDefault());
+
+  private final int NETWORK_SIZE = 5;
+
+  private final TestContext context =
+      TestContextFactory.createTestEnvironment(NETWORK_SIZE, 0, fixedClock);
+  private final ConsensusRoundIdentifier roundId = new ConsensusRoundIdentifier(1, 0);
+  private final RoundSpecificNodeRoles roles = context.getRoundSpecificRoles(roundId);
+
+  private final MessageFactory localNodeMessageFactory = context.getLocalNodeMessageFactory();
+
+  private final Block blockToPropose = context.createBlockForProposal(0, 15);
+
+  @Before
+  public void setup() {
+    context.getController().start();
+  }
+
+  @Test
+  public void onRoundChangeTimerExpiryEventRoundChangeMessageIsSent() {
+
+    // NOTE: The prepare certificate will be empty as insufficient Prepare msgs have been received.
+    final ConsensusRoundIdentifier targetRound = new ConsensusRoundIdentifier(1, 1);
+    final SignedData<RoundChangePayload> expectedTxRoundChange =
+        localNodeMessageFactory.createSignedRoundChangePayload(targetRound, empty());
+    context.getController().handleRoundExpiry(new RoundExpiry(roundId));
+    assertPeersReceivedExactly(roles.getAllPeers(), expectedTxRoundChange);
+  }
+
+  @Test
+  public void roundChangeHasEmptyCertificateIfNoPrepareMessagesReceived() {
+    final ConsensusRoundIdentifier targetRound = new ConsensusRoundIdentifier(1, 1);
+    final SignedData<RoundChangePayload> expectedTxRoundChange =
+        localNodeMessageFactory.createSignedRoundChangePayload(targetRound, empty());
+
+    roles.getProposer().injectProposal(roundId, blockToPropose);
+    roles.getAllPeers().forEach(ValidatorPeer::clearReceivedMessages);
+
+    context.getController().handleRoundExpiry(new RoundExpiry(roundId));
+    assertPeersReceivedExactly(roles.getAllPeers(), expectedTxRoundChange);
+  }
+
+  @Test
+  public void roundChangeHasEmptyCertificateIfInsufficientPreparesAreReceived() {
+    // Note: There are 4 validators, thus Quorum is 3 and Prepare Msgs are 2 - thus
+    // receiving only a single Prepare msg will result in no PreparedCert.
+    final ConsensusRoundIdentifier targetRound = new ConsensusRoundIdentifier(1, 1);
+    final SignedData<RoundChangePayload> expectedTxRoundChange =
+        localNodeMessageFactory.createSignedRoundChangePayload(targetRound, empty());
+
+    roles.getProposer().injectProposal(roundId, blockToPropose);
+    roles.getNonProposingPeer(1).injectPrepare(roundId, blockToPropose.getHash());
+    roles.getAllPeers().forEach(ValidatorPeer::clearReceivedMessages);
+
+    context.getController().handleRoundExpiry(new RoundExpiry(roundId));
+    assertPeersReceivedExactly(roles.getAllPeers(), expectedTxRoundChange);
+  }
+
+  @Test
+  public void roundChangeHasPopulatedCertificateIfQuorumPrepareMessagesAndProposalAreReceived() {
+    final ConsensusRoundIdentifier targetRound = new ConsensusRoundIdentifier(1, 1);
+    final SignedData<PreparePayload> localPrepareMessage =
+        localNodeMessageFactory.createSignedPreparePayload(roundId, blockToPropose.getHash());
+
+    final SignedData<ProposalPayload> proposal =
+        roles.getProposer().injectProposal(roundId, blockToPropose);
+    roles.getAllPeers().forEach(ValidatorPeer::clearReceivedMessages);
+
+    final SignedData<PreparePayload> p1 =
+        roles.getNonProposingPeer(0).injectPrepare(roundId, blockToPropose.getHash());
+    roles.getAllPeers().forEach(ValidatorPeer::clearReceivedMessages);
+    final SignedData<PreparePayload> p2 =
+        roles.getNonProposingPeer(1).injectPrepare(roundId, blockToPropose.getHash());
+    roles.getAllPeers().forEach(ValidatorPeer::clearReceivedMessages);
+
+    final SignedData<RoundChangePayload> expectedTxRoundChange =
+        localNodeMessageFactory.createSignedRoundChangePayload(
+            targetRound,
+            Optional.of(
+                new PreparedCertificate(
+                    proposal, Lists.newArrayList(localPrepareMessage, p1, p2))));
+
+    context.getController().handleRoundExpiry(new RoundExpiry(roundId));
+    assertPeersReceivedExactly(roles.getAllPeers(), expectedTxRoundChange);
+  }
+
+  @Test
+  public void whenSufficientRoundChangeMessagesAreReceivedForNewRoundLocalNodeCreatesNewRoundMsg() {
+    // Note: Round-4 is the next round for which the local node is Proposer
+    final ConsensusRoundIdentifier targetRound = new ConsensusRoundIdentifier(1, 4);
+    final Block locallyProposedBlock =
+        context.createBlockForProposal(targetRound.getRoundNumber(), blockTimeStamp);
+
+    final SignedData<RoundChangePayload> rc1 =
+        roles.getNonProposingPeer(0).injectRoundChange(targetRound, empty());
+    final SignedData<RoundChangePayload> rc2 =
+        roles.getNonProposingPeer(1).injectRoundChange(targetRound, empty());
+    final SignedData<RoundChangePayload> rc3 =
+        roles.getNonProposingPeer(2).injectRoundChange(targetRound, empty());
+    final SignedData<RoundChangePayload> rc4 =
+        roles.getProposer().injectRoundChange(targetRound, empty());
+
+    final SignedData<NewRoundPayload> expectedNewRound =
+        localNodeMessageFactory.createSignedNewRoundPayload(
+            targetRound,
+            new RoundChangeCertificate(Lists.newArrayList(rc1, rc2, rc3, rc4)),
+            localNodeMessageFactory.createSignedProposalPayload(targetRound, locallyProposedBlock));
+
+    assertPeersReceivedExactly(roles.getAllPeers(), expectedNewRound);
+  }
+
+  @Test
+  public void newRoundMessageContainsBlockOnWhichPeerPrepared() {
+    final long ARBITRARY_BLOCKTIME = 1500;
+
+    final PreparedCertificate earlierPrepCert =
+        createValidPrepCert(
+            new ConsensusRoundIdentifier(1, 1),
+            context.createBlockForProposal(1, ARBITRARY_BLOCKTIME / 2));
+
+    final PreparedCertificate bestPrepCert =
+        createValidPrepCert(
+            new ConsensusRoundIdentifier(1, 2),
+            context.createBlockForProposal(2, ARBITRARY_BLOCKTIME));
+
+    final ConsensusRoundIdentifier targetRound = new ConsensusRoundIdentifier(1, 4);
+
+    final SignedData<RoundChangePayload> rc1 =
+        roles.getNonProposingPeer(0).injectRoundChange(targetRound, empty());
+
+    // Create a roundChange with a PreparedCertificate from an earlier Round (should not be used
+    final SignedData<RoundChangePayload> rc2 =
+        roles.getNonProposingPeer(1).injectRoundChange(targetRound, Optional.of(earlierPrepCert));
+
+    // Create a roundChange with a PreparedCertificate from an earlier Round (should not be used
+    final SignedData<RoundChangePayload> rc3 =
+        roles.getNonProposingPeer(2).injectRoundChange(targetRound, Optional.of(earlierPrepCert));
+
+    // Create a roundChange containing a PreparedCertificate
+    final SignedData<RoundChangePayload> rc4 =
+        roles.getProposer().injectRoundChange(targetRound, Optional.of(bestPrepCert));
+
+    // Expected to use the block with "ARBITRARY_BLOCKTIME" (i.e. latter block) but with the target
+    // round number.
+    final Block expectedBlockToPropose =
+        context.createBlockForProposal(targetRound.getRoundNumber(), ARBITRARY_BLOCKTIME);
+
+    final SignedData<NewRoundPayload> expectedNewRound =
+        localNodeMessageFactory.createSignedNewRoundPayload(
+            targetRound,
+            new RoundChangeCertificate(Lists.newArrayList(rc1, rc2, rc3, rc4)),
+            localNodeMessageFactory.createSignedProposalPayload(
+                targetRound, expectedBlockToPropose));
+
+    assertPeersReceivedExactly(roles.getAllPeers(), expectedNewRound);
+  }
+
+  @Test
+  public void cannotRoundChangeToAnEarlierRound() {
+    // Controller always starts at 1:0. This test moves to 1:7, then attempts to move back to 1:3.
+
+    final ConsensusRoundIdentifier futureRound = new ConsensusRoundIdentifier(1, 9);
+    final List<SignedData<RoundChangePayload>> roundChangeMessages = Lists.newArrayList();
+    for (final ValidatorPeer peer : roles.getAllPeers()) {
+      roundChangeMessages.add(peer.injectRoundChange(futureRound, empty()));
+    }
+
+    final ConsensusRoundIdentifier priorRound = new ConsensusRoundIdentifier(1, 4);
+    for (final ValidatorPeer peer : roles.getAllPeers()) {
+      peer.injectRoundChange(priorRound, empty());
+    }
+
+    final Block locallyProposedBlock =
+        context.createBlockForProposal(futureRound.getRoundNumber(), blockTimeStamp);
+
+    final SignedData<NewRoundPayload> expectedNewRound =
+        localNodeMessageFactory.createSignedNewRoundPayload(
+            futureRound,
+            new RoundChangeCertificate(roundChangeMessages),
+            localNodeMessageFactory.createSignedProposalPayload(futureRound, locallyProposedBlock));
+
+    assertPeersReceivedExactly(roles.getAllPeers(), expectedNewRound);
+  }
+
+  @Test
+  public void multipleRoundChangeMessagesFromSamePeerDoesNotTriggerRoundChange() {
+    // Note: Round-3 is the next round for which the local node is Proposer
+    final ConsensusRoundIdentifier targetRound = new ConsensusRoundIdentifier(1, 4);
+
+    final ValidatorPeer transmitter = roles.getNonProposingPeer(0);
+
+    for (int i = 0; i < IbftHelpers.calculateRequiredValidatorQuorum(NETWORK_SIZE); i++) {
+      transmitter.injectRoundChange(targetRound, empty());
+    }
+
+    assertPeersReceivedNoMessages(roles.getAllPeers());
+  }
+
+  @Test
+  public void subsequentRoundChangeMessagesFromPeerDoNotOverwritePriorMessage() {
+    final long ARBITRARY_BLOCKTIME = 1500;
+
+    final ConsensusRoundIdentifier targetRound = new ConsensusRoundIdentifier(1, 4);
+
+    final PreparedCertificate prepCert =
+        createValidPrepCert(
+            new ConsensusRoundIdentifier(1, 2),
+            context.createBlockForProposal(2, ARBITRARY_BLOCKTIME));
+
+    List<SignedData<RoundChangePayload>> roundChangeMessages = Lists.newArrayList();
+    // Create a roundChange containing a PreparedCertificate
+    roundChangeMessages.add(
+        roles.getProposer().injectRoundChange(targetRound, Optional.of(prepCert)));
+
+    // Attempt to override the previously received RoundChange (but now without a payload).
+    roles.getProposer().injectRoundChange(targetRound, empty());
+
+    roundChangeMessages.addAll(
+        roles
+            .getNonProposingPeers()
+            .stream()
+            .map(peer -> peer.injectRoundChange(targetRound, empty()))
+            .collect(Collectors.toList()));
+
+    final Block expectedBlockToPropose =
+        context.createBlockForProposal(targetRound.getRoundNumber(), ARBITRARY_BLOCKTIME);
+
+    final SignedData<NewRoundPayload> expectedNewRound =
+        localNodeMessageFactory.createSignedNewRoundPayload(
+            targetRound,
+            new RoundChangeCertificate(Lists.newArrayList(roundChangeMessages)),
+            localNodeMessageFactory.createSignedProposalPayload(
+                targetRound, expectedBlockToPropose));
+
+    assertPeersReceivedExactly(roles.getAllPeers(), expectedNewRound);
+  }
+
+  @Test
+  public void messagesFromPreviousRoundAreDiscardedOnTransitionToFutureRound() {
+    roles.getProposer().injectProposal(roundId, blockToPropose);
+
+    // timeout into next round
+    context.getController().handleRoundExpiry(new RoundExpiry(roundId));
+
+    // Clear prior Prepare msg and RoundChange message
+    roles.getAllPeers().forEach(ValidatorPeer::clearReceivedMessages);
+
+    // inject enough prepares from prior round to trigger a commit
+    roles
+        .getNonProposingPeers()
+        .forEach(peer -> peer.injectPrepare(roundId, blockToPropose.getHash()));
+
+    assertPeersReceivedNoMessages(roles.getAllPeers());
+  }
+
+  private PreparedCertificate createValidPrepCert(
+      final ConsensusRoundIdentifier preparedRound, final Block block) {
+    final RoundSpecificNodeRoles roles = context.getRoundSpecificRoles(preparedRound);
+
+    return new PreparedCertificate(
+        roles.getProposer().getMessageFactory().createSignedProposalPayload(preparedRound, block),
+        roles
+            .getNonProposingPeers()
+            .stream()
+            .map(
+                role ->
+                    role.getMessageFactory()
+                        .createSignedPreparePayload(preparedRound, block.getHash()))
+            .collect(Collectors.toList()));
+  }
+}

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

@@ -48,7 +48,7 @@ public class RoundChangeManager {
 
     // Store only 1 round change per round per validator
     @VisibleForTesting
-    final Map<Address, SignedData<RoundChangePayload>> receivedMessages = Maps.newHashMap();
+    final Map<Address, SignedData<RoundChangePayload>> receivedMessages = Maps.newLinkedHashMap();
 
     private boolean actioned = false;
 
@@ -58,7 +58,7 @@ public class RoundChangeManager {
 
     public void addMessage(final SignedData<RoundChangePayload> msg) {
       if (!actioned) {
-        receivedMessages.put(msg.getSender(), msg);
+        receivedMessages.putIfAbsent(msg.getSender(), msg);
       }
     }
 

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

@@ -42,8 +42,8 @@ public class RoundState {
 
   // Must track the actual Prepare message, not just the sender, as these may need to be reused
   // to send out in a PrepareCertificate.
-  private final Set<SignedData<PreparePayload>> preparePayloads = Sets.newHashSet();
-  private final Set<SignedData<CommitPayload>> commitPayloads = Sets.newHashSet();
+  private final Set<SignedData<PreparePayload>> preparePayloads = Sets.newLinkedHashSet();
+  private final Set<SignedData<CommitPayload>> commitPayloads = Sets.newLinkedHashSet();
 
   private boolean prepared = false;
   private boolean committed = false;